Server ECDSA for P-256 #436
Merged
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
kazu-yamamoto
requested changes
Jul 6, 2020
core/Network/TLS/Handshake/Server.hs
Outdated
| -- ECDSA keys are tested against supported elliptic curves until TLS12 but | ||
| -- not after. With TLS13, the curve is linked to the signature algorithm | ||
| -- and client support is tested with signatureCompatible13. | ||
| let p | ver > TLS12 = const True |
There was a problem hiding this comment.
The comment talks about < TLS12 first but the code handles TLS13 first. I would like to have the same ordering. And I don't know why both let and where are used. So, I would like to recommend the following code:
isCredentialAllowed :: Version -> [ExtensionRaw] -> Credential -> Bool
isCredentialAllowed ver exts cred =
pubkey `versionCompatible` ver && satisfiesEcPredicate p pubkey
where (pubkey, _) = credentialPublicPrivateKeys cred
-- ECDSA keys are tested against supported elliptic curves until
-- TLS12 but not after. With TLS13, the curve is linked to the
-- signature algorithm and client support is tested with
-- signatureCompatible13.
p | ver <= TLS12 = case extensionLookup extensionID_NegotiatedGroups exts >>= extensionDecode MsgTClientHello of
Nothing -> const True
Just (NegotiatedGroups sg) -> (`elem` sg)
| otherwise = const True
|
I forgot to mention the PR adds client certificates with P-256. |
|
Merged. |
|
Thank you for your review. |
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.
With cryptonite-0.27 it's now possible to use the optimized P256 implementation when verifying ECDSA signatures for this curve. I also believe P256 signature generation is safe enough to be enabled on server side.
The PR extends the crypto module with necessary utilities.
Safety checks are present to ensure ECDSA signature is not generated for other curves.
And the handshake code ignores ECDSA credentials that are not for P256.
Server-side behavior for the "elliptic_curves" extension is also added.
And the test suite can finally include ECDSA ciphers.