FHIR Chat · docs / Issue #87 Provide guidance for where CDS Services ... · cds hooks/github

Stream: cds hooks/github

Topic: docs / Issue #87 Provide guidance for where CDS Services ...

---

Github Notifications (Sep 09 2017 at 20:23):

brettmarquard opened Issue #87

Open to suggestions -- but ideally we will have a few concrete proposals before discussing on a call.

Github Notifications (Sep 09 2017 at 20:25):

kpshek milestoned Issue #87

Github Notifications (Oct 02 2017 at 21:50):

isaacvetter commented on Issue #87

There only two choices:
1) how SMART does it -- in the version of SMART that's balloting into HL7
2) in the Conformance / CapabilityStatement at /metadata

What are pros and cons?

Github Notifications (Oct 02 2017 at 21:53):

isaacvetter assigned Issue #87

Github Notifications (Oct 02 2017 at 21:53):

kpshek commented on Issue #87

We also have secret option 3...leave this purposely undefined and swing back on this post 1.0

Github Notifications (Oct 19 2017 at 20:56):

brettmarquard labeled Issue #87

Github Notifications (Oct 20 2017 at 03:36):

kpshek commented on Issue #87

In an offline conversation with @yashaskram, he brought up some great points around this topic from our Connectathon testing in San Diego:

Every EHR have taken slightly different approach. For example: Cerner is using .pem format for public key and ES256 signing algorithm (Connectathon testing). Athena is using .pub format for the public key and RS256 (Connectathon testing). T-Systems is using .cer format of public key and RS256. But originally what I was hoping to receive from EHR is jwk format. Do you think we can get consensus on the public key format? And also wondering if the specification should help narrow signing algorithms.

Format of PublicKey
- .pub,
- .pem
- .jwk (json)
- .cer

Signing Algorithm
- "id_token_signing_alg_values_supported": ["HS256","HS384","HS512","RS256","RS384","RS512","ES256","ES384","ES512","PS256","PS384","PS512","none"

UpToDate prototype can currently supports HS256, RS256 and ES256. And also I have validated that our key validation library can support .pub, .pem, .cer, jwk formats.

Github Notifications (Oct 22 2017 at 19:55):

grahamegrieve commented on Issue #87

jwks would always be my preference, but for clarity, when does the cds service need the EHR's public key? we're not talking about SSL here are we?

Github Notifications (Oct 26 2017 at 21:38):

isaacvetter commented on Issue #87

@yashaskram, @kpshek - agree that the spec should dictate jwk.
@grahamegrieve - the public key is used by the cds service to validate the signature of the jwt provided by the EHR according to #72 and specifically this change.

Github Notifications (Feb 01 2018 at 20:30):

mattvarghese commented on Issue #87

So we had a few discussions with our security developers around JWT and what it serves to do, as well as, I had a conversation with @kpshek at the connectathon as well. Adding all of that here as a summary.

• The purpose of the JWT is to authenticate the EHR or other CDS-Hooks client to the CDS Service, as an entity that is allowed to make a CDS-Hooks request of the CDS Service. Making sure that we are all agreed on this.

The choices for how the CDS Service acquires the public key of the client to validate the signature on the JWT is somewhat 'handwaved' in the JWT specs. The onus of verifying that the public key indeed belongs to a legitimate client is the responsibility of CDS Service - however, how that is accomplished is not mandated or clarified as I understand. So I will request that whatever key distribution mechanism we agree upon MUST clarify (a) how the CDS Service will validate that the key belongs to the calling client and (b) the key belongs to a client authorized to call the CDS Service.

As I understand, the following properties in the JWT are relevant.

JWT Header:
- kid: this is an identifier for the key. (In our implementation, we use a base64 encoded thumbprint)
- jku: this is the url pointing to a JWK file/object containing the keys, one of which is our key identified by the kid. As I understand, currently we don't use this in CDS-Hooks.

JWT Body:
- iss: this is the issuer of the JWT. Currently, we have this match the FHIR server url. However, in a conversation with @kpshek I pointed out that there is no particular reason for the iss to be a URL, unless the JWK is at a url constructed off of the iss. @kpshek suggested that this could even be a URI.
- sub: this is more vague. As I understand, we are currently talking of this being a client ID of the EHR, issued to the EHR by the CDS-Service.

Regardless of how the key is distributed, the CDS-Service will need to keep a whitelist of the iss (or another option would be to whitelist the sub) to make sure that only authorized services are calling it. Before matching the iss in the JWT against this whitelist, therefore, the CDS-Service MUST validate that the JWT is signed by a key that indeed belongs to the iss.

Some of the options for key distribution as we discussed are:
1. Have the JWK url be in the jku in the JWT header. (in this case iss can just be a urn, and not a url) The concern here is that, if I am a hacker, I can publish the public key on the internet, and create a JWT which contains a valid iss but where the key does not match the iss. To validate the key, the CDS-Service is now required to store the url of the JWK in the whitelist alongside the iss, or the key must be backed by a Certificate Authority. IF the former, then the jku in the JWT header becomes redundant. IF the latter, that introduces its own set of inconveniences.
2. Have iss be a URL, and the key be published as a JWK at specified URL with the iss as the base/prefix of it. (in this case, basing the whitelist off sub won't work). In this case, we have the option to (a) specify/fix the suffix part of the JWK url in the standard, in which case JWT will not need to contain jku, or (b) the suffix can be variable, in which case the JWK url is in the jku, but is constrained such that iss forms the prefix of this url. The concerns are option (a) is rigid whereas option (b) allows a hacker who has access to some publicly accessible server within the organization to host a public key which doesn't belong to the org.
3. Have JWK url be part of conformance/metadata. This however means the JWT is intricately tied to the FHIR server.
4. Not define the key distribution mechanism. When an EHR sets up an integration with a CDS-Service, they have to exchange keys, and figure out the mechanism for renewing or updating keys. Personally, I like this option the least.

Further thoughts
- If we do go with option 1, I will suggest that the spec MUST require CDS-Services to validate that the key does indeed belong to the issuer that the caller is claiming to be.
- If we use any jku, whether option 1 or 2b, Section 4.1.2 and Section 8 of the JWT spec RFC MUST not be ignored.

---

All capitalized instance of MUST used according to RFC 2119 in this comment.

Github Notifications (Feb 01 2018 at 20:44):

jmandel commented on Issue #87

That's a delightful, clear, and comprehensive write-up, @mattvarghese -- thanks for working through this so clearly.

I'd add that in some use cases, the EHR (i.e the CDS Services client) might not be able to host a key in a web location that's routable from the CDS Service, so there's probably a fallback option of "just negotiate it out of band". But I agree that we want to choose one preferred in-band approach.

Personally I don't mind the rigidity of a URL that's fixed relative to issuer (i.e. your option 2a), but I'd be interested to hear if others have practical concerns.

Github Notifications (Feb 01 2018 at 23:14):

yashaskram commented on Issue #87

The choices for how the CDS Service acquires the public key of the client to validate the signature on the JWT is somewhat 'handwaved' in the JWT specs.

I agree

The way OpenID Connect handles this is through OpenID connect discovery specification https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata

jwks_uri is one of the provider metadata value which is the URL of the JSON Web Key Set [JWK] document. Same mechanism is specified to serve token_endpoint and authorization_endpoint in the OpenID Connect world. The latter (token_endpoint & authorization_endpoint) is done through FHIR conformance in the case of SMART on FHIR. Using FHIR conformance for jwks_uri (Option 3) forces CDS service to make two round trips- first to get the jwks_uri and second to get the jwk which can get expensive when keys are rotated frequently

I am inclined for option 2a as for the preferred approach but look forward to seeing other responses

Github Notifications (Feb 02 2018 at 19:11):

mattvarghese commented on Issue #87

Thinking more about this, and from a few more internal discussions, here's another update.

Option 2 above has iss as prefix of the JWK url. 2(a) has a fixed suffix, 2(b) has a variable suffix. Adding an option 2(c) where there is NO suffix - the iss is the JWK url itself. This is currently possible, because there is no other purpose for any url in the iss today.

Based on that, this leads to an Option 6 - the url is in the jku. This is different from Option 1 in that the whitelist of who is allowed to access the CDS-Service must be based on the jku rather than on the iss now. That leaves the iss as optional in the JWT. One good thing here is, now we retain the pro of option 1 which is we are using a field intended by the RFC to convey the JWK url for that purose, while not inheriting the disadvantages of option 1 because our whitelist is based on jku and not iss. This is convenient because if we then define the iss as the client ID of the issuer (and sub as the client ID of the caller, which is already the case) - that will align with other use cases of JWT such as backend services. That way EMRs will not have to keep multiple incompatible code-paths for generating JWTs.

The more we discussed this internally, the option 6 felt the best to us. If we go this route, (1) the CDS-Service MUST validate that the jku is a URL of someone that is known to them as authorized to issue JWTs for the purpose of calling the service. Additionally, the CDS-Service MAY choose to be more stringent in its validation and check that (2) the client ID communicated in the iss matches the JWK url in jku and that (3) client ID in the sub is authorized to call the service.

I am also proposing including a ver attribute in the JWT header. This can be "CDS-Hooks-JWT-v1.0" or some such string. This will allow us to finalize CDS-Hooks spec 1.0 while still being able to revise the JWT if we see security concerns arise in the future. The JWT MUST be of ver = "CDS-Hooks-JWT-v1.0" for CDS-Hooks version 1.0. Future revisions of CDS-Hooks spec MAY support additional versions of the JWT and/or MAY remove support of older versions of JWT.

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

kpshek commented on Issue #87

Thanks @mattvarghese for these write ups and my apologies for not responding earlier. This is the last big 1.0 issue I want to wrap up before our upcoming HL7 ballot and I've been doing some thinking on this.

I've distilled @mattvarghese's options down to two that I want to run by everyone:

1. iss only

iss MUST be either a URI or URL and is used by CDS Services to uniquely identify and whitelist trusted EHRs. If the iss value is a URI, the JWK Set is negotiated out-of-band with CDS Services. Otherwise, if the iss value is a URL, the JWK Set must be available via {{iss}}/jwks.json

2. iss and new jku header

iss MUST be a URI and is used by CDS Services to uniquely identify and whitelist trusted EHRs.

The JWT header MAY contain a jku field that contains a publicly accessible URL to the JWK Set (per rfc7515 section 4.1.2). If a jku header is not present, the JWK Set is negotiated out-of-band with CDS Services.

---

Personally, I'm leaning towards this second option (adding a new jku header).

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

isaacvetter commented on Issue #87

In conversation with @brynrhodes , @brettmarquard , @kpshek - Note that PR #174 already standardized the format of the public key. With consensus, it would be nice to standardize the location of this key as part of 1.0.

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

kpshek commented on Issue #87

I have a pull request (#186) based upon the second option for review for anyone interested.

@mattvarghese - Thanks again for writing all of this up and for calling out the jku header which I think is a great option. Let's split off a notion of a ver header as a separate issue if you feel that is still worth pursuing.

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

kpshek closed Issue #87

---

Last updated: Apr 12 2022 at 19:14 UTC