cds-hooks / docs

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

Proposed Security Model Questions/Issues #55

Open robs16 opened 7 years ago

robs16 commented 7 years ago

Reposting from Zulip to gather feedback:

In looking over the proposed security model I have a few questions that are somewhat inter-related:

  1. I may be missing something in the documentation on SMART and in the proposed security model, but I do not understand how the app pointed at by link.url authenticates to the CDS service. There is no requirement that they are the same, yet the app will generally need to access the CDS service (probably more often than the FHIR service) and will need to reference the session created by the request that provided the link.url. This applies equally to the non-SMART app. I am assuming that authentication would need to be part of the URL?

  2. How will the analytics endpoints be authenticated by the service? How will the service know which hookInstance an analytics request is about?

  3. Depending on the answers from 1 and 2, both link.url and the analytic endpoints have a fixed string which may contain authentication credentials. To maintain security, these must have a timeout and may be one-time in nature. How do we inform the EMR of this? I mean there is nothing that says that a displayed card needs to be acted on within an hour, a day, a week, month, etc.

  4. Proposed JWT aud. What does this refer to? The specific endpoint, a prefix to it, or a token request endpoint? ie: 'https://example.com/cds-services', 'https://example.com/cds-services/medication-echo', 'https://example.com/cds-services/medication-echo/analytics/C919155B-2138-4177-A316-524CB48B4EC9', 'https://example.com/token'

  5. The JWT issuer is based on the associated FHIR server. I think there is an implicit assumption that this issuer will contain the client/practice for cloud EHR providers. I am unsure if this solves all of the nuances of working with a cloud EHR vs a single EHR. Are there cases where an EHR and a FHIR server are not 1 to 1?

kpshek commented 7 years ago

Thanks for the thoughtful question, @robs16.

1. How do the CDS Service and app represented in resultant CDS cards (link.url) share state if owned by the CDS Service provider?

On our Argonaut call today we talked about some options, one of which was someone (maybe you?) was working with a system in which the SMART app launch URL could take an arbitrary parameter. This value of this parameter could be a hookInstance.

I have to think through this a bit more but my initial thought is that if we were to explore something like the aforementioned strategy this would be best addressed as a core change to the SMART specification and that we would leverage that here in CDS Hooks. However, I'd like to mull and ponder over this further to see if we can uncover additional options that leverage existing capabilities today.

2. Analytics endpoint authentication

To be honest, I haven't put much thought into our analytics endpoint. We created it after great discussion in the January 2016 Connectathon but I haven't seen much implementation on create use cases. I would like to exclude analytics from our 1.0 release and if we reach consensus on this, we can incorporate it into a future release at which point we can think through it more holistically (including how security is incorporated).

3. Security of link.url and analytics endpoints

If the CDS Service wants link.url to point to a time bounded page, it will need to construct a URL that either contains an identifier pointing to private server state that indicates the expiry time OR containers an encrypted value that contains the expiry time. If the link should be a view once only URL, this will need to be tracked server-side by the web app being the link.url. In both cases these seem like concerns of the CDS Service provider and not something that would require a specification/API change. I do think we'll have implementation guides for developers and information like this can be incorporated in that if we think this is a use case that other developers would benefit from guidance on.

I'll defer my analytics endpoint thoughts based upon my answer above.

4. aud value

This would be the fully qualified URL to the CDS Service endpoint. In our documentation, we refer to this as {baseUrl}/cds-services/{id}

5. iss value

Is your concern articulated by this issue in SMART?: smart-on-fhir/smart-on-fhir.github.io#133

robs16 commented 7 years ago

This discussion brings up a broader issue: The temporal aspect of the response needs to be well thought out. To my understanding there are no rules and no documentation which states that the result of the CDS request is valid for a limited amount of time. Depending on the EHR workflow and where the CDS request is executed, it may not be unusual for the card to take some time (hours) before it is utilized. However, taken to an extreme, the cards and all contained data could be expected to be valid literally forever. This is also compounded by the re-triggering functionality to obtain the current state.

For the CDS Service this means it must maintain hook instance state for an unlimited amount of time. It also negatively impacts the security of the resulting integration (items 1 and 3).

I don't think this is the intention, so there needs to be documentation which defines theses constraints and/or provides them within the API. As a solution, I propose that per card or perhaps at the response level there is a required field which defines the expiration time of the contained data. It might be also useful/necessary to have the EHR define in the request how long they need the CDS service to maintain state for the provided hookInstance.

1. How do the CDS Service and app represented in resultant CDS cards (link.url) share state if owned by the CDS Service provider? We have given some thought to transferring the hook instance in the URL. The issue with this is that the hook instance is then treated as an actual bearer credential, which is reasonably secure only if it is both random and expires rather quickly.

Also of note is that links don't have to lead to a SMART App, but may indeed have the same need for credentials or expiration. Ideally any solution covers both SMART and !SMART Apps.

2. Analytics endpoint authentication I would like to see the analytics endpoint kept in the 1.0 proposal as it serves as a primary way for the CDS Service to determine which suggestions were followed. I didn't realize there was any plan to remove it. I would propose as a solution to my questions the following: Use the same authentication/authorization mechanism for the CDS endpoints on calls to the analytic endpoints (JWT). Rather than include the UUID in the URL, include the hook instance and UUID in the POST body.

3. Security of link.url and analytics endpoints They are actually a concern for both the EHR and CDS Service because navigating to an expired link will probably result in a 4xx error and will only serve to negatively impact the clinician's time (as well as their view of the CDS service and/or EHR). Ideally the EHR knows if a link has 'expired', so the hook instance can be re-triggered to get valid links before such navigation.

I see this as an API issue because without a mechanism to define how long the response/card/link is valid, each integration must be customized between the service provider and EHR vendor.

4. aud value Ok. So this implies that the discovery endpoint should not be secured, correct?

5. iss value To some degree, yes. It drives to clarify the entity asking for the service and considers FHIR server base urls with multiple tenants. It indicates that the answer to my question is 'yes'. However no conclusion appears to be reached. So, it needs to be concluded and the resulting solution applied to the JWT provided to the CDS Service.

robs16 commented 7 years ago

So based on the security updates and comments on the call:

  1. solved (will have a JWT sent),
  2. solved (aud will be the oauth client id, discovery endpoint will be secured)
  3. Really is not going to be solved - tenancy will be custom

Any thoughts on the temporal aspects, 1 and 3 @kpshek ?