-
Notifications
You must be signed in to change notification settings - Fork 5
add google auth to the internal/staff only docs endpoints #3141
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
base: jd/discount-eligibility-list
Are you sure you want to change the base?
add google auth to the internal/staff only docs endpoints #3141
Conversation
# Conflicts: # cdk/lib/__snapshots__/discount-api.test.ts.snap # cdk/lib/cdk/policies.ts # cdk/lib/cdk/sr-api-lambda.ts # cdk/lib/discount-api.ts # update-stack.sh
| }, | ||
| ":userpool/", | ||
| { | ||
| "Fn::ImportValue": "UserPoolId-CODE", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the user pool id is exported from the staff-access stack - it must be deployed first for this to successfully deploy
| }, | ||
| "HttpMethod": "GET", | ||
| "Integration": { | ||
| "IntegrationHttpMethod": "POST", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cdk/lib/cdk/SrApiLambda.ts
Outdated
| /** | ||
| * Set this to false to skip adding the main proxy endpoint | ||
| */ | ||
| proxy?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we add the base endpoint {proxy+} then it conflicts with the {app}/{proxy+} one I add manually later. TODO Perhaps I should just use that rather than trying to add an exception here
…ity-list-auth # Conflicts: # cdk/lib/cdk/SrApiLambda.ts # cdk/lib/cdk/sr-api-lambda.ts
4ff0ec3 to
fc5a0fe
Compare
| }, | ||
| ], | ||
| "RestApiId": { | ||
| "Ref": "discountapilambdadiscountapiCODEE1E5C869", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the authoriser has to be in the same stack as the RestApi because it references the RestApi directly.
cdk/lib/discount-api.ts
Outdated
| const userPoolId = Fn.importValue(`UserPoolId-${stage}`); | ||
|
|
||
| const userPool = UserPool.fromUserPoolId( | ||
| this, | ||
| 'ImportedUserPool', | ||
| userPoolId, | ||
| ); | ||
|
|
||
| const cognitoAuthorizer = new CognitoUserPoolsAuthorizer( | ||
| this, | ||
| 'CognitoAuthorizer', | ||
| { | ||
| cognitoUserPools: [userPool], | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo this can be hidden in the SrApiLambda
| // CFN doesn't support secret values https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/dynamic-references-ssm-secure-strings.html#template-parameters-dynamic-patterns-resources | ||
| // and using AwsCustomResource to read it also doesn't work as it seems to not find the built in lambda to implement it | ||
| clientSecretValue: SecretValue.unsafePlainText(googleOAuthClientSecret), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
although this google auth is only protecting private information rather than secrets or PII, it would still be nice to protect the client secret according to prevailing security standards
| errorImpact: | ||
| 'staff are getting errors when viewing docs served by our API layer', | ||
| }, | ||
| isPublic: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the staff-access http server is public, as all auth is enforced by the downstream lambdas
| path?: z.Schema<TPath, ZodTypeDef, unknown>; | ||
| body?: z.Schema<TBody, ZodTypeDef, unknown>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this allows us to use .transform on the schema, i.e. we can have a different output type to the input type.
| }, | ||
| "Type": "COGNITO_USER_POOLS", | ||
| }, | ||
| "Type": "AWS::ApiGateway::Authorizer", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
each secured lambda needs to have an authoriser that points to the user pool that's exported from the staff-access stack, this is so it can check bearer tokens
It has to be in this stack as it references the RestApi directly - only the User pool etc can be shared.
| "UserPoolId": { | ||
| "Export": { | ||
| "Name": "UserPoolId-CODE", | ||
| }, | ||
| "Value": { | ||
| "Ref": "UserPool6BA7E5F2", | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
user pool is exported (separately for CODE and PROD) so api gateway can check the tokens
cdk/lib/cdk/SrAppConfigKey.ts
Outdated
| export function buildAppConfigKey(scope: SrStack, configKey: string) { | ||
| return ( | ||
| '/' + [scope.stage, scope.stack, scope.app].join('/') + '/' + configKey | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extract for reusability
| export function domainForStack(scope: SrStack, suffixProdDomain?: boolean) { | ||
| const isProd = scope.stage === 'PROD'; | ||
| const cert = certForStack[scope.stack]; | ||
| const domainName = `${scope.app}${isProd && !suffixProdDomain ? '' : '-' + scope.stage.toLowerCase()}.${cert.domainName}`; | ||
| return { cert, domainName }; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extract for reuse to calculate the allowed cognito redirect url
produces something like discount-api-code.support.guardianapis.com as well as the necessary cert info
| } | ||
| } | ||
|
|
||
| addPublicPath(path: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replaced addPublicPath as we don't need it any more, but we can bring it back if we genuinely want some public paths on a private lambda in future.
| ## TODO (in later PRs) | ||
| - consider to add a neater fastly domain https://staff-access.guardianapis.com/discount-api/docs | ||
| - some kind of index so we can find all the endpoints | ||
| - make the google client secret a securestring if possible (not easy as it's needed in the CFN) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried for a while to get it working, it seems like Aws Resource type should be able to do it, but when synthing the cdk, it produces something that needs bucket/key parameters for a lambda, which then fails to deploy as I don't know what value it needs.
I found something to do with cdk bootstrap that we should be doing, but I couldn't run it because of drift in the stack, I suspect we deleted stuff that was unused. I wasn't sure if I was too far off piste by that point - might well need Devx input to resolve properly.
| targetApp: z.string().regex(/^[a-z-]{1,20}$/), // only accept reasonable values for the lambda name | ||
| targetPath: z.string().regex(/^[/a-zA-Z0-9]{1,20}$/), // only really boring paths allowed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been as strict as possible with these, as I don't want any risk of people putting in strange characters and hitting things they shouldn't. It's possible we might need to slacken them off, especially if we want url parameters to work (correctly escaped!)
81a1384 to
43dd9c2
Compare
43dd9c2 to
863d237
Compare
…ity-list-auth # Conflicts: # modules/aws/src/appConfig.ts # modules/zuora/src/zuoraClient.ts
f3afc3c to
61e41a7
Compare
modules/routing/src/logger.ts
Outdated
| } | ||
| if (value instanceof Error) { | ||
| return value.stack ?? ''; | ||
| return (value.stack ?? '') + '\n' + this.objectToPrettyString(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stack returns the message and stack trace, but any other properties are not shown.
This change will log any other properties, which is useful for HTTP errors where most information is in other properties.
modules/routing/src/router.ts
Outdated
| const lastRoutePart = routeParts[routeParts.length - 1]!; | ||
| const routeIsGreedy = lastRoutePart.endsWith('+}'); | ||
| let adjustedEventParts: string[]; | ||
| if (routeIsGreedy && routeParts.length < eventParts.length) { | ||
| const excessParts = eventParts.slice(routeParts.length - 1); | ||
| const joinedGreedyValue = excessParts.join('/'); | ||
| adjustedEventParts = [ | ||
| ...eventParts.slice(0, routeParts.length - 1), | ||
| joinedGreedyValue, | ||
| ]; | ||
| } else { | ||
| adjustedEventParts = eventParts; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this lets us use + at the end of a pattern to make it greedy. This is the same as what API gateway does.
So if I have {app}/{proxy+} and the input is cheese/multi/part/path then it will make app=cheese and proxy=multi/part/path
…t-auth # Conflicts: # cdk/lib/discount-api.ts
…ity-list-auth # Conflicts: # BUILDCHECK.md # buildcheck/data/build.ts

To merge into #3134 before it can go live
In the above PR #3134 I added a docs endpoint to serve up some internal information about discounts. We would like to make it less easy to access, and it would be a good idea in general if we could protect endpoints for staff only (including ones that may have side effects)
Google auth is an established system that would let us know that people are internal to the guardian, so it would be ideal to protect endpoints like the above.
This PR adds google auth to the public docs endpoint.
It does that by
See inline comments.
TODO before going live (also see longer term things in readme.md)