FHIR Chat · docs / PR #72 Define security model · cds hooks/committers

Stream: cds hooks/committers

Topic: docs / PR #72 Define security model

---

Github Notifications (Jul 27 2017 at 18:04):

kpshek opened PR #72
from issue/7-security to master

This is an in-progress branch -- please don't bother reviewing right now. The purpose for issuing this pull request early is to get Heroku review apps deployed.

Github Notifications (Aug 10 2017 at 16:14):

kpshek synchronized PR #72

Github Notifications (Aug 10 2017 at 20:04):

kpshek synchronized PR #72

Github Notifications (Aug 31 2017 at 19:51):

kpshek synchronized PR #72

Github Notifications (Aug 31 2017 at 21:04):

kpshek commented on PR #72

Thanks for the great suggestions & feedback @olbrich - I've replied to each inline above

Github Notifications (Sep 08 2017 at 21:27):

kpshek synchronized PR #72

Github Notifications (Sep 09 2017 at 04:13):

kpshek synchronized PR #72

Github Notifications (Sep 09 2017 at 13:12):

kpshek synchronized PR #72

Github Notifications (Sep 09 2017 at 17:21):

kpshek synchronized PR #72

Github Notifications (Sep 09 2017 at 17:43):

kpshek synchronized PR #72

Github Notifications (Sep 10 2017 at 15:22):

kpshek commented on PR #72

At the San Diego Connectathon this weekend (2017-09-09), a large group of us participating in the CDS Hooks track discussed this issue in an offline discussion. This group of ~30 represented a broad set of stakeholders, from multiple EHR vendors, several CDS Service providers, healthcare organizations, and those representing various HL7 working groups. What follows is the summary of our discussion and the consensus of the group.

We affirmed our desire to define a standard approach to security within CDS Hooks. Additionally, our current proposed strategy has received overall positive implementer feedback. Particulars around the JWT did receive feedback that will warrant changes. Today, our JWT is based upon the JWT that SMART uses as part of the OpenID Connect workflow to assert a user identity. Based upon implementation feedback and considerations, we agreed that our use of a JWT is not to establish user identity but rather a bearer token for the CDS Service to authorize an EHR. As such, the content of our JWT would change, the details of which are below.

• sub: remove this field as our JWT is not trying to assert a user identity
• iss: Change this from the FHIR server URL to the URL of the actual issuer. This could still be the FHIR server but not guaranteed. For instance, in the case of the CDS Hooks Sandbox, the issuer would change to http://sandbox.cds-hooks.org. The reason for this change is that the iss value should be the entity responsible for signing the JWT. In the case of the CDS Hooks Sandbox, or other CDS Hooks systems which are capable of connecting to multiple/arbitrary FHIR servers, having the issuer be strictly the FHIR server is wrong.
• aud: Change this to the URL of the CDS Service being invoked. Previously, this was the OAuth 2 client id of the CDS Service but the consensus of the community was that the CDS Service does not need to know this value. Additionally, making the audience the URL of the CDS Service being invoked will protect against JWTs be sent to incorrect audiences.
• jti: Add this new field which is a nonce (UUID) to protect against replay attacks.

I'll be updating the documentation to reflect these changes shortly.

Github Notifications (Sep 10 2017 at 16:19):

kpshek synchronized PR #72

Github Notifications (Oct 02 2017 at 22:13):

brynrhodes review_requested PR #72

Github Notifications (Oct 03 2017 at 01:49):

isaacvetter commented on PR #72

Kevin,

I re-read the whole security section and came up with one substantive suggestion and many non-substantive suggestions.

== Substantive ==
This sentence:

As such, the access token should be scoped to:
The CDS Service being invoked
The current user
The current patient

I don't think that the access_token should be limited to a specific patient. The CDS Service is acting on behalf of the user and therefore should have the same level of data access as the current user.

==Non-Substantive ==

1. Replace "The proposed security model has not yet received implementer feedback and as such, is subject to change" with "The CDS Hooks security model is undergoing a rigorous security assessment and as such, may be subject to change"

1. The language in the security section uses conditional sentences, which weaken the specification and use more words than necessary. Notably, look at the use of the word "should". Some of these uses should be replaced with current tense, factual statements, using words like "is" and "does". Let's only use "should" when there really is a choice to be made by an implementer.
* Each time the EHR makes a request to the CDS Service, it _should send_ an Authorization header
* Each time the EHR makes a request to the CDS Service, it _sends_ an Authorization header

• When the EHR invokes the CDS Service discovery endpoint, the aud value _should be either_
• When the EHR invokes the CDS Service discovery endpoint, the aud value _is either_

1. These two sentences are overly complicated: "Mutual TLS (mTLS) can be used between an EHR and CDS Service and that would allow the CDS Service to establish trust of the EHR. However, if mTLS is used, this should be in addition to using JSON web tokens to establish trust of the EHR." Replace with:
"Mutual TLS (mTLS) _may be used alongside JSON web tokens to establish trust of the EHR by the CDS Service_. "

1. Using the slate doc column format (instead of the MkDocs inline code snippets), this sentence is pretty confusing: "Try taking the example JWT here and pasting it into the form at https://jwt.io/ to see how the token is constructed."

1. " TODO: Need to outline the risks of noalg signatures" Do we really?

1. Suggest replacing " TODO: Need to propose how the EHR public key is discovered. Preference would be a further extension in the EHR FHIR server Conformance resource." with " TODO: CDS Hooks doesn't yet specify how a CDS Service discovers the public key that the EHR uses to sign the JWT. "

1. "likely result in a slow performing CDS Service due to the authorization overhead." should either use the term "slow" or "poorly performing" in place of "slow performing".

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

isaacvetter labeled PR #72

Github Notifications (Oct 23 2017 at 20:11):

kpshek synchronized PR #72

Github Notifications (Oct 30 2017 at 19:24):

kpshek synchronized PR #72

Github Notifications (Oct 30 2017 at 19:42):

kpshek synchronized PR #72

Github Notifications (Oct 30 2017 at 19:52):

kpshek commented on PR #72

@Isaac Vetter & @brynrhodes - I've addressed all of your comments in 3cb827a.

Could you guys both take a look at this document and approve the merge if it looks good?

Github Notifications (Oct 30 2017 at 20:17):

kpshek synchronized PR #72

Github Notifications (Oct 30 2017 at 20:29):

kpshek synchronized PR #72

Github Notifications (Oct 30 2017 at 20:41):

kpshek synchronized PR #72

Github Notifications (Oct 30 2017 at 21:00):

kpshek closed PR #72

---

Last updated: Apr 12 2022 at 19:14 UTC