1290762 - Accept 0 length CertificateRequest.context

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Eric Rescorla (:ekr)]

I meant for this to be forbidden but the current spec says otherwise, so we need to accept it

Comment 1

[Eric Rescorla (:ekr)]

Attachment: 0002-Accept-empty-CertificateRequest.context.patch

Comment 2

[Martin Thomson [:mt:]]

Comment on attachment 8776418 [details] [diff] [review]
0002-Accept-empty-CertificateRequest.context.patch

Review of attachment 8776418 [details] [diff] [review]:
-----------------------------------------------------------------

I think that we disagree about the context length.  The point is to ensure that the context is unique.  That can be achieved with a zero-length context.  My memory of discussions with David was that we sort of agreed that it would be nice to allow (or even force) context to be zero-length in the handshake so that the two endpoints didn't have to retain extra state to handle a certificate request.  A nice rule might be: in handshake = zero length; out of handshake = non-zero length.

Comment 3

[Eric Rescorla (:ekr)]

(In reply to Martin Thomson [:mt:] from comment #2)
> Comment on attachment 8776418 [details] [diff] [review]
> 0002-Accept-empty-CertificateRequest.context.patch
> 
> Review of attachment 8776418 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think that we disagree about the context length.  The point is to ensure
> that the context is unique.  That can be achieved with a zero-length
> context.

Provided that only one CR is sent, yes. But then this requires all kinds of
special case logic on both sides.


>  My memory of discussions with David was that we sort of agreed
> that it would be nice to allow (or even force) context to be zero-length in
> the handshake so that the two endpoints didn't have to retain extra state to
> handle a certificate request. 

I don't think that we agreed to this and in any case it's not a particularly
accurate description of the burdens on both sides. Specifically, it's no harder
for the server to have a fixed string as context than it is to have an empty
value. It still needs to validate that whatever the client sends back matches.
As for the client, given that it needs to send the Cert/CV/Fin immediately, it's
not particularly difficult to store the context (as is clear from the NSS implementation).
It's true that it's a pain to store an unbounded number of CRs but that's inherent
in the post-handshake scenario, and isn't alleviated by allowing a zero-length
context in CR during the handshake.

Comment 4

[Eric Rescorla (:ekr)]

In any case, this is a standards issue. Not landing this patch merely leaves us non-compliant.

Perhaps you're objecting to the TODO, but removing that also makes the situation worse, because even in your design we should have a check for non-emptiness during post-handshake, and this is a placeholder for resolution of that issue.

Comment 5

[Martin Thomson [:mt:]]

Yes, the objection is to the comment.  I would be happier if the code were just removed.  We can track (any) disagreements in bugs.

Comment 6

[Eric Rescorla (:ekr)]

(In reply to Martin Thomson [:mt:] from comment #5)
> Yes, the objection is to the comment.  I would be happier if the code were
> just removed.  We can track (any) disagreements in bugs.

As noted, this *still* leaves us with a mess to clean up when we add post-handshake client auth.

Comment 7

[Martin Thomson [:mt:]]

Future us are probably better equipped to handle this.

Comment 8

[Eric Rescorla (:ekr)]

Attachment: 0001-Bug-1290762-Allow-empty-CertificateRequest.patch

Comment 9

[Eric Rescorla (:ekr)]

Landed as:
https://hg.mozilla.org/projects/nss/rev/162ac549109c