Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds TLS requirements for issuer keyset #176

Merged
merged 9 commits into from
Sep 24, 2021

Conversation

christianpaquin
Copy link
Contributor

@christianpaquin christianpaquin commented Aug 31, 2021

There have been discussions about moving the proposed TLS requirements for the VCI directory to the core spec. Here is proposal that explicitly cites the cryptographic requirements (without referring to 3rd party requirements).

9/16/2021 update: following some discussions, bcp195 is used as an external requirement source, following usage in, e.g., FHIR R4 security, QHIN Tech Framework. This seems to be a good set of recommendations for the core spec; downstream trust frameworks can always limit the requirements even more if needed. I changed the PR to reflect the language of the QHIN doc, to allow TLS 1.3 (not yet recommended by BCP 195).

docs/changelog.md Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
@isaacvetter
Copy link
Member

Hey Guys,

What's the timeline for review before this PR is merged? There's a lot of technical detail. Can it be not before the end of next week?

Isaac

@christianpaquin
Copy link
Contributor Author

What's the timeline for review before this PR is merged? There's a lot of technical detail. Can it be not before the end of next week?

We should definitely give time for review. Please let us know (anyone) how much time you'd need.

docs/index.md Outdated Show resolved Hide resolved
@lcmaas
Copy link
Contributor

lcmaas commented Sep 2, 2021

Hi, have we fully considered referring to the best practice documents for TLS that are managed by IETF instead? (BCP 11 and 195); perhaps recommending following these best practices is a good start as this is a nuanced topic to get right, and these docs get updated as deficiencies/vulnerabilities of different cryptographic algorithms are identified. I think this might generally be simpler (and more secure) than trying to list out specific ciphersuites in this guide. To @isaacvetter 's point, I would note that BCP 11 addresses both client and server considerations.

I will add, however, that using TLS to protect the distribution of an issuer's naked public keys (naked=not in an X.509 certificate) introduces additional security considerations: the strength of the keys and ciphersuites used to protect the TLS connection really need to be at least as strong as the key material being protected, because the TLS connection is being repurposed as a mechanism to securely bind the public key to the identity of the issuer (or in this case to the DNS domain of the issuer). For examples, using a P-256 EC based key agreement protocol or AES-128 encryption for the TLS connection decreases the effective strength of a P-384 key.

I would also note that the TLS connection issues would not be relevant if every JWKS returned included an X509 cert for each public key, because the CA signature on an X.509 cert is already at least as strong as the public key in the cert (for any competent CA) and therefore the keys/certs can actually be safely distributed without TLS at all (because the certificates can be independently validated by the party receiving them). This is commonplace in PKIX communities and also avoids circular trust boot-strapping issues. Unfortunately, as verifier support for certificate based trust of issuer keys is currently optional in the spec, we are probably stuck with explaining that properly configuring and enforcing TLS parameters is critical when either the issuer or verifier relies on TLS to bind public keys to an issuer identity.

@christianpaquin
Copy link
Contributor Author

Hi, have we fully considered referring to the best practice documents for TLS that are managed by IETF instead? (BCP 11 and 195); perhaps recommending following these best practices is a good start as this is a nuanced topic to get right, and these docs get updated as deficiencies/vulnerabilities of different cryptographic algorithms are identified.

That would also be a good approach. Better to refer to a standard body's recommendations (such as the IETF TLS ones you referred to) than to some vendors (e.g., OpenSSL's, Apple's, like in the VCI PR) best practices, IMO, but these larger recommendations are also very lenient in what they accept, so this PR’s intent was to limit options to a secure min bar (which would need to evolve over time, yes).

For examples, using a P-256 EC based key agreement protocol or AES-128 encryption for the TLS connection decreases the effective strength of a P-384 key.

Yes, but since SHC mandates a P-256 issuer key, the proposed min bar TLS config matches that security level.

@christianpaquin
Copy link
Contributor Author

It seems we have a decision to make: enforce a MUST support (server must offer the min bar config, but client can ask for less secure options) vs. MUST use (server only offers the min bar config). If we opt for the former, we can adapt the language per @jmandel's comment (or refer to an external document with similar recommendation, per @lcmaas's comment). Comments? Should this be discussed on a call?

@isaacvetter
Copy link
Member

@christianpaquin ,

enforce a MUST support (server must offer the min bar config, but client can ask for less secure options) vs. MUST use (server only offers the min bar config).

It's important to note that a new spec != new implementation or deployments. The vast majority of the urls in the directory, for example, are not net-new and also host pre-existing FHIR servers which support existing production integrations supporting patient care.

Because of this, we'd strongly recommend the "MUST support" option.

we can adapt the language per @jmandel's comment (or refer to an external document with similar recommendation, per @lcmaas's comment).

Towards the goal of making it operationally possible to comply with the SHC specification while also scaling support at a multi-national level, it's important that the security requirements be relatively stable. So, if an external document, which? What's the change control process of the external document? The ballot process? The timelines for changes? To avoid answering these questions, it would be nice to adopt Josh's language.

I'd love a call, if it would be helpful!

Isaac

docs/index.md Outdated Show resolved Hide resolved
@christianpaquin
Copy link
Contributor Author

christianpaquin commented Sep 9, 2021

I'd love a call, if it would be helpful!

Please fill out this scheduling pool if you are interested in discussing this on a call. @jmandel, @isaacvetter, @lcmaas.

@christianpaquin
Copy link
Contributor Author

Seems like Thu Sept 16th 10am Eastern is the best option. I've scheduled a Teams meeting. Let me know if you'd like a calendar invite, otherwise join using this link.

Questions to discuss (per this PR's threads):

  • Should the proposed change bump the spec version to 1.0.3 or 1.1.0 (minor vs. patch number increase)?
  • Is the proposed minimum bar ok? In particular, should we only use FIPS(NIST)-certified algs, or are other TLS-valid algs (e.g. CHACHA20_POLY1305) ok?
  • Should the minimum bar be required to use, or required to support (offered by the endpoint)?
  • Should we specify everything locally in the spec, or offload to another document (e.g., IETF’s recommendation for TLS)

@isaacvetter
Copy link
Member

Christian,

From some internal conversations, it almost feels like the documented requirements naturally fall into two categories:

  1. Pretty rock solid and quite justifiable requirements, e.g. no TLS 1.0 or TLS 1.1. Excluding DES and RCA would fit into this category, for example.
  2. More aspirational or recommended best practices, including your list of ciphers.

I wonder if a reasonable approach would be to require to use/not use #1 and require to support at least some of #2?

Isaac

@christianpaquin
Copy link
Contributor Author

I wonder if a reasonable approach would be to require to use/not use #1 and require to support at least some of #2?

That indeed sounds like the right approach, we just need to finalize what is in the #1 and #2 buckets. After the call next week, I'll update the PR with the emerging consensus, and give a chance to all spec reviewers to take a fresh look.

@christianpaquin
Copy link
Contributor Author

christianpaquin commented Sep 16, 2021

Seems like Thu Sept 16th 10am Eastern is the best option. I've scheduled a Teams meeting. Let me know if you'd like a calendar invite, otherwise join using this link.

Just had the call with @isaacvetter, @lcmaas, @GaryMcGregor, @jdkizer9. Seems like there is consensus at referring to an external doc for the details of the TLS requirements, specifically bcp195 (as done, e.g., in the FHIR security section). I'll give it a detail read, and update the PR. Trust frameworks (e.g. VCI directory) are at liberty of specifying more restrictions as appropriate.

Questions to discuss (per this PR's threads):

  • Should the proposed change bump the spec version to 1.0.3 or 1.1.0 (minor vs. patch number increase)?

We didn't discuss this much, didn't seem like there was a strong opinion about this. I'd be in favor of only incrementing the patch version, as to not invalidate current implementation.

@jmandel
Copy link
Member

jmandel commented Sep 17, 2021

We didn't discuss this much, didn't seem like there was a strong opinion about this. I'd be in favor of only incrementing the patch version, as to not invalidate current implementation.

From a compatibility perspective:

  • clients get a new documented feature they can rely on(ability to use specific algorithms). This implies a minor version update (at least)
  • servers that were previously compatible with our spec may cease being compatible since we're adding new "SHALL" level requirements. This seems to imply a major version update.

I'm having trouble seeing the argument for "patch" only, given these compatibility implications. (To be clear, I don't think this is a huge deal; I just want a consistent approach to the analysis so we know how to apply the same reasoning next time.)

@@ -112,7 +112,8 @@ certificate or certificate chain (see [RFC7517](https://tools.ietf.org/html/rfc7
The public key listed in the first certificate in the `"x5c"` array SHALL match the public key specified by the `"crv"`, `"x"`, and `"y"` parameters of the same JWK entry.
If the issuer has more than one certificate for the same public key (e.g. participation in more than one trust community), then a separate JWK entry is used for each certificate with all JWK parameter values identical except `"x5c"`.

Issuers SHALL publish their public keys as JSON Web Key Sets (see [RFC7517](https://tools.ietf.org/html/rfc7517#section-5)), available at `<<iss value from JWS>>` + `/.well-known/jwks.json`, with [Cross-Origin Resource Sharing (CORS)](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Origin) enabled.

Issuers SHALL publish their public keys as JSON Web Key Sets (see [RFC7517](https://tools.ietf.org/html/rfc7517#section-5)), available at `<<iss value from JWS>>` + `/.well-known/jwks.json`, with [Cross-Origin Resource Sharing (CORS)](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Origin) enabled, using TLS version 1.2 following the IETF [BCP 195](https://www.rfc-editor.org/info/bcp195) recommendations or TLS version 1.3 (with any configuration).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks -- "with any configuration" heads off the question I had when I first read this :-)

@jmandel jmandel merged commit 1446052 into smart-on-fhir:main Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants