supporting X25519 and X448 in the IES style.

[kazu-yamamoto]

This implements #204.

First of all, sorry for the jumbo patch. I could not divide this into small pieces.

For the extension name:

• RFC 4492 and bis use NamedCurve
• TLS 1.3 uses NamedGroup since DH is integrated

I followed the TLS 1.3 way.

To remove duplicated NIST curves and support X25519 and X448:

• Extension.EC is deleted
• Extension is modified
• Crypto.Types is introduced

To integrate NIST curves and X*:

• Crypto.IES is introduced based on ECIES API of cryptonite
• Other ECDH related modules uses the APIs defined in Crypto.IES

Note that DH remained as is.

[kazu-yamamoto]

I will try to divide this jumbo patch. Just a moment.

[kazu-yamamoto]

Sorry. I gave up. Everything is related and I cannot divide it into pieces.

[ocheron]

Some first items I looked yesterday:

• my feeling is that removing generateECDHE changes the entropy source from the TLS context to IO, if confirmed this should be reverted
• groupGetShared should return a Maybe instead of calling error
• function processClientKeyXchg should use throwCore instead of error
• we may want to reorder availableGroups so that the head of intersection is the maximum strength like before

I'll try to continue reviewing during the week-end.

[kazu-yamamoto]

my feeling is that removing generateECDHE changes the entropy source from the TLS context to IO, if confirmed this should be reverted

If this is true, this is a fundamental problem between cryptonite and tls.
@vincenthz What do you think?

[kazu-yamamoto]

generateECDHE uses:

withRNG :: MonadPseudoRandom StateRNG a -> TLSSt a
withRNG f = do
    st <- get
    let (a,rng') = withTLSRNG (stRandomGen st) f
    put (st { stRandomGen = rng' })
    return a

deriveEncrypt uses getEntropy:

getEntropy :: ByteArray byteArray => Int -> IO byteArray
getEntropy n = do
    backends <- catMaybes `fmap` sequence supportedBackends
    B.alloc n (replenish n backends)

[kazu-yamamoto]

we may want to reorder availableGroups so that the head of intersection is the maximum strength like before

In my TLS 1.3 branch, supportedGroups :: [Group] is already implemented. This variable controls the preference order.

If we define supportedEllipticCurves :: [NamedCurve], its value would be [SEC SEC_p256r1, SEC SEC_p384r1, SEC SEC_p521r1]. This is really strange to users. That's why I simplified NamedCurve to Group.

[ocheron]

I don't see any fundamental problem other than the one we had a solution for. Upon closer inspection you removed the call to withRNG, so now the MonadRandom-based crypto runs directly in the IO monad instead of MonadPseudoRandom StateRNG like before. We don't want all concurrent TLS contexts to compete on the same global DRG.

I'm OK for not implementing supportedGroups in this PR, as today we don't have supportedEllipticCurves. We'll obviously want to implement only one, and after your PR that will be supportedGroups. Still for now the list intersection will favor the order from the first argument. So unless there is something I missed, your PR chooses the minimum strength. And that's a change of behavior easy to avoid by reversing the list.

[ocheron]

Here's my last comment about the detailed implementation: you moved functions related to TLS protocol (toGroup, fromGroup) into the "Crypto" modules but this is not related to crypto and would be best elsewhere, like module Struct or Extension.

Some ideas for a naming more consistent, tell me what you think:

• extensionID_NegotiatedGroups instead of extensionID_Groups ?
• ClientGroupSuggest instead of ClientEllipticCurveSuggest ?
• availableECDHGroups instead of availableGroups ?

I think that concludes my review.

[kazu-yamamoto]

extensionID_NegotiatedGroups instead of extensionID_Groups ?

Done.

ClientGroupSuggest instead of ClientEllipticCurveSuggest ?

Done.

[kazu-yamamoto]

availableECDHGroups instead of availableGroups ?

Group includes DH. So, I want to keep as is.

[kazu-yamamoto]

Here's my last comment about the detailed implementation: you moved functions related to TLS protocol (toGroup, fromGroup) into the "Crypto" modules but this is not related to crypto and would be best elsewhere, like module Struct or Extension.

Now, it is Extension.Group.

[kazu-yamamoto]

supportedGroups has been implemented.

[kazu-yamamoto]

I don't see any fundamental problem other than the one we had a solution for. Upon closer inspection you removed the call to withRNG, so now the MonadRandom-based crypto runs directly in the IO monad instead of MonadPseudoRandom StateRNG like before. We don't want all concurrent TLS contexts to compete on the same global DRG.

Thank you for letting me know this. I have implemented this.

[kazu-yamamoto]

@ocheron I believe that all issues have been resolved. Please review this PR again.

[kazu-yamamoto]

I noticed that cabal test sometimes fails.
I have created a minimum test case to reproduce this and try to fix it tomorrow.
Probably, it's relating to ECDHE.

[ocheron]

About modules "Crypto" vs "Extension": I was referring only to functions fromGroup/toGroup.
The data type Group should stay in the crypto modules, possibly in Crypto.IES itself.

OK for adding supportedGroups. You didn't explain why you chose the minimum strength.
I see a reason (cryptonite P256 code) and also it's configurable, so that's fine.
But please note that we will still need:

1. cipherAllowed checks to prevent the "no common groups" error from happening
2. test cases with non-default supportedGroups

[ocheron]

The test failure looks like a segmentation fault related to P256.

[kazu-yamamoto]

If I add trace to Crypto.PubKey.ECC.P256.pointDh, I cannot reproduce this problem.
There might be a race condition.
I suspect that the C backend of P256 is not thread-safe.

Cc: @vincenthz

[kazu-yamamoto]

Here is a small test to reproduce this: https://gist.github.com/kazu-yamamoto/25b7b4ee7884e30a93355e20f5efe0ac

[kazu-yamamoto]

It seems to me that scalarGenerate causes segfault.

[kazu-yamamoto]

getRandomBytes causes segufault.
Due to a lack of random source?

[kazu-yamamoto]

OK. I think that I have resolved all issues.

[ocheron]

We're getting closer... but I see you tried to optimize things by storing the group in the TLS state.

I'd rather have the same two-step process than signature_algorithms (i.e. functions hashAndSignaturesInCommon and decideHash).

It's probably a bit slower, but more organized. The group is already stored in ServerECDHParams in handshake state. We don't want another copy in the TLS state, especially if it's used only for the duration of one message (ServerHello).

[ocheron]

Also I believe the FIXME in handshakeServerWith has been addressed (or at least the "elliptic curve" part).

👍 for the test suite and the module structure.

[kazu-yamamoto]

I think I have done everything.

[ocheron]

Great work!

[kazu-yamamoto]

Thank you for your patient review.
I have merged this PR.