Bug 1237514 - Convert certificate indexing to SSLAuthType

Bug 1237514 - Convert certificate indexing to SSLAuthType

[wtc1, 2016-01-11 23:45:56]

Review comments on patch set 1:

Martin: thank you very much for writing the CL. I am still
trying to understand the CL. I only reviewed the header files
and I would like to give you my preliminary comments first.

https://codereview.appspot.com/280660043/diff/1/lib/ssl/ssl.h
File lib/ssl/ssl.h (right):

https://codereview.appspot.com/280660043/diff/1/lib/ssl/ssl.h#newcode1048
lib/ssl/ssl.h:1048: * Returns authentication type of the keys in an SSL server
certificate.

I must say "authentication type" is a bad way to refer to the type of the key in
a certificate. Also, "keys" should be "key" (singular). So I think this comment
should say:

  Returns the type of the key in an SSL server certificate.

or

  Returns the type of the key in an SSL server authentication certificate.

https://codereview.appspot.com/280660043/diff/1/lib/ssl/sslimpl.h
File lib/ssl/sslimpl.h (left):

https://codereview.appspot.com/280660043/diff/1/lib/ssl/sslimpl.h#oldcode39
lib/ssl/sslimpl.h:39: typedef SSLKEAType      SSL3KEAType;

Why did you remove SSL3KEAType?

https://codereview.appspot.com/280660043/diff/1/lib/ssl/sslimpl.h
File lib/ssl/sslimpl.h (right):

https://codereview.appspot.com/280660043/diff/1/lib/ssl/sslimpl.h#newcode354
lib/ssl/sslimpl.h:354: SECItem               signedCertTimestamps;

This is a good cleanup!

https://codereview.appspot.com/280660043/diff/1/lib/ssl/sslimpl.h#newcode743
lib/ssl/sslimpl.h:743: SSLAuthType authType;

For consistency with the previous two members, I think this member should be
named "authKeyType".

https://codereview.appspot.com/280660043/diff/1/lib/ssl/sslinfo.c
File lib/ssl/sslinfo.c (right):

https://codereview.appspot.com/280660043/diff/1/lib/ssl/sslinfo.c#newcode132
lib/ssl/sslinfo.c:132: #define S_KEA   "KEA", ssl_auth_kea

We may want to delete the S_KEA macro definition because it is unused:
http://mxr.mozilla.org/nss/ident?i=S_KEA

https://codereview.appspot.com/280660043/diff/1/lib/ssl/sslt.h
File lib/ssl/sslt.h (right):

https://codereview.appspot.com/280660043/diff/1/lib/ssl/sslt.h#newcode92
lib/ssl/sslt.h:92: ssl_auth_kea    = 3,

IMPORTANT: I don't understand what ssl_auth_kea means.
These two MXR queries show it is unused:
http://mxr.mozilla.org/nss/ident?i=ssl_auth_kea
http://mxr.mozilla.org/nss/ident?i=S_KEA

I suspect it was used by an obsolete feature, perhaps FORTEZZA?
It would be good to ask Bob Relyea to write a comment for ssl_auth_kea;
until then we should add a comment to note that this is unused
and delete the S_KEA macro definition from sslinfo.c.

https://codereview.appspot.com/280660043/diff/1/lib/ssl/sslt.h#newcode96
lib/ssl/sslt.h:96: } SSLAuthType;

1. IMPORTANT: please document what SSLAuthType is. From your explanation in the
bug report:

    the type of key that the certificate contains

2. Should we also add ssl_auth_dh? Or do you think we should wait until NSS
supports DH_RSA or DH_DSS cipher suites?

[mt, 2016-01-12 00:22:27]

https://codereview.appspot.com/280660043/diff/1/lib/ssl/ssl.h
File lib/ssl/ssl.h (right):

https://codereview.appspot.com/280660043/diff/1/lib/ssl/ssl.h#newcode1048
lib/ssl/ssl.h:1048: * Returns authentication type of the keys in an SSL server
certificate.
On 2016/01/11 23:45:56, wtc1 wrote:
> 
> I must say "authentication type" is a bad way to refer to the type of the key
in
> a certificate. Also, "keys" should be "key" (singular). So I think this
comment
> should say:
> 
>   Returns the type of the key in an SSL server certificate.
> 
> or
> 
>   Returns the type of the key in an SSL server authentication certificate.

Done.

https://codereview.appspot.com/280660043/diff/1/lib/ssl/sslimpl.h
File lib/ssl/sslimpl.h (left):

https://codereview.appspot.com/280660043/diff/1/lib/ssl/sslimpl.h#oldcode39
lib/ssl/sslimpl.h:39: typedef SSLKEAType      SSL3KEAType;
On 2016/01/11 23:45:56, wtc1 wrote:
> 
> Why did you remove SSL3KEAType?

It wasn't doing any good and was only used in a few places, all of which I was
forced to touch.

https://codereview.appspot.com/280660043/diff/1/lib/ssl/sslinfo.c
File lib/ssl/sslinfo.c (right):

https://codereview.appspot.com/280660043/diff/1/lib/ssl/sslinfo.c#newcode132
lib/ssl/sslinfo.c:132: #define S_KEA   "KEA", ssl_auth_kea
On 2016/01/11 23:45:56, wtc1 wrote:
> 
> We may want to delete the S_KEA macro definition because it is unused:
> http://mxr.mozilla.org/nss/ident?i=S_KEA

Done.

https://codereview.appspot.com/280660043/diff/1/lib/ssl/sslt.h
File lib/ssl/sslt.h (right):

https://codereview.appspot.com/280660043/diff/1/lib/ssl/sslt.h#newcode92
lib/ssl/sslt.h:92: ssl_auth_kea    = 3,
On 2016/01/11 23:45:56, wtc1 wrote:
> 
> IMPORTANT: I don't understand what ssl_auth_kea means.
> These two MXR queries show it is unused:
> http://mxr.mozilla.org/nss/ident?i=ssl_auth_kea
> http://mxr.mozilla.org/nss/ident?i=S_KEA
> 
> I suspect it was used by an obsolete feature, perhaps FORTEZZA?
> It would be good to ask Bob Relyea to write a comment for ssl_auth_kea;
> until then we should add a comment to note that this is unused
> and delete the S_KEA macro definition from sslinfo.c.

I've asked Bob.  I think that you are right and it is FORTEZZA, but my knowledge
there is very patchy.

https://codereview.appspot.com/280660043/diff/1/lib/ssl/sslt.h#newcode96
lib/ssl/sslt.h:96: } SSLAuthType;
On 2016/01/11 23:45:56, wtc1 wrote:
> 
> 1. IMPORTANT: please document what SSLAuthType is. From your explanation in
the
> bug report:
> 
>     the type of key that the certificate contains

Done.

> 2. Should we also add ssl_auth_dh? Or do you think we should wait until NSS
> supports DH_RSA or DH_DSS cipher suites?

I doubt that we will ever support that sort of cipher suite, but I'd rather wait
until we do.

[ttaubert, 2016-01-22 10:55:30]

This is great stuff. I'm rather surprised we got away this long handling cipher
suites as we currently do. The patch seems quite solid.

https://codereview.appspot.com/280660043/diff/20001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/280660043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:5658: * shared secret is treated the same way as Fortezza's
Ks, i.e.,
nit: can we get rid of the Fortezza reference?

https://codereview.appspot.com/280660043/diff/20001/lib/ssl/sslsecur.c
File lib/ssl/sslsecur.c (right):

https://codereview.appspot.com/280660043/diff/20001/lib/ssl/sslsecur.c#newcod...
lib/ssl/sslsecur.c:851: tag == SEC_OID_PKCS1_RSA_ENCRYPTION;
nit: indent

https://codereview.appspot.com/280660043/diff/20001/lib/ssl/sslsecur.c#newcod...
lib/ssl/sslsecur.c:856: tag == SEC_OID_X942_DIFFIE_HELMAN_KEY;
nit: indent