Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

verifiable consent of visa usage #40

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

verifiable consent of visa usage #40

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e36ca3e
verifiable consent of visa usage
dvoet Feb 14, 2020
e5dc9ad
add version info
dvoet Feb 14, 2020
a0fbb0b
address PR comments
dvoet Mar 10, 2020
9debd79
minor language updates
dvoet Mar 30, 2020
303994e
Merge branch 'master' into master
dvoet Mar 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions AAI/AAIConnectProfile.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

| Version | Date | Editor | Notes |
|---------|---------|--------------------------------------------|-------------------------|
| 1.0.2 | 2020-02 | Douglas Voet | Add audiences to embedded tokens |
| 1.0.1 | 2019-10 | David Bernick | Clarify that non-GA4GH claims are allowed in tokens |
| 1.0.0 | 2019-10 | Approved by GA4GH Steering Committee | |
| 0.9.9 | 2019-10 | David Bernick, Craig Voisin, Mikael Linden | Approved standard |
Expand Down Expand Up @@ -396,6 +397,10 @@ the Broker.
presented and/or transformed without misrepresenting the original intent,
except for accommodating for `exp` timestamps to be represented as
indicated above.

3. An Embedded Token Issuer MAY include the `aud` field to indicate which
[Brokers](#term-broker) were consented by the user. The values within the `aud`
Copy link
Collaborator

Choose a reason for hiding this comment

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

"consent" has its own flavor of interpretation that may be both too broad (perhaps in GDPR) and too narrow (in that it doesn't have to involve a user). For the Passport spec, the relationship between the Broker and the Visa Issuer is purposely unspecified except to follow this spec. Agreed, there is a MAY here, but it won't have much meaning if the wording covers a more narrow use case.

Instead of "to indicate which Brokers were consented by the user", I would pivot "to identify the Brokers as the intended audience as specified by RFC 7523 Section 3".

Although this seems more vague, it does provide more coverage for who can make use of this statement such that the network can handle the "how" and "what" the user interaction is that leads to this result.

field must match the `iss` field of OIDC access tokens issued by consented Brokers.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Based on previous feedback on the spec, we try to avoid the term "OIDC access token" due to the various layers of specs involved. Just "access tokens" here is preferred based on those previous nits and not to undo that previous nit-fixing work.


#### Conformance for Claim Clearinghouses (consuming Access Tokens to give access to data)

Expand Down Expand Up @@ -426,6 +431,8 @@ the Broker.
any other Broker involved in the propagation of the claims to
also be trusted if the Claim Clearinghouse needs to restrict its
trust model).
2. If Embedded Token contains `aud` field, Clearinghouse MUST check
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add "an" to yield: "If an Embedded Token".

that the Embedded Token's `aud` contains `iss` of access token provided by Broker.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couple of things here:

  • contains is too vague, it must be an exact match as per https://tools.ietf.org/html/rfc3986#section-6.2.1
  • "access token provided by Broker" can be ambiguous in cases where the there are multiple brokers involved or multiple embedded tokens that happen to be signed by one or more brokers. The intent comes across, but it just isn't tightly worded.

To fix:
"[matches] (https://tools.ietf.org/html/rfc3986#section-6.2.1) the Broker's iss claim (i.e. a Broker's access token iss claim must match the aud claim within its Embedded Tokens if the Embedded Token aud claim is provided)"


3. MUST check `exp` to ensure the token has not expired.

Expand Down Expand Up @@ -628,6 +635,8 @@ where:

2. The header MUST NOT contain a `jku`.

3. JWT payload MAY contain `aud` to list approved brokers.
Copy link
Collaborator

@cdvoisin cdvoisin Mar 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This section is for headers and doesn't carry aud (even though the sentence mentions payload). To solve, delete this line and leave the "payload" line that already exists below.

Let me know if your intent is to replicate claims here: https://tools.ietf.org/html/draft-ietf-oauth-json-web-token-32#section-5.3

This should be done with caution if using the aud field.


Payload format:

```
Expand Down Expand Up @@ -701,6 +710,8 @@ Issuer.
- `<ga4gh-spec-claims>`: OPTIONAL. One or more GA4GH Claims MAY be
provided. See [Authorization/Claims](#authorizationclaims) for an
example.

- MAY contain `aud` to list approved brokers
Copy link
Collaborator

Choose a reason for hiding this comment

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

micro-nit: please add 2 spaces after the dash ("-") to align with the other entries above and add period punctuation at the end for consistency.


#### Authorization/Claims

Expand Down