s3 connector (for data detection & discovery) POC - fides #4930

adamsachs · 2024-05-29T14:53:05Z

‼️ includes a DB migration, bump downrev as needed!

Needed for https://github.com/ethyca/fidesplus/pull/1456. See loom there for a demo of the connector functionality that's been added here..

Description Of Changes

OSS-side changes to support s3 as (initially) a d&d data source. Eventually we'll want to support S3 as a DSR data source, but that's not supported for now.

This PR creates an "S3" connection type that provides a space for AWS authentication credentials to be stored. We implement a new (and improved!) AWS auth paradigm that can leverage an optional "assume role ARN" to assume a specific role for S3 communications. This paradigm is not yet leveraged for our other AWS connections (e.g. DynamoDB), but the underlying utility can easily be leveraged, and we should look to support this paradigm for those connection types soon.

The S3 connection type implements a basic connection test to list buckets (as that's a key operation for our d&d monitor), but no support for the methods that need to be implemented for DSR processing.

Code Changes

add S3Connector, ConnectionType.s3 enum, and S3Schema for secrets/credentials. add logo for the FE
refactor, rename and improve an aws_util..py that can be used generically to get an AWS session with provided credentials that optionally assumes a provided role ARN

Steps to Confirm

connection type can be selected in the UI and tested successfully - with and without a provided assume role ARN
further manual testing done while integrating with the associated fidesplus PR

Pre-Merge Checklist

vercel · 2024-05-29T14:53:09Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
fides-plus-nightly	⬜️ Ignored (Inspect)	Visit Preview		Jun 14, 2024 6:28pm

cypress · 2024-05-31T18:45:52Z

Passing run #8335 ↗︎

0	4	0	0	0
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.

Details:

Merge `1c76a85` into `3b92387`...
Project: fides	Commit: `969fd7c591 ℹ️`
Status: Passed	Duration: 00:34 💡
Started: Jun 14, 2024 6:39 PM	Ended: Jun 14, 2024 6:40 PM

Review all test suite changes for PR #4930 ↗︎

… some shared aws utils

adamsachs

some self-review

adamsachs · 2024-06-12T14:24:33Z

src/fides/api/util/aws_util.py

changed name of storage_authenticator.py to make this more generic. also updated some functionality too (see below)

adamsachs · 2024-06-12T14:26:51Z

src/fides/api/util/aws_util.py

+        raise ValueError(f"Auth method not supported for S3: {auth_method}")
+
+    if assume_role_arn:
+        if sts_client is None:


i wasn't sure if it was safe to call the

sts_client = session.client("sts") sts_client.get_caller_identity()

block for all use cases with the "automatic" auth method, since we hadn't been doing that before. so rather than risk a regression, i'm only creating the sts client when secret keys are provided (as it had been done before), OR unless it's explicitly needed to assume role...the code is a bit harder to follow/less DRY, but i'd rather be safe for now...

adamsachs · 2024-06-12T14:27:37Z

src/fides/api/util/aws_util.py

+from fides.api.schemas.storage.storage import AWSAuthMethod, StorageSecrets
+
+
+def get_aws_session(


more appropriately/generically named function now. the function here can and should be used by some of our other AWS connectors/codepaths, since this seems like a pretty solid authentication paradigm to promote across AWS resource types.

src/fides/api/service/connectors/s3_connector.py

adamsachs · 2024-06-12T14:30:10Z

src/fides/api/schemas/connection_configuration/connection_secrets_dynamodb.py

@@ -25,6 +25,8 @@ class DynamoDBSchema(ConnectionConfigSecretsSchema):
        sensitive=True,
    )

+    # TODO: include an aws_assume_role_arn and more closely follow the pattern in `connection_secrets_s3`


i don't want to make our changes too wide-ranging quite yet, so i've just put in a note that we should look to switch dynamoDB over to this authentication paradigm soon. dynamoDB does require a region (I think?), so i'm not sure how that changes things. but I still think that provided an option to assume a role will be the right thing to support here moving forward 👍

codecov · 2024-06-12T14:56:57Z

Codecov Report

Attention: Patch coverage is 70.96774% with 27 lines in your changes missing coverage. Please review.

Project coverage is 86.19%. Comparing base (3b92387) to head (1c76a85).
Report is 1 commits behind head on main.

Files	Patch %	Lines
src/fides/api/service/connectors/s3_connector.py	63.33%	11 Missing ⚠️
src/fides/api/util/aws_util.py	67.74%	10 Missing ⚠️
.../connection_configuration/connection_secrets_s3.py	83.33%	3 Missing ⚠️
src/fides/api/task/task_resources.py	33.33%	2 Missing ⚠️
src/fides/api/tasks/storage.py	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4930      +/-   ##
==========================================
- Coverage   86.27%   86.19%   -0.08%     
==========================================
  Files         349      351       +2     
  Lines       21628    21693      +65     
  Branches     2869     2872       +3     
==========================================
+ Hits        18659    18699      +40     
- Misses       2470     2495      +25     
  Partials      499      499

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

daveqnet · 2024-06-12T15:15:20Z

src/fides/api/schemas/connection_configuration/connection_secrets_s3.py

+    aws_access_key_id: Optional[str] = Field(
+        title="Access Key ID",
+        description="Part of the credentials that provide access to your AWS account. This is required if using secret key authentication.",
+    )
+    aws_secret_access_key: Optional[str] = Field(
+        title="Secret Access Key",
+        description="Part of the credentials that provide access to your AWS account. This is required if using secret key authentication.",
+        sensitive=True,
+    )
+
+    aws_assume_role_arn: Optional[str] = Field(
+        title="Assume Role ARN",
+        description="If provided, the ARN of the role that should be assumed to connect to s3.",
+    )


Schema looks good! From my perspective:

Access Key ID: not particularly sensitive, a bit like a username

Secret Access Key: sensitive, treat it like a password

Assume Role ARN: not particularly sensitive, again a bit like a username

daveqnet · 2024-06-12T15:36:32Z

src/fides/api/util/aws_util.py

+
+from fides.api.common_exceptions import StorageUploadError
+from fides.api.schemas.storage.storage import AWSAuthMethod, StorageSecrets
+


Just a general comment on the utility here rather than any specific line of code: great idea to make a generic utility than can be reused, AWS auth won't change.

As I understand it, the utility gives three authn options:

Use an IAM user's long-term access key to authenticate for the entire session.

Rely on boto3's automatic method of checking the local environment for different ways AWS credentials can be provided by the env, and authenticate using them for the session. There are a few of these, ranging from env vars to using the EC2 instance metadata service, all documented by AWS here.

Use an IAM user's long-term access key to get temporary security credentials (a.k.a. assume an IAM role) and use these temporary creds for the session.

that's just about spot on! the only slight difference is that i've also allowed the AWS credentials derived by boto3 (i.e. option #2) to be used in step #3, i.e. to assume an IAM role (if a user has specified automatic authentication method while also providing an assume role ARN).

is that even a valid use case? i was a bit unsure whether this is something that can (or should) be supported. if not, i can easily clean up the code and make this a bit more straightforward.

adamsachs · 2024-06-12T21:53:13Z

@pattisdr i wanted to get this OSS-side PR on your queue for review, no big rush though. i'm still working through the final touches on the plus-side to get it ready for review, but i think this can be reviewed in isolation and don't want to leave it totally held up by the plus one!

pattisdr · 2024-06-13T19:47:10Z

Starting review - apologies for the merge conflicts 😓

src/fides/api/schemas/connection_configuration/connection_secrets_s3.py

src/fides/api/alembic/migrations/versions/cb344673f633_add_s3_connection_type.py

src/fides/api/service/connectors/s3_connector.py

src/fides/api/schemas/storage/storage.py

src/fides/api/service/connectors/s3_connector.py

src/fides/api/schemas/connection_configuration/connection_secrets_s3.py

src/fides/api/service/connectors/s3_connector.py

src/fides/api/util/aws_util.py

pattisdr

this looks good to me @adamsachs nice round of changes

adamsachs · 2024-06-14T21:10:55Z

test is failing in main and is unrelated - will go ahead and merge 👍

cypress · 2024-06-14T21:23:21Z

Passing run #8343 ↗︎

0	4	0	0	0
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.

Details:

s3 connector (for data detection & discovery) POC - fides (#4930)
Project: fides	Commit: `a82c7c446a`
Status: Passed	Duration: 00:36 💡
Started: Jun 14, 2024 9:22 PM	Ended: Jun 14, 2024 9:23 PM

Review all test suite changes for PR #4930 ↗︎

adamsachs added 3 commits June 7, 2024 17:58

add s3 connection type; some related reorg for better code sharing

3000dae

add required components list for s3 secrets

9e28540

migration to add s3 to db connection type enum

4db3d15

adamsachs force-pushed the asachs/PROD-2098-s3-3d-poc branch from 4c2154f to 4db3d15 Compare June 7, 2024 21:58

adamsachs added 7 commits June 8, 2024 12:11

bump downrev

22fcac8

Merge branch 'main' into asachs/PROD-2098-s3-3d-poc

d53c6de

bump downrev

99c8cc0

support aws assume role; provide a basic s3 connector shell; refactor…

1e520fa

… some shared aws utils

add s3 connection type logo

7c8487e

clean up static checks and fix tests

9d42987

remove unused import

d4afd1d

adamsachs changed the title ~~DRAFT: s3 3d (data detection & discovery) POC - fides~~ s3 connector (for data detection & discovery) POC - fides Jun 12, 2024

udpate changelog

3466682

adamsachs self-assigned this Jun 12, 2024

adamsachs commented Jun 12, 2024

View reviewed changes

adamsachs marked this pull request as ready for review June 12, 2024 14:30

daveqnet reviewed Jun 12, 2024

View reviewed changes

adamsachs requested a review from pattisdr June 12, 2024 21:52

pattisdr reviewed Jun 13, 2024

View reviewed changes

adamsachs added 5 commits June 14, 2024 07:02

s3 connector cleanup

7183bf2

Merge branch 'main' into asachs/PROD-2098-s3-3d-poc

1cfd96f

reconcile connection type migration with new scylla migration from main

11d9b63

fix up broken merge conflict

42ba44f

improve aws session logic

5133c64

pattisdr approved these changes Jun 14, 2024

View reviewed changes

adamsachs added 2 commits June 14, 2024 14:04

Merge branch 'main' into asachs/PROD-2098-s3-3d-poc

1c1c956

fix tests

1c76a85

adamsachs merged commit a82c7c4 into main Jun 14, 2024
46 of 48 checks passed

adamsachs deleted the asachs/PROD-2098-s3-3d-poc branch June 14, 2024 21:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

s3 connector (for data detection & discovery) POC - fides #4930

s3 connector (for data detection & discovery) POC - fides #4930

adamsachs commented May 29, 2024 •

edited

Loading

vercel bot commented May 29, 2024 •

edited

Loading

cypress bot commented May 31, 2024 •

edited

Loading

adamsachs left a comment

adamsachs Jun 12, 2024

adamsachs Jun 12, 2024

adamsachs Jun 12, 2024

adamsachs Jun 12, 2024

codecov bot commented Jun 12, 2024 •

edited

Loading

daveqnet Jun 12, 2024 •

edited

Loading

daveqnet Jun 12, 2024 •

edited

Loading

adamsachs Jun 12, 2024

adamsachs commented Jun 12, 2024 •

edited

Loading

pattisdr commented Jun 13, 2024

pattisdr left a comment

adamsachs commented Jun 14, 2024

cypress bot commented Jun 14, 2024

		from fides.api.schemas.storage.storage import AWSAuthMethod, StorageSecrets


		def get_aws_session(


		from fides.api.common_exceptions import StorageUploadError
		from fides.api.schemas.storage.storage import AWSAuthMethod, StorageSecrets

s3 connector (for data detection & discovery) POC - fides #4930

s3 connector (for data detection & discovery) POC - fides #4930

Conversation

adamsachs commented May 29, 2024 • edited Loading

Description Of Changes

Code Changes

Steps to Confirm

Pre-Merge Checklist

vercel bot commented May 29, 2024 • edited Loading

cypress bot commented May 31, 2024 • edited Loading

Passing run #8335 ↗︎

Review all test suite changes for PR #4930 ↗︎

adamsachs left a comment

Choose a reason for hiding this comment

adamsachs Jun 12, 2024

Choose a reason for hiding this comment

adamsachs Jun 12, 2024

Choose a reason for hiding this comment

adamsachs Jun 12, 2024

Choose a reason for hiding this comment

adamsachs Jun 12, 2024

Choose a reason for hiding this comment

codecov bot commented Jun 12, 2024 • edited Loading

Codecov Report

daveqnet Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

daveqnet Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

adamsachs Jun 12, 2024

Choose a reason for hiding this comment

adamsachs commented Jun 12, 2024 • edited Loading

pattisdr commented Jun 13, 2024

pattisdr left a comment

Choose a reason for hiding this comment

adamsachs commented Jun 14, 2024

cypress bot commented Jun 14, 2024

Passing run #8343 ↗︎

Review all test suite changes for PR #4930 ↗︎

adamsachs commented May 29, 2024 •

edited

Loading

vercel bot commented May 29, 2024 •

edited

Loading

cypress bot commented May 31, 2024 •

edited

Loading

codecov bot commented Jun 12, 2024 •

edited

Loading

daveqnet Jun 12, 2024 •

edited

Loading

daveqnet Jun 12, 2024 •

edited

Loading

adamsachs commented Jun 12, 2024 •

edited

Loading