Skip to content
This repository has been archived by the owner on Oct 23, 2023. It is now read-only.

[#227] implement jku checking feature #424

Merged
merged 2 commits into from
Jun 26, 2023
Merged

Conversation

gimppa
Copy link
Contributor

@gimppa gimppa commented Jun 22, 2023

Description

Related issues

Implements #227

Type of change

  • New feature (non-breaking change which adds functionality)

Changes Made

Added new config variable trusted_jkus and a check in beacon_api.ga4gh.get_ga4gh_permissions to see if passport jku is whitelisted.

Testing

  • Unit Tests

Mentions

@teemukataja helped with writing tests

@gimppa gimppa requested a review from teemukataja June 22, 2023 05:02
@gimppa gimppa self-assigned this Jun 22, 2023
@gimppa gimppa changed the base branch from master to dev June 22, 2023 05:03
@blankdots
Copy link
Contributor

might want to base this branch on feature/python_3_10 so that 3.10 changes are take into consideration

Copy link
Contributor

@blankdots blankdots left a comment

Choose a reason for hiding this comment

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

i think we need a combination of iss and jku, because this only trusts the jku not the actual issuer

@blankdots blankdots self-requested a review June 26, 2023 09:53
@blankdots
Copy link
Contributor

i think we need a combination of iss and jku, because this only trusts the jku not the actual issuer

we can do that in a different PR because:

If a Passport Clearinghouse is to use the jku URL to fetch the public keys to verify the signature, then it MUST verify that the jku is trusted for the given iss as part of the Passport Clearinghouse's trusted issuer configuration. This check MUST be performed before calling the jku endpoint.

source: https://github.com/ga4gh/data-security/blob/master/AAI/AAIConnectProfile.md#conformance-for-passport-clearinghouses

@teemukataja seems like you suggested that: neicnordic/sda-download#82 (review)

@gimppa gimppa merged commit 1f2533a into dev Jun 26, 2023
@gimppa gimppa deleted the feature/visa_issuer_check branch June 26, 2023 10:50
@teemukataja
Copy link
Contributor

i think we need a combination of iss and jku, because this only trusts the jku not the actual issuer

we can do that in a different PR because:

If a Passport Clearinghouse is to use the jku URL to fetch the public keys to verify the signature, then it MUST verify that the jku is trusted for the given iss as part of the Passport Clearinghouse's trusted issuer configuration. This check MUST be performed before calling the jku endpoint.

source: https://github.com/ga4gh/data-security/blob/master/AAI/AAIConnectProfile.md#conformance-for-passport-clearinghouses

@teemukataja seems like you suggested that: neicnordic/sda-download#82 (review)

I was thinking that we don't need to check the issuer, because only the key matters for the validity of the token (as long as we trust the key, the token is good), and this way it also makes a simpler implementation, when we can load a csv from env, and don't need to worry about the issuer, which can be just any string (but usually an address) 🤔

But the sda-download implementation is correct according to the spec 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants