Improve selection of server certificate and use "signature_algorithms" extension #236
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is the name chosen in draft-ietf-tls-rfc4492bis-17 and appearing in TLS HashAlgorithm Registry at IANA.
The code was overly complex because the list of server ciphers was used twice, once for IDs and once more to get the Cipher records. Related to #191.
Embedding the logic to filter server ciphers directly in the parameters themselves seems convoluted. Moreover it applies only to server ciphers and client ciphers do not need the extraCreds. As credentials need to be filtered based on ClientHello extensions it's best to move the function getCiphers directly in server code.
Implements a requirement from RFC 5246 section 7.4.2. Fixes #195.
When client and server do not agree on hash/signature algorithms, ciphers are not eliminated directly but indirectly through available credentials. This is necessary for EdDSA certificates: the same cipher key exchange ECDHE_ECDSA is reused for 3 public key algorithms ECDSA, Ed25519 and Ed448. And filtering only ciphers does not provide enough granularity anymore. For example ECDHE_ECDSA may be used with server having both ECDSA and Ed25519 certificates. If client supports only plain old ECDSA, the server should not pick Ed25519 credentials. The list of credentials 'creds' now becomes two lists 'creds' and 'signatureCreds'. Function 'getCiphers' uses one of both lists depending on cipher key-exchange type.
Draft TLS 1.3 section 4.4.2.2: If the server cannot produce a certificate chain that is signed only via the indicated supported algorithms, then it SHOULD continue the handshake by sending the client a certificate chain of its choice that may include algorithms that are not known to be supported by the client.
Draft TLS 1.3 section 4.4.2.2: Certificates that are self-signed or certificates that are expected to be trust anchors are not validated as part of the chain and therefore MAY be signed with any algorithm. This implementation is positional only. Server does not necessarily knows what level in the chain the client considers as trust anchor, so the code evaluates only a leaf certificate that is not self-signed. Test suite does not use self-signed certificates anymore.
|
I think I simplified the fallback condition too much and that may cause failures for some cases not covered by the test suite. I'll need to test more. |
|
@ocheron I'm busy in this week. So, it would take time to review this PR. |
|
OK this can wait and I still need to avoid a failure case anyway. The real interesting part is not #195 but the redesign in 6e553b5. IIUC the full mapping is:
|
This now tests that credentials have possible ciphers.
|
It should be safe now. |
|
Merged. |
|
Thanks. My experiment about EdDSA went very well, as I have ECDSA/Ed25519/Ed448 working correctly. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds behavior described in #195, with a rework of how cipher/credential code is organized.
I was reluctant to create impacts for something non essential like #195 but see now that supporting EdDSA in the future will require filtering credentials (instead of ciphers like we did before).
On a diagram we need to go from this:
to this:
This is because the relation between key exchange and certificate will become n..n:
Changing the design, it is easier to add something like #195. With the fallback mechanism described in TLS 1.3 this is not supposed to restrict handshake possibilities (i.e. the PR does not change test cases). So this looks safe for TLS 1.2 too.
I limited verification to the leaf certificate because a server has no reliable way to know what is the client view of trust anchors.