search

cds-hooks / docs

CDS Hooks website & specification
http://cds-hooks.org
Apache License 2.0
166 stars 62 forks source link

Provide guidance for where CDS Services can retrieve the EHR's public key #87

Closed brettmarquard closed 6 years ago

brettmarquard commented 7 years ago

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

isaacvetter commented 6 years ago

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?

kpshek commented 6 years ago

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

kpshek commented 6 years ago

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.

grahamegrieve commented 6 years ago

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?

isaacvetter commented 6 years ago

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

mattvarghese commented 6 years ago

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

JWT Body:

Regardless of how the key is distributed, the CDS-Service MUST 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 (CDS-Service MUST validate this constraint). 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.
  5. This is an option we MUST NOT select, but since it was discussed at the connectathon, including here. This option is to just include the key in the body of the JWT. This is a bad option because, this does not give any way to validate that the key does indeed belong to the issuer that the caller is claiming to be.

Further thoughts

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

jmandel commented 6 years ago

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.

yashaskram commented 6 years ago

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

mattvarghese commented 6 years ago

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, renaming iss in option 2(c) to jku 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 JWT RFC and 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 (in other words, this MUST be whitelisted by services that charge / care about who is calling them. A publicly available service MAY say that the validation is 'any jku url is acceptable'). 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. (1) is defined as required by the CDS-Hooks spec, (2)&(3) are optional/good-practice up to the discretion of the CDS-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.

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

kpshek commented 6 years ago

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

  1. 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).

isaacvetter commented 6 years ago

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.

kpshek commented 6 years ago

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.