-
Notifications
You must be signed in to change notification settings - Fork 82
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
Reauth workflows #787
Reauth workflows #787
Conversation
…748) ### Feature or Bugfix <!-- please choose --> - Feature Enhancement ### Detail - Adding additional error messages for KMS Key lookup when importing a new dataset - 1 Error message to determine if the KMS Key Alias Exists - 1 Error message to determine if the PivotRole has permissions to describe the KMS Key ### Relates - #712 ### Security Please answer the questions below briefly where applicable, or write `N/A`. Based on [OWASP 10](https://owasp.org/Top10/en/). - Does this PR introduce or modify any input fields or queries - this includes fetching data from storage outside the application (e.g. a database, an S3 bucket)? - Is the input sanitized? - What precautions are you taking before deserializing the data you consume? - Is injection prevented by parametrizing queries? - Have you ensured no `eval` or similar functions are used? - Does this PR introduce any functionality or component that requires authorization? - How have you ensured it respects the existing AuthN/AuthZ mechanisms? - Are you logging failed auth attempts? - Are you using or adding any cryptographic features? - Do you use a standard proven implementations? - Are the used keys controlled by the customer? Where are they stored? - Are you introducing any new policies/roles/users? - Have you used the least-privilege principle? How? By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
### Feature or Bugfix <!-- please choose --> - NA ### Detail - Get latest code in `main` to `v2m1m0` branch to keep in sync ### Relates - NA ### Security Please answer the questions below briefly where applicable, or write `N/A`. Based on [OWASP 10](https://owasp.org/Top10/en/). NA ``` - Does this PR introduce or modify any input fields or queries - this includes fetching data from storage outside the application (e.g. a database, an S3 bucket)? - Is the input sanitized? - What precautions are you taking before deserializing the data you consume? - Is injection prevented by parametrizing queries? - Have you ensured no `eval` or similar functions are used? - Does this PR introduce any functionality or component that requires authorization? - How have you ensured it respects the existing AuthN/AuthZ mechanisms? - Are you logging failed auth attempts? - Are you using or adding any cryptographic features? - Do you use a standard proven implementations? - Are the used keys controlled by the customer? Where are they stored? - Are you introducing any new policies/roles/users? - Have you used the least-privilege principle? How? ``` By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dlpzx <71252798+dlpzx@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: jaidisido <jaidisido@gmail.com> Co-authored-by: dlpzx <dlpzx@amazon.com> Co-authored-by: mourya-33 <134511711+mourya-33@users.noreply.github.com>
Additional Considerations/Requirements
|
In the latest commits I have added the ability to retry the API Request that required ReAuth upon a User re-logging in to the data.all UI. For retry logic, the reauth operation will be retried if:
For queries, the user will also be redirected to the page they were formerly on. For mutations this redirection is not built in as it may not make sense for most mutations (i.e. take the example of a user deleting a dataset… upon re-auth and the retry of the Testing in AWS Deployment
|
@@ -47,6 +47,7 @@ def __init__( | |||
prod_sizing=False, | |||
user_pool=None, | |||
pivot_role_name=None, | |||
reauth_ttl=5, |
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.
In the cdk.json it will be an int right? Then we will define it in the env variables as string and then back to int and datetime delta in api_handler
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.
Yes - environment variables are handled as a string so casting it as such before passing in to lambda and then casting back during runtime of the function.
I thought it would be confusing for a user to enter a string format of a numeric TTL value so thought we could just handle casting to string and then back as int - let me know your thoughts
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 agree! In the cdk.template can you add the info that it is INT and in minutes?
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.
LGTM!
@dbalintx and I tested in existing deployment with internet_facing=true and custom_domain. The only remark is that it is relatively easy to make mistakes when defining the reauth_apis. To use this feature we need to make it very clear in the docs that the query is the one defined in the gql schema and how to obtain it. But overall, incredible work @noah-paige |
Feature or Bugfix
Detail
Add Configurable feature to have sensitive API Operations require re-auth before proceeding
For specific sensitive actions, such as destructive actions like "deletes" of AWS Resources, there should be additional steps for a user before they can proceed with performing said action
Enforce a user to re-authenticate themselves before they are able to successfully call the API request performing the sensitive action
Technical Implementation:
Backend Process
auth_time
header from API RequestTTL
window from the latest authentication time (auth_time
) and if not return a 401 Unauthorized Error with extension codeREAUTH
Frontend Error Handling (if receiving custom 401 error indicating re-auth required)
NOTE: The re-login flow will give user new tokens and update latest
auth_time
in future API RequestsUser Experience
User A logs in to data.all and immediately requests a sensitive action (e.g. DELETE_DATASET) on the data.all UI for DatasetA


auth_time
is within the configuredTTL
time window (configured at 5 minutes) so the delete action is allowedUser A waits 10 minutes and requests another sensitive action (e.g. DELETE_DATASET) on the data.all UI for DatasetB
auth_time
is NOT within the configuredTTL
time window anymore so a 401 is returned and the ReAuth modal pops upUserA decides to re-authenticated is a logged out of data.all
UserA provides proper credentials to its Identity Provider and receives a new token with a new

auth_time
Relates
Security
Please answer the questions below briefly where applicable, or write
N/A
. Based onOWASP 10.
fetching data from storage outside the application (e.g. a database, an S3 bucket)?
eval
or similar functions are used?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.