From 41253a851d1f22698e3316b579b0a7ad530311fd Mon Sep 17 00:00:00 2001 From: Rohan Weeden Date: Fri, 28 Oct 2022 11:13:11 -0800 Subject: [PATCH 01/10] Update dependencies --- requirements/requirements-dev.txt | 12 ++++++------ requirements/requirements.in | 2 +- requirements/requirements.txt | 8 +++----- 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/requirements/requirements-dev.txt b/requirements/requirements-dev.txt index df8a9afe..def1507c 100644 --- a/requirements/requirements-dev.txt +++ b/requirements/requirements-dev.txt @@ -8,14 +8,14 @@ attrs==21.4.0 # via # -c requirements/requirements.txt # pytest -boto3==1.24.94 +boto3==1.25.3 # via -r requirements/requirements-dev.in -botocore==1.27.94 +botocore==1.28.3 # via # -c requirements/requirements.txt # boto3 # s3transfer -build==0.8.0 +build==0.9.0 # via pip-tools click==8.1.3 # via @@ -25,6 +25,8 @@ coverage[toml]==6.5.0 # via pytest-cov deprecated==1.2.13 # via opentelemetry-api +exceptiongroup==1.0.0 + # via pytest iniconfig==1.1.1 # via pytest jmespath==1.0.1 @@ -46,11 +48,9 @@ pip-tools==6.9.0 # via -r requirements/requirements-dev.in pluggy==1.0.0 # via pytest -py==1.11.0 - # via pytest pyparsing==3.0.9 # via packaging -pytest==7.1.3 +pytest==7.2.0 # via # -r requirements/requirements-dev.in # pytest-cov diff --git a/requirements/requirements.in b/requirements/requirements.in index 2a27507a..3caf773b 100644 --- a/requirements/requirements.in +++ b/requirements/requirements.in @@ -2,5 +2,5 @@ cachetools cfnresponse chalice flatdict -git+https://github.com/asfadmin/rain-api-core.git@e1638eec08353651ed42105840ba6a80a7141ac2 +git+https://github.com/asfadmin/rain-api-core.git@16329f3fa7326edd668e26c2d817cf430268faad netaddr diff --git a/requirements/requirements.txt b/requirements/requirements.txt index e9555414..b7dc6d3a 100644 --- a/requirements/requirements.txt +++ b/requirements/requirements.txt @@ -8,7 +8,7 @@ attrs==21.4.0 # via chalice blessed==1.19.1 # via inquirer -botocore==1.27.94 +botocore==1.28.3 # via chalice cachetools==5.0.0 # via @@ -42,7 +42,7 @@ netaddr==0.8.0 # rain-api-core pycparser==2.21 # via cffi -pyjwt[crypto]==2.5.0 +pyjwt[crypto]==2.6.0 # via rain-api-core python-dateutil==2.8.2 # via botocore @@ -52,7 +52,7 @@ pyyaml==6.0 # via # chalice # rain-api-core -rain-api-core @ git+https://github.com/asfadmin/rain-api-core.git@e1638eec08353651ed42105840ba6a80a7141ac2 +rain-api-core @ git+https://github.com/asfadmin/rain-api-core.git@16329f3fa7326edd668e26c2d817cf430268faad # via -r requirements/requirements.in readchar==4.0.3 # via inquirer @@ -61,8 +61,6 @@ six==1.16.0 # blessed # chalice # python-dateutil -types-cryptography==3.3.23.1 - # via pyjwt typing-extensions==4.4.0 # via chalice urllib3==1.26.12 From 2dedc86c655e7a9acd9e4d8b4e832d506e13ad54 Mon Sep 17 00:00:00 2001 From: Rohan Weeden Date: Fri, 28 Oct 2022 15:05:53 -0800 Subject: [PATCH 02/10] Enable IAM compatibility checking only when s3credentials are enabled --- cloudformation/thin-egress-app.yaml.j2 | 1 + lambda/app.py | 22 +++++++++++++++++----- tests/test_app.py | 22 ++++++++++++++++++++++ 3 files changed, 40 insertions(+), 5 deletions(-) diff --git a/cloudformation/thin-egress-app.yaml.j2 b/cloudformation/thin-egress-app.yaml.j2 index 5808dc74..a13e85c3 100644 --- a/cloudformation/thin-egress-app.yaml.j2 +++ b/cloudformation/thin-egress-app.yaml.j2 @@ -620,6 +620,7 @@ Resources: HTML_TEMPLATE_DIR: !Ref HtmlTemplateDir JWT_KEY_SECRET_NAME: !Ref JwtKeySecretName JWT_ALGO: !Ref JwtAlgo + ENABLE_S3_CREDENTIALS_ENDPOINT: !Ref EnableS3CredentialsEndpoint AWS_LAMBDA_EXEC_WRAPPER: !Ref OtLambdaExecWrapper OPENTELEMETRY_COLLECTOR_CONFIG_FILE: !Ref OtCollectorConfigFile OTEL_LOG_LEVEL: !Ref OtLogLevel diff --git a/lambda/app.py b/lambda/app.py index 18d3ee30..c5fe411e 100644 --- a/lambda/app.py +++ b/lambda/app.py @@ -381,13 +381,25 @@ def restore_bucket_vars(): b_map_dict = get_yaml_file(conf_bucket, bucket_map_file) reverse = os.getenv('USE_REVERSE_BUCKET_MAP', 'FALSE').lower() == 'true' + iam_compatible = os.getenv('ENABLE_S3_CREDENTIALS_ENDPOINT', 'FALSE').lower() == 'true' log.debug('bucket map: %s', b_map_dict) - b_map = BucketMap( - b_map_dict, - bucket_name_prefix=get_bucket_name_prefix(), - reverse=reverse - ) + try: + b_map = BucketMap( + b_map_dict, + bucket_name_prefix=get_bucket_name_prefix(), + reverse=reverse, + iam_compatible=iam_compatible + ) + except ValueError: + log.error('Invalid bucket map, please consult the TEA documentation') + if iam_compatible: + log.info( + 'Your bucket map permissions are configured in such a way ' + 'that they cannot be converted to an IAM policy. Either ' + 'fix your bucket map, or disable S3 credentials.' + ) + raise else: log.info('reusing old bucket configs') diff --git a/tests/test_app.py b/tests/test_app.py index cd4c9428..33011afb 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -384,6 +384,28 @@ def test_restore_bucket_vars(mock_get_yaml_file, resources): assert app.b_map.bucket_map == buckets +@mock.patch(f"{MODULE}.get_yaml_file", autospec=True) +def test_restore_bucket_vars_iam_compatibility_error( + mock_get_yaml_file, + monkeypatch +): + mock_get_yaml_file.return_value = { + "PATH": "bucket", + "PRIVATE_BUCKETS": { + "bucket/prefix/": ["group"] + } + } + + app.b_map = None + monkeypatch.setenv("ENABLE_S3_CREDENTIALS_ENDPOINT", "False") + app.restore_bucket_vars() + + app.b_map = None + monkeypatch.setenv("ENABLE_S3_CREDENTIALS_ENDPOINT", "True") + with pytest.raises(ValueError, match="Invalid prefix permissions"): + app.restore_bucket_vars() + + @mock.patch(f"{MODULE}.get_urs_url", autospec=True) def test_do_auth_and_return(mock_get_urs_url, monkeypatch): mock_get_urs_url.side_effect = lambda _ctx, redirect: redirect From ebca8913ccdd9c012ddf710a88b0ee8d6d819961 Mon Sep 17 00:00:00 2001 From: Rohan Weeden Date: Fri, 28 Oct 2022 15:06:33 -0800 Subject: [PATCH 03/10] Fix test being affected by other tests --- tests/test_app.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_app.py b/tests/test_app.py index 33011afb..d659941c 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -578,6 +578,7 @@ def test_try_download_from_bucket( client = mock_get_role_session().client() client.get_bucket_location.return_value = {"LocationConstraint": "us-east-1"} client.head_object.return_value = {"ContentLength": 2048} + app.b_map = None response = app.try_download_from_bucket("somebucket", "somefile", user_profile, {}) client.head_object.assert_called_once() From c80888992e59140ec43a7a7d9f20099523b2ca13 Mon Sep 17 00:00:00 2001 From: Rohan Weeden Date: Tue, 1 Nov 2022 14:17:50 -0700 Subject: [PATCH 04/10] Adjust docs to discuss IAM compatibility --- docs/configuration.md | 90 +++++++++++++++++++++++++++++++++++++++---- 1 file changed, 82 insertions(+), 8 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 3ec6e680..0e4b2896 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -101,22 +101,96 @@ object prefixing to control access: ```yaml MAP: - data-path: data-bucket + data-path: data-bucket PUBLIC_BUCKETS: - data-bucket/browse/: "Browse image" + data-bucket/browse/: "Browse image" PRIVATE_BUCKETS: - data-bucket/pre-commission-data/: - - internal_users - - external_team + data-bucket/: + - internal_users + - external_team +``` + +In the above example, while access to data in `data-bucket` requires EDL App +group access to one of the specified groups, accessing an object with the +prefix `browse/` will not require any auth. + +TEA will always check the rules from most deeply nested to most shallow. So +for example, in this bucket map: + +```yaml +MAP: + data-path: data-bucket + +PUBLIC_BUCKETS: + data-bucket/external/public/: "Browse image" + +PRIVATE_BUCKETS: + data-bucket/: + - internal_users + data-bucket/external/: + - internal_users + - external_team ``` -In the above example, while access to data in `data-bucket` requires simple auth, -accessing an object with the prefix `browse/` will require NO auth, and -`pre-commission-data/` will require EDL App group access as specified. +Any data in the prefix `external/public/` will be public, data in the prefix +`external/` but not in the prefix `external/public/` will be available to users +in either of the defined EDL App groups, and everything else in the bucket will +be available to users in the `internal_users` group only. + +##### S3 direct access compatibility + +Note that there are some access configurations supported by the standard HTTP +access method, that are not allowed when S3 direct access is enabled (see +[S3 direct access](#s3-direct-access)). This is due to a limitation with how +IAM policies work. In particular, in IAM compatibility mode prefixes must +always become more open as they become more nested. All of the bucket maps +shown above are compatible with s3 direct access, however, long time users of +TEA might recognize the following configuration example from previous versions +which will be rejected when S3 direct access is enabled. + +Bad Example: +```yaml +MAP: + data-path: data-bucket + +PUBLIC_BUCKETS: + data-bucket/browse/: "Browse image" + +PRIVATE_BUCKETS: + # Since no permission is specified for `data-bucket/`, the default is used + # which allows access to any authenticated user. Therefore, adding a + # prefix `pre-commission-data/` that is more restrictive will break IAM + # compatibility. + data-bucket/pre-commission-data/: + - internal_users + - external_team +``` + +To fix this, the bucket map could modified as follows: + +Good Example: +```yaml +MAP: + data-path: data-bucket + +PUBLIC_BUCKETS: + data-bucket/browse/: "Browse image" + +PRIVATE_BUCKETS: + data-bucket/: + - internal_users + - external_team + # The following rule is redundant, but might not be in the presence of + # other rules. + data-bucket/pre-commission-data/: + - internal_users + - external_team +``` #### S3 Direct Access + *NOTE: Support for S3 direct access is currently experimental* TEA can be deployed with an `/s3credentials` endpoint (See From 580f752339963ad38f1eaf55b78d4ed59eef33fc Mon Sep 17 00:00:00 2001 From: Rohan Weeden Date: Fri, 4 Nov 2022 15:39:15 -0700 Subject: [PATCH 05/10] Use `published` actions type This will also trigger on prereleases --- .github/workflows/release.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 9a14936b..36c24792 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -6,7 +6,7 @@ name: Release on: release: types: - - released + - published jobs: build: From 52be118e7b1105656fbe8ddbc6c0b3fd92f9cb1f Mon Sep 17 00:00:00 2001 From: McKade Sorensen Date: Thu, 10 Nov 2022 17:47:10 -0900 Subject: [PATCH 06/10] Fixing wheel cve --- requirements/requirements-dev.txt | 2 +- requirements/requirements.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/requirements/requirements-dev.txt b/requirements/requirements-dev.txt index def1507c..bec037b6 100644 --- a/requirements/requirements-dev.txt +++ b/requirements/requirements-dev.txt @@ -80,7 +80,7 @@ urllib3==1.26.12 # via # -c requirements/requirements.txt # botocore -wheel==0.37.1 +wheel==0.38.4 # via # -c requirements/requirements.txt # pip-tools diff --git a/requirements/requirements.txt b/requirements/requirements.txt index b7dc6d3a..3709534c 100644 --- a/requirements/requirements.txt +++ b/requirements/requirements.txt @@ -69,7 +69,7 @@ urllib3==1.26.12 # cfnresponse wcwidth==0.2.5 # via blessed -wheel==0.37.1 +wheel==0.38.4 # via chalice # The following packages are considered to be unsafe in a requirements file: From 6647ae5fff256cbe881cc00b7e70cd52e9a7fd12 Mon Sep 17 00:00:00 2001 From: krobin10 <102176281+krobin10@users.noreply.github.com> Date: Tue, 15 Nov 2022 16:35:52 -0900 Subject: [PATCH 07/10] Update configuration.md Optional S3 Direct Access Compatibility section changes --- docs/configuration.md | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 0e4b2896..8c375ffe 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -139,16 +139,15 @@ Any data in the prefix `external/public/` will be public, data in the prefix in either of the defined EDL App groups, and everything else in the bucket will be available to users in the `internal_users` group only. -##### S3 direct access compatibility +##### S3 Direct Access Compatibility -Note that there are some access configurations supported by the standard HTTP -access method, that are not allowed when S3 direct access is enabled (see -[S3 direct access](#s3-direct-access)). This is due to a limitation with how -IAM policies work. In particular, in IAM compatibility mode prefixes must -always become more open as they become more nested. All of the bucket maps -shown above are compatible with s3 direct access, however, long time users of -TEA might recognize the following configuration example from previous versions -which will be rejected when S3 direct access is enabled. +Some access configurations supported by the standard HTTP methods are not allowed when S3 direct access is enabled. Of note: + +1. The first prefix in the bucket map will need to be set to the most restrictive access level and subsequent prefixes must have access levels that become successively more open. This is due to a limitation with how IAM policies work (For more information, see [S3 direct access](#s3-direct-access)). +2. Public buckets will require EDL authentication for S3 direct access. e.g. "Browse image" + +All of the bucket maps shown above are compatible with S3 direct access; however, long time users of +TEA might recognize the following configuration example from previous versions which will be rejected when S3 direct access is enabled. Bad Example: ```yaml @@ -168,7 +167,7 @@ PRIVATE_BUCKETS: - external_team ``` -To fix this, the bucket map could modified as follows: +To fix this, the bucket map could be modified as follows: Good Example: ```yaml @@ -191,7 +190,7 @@ PRIVATE_BUCKETS: #### S3 Direct Access -*NOTE: Support for S3 direct access is currently experimental* +*NOTE: S3 direct access is currently experimental* TEA can be deployed with an `/s3credentials` endpoint (See [Enabling S3 direct access](deploying.md#enabling-s3-direct-access)) for From e789ef155f68add3b5e71586bf4701b927f6d277 Mon Sep 17 00:00:00 2001 From: Rohan Weeden Date: Mon, 12 Dec 2022 14:04:36 -0900 Subject: [PATCH 08/10] Change s3credential response to use camelCase keys --- docs/s3access.md | 14 +++++++------- lambda/app.py | 7 +++++++ tests/test_app.py | 8 ++++---- tests_e2e/test_s3credentials.py | 8 ++++---- 4 files changed, 22 insertions(+), 15 deletions(-) diff --git a/docs/s3access.md b/docs/s3access.md index ad5d6d42..fc441e81 100644 --- a/docs/s3access.md +++ b/docs/s3access.md @@ -48,10 +48,10 @@ The response is your temporary credentials as returned by Amazon STS. See the **Example:** ```json { - "AccessKeyId": "AKIAIOSFODNN7EXAMPLE", - "SecretAccessKey": "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY", - "SessionToken": "LONGSTRINGOFCHARACTERS.../HJLgV91QJFCMlmY8slIEOjrOChLQYmzAqrb5U1ekoQAK6f86HKJFTT2dONzPgmJN9ZvW5DBwt6XUxC9HAQ0LDPEYEwbjGVKkzSNQh/", - "Expiration": "2021-01-27 00:50:09+00:00" + "accessKeyId": "AKIAIOSFODNN7EXAMPLE", + "secretAccessKey": "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY", + "sessionToken": "LONGSTRINGOFCHARACTERS.../HJLgV91QJFCMlmY8slIEOjrOChLQYmzAqrb5U1ekoQAK6f86HKJFTT2dONzPgmJN9ZvW5DBwt6XUxC9HAQ0LDPEYEwbjGVKkzSNQh/", + "expiration": "2021-01-27 00:50:09+00:00" } ``` @@ -94,9 +94,9 @@ def lambda_handler(event, context): # Set up separate boto3 clients for download and upload download_client = boto3.client( "s3", - aws_access_key_id=creds["AccessKeyId"], - aws_secret_access_key=creds["SecretAccessKey"], - aws_session_token=creds["SessionToken"] + aws_access_key_id=creds["accessKeyId"], + aws_secret_access_key=creds["secretAccessKey"], + aws_session_token=creds["sessionToken"] ) # Lambda needs to have permission to upload to destination bucket upload_client = boto3.client("s3") diff --git a/lambda/app.py b/lambda/app.py index c5fe411e..ec317326 100644 --- a/lambda/app.py +++ b/lambda/app.py @@ -1008,6 +1008,13 @@ def s3credentials(): creds = get_s3_credentials(user_profile.user_id, role_session_name, policy) timer.mark() + creds = { + "accessKeyId": creds["AccessKeyId"], + "secretAccessKey": creds["SecretAccessKey"], + "sessionToken": creds["SessionToken"], + "expiration": creds["Expiration"] + } + log.debug("timing for s3credentials()") timer.log_all(log) diff --git a/tests/test_app.py b/tests/test_app.py index d659941c..f96656c9 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -1430,10 +1430,10 @@ def test_s3credentials( response = client.http.get("/s3credentials") assert response.json_body == { - "AccessKeyId": "access_key", - "SecretAccessKey": "secret_access_key", - "SessionToken": "session_token", - "Expiration": "expiration" + "accessKeyId": "access_key", + "secretAccessKey": "secret_access_key", + "sessionToken": "session_token", + "expiration": "expiration" } assert response.status_code == 200 diff --git a/tests_e2e/test_s3credentials.py b/tests_e2e/test_s3credentials.py index dad281d3..5a627940 100644 --- a/tests_e2e/test_s3credentials.py +++ b/tests_e2e/test_s3credentials.py @@ -17,8 +17,8 @@ def test_authenticated_user_can_get_creds(urls, auth_cookies): assert r.status_code == 200 assert list(data.keys()) == [ - "AccessKeyId", - "SecretAccessKey", - "SessionToken", - "Expiration" + "accessKeyId", + "secretAccessKey", + "sessionToken", + "expiration" ] From 0826df457ad3137947e07f95a75fe2b76478678f Mon Sep 17 00:00:00 2001 From: Rohan Weeden Date: Mon, 12 Dec 2022 16:16:15 -0900 Subject: [PATCH 09/10] Pin ubuntu version in e2e github workflow --- .github/workflows/re-test-e2e.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/re-test-e2e.yml b/.github/workflows/re-test-e2e.yml index 987575ca..569495b0 100644 --- a/.github/workflows/re-test-e2e.yml +++ b/.github/workflows/re-test-e2e.yml @@ -32,7 +32,8 @@ on: jobs: # Deploy to the test environment and run end to end tests test-end-to-end: - runs-on: ubuntu-latest + # Version of gdal installed depends on ubuntu version + runs-on: ubuntu-20.04 environment: ${{ inputs.environment }} env: AWS_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESS_KEY_ID }} From 39ac6a0f37ee982d00d00a07ef48f410d1b8bf8e Mon Sep 17 00:00:00 2001 From: McKade Sorensen Date: Tue, 13 Dec 2022 09:38:01 -0900 Subject: [PATCH 10/10] Updating region --- setup_jwt_cookie.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup_jwt_cookie.sh b/setup_jwt_cookie.sh index 6921925b..885729e3 100644 --- a/setup_jwt_cookie.sh +++ b/setup_jwt_cookie.sh @@ -25,7 +25,7 @@ function GENERATE_TEA_CREDS { } GENERATE_TEA_CREDS -aws secretsmanager create-secret --name jwt_secret_for_tea --profile ${profile_name:-default} --region ${aws_region:-us-east-1} \ +aws secretsmanager create-secret --name jwt_secret_for_tea --profile ${profile_name:-default} --region ${aws_region:-us-west-2} \ --description "RS256 keys for TEA app JWT cookies" \ --secret-string file:///tmp/jwtkeys.json