verifiable consent of visa usage #40

dvoet · 2020-02-14T15:43:07Z

When considering implementation of the AAI spec, we have a concern that there is no way for a clearinghouse to verify that the user has consented to allow a visa to be used by a broker. As the spec currently stands users who trust a Broker A effectively must trust all other brokers that Broker A trusts and so on down the trust chain. The user has no visibility into or mechanism to limit this.

This addition to the spec hopes to achieve this by adding audiences to Embedded Access Tokens or Embedded Document Tokens. Users can consent to a list of brokers when the tokens are generated. If the audience field does not exist then there is no restriction. How the token issuer knows what list to present to the user is unspecified.

Example embedded access token payload:

{
 "iss": "https://<embedded-token-issuer-website>/",
 "sub": "<subject-identifier>",
 "aud": [
  "https://<consented-broker1-website>/",
  "https://<consented-broker2-website>/" ...
 ],
 "iat": <seconds-since-epoch>,
 "exp": <seconds-since-epoch>,
 "jti": <token-identifier>,
 "scope": "openid <ga4gh-spec-scopes>",
}

Example access token issued by broker to clearinghouse:

{
 "iss": "https://<consented-broker1-website>/",
 "sub": "<subject-identifier>",
 "iat": <seconds-since-epoch>,
 "exp": <seconds-since-epoch>,
 "jti": <token-identifier>,
 "scope": "openid <ga4gh-spec-scopes>",
}

In this example, the clearinghouse MUST verify that the aud field in the embedded access token payload contains the iss field of the broker access token.

davidbernick · 2020-02-14T19:08:18Z

I'd like @cdvoisin to take first pass. Might also be worth having @mikael-linden look and chat...

davidbernick · 2020-02-14T20:02:16Z

@dvoet please add versioning info on top

mikael-linden · 2020-02-27T08:14:41Z

Looks reasonable. Aud is a common OIDC practice to restrict access token consumers.

davidbernick · 2020-02-28T16:30:48Z

Great if Mikael is a thumb, then once Craig says yes we're good to go.

cdvoisin · 2020-03-06T20:01:19Z

AAI/AAIConnectProfile.md

@@ -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`


"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.

cdvoisin · 2020-03-06T20:05:12Z

AAI/AAIConnectProfile.md

+
+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`
+    field must match the `iss` field of OIDC access tokens issued by consented Brokers.


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.

cdvoisin · 2020-03-06T20:07:44Z

AAI/AAIConnectProfile.md

@@ -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


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

cdvoisin · 2020-03-06T20:23:17Z

AAI/AAIConnectProfile.md

@@ -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
+                that the Embedded Token's `aud` contains `iss` of access token provided by Broker.


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)"

cdvoisin · 2020-03-06T20:27:35Z

AAI/AAIConnectProfile.md

@@ -628,6 +635,8 @@ where:

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

+3. JWT payload MAY contain `aud` to list approved brokers.


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.

cdvoisin · 2020-03-06T20:28:42Z

AAI/AAIConnectProfile.md

@@ -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


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.

dvoet · 2020-03-10T13:35:55Z

@cdvoisin I think I addressed your comments. Please take another look. Thanks.

davidbernick · 2020-03-24T19:44:05Z

@cdvoisin -- can you take a look?

cdvoisin · 2020-03-24T21:02:26Z

AAI/AAIConnectProfile.md

+3.  An Embedded Token Issuer MAY include the `aud` field to identify the 
+    [Brokers](#term-broker) as the intended audience as specified by 
+    [RFC 7523 Section 3](https://tools.ietf.org/html/rfc7523#section-3).
+    The values within the `aud` field must match the `iss` field of access tokens 


Suggestion just to make language clear. Given this is a MAY, and then have a sentence here saying "the values ... must match". How about:

"When an aud claim is specified, the values within the aud claim MUST match the iss claim for the Broker that includes the embedded token."

cdvoisin · 2020-03-24T21:02:42Z

AAI/AAIConnectProfile.md

@@ -396,6 +397,12 @@ 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 identify the 


"field" -> "claim"

cdvoisin · 2020-03-24T21:07:53Z

@dvoet almost there, just a couple more nits for you to look at. Ping me when ready.

davidbernick

Everyone okay with this now?

cdvoisin · 2020-03-30T19:38:58Z

AAI/AAIConnectProfile.md

+                that one of the Embedded Token's `aud` entries 
+                [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).


aud -> aud

martin-kuba · 2020-03-31T09:09:12Z

AAI/AAIConnectProfile.md

@@ -429,6 +436,11 @@ 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 an Embedded Token contains an `aud` claim, Clearinghouse MUST check
+                that one of the Embedded Token's `aud` entries 
+                [matches](https://tools.ietf.org/html/rfc3986#section-6.2.1) the Broker's 


Why broker's? The "aud" claim means "audience", so it should match the clearing house's client id, not the broker's issuer id. The audience ("the recipients processing the JWT" as per RFC 7519#4.1.3) is the clearing house, not the broker.

Please see the PR summary for what I am trying to achieve.

In addition to allowing the aud claim to contain brokers we could say it must also contain clearing houses then the clearing house must verify that it is in the list itself as well as the broker. Essentially the visa issuer would specify all the parties, brokers and clearing houses, who are allowed to use the JWT. This spec would then go above and beyond the JWT RFC by saying the clearing house much check that the broker is in the aud claim.

If aud is not the right claim to use then I would propose adding a new claim to the spec and registering that with IANA.

Defining a new claim seems to be a better way to me.

itsJiaqi · 2020-09-11T17:20:54Z

I think adding aud as a claim would be beneficial for a variety of use cases so certainly no objections from me on this PR.

However, this doesn't address consent or verifiable consent at all.

As I understand it, consent is an agreement between the user and an organization or company (a legal entity).

In the proposed flow, the organization needs to obtain the user’s data (their Visa) and parse it in order to verify if the user has consented or not. That’s already too late. The organization should not even have access to the user’s data without knowing that the user has consented. Verification of user consent needs to happen prior to the user’s data being shared.

davidbernick · 2020-09-13T19:52:42Z

A key philosophy that we ask across the AAI and Passport, without asking, is "Should a JWT, signed from a trusted source, full of visas from sources that can be checked, sufficient for Authentication?"

It might help us to include things about JWT-only auth, such as that it is assumed that JWT-only auth works in a fairly closed network of sources where it is ensured that the User understands that their JWT might be used be these related downstream systems. These systems are NOT 3rd parties in the traditional sense. Maybe I'll write something and add it to the spec, at least as an explanation.

It is NOT a replacement for OIDC/Oauth but rather an expression of them upstream. It is NOT the intention of AAI/Passports ONLY to "just be OIDC", but rather an alternative. JWT-only auth IS allowed by AAI/Passport provided that risk is appropriately assessed.

For issues brought up here, #40, we are trying to clarify that GA4GH explicitly supports JWT-only auth though it is not the most recommended way to use it.

davidbernick · 2020-09-25T19:24:22Z

Hi @jiaqi216, I've started this PR #41 to make sure that the GA4GH spec explicitly mentions JWT_only auth as a possible, and not recommended, method of auth. I feel, with your feedback, that it was lacking to have an explicit callout.

verifiable consent of visa usage

e36ca3e

dvoet requested review from cdvoisin and davidbernick February 14, 2020 15:43

add version info

e5dc9ad

davidbernick requested a review from mikael-linden February 22, 2020 17:29

cdvoisin requested changes Mar 6, 2020

View reviewed changes

address PR comments

a0fbb0b

cdvoisin requested changes Mar 24, 2020

View reviewed changes

dvoet added 2 commits March 30, 2020 09:44

minor language updates

9debd79

Merge branch 'master' into master

303994e

davidbernick approved these changes Mar 30, 2020

View reviewed changes

cdvoisin approved these changes Mar 30, 2020

View reviewed changes

martin-kuba requested changes Mar 31, 2020

View reviewed changes

davidbernick added a commit that referenced this pull request Sep 25, 2020

First pass at fixing JWT-only access

e27cafd

For issues brought up here, #40, we are trying to clarify that GA4GH explicitly supports JWT-only auth though it is not the most recommended way to use it.

davidbernick mentioned this pull request Sep 25, 2020

Clarification for JWT-only access #41

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

verifiable consent of visa usage #40

verifiable consent of visa usage #40

dvoet commented Feb 14, 2020

davidbernick commented Feb 14, 2020

davidbernick commented Feb 14, 2020

mikael-linden commented Feb 27, 2020

davidbernick commented Feb 28, 2020

cdvoisin Mar 6, 2020

cdvoisin Mar 6, 2020

cdvoisin Mar 6, 2020

cdvoisin Mar 6, 2020

cdvoisin Mar 6, 2020 •

edited

Loading

cdvoisin Mar 6, 2020

dvoet commented Mar 10, 2020

davidbernick commented Mar 24, 2020

cdvoisin Mar 24, 2020

cdvoisin Mar 24, 2020

cdvoisin commented Mar 24, 2020

davidbernick left a comment

cdvoisin Mar 30, 2020

martin-kuba Mar 31, 2020

dvoet Mar 31, 2020

martin-kuba Mar 31, 2020

itsJiaqi commented Sep 11, 2020

davidbernick commented Sep 13, 2020

davidbernick commented Sep 25, 2020

		@@ -628,6 +635,8 @@ where:

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

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

verifiable consent of visa usage #40

Are you sure you want to change the base?

verifiable consent of visa usage #40

Conversation

dvoet commented Feb 14, 2020

davidbernick commented Feb 14, 2020

davidbernick commented Feb 14, 2020

mikael-linden commented Feb 27, 2020

davidbernick commented Feb 28, 2020

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

cdvoisin Mar 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoet commented Mar 10, 2020

davidbernick commented Mar 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdvoisin commented Mar 24, 2020

davidbernick left a comment

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

Choose a reason for hiding this comment

itsJiaqi commented Sep 11, 2020

davidbernick commented Sep 13, 2020

davidbernick commented Sep 25, 2020

cdvoisin Mar 6, 2020 •

edited

Loading