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

Add SCP error handling in Quicksight identity region checks #896

Merged
merged 5 commits into from
Dec 6, 2023

Conversation

dlpzx
Copy link
Contributor

@dlpzx dlpzx commented Nov 30, 2023

Feature or Bugfix

  • Feature
  • Bugfix

Detail

There is no API to obtain the Quicksight identity region used for an account, we obtain it form the error logs of the response of describe_groups. However, it does not take into account AccessDenied errors based on SCPs.
A more detailed description of the issue can be found in #851

This PR:

  • handles AccessDenied errors based on SCPs and retries other Quicksight identity regions
  • fixes some methods for registering users that should be using the Quicksight client in the identity region.

Relates

Security

Please answer the questions below briefly where applicable, or write N/A. Based on
OWASP 10. --> N/A

  • 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.

@dlpzx
Copy link
Contributor Author

dlpzx commented Dec 1, 2023

Tested in AWS

  • Create session with new user for Quicksight with identity region us-east-1
  • Create session with new user for Quicksight with identity region eu-central-1
  • Create session with new user for Quicksight with identity region eu-central-1 and SCPs blocking all actions in us-east-1 in the account ---> Found some edge cases, fixing at the moment

@dlpzx
Copy link
Contributor Author

dlpzx commented Dec 4, 2023

Re-Tested in AWS

  • Create session with new user for Quicksight with identity region eu-central-1 no SCPs --> successfully uses eu-central-1
  • Create session with new user for Quicksight with identity region eu-central-1 and SCPs blocking all actions in all regions except eu-central-1 in the account --> successfully uses eu-central-1
  • Create session with new user for Quicksight with identity region eu-central-1 and SCPs blocking all actions in region us-east-1 in the account --> successfully uses eu-central-1
  • Create session with new user for Quicksight with SCPs blocking all Quicksight actions in all regions in the account --> fails and shows error "Quicksight subscription is inactive or the identity region has SCPs preventing access from data.all to account "

Screenshot for "Create session with new user for Quicksight with identity region eu-central-1 and SCPs blocking all actions in region us-east-1 in the account"

Screenshot 2023-12-04 at 13 53 31

@noah-paige
Copy link
Contributor

noah-paige commented Dec 4, 2023

I have tested the above in an AWS Deployment with the following tests:

  • Environment w/ QS identity region us-east-1 can start a session (No SCPs)

    • Correctly identify us-east-1 as identity region
    • User Created/Registered in QS Group
  • Environment w/ QS identity region us-east-1 fails to start a session (SCP enforcing eu-central ONLY)

  • Environment w/ QS identity region eu-central-1 can start a session (No SCPs)

    • Correctly identify eu-central-1 as identity region from error message
    • User Created/Registered in QS Group
  • Environment w/ QS identity region eu-central-1 can start a session (SCP enforcing eu-central ONLY)

    • Correctly identify eu-central-1 as identity region from trial and error
    • User Created/Registered in QS Group
    • Works only if dataall QS Group already created

I believe I found 1 issue, when I perform the following steps:

  • I turn on dashboards enabled for my Env
  • I create my QS subscription in my linked Env
  • Then immediately try to start a session and get an error (I found this error when enforcing SCPs for eu-central-1 only)

I believe this is because we do not actually create the dataall QS Group until we either create a dataset or process an LF table share. Meaning when I was trying to create the session in the above the dataall QS group did not exist.

Then, when get_identity_region() is called - for the SCP deny regions it skips and continues with newer regions. But when it gets to the correct identity region eu-central-1 it throws a ResourceNotFoundException rather than a AccessDeniedException and just calls pass

Some ideas to resolve the above:

  1. Can we confidently say that if we get the ResourceNotFoundException then the identity region is the current one being used and that the dataall QS Group just does not exist? (i.e. instead of calling pass call return identity_region)
  • This is likely the simplest fix because the next step after getting identity region will be to create the QS Group if it does not exist
  1. Use a different QS API instead of describe_group() in def get_identity_region() to determine the identity region (if possible could we use describe_account_settings() - I am not sure the exceptions would be the exact same so logic may have to be changed then on error handling)

@noah-paige
Copy link
Contributor

Along similar lines to the above comment - do we not need to specify the correct identity region for check_quicksight_enterprise_subscription()?

I imagine it depends on the SCP being enforced whether this will work or not but to be safe should we ensure this check is also using the correct identity region, otherwise we may face trouble creating datasets or processing shares when QS is enabled on the parent environment

@dlpzx
Copy link
Contributor Author

dlpzx commented Dec 5, 2023

Hi @noah-paige sorry for the bug :S
I have been doing some testing on Quicksight APIs (the documentation does not clearly define which APIs need to be called using the identity region). In the testing scenario, Quicksight actions are denied by SCPs in all regions except for us-east-1 and eu-central-1. And the identity region is eu-central-1

Finding 1: describe-account-subscription does NOT need to be called against the identity region:
Screenshot 2023-12-05 at 09 19 01

Finding 2: describe-account-settings needs to be called against the identity region:
Screenshot 2023-12-05 at 09 22 51

Finding 3: describe-account-settings returns the same info if a Quicksight Subscription is in "UNSUBSCRIBED" state. So we cannot use it to check the subscription.

Screenshot 2023-12-05 at 09 50 51

@dlpzx
Copy link
Contributor Author

dlpzx commented Dec 5, 2023

With the above in mind I am implementing the following changes:

  • directly use describe-account-settings to get the identity region
  • add quicksight:DescribeAccountSettings to pivot role permissions
  • Instead of using the default hardcoded eu-west-1 indescribe-account-subscription, use the environment region. This way we avoid SCPs in regions out of the environment region. We are therefore assuming that in the environment region, if Dashboards are enabled, Quicksight should not be restricted by SCPs. It is an opinionated assumption, so I would like your view on this.

@dlpzx
Copy link
Contributor Author

dlpzx commented Dec 5, 2023

Re-tested in AWS:
On a new QS subscription with SCPs (no groups or user created) with SCPs blocking all except identity region eu-central-1 and another region us-west-2

  • Correctly identify eu-central-1 as identity region from trial and error ---> skips us-east-1 and us-east-2 and identifies endpoint when calling us-west-2

On a new QS subscription (no groups or user created) with SCPs blocking all except environment.region (us-east-1) and identity region eu-central-1

  • Correctly identify eu-central-1 as identity region from trial and error
  • Correctly check subscription and create group

@noah-paige
Copy link
Contributor

Deploying the latest changes now to test...

I think the assumption is fair that if Dashboards are enabled on an environment then Quicksight should not be restricted by SCPs. Using env region aligns with the way we set up client in DashboardQuicksightClient() to generate the qs sessions or work with dashboards

@noah-paige
Copy link
Contributor

Final Testing:

  • Environment w/ QS identity region us-east-1 can start a session (No SCPs)

    • Correctly identify us-east-1 as identity region
    • User Created/Registered in QS Group
  • Environment w/ QS identity region us-east-1 fails to start a session (SCP enforcing eu-central ONLY)

  • Environment w/ QS identity region eu-central-1 can start a session (No SCPs)

    • Correctly identify eu-central-1 as identity region from error message
    • User Created/Registered in QS Group
  • Environment w/ QS identity region eu-central-1 can start a session (SCP enforcing eu-central-1 and us-east-1 ONLY)

    • Correctly identify eu-central-1 as identity region from trial and error
    • User Created/Registered in QS Group

So our prescriptive guidance will have to be - you must allow quicksight permissions to both the identity region and the environment region (if they differ)

Ultimately one could break down the exact permissions required for each (i.e. identity region needs CreateGroup,... while env region needs GenerateEmbedUrlForRegisteredUser,...)

Copy link
Contributor

@noah-paige noah-paige left a comment

Choose a reason for hiding this comment

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

After final review of the code and final testing in AWS I think this PR looks good to be merged!

@dlpzx dlpzx merged commit 2e0fd39 into main Dec 6, 2023
9 checks passed
@dlpzx dlpzx deleted the fix/quicksight-identity-region branch December 18, 2023 15:15
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.

2 participants