Use CERT_GetCertKeyType to get KeyType for ssl3_PlatformSignHashes.

Use CERT_GetCertKeyType to get KeyType for ssl3_PlatformSignHashes. 

This is based on issue 14187006, which was closed due to invalid Base URL.

BUG=226455
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196824

[mef, 2013-04-24 15:17:08]

Ryan,

I've rolled the DEPS and followed up on your comments from CL 14187006. 

I had to create a new issue as that one had bad Base URL.

thanks,
-m

[mef, 2013-04-25 15:03:44]

Hi Ryan,

just a quick couple of questions. 

- Could you please review this CL? 
- I'd like to add a unit test or more high level test. Can you suggest me a good
place or pattern to follow?

thanks,
-m
On 2013/04/24 15:17:08, mef wrote:
> Ryan,
> 
> I've rolled the DEPS and followed up on your comments from CL 14187006. 
> 
> I had to create a new issue as that one had bad Base URL.
> 
> thanks,
> -m

[Ryan Sleevi, 2013-04-25 18:37:42]

LGTM, mod indent nits

https://codereview.chromium.org/13843023/diff/1/net/third_party/nss/ssl/ssl3c...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/13843023/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:5370:
&ss->ssl3.clientCertificate->subjectPublicKeyInfo));
Looks like this function uses four tabs to indent, and then covers the remaining
with spaces.

[mef, 2013-04-25 19:06:05]

Ryan,

thanks a lot! I'll fix the tabs. Any suggestions about tests? 
I did manual test with clauer device, should I do anything else?

thanks,
-m

On 2013/04/25 18:37:42, Ryan Sleevi wrote:
> LGTM, mod indent nits
> 
>
https://codereview.chromium.org/13843023/diff/1/net/third_party/nss/ssl/ssl3c...
> File net/third_party/nss/ssl/ssl3con.c (right):
> 
>
https://codereview.chromium.org/13843023/diff/1/net/third_party/nss/ssl/ssl3c...
> net/third_party/nss/ssl/ssl3con.c:5370:
> &ss->ssl3.clientCertificate->subjectPublicKeyInfo));
> Looks like this function uses four tabs to indent, and then covers the
remaining
> with spaces.

[wtc, 2013-04-25 19:10:17]

Patch set 1 LGTM.

You need to update net/third_party/nss/patches/clientauth.patch
after the CL is final.

https://codereview.chromium.org/13843023/diff/1/net/third_party/nss/ssl/ssl3c...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/13843023/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:5370:
&ss->ssl3.clientCertificate->subjectPublicKeyInfo));
For NSS files, set the tabstop to 8 (as opposed to 2 or 4).

https://codereview.chromium.org/13843023/diff/1/net/third_party/nss/ssl/sslpl...
File net/third_party/nss/ssl/sslplatf.c (right):

https://codereview.chromium.org/13843023/diff/1/net/third_party/nss/ssl/sslpl...
net/third_party/nss/ssl/sslplatf.c:271: switch (sigAlg) {

You should also replace |sigAlg| with |keyType| in the
XP_MACOSX code.

[mef, 2013-04-25 19:31:15]

I've fixed the tabs and will update Mac code from my Mac machine.

https://codereview.chromium.org/13843023/diff/1/net/third_party/nss/ssl/ssl3c...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/13843023/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:5370:
&ss->ssl3.clientCertificate->subjectPublicKeyInfo));
Is there 80 column limit on line length?
On 2013/04/25 19:10:17, wtc wrote:
> For NSS files, set the tabstop to 8 (as opposed to 2 or 4).
>

https://codereview.chromium.org/13843023/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:5370:
&ss->ssl3.clientCertificate->subjectPublicKeyInfo));
On 2013/04/25 18:37:42, Ryan Sleevi wrote:
> Looks like this function uses four tabs to indent, and then covers the
remaining
> with spaces.

Done.

https://codereview.chromium.org/13843023/diff/1/net/third_party/nss/ssl/sslpl...
File net/third_party/nss/ssl/sslplatf.c (right):

https://codereview.chromium.org/13843023/diff/1/net/third_party/nss/ssl/sslpl...
net/third_party/nss/ssl/sslplatf.c:271: switch (sigAlg) {
I'll be glad to do that.
On 2013/04/25 19:10:17, wtc wrote:
> 
> You should also replace |sigAlg| with |keyType| in the
> XP_MACOSX code.

[wtc, 2013-04-25 19:57:24]

https://codereview.chromium.org/13843023/diff/1/net/third_party/nss/ssl/ssl3c...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/13843023/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:5370:
&ss->ssl3.clientCertificate->subjectPublicKeyInfo));

On 2013/04/25 19:31:15, mef wrote:
> Is there 80 column limit on line length?

Yes, the same 80 column limit is used in NSS files.

[mef, 2013-04-26 16:08:05]

I've updated indenting in ssl3con to fit into 80 chars with 8 char tab
expansion, and also modified ssl3_PlatformSignHashes to use keyType on Mac. 

It builds on Mac, but I don't really know how to test it there.

https://chromiumcodereview.appspot.com/13843023/diff/15001/net/third_party/ns...
File net/third_party/nss/ssl/ssl3con.c (right):

https://chromiumcodereview.appspot.com/13843023/diff/15001/net/third_party/ns...
net/third_party/nss/ssl/ssl3con.c:5370:
CERT_GetCertKeyType(&ss->ssl3.clientCertificate->subjectPublicKeyInfo));
With 8 chars tab size this function call didn't fit into 80 chars otherwise.

[wtc, 2013-04-26 17:38:03]

Patch set 3 LGTM.

Could you explain why this change is helpful to the OS_WIN
code? Thanks.

https://chromiumcodereview.appspot.com/13843023/diff/15001/net/third_party/ns...
File net/third_party/nss/ssl/ssl3con.c (right):

https://chromiumcodereview.appspot.com/13843023/diff/15001/net/third_party/ns...
net/third_party/nss/ssl/ssl3con.c:5370:
CERT_GetCertKeyType(&ss->ssl3.clientCertificate->subjectPublicKeyInfo));

We can use a local variable:
        CERTSubjectPublicKeyInfo *spki =
            &ss->ssl3.clientCertificate->subjectPublicKeyInfo;
        rv = ssl3_PlatformSignHashes(&hashes, ss->ssl3.platformClientKey,
            &hashes, ss->ssl3.platformClientKey, &buf, isTLS,
            CERT_GetCertKeyType(spki));

Or you can fold the CERT_GetCertKeyType call:
            CERT_GetCertKeyType(
                &ss->ssl3.clientCertificate->subjectPublicKeyInfo));

https://chromiumcodereview.appspot.com/13843023/diff/15001/net/third_party/ns...
File net/third_party/nss/ssl/sslplatf.c (right):

https://chromiumcodereview.appspot.com/13843023/diff/15001/net/third_party/ns...
net/third_party/nss/ssl/sslplatf.c:271: switch (keyType) {

Hmm... so we still need cssmKey and sigAlg in the OS_MACOSX
code. This means the keyType argument isn't useful to
OS_MACOSX.

If you want to avoid the need to test, you can either leave
the OS_MACOSX code unchanged, or simply assert that keyType
and sigAlg are consistent:

    switch (sigAlg) {
        case CSSM_ALGID_RSA:
            PORT_Assert(keyType == rsaKey);
            ...
        case CSSM_ALGID_ECDSA:
        case CSSM_ALGID_DSA:
            if (sigAlg == CSSM_ALGID_ECDSA) {
                PORT_Assert(keyType == ecKey);
                doDerEncode = PR_TRUE;
            } else {
                PORT_Assert(keyType == dsaKey);
                doDerEncode = isTLS;
            }
            ...

https://chromiumcodereview.appspot.com/13843023/diff/15001/net/third_party/ns...
net/third_party/nss/ssl/sslplatf.c:282: }

Delete the spaces at the end of the line.

[Ryan Sleevi, 2013-04-26 18:08:49]

On 2013/04/26 17:38:03, wtc wrote:
> Patch set 3 LGTM.
> 
> Could you explain why this change is helpful to the OS_WIN
> code? Thanks.

Smartcard middleware not implementing all the CAPI functions, because IE doesn't
call them.

IE (really, SChannel) gates the key type based on the SChannel provider that was
invoked - that is, SChannel has distinct "providers" for DH and RSA, and thus
never queries the key type because it assumes the key type specified will always
be correct.

CNG goes an entirely different API route with the SSL providers, in that the
middleware (can) provide the sign hashes function itself.

> 
>
https://chromiumcodereview.appspot.com/13843023/diff/15001/net/third_party/ns...
> File net/third_party/nss/ssl/ssl3con.c (right):
> 
>
https://chromiumcodereview.appspot.com/13843023/diff/15001/net/third_party/ns...
> net/third_party/nss/ssl/ssl3con.c:5370:
> CERT_GetCertKeyType(&ss->ssl3.clientCertificate->subjectPublicKeyInfo));
> 
> We can use a local variable:
>         CERTSubjectPublicKeyInfo *spki =
>             &ss->ssl3.clientCertificate->subjectPublicKeyInfo;
>         rv = ssl3_PlatformSignHashes(&hashes, ss->ssl3.platformClientKey,
>             &hashes, ss->ssl3.platformClientKey, &buf, isTLS,
>             CERT_GetCertKeyType(spki));
> 
> Or you can fold the CERT_GetCertKeyType call:
>             CERT_GetCertKeyType(
>                 &ss->ssl3.clientCertificate->subjectPublicKeyInfo));
> 
>
https://chromiumcodereview.appspot.com/13843023/diff/15001/net/third_party/ns...
> File net/third_party/nss/ssl/sslplatf.c (right):
> 
>
https://chromiumcodereview.appspot.com/13843023/diff/15001/net/third_party/ns...
> net/third_party/nss/ssl/sslplatf.c:271: switch (keyType) {
> 
> Hmm... so we still need cssmKey and sigAlg in the OS_MACOSX
> code. This means the keyType argument isn't useful to
> OS_MACOSX.
> 
> If you want to avoid the need to test, you can either leave
> the OS_MACOSX code unchanged, or simply assert that keyType
> and sigAlg are consistent:
> 
>     switch (sigAlg) {
>         case CSSM_ALGID_RSA:
>             PORT_Assert(keyType == rsaKey);
>             ...
>         case CSSM_ALGID_ECDSA:
>         case CSSM_ALGID_DSA:
>             if (sigAlg == CSSM_ALGID_ECDSA) {
>                 PORT_Assert(keyType == ecKey);
>                 doDerEncode = PR_TRUE;
>             } else {
>                 PORT_Assert(keyType == dsaKey);
>                 doDerEncode = isTLS;
>             }
>             ...
> 
>
https://chromiumcodereview.appspot.com/13843023/diff/15001/net/third_party/ns...
> net/third_party/nss/ssl/sslplatf.c:282: }
> 
> Delete the spaces at the end of the line.

[commit-bot: I haz the power, 2013-04-26 18:35:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mef@chromium.org/13843023/23001

[commit-bot: I haz the power, 2013-04-26 20:53:32]

Change committed as 196824