cds-hooks / docs

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

Feedback on Security Proposals #101

Open stefan-evinance opened 6 years ago

stefan-evinance commented 6 years ago
isaacvetter commented 6 years ago

Hey @stefan-evinance ,

Overall, the current approach doesn't exclude an EHR from using a 3rd party authentication provider, it simply doesn't define the interaction between the EHR and this provider. Note that this is the same approach as SMART.

The defined approach (a jwt in the authorization http header) is an authentication mechanism that's reasonable for my EHR as is the OAuth2 access_token for authentication.

We do have a fair number of EHRs involved in the review, testing and creation of this specification. Can you help us get the feedback from these additional EHRs that you're suggesting is needed?

The proposed CORS configuration is clearly identified as for testing ...

You should carefully consider how you support CORS, but a quick starting point for testing would be to ensure your CDS Service returns the following HTTP headers:

I think that it's outside the scope of this spec to instruct implementers on CORS configuration. Do you think that this section should be removed?

Isaac

kpshek commented 6 years ago

Thanks for the feedback @stefan-evinance!

Trusting EHRs: usually JWTs are issued and signed by a third party, not by the party using the token. As mentioned in the spec, discovering the EHR's public key is not covered by the proposal, yet it is critical. The proposed approach is generic, but in practice, based on our experience with some EHRs, it may not be doable. EHRs may already work with existing authentication providers and would likely prefer to give the JWT issuing responsibility to the 3rd party authentication provider. This makes integration less generic. We should understand the authentication mechanisms used by major EHRs and take that into consideration.

We have #87 to address how public keys are discovered to address your concern. The JWT allows the CDS Service to establish whether the entity making a request to it is a trusted EHR. We are not using the JWT to authenticate or assert an identity of a user.

FHIR Resource Access: in principal I agree with the proposal but in practice our experience with some EHRs shows that liability is more important than performance. An EHR may prefer to not be involved in the authentication flow between the CDS Service and the user (physician). While the SMART standard may cover this requirement it also imposes a number of additional requirements on the EHR hosting the SMART app, hence may not be a suitable replacement for CDS Hooks. Currently we have implemented custom FHIR Resource Access logic. I would like the CDSHooks standard or FHIR Conformance resource to include optional OpenID Connect Discovery URL. Still, in the case of some EHRs the access_token is just one part of the required FHIR Resource Access information, hence we may always need custom integration. Feedback from more EHRs is needed.

We chose to not have the CDS Service obtain an access token on each request as this would increase latency / the amount of time to service a request (since a token would have to be obtained on each call).

The interesting thing with your use case is that the authorization server is not owned by the EHR. With SMART and the vendor implementations I am aware of, the EHR and authorization server are assumed to have a trusted relationship (and likely owned/written by the same vendor). In your case, if the authorization server is a separate entity from the EHR, then I agree that having the EHR take the access token and provide it to the CDS Service opens up another threat.

However, as @isaacvetter mentioned, I'll echo his comments that with both SMART and CDS Hooks we are assuming that there exists a trust relationship between the EHR and authorization server. In the Argonaut project, we are going to be doing a 3rd party threat assessment of the security model in CDS Hooks. I'll make sure we enumerate this threat.

Another thing I should note is that we had a conversation yesterday in which we discussed whether an EHR is required to provide an access token in the CDS Service request. I think we landed on that this is optional. For your use case, I think this would alleviate your concerns.

Cross-Origin Resource Sharing: the proposed testing configuration is insecure and should not be used in production.

Agree with both your feedback here as what @isaacvetter said. We need to clarify this.