net: add missing return value in SSL_SetNextProtoCallback

net: add missing return value in SSL_SetNextProtoCallback

Also take a couple of changes from Brain Smith's version of the code which
exclude NPN from renegotiations.


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

[wtc, 2011-10-28 18:22:05]

LGTM.

We should add a note about renegotiation to the NPN Internet
draft (or Google technical note).

I'd like to take the opportunity to ask you a question:
for the server side implementation, it seems that the
SSL_SetNextProtoNego could be used by a server to specify
its list of protocols, and SSL_GetNextProto could be used
by a server to get the protocol selected by the client.
Agreed?

http://codereview.chromium.org/8417032/diff/1/net/third_party/nss/ssl/ssl3con.c
File net/third_party/nss/ssl/ssl3con.c (right):

http://codereview.chromium.org/8417032/diff/1/net/third_party/nss/ssl/ssl3con...
net/third_party/nss/ssl/ssl3con.c:5902: if (!ss->firstHsDone) {

We should add a comment to explain this, because the reason
is not obvious.  I guess this is because the next protocol
is not part of the SSL session state, so it is irrelevant
when renegotiating the SSL session parameters.

http://codereview.chromium.org/8417032/diff/1/net/third_party/nss/ssl/ssl3con...
net/third_party/nss/ssl/ssl3con.c:8846: }

It seems that this whole thing can be removed.  I guess this
is for the session resumption case, because the next protocol
by definition is not part of the SSL session state, and so
still needs to be negotiated in a session resumption handshake?

[agl, 2011-10-28 19:15:15]

On Fri, Oct 28, 2011 at 2:22 PM,  <wtc@chromium.org> wrote:
> I'd like to take the opportunity to ask you a question:
> for the server side implementation, it seems that the
> SSL_SetNextProtoNego could be used by a server to specify
> its list of protocols, and SSL_GetNextProto could be used
> by a server to get the protocol selected by the client.
> Agreed?

Yes, I don't see any problems with that.


Cheers

AGL

[agl, 2011-10-28 19:15:18]

http://codereview.chromium.org/8417032/diff/1/net/third_party/nss/ssl/ssl3con.c
File net/third_party/nss/ssl/ssl3con.c (right):

http://codereview.chromium.org/8417032/diff/1/net/third_party/nss/ssl/ssl3con...
net/third_party/nss/ssl/ssl3con.c:5902: if (!ss->firstHsDone) {
On 2011/10/28 18:22:05, wtc wrote:
> 
> We should add a comment to explain this, because the reason
> is not obvious.  I guess this is because the next protocol
> is not part of the SSL session state, so it is irrelevant
> when renegotiating the SSL session parameters.

Done.

http://codereview.chromium.org/8417032/diff/1/net/third_party/nss/ssl/ssl3con...
net/third_party/nss/ssl/ssl3con.c:8846: }
On 2011/10/28 18:22:05, wtc wrote:
> 
> It seems that this whole thing can be removed.  I guess this
> is for the session resumption case, because the next protocol
> by definition is not part of the SSL session state, and so
> still needs to be negotiated in a session resumption handshake?

Right, we explicitly say that NPN isn't part of the session state.

[commit-bot: I haz the power, 2011-10-28 19:33:31]

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

[commit-bot: I haz the power, 2011-10-28 21:08:58]

Change committed as 107791