FHIR Chat · docs / PR #186 Document how the JWK Set is communicated (... · cds hooks/committers

Stream: cds hooks/committers

Topic: docs / PR #186 Document how the JWK Set is communicated (...

---

Github Notifications (Mar 27 2018 at 16:15):

kpshek opened PR #186
from issue/87-public-keys to master

In addition to documenting that the JWK Set is communicated either via the standard jku header or out-of-band, I relaxed the iss value from URL to URI. This is to address deployment scenarios where the issuer cannot be a URL.

Github Notifications (Mar 27 2018 at 16:17):

kpshek edited PR #186
from issue/87-public-keys to master

In addition to documenting that the JWK Set is communicated either via the standard jku header or out-of-band, I relaxed the iss value from URL to URI. This is to address deployment scenarios where the issuer cannot be a URL.

Fixes #87

Github Notifications (Mar 27 2018 at 16:18):

kpshek review_requested PR #186

Github Notifications (Mar 27 2018 at 16:18):

kpshek review_requested PR #186

Github Notifications (Mar 27 2018 at 16:18):

kpshek review_requested PR #186

Github Notifications (Mar 27 2018 at 16:18):

kpshek review_requested PR #186

Github Notifications (Mar 27 2018 at 16:19):

kpshek synchronized PR #186

Github Notifications (Mar 27 2018 at 21:55):

isaacvetter commented on PR #186

Hey @kpshek ,

A bit of feedback; overall, it's a solid design:

1) It's important that the jku value be whitelisted by the CDS Service. This was (one of) @mattvarghese's points in #87. This url should be communicated out-of-band. The CDS Service client_id creation process already exists and would be a good time to do this.

2) Our current alg example shows HS256. For commonality in implementations, the spec should dictate that CDS Services MUST support an algorithm. We're slightly partial to EC386, but HS256 would also work fine. The important thing is to standardize it.

3) Note that in the JWK RFC, kid is not actually required to be unique. Further, when it's not unique the RFC doesn't specify the actual lookup mechanism that a client should use. The CDS Hooks spec should just state that kid values MUST be unique within the jwk set.

4) We should provide a guidance section to CDS Service developers containing the following guidelines:
* The jku and iss urls should be whitelisted within your service.
* Using the ita field, CDS Services should reject any requests older than 5 minutes.
* CDS Services should validate that a newly received jti is unique over the past 5 minutes. This prevents replay attacks.

5) Lastly, the example json should include an example jku.

Want me to help by making commits?

Isaac

Github Notifications (Mar 27 2018 at 22:02):

isaacvetter synchronized PR #186

Github Notifications (Mar 27 2018 at 22:05):

isaacvetter synchronized PR #186

Github Notifications (Mar 27 2018 at 22:09):

isaacvetter synchronized PR #186

Github Notifications (Mar 27 2018 at 22:10):

kpshek commented on PR #186

Thanks for the feedback @isaacvetter! I agree with all of your comments and had thought of most of these gaps. However, I was planning on addressing them via the ballot process since most of these comments are outside of the scope of this issue: how do we communicate the JWK Set in a standard way?

All of the guidance around how CDS Services should use these particular JWT header/payload fields, etc are fantastic. I just want to get this PR in so that we can cut a ballot version.

I really appreciate you helping out to address these additional concerns but given that they are outside of the scope of this issue, can we revert your additional comments here and put them on a separate branch for another issue please?

Github Notifications (Mar 27 2018 at 22:17):

isaacvetter synchronized PR #186

Github Notifications (Mar 27 2018 at 22:25):

isaacvetter commented on PR #186

Hey Kevin,

I think that your incorrect to state that none of these changes are relevant ...

Isaac

Github Notifications (Mar 28 2018 at 14:34):

bkaney commented on PR #186

Our current alg example shows HS256. For commonality in implementations, the spec should dictate that CDS Services MUST support an algorithm. We're slightly partial to ES386, but HS256 would also work fine. The important thing is to standardize it.

I am not at all a crypto expert, but it is my understanding that HS256 is a symmetric algorithm with only one (secret) key that is shared between the two parties. ES386 is asymmetric with a public/private key pair.

After reading this: https://auth0.com/blog/critical-vulnerabilities-in-json-web-token-libraries/ there is a catch-22 with having the algorithm be discovered in the JWT header of which is supposed to be signed. I think the upshot is the algorithm should be pre-coordinated out-of-band (or specified, as ES386). Thoughts?

Github Notifications (Mar 28 2018 at 15:21):

isaacvetter commented on PR #186

Talked with @kpshek this morning:

Keeping in this PR:
* kid should be unique
* jku example
* jku should be whitelisted

Moving to issue:
- Guidance to CDS services to validate a JWT - how to verify a JWT? (guidance around iat, jti)
- is this an imlpementation guide?
- Standardizing JWT alg.

Github Notifications (Mar 28 2018 at 15:33):

isaacvetter synchronized PR #186

Github Notifications (Mar 28 2018 at 15:49):

kpshek closed PR #186

Github Notifications (Mar 28 2018 at 21:08):

yashaskram commented on PR #186

jku should be a required field.

Github Notifications (Mar 28 2018 at 21:29):

isaacvetter commented on PR #186

Hey Raj,

The EHR MAY make its JWK Set available via a URL identified by the jku header field, as defined by rfc7517 4.1.2. If the jku header field is ommitted, the EHR and CDS Service SHALL communicate the JWK Set out-of-band.

Mind creating an issue and we can talk it through as part of the ballot? We'll support jku, but I'm not convinced that you're right.

I _think_ that right now, it's still possible for an EHR to not expose any endpoints to a CDS Hooks service and still be compliant with the spec.

Why do you think it should be required?

Isaac

Github Notifications (Mar 29 2018 at 02:23):

yashaskram commented on PR #186

Hey @isaacvetter,

First off, I have to admit that I missed to read below. Below allows to have more one option to share JWK. I agree jku can be optional with below

The EHR MAY make its JWK Set available via a URL identified by the jku header field, as defined by rfc7517 4.1.2. If the jku header field is ommitted, the EHR and CDS Service SHALL communicate the JWK Set out-of-band.

I thought issue https://github.com/cds-hooks/docs/issues/87 is to come with an one universal approach for JWK discovery. I assumed jku is the only option to share JWK and just reading the line below, I jumped to the conclusion that the spec allows sharing of JWK to be optional

jku OPTIONAL url The URL to the JWK Set containing the public key(s).

In general it is ok to allow out of band JWK sharing in addition to jku. But I am going to create an issue on this topic to gather feedback from others given the need to maintain combination of whitelist across jku, issuer and sub on the CDS service provider end vs exposing endpoint on the EHR side.

There typo in the note below.

The EHR MAY make its JWK Set available via a URL identified by the jku header field, as defined by rfc75177515 4.1.2. If the jku header field is ommitted, the EHR and CDS Service SHALL communicate the JWK Set out-of-band.

Github Notifications (Mar 29 2018 at 03:08):

isaacvetter commented on PR #186

Raj! Great catch on the rfc typo - I went ahead and created #189 for this.

I know how deeply you think about these security issues, as you think through the whitelisting considerations, they might fit well into #188. Also, your feedback is needed on #187 as well.

---

Last updated: Apr 12 2022 at 19:14 UTC