(PXP-6339): RAS visa validation #933

vpsx · 2021-06-09T22:50:27Z

Jira Ticket: PXP-6339

minor version bump to 5.2.0

New Features

Validate RAS visas when they are fetched upon RAS login
Validate RAS visas when they are updated during visa update cronjob
Visas are validated according to the GA4GH AAI spec--see https://github.com/ga4gh/data-security/blob/master/AAI/AAIConnectProfile.md#embedded-access-token-format

Breaking Changes

Bug Fixes

Improvements

Dependency updates

Deployment changes

By default Fence will allow only visas issued by RAS staging, RAS prod, and itself; if a different set of allowed visa issuers is needed, edit the GA4GH_VISA_ISSUER_ALLOWLIST config value. Otherwise, no action needed.

github-actions · 2021-06-09T22:50:59Z

The style in this PR agrees with black. ✔️

This formatting comment was generated automatically by a script in uc-cdis/wool.

coveralls · 2021-06-09T22:55:31Z

Pull Request Test Coverage Report for Build 11239

48 of 72 (66.67%) changed or added relevant lines in 3 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.08%) to 71.044%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
fence/blueprints/login/ras.py	5	17	29.41%
fence/resources/openid/ras_oauth2.py	41	53	77.36%

Files with Coverage Reduction	New Missed Lines	%
fence/resources/openid/ras_oauth2.py	1	53.44%

Totals
Change from base Build 11233:	-0.08%
Covered Lines:	6178
Relevant Lines:	8696

💛 - Coveralls

vpsx · 2021-06-10T17:00:17Z

fence/resources/openid/ras_oauth2.py

+                )
+                continue  # Not raise: If visa malformed, does not make sense to retry
+
+            # See if pkey is in cronjob cache; if not, update cache.


Note: Most of this code is very similar to the code from this recent authutils commit.

The authutils utils rely on keeping a public key cache in the flask app context, and the cronjob runs without one, so this code is here.

The plan is to implement a more persistent cache than this, so here I have taken the simplest and dumbest approach of just repeating much of the authutils code instead of refactoring authutils.

Avantol13 · 2021-06-10T17:02:32Z

fence/blueprints/login/ras.py

+                decoded_visa = validate_jwt(
+                    encoded_visa,
+                    public_key,
+                    # Embedded token must not contain aud claim


is this based on the AAI spec?

Yup! https://github.com/ga4gh/data-security/blob/master/AAI/AAIConnectProfile.md#embedded-access-token-format The payload claims MUST NOT include aud.

(Edit: permalink )

vpsx · 2021-06-10T17:03:42Z

tests/ras/test_ras.py

@@ -134,7 +134,7 @@ def test_update_visa_token(
        },
    }

-    headers = {"kid": kid_2}
+    headers = {"kid": kid}


Note: I couldn't tell if there was a particular reason to be using a different key for the second visa, so figured I'd just use the same one to reduce noise/readability and make it more evident what is being tested. But if I simply missed something, I can definitely switch this back.

Same goes for the rest of this file!

Avantol13 · 2021-06-10T17:08:46Z

fence/config-default.yaml

@@ -824,6 +824,9 @@ ASSUME_ROLE_CACHE_SECONDS: 1800

 # RAS refresh_tokens expire in 15 days
 RAS_REFRESH_EXPIRATION: 1296000
+# List of JWT issuers from which Fence will accept GA4GH visas
+GA4GH_VISA_ISSUER_WHITELIST: []


can this populate itself with the "issuers" from well-known for configured logins on startup? might be overkill, but would be really convenient to not have to keep this up to date. Or, maybe even just adding the RAS issuer and ourselves into the default 🤷‍♂️ ?

fence/fence/config.py

Line 18 in d75257e

def post_process(self):

post process of cfg could maybe shoot out requests to get issuers and populate this, idk, maybe that is overkill. Not all IDPs are GA4GH brokers. But I think "hard coding" RAS and us in the default would make sense and be more convenient for deployments

Oh I figured more explicit was better for a visa whitelist but I can look into this--do you mean like, on app startup, maybe during the client init, hit all of the configured idps' well-known endpoints and add(?) the "issuer" values to this config value?
(Edit: 🤦‍♂️ only saw the first comment, GH didn't refresh with the second)(I'm slightly leaning towards the second approach, partly because the first approach might become sort of an anti-feature if we ever want to exclude an idp from being a visa issuer, partly just because KISS)

Otherwise if taking the second approach--that sounds fine; would you use stsstg or prod RAS or both?

Agreed, very good points. For the second approach, we should probably add both? whatever issuers RAS has for staging and prod

Oh! Minor suggestion, I've been trying to move towards "ALLOWLIST" vs "WHITELIST" but up to you

Word! Thank you 🙏

Avantol13

see comments

… visas

…t update fn

vpsx force-pushed the feat/visa-validate branch 2 times, most recently from c774b71 to 5ff476c Compare June 10, 2021 16:08

vpsx changed the title ~~Feat/visa validate~~ (PXP-6339): RAS visa validation Jun 10, 2021

vpsx commented Jun 10, 2021

View reviewed changes

Avantol13 reviewed Jun 10, 2021

View reviewed changes

vpsx commented Jun 10, 2021

View reviewed changes

Avantol13 reviewed Jun 10, 2021

View reviewed changes

Avantol13 requested changes Jun 10, 2021

View reviewed changes

Avantol13 approved these changes Jun 10, 2021

View reviewed changes

vpsx force-pushed the feat/visa-validate branch 3 times, most recently from b4d02cf to a868796 Compare June 15, 2021 15:49

PlanXCyborg added test-apis-dbgapTest test-google-googleDataAccessTest setting label to retry specific feature and removed test-google-googleDataAccessTest setting label to retry specific feature labels Jun 15, 2021

vpsx added 12 commits June 16, 2021 10:45

feat(visa-validate): Add GA4GH_VISA_ISSUER_WHITELIST config val

b1bb1dc

feat(visa-validate): validate visas

2b25efa

feat(visa-validate): Add try/except and logs around validation

3e9bc03

feat(visa-validate): Update deps, lockfile

7f6ac92

feat(visa-validate): Validate visas in visa update cronjob

1cccd99

feat(visa-validate): Add comment to existing code about clearing user…

0e6a437

… visas

feat(visa-validate): Rm dead code -- visa validation happens in clien…

10a056a

…t update fn

feat(visa-validate): Catch exception when visa is not JWT

21211d2

test(visa-validate): Add visa issuer whitelist to test config

1651066

test(visa-validate): Update RAS tests with public key cache

24cd9c9

feat(visa-validate): Fix dangerous default empty dict argument

4f547af

feat(visa-validate): Rm unused imports

adb2762

This was referenced Oct 19, 2021

feat: update to 2021.10 uc-cdis/gitops-qa#1701

Merged

feat: committing changes from qa-mickey uc-cdis/gitops-qa#1702

Merged

Feat/midrc staging uc-cdis/cdis-manifest#3668

Merged

PlanXCyborg mentioned this pull request Nov 17, 2021

feat(ga4gh-prep): make more similar to BDCat env for testing uc-cdis/gitops-qa#1769

Merged

PlanXCyborg mentioned this pull request Dec 14, 2021

HEAL External Data - update services to 2021.12 uc-cdis/cdis-manifest#3917

Merged

PlanXCyborg mentioned this pull request Jan 10, 2022

Gen3 Monthly Release 2022.01 acct.bionimbus.org 1641853042 uc-cdis/cdis-manifest#3982

Merged

This was referenced Mar 7, 2022

Gen3 Monthly Release 2022.03 gen3.datacommons.io 1646690027 uc-cdis/cdis-manifest#4270

Closed

OADC demo commons update release to 2022.03 uc-cdis/cdis-manifest#4303

Merged

This was referenced Mar 21, 2022

g3dc 2022.03 gen3.datacommons.io 1647895607 uc-cdis/cdis-manifest#4346

Merged

g3dc 2022.03 internal-icgc.bionimbus.org 1648239285 uc-cdis/cdis-manifest#4355

Merged

g3dc 2022.03 icgc.bionimbus.org 1648239323 uc-cdis/cdis-manifest#4356

Merged

This was referenced Mar 29, 2022

Gen3 Monthly Release 2022.04 tbi.datacommons.io 1648529943 uc-cdis/cdis-manifest#4376

Closed

Gen3 Monthly Release 2022.04 gen3-neuro.datacommons.io 1648533542 uc-cdis/cdis-manifest#4377

Closed

This was referenced Apr 30, 2022

Gen3 Monthly Release 2022.05 tbi.datacommons.io 1651309735 uc-cdis/cdis-manifest#4576

Closed

Gen3 Monthly Release 2022.05 gen3-neuro.datacommons.io 1651313335 uc-cdis/cdis-manifest#4577

Closed

This was referenced Jun 23, 2022

Gen3 Monthly Release 2022.06 tbi.datacommons.io 1655955061 uc-cdis/cdis-manifest#4792

Closed

Gen3 Monthly Release 2022.06 gen3-neuro.datacommons.io 1655958659 uc-cdis/cdis-manifest#4793

Closed

PlanXCyborg mentioned this pull request Jul 21, 2022

Update qa-anvil to 2022.07 qa-anvil.planx-pla.net - 2022.07 1658416156 uc-cdis/gitops-qa#2107

Open

PlanXCyborg mentioned this pull request Sep 14, 2022

chore/UpdateBloodpac: update servaces to match production versions uc-cdis/gitops-qa#2187

Merged

PlanXCyborg mentioned this pull request Oct 21, 2022

Gen3 Monthly Release 2022.10 dcf-interop.kidsfirstdrc.org 1666320713 uc-cdis/cdis-manifest#5281

Closed

github-actions bot mentioned this pull request Aug 15, 2024

Updated fence to expose metrics uc-cdis/cdis-manifest#7742

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(PXP-6339): RAS visa validation #933

(PXP-6339): RAS visa validation #933

vpsx commented Jun 9, 2021 •

edited

Loading

github-actions bot commented Jun 9, 2021

coveralls commented Jun 9, 2021 •

edited

Loading

vpsx Jun 10, 2021

Avantol13 Jun 10, 2021

vpsx Jun 10, 2021 •

edited

Loading

vpsx Jun 10, 2021

Avantol13 Jun 10, 2021

Avantol13 Jun 10, 2021

vpsx Jun 10, 2021 •

edited

Loading

Avantol13 Jun 10, 2021

Avantol13 Jun 10, 2021

vpsx Jun 10, 2021

Avantol13 left a comment

(PXP-6339): RAS visa validation #933

(PXP-6339): RAS visa validation #933

Conversation

vpsx commented Jun 9, 2021 • edited Loading

New Features

Breaking Changes

Bug Fixes

Improvements

Dependency updates

Deployment changes

github-actions bot commented Jun 9, 2021

coveralls commented Jun 9, 2021 • edited Loading

Pull Request Test Coverage Report for Build 11239

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpsx Jun 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpsx Jun 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avantol13 left a comment

Choose a reason for hiding this comment

vpsx commented Jun 9, 2021 •

edited

Loading

coveralls commented Jun 9, 2021 •

edited

Loading

vpsx Jun 10, 2021 •

edited

Loading

vpsx Jun 10, 2021 •

edited

Loading