Support TLS Channel IDs in NSS

Support TLS Channel IDs in NSS

BUG=129174
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139919

[wtc, 2012-05-24 23:10:03]

Patch set 1 LGTM.  Please note the comments marked with "BUG"
or "IMPORTANT".

High-level design issue:

This requires that channel IDs be generated in advance,
whether the server requests a channel ID or not.  I think
we need to imitate client authentication handling and add
a callback to obtain the channel ID if the server requests
a channel ID.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
File net/third_party/nss/ssl/ssl.h (right):

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl.h:948: /* SSL_SetChannelD sets a P-256, ECC private
key to use as the TLS Channel ID.

"Channel ID" is not specific enough. I think we should name
it "Client ID" or "Client Channel ID".

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl.h:952: SSL_IMPORT SECStatus
SSL_SetChannelD(PRFileDesc *fd, SECKEYPrivateKey *id,

Typo: SSL_SetChannelD => SSL_SetChannelID

The comment above has the same typo.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
File net/third_party/nss/ssl/ssl3con.c (right):

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:8829: /* called from
ssl3_HandleServerHelloDone

This comment is incomplete -- ssl3_SendNextProto is called
from two places:
- ssl3_SendClientSecondRound (was ssl3_HandleServerHelloDone
  before Brian Smith's refactoring)
- ssl3_HandleFinished

If you agree, I will fix this comment upstream.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:8863: /* called from
ssl3_HandleServerHelloDone

This comment should be updated in the same way.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:8866: ssl3_SendEncryptedExtensions(sslSocket
*ss)

Ideally this function should be structured so that it calls
encrypted extension senders.  Right now it is hardcoded to
send the Channel ID extension only.  This is acceptable as
a first implementation.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:8873: unsigned char
signed_data[sizeof(CHANNEL_ID_MAGIC) + sizeof(SSL3Hashes)];

IMPORTANT: By using sizeof(CHANNEL_ID_MAGIC), the terminating null
byte in the "TLS Channel ID signature" string literal will
be included.  Just wanted to confirm this is what you wanted.

If not, you need to subtract one from sizeof(CHANNEL_ID_MAGIC)
to get the string length.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:8880: return SECSuccess;

Move these two lines after the lock assertions:

    PORT_Assert(ss->opt.noLocks || ssl_HaveXmitBufLock(ss));
    PORT_Assert(ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss));

    if (!ssl3_ExtensionNegotiated(ss, ssl_channel_id_xtn))
        return SECSuccess;

    PORT_Assert(ss->ssl3.channelID);

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:8889: static const int CHANNEL_ID_LENGTH =
128;

Move these two lines up to the variable declaration section.
(NSS is still being compiled by pre-C99 compilers.)

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:8897: rv = ssl3_ComputeHandshakeHashes(ss,
ss->ssl3.cwSpec, &hashes, PR_TRUE);

Pass 0 as the last argument.  It is only used by SSLv3.

I suggest holding the spec read lock across this
ssl3_ComputeHandshakeHashes call.

Are you sure we should use cwSpec rather than pwSpec?

See the equivalent code in ssl3_SendCertificateVerify:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3...

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:8924: static const unsigned char
P256_SPKI_PREFIX[] = {

Move this up to the variable declaration section.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:8934: return SECFailure;

Replace
        return SECFailure;
by
        rv = SECFailure;
        goto loser;

otherwise |spki| is leaked.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:8942: rv = SHA256_HashBuf(digest, signed_data,
sizeof(signed_data));

Please use the PK11_ function instead:

  rv = PK11_HashBuf(SEC_OID_SHA256, digest, signed_data, sizeof(signed_data));

SHA256_HashBuf can only be used when we are in bypassPKCS11
mode.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:8946: digest_item.data = (void*) &digest;

BUG: I believe this should be
    digest_item.data = digest;

Similarly line 8949 below should be
    signature_item.data = signature;

Perhaps for an array |digest|, &digest is the same as
&digest[0]?

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:8959: rv = SECSuccess;

BUG: remove this line because it suppresses the failure
of the ssl3_AppendHandshake call.

Add error checking:
    rv = ssl3_AppendHandshake(ss, pub_bytes, CHANNEL_ID_PUBLIC_KEY_LENGTH);
    if (rv != SECSuccess)
        goto loser;
    rv = ssl3_AppendHandshake(ss, signature, sizeof(signature));

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:9223: rv = ssl3_SendEncryptedExtensions(ss);

This means Channel ID must not be used in renegotiation,
right?

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:10535:
SECKEY_DestroyPublicKey(ss->ssl3.channelIDPub);

I suggest adding these right after line 10499 (where we
destroy ss->ssl3.clientCertChain) because this is similar
to client certificates.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
File net/third_party/nss/ssl/ssl3ext.c (right):

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3ext.c:90: PRUint32 maxBytes);

We should add a server side implementation that at least
echoes the extension and verifies the signature in the
EncryptedExtensions message.  This can be done later.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3ext.c:645: ssl3_ClientHandleChannelIDXtn(sslSocket
*ss, PRUint16 ex_type,

Please move this next to the ssl3_ClientSendChannelIDXtn
function.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3ext.c:649: return 0;

BUG: we should return either SECSuccess or SECFailure here.
It seems that we should return SECFailure (and set an
error code) if ss->ssl3.channelID is NULL.

Actually, this cannot happen, so you can remove this check
(or replace it with a PORT_Assert).

If ss->ssl3.channelID is NULL, we won't send the extension.

If the server sends an unsolicited extension, NSS will
detect that and won't call the client extension handler.

This is why ss->ssl3.channelID cannot be NULL here.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3ext.c:698: if (!ss->ssl3.channelID)

BUG: this should be
    /* Renegotiations do not send this extension. */
    if (!ss->ssl3.channelID || ss->firstHsDone)
        return 0;

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3ext.c:715: return extension_length;

Nit: I find this logic difficult to understand.  I know
you copied the logic from other functions.  I think this
is equivalent and clearer:

    if (maxBytes < extension_length) {
        PORT_Assert(0);
        return 0;
    }
    if (append) {
        SECStatus rv;
        rv = ssl3_AppendHandshakeNumber(ss, ssl_channel_id_xtn, 2);
        ...
    }
    return extension_length;

What do you think?

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
File net/third_party/nss/ssl/sslerr.h (right):

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/sslerr.h:222: SSL_ERROR_INVALID_CHANNEL_ID_KEY =
(SSL_ERROR_BASE + 125),

Could you also update SSLerrs.h?  It is not used by Chromium,
but will be required by NSS upstream.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
File net/third_party/nss/ssl/sslimpl.h (right):

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/sslimpl.h:953: SECKEYPublicKey *    channelIDPub;	/* ECC
public key. Used by client. */

Since these are closely related to client certificates, I
suggest adding them right below the "sendEmptyCert" member.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
File net/third_party/nss/ssl/sslt.h (right):

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/sslt.h:218: ssl_channel_id_xtn               = 30031,

Why don't you continue to use an extension number in
the 131xx range?

[agl, 2012-05-30 15:28:33]

Thanks. Important changes:

1) have switched to a callback system as suggested.
2) the encrypted extensions are sent on renegotiation now as the idea has been
floated that we may want to time them out.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
File net/third_party/nss/ssl/ssl.h (right):

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl.h:948: /* SSL_SetChannelD sets a P-256, ECC private
key to use as the TLS Channel ID.
On 2012/05/24 23:10:03, wtc wrote:
> 
> "Channel ID" is not specific enough. I think we should name
> it "Client ID" or "Client Channel ID".

Done.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl.h:952: SSL_IMPORT SECStatus
SSL_SetChannelD(PRFileDesc *fd, SECKEYPrivateKey *id,
On 2012/05/24 23:10:03, wtc wrote:
> 
> Typo: SSL_SetChannelD => SSL_SetChannelID
> 
> The comment above has the same typo.

Done.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
File net/third_party/nss/ssl/ssl3con.c (right):

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:8829: /* called from
ssl3_HandleServerHelloDone
On 2012/05/24 23:10:03, wtc wrote:
> If you agree, I will fix this comment upstream.

Yes, that's correct. I'll avoid changing it here to prevent a merge conflict
form upstream in the future.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:8863: /* called from
ssl3_HandleServerHelloDone
On 2012/05/24 23:10:03, wtc wrote:
> 
> This comment should be updated in the same way.

Done.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:8873: unsigned char
signed_data[sizeof(CHANNEL_ID_MAGIC) + sizeof(SSL3Hashes)];
On 2012/05/24 23:10:03, wtc wrote:
> 
> IMPORTANT: By using sizeof(CHANNEL_ID_MAGIC), the terminating null
> byte in the "TLS Channel ID signature" string literal will
> be included.  Just wanted to confirm this is what you wanted.

Yes, I intended to include the NUL byte. It's not terribly important but the
thinking was that we want signature context strings to be prefix free: i.e. we
don't want context strings "abc" and "abcdef" both to be in use because the
latter is valid for the former.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:8880: return SECSuccess;
On 2012/05/24 23:10:03, wtc wrote:
> 
> Move these two lines after the lock assertions:
> 
>     PORT_Assert(ss->opt.noLocks || ssl_HaveXmitBufLock(ss));
>     PORT_Assert(ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss));
> 
>     if (!ssl3_ExtensionNegotiated(ss, ssl_channel_id_xtn))
>         return SECSuccess;
> 
>     PORT_Assert(ss->ssl3.channelID);

Done.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:8889: static const int CHANNEL_ID_LENGTH =
128;
On 2012/05/24 23:10:03, wtc wrote:
> 
> Move these two lines up to the variable declaration section.
> (NSS is still being compiled by pre-C99 compilers.)

Done.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:8897: rv = ssl3_ComputeHandshakeHashes(ss,
ss->ssl3.cwSpec, &hashes, PR_TRUE);
On 2012/05/24 23:10:03, wtc wrote:
> Pass 0 as the last argument.  It is only used by SSLv3.

Done.

> I suggest holding the spec read lock across this
> ssl3_ComputeHandshakeHashes call.

Done.

> Are you sure we should use cwSpec rather than pwSpec?
> 
> See the equivalent code in ssl3_SendCertificateVerify:

Unlike when sending the CertificateVerify, this message is sent under
encryption. Therefore I believe that the pending spec has become the current
spec at this point.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:8924: static const unsigned char
P256_SPKI_PREFIX[] = {
On 2012/05/24 23:10:03, wtc wrote:
> 
> Move this up to the variable declaration section.

Done.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:8934: return SECFailure;
On 2012/05/24 23:10:03, wtc wrote:
> 
> Replace
>         return SECFailure;
> by
>         rv = SECFailure;
>         goto loser;
> 
> otherwise |spki| is leaked.

Good catch, thanks!

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:8942: rv = SHA256_HashBuf(digest, signed_data,
sizeof(signed_data));
On 2012/05/24 23:10:03, wtc wrote:
> 
> Please use the PK11_ function instead:
> 
>   rv = PK11_HashBuf(SEC_OID_SHA256, digest, signed_data, sizeof(signed_data));
> 
> SHA256_HashBuf can only be used when we are in bypassPKCS11
> mode.

Done.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:8946: digest_item.data = (void*) &digest;
On 2012/05/24 23:10:03, wtc wrote:
> Perhaps for an array |digest|, &digest is the same as
> &digest[0]?

Yes, that's the case. However it's surprising so I've removed the &.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:8959: rv = SECSuccess;
On 2012/05/24 23:10:03, wtc wrote:
> 
> BUG: remove this line because it suppresses the failure
> of the ssl3_AppendHandshake call.
> 
> Add error checking:
>     rv = ssl3_AppendHandshake(ss, pub_bytes, CHANNEL_ID_PUBLIC_KEY_LENGTH);
>     if (rv != SECSuccess)
>         goto loser;
>     rv = ssl3_AppendHandshake(ss, signature, sizeof(signature));

Thanks!

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:9223: rv = ssl3_SendEncryptedExtensions(ss);
On 2012/05/24 23:10:03, wtc wrote:
> 
> This means Channel ID must not be used in renegotiation,
> right?

It did, but that is no longer correct. I've moved the two calls outside of if
(!ss->firstHsDone) so that they are sent on renegotiation.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:10535:
SECKEY_DestroyPublicKey(ss->ssl3.channelIDPub);
On 2012/05/24 23:10:03, wtc wrote:
> 
> I suggest adding these right after line 10499 (where we
> destroy ss->ssl3.clientCertChain) because this is similar
> to client certificates.

Done.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
File net/third_party/nss/ssl/ssl3ext.c (right):

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3ext.c:645: ssl3_ClientHandleChannelIDXtn(sslSocket
*ss, PRUint16 ex_type,
On 2012/05/24 23:10:03, wtc wrote:
> 
> Please move this next to the ssl3_ClientSendChannelIDXtn
> function.

Done.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3ext.c:649: return 0;
On 2012/05/24 23:10:03, wtc wrote:
> 
> BUG: we should return either SECSuccess or SECFailure here.
> It seems that we should return SECFailure (and set an
> error code) if ss->ssl3.channelID is NULL.
> 
> Actually, this cannot happen, so you can remove this check
> (or replace it with a PORT_Assert).
> 
> If ss->ssl3.channelID is NULL, we won't send the extension.
> 
> If the server sends an unsolicited extension, NSS will
> detect that and won't call the client extension handler.
> 
> This is why ss->ssl3.channelID cannot be NULL here.

Done.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3ext.c:698: if (!ss->ssl3.channelID)
On 2012/05/24 23:10:03, wtc wrote:
> 
> BUG: this should be
>     /* Renegotiations do not send this extension. */
>     if (!ss->ssl3.channelID || ss->firstHsDone)
>         return 0;

As noted in ssl3con.c, the semantics have changed so that this is sent on
renegotiation. So the bug was that the code *wasn't* sending it on renegotiation
in ssl3con, rather than here.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3ext.c:715: return extension_length;
On 2012/05/24 23:10:03, wtc wrote:
> What do you think?

Agreed. Done.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
File net/third_party/nss/ssl/sslimpl.h (right):

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/sslimpl.h:953: SECKEYPublicKey *    channelIDPub;	/* ECC
public key. Used by client. */
On 2012/05/24 23:10:03, wtc wrote:
> 
> Since these are closely related to client certificates, I
> suggest adding them right below the "sendEmptyCert" member.

Done.

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
File net/third_party/nss/ssl/sslt.h (right):

https://chromiumcodereview.appspot.com/10424013/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/sslt.h:218: ssl_channel_id_xtn               = 30031,
On 2012/05/24 23:10:03, wtc wrote:
> 
> Why don't you continue to use an extension number in
> the 131xx range?

I don't especially want to be seen as claiming a part of the extension space so
I picked this number at random.

[wtc, 2012-05-31 00:36:32]

Patch set 4 LGTM.

Please upload a new patch set after making my suggested
changes.  You can commit it without waiting for my review.
Thanks.

https://chromiumcodereview.appspot.com/10424013/diff/13001/net/third_party/ns...
File net/third_party/nss/ssl/ssl.h (right):

https://chromiumcodereview.appspot.com/10424013/diff/13001/net/third_party/ns...
net/third_party/nss/ssl/ssl.h:950: * must have written a P-256 key pair to
|out_public_key| and

Nit: add "EC" after "P-256"

Nit: add '*' to |out_public_key| and |out_private_key|

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

https://chromiumcodereview.appspot.com/10424013/diff/13001/net/third_party/ns...
net/third_party/nss/ssl/ssl3con.c:8961: digest_item.data = (void*) digest;

Remove the (void*) casts here and on line 8964.

https://chromiumcodereview.appspot.com/10424013/diff/13001/net/third_party/ns...
net/third_party/nss/ssl/ssl3con.c:8978: rv = SECSuccess;

Nit: lines 8975-8978 can be omitted because they are after
the last "rv = ..." call.

https://chromiumcodereview.appspot.com/10424013/diff/13001/net/third_party/ns...
File net/third_party/nss/ssl/sslerr.h (right):

https://chromiumcodereview.appspot.com/10424013/diff/13001/net/third_party/ns...
net/third_party/nss/ssl/sslerr.h:223: SSL_ERROR_CHANNEL_ID_CALLBACK_FAILED =
(SSL_ERROR_BASE + 126),

Nit: name this error SSL_ERROR_GET_CHANNEL_ID_FAILED or
SSL_ERROR_FAILED_TO_GET_CHANNEL_ID?

https://chromiumcodereview.appspot.com/10424013/diff/13001/net/third_party/ns...
File net/third_party/nss/ssl/sslimpl.h (right):

https://chromiumcodereview.appspot.com/10424013/diff/13001/net/third_party/ns...
net/third_party/nss/ssl/sslimpl.h:1202: void                     *nextProtoArg;

channelIDCallback and its argument should be added here.

Perhaps the callback should be named getChannelID to
match the similar callback "getClientAuthData" (see lines
1188-1189).

https://chromiumcodereview.appspot.com/10424013/diff/13001/net/third_party/ns...
File net/third_party/nss/ssl/sslsock.c (right):

https://chromiumcodereview.appspot.com/10424013/diff/13001/net/third_party/ns...
net/third_party/nss/ssl/sslsock.c:376: ss->pkcs11PinArg          =
os->pkcs11PinArg;

Copy os->channelIDCallback and its argument.

https://chromiumcodereview.appspot.com/10424013/diff/13001/net/third_party/ns...
net/third_party/nss/ssl/sslsock.c:1690: ss->pkcs11PinArg          =
sm->pkcs11PinArg;

Copy sm->channelIDCallback and its argument.

https://chromiumcodereview.appspot.com/10424013/diff/13001/net/third_party/ns...
net/third_party/nss/ssl/sslsock.c:1882:
SSL_SetClientChannelIDCallback(PRFileDesc *fd,

Define this function in sslauth.c, right below the related
SSL_GetClientAuthDataHook function.

https://chromiumcodereview.appspot.com/10424013/diff/13001/net/third_party/ns...
net/third_party/nss/ssl/sslsock.c:2958: ss->pkcs11PinArg       = NULL;

Initialize ss->channelIDCallback and its argument to NULL
explicitly here?

This is not necessary because the entire 'ss' structure is
allocated zero-filled.  But we initialize many other callbacks
to NULL explicitly here.

[agl, 2012-05-31 17:19:53]

https://chromiumcodereview.appspot.com/10424013/diff/13001/net/third_party/ns...
File net/third_party/nss/ssl/ssl.h (right):

https://chromiumcodereview.appspot.com/10424013/diff/13001/net/third_party/ns...
net/third_party/nss/ssl/ssl.h:950: * must have written a P-256 key pair to
|out_public_key| and
On 2012/05/31 00:36:32, wtc wrote:
> 
> Nit: add "EC" after "P-256"
> 
> Nit: add '*' to |out_public_key| and |out_private_key|

Done.

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

https://chromiumcodereview.appspot.com/10424013/diff/13001/net/third_party/ns...
net/third_party/nss/ssl/ssl3con.c:8961: digest_item.data = (void*) digest;
On 2012/05/31 00:36:32, wtc wrote:
> 
> Remove the (void*) casts here and on line 8964.

Done.

https://chromiumcodereview.appspot.com/10424013/diff/13001/net/third_party/ns...
net/third_party/nss/ssl/ssl3con.c:8978: rv = SECSuccess;
On 2012/05/31 00:36:32, wtc wrote:
> 
> Nit: lines 8975-8978 can be omitted because they are after
> the last "rv = ..." call.

Done.

https://chromiumcodereview.appspot.com/10424013/diff/13001/net/third_party/ns...
File net/third_party/nss/ssl/sslerr.h (right):

https://chromiumcodereview.appspot.com/10424013/diff/13001/net/third_party/ns...
net/third_party/nss/ssl/sslerr.h:223: SSL_ERROR_CHANNEL_ID_CALLBACK_FAILED =
(SSL_ERROR_BASE + 126),
On 2012/05/31 00:36:32, wtc wrote:
> 
> Nit: name this error SSL_ERROR_GET_CHANNEL_ID_FAILED or
> SSL_ERROR_FAILED_TO_GET_CHANNEL_ID?

Done.

https://chromiumcodereview.appspot.com/10424013/diff/13001/net/third_party/ns...
File net/third_party/nss/ssl/sslimpl.h (right):

https://chromiumcodereview.appspot.com/10424013/diff/13001/net/third_party/ns...
net/third_party/nss/ssl/sslimpl.h:1202: void                     *nextProtoArg;
On 2012/05/31 00:36:32, wtc wrote:
> 
> channelIDCallback and its argument should be added here.
> 
> Perhaps the callback should be named getChannelID to
> match the similar callback "getClientAuthData" (see lines
> 1188-1189).

Done.

https://chromiumcodereview.appspot.com/10424013/diff/13001/net/third_party/ns...
File net/third_party/nss/ssl/sslsock.c (right):

https://chromiumcodereview.appspot.com/10424013/diff/13001/net/third_party/ns...
net/third_party/nss/ssl/sslsock.c:376: ss->pkcs11PinArg          =
os->pkcs11PinArg;
On 2012/05/31 00:36:32, wtc wrote:
> 
> Copy os->channelIDCallback and its argument.

Done.

https://chromiumcodereview.appspot.com/10424013/diff/13001/net/third_party/ns...
net/third_party/nss/ssl/sslsock.c:1690: ss->pkcs11PinArg          =
sm->pkcs11PinArg;
On 2012/05/31 00:36:32, wtc wrote:
> 
> Copy sm->channelIDCallback and its argument.

Done.

https://chromiumcodereview.appspot.com/10424013/diff/13001/net/third_party/ns...
net/third_party/nss/ssl/sslsock.c:1882:
SSL_SetClientChannelIDCallback(PRFileDesc *fd,
On 2012/05/31 00:36:32, wtc wrote:
> 
> Define this function in sslauth.c, right below the related
> SSL_GetClientAuthDataHook function.

Done.

https://chromiumcodereview.appspot.com/10424013/diff/13001/net/third_party/ns...
net/third_party/nss/ssl/sslsock.c:2958: ss->pkcs11PinArg       = NULL;
On 2012/05/31 00:36:32, wtc wrote:
> 
> Initialize ss->channelIDCallback and its argument to NULL
> explicitly here?
> 
> This is not necessary because the entire 'ss' structure is
> allocated zero-filled.  But we initialize many other callbacks
> to NULL explicitly here.

Done.

[commit-bot: I haz the power, 2012-05-31 17:23:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/10424013/4004

[commit-bot: I haz the power, 2012-05-31 17:40:42]

Try job failure for 10424013-4004 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[wtc, 2012-05-31 18:19:53]

Patch set 6 LGTM.  Thanks!

Just two nits about the error messages.

https://chromiumcodereview.appspot.com/10424013/diff/4004/net/third_party/nss...
File net/third_party/nss/ssl/SSLerrs.h (right):

https://chromiumcodereview.appspot.com/10424013/diff/4004/net/third_party/nss...
net/third_party/nss/ssl/SSLerrs.h:437: "An invalid TLS Channel ID key was
returned by the callback.")

Nit: it's not clear what "callback" means.  How about
"The application provided an invalid TLS Channel ID key." or
"The TLS Channel ID key was invalid."

https://chromiumcodereview.appspot.com/10424013/diff/4004/net/third_party/nss...
net/third_party/nss/ssl/SSLerrs.h:440: "The callback set by
SSL_SetClientChannelIDCallback failed.")

Nit: this error message seems to expose too much implementation
detail.  (Some applications simply display these error
messages to their users.)

How about "The application could not get a TLS Channel ID."
or "The application could not provide a TLS Channel ID."?

An alternative design is to require the callback to call
PORT_SetError() to set an NSPR/NSS error code, and NSS
just reports that error.

[agl, 2012-05-31 19:48:23]

https://chromiumcodereview.appspot.com/10424013/diff/4004/net/third_party/nss...
File net/third_party/nss/ssl/SSLerrs.h (right):

https://chromiumcodereview.appspot.com/10424013/diff/4004/net/third_party/nss...
net/third_party/nss/ssl/SSLerrs.h:437: "An invalid TLS Channel ID key was
returned by the callback.")
On 2012/05/31 18:19:53, wtc wrote:
> 
> Nit: it's not clear what "callback" means.  How about
> "The application provided an invalid TLS Channel ID key." or
> "The TLS Channel ID key was invalid."

Done.

https://chromiumcodereview.appspot.com/10424013/diff/4004/net/third_party/nss...
net/third_party/nss/ssl/SSLerrs.h:440: "The callback set by
SSL_SetClientChannelIDCallback failed.")
On 2012/05/31 18:19:53, wtc wrote:
> 
> Nit: this error message seems to expose too much implementation
> detail.  (Some applications simply display these error
> messages to their users.)
> 
> How about "The application could not get a TLS Channel ID."
> or "The application could not provide a TLS Channel ID."?
> 
> An alternative design is to require the callback to call
> PORT_SetError() to set an NSPR/NSS error code, and NSS
> just reports that error.

Done.

[commit-bot: I haz the power, 2012-05-31 19:48:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/10424013/7007

[commit-bot: I haz the power, 2012-05-31 23:43:49]

Change committed as 139919