Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move login flow to accounts website. #4964

Conversation

johnwchadwick
Copy link
Contributor

@johnwchadwick johnwchadwick commented Jul 13, 2022

UI

The UI from the application side looks like this.

Login Modal

Initial State

User action Session expiration
screenshot of the login modal when it is first opened by manual action; copy indicates browser has already opened screenshot of the login modal when it is opened automatically; copy indicates that you must click a button to open the browser

Enter Manual Token

login modal displaying form to manually enter token

Rationale

Our intent with this PR is to unify the SRP handshake and key derivation in one place. To spell it out explicitly, the intent of this PR is not to obfuscate the login process from the public, but just to make the app more flexible so that we can implement additional features to login without needing to duplicate it between the app and the accounts website.

Security

The security model of this new login process has some implications. The general flow works like this:

  1. On app startup, an X25519 keypair is generated for login delegation purposes.
  2. When the login modal is opened, a new URL is opened linking into the accounts website, containing the public key.
  3. The login process is continued in the browser, including SRP and key derivation. The accounts website always requires a password, even if the user is already logged in.
  4. The accounts website running in the browser invokes an insomnia:// deep-link containing a sealed box with the derived key and session token.
  5. The app grabs the account keys and uses the derived key to decrypt them.

Though this removes both SRP and key derivation from the app side of the login flow, some code in the app still needs to deal with SRP and the key derivation process for the time being; this will be dealt with in the future, allowing us to remove a significant amount of code and some dependencies. For now, the remainder of session code is unchanged.

There are a few assumptions:

  1. The end user device is trusted and not compromised; if it were not trusted, the current login flow would still be insecure, as it could be defeated with a simple key logger.
  2. The app is not compromised; if the app were compromised, the existing login flow would also be unsafe, since either the raw credentials or the long-term session could be stolen.
  3. The browser is trusted and not compromised; This is a new assumption from this PR. However, if the browser was compromised, many other account-related actions would be insecure, as they can only be performed in the browser.

This login flow is assumed to be secure with those assumptions.

Here are some example scenarios:

  • Phishing/token theft: An attacker (e.g. running in another browser tab) could try to start the app authentication flow and get the token from it somehow. The following mitigations apply:
    • Deep links: An attacker would need to register the insomnia:// protocol to be able to steal the tokens directly out of the automatic flow. This is unlikely in cases where the computer is not compromised. (They could still try to convince the user to copy and paste the token into an attacker-controlled input.)
    • Password requirement: Other security issues require the user to explicitly input their password to finish the login process. Because of this, the login flow itself could never facilitate an automated attack, even in the case where the computer is compromised.
    • We could also improve this by implementing the login token as a one-time exchangeable token where the API call to exchange the token is inaccessible via browsers as a result of Same-Origin policy. This would increase the complexity of a phishing attack and make it easier to detect.
  • Keymatter/ciphertext leakage: An attacker could try to intercept the public key or authentication box somehow.
    • The public key and cipher text are both useless without the private key, which never leaves the app's memory. Assuming the app is not compromised, this flow is already very secure in this regard.
    • We could still improve it by implementing the one-time token mechanism discussed above: then, the session token in the box would become useless after it is used.
  • Accounts website XSS: An attacker could gain (possibly persistent) code execution on the accounts website, allowing the attacker to directly exercise code in the context of app authentication.
    • Due to the requirement to enter a password to create the app session, we are happily not introducing additional risk here.
    • If an attacker has code execution on the accounts website, they can potentially steal the derived key, OR they can potentially read the user's credentials as they are typing them. With regards to app authentication, while the attacker could now steal a long-term token out of the app auth flow, this is effectively not different from stealing the momentarily entered credentials/passphrase. And since the app authentication requires the credentials on the API side, there's no additional risk here.
    • Again, this can be improved by implementing a one-time token exchange mechanism, as it would make it impossible to get the token from the browser context alone.

changelog(Improvements): Login flow is now done using the app.insomnia.rest website

@@ -4,7 +4,7 @@
<meta charset="utf-8" />
<meta
http-equiv="Content-Security-Policy"
content="font-src 'self' data:; default-src * insomnia://*; img-src blob: data: * insomnia://*; script-src 'self' 'unsafe-eval'; style-src 'self' 'unsafe-inline'; media-src blob: data: mediastream: * insomnia://*;"
content="font-src 'self' data:; connect-src * data:; default-src * insomnia://*; img-src blob: data: * insomnia://*; script-src 'self' 'unsafe-eval'; style-src 'self' 'unsafe-inline'; media-src blob: data: mediastream: * insomnia://*;"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This is to allow for fast ArrayBuffer base64 computation via fetch and blob. It is a bit of a hack, but the approach is very fast in both Electron and the browser. I sincerely believe there is no security implication of adding data: to connect-src`, but I am not 100% certain of this.

@dimitropoulos

This comment was marked as resolved.

@johnwchadwick

This comment was marked as off-topic.

@DMarby DMarby self-requested a review July 19, 2022 15:36
@johnwchadwick johnwchadwick mentioned this pull request Jul 21, 2022
@johnwchadwick johnwchadwick force-pushed the feature/ins-1562-move-srp-authentication-from-the-app-to branch 2 times, most recently from d8a0e7a to 139c295 Compare July 26, 2022 19:26
@johnwchadwick johnwchadwick force-pushed the feature/ins-1562-move-srp-authentication-from-the-app-to branch from 139c295 to bc4dd93 Compare July 26, 2022 20:57
@johnwchadwick johnwchadwick marked this pull request as ready for review July 26, 2022 20:57
@wdawson
Copy link
Contributor

wdawson commented Jul 27, 2022

Does the session expiration flow still say "...continue using Insomnia Plus"? If so, "Insomnia Plus" isn't a thing anymore so we should figure out different language.

@johnwchadwick
Copy link
Contributor Author

Does the session expiration flow still say "...continue using Insomnia Plus"? If so, "Insomnia Plus" isn't a thing anymore so we should figure out different language.

This is from the API, not this PR, so it will be fixed whenever the change is deployed in the backend.

Copy link
Contributor

@wdawson wdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@johnwchadwick johnwchadwick force-pushed the feature/ins-1562-move-srp-authentication-from-the-app-to branch from bc4dd93 to 2220807 Compare July 27, 2022 16:07
@DMarby DMarby force-pushed the feature/ins-1562-move-srp-authentication-from-the-app-to branch from 5f8d4aa to 6d0cd96 Compare July 27, 2022 18:19
@johnwchadwick
Copy link
Contributor Author

OK, I could probably get more reviews to be safe, but I actually feel pretty confident with the state we got it in, so I'm going to just go for the merge. Thanks everyone.

@johnwchadwick johnwchadwick merged commit 585ad9e into Kong:develop Jul 28, 2022
@johnwchadwick johnwchadwick deleted the feature/ins-1562-move-srp-authentication-from-the-app-to branch July 28, 2022 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants