From 945ca0b7c637269c02fb24314ec848c53af8ecee Mon Sep 17 00:00:00 2001 From: vpsx <19900057+vpsx@users.noreply.github.com> Date: Wed, 15 Jul 2020 16:18:43 -0500 Subject: [PATCH 1/6] feat(ga4gh-scope): Add ga4gh_passport_v1 to user/sess scopes --- fence/config-default.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/fence/config-default.yaml b/fence/config-default.yaml index 9a648dfe1..be1d2ef84 100644 --- a/fence/config-default.yaml +++ b/fence/config-default.yaml @@ -210,6 +210,7 @@ USER_ALLOWED_SCOPES: - "google_credentials" - "google_service_account" - "google_link" + - "ga4gh_passport_v1" # these are the scopes that a browser session can create for a user (very # similar to USER_ALLOWED_SCOPES, as the session will actually create access_tokens @@ -223,6 +224,7 @@ SESSION_ALLOWED_SCOPES: - "google_credentials" - "google_service_account" - "google_link" + - "ga4gh_passport_v1" # ////////////////////////////////////////////////////////////////////////////////////// # LOGIN From dee35b54c9d491628214c0ff0b9e21c174597815 Mon Sep 17 00:00:00 2001 From: vpsx <19900057+vpsx@users.noreply.github.com> Date: Wed, 15 Jul 2020 16:20:33 -0500 Subject: [PATCH 2/6] feat(ga4gh-scope): Add new scopes claim to access tokens --- fence/jwt/token.py | 1 + 1 file changed, 1 insertion(+) diff --git a/fence/jwt/token.py b/fence/jwt/token.py index b7d3dbe47..39056264d 100644 --- a/fence/jwt/token.py +++ b/fence/jwt/token.py @@ -385,6 +385,7 @@ def generate_signed_access_token( "iat": iat, "exp": exp, "jti": jti, + "scope": scopes, "context": { "user": { "name": user.username, From d2df60ce65d79e5c60d7a9d60de91f28e6a13cac Mon Sep 17 00:00:00 2001 From: vpsx <19900057+vpsx@users.noreply.github.com> Date: Wed, 15 Jul 2020 16:21:34 -0500 Subject: [PATCH 3/6] feat(ga4gh-scope): Check access tkn scopes in userinfo --- fence/resources/user/__init__.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/fence/resources/user/__init__.py b/fence/resources/user/__init__.py index 5b0e2ad05..d63ff4435 100644 --- a/fence/resources/user/__init__.py +++ b/fence/resources/user/__init__.py @@ -130,9 +130,15 @@ def get_user_info(current_session, username): info.update(optional_info) # Include ga4gh passport visas - # TODO: Respect access token claims (only include if ga4gh_passport_v1 scope present) - encoded_visas = [row.ga4gh_visa for row in user.ga4gh_visas_v1] - info["ga4gh_passport_v1"] = encoded_visas + if not flask.g.access_token: + logger.warning( + "Session token present but no access token found. Unable to check scopes in userinfo; returning." + ) + else: + at_scopes = jwt.decode(flask.g.access_token, verify=False).get("scope", "") + if "ga4gh_passport_v1" in at_scopes: + encoded_visas = [row.ga4gh_visa for row in user.ga4gh_visas_v1] + info["ga4gh_passport_v1"] = encoded_visas return info From e73d5802e8f9a33e1d9664cd6d5a892041a6f1e9 Mon Sep 17 00:00:00 2001 From: vpsx <19900057+vpsx@users.noreply.github.com> Date: Wed, 15 Jul 2020 16:27:08 -0500 Subject: [PATCH 4/6] docs(debt-scopes): Add tech debt note on aud/scope claims --- TECHDEBT.md | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 TECHDEBT.md diff --git a/TECHDEBT.md b/TECHDEBT.md new file mode 100644 index 000000000..8c87faa6f --- /dev/null +++ b/TECHDEBT.md @@ -0,0 +1,23 @@ +# Tech debt + +### Using 'aud' claim for scopes +Observed: July 2020 +Impact: (If this tech debt affected your work somehow, add a +1 here with a date and note) ++1 Zoe 2020 July 15 This is an example of a +1 + +##### Problem: +Fence puts OAuth2 scopes into the 'aud' claim of access tokens. +##### Why it was done this way: +We don't know. +##### Why this way is problematic: +Per RFC7519 the aud claim is not meant for scopes: https://tools.ietf.org/html/rfc7519#section-4.1.3 +##### What the solution might be: +GA4GH AAI already requires that a 'scope' claim be included in access tokens issued by Passport Brokers: +https://github.com/ga4gh/data-security/blob/master/AAI/AAIConnectProfile.md#access_token-issued-by-broker +So as of July 2020 we will put scopes in the 'scope' claim. However, this is in addition to keeping them in the 'aud' claim. Ideally we would only have the scopes in the 'scope' claim. +##### Why we aren't already doing the above: +Fence presently guards several endpoints (e.g. /data, signed urls, SA registration) by checking the scopes in the 'aud' claim of the JWT. This code would need to be changed. +##### Next steps: +Address above. +##### Other notes: +n/a From 140c59b8d9390a971fa1b47c9867ce4085bb0ee2 Mon Sep 17 00:00:00 2001 From: vpsx <19900057+vpsx@users.noreply.github.com> Date: Wed, 15 Jul 2020 16:37:26 -0500 Subject: [PATCH 5/6] test(scope): Fix test so it expects new scope claim in tkn --- tests/oidc/core/token/test_token_response.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/oidc/core/token/test_token_response.py b/tests/oidc/core/token/test_token_response.py index fa635bfb2..74468fb7b 100644 --- a/tests/oidc/core/token/test_token_response.py +++ b/tests/oidc/core/token/test_token_response.py @@ -72,6 +72,7 @@ def test_access_token_correct_fields(token_response): "exp", "iat", "jti", + "scope", "context", "azp", } From 3c80ad3de2d0fafd304adf4df1c99b8de7affb10 Mon Sep 17 00:00:00 2001 From: vpsx <19900057+vpsx@users.noreply.github.com> Date: Thu, 23 Jul 2020 14:21:10 -0500 Subject: [PATCH 6/6] docs(debt-scopes): Reformat TECHDEBT.md --- TECHDEBT.md | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/TECHDEBT.md b/TECHDEBT.md index 8c87faa6f..774f84bb4 100644 --- a/TECHDEBT.md +++ b/TECHDEBT.md @@ -1,20 +1,17 @@ # Tech debt ### Using 'aud' claim for scopes -Observed: July 2020 -Impact: (If this tech debt affected your work somehow, add a +1 here with a date and note) -+1 Zoe 2020 July 15 This is an example of a +1 - +- Observed: July 2020 +- Impact: (If this tech debt affected your work somehow, add a +1 here with a date and note) + - +1 Zoe 2020 July 15 This is an example of a +1 ##### Problem: Fence puts OAuth2 scopes into the 'aud' claim of access tokens. ##### Why it was done this way: We don't know. ##### Why this way is problematic: -Per RFC7519 the aud claim is not meant for scopes: https://tools.ietf.org/html/rfc7519#section-4.1.3 +Per RFC7519 the aud claim [is not meant for scopes](https://tools.ietf.org/html/rfc7519#section-4.1.3). ##### What the solution might be: -GA4GH AAI already requires that a 'scope' claim be included in access tokens issued by Passport Brokers: -https://github.com/ga4gh/data-security/blob/master/AAI/AAIConnectProfile.md#access_token-issued-by-broker -So as of July 2020 we will put scopes in the 'scope' claim. However, this is in addition to keeping them in the 'aud' claim. Ideally we would only have the scopes in the 'scope' claim. +GA4GH AAI [already requires](https://github.com/ga4gh/data-security/blob/master/AAI/AAIConnectProfile.md#access_token-issued-by-broker) that a 'scope' claim be included in access tokens issued by Passport Brokers. So as of July 2020 we will put scopes in the 'scope' claim. However, this is in addition to keeping them in the 'aud' claim. Ideally we would only have the scopes in the 'scope' claim. ##### Why we aren't already doing the above: Fence presently guards several endpoints (e.g. /data, signed urls, SA registration) by checking the scopes in the 'aud' claim of the JWT. This code would need to be changed. ##### Next steps: