Bug 1247278 - Determine keaKeyBits correctly for NSS_ECC_MORE_THAN_SUITE_B=1

Bug 1247278 - Determine keaKeyBits correctly for NSS_ECC_MORE_THAN_SUITE_B=1

[ekr-rietveld, 2016-02-10 15:15:08]

https://codereview.appspot.com/289090043/diff/1/external_tests/ssl_gtest/libs...
File external_tests/ssl_gtest/libssl_internals.c (right):

https://codereview.appspot.com/289090043/diff/1/external_tests/ssl_gtest/libs...
external_tests/ssl_gtest/libssl_internals.c:40: PRUint32 curves =
ss->ssl3.hs.negotiatedECCurves;
This doesn't seem right because what we want to test is partly the negotiation
logic. You need to replicate that logic outside libssl.

https://codereview.appspot.com/289090043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_agent.cc (right):

https://codereview.appspot.com/289090043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.cc:302: case ssl_auth_rsa: {
Doesn't this SSLInt_Determine... code work whether we have
NSS_ECC_MORE_THAN_SUITE_B set or not? I would think that that conditional would
be isolated to the function, not here.

[ttaubert, 2016-02-10 15:16:56]

https://codereview.appspot.com/289090043/diff/1/external_tests/ssl_gtest/libs...
File external_tests/ssl_gtest/libssl_internals.c (right):

https://codereview.appspot.com/289090043/diff/1/external_tests/ssl_gtest/libs...
external_tests/ssl_gtest/libssl_internals.c:40: PRUint32 curves =
ss->ssl3.hs.negotiatedECCurves;
On 2016/02/10 15:15:08, ekr-webrtc wrote:
> This doesn't seem right because what we want to test is partly the negotiation
> logic. You need to replicate that logic outside libssl.

Ah, that's what I almost expected.

https://codereview.appspot.com/289090043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_agent.cc (right):

https://codereview.appspot.com/289090043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.cc:302: case ssl_auth_rsa: {
On 2016/02/10 15:15:08, ekr-webrtc wrote:
> Doesn't this SSLInt_Determine... code work whether we have
> NSS_ECC_MORE_THAN_SUITE_B set or not? I would think that that conditional
would
> be isolated to the function, not here.

Right... I think that should work.

[ttaubert, 2016-02-10 17:21:53]

On 2016/02/10 15:16:56, ttaubert wrote:
> On 2016/02/10 15:15:08, ekr-webrtc wrote:
> > Doesn't this SSLInt_Determine... code work whether we have
> > NSS_ECC_MORE_THAN_SUITE_B set or not? I would think that that conditional
> would
> > be isolated to the function, not here.
> 
> Right... I think that should work.

It does of course work. I read your comment wrong, sorry.

[ttaubert, 2016-02-15 14:15:23]

On 2016/02/10 15:15:08, ekr-webrtc wrote:
>
https://codereview.appspot.com/289090043/diff/1/external_tests/ssl_gtest/libs...
> external_tests/ssl_gtest/libssl_internals.c:40: PRUint32 curves =
> ss->ssl3.hs.negotiatedECCurves;
> This doesn't seem right because what we want to test is partly the negotiation
> logic. You need to replicate that logic outside libssl.

How much of that logic are we willing to replicate? Afaict we never call
NSS_SetAlgorithmPolicy() in gtests so either all curves, or only those of Suite
B are enabled and we could thus simply assume all 25 or all 3 curves to be
"negotiated".

If we wanted to replicate the negotiation in case we ever disable single curves
then we either would have to pass the other peer's ssl_fd_ into CheckKEAType()
and call ssl3_GetSupportedECCurveMask(), or always filter for SupportedCurvesXtn
and compare this against ssl3_GetSupportedECCurveMask().

The chosen curve depends on the order of curves in bits2curve[] defined in
ssl3ecc. We either have to export that, or keep it in sync. The latter doesn't
seem like a great idea. We could also simply call
ssl3_GetCurveWithECKeyStrength() but that doesn't work if that's part of what we
want to test.

[ekr-rietveld, 2016-02-15 17:07:50]

https://codereview.appspot.com/289090043/diff/20001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/tls_agent.cc (right):

https://codereview.appspot.com/289090043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_agent.cc:296: // The first one in the list of Suite
B curves is P-256.
You say KEA, but this is the auth algorithm.

https://codereview.appspot.com/289090043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_agent.cc:301: // again pick P-256 for ECDSA, or
this time P-192 for RSA auth.
It's not P256, right? It's server_key_bits.

I think you should be a bit smarter about using the RSA key to drive it.
At least handle 1024 and 2048.

[ttaubert, 2016-02-15 18:26:10]

https://codereview.appspot.com/289090043/diff/20001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/tls_agent.cc (right):

https://codereview.appspot.com/289090043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_agent.cc:296: // The first one in the list of Suite
B curves is P-256.
On 2016/02/15 17:07:50, ekr-webrtc wrote:
> You say KEA, but this is the auth algorithm.

Done.

https://codereview.appspot.com/289090043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_agent.cc:301: // again pick P-256 for ECDSA, or
this time P-192 for RSA auth.
On 2016/02/15 17:07:50, ekr-webrtc wrote:
> It's not P256, right? It's server_key_bits.
> 
> I think you should be a bit smarter about using the RSA key to drive it.
> At least handle 1024 and 2048.

Done.

[ekr-rietveld, 2016-02-15 18:36:49]

https://codereview.appspot.com/289090043/diff/40001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/libssl_internals.c (right):

https://codereview.appspot.com/289090043/diff/40001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/libssl_internals.c:51: #endif
Given that this code doesn't know about all the curves anyway,
why don't you just do:

#ifndef NSS_ECC_MORE_THAN_SUITE_B
if (keaBits < 256)
   return 256U;
#endif

return keaBits;

[ttaubert, 2016-02-16 16:55:12]

https://codereview.appspot.com/289090043/diff/40001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/libssl_internals.c (right):

https://codereview.appspot.com/289090043/diff/40001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/libssl_internals.c:51: #endif
On 2016/02/15 18:36:49, ekr-webrtc wrote:
> Given that this code doesn't know about all the curves anyway,
> why don't you just do:
> 
> #ifndef NSS_ECC_MORE_THAN_SUITE_B
> if (keaBits < 256)
>    return 256U;
> #endif
> 
> return keaBits;

Done.

[ekr-rietveld, 2016-02-16 17:22:59]

LGTM

https://codereview.appspot.com/289090043/diff/60001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/libssl_internals.c (right):

https://codereview.appspot.com/289090043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/libssl_internals.c:47: #endif
You do not need an #else here.

[ttaubert, 2016-02-16 21:55:25]

On 2016/02/16 17:22:59, ekr-webrtc wrote:
>
https://codereview.appspot.com/289090043/diff/60001/external_tests/ssl_gtest/...
> external_tests/ssl_gtest/libssl_internals.c:47: #endif
> You do not need an #else here.

Kept the else branch #ifdef'ed so that we don't return 256 bits when we actually
would expect a 224-bit for e.g. a 2048-bit RSA key. Cleaned up the code some
more by using PR_MAX().

[ekr-rietveld, 2016-02-17 16:30:50]

LGTM