Open mmathys opened 8 years ago
Please say I'm missing something
ASAR archives are simply a concatenation of source files... it is easy to extract any original plain text. Still, OAuth can be used also in JavaScript-centric applications.
@marcoancona but this package doesn't follow those guidelines from google, right?
I am not sure, I am not an expert of OAuth nor of this library. What do you think it's not matching the guideline?
@mmathys you can use it for electron for example, that way you can hide the secret
@AlexandreKilian do you store the secret inside the electron application source code?
Having the secret inside of the client is a huge no no, this library should either be using the implicit grant flow or it should be retrieving a code which get's sent up to an intermediate api which then exchanges it for an access token using the secret.
I'd recommend removing the function which can exchange the code for a token as the secret should never be inside of the client.
The authorization_code flow is the correct decision, but because the client is a public client, you should also implement the Proof Key for Code Exchange (PKCE)
Here's the RFC called OAuth 2.0 for Native Apps
that describes in details how it should work https://tools.ietf.org/html/rfc8252
This is also what google recommends - https://developers.google.com/identity/protocols/OAuth2InstalledApp
After researching OAuth solutions for Electron apps for several weeks and trying to determine if this library was safe for production, I don't think it is.
+1 for what @mcastany said. RFC 8252 (also known as BCP 212) is where the industry seems to be going. That doc, along with Google's recommendation for Native Apps, are very good explanations of all the intricacies. I would add to @mcastany's comment that you should not use a secret with Google or other services.
From RFC 8252, Section 8.5:
Secrets that are statically included as part of an app distributed to multiple users should not be treated as confidential secrets, as one user may inspect their copy and learn the shared secret. For this reason, and those stated in Section 5.3.1 of [RFC6819], it is NOT RECOMMENDED for authorization servers to require client authentication of public native apps clients using a shared secret, as this serves little value beyond client identification which is already provided by the "client_id" request parameter.
Authorization servers that still require a statically included shared secret for native app clients MUST treat the client as a public client (as defined by Section 2.1 of OAuth 2.0 [RFC6749]), and not accept the secret as proof of the client's identity. Without additional measures, such clients are subject to client impersonation (see Section 8.6).
google-auth-library-nodejs recommends using the iOS setting in the Google API Console so the server doesn't require a secret:
If you're authenticating with OAuth2 from an installed application (like Electron), you may not want to embed your client_secret inside of the application sources. To work around this restriction, you can choose the iOS application type when creating your OAuth2 credentials in the Google Developers console
Including a client ID and secret in your Electron app makes it trivial for other developers/hackers to impersonate your app. It's not as simple as "use the authorization code flow", because the server-side way of using the authentication code flow and the client-side (i.e. Electron) way of using authorization code flow have different security vulnerabilities (i.e. in Electron a secret cannot be kept!). The way around this is using a loopback IP address as the redirect URL, along with a few other tricks (PKCE, state parameter) to ensure that only your app can receive authentication responses. RFC 8252 is actually a very simple read and if you're doing anything in this ballpark is worth your 20 minutes.
From working on OAuth in Electron for several weeks, I believe these practices should be followed:
state
parameter when the OAuth server supports it to prevent a Cross-App Request Forgery attack (https://tools.ietf.org/html/rfc8252#section-8.9, supported by Google)state
parameter, do PKCE, etc.Basically, setting up OAuth with Electron the right way is pretty involved, and not something that is easy to provide out-of-the-box. This repo no longer supports the best practices.
I haven't had time to mess with it, but AppAuth is an effort to implement these patterns in iOS, macOS, Android, and JavaScript. AppAuth-JS looks very promising as a successor to this project. An AppAuth-Electron build on top of AppAuth-JS would be very interesting.
@mawie81 At this point, given that there are official IETF recommendations on how to do OAuth in Native Apps, I'd recommend that a very large, prominent disclaimer is put at the top of the README saying:
It's important to protect the reputation of Electron and the Electron community by ensuring that the highest number of apps possible support security best practices, especially when it comes to authenticating with 3rd party services.
@aguynamedben, thanks for the detailed analysis. I wonder, however, how robust the loopback IP approach is, when there is a system or network firewall in place, as within the network of most companies. Also, what if the port declared in the Redirect URL is already in use in the system?
Thank you @aguynamedben for looking so deeply into this. I agree that it is not a good idea to put secrets into your electron apps used in production. I therefore added the proposed disclaimer.
To make it even more clear I'll look into deprecating the package on npm and will most likely put the whole repo into read-only mode in a couple of days.
Ones again thanks for your support and putting time into this.
@marcoancona For what's it's worth, there are some notes about the loopback IP and firewalls in RFC 8252, Section 8.3:
8.3. Loopback Redirect Considerations
Loopback interface redirect URIs use the "http" scheme (i.e., without Transport Layer Security (TLS)). This is acceptable for loopback interface redirect URIs as the HTTP request never leaves the device.
Clients should open the network port only when starting the authorization request and close it once the response is returned.
Clients should listen on the loopback network interface only, in order to avoid interference by other network actors.
While redirect URIs using localhost (i.e., "http://localhost:{port}/{path}") function similarly to loopback IP redirects described in Section 7.3, the use of localhost is NOT RECOMMENDED. Specifying a redirect URI with the loopback IP literal rather than localhost avoids inadvertently listening on network interfaces other than the loopback interface. It is also less susceptible to client-side firewalls and misconfigured host name resolution on the user's device.
So it's recommended to use 127.0.0.1, not localhost. I have not yet run into any issues using 127.0.0.1 aside from it's a little awkward looking for a user to land on a 127.0.0.1 address in their web browser after they OAuth. From there, I use JavaScript to open a deep-linked URL (i.e. myapp://oauth/finished) that opens my app again (see app.setAsDefaultProtocolClient()).
@mawie81 Thanks for doing that, and thanks for the effort you took in originally releasing this helpful code. It helped me get my project off the ground very quickly going down the OAuth rabbit hole until that was appropriate. 🙏
@aguynamedben appreciate the research, @mawie81 submitted it to nodesecurity.io
Why would you authenticate a public client with OAuth?
You have nowhere to hide the client secret so in the end everyone has access to all the data of all the users.