auth-backend: Encode `env` in the OAuth flow into the `state` parameter. Refs #1775 #1812

GoWind · 2020-08-03T16:22:28Z

env param in the OAuth request flow is now encoded in the state parameter.

The env parameter used to select the right EnvironmentHandlers is an explicit parameter that might break the flow for Users of backstage. To solve this issue, the env and the nonce parameter are encoded in the state parameter of the request.

Previously, the state parameter contained only the nonce used for preventing CSRF attacks. This change will create an object with the nonce and env as keys in it and URL encode the object before setting it as the value of the state parameter.

✔️ Checklist

All tests are passing yarn test
Screenshots attached (for UI changes)
Relevant documentation updated
Prettier run on changed files
Tests added for new functionality
Regression tests added for bug fixes

`Env` passed as a separate parameter in the request might break the flow of the authorization flow. Spotify considers the `env` as a first class abstraction and use it across their application stack. To ensure both the existing flow and newer flows are supported, use the `state` parameter to encode an object, consisting of the `nonce` and the `env` which is then passed to the authorization server. This parameter is returned to the application in the callbackURL

The `state` was modified to be an encoded object of `nonce` and `env`. When verifying the Nonce value in the callback from the authorization server, parse the state parameter string to read the right value of nonce

Update OAuthProvider tests to ensure that changes to nonce and environment handling do not cause regressions

freben · 2020-08-03T17:37:33Z

There are a number of linting errors, you may want to set up your editor to run prettier automatically.

freben · 2020-08-03T17:37:59Z

plugins/auth-backend/src/lib/EnvironmentHandler.ts

+  /* Return the value of `env` key, if it is exists, encoded within
+     the `state` parameter in the request
+  */
+  private getEnv(stateParams: Array<string>): string {


Will remove it

freben · 2020-08-03T17:43:56Z

plugins/auth-backend/src/lib/OAuthProvider.ts

+/* Return the value of `env` key, if it is exists, encoded within
+     the `state` parameter in the request
+  */
+const readStateParameter = (


Not sure about all of this manual string fiddling - can't the state be base64 of the json encoded state object or something, and make a proper Typescript type for the state so we can use it more cleanly?

Yes, I think turning the state into a type makes sense.
I admit that the serializing is messy. I am just getting started with Typescript and my code wont be idiomatic. Will try to find a simple way to serialize and deserialize it.

freben · 2020-08-03T17:52:42Z

plugins/auth-backend/src/lib/OAuthProvider.ts

 export const verifyNonce = (req: express.Request, providerId: string) => {
  const cookieNonce = req.cookies[`${providerId}-nonce`];
-  const stateNonce = req.query.state;
+  const state: Array<string> = readState(req.query.state?.toString() ?? '');


Again, I feel this should just be a proper object type with named fields. Also is the toString necessary here?

GoWind · 2020-08-04T10:42:01Z

@freben : I noticed that the build failed due to a linting error > 110 | identifyEnv(_req: express.Request): string { . The function is basically a NOP and I prefix-ed the variable with an _ assuming that this would signal the linter that the variable is unused (after noticing the same pattern in some other part of the code). Do you have any recommendations for doing the same that is also acceptable to the linter ?

Create a new `OAuthState` type for storing the fields we want in the `state` parameter. Update the `encodeState` and `readState` fns to use the new `OAuthState` type. Update tests to reflect the new type

GoWind · 2020-08-04T15:35:06Z

@freben , I fixed the lint/type check errors and refactored the code. Can you take a look at the updated changes ?

Rugvip

Thanks! 😁

Rugvip · 2020-08-04T16:09:41Z

plugins/auth-backend/src/lib/OAuthProvider.ts

+};
+
+export const encodeState = (state: OAuthState): string => {
+  return encodeURIComponent(JSON.stringify(state));


Tbh I prefer query param encoding twice over JSON. The JSON delimiters end up really ugly and take a lot of space. It would actually be more compact to url-safe base64 encode this.

e.g.

const encoded = encodeURIComponent(new URLSearchParams(state).toString()); const decoded = Object.fromEntries(new URLSearchParams(decodeURIComponent(encoded)));

For comparison it's

env%3Ddevelopment%26nonce%3D45c8g7yn452v4v

vs

%7B%22env%22%3A%22development%22%2C%22nonce%22%3A%2245c8g7yn452v4v%22%7D

👍 for this suggestion. Will update it.

Rugvip · 2020-08-04T16:12:56Z

plugins/auth-backend/src/providers/types.ts

+export type OAuthState = {
+  /* A type for the serialized value in the `state` parameter of the OAuth authorization flow
+   */
+  nonce?: string;


👍 for adding this type. I think these should both be required right, i.e. no ??

IIRC, I did this because the type-checker wasn't allowing me to set nonce and env to null if the de-serialization fails. I then check if these fields are empty when using the OAuthState. While nonce is mandatory, I think env can be optional should other users of backstage have a different way of identifying the env (or completely ignoring it).
How would you recommend having a filed that could be empty/null ? is a string | null type better instead of making a field optional ?

With them required you can reject the request early if they're not set, and then the rest of the code can be sure that they're there. I see the case for env being optional, but since we don't have that atm, perhaps we could keep it required for now at least?

Regarding making things optional this is the way to go usually. Don't use null 😁.

Also , there seems to be an issue with Error handling. If nonce or env were null and an Exception were thrown during parsing, the rest of the code was continued to be called instead of aborting the request processing flow.

So the error handling might look a bit weird in the frame handler endpoint. For the error to reach the user we actually still need to return a 200 response with a script that posts to the parent frame, but the posted payload is the error instead of an auth response.

Understood. I removed the optional fields so that the values are no longer null. Please let review the changes again :)

Rugvip

A bunch of comments around the usage of empty strings 😅 . So by using empty strings we essentially opt-out of type checking, since it's not a correct value at runtime, but it conforms to the type string. We want to really avoid ever assigning an empty string when working with the nonce and env, as those should always be defined.

The other aspect of it is to fail as early as possible if something's wrong. It makes it easier to debug issues and likely wastes less time of the user 😁. An example is allowing an empty env param in the initial auth request but then failing in the auth handler. At that point the user has already gone through the process of signing in, when we could've just thrown an error in the initial request.

Rugvip · 2020-08-05T10:13:48Z

plugins/auth-backend/src/lib/OAuthProvider.ts

+      new URLSearchParams(decodeURIComponent(stateString)),
+    );
+    return {
+      nonce: state.nonce ?? '',


Best not assign any empty strings here, it's essentially makes these optional values but without type checking.

I think it makes sense to check if both state.nonce and state.env are present and non-empty strings here, and throw appropriate errors otherwise.

Rugvip · 2020-08-05T10:14:38Z

plugins/auth-backend/src/lib/OAuthProvider.ts


  if (!cookieNonce) {
    throw new Error('Auth response is missing cookie nonce');
  }
-  if (!stateNonce) {
+  if (stateNonce.length === 0) {
    throw new Error('Auth response is missing state nonce');


This error could for example be moved in and thrown from the readState function instead.

Done. I moved the empty string check into the readState function.

Rugvip · 2020-08-05T10:34:09Z

plugins/auth-backend/src/lib/OAuthProvider.ts

@@ -103,6 +130,7 @@ export class OAuthProvider implements AuthProviderRouteHandlers {
  async start(req: express.Request, res: express.Response): Promise<void> {
    // retrieve scopes from request
    const scope = req.query.scope?.toString() ?? '';
+    const env = req.query.env?.toString() ?? '';


Same goes for this. If no env is set we want to fail early and throw an InputError

start now throws an Error if env is empty or missing.

Rugvip · 2020-08-05T10:35:17Z

plugins/auth-backend/src/lib/OAuthProvider.ts

+      return '';
+    }
+    const env = readState(stateParams).env;
+    return env ?? '';


Best to return undefined here and above as well

Rugvip · 2020-08-05T10:35:32Z

plugins/auth-backend/src/providers/saml/provider.ts

@@ -106,6 +106,10 @@ export class SamlAuthProvider implements AuthProviderRouteHandlers {
  async logout(_req: express.Request, res: express.Response): Promise<void> {
    res.send('noop');
  }
+
+  identifyEnv(): string {
+    return '';


undefined here too

Type fn that identifies the env (environment) of a Request Fail early if `env` is not present in the request or has an invalid `env` in the state

Rugvip

Looks good, thanks again! 👍

Rugvip · 2020-08-06T10:31:02Z

plugins/auth-backend/src/providers/oauth2/provider.ts

@@ -153,7 +153,7 @@ export function createOAuth2Provider(
  const appOrigin = envConfig.getString('appOrigin');
  const clientID = envConfig.getString('clientId');
  const clientSecret = envConfig.getString('clientSecret');
-  const callbackURL = `${baseUrl}/${providerId}/handler/frame?env=${env}`;


Realized this only removes the param from okta, I think we can remove it from all the OAuth providers?

I didn't notice it either. Yes, will remove the env parameter from the callbackURL of other providers also.

Rugvip · 2020-08-06T10:39:50Z

Just tried this out since I wasn't sure if it's a breaking change or not. It definitely is for Google Auth, and probably others too. Depends on how picky they are about the redirect URL.

Was looking into methods to display a clear error message but we're not really in the request path when the error happens. Usually providers display good error messages, but there might be some confusion about why the current configuration is not working all of a sudden.

I'll look into getting some kind of basic changelog going so we can highlight some of these breaking changes.

providers

GoWind · 2020-08-06T11:05:59Z

@Rugvip 👍 on checking with Google.

I'll look into getting some kind of basic changelog going so we can highlight some of these breaking changes.

Sounds great !
Meanwhile, I will try to use this opportunity to see if I can contribute documentation, perhaps a Documentation PR , explaining in little more detail how the state is encoded and the entire structure of the Authentication Providers and classes.

Rugvip · 2020-08-06T11:10:28Z

Changelog PR here: #1845, we can add an entry after this is merged though

Rugvip

Let's , followup doc PR would be awesome tho!

Govindarajan Nagarajan added 4 commits August 3, 2020 15:33

Refactor: parse state query param to read nonce

081fe60

The `state` was modified to be an encoded object of `nonce` and `env`. When verifying the Nonce value in the callback from the authorization server, parse the state parameter string to read the right value of nonce

Refactor: move encoding state parameters into a separate function

3e5dfbc

Tests: update tests for OAuthProvider to handle

51fad03

Update OAuthProvider tests to ensure that changes to nonce and environment handling do not cause regressions

GoWind requested a review from a team as a code owner August 3, 2020 16:22

freben reviewed Aug 3, 2020

View reviewed changes

GoWind force-pushed the abstract_env branch 2 times, most recently from 445c106 to e02d2a9 Compare August 4, 2020 14:24

Refactor: Address PR Comments

2d20cf3

Create a new `OAuthState` type for storing the fields we want in the `state` parameter. Update the `encodeState` and `readState` fns to use the new `OAuthState` type. Update tests to reflect the new type

GoWind force-pushed the abstract_env branch from e02d2a9 to 2d20cf3 Compare August 4, 2020 15:07

Rugvip reviewed Aug 4, 2020

View reviewed changes

Govindarajan Nagarajan added 3 commits August 4, 2020 18:42

Refactor: encode state Parameters as URLSearchParams instead of JSON

cb80b2b

Refactor: Make properties of OAuthState non-optional

4ae351b

Cleanup: Remove deadcode

75467ec

Rugvip reviewed Aug 5, 2020

View reviewed changes

Address PR comments: Use undefined instead of passing Empty strings

5f06a08

Type fn that identifies the env (environment) of a Request Fail early if `env` is not present in the request or has an invalid `env` in the state

Rugvip approved these changes Aug 6, 2020

View reviewed changes

Rugvip reviewed Aug 6, 2020

View reviewed changes

Refactor: remove env parameter from callbackURL of all OAuth based

959cc6f

providers

Rugvip approved these changes Aug 6, 2020

View reviewed changes

Rugvip merged commit 8bfba79 into backstage:master Aug 6, 2020

Rugvip mentioned this pull request Aug 10, 2020

Use OAuth state to forward the environment to the handler #1775

Closed

auth-backend: Encode env in the OAuth flow into the state parameter. Refs #1775 #1812

auth-backend: Encode env in the OAuth flow into the state parameter. Refs #1775 #1812

Conversation

GoWind commented Aug 3, 2020

✔️ Checklist

freben commented Aug 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GoWind commented Aug 4, 2020

GoWind commented Aug 4, 2020

Rugvip left a comment

Choose a reason for hiding this comment

Rugvip Aug 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rugvip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rugvip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rugvip commented Aug 6, 2020

GoWind commented Aug 6, 2020 • edited

Rugvip commented Aug 6, 2020

Rugvip left a comment

Choose a reason for hiding this comment

auth-backend: Encode `env` in the OAuth flow into the `state` parameter. Refs #1775 #1812

auth-backend: Encode `env` in the OAuth flow into the `state` parameter. Refs #1775 #1812

Rugvip Aug 4, 2020 •

edited

GoWind commented Aug 6, 2020 •

edited