Nelson Bolyard (seldom reads bugmail) Reporter
	Description • 18 years ago

RFC 4366 specifies an extension for the TLS hello messages, known as the
"Certificate Status Request" extension, better known as "OCSP Stapling".

The idea is that the server sends its certificate AND the OCSP response
from it certificate's issuer, declaring that the server's cert is not 
revoked.  The OCSP response is thus "stapled" to the certificate. 

If the response is sufficiently recent and its signature can be verified
as coming from the issuer or the issuer's designated OCSP responder, then
the stapled response obviates an additional round trip for the OCSP request
and response.  

We should implement this in our libSSL.  To do so, we need the OCSP response
cache to be implemented.  The implementation would validate the response 
and then insert it into the cache.  Then later, at the point where we are
validating the server cert, and we're about to issue the OCSP request, we
will see the response in the cache and use it instead.  

It is debatable whether this needs to be implemented before OCSP can be 
enabled by default in the browser.

	Gervase Markham [:gerv]
	Comment 1 • 15 years ago

Nelson: are you able to provide a quick technical update here? For example, my understanding is that NSS now implements an OCSP response cache... is that right?

Gerv

	Nelson Bolyard (seldom reads bugmail) Reporter
	Comment 2 • 15 years ago

Yes, the OCSP cache was implemented in bug 205406 in 2007 and has been in 
NSS since NSS 3.12.0.  I could provide much more update than than, but it
would be political, not technical.

	Nelson Bolyard (seldom reads bugmail) Reporter
	Comment 3 • 15 years ago

A New Internet-Draft is available from the on-line Internet-Drafts
directories.

  Title       : Adding Multiple TLS Certificate Status Extension requests
  Author(s)   : Y. Pettersen
  Filename    : draft-pettersen-tls-ext-multiple-ocsp-00.txt
  Pages       : 9
  Date        : 2009-10-16

This document introduces a replacement of the TLS Certificate Status Extension 
to allow clients to specify and support multiple certificate status methods.  
Also being introduced is a new OCSP based method that servers can use to 
provide status information not just about the server's own certificate, 
but also the status of intermediate certificates in the chain.

A URL for this Internet-Draft is:
http://www.ietf.org/id/draft-pettersen-tls-ext-multiple-ocsp-00.txt

	Gervase Markham [:gerv]
	Comment 4 • 15 years ago

I am informed that Nagendra Modadugu, the assignee of this bug, is working on it and plans to complete it. :-)

Gerv

	Adam Langley
	Comment 5 • 15 years ago

Here's a prelim patch to add OCSP stapling support to libssl.

I think the OCSP cache is supposed to be separate from libssl, so there's no interaction here. What it does provide is a way for the user of libssl to get the raw DER bytes of the response.

It's a shame that the RFC says that CertificateStatus comes after Certificate as this patch wants it the other way round. So it buffers the Certificate message until it either has a CertificateStatus, or it gets a message that means that a CertificateStatus isn't coming.

If we pulled the bottom half of ssl3_HandleCertificate into its own function then we could do without buffering the message. I must admit that the error handling in ssl3_HandleCertificate scared me away and I went for the easy answer.

(Only tested against Apache SVN. If anyone knows of a site that supports OCSP stapling, then I'd be happy to know.)

	Wan-Teh Chang
	Comment 6 • 15 years ago

Adam, thank you for the patch.  To accept this patch we
also need a server-side implmenetation.  At least, that
will allow us to test NSS against itself.

Nagendra, what's the status of your work?  If you won't
be able to finish it soon, could you attach your work
in progress as a reference?  Thanks.

	Adam Langley
	Comment 7 • 15 years ago

I can write the server side too. I'd like a little guidance about the interface.

In the client-side patch I keep OCSP completely separate from the rest of NSS since I believe that libssl is supposed to be somewhat independent. So would a server side patch that added a callback (i.e. "CertificateStatusCallback") be the correct design? The libssl user could register that callback and provide an OCSP reply (or not) for each new pertinent connection.

	Nelson Bolyard (seldom reads bugmail) Reporter
	Comment 8 • 15 years ago

Comment on attachment 418746 [details] [diff] [review]
OCSP stapling support for libssl

I just glanced at this patch and found it to be much smaller than I would have guessed.  Is it thought to be complete (for the client side)? 
I will review it at length, hopefully within a week's time.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 9 • 14 years ago

Adam, I discussed this with Robert Relyea and Wan-Teh and we agreed that the server side only needs to be implemented enough to allow effective testing of the client side code; we don't expect any NSS server products to enable the OCSP stapling support unless/until a server product vendor provides a real server side implementation. If you have any work in progress for the server side, please attach it to the bug.

Nominated as a FF 4.0 blocker. I discussed this with Sid Stamm and Dan Veditz and it was determined that this is important enough to try to get into the FF 4.0 release because it is a big privacy and performance win when the server supports it, and because it will help strengthen our CA policy (see bug 585122) to do EV revocation checking correctly. Besides this patch, we would need to create a patch to implement enough of the server side to enable automated tests.

We should also use the SSL Labs list, at least, to test NSS against existing OCSP-stapling-enabled servers. I will also find out the exact default OCSP stapling behavior of IE8 and IE9. If no regressions are detected using the SSL Labs tests and IE already supports stapling by default, then that should provide enough assurance that OCSP stapling will not be problematic to enable for FF 4.0.

	Adam Langley
	Comment 10 • 14 years ago

I don't consider this a priority I'm afraid. OCSP stapling is useful for very large sites to reduce load on OCSP responders and variance in revocation checking times. (That's why google.com uses it.) But it's busted by design. It can only carry a single response and hardly any sites have only one OCSP certificate in their chain these days.

So it doesn't eliminate the OCSP lookup delay, which it's primary attraction.

In the future, a multi-OCSP extension could be deployed, but I don't currently have the capacity to push that through at the moment.

	Gervase Markham [:gerv]
	Comment 11 • 14 years ago

The primary attraction for sites is to eliminate the delay (although that could also be done, to an extent, by making the request asynchronous and aborting document loading if it fails), but the primary attraction for CAs is to avoid melting their OCSP servers.

I agree that the right topology for certs has an (offline) root, an intermediate and an end-entity cert, and the latter two should have OCSP capability. But surely we can e.g. have the end-entity response returned stapled, and it's quite possible the intermediate (if it's from a common CA) will be cached?

Having said that, permitting the stapling of only one response was rather, er, short-sighted. :-|

Gerv

	Kai Engert (:KaiE:) Assignee
	Comment 12 • 14 years ago

(In reply to comment #11)
>
> Having said that, permitting the stapling of only one response was rather, er,
> short-sighted. :-|

Could we suggest to the authors of RFC 4366, or the IETF-TLS mailing list, that a new variable-length TLS-extension should be created, that may n OCSP responses, where n <= length of chain? Maybe that's not too difficult to get RFC'd?

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 13 • 14 years ago

Being able to get the one OCSP response stapled is still a big benefit, especially for popular CAs as it is likely we would already have the CA OCSP responses in the cache (especially after we implement the persistent cache).

> Could we suggest to the authors of RFC 4366, or the IETF-TLS mailing list, that
> a new variable-length TLS-extension should be created, that may n OCSP
> responses, where n <= length of chain? Maybe that's not too difficult to get
> RFC'd?

There is/was already a draft for such an extension. See Bug 611836.

	Rob Stradling
	Comment 14 • 14 years ago

(In reply to comment #9)
<snip>
> Nominated as a FF 4.0 blocker. I discussed this with Sid Stamm and Dan Veditz
> and it was determined that this is important enough to try to get into the FF
> 4.0 release because it is a big privacy and performance win when the server
> supports it, and because it will help strengthen our CA policy (see bug 585122)
> to do EV revocation checking correctly. Besides this patch, we would need to
> create a patch to implement enough of the server side to enable automated
> tests.

I understand that FF 4.0 is likely to be released very soon.  Will it feature support for OCSP Stapling?

	Gervase Markham [:gerv]
	Comment 15 • 14 years ago

Rob: no. I'm afraid we are well past the time for taking changes of this magnitude. This bug has been marked "blocking2.0-", which means the product team will not block releasing Firefox for this bug. Also, no work has been done on the patch since 2009.

Gerv

	Adam Langley
	Comment 16 • 14 years ago

(side note: Chromium has been running with this patch on Linux for some time and, therefore, should support OCSP stapling. I have, sadly, been too busy recently to actually verify that everything is actually working, but it certainly didn't break anything. We only support it on Linux because, on other platforms, we use the platform's certificate validation. On Windows this means that we can plumb the stapling response from libssl into the validation. On Mac, I don't think we support stapling.)

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 17 • 14 years ago

I agree with the blocking decision here, removing [d?]

	Kai Engert (:KaiE:) Assignee
	Comment 18 • 14 years ago

Do we know of sites who already implement OCSP stapling, that could be used to test this code?

	Wan-Teh Chang
	Comment 19 • 14 years ago

You can try Google's websites.  Adam Langley set up OCSP stapling
on them.

	Adam Langley
	Comment 20 • 14 years ago

Actually, Google isn't running OCSP stapling at the moment. It was shutdown due to the OpenSSL security issue [1] and I didn't switch it back on afterwards. It wasn't clear that the extra 1KB per connection was worthwhile when clients were caching the OCSP reply on disk anyway.

accounts.live.com:443 is doing stapling however if you need a test.

[1] http://www.openssl.org/news/secadv_20110208.txt

	Kai Engert (:KaiE:) Assignee
	Comment 21 • 14 years ago

Thank you very much for this initial patch, which was a helpful starting point. I merged the patch to NSS trunk and enhanced it.

We should try to avoid the need to allocate-and-save the certificate message.
This enhanced patch attempts to implement that.

The idea is, split funtion ssl3_HandCertificate into two pieces. The second piece is the authentication piece, which get delayed, until we're sure the CertStatus has been received and imported into the cache.

I didn't like the proposed implementation of GetStapledData, because the caller should be able to retrieve the required buffer size, and not be required to guess it. I enhanced flexibility of the function, see the updated interface description in the patch.

r=kaie on the locking in SSL_GetOCSPStapledData, because that is the same as being used in similar functions.

The patch did not yet insert any data into the cache (this has already been mentioned in the bug). I made an initial attempt to implement that. It builds, but is not yet tested.

Thanks to the person who had implmented CERT_CacheOCSPResponseFromSideChannel already. 

This is a local diff. I'll attach a real cvs diff with more context etc. later.

	Adam Langley
	Comment 22 • 14 years ago

I agree that allocating and saving the certificate message was a mess. I think I like your solution better.

The old GetStapledData could be called with NULL, 0 in order to get the size of the buffer but, if that wasn't obvious, then it was a bad API.

Also, the reason for not calling CERT_CacheOCSPResponseFromSideChannel was that, in Chromium, we don't ship with libnss on Linux so we can't know if that function is available. However, that shouldn't influence the code that goes into NSS.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 23 • 14 years ago

(In reply to comment #21)
> The idea is, split funtion ssl3_HandCertificate into two pieces. The second
> piece is the authentication piece, which get delayed, until we're sure the
> CertStatus has been received and imported into the cache.

This is a good idea, but don't rename the ssl3_HandleCertficate to ssl3_HandleCertificateData. In ssl3con, all the ssl3_Handle* functions are named to match the handshake messages they process, and we should not change this.

> Thanks to the person who had implmented
> CERT_CacheOCSPResponseFromSideChannel already. 

We will need to add the automated tests for this function, to make sure it behaves as we expect. (We should also document our expectations of the semantics. A "revoked" status in the cache should never be replaced with a status that isn't "revoked," even if the non-revoked status is newer. Also, how do we deal with "try later", etc.)

I am no fan of "goto," but I don't think the refactoring of the error handling in ssl3_HandleCertificate is necessary and it makes the patch very hard to review. Please leave the error handling as it was.

Your new ss->ssl3.hs.pending_certs member is basically the same as ss->sec.peerCertChain. It is OK to assign ss->sec.peerCertChain in ssl3_HandleCertificate instead. In fact, it is useful to do so, not just for this bug, but for bug 651996 too. Note also that everything in ss->sec is "pending" during a handshake.

	Kai Engert (:KaiE:) Assignee
	Comment 24 • 14 years ago

(In reply to comment #23)
> (In reply to comment #21)
> > The idea is, split funtion ssl3_HandCertificate into two pieces. The second
> > piece is the authentication piece, which get delayed, until we're sure the
> > CertStatus has been received and imported into the cache.
> 
> This is a good idea, but don't rename the ssl3_HandleCertficate to
> ssl3_HandleCertificateData. In ssl3con, all the ssl3_Handle* functions are
> named to match the handshake messages they process, and we should not change
> this.

ok


> > Thanks to the person who had implmented
> > CERT_CacheOCSPResponseFromSideChannel already. 
> 
> We will need to add the automated tests for this function, to make sure it
> behaves as we expect. (We should also document our expectations of the
> semantics. A "revoked" status in the cache should never be replaced with a
> status that isn't "revoked," even if the non-revoked status is newer.

I agree in general, but the logic might need improvement for "temporarily rejected" messages.


> Also,
> how do we deal with "try later", etc.)

I think the existing cache only stores meaningful responses.


> I am no fan of "goto," but I don't think the refactoring of the error
> handling in ssl3_HandleCertificate is necessary and it makes the patch very
> hard to review. Please leave the error handling as it was.

Removing the goto was necessary in order to split the function.

I don't understand why you see a reason to do a newer patch that tries to add gotos back. I think this is a good opportunity to get rid of the gotos.



> Your new ss->ssl3.hs.pending_certs member is basically the same as
> ss->sec.peerCertChain. It is OK to assign ss->sec.peerCertChain in
> ssl3_HandleCertificate instead. In fact, it is useful to do so, not just for
> this bug, but for bug 651996 too. Note also that everything in ss->sec is
> "pending" during a handshake.

I would have to look at this in more detail.

	Rob Stradling
	Comment 25 • 14 years ago

(In reply to comment #24)
> (In reply to comment #23)
> > (We should also document our expectations of the semantics. A "revoked"
> > status in the cache should never be replaced with a status that isn't
> > "revoked," even if the non-revoked status is newer.
>
> I agree in general, but the logic might need improvement for "temporarily
> rejected" messages.

RFC 2560 says:
   The "revoked" state indicates that the certificate has been revoked
   (either permanantly [sic] or temporarily (on hold)).

So I think that a newer non-revoked status *should* replace an older revoked status in the cache.

	Kai Engert (:KaiE:) Assignee
	Comment 26 • 13 years ago

This is the previous patch merged to NSS trunk.

This patch passed my tests. I'll provide some updated on the tests I performed.

Let's try to get this into 3.13.3

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 27 • 13 years ago

Comment on attachment 572853 [details] [diff] [review]
library patch v3

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

Heads up: Your refactoring of ssl3_HandleCertificate has many conflicts with the work I am doing for bug 542832.

I did not review the entire patch but I have added a few comments for ssl3con.c only.

::: mozilla/security/nss/lib/ssl/ssl3con.c
@@ +7878,5 @@
>   * ssl3 Certificate message.
>   * Caller must hold Handshake and RecvBuf locks.
>   */
>  static SECStatus
> +ssl3_HandleCertificateData(sslSocket *ss, SSL3Opaque *b, PRUint32 length)

Don't rename this function. All ssl3_HandleXXX functions are named after the handshake message that they parse, and we should maintain this naming convention.

@@ +8033,3 @@
>  
> +static SECStatus
> +ssl3_HandleCertificateAuth(sslSocket *ss, SSL3Opaque *b, PRUint32 length)

Let's not use the ssl3_Handle* naming convention for this function because ssl3_Handle* means something specific in libssl. I suggest "ssl3_AuthCertificate".

@@ +8034,5 @@
> +static SECStatus
> +ssl3_HandleCertificateAuth(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
> +{
> +    SECStatus        rv;
> +    PRBool           isServer   = (PRBool)(!!ss->sec.isServer);

Need to get or assert that we have a lock here (I think ss->sec.isServer is protected by the 1st handshake lock).

@@ +8036,5 @@
> +{
> +    SECStatus        rv;
> +    PRBool           isServer   = (PRBool)(!!ss->sec.isServer);
> +    int              errCode    = SSL_ERROR_RX_MALFORMED_CERTIFICATE;
> +    PRBool isTLS = (PRBool)(ss->ssl3.prSpec->version > SSL_LIBRARY_VERSION_3_0);

AFAICT, you can get the negotiated version from ss->version instead. Otherwise, you would need to get or assert the spec. read lock before accessing ss->ssl3.prSpec.

@@ +8037,5 @@
> +    SECStatus        rv;
> +    PRBool           isServer   = (PRBool)(!!ss->sec.isServer);
> +    int              errCode    = SSL_ERROR_RX_MALFORMED_CERTIFICATE;
> +    PRBool isTLS = (PRBool)(ss->ssl3.prSpec->version > SSL_LIBRARY_VERSION_3_0);
> +    ssl3CertNode * certs = ss->ssl3.hs.pending_certs;

Need to get or assert the SSL3 handshake lock before accessing ss->ssl3.hs

@@ +8229,5 @@
> +loser(sslSocket *ss, ssl3CertNode *certs, SSL3AlertDescription desc, int errCode)
> +{
> +    ss->ssl3.peerCertChain = certs;
> +    /* what's the purpose of assigning null to these pointers?
> +     * is this an old leak?

No, it isn't a leak and I have clarified the code in my patch for bug 542832.

@@ +8820,4 @@
>  	rv = ssl3_HandleServerHello(ss, b, length);
>  	break;
>      case certificate:
> +	rv = ssl3_HandleCertificateData(ss, b, length);

It is important, for performance reasons, to start the certificate authentication as early as possible. In particular, we should call ssl3_HandleCertificateAuth here if !ss->ssl3.hs.may_get_cert_status.

@@ +8835,5 @@
>  	    return SECFailure;
>  	}
> +        rv = ssl3_HandleCertificateAuth(ss, b, length);
> +	if (rv != SECSuccess)
> +	    break;

AFAICT, this call isn't necessary if we do what I suggested above (adding the ssl3_HandleCertificateAuth call back to the processing of the Certificate message.)

@@ +8845,4 @@
>  	    PORT_SetError(SSL_ERROR_RX_UNEXPECTED_CERT_REQUEST);
>  	    return SECFailure;
>  	}
> +        rv = ssl3_HandleCertificateAuth(ss, b, length);

Ditto (see previous comment).

@@ +8861,4 @@
>  	    PORT_SetError(SSL_ERROR_RX_UNEXPECTED_HELLO_DONE);
>  	    return SECFailure;
>  	}
> +        rv = ssl3_HandleCertificateAuth(ss, b, length);

Ditto.

@@ +9714,5 @@
>  	ss->ssl3.hs.messages.len = 0;
>  	ss->ssl3.hs.messages.space = 0;
>      }
> +    ss->ssl3.hs.pending_certs = NULL;
> +    if (ss->ssl3.hs.cert_status.data) {

We should free the cert_status data as soon as we've called CERT_CacheOCSPResponseFromSideChannel, and in ssl3_DestroySSL3Info.

::: mozilla/security/nss/lib/ssl/ssl3ext.c
@@ +1056,5 @@
>  SECStatus
> +ssl3_ClientHandleCertificateStatusXtn(sslSocket *ss, PRUint16 ex_type,
> +                                      SECItem *data)
> +{
> +    /* If we didn't request this extension, then the server may not echo it. */

I believe that libssl enforces this automatically. (I would double-check though).

	Kai Engert (:KaiE:) Assignee
	Comment 28 • 13 years ago

> > This is a good idea, but don't rename the ssl3_HandleCertficate to
> > ssl3_HandleCertificateData. 

I will reverted this change in the next patch.


> > > Thanks to the person who had implmented
> > > CERT_CacheOCSPResponseFromSideChannel already. 
> > 
> > We will need to add the automated tests for this function, to make sure it
> > behaves as we expect.

I cannot offer automated tests for this yet.
But I performed manual testing.

I've set up 3 server ports that support OCSP stapling.
Thanks to StartCom for providing me with free certs and free revocation for test purposes.

- https://kuix.de:5143
  uses a valid cert, with OCSP data saying "good cert"

- https://kuix.de:5144
  uses a revoked cert, with OCSP data saying "bad cert"

- https://kuix.de:5145
  uses an old expired test cert, where the OCSP server responds with "unauthorized request"

For testing purposes, I built Firefox with this patch, plus the patch from bug 700693 to enable client side request, set the pref to "true/enabled", and traced the OCSP behaviour, both using a network sniffing tool (tcpdump) and the OCSP tracing mechanisms (https://wiki.mozilla.org/NSS:Tracing ). Also I set OCSP to mandatory in Firefox validation prefs.

At port 5143, the good OCSP response gets inserted into the cache.
As expected, NSS will not attempt to contact the OCSP server.
This confirms that CERT_CacheOCSPResponseFromSideChannel works.

At port 5144 and port 5145, Firefox will ignore the stapled responses coming in from the SSL server. This is expected as commented in the patch. (We don't want a MITM server to send false information and poison our cache.) As expected, NSS will contact the OCSP server for current information.


> (We should also document our expectations of the
> > semantics.

Brian, where do you want the behaviour of CERT_CacheOCSPResponseFromSideChannel documented?
Which documentation is missing?
Please have a look at the implementation of CERT_CacheOCSPResponseFromSideChannel which already has a detailed comment.


> > > A "revoked"
> > > status in the cache should never be replaced with a status that isn't
> > > "revoked," even if the non-revoked status is newer.
> >
> > I agree in general, but the logic might need improvement for "temporarily
> > rejected" messages.
> 
> RFC 2560 says:
>    The "revoked" state indicates that the certificate has been revoked
>    (either permanantly [sic] or temporarily (on hold)).
> 
> So I think that a newer non-revoked status *should* replace an older revoked
> status in the cache.

In my opinion, for the sake of simplicity, this detail should be dealt with in a separate bug.

This concern is a general remark about the behaviour of the OCSP cache, which is not modified in this bug/task.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 29 • 13 years ago

(In reply to Kai Engert (:kaie) from comment #28)
> > > We will need to add the automated tests for this function, to make sure it
> > > behaves as we expect.
> 
> I cannot offer automated tests for this yet.
> But I performed manual testing.

I think I have some code laying around to support OCSP unit tests. I will find it and post it for you.

> - https://kuix.de:5143
>   uses a valid cert, with OCSP data saying "good cert"
> 
> - https://kuix.de:5144
>   uses a revoked cert, with OCSP data saying "bad cert"
> 
> - https://kuix.de:5145
>   uses an old expired test cert, where the OCSP server responds with
> "unauthorized request"

> > (We should also document our expectations of the
> > > semantics.
> 
> Brian, where do you want the behaviour of
> CERT_CacheOCSPResponseFromSideChannel documented?
> Which documentation is missing?
> Please have a look at the implementation of
> CERT_CacheOCSPResponseFromSideChannel which already has a detailed comment.

I mean we should state clearly what semantics libssl's OCSP stapling support should have, not what semantics CERT_CacheOCSPResponseFromSideChannel should have. This will be necessary for the security review. I will post a separate comment detailing the semantics I think we should have once I have thought about it more.

> In my opinion, for the sake of simplicity, this detail should be dealt with
> in a separate bug.
>
> This concern is a general remark about the behaviour of the OCSP cache,
> which is not modified in this bug/task.

We have to answer these questions to know if using CERT_CacheOCSPResponseFromSideChannel is appropriate in the first place.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 30 • 13 years ago

Kai, I remember why we cannot add the call to CERT_CacheOCSPResponseFromSideChannel inside of libssl. CERT_CacheOCSPResponseFromSideChannel internally calls CERT_VerifyCert*. In fact, IIRC, it calls it multiple times, wastefully. We cannot call CERT_VerifyCert* within libssl, because:

(1) AIA fetching, CRL fetching, etc. may need to be done, and that may need to be done on the thread that the call into libssl was done on
(2) We do not know how the application wishes to validate certificates. It may use a non-NSS certificate validation mechanism but still want to support OCSP stapling.

I believe that the original patch that Adam Langley posted in this bug added a function that allows the application to retrieve the raw OCSP response bytes during certificate validation. Then, the application is responsible for getting its certificate path validation engine to see the OCSP response (e.g. by calling CERT_CacheOCSPResponseFromSideChannel). I think we should take that approach. Perhaps we can even take that patch as-is, since it was already reviewed for Chromium.

(Also, in my half-started work on OCSP improvements in libpkix, I noticed that OCSP validation in NSS is needlessly inefficient. In particular, it doesn't take advantage of the fact that the OCSP response must either be signed by the signer of the certificate, or by a certificate that is directly signed by the signer of the certificate. Instead, when verifying an OCSP response, it needlessly builds a path all the way to the root CA cert and then verifies that. This causes many unnecessary PKI operations and certdb accesses. AFAICT, the only way to have efficient OCSP response verification is to extend libpkix so that we can pass the stapled OCSP response to CERT_PKIXVerifyCert.)

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 31 • 13 years ago

(3) It uses non-libpkix certificate path validation instead of libpkix.
(4) If we were to change CERT_CacheOCSPResponseFromSideChannel to use libpkix, we would also have to add a mechanism to libssl to configure the libpkix cert_pi_* and/or cert_po_* flags for that validation. That would be very messy.

	Kai Engert (:KaiE:) Assignee
	Comment 32 • 13 years ago

Ok.

My understanding is, that my plan WOULD work based on the old logic and implementation of libssl and PSM's SSL thread.

I had a chat with Brian, and he's telling me that he's doing a major reorganization of this code.

I understand I will have to wait until this work is done.

	Kai Engert (:KaiE:) Assignee
	Comment 33 • 13 years ago

(In reply to Kai Engert (:kaie) from comment #32)
> 
> I understand I will have to wait until this work is done.


Brian, which bug is the main one that tracks your reorganization work, that I should wait for (and that this bug should depend on)?

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 34 • 13 years ago

Bug 674147 is the bug you are referring to. But, there is no reason for that Firefox bug to block work on this bug. IIRC, Chromium works in a very similar way and Adam or Wan-Teh could probably refute or confirm my comments in comment 30 and comment 31.

	Kai Engert (:KaiE:) Assignee
	Comment 35 • 13 years ago

I don't want to repeat the mistake of writing code that is no longer appropriate. Instead of trying to write code against a moving target, I would like to wait until your work has settled down and is known to be stable.

	Kai Engert (:KaiE:) Assignee
	Comment 36 • 13 years ago

Ok, after digesting comment 30 + 31...

I agree that the fact that our OCSP handling code has CERT_Verify* calls embedded is a problem, because the app wants to decide how to verify the OCSP signer cert (classic vs. pkix code). Verification should probably use the registered AuthCertificate callback.

I agree with your request that the application should trigger the verification, allowing it to control the thread that calls into verification.

However, where should the code reside, that processes the OCSP data, drives verification and cache injection? I tend to add this code to NSS, but allow the application to trigger this code at its prefered point of time.

Here is a proposed design:


- libSSL remembers the CertStatus data retrieved during the handshake,
  it can be queried using the file descriptor.

  (the patch already does that in function SSL_GetOCSPStapledData,
   I propose to rename the function to SSL_RevealCertificateStatus)


- libSSL triggers the cert validation by calling authCertificate,
  which calls either default function SSL_AuthCertificate or
  the application registered AuthCertificateHook
  
  (we already do that as of today)


- we implement a new function SSL_ProcessStapledCertStatus,
  which retrieves the stapled information using SSL_RevealCertificateStatus,
  iterates over the list of stapled OCSP responses and
  injects them into the cache.

  (It could call existing CERT_CacheOCSPResponseFromSideChannel 
   for each of the stapled responses).

  The function is a no-op if no status was retrieved from the server.


- we change the default SSL_AuthCertificate implementation
  to run SSL_ProcessStapledCertStatus prior to cert verification.


- in PSM, in the application provided authCertificate code,
  we'll call SSL_ProcessStapledCertStatus, prior to executing the 
  cert verification.

  PSM can return the would-block status prior to calling 
  SSL_ProcessStapledCertStatus, which means it can be executed in 
  the same asynchronous job that performs the certificate verification.

  With this approach, the handshake must be restarted only once,
  despite potentially verifying multiple certificates.


How can we control whether classic or pkix verification code will be called?

In my opinion, we must allow NSS code to execute verification functions on its own,
I assume NSS already does that in various scenarios.

In my opinion, the OCSP response handling code (in particular function
CERT_VerifyOCSPResponseSignature), which as of today calls CERT_VerifyCert,
should *not* be changed to call an application provided callback.

(In particular the authCertificate hook is inappropriate, because there is
 no SSL socket involved at this point.)

We should change CERT_VerifyOCSPResponseSignature to behave depending on
NSS' global flag usePKIXValidationEngine, which can be set by the application
using CERT_SetUsePKIXForValidation.

Depending on the flag, CERT_VerifyOCSPResponseSignature should call either
classic or pkix validation code, using NSS-level defined parameters.

(If you didn't like this approach, we whould have to add a new callback function
 for the purpose of application performed cert verification,
 independent from the context of a file descriptor.)


(Also note that PSM currently does not make any calls to the default 
 implementation SSL_AuthCertificate. With the above plan, processing of the
 cert status will happen only once.)


Another related proposal:
I think the application should not deal with the raw OCSP bytes, because some of the knowledge required for decoding the response might be private? I might be wrong here, but instead, my preference is to do the following:

Function SSL_GetOCSPStapledData (renamed to SSL_RevealCertificateStatus) should return a list of OCSP responses.

Why? My preference is that our implementation is already prepared with future multi-stapling. The code that retrieves data from SSL_RevealCertificateStatus shouldn't have to guess whether the data is a single OCSP response or a list of responses. By returning a list of OCSP responses, this code can be compatible with today and the future requirement of a multiple stapling handshakes.

	Kai Engert (:KaiE:) Assignee
	Comment 37 • 13 years ago

(In reply to Adam Langley from comment #20)
> 
> accounts.live.com:443 is doing stapling however if you need a test.

This site is gone, but https://login.live.com/ now uses stapling.

	Kai Engert (:KaiE:) Assignee
	Comment 38 • 13 years ago

CVS Patch for NSS: I'm unable to create that at this time, because bug 542832 is not checked in, and my code needs careful merging with that code.

I can attach a patch that was created using hg - it applies fine on the NSS tree, after having applied the patches from bug 542832 first.

	Kai Engert (:KaiE:) Assignee
	Comment 39 • 13 years ago

patch on top of bug 542832

	Kai Engert (:KaiE:) Assignee
	Comment 40 • 13 years ago

Patch v10 implements most of comment 36.

It does not yet implement the dynamic switch between classic/pkix cert verification code inside the OCSP verification code. I would like to do that in a future step.

I attached a patch with PSM level changes to bug 700693.

Initial testing with Firefox looks good. (Not yet tested with the NSS test suite.)

	Kai Engert (:KaiE:) Assignee
	Comment 41 • 13 years ago

(In reply to Brian Smith (:bsmith) from comment #29)
> 
> I think I have some code laying around to support OCSP unit tests. I will
> find it and post it for you.

Have you found it?

I would appreciate help or advice on adding automated tests for OCSP staplig.

What's today's state of OCSP testing?
Does it create OCSP responses on its own?

(If we didn't have any automated creation of OCSP responses yet, then I'd argue, manual testing should be sufficient to land this code.)

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 42 • 13 years ago

(In reply to Kai Engert (:kaie) from comment #36)
> I agree that the fact that our OCSP handling code has CERT_Verify* calls
> embedded is a problem, because the app wants to decide how to verify the
> OCSP signer cert (classic vs. pkix code). Verification should probably use
> the registered AuthCertificate callback.

I think it is OK to leave the CERT_Verify* calls for now, as long as we don't call the functions within libssl, or on the socket transport thread within Gecko.

> - we implement a new function SSL_ProcessStapledCertStatus,
>   which retrieves the stapled information using SSL_RevealCertificateStatus,
>   iterates over the list of stapled OCSP responses and
>   injects them into the cache.
>
>   (It could call existing CERT_CacheOCSPResponseFromSideChannel 
>    for each of the stapled responses).

I would prefer that we do not couple libssl to certhigh or any of the certificate validation/OCSP functions. Google Chrome on Windows uses libssl without using those functions, for example. I think it is reasonable to just have the application (PSM) loop through all the stapled OCSP response data itself for now, calling CERT_CacheOCSPResponseFromSideChannel itself.

> - we change the default SSL_AuthCertificate implementation
>   to run SSL_ProcessStapledCertStatus prior to cert verification.

+1. 

>   PSM can return the would-block status prior to calling 
>   SSL_ProcessStapledCertStatus, which means it can be executed in 
>   the same asynchronous job that performs the certificate verification.

PSM needs to extract out the OCSP response data before returning would-block, put them call do the calls to CacheOCSPResponseFromSideChannel on one of the certificate validation threads, for the reasons I explained in bug 700693 comment 3.

> We should change CERT_VerifyOCSPResponseSignature to behave depending on
> NSS' global flag usePKIXValidationEngine, which can be set by the application
> using CERT_SetUsePKIXForValidation.

I believe that, since CERT_VerifyOCSPResponseSignature calls CERT_VerifyCert, and CERT_VerifyCert already respects the "use libpkix" flag, this already happens.

> Another related proposal:
> I think the application should not deal with the raw OCSP bytes, because
> some of the knowledge required for decoding the response might be private? I
> might be wrong here, but instead, my preference is to do the following:
> 
> Function SSL_GetOCSPStapledData (renamed to SSL_RevealCertificateStatus)
> should return a list of OCSP responses.
> 
> Why? My preference is that our implementation is already prepared with
> future multi-stapling. The code that retrieves data from
> SSL_RevealCertificateStatus shouldn't have to guess whether the data is a
> single OCSP response or a list of responses. By returning a list of OCSP
> responses, this code can be compatible with today and the future requirement
> of a multiple stapling handshakes.

I agree that, if it is simple and clear how to add multi-stapling support, then it is OK to take that into consideration for this API. But, I would prefer to return a list of SECItems that contain the raw OCSP response data. I think this would accomplish the same thing with respect to future multi-stapling support. I would avoid adding any references to the CERTOCSPResponse type to the libssl API for the reason I mentioned previously--we shouldn't add a new dependency on certhigh's OCSP types or data structures, to support applications that use non-NSS certificate validation with libssl, like Chromium.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 43 • 13 years ago

(In reply to Brian Smith (:bsmith) from comment #42)
> PSM needs to extract out the OCSP response data before returning
> would-block, put them call do the calls to CacheOCSPResponseFromSideChannel

s/put them/but then/

	Kai Engert (:KaiE:) Assignee
	Comment 44 • 13 years ago

Brian, I believe this patch addresses all your requests and suggestions.

I still want to provide a kind of default implementation.
In order to avoid the dependency between OCSP and SSL code, I introduced a new callback function.

An application that wishes to use the default processing can call 
  SSL_ProcessCertStatusHook(fd, CERT_CacheOCSPResponseFromSideChannel)
and it will be called from inside the default SSL_AuthCertificate code.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 45 • 13 years ago

Comment on attachment 581795 [details] [diff] [review]
patch v11

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

Please include showfunc = true in your hgrc, so that the patch context includes the function name.

You set the sr? but did not provide an email address. Who do you intend to do the sr? I recommend Wan-Teh, especially if agl doesn't have time to provide feedback.

::: security/nss/lib/certhigh/ocsp.c
@@ +809,5 @@
>      SECStatus rv;
>      OCSPCacheItem *cacheItem;
>      OCSP_TRACE(("OCSP ocsp_CreateOrUpdateCacheEntry\n"));
>    
> +    if (certIDWasConsumed)

Kai, why not just pass a valid pointer for certIDWasConsumed in the new code below? Then we can avoid changing this function, which seems to be used via many code paths, which do not pass NULL here.

@@ +4893,5 @@
>   *   CERTCertDBHandle *handle
>   *     certificate DB of the cert that is being checked
>   *   CERTCertificate *cert
>   *     the certificate being checked
> + *     if cert is null, function will cache all valid responses

A caller should never pass NULL for cert. At least for OCSP stapling, we never need to do so.

The check that that the OCSP response is applicable to a given certificate is a very important one for security. If you don't think this check is needed, please explain why you think it is safe to omit it, and also why it is necessary for this change to be made for enabling EE OCSP stapling.

::: security/nss/lib/ssl/ssl.h
@@ +397,5 @@
> + * should call
> + *      SSL_ProcessCertStatusHook(fd, CERT_CacheOCSPResponseFromSideChannel)
> + * after each socket construction.
> + */
> +SSL_IMPORT SECStatus SSL_ProcessCertStatusHook(PRFileDesc *fd, 

Sorry I wasn't clear before. I think it is OK for SSL_AuthCertificate to call CERT_CacheOCSPResponseFromSideChannel directly, because (a) SSL_AuthCertificate already has a dependency on certhigh, (b) applications that don't want to use NSS for certificate validation will not use SSL_AuthCertificate, and (c) Chromium builds NSS in such a way that SSL_AuthCertificate doesn't get linked in, so it shouldn't matter for them, AFAICT.

Accordingly, we don't need SSL_ProcessCertStatusHook.

::: security/nss/lib/ssl/ssl3con.c
@@ +7827,5 @@
> +    SSL3AlertDescription desc;
> +
> +    if (!ss->ssl3.hs.may_get_cert_status ||
> +       ss->ssl3.hs.ws != wait_server_cert ||
> +       ss->ssl3.hs.cert_status.len) {

AFAICT, this condition should just be ss->ssl3.hs.ws != wait_cert_status. The spec says:

> If a server returns a "CertificateStatus" message, then
> the server MUST have included an extension of type 
> "status_request" with empty "extension_data" in the
> extended server hello.

In ssl3_HandleCertificate, we can set ss->ssl3.hs.ws to wait_cert_status, and check in ssl3_HandleCertificateStatus that ws == wait_cert_status. That way, we can guard against the case where a CertificateStatus message is sent before (or instead of) the Certificate message, and/or the case where multiple CertificateStatus messages are sent.

To handle the case where the server negotiated the extension but doesn't include a CertificateStatus message, ssl3_HandleCertificateAuth should advance from the wait_cert_status to the next state. (In fact, it already advances to the next state.)

Such changes may make pendingHandleCertificateAuth redundant. (I am not sure.)

@@ +7835,5 @@
> +    }
> +
> +    status = ssl3_ConsumeHandshakeNumber(ss, 1, &b, &length);
> +    if (status != 1)
> +       goto format_loser;

This 4-space-then-3-space indention style doesn't match the rest of libssl. Tabs are always 4 spaces and it is also required to use tabs instead of 8 leading spaces in libssl. (Maybe you are using 8 spaces and this is just a weird thing in the Splinter review tool; in any case, these should be tabs.)

@@ +7845,5 @@
> +    SECITEMARRAY_Alloc(NULL, &ss->ssl3.hs.cert_status, 1);
> +    if (!ss->ssl3.hs.cert_status.items)
> +       return SECFailure;
> +
> +    ss->ssl3.hs.cert_status.items[0].data = PORT_Alloc(length);

We need to check that the OCSP response is of a sane size, like we do for the Certificate message. I suggest 16KB is more than enough.

@@ +7859,5 @@
> +    return SECSuccess;
> +
> +format_loser:
> +    errCode = SSL_ERROR_BAD_CERT_STATUS_RESPONSE_ALERT;
> +    desc = bad_certificate_status_response;

I believe that the bad_certificate_status_response is only supposed to be returned if there is something wrong with the OCSP response itself, not the CertificateStatus message. I think decode_error is the correct alert for these types of errors. (And then, you can use your new decode_loser function to report the errror.)

The cases where bad_certificate_status_response should be sent are cases like (a) the OCSP response's cert ID doesn't match the EE certificate, (b) the OCSP response was signed by a CA that didn't issue the certificate, (c) the OCSP response couldn't be parsed, etc. Basically, these are the cases where CERT_CacheOCSPResponseFromSideChannel returns an error (other than memory allocation errors and the like).

Unfortunately, the spec says that we MUST return bad_certificate_status_response when the OCSP response is bad. AFAICT, the only way we can do that is to document that the cert auth hook must return SSL_ERROR_BAD_CERT_STATUS_RESPONSE_ALERT if there is a problem with the OCSP response, and then send the alert if it does so.

But, that means that the async interface I defined in bug 542832 is insufficient, because it doesn't allow an async auth cert hook to return errors to libssl. It looks like I need to change that interface so that it can do so. :(

@@ +7914,5 @@
>  	CERT_DestroyCertificate(ss->sec.peerCert);
>  	ss->sec.peerCert = NULL;
>      }
> +    
> +    ss->ssl3.hs.pendingHandleCertificateAuth = PR_TRUE;

Note that in the "no certificate" case for servers, pendingHandleCertificateAuth should be set to false before we "return SECSuccess" below, right?

@@ +7944,5 @@
> +            return loser(ss, desc, errCode);
> +	}
> +        ss->ssl3.hs.ws = wait_client_key;
> +        return SECSuccess;
> +        /*return check_loser(rv, ss, desc);*/

remove the comment.

@@ +8033,5 @@
> +    return SECSuccess;
> +}
> +
> +static SECStatus
> +ssl3_HandleCertificateAuth(sslSocket *ss, SSL3Opaque *b, PRUint32 length)

I suggest using the name ssl3_AuthCertificateIfNeeded. ssl3_Handle* is the convention for functions that parse peer records/messages.

@@ +8037,5 @@
> +ssl3_HandleCertificateAuth(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
> +{
> +    SECStatus        rv;
> +    PRBool           isServer   = (PRBool)(!!ss->sec.isServer);
> +    int              errCode    = SSL_ERROR_RX_MALFORMED_CERTIFICATE;

Do not initialize errCode to SSL_ERROR_RX_MALFORMED_CERTIFICATE because that error code is not valid for anything this function checks.

@@ +8039,5 @@
> +    SECStatus        rv;
> +    PRBool           isServer   = (PRBool)(!!ss->sec.isServer);
> +    int              errCode    = SSL_ERROR_RX_MALFORMED_CERTIFICATE;
> +    PRBool isTLS = (PRBool)(ss->ssl3.prSpec->version > SSL_LIBRARY_VERSION_3_0);
> +    SSL3AlertDescription desc   = bad_certificate;

We may need to send either bad_certificate or bad_certificate_status_response depending on why the cert authentication failed.

(Again, there is a bug in my definition of SSL_RestartHandshakeAfterAuthCertificate that causes us to never send a bad_certificate alert.)

@@ +8809,5 @@
>  	}
>  	rv = ssl3_HandleServerHello(ss, b, length);
>  	break;
>      case certificate:
>  	rv = ssl3_HandleCertificate(ss, b, length);

Here, we should call ssl3_HandleCertificateAuth if the certificate status extension wasn't negotiated, because we want to start authenticating the certificate ASAP for performance reasons.

@@ +9699,5 @@
>  	ss->ssl3.hs.messages.len = 0;
>  	ss->ssl3.hs.messages.space = 0;
>      }
>  
> +    if (ss->ssl3.hs.cert_status.len) {

This is good. But, we should be careful to document in ssl.h that the certificate status data is only available within the auth certificate hook, and not elsewhere.

::: security/nss/lib/ssl/ssl3ext.c
@@ +1056,5 @@
> +ssl3_ClientHandleCertificateStatusXtn(sslSocket *ss, PRUint16 ex_type,
> +                                      SECItem *data)
> +{
> +    /* If we didn't request this extension, then the server may not echo it. */
> +    if (!ss->opt.enableOCSPStapling)

Is this check necessary? i thought there was already a check in the code that would call this, such that this function will only be called when the extension was correctly negotiated.

@@ +1060,5 @@
> +    if (!ss->opt.enableOCSPStapling)
> +	return SECFailure;
> +
> +    /* The echoed extension must be empty. */
> +    if (data->len != 0)

Need to set error code.

@@ +1063,5 @@
> +    /* The echoed extension must be empty. */
> +    if (data->len != 0)
> +	return SECFailure;
> +
> +    ss->ssl3.hs.may_get_cert_status = PR_TRUE;

AFAICT, we don't need this variable. We can just call ssl3_ExtensionNegotiated in ssl3_HandleCertificateStatus.

::: security/nss/lib/ssl/sslauth.c
@@ +263,5 @@
>      CERTCertDBHandle * handle;
>      sslSocket *        ss;
>      SECCertUsage       certUsage;
>      const char *             hostname    = NULL;
> +    PRTime             tnow = PR_Now();

Nit: "now" is a better name.

::: security/nss/lib/ssl/sslimpl.h
@@ +779,5 @@
>      SECItem               ca_list;     /* used only by client */
>      PRBool                isResuming;  /* are we resuming a session */
>      PRBool                usedStepDownKey;  /* we did a server key exchange. */
>      PRBool                sendingSCSV; /* instead of empty RI */
> +    PRBool                may_get_cert_status; /* the server echoed a

use camelCase.

@@ +784,5 @@
> +                                                  cert_status extension so may
> +                                                  send a CertificateStatus
> +                                                  handshake message. */
> +    PRBool                pendingHandleCertificateAuth;
> +    SECItemArray          cert_status; /* an array of OCSP responses */

use camelCase.

::: security/nss/lib/ssl/sslreveal.c
@@ +65,5 @@
>  
> +/* TODO: Implement new function SSL_RevealCertList
> + *       that returns all certificates sent by the server.
> + *       It will be required to implement tls-ext-multiple-ocsp.
> + */

Remove this comment. There is already an open bug in bugzilla for this feature.

@@ +105,5 @@
>  
> +/* Given PRFileDesc, returns the OCSP information that was sent by the server
> + * during the handshake.
> + * We use a list of OCSP responses, to prepare for tls-ext-multiple-ocsp.
> + * Caller must free data when done.

Move the application-level documentation to ssl.h.

Document that the function may only be called from the auth certificate hook (or bad cert handler?).

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 46 • 13 years ago

I suggest that we do the following:

1. define a new error code SSL_ERROR_BAD_CERTIFICATE_STATUS_RESPONSE.

2. If the auth certificate hook returns SECfailure with PR_GetError() returning SSL_ERROR_BAD_CERTIFICATE_STATUS_RESPONSE, then send the bad_certificate_status_response alert. Otherwise, send the bad_certificate alert for other error codes as is currently done.

3. Update the patch in bug 542832 so that the SSL_RestartHandshakeAfterAuthCertificate function is given the following signature:

  SECStatus SSL_AuthCertificateComplete(PRFileDesc* fd, PRErrorCode status);

Then, interpret the value of status like we would interpret the value of PR_GetError() in #2 above (sending an alert mapped from the error code, if/as appropriate).

I am updating the patch in bug 542832 to do this now.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 47 • 13 years ago

I modified the patches in bug 542832 so that the application can pass the error back to libssl, so that libssl can send the appropriate alert. In the process of doing so, I had to factor out the code that maps errors to alerts in ssl3_HandleCertificate (see bug 542832 attachment 584595 [details] [diff] [review]). I believe this will make your patch much simpler as, AFAICT, it will no longer have to refactor the error handling in ssl3_HandleCertificate.

	Wan-Teh Chang
	Comment 48 • 13 years ago

This is the current version of Adam Langley's patch (attachment 418746 [details] [diff] [review]).

	Wan-Teh Chang
	Comment 49 • 13 years ago

Comment on attachment 616669 [details] [diff] [review]
OCSP stapling patch used in Chromium

Two comments on this patch:

1. The use of "unsigned int *len" by SSL_GetStapledOCSPResponse as an in/out argument
does not match the NSS convention.  We should add a separate "unsigned int maxLen"
input argument, and use "unsigned int *len" as an output argument.

2. Adam's original patch used the -O option to enable OCSP stapling in tstclnt,
presumably because "OCSP" starts with "O".

The -O option has since been taken for synchronous certificate validation.  So I
changed the patch to use -P to enable OCSP stapling.

	Kai Engert (:KaiE:) Assignee
	Comment 50 • 13 years ago

Comment on attachment 616669 [details] [diff] [review]
OCSP stapling patch used in Chromium

The alternative chromium patch described in comment 48 and 49 appears to be incomplete.

I don't see any interaction with the OCSP cache

(As a reminder, the interaction with the OCSP cache, using a strategy that avoids a direct call from libSSL into the OCSP cache, that was the reason why I have introduced callbacks and why my patch is longer.)

	Kai Engert (:KaiE:) Assignee
	Comment 51 • 13 years ago

Patch v11 == bitrotted

Merged to trunk => Patch v12

Now with function names and larger context (diff -u20 -p)

	Rob Stradling
	Comment 52 • 13 years ago

(In reply to Kai Engert (:kaie) from comment #50)
> I don't see any interaction with the OCSP cache

I think the code you're looking for is...
http://src.chromium.org/viewvc/chrome/trunk/src/net/socket/ssl_client_socket_nss.cc?view=markup
(Search for the first instance of "CertSetCertificateContextProperty").

	Kai Engert (:KaiE:) Assignee
	Comment 53 • 13 years ago

(In reply to Rob Stradling from comment #52)
> 
> I think the code you're looking for is...
> http://src.chromium.org/viewvc/chrome/trunk/src/net/socket/
> ssl_client_socket_nss.cc?view=markup
> (Search for the first instance of "CertSetCertificateContextProperty").

Thanks, but in my understanding that's application code, while this bug rather needs general purpose library level patches.

	Kai Engert (:KaiE:) Assignee
	Comment 54 • 13 years ago

> 
> 2. Adam's original patch used the -O option to enable OCSP stapling in
> tstclnt,
> presumably because "OCSP" starts with "O".
> 
> The -O option has since been taken for synchronous certificate validation. 
> So I
> changed the patch to use -P to enable OCSP stapling.


Let's handle enhancements of the tools in bug 700701.

	Kai Engert (:KaiE:) Assignee
	Comment 55 • 13 years ago

Comment on attachment 618252 [details] [diff] [review]
Patch v12

Patch fails in test suite, needs investigation

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 56 • 13 years ago

(In reply to Kai Engert (:kaie) from comment #53)
> Thanks, but in my understanding that's application code, while this bug
> rather needs general purpose library level patches.

You are both right. For Firefox (and Chrome of course) we should do things similar to the way Chrome does it now, but also the default SSL certificate validation function SSL_AuthCertificate should be modified to provide an example (and default implementation) for applications, using the same functions that Firefox and Chrome would use to extract out the stapled OCSP response and add it to the OCSP cache, as they do not use SSL_AuthCertificate. This was discussed already in previous review comments.

	Kai Engert (:KaiE:) Assignee
	Comment 57 • 13 years ago

> For Firefox (and Chrome of course) we should do things
> similar to the way Chrome does it now, 

I don't understand why you are proposing this.

Obviously the Chrome patch isn't a complete NSS library patch.

My patch is, and my patch already contains a solution for injecting the data into the OCSP cache.

	Kai Engert (:KaiE:) Assignee
	Comment 58 • 13 years ago

> using the same
> functions that Firefox and Chrome would use to extract out the stapled OCSP
> response and add it to the OCSP cache

I'm opposing any solution that requires application level code for this to function.

I don't want a solution that requires the application to deal with this.

I want a general purpose solution at the NSS library level, that will work by default for any application using NSS SSL.

Can I get any support for this point of view?

	Kai Engert (:KaiE:) Assignee
	Comment 59 • 13 years ago

Patch v12 required some small additions.

This patch v14 successfully passes the NSS test suite.

	Kai Engert (:KaiE:) Assignee
	Comment 60 • 13 years ago

I'll now try to work through the change proposals from comment 45 and later.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 61 • 13 years ago

(In reply to Kai Engert (:kaie) from comment #58)
> I don't want a solution that requires the application to deal with this.

> I want a general purpose solution at the NSS library level, that will work
> by default for any application using NSS SSL.

I agree that it is good to have a default implementation for most applications to use, and especially for use in testing at the NSS level.

But, AFAICT, it is easy to make your changes to ocsp/*, sslauth.c on top of Google's patch to get a default implementation, right? That would be much less code to review because AGL and WTC already wrote, reviewed, tested, and deployed to production the vast majority of the code.

In my previous comments, I mentioned that it is important that there not be any calls to ocsp_*/OCSP_* in the core of libssl. I see that you addeded the processCertStatus in response to that. I was unclear in what I meant: it is OK to call ocsp_*/OCSP_* in SSL_AuthCertificate, but not elsewhere in libssl, because applications for which those calls would be problematic (Chrome, Firefox) also need to avoid calling SSL_AuthCertificate for similar reasons. So, the complexity added by the processCertStatus callback part is unnecessary.

Also, in your patch, I didn't see a default implementation of the processCertStatus hook. Did I overlook it?

	Kai Engert (:KaiE:) Assignee
	Comment 62 • 13 years ago

(In reply to Brian Smith (:bsmith) from comment #61)
> 
> But, AFAICT, it is easy to make your changes to ocsp/*, sslauth.c on top of
> Google's patch to get a default implementation, right? 


Why should *I* write a new patch on top of someone's else patch, if I already have my own patch, which is more complete?

	Kai Engert (:KaiE:) Assignee
	Comment 63 • 13 years ago

(In reply to Brian Smith (:bsmith) from comment #45)
> 
> > +    if (certIDWasConsumed)
> 
> why not just pass a valid pointer for certIDWasConsumed in the new code
> below? Then we can avoid changing this function, which seems to be used via
> many code paths, which do not pass NULL here.


Sorry, I don't understand your proposal,
or the way I understand it doesn't make sense to me.

I'm not changing the way how this function is being used,
it should be fine to keep this function in the way I change it.

If you still think it's absolutely necessary that this change must be made,
the please explain in more detail.
Or maybe you should implement this change yourself, 
in order to push things forward.


> > + *     if cert is null, function will cache all valid responses
> 
> A caller should never pass NULL for cert. At least for OCSP stapling, we
> never need to do so.


Ok. I had misinterpreted the multi-stapling RFC (or it might have changed
after I read it the last time).

The latest draft 3 says that the multi stapling response may contain
OCSP responses for certificates found in the handshake message,
but not for any unrelated certificates.

I'm changing the code to require that a cert is provided.


> ::: security/nss/lib/ssl/ssl3con.c
> @@ +7827,5 @@
> > +    SSL3AlertDescription desc;
> > +
> > +    if (!ss->ssl3.hs.may_get_cert_status ||
> > +       ss->ssl3.hs.ws != wait_server_cert ||
> > +       ss->ssl3.hs.cert_status.len) {
> 
> AFAICT, this condition should just be ss->ssl3.hs.ws != wait_cert_status.


I had taken this code unchanged from the original patch.

Following your argument that the Chrome code has
already been tested, I propose we follow the latest implementation
of function ssl3_HandleCertificateStatus from the chrome patch

(... ignoring a small delta related to allocation,
 caused by the nature of the different "pending data"
 strategy used in the chrome patch).

I think this suggests that we should keep the current expression.

Given that this code works, I don't see a reason to follow
your suggestion.


> This 4-space-then-3-space indention style doesn't match the rest of libssl.


Please let's postpone any complaints about whitespace, 
and let's rather focus on wording towards a functionally acceptable patch.


> > +    SECITEMARRAY_Alloc(NULL, &ss->ssl3.hs.cert_status, 1);
> > +    if (!ss->ssl3.hs.cert_status.items)
> > +       return SECFailure;
> > +
> > +    ss->ssl3.hs.cert_status.items[0].data = PORT_Alloc(length);
> 
> We need to check that the OCSP response is of a sane size, like we do for
> the Certificate message. I suggest 16KB is more than enough.


I agree with the idea of a maximum, but let's be prepared for multi-stapling.

16 KB seems to small to me, if multiple certificates can be involved.
Even a much larger limit will protect us from
malicious servers that try to crash the client.

We should use an official limit, if there is one.
Let's start with a larger value.
You can file a follow-up bug if you want to lower this limit,
or research the correct limit.


> > +format_loser:
> > +    errCode = SSL_ERROR_BAD_CERT_STATUS_RESPONSE_ALERT;
> > +    desc = bad_certificate_status_response;
> 
> I believe that the bad_certificate_status_response is only supposed to be
> returned if there is something wrong with the OCSP response itself, not the
> CertificateStatus message. 


Given that you're willing to accept Chrome's patch,
and given that that patch uses the same approach,
I propose we postpone your worry to some followup bug.


> > +    ss->ssl3.hs.pendingHandleCertificateAuth = PR_TRUE;
> 
> Note that in the "no certificate" case for servers,
> pendingHandleCertificateAuth should be set to false before we "return
> SECSuccess" below, right?

Yes. I also discovered this today because of a test suite failure.
Done


> > +        /*return check_loser(rv, ss, desc);*/
> 
> remove the comment.

ok


> > +ssl3_HandleCertificateAuth(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
> 
> I suggest using the name ssl3_AuthCertificateIfNeeded. ssl3_Handle* is the
> convention for functions that parse peer records/messages.

ok




> > +ssl3_HandleCertificateAuth(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
> > +{
> > +    SECStatus        rv;
> > +    PRBool           isServer   = (PRBool)(!!ss->sec.isServer);
> > +    int              errCode    = SSL_ERROR_RX_MALFORMED_CERTIFICATE;
> 
> Do not initialize errCode to SSL_ERROR_RX_MALFORMED_CERTIFICATE because that
> error code is not valid for anything this function checks.

It's not "my" error code init.
This is the same default that was used in this code path.
Let's keep this default for now, rather than introducing new scenarios.


> > +    SECStatus        rv;
> > +    PRBool           isServer   = (PRBool)(!!ss->sec.isServer);
> > +    int              errCode    = SSL_ERROR_RX_MALFORMED_CERTIFICATE;
> > +    PRBool isTLS = (PRBool)(ss->ssl3.prSpec->version > SSL_LIBRARY_VERSION_3_0);
> > +    SSL3AlertDescription desc   = bad_certificate;
> 
> We may need to send either bad_certificate or
> bad_certificate_status_response depending on why the cert authentication
> failed.

This code path handles the certificate message, not the status message.


> >  	}
> >  	rv = ssl3_HandleServerHello(ss, b, length);
> >  	break;
> >      case certificate:
> >  	rv = ssl3_HandleCertificate(ss, b, length);
> 
> Here, we should call ssl3_HandleCertificateAuth if the certificate status
> extension wasn't negotiated, because we want to start authenticating the
> certificate ASAP for performance reasons.


This isn't a change, it's context.
This bug is about getting the functionality implemented.
Let's do performance tweaks in a follow up bug.


> > +    if (ss->ssl3.hs.cert_status.len) {
> 
> This is good. But, we should be careful to document in ssl.h that the
> certificate status data is only available within the auth certificate hook,
> and not elsewhere.

Should be unnecessary.
This cleanup code is called from ssl3_DestroySSL3Info,
which is called from ssl_DestroySocketContents,
and all the socket information is destroyed together.


> > +ssl3_ClientHandleCertificateStatusXtn(sslSocket *ss, PRUint16 ex_type,
> > +                                      SECItem *data)
> > +{
> > +    /* If we didn't request this extension, then the server may not echo it. */
> > +    if (!ss->opt.enableOCSPStapling)
> 
> Is this check necessary? i thought there was already a check in the code
> that would call this, such that this function will only be called when the
> extension was correctly negotiated.

I don't know.
Given this check is in both the original and the latest chrome patch,
let's keep it.


> > +    if (!ss->opt.enableOCSPStapling)
> > +	return SECFailure;
> > +
> > +    /* The echoed extension must be empty. */
> > +    if (data->len != 0)
> 
> Need to set error code.

Chrome patch doesn't set error code.


> > +    ss->ssl3.hs.may_get_cert_status = PR_TRUE;
> 
> AFAICT, we don't need this variable. We can just call
> ssl3_ExtensionNegotiated in ssl3_HandleCertificateStatus.

Chrome patch still uses it.
Let's keep it, as your proposal would require rewriting the logic,
and I'm glad that it's working.


> > +    PRBool                may_get_cert_status; /* the server echoed a
> 
> use camelCase.

ok


> > +                                                  handshake message. */
> > +    PRBool                pendingHandleCertificateAuth;
> > +    SECItemArray          cert_status; /* an array of OCSP responses */
> 
> use camelCase.

If we make this change, it will make it quite difficult 
to compare earlier patches with the newer patches.

This is an unnecessary change request,
there are plenty of other member variables inside the same structure
that use the underscore style.


> > +/* TODO: Implement new function SSL_RevealCertList
> > + *       that returns all certificates sent by the server.
> > + *       It will be required to implement tls-ext-multiple-ocsp.
> > + */
> 
> Remove this comment. There is already an open bug in bugzilla for this
> feature.


I don't understand the benefit of removing a comment.
I think it's good to keep it, because I added function name SSL_RevealCertList
at two places in the code. I believe that's a very helpful recording
of knowledge for the later work of getting it implemented.

I shortened the comment and added the bug number 611836.


> > +/* Given PRFileDesc, returns the OCSP information that was sent by the server
> > + * during the handshake.
> > + * We use a list of OCSP responses, to prepare for tls-ext-multiple-ocsp.
> > + * Caller must free data when done.
> 
> Move the application-level documentation to ssl.h.

All functions in sslreveal.c have their documentation next to the implementation.


> Document that the function may only be called from the auth certificate hook
> (or bad cert handler?).

We don't appear to have this limitation.

The function uses locking, it duplicates the data.
It seems it's fine to call this function after the handshake,
until the socket gets destroyed.


(In reply to Brian Smith (:bsmith) from comment #61)
> 
> In my previous comments, I mentioned that it is important that there not be
> any calls to ocsp_*/OCSP_* in the core of libssl. I see that you addeded the
> processCertStatus in response to that. I was unclear in what I meant: it is
> OK to call ocsp_*/OCSP_* in SSL_AuthCertificate, but not elsewhere in
> libssl, because applications for which those calls would be problematic
> (Chrome, Firefox) also need to avoid calling SSL_AuthCertificate for similar
> reasons. So, the complexity added by the processCertStatus callback part is
> unnecessary.


So you've changed your opinion that calling OCSP code from within
SSL_AuthCertificate shall be fine, but what about the
#include dependency?

It was my understanding that such a dependency is considered
undesirable by the chrome guys.

(Sigh. I had already removed this code, until I discovered 
the #include dependency. I added it back.)

The code I've written avoids that dependency

	Kai Engert (:KaiE:) Assignee
	Comment 64 • 13 years ago

Can we handle comment 46 and comment 47 in a follow up bug?
Maybe Brian can do this work, after the initial step is done?
I don't know if this work is still necessary.

	Kai Engert (:KaiE:) Assignee
	Comment 66 • 13 years ago

Wan-Teh, thank you very much for attaching the latest patch used by chrome.

I compared my latest patch (something after v14) and the chrome patch.

I copied/merged the various enhancements that you made, they are contained in patch v15.

	Kai Engert (:KaiE:) Assignee
	Comment 67 • 13 years ago

I would like to make it easy for everyone to compare chrome's latest patch and my latest patch.

Therefore I manually created a version of v15, where I rearranged the pieces, so that they are in the same order as in attachment 616669 [details] [diff] [review].

You could download both attachment 616669 [details] [diff] [review] and this attachment and compare them, for example using "tkdiff".

	Kai Engert (:KaiE:) Assignee
	Comment 68 • 13 years ago

(In reply to Brian Smith (:bsmith) from comment #61)
> 
> Also, in your patch, I didn't see a default implementation of the
> processCertStatus hook. Did I overlook it?


True, all I have so far is this:

+ * An application that uses the default SSL_AuthCertificateHook
+ * and wishes to enable NSS' default processing of received OCSP stapling data
+ * should call
+ *      SSL_ProcessCertStatusHook(fd, CERT_CacheOCSPResponseFromSideChannel)
+ * after each socket construction.

	Kai Engert (:KaiE:) Assignee
	Comment 69 • 13 years ago

(In reply to Kai Engert (:kaie) from comment #68)
> 
> True, all I have so far is this:
> 
> + * An application that uses the default SSL_AuthCertificateHook
> + * and wishes to enable NSS' default processing of received OCSP stapling
> data
> + * should call
> + *      SSL_ProcessCertStatusHook(fd, CERT_CacheOCSPResponseFromSideChannel)
> + * after each socket construction.


Given that a requirement was
  "no include dependency from libSSL to the rest of NSS"
it's difficult to provide a full default implementation,
since such an implementation would have to live inside libSSL and call
CERT_CacheOCSPResponseFromSideChannel.

	Kai Engert (:KaiE:) Assignee
	Comment 70 • 13 years ago

Comment on attachment 618505 [details] [diff] [review]
Patch v15

We decided that introducing a direct call from libSSL into the OCSP cache is acceptable, and preferable, because it avoids complexity.

I'll attach such a patch soon.

	Kai Engert (:KaiE:) Assignee
	Comment 72 • 13 years ago

Comment on attachment 618749 [details] [diff] [review]
Patch v16

In case you wonder why I added this fragment:

>     case client_key_exchange:
> 	if (!ss->sec.isServer) {
> 	    (void)SSL3_SendAlert(ss, alert_fatal, unexpected_message);
> 	    PORT_SetError(SSL_ERROR_RX_UNEXPECTED_CLIENT_KEY_EXCH);
> 	    return SECFailure;
> 	}
>+        rv = ssl3_AuthCertificateIfNeeded(ss, b, length);
>+        if (rv != SECSuccess)
>+            break;
> 	rv = ssl3_HandleClientKeyExchange(ss, b, length);
> 	break;


Without it, the test suite fails if the server requests client authentication.

	Kai Engert (:KaiE:) Assignee
	Comment 73 • 13 years ago

I ran into a crash with the patch from bug 700701,
when the handshake-callback called SSL_RevealCertStatus.

We hit an assertion related to firstHandshakeLock
caused by the locking in SSL_RevealCertStatus
(I had copied the locking from the chrome patch).

I haven't studied how the locks work in libSSL code,
so I'd welcome advice.

This function should be safe to call any time after the initial handshake has completed,
and inside the lock we just need to copy a buffer.

If the buffer can never change after the first handshake,
we could remove the lock, check that (ss->firstHsDone) is true,
and simply copy the buffer.

Both SSL_RevealCert and SSL_RevealURL use this strategy (no lock).

Function SSL_HandshakeNegotiatedExtension reads (ss->firstHsDone) without locking.

	Kai Engert (:KaiE:) Assignee
	Comment 74 • 13 years ago

#0  0xb7fff424 in __kernel_vsyscall ()
#1  0x4326798f in raise () from /lib/libc.so.6
#2  0x432692d5 in abort () from /lib/libc.so.6
#3  0xb7d930ec in PR_Assert (s=0xb7ff0e58 "PZ_InMonitor((ss)->firstHandshakeLock) || !ssl_HaveRecvBufLock(ss)", file=0xb7ff0e4c "sslreveal.c", ln=84) at ../../../../pr/src/io/prlog.c:554
#4  0xb7fd500e in SSL_RevealCertStatus (fd=0x807b2d8) at sslreveal.c:84
#5  0x0804bb0b in printSecurityInfo (fd=0x807b2d8) at tstclnt.c:156
#6  0x0804bb91 in handshakeCallback (fd=0x807b2d8, client_data=0x0) at tstclnt.c:171
#7  0xb7fc3583 in ssl3_FinishHandshake (ss=0x807bbf8) at ssl3con.c:9061
#8  0xb7fc3420 in ssl3_HandleFinished (ss=0x807bbf8, 
    b=0x808367c "x\267\224]\256ph\250\305c\234$\246@\360\257\246\362\227\035\tE\360kc\220\203\325x\355\211ԇ< WeE\367\021\363\347\365\070L\256ˆ\325\375f\244+\"MJ\343\356K\016\251_VA\b\270", <incomplete sequence \321>, length=12, 
    hashes=0xbfffd92c) at ssl3con.c:9037
#9  0xb7fc3e3a in ssl3_HandleHandshakeMessage (ss=0x807bbf8, 
    b=0x808367c "x\267\224]\256ph\250\305c\234$\246@\360\257\246\362\227\035\tE\360kc\220\203\325x\355\211ԇ< WeE\367\021\363\347\365\070L\256ˆ\325\375f\244+\"MJ\343\356K\016\251_VA\b\270", <incomplete sequence \321>, length=12)
    at ssl3con.c:9269
#10 0xb7fc4098 in ssl3_HandleHandshake (ss=0x807bbf8, origBuf=0x807be60) at ssl3con.c:9343
#11 0xb7fc4f8d in ssl3_HandleRecord (ss=0x807bbf8, cText=0xbfffdae0, databuf=0x807be60) at ssl3con.c:9773
#12 0xb7fc68bd in ssl3_GatherCompleteHandshake (ss=0x807bbf8, flags=0) at ssl3gthr.c:361
#13 0xb7fc6a54 in ssl3_GatherAppDataRecord (ss=0x807bbf8, flags=0) at ssl3gthr.c:404
#14 0xb7fd6676 in DoRecv (ss=0x807bbf8, out=0xbfffdc5c "GET / HTTP/1.0\r\n\r\n", len=4000, flags=0) at sslsecur.c:535
#15 0xb7fd7a07 in ssl_SecureRecv (ss=0x807bbf8, buf=0xbfffdc5c "GET / HTTP/1.0\r\n\r\n", len=4000, flags=0) at sslsecur.c:1143
#16 0xb7fe0a84 in ssl_Recv (fd=0x807b2d8, buf=0xbfffdc5c, len=4000, flags=0, timeout=4294967295) at sslsock.c:2003
#17 0xb7d8f31f in PR_Recv (fd=0x807b2d8, buf=0xbfffdc5c, amount=4000, flags=0, timeout=4294967295) at ../../../../pr/src/io/priometh.c:188
#18 0x0804eb83 in main (argc=14, argv=0xbfffeee4) at tstclnt.c:1442

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 75 • 13 years ago

(In reply to Kai Engert (:kaie) from comment #73)
> I haven't studied how the locks work in libSSL code,
> so I'd welcome advice.

Some functions in libssl are only (thread-)safe to call when they are called from callback functions. See bug 707394 where I started to document this.

In the case of SSL_RevealCertStatus, we should document that it must only be called within the cert auth hook, and remove the locking.

	Kai Engert (:KaiE:) Assignee
	Comment 76 • 13 years ago

> In the case of SSL_RevealCertStatus, we should document that it must only be
> called within the cert auth hook

Maybe we can allow the handshake-callback, too?

	Kai Engert (:KaiE:) Assignee
	Comment 77 • 13 years ago

Patch v20 combines (this) bug 360420
and bug 554369 and bug 700701 and bug 749714.

It adds basic OCSP sapling testing against known servers,
the test suite passes.

This patch is for backup and demonstration purposes, if you want to try it yourself.
We should probably continue to review the patches individuall, 
and I'll attach (later) an updated subset of the patch.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 78 • 13 years ago

(In reply to Kai Engert (:kaie) from comment #76)
> > In the case of SSL_RevealCertStatus, we should document that it must only be
> > called within the cert auth hook
> 
> Maybe we can allow the handshake-callback, too?

Let's consider bug 731478. If we need to cache the intermediate CA certificates in the session cache, shouldn't we also cache the OCSP data in the session cache too? It seems we should do so, if the purpose of bug 731478 is to allow an application to have access to all the data needed to re-validate the certificate in a resumption handshake. In that case, it does make sense to do the memory allocation and save the OCSP data in ss->sec.ci.sid->cert_status.

I guess that because Wan-Teh filed bug 731478, Chromium must need to have the information needed to do cert re-verification in its handshake callback, since that is the only callback that gets called during resumptions.

So, it seems like, for Chromium's sake, at least, we should document support for accessing the OCSP data in the handshake callback in addition to the auth certificate hook.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 79 • 13 years ago

When reviewing the patches, I noticed that *both* patches are more complicated than necessary. I believe that Adam's patch is too optimized for minimizing the diff to mainline CVS at the cost of readability/understandability. Kai's patch is too complicated regarding the refactoring of ssl3_HandleCertificate.

Also, I am concerned about the additional complexity in Kai's patch to support multi-stapling. Without actually implementing the entire multi-stapling spec, it is hard to tell whether that extra complexity is worthwhile. I talked with EKR and AFAICT, we both agree that multi-stapling is a good idea but that it isn't a high priority because most browsers have alternate mechanisms for dealing with intermediate certificate revocations for the vast majority of websites. Also, I suspect that there will still be a lot of changes to the multi-stapling spec itself as it is still going through standardization. IMO, it is better to experiment with implementing it in its own bug, bug 611836. I think that most of Kai's patch will be useful for fixing that bug.

That means it makes sense to use a patch more like Adam's. However, I think that Kai's restructuring of ssl3_HandleCertificate is much better than Adam's, except for the replacement of gotos with return XXX_error() calls.

Since Kai asked me to make any significant changes to his patch myself, I have done a three-way merge of the two patches and done further simplifications; I believe the merged, simplified patch will be more smaller, more obviously correct, and easier to maintain than either of the current ones. I will finish it up tomorrow, after I verify that it is simple to modify it to handle the changes that will be necessary as part of bug 731478.

FWIW, I spoke with EKR about this and he is in favor of taking Google's patch as-is, or with minor changes. I am also in favor of doing that, if we do not agree that my simplified version of Kai's changes to ssl3_AuthCertificate are not worthwhile.

Wan-Teh objected to the signature of SSL_GetStapledOCSPResponse in Adam's patch. In that patch, the caller of SSL_GetStapledOCSPResponse is responsible for providing a buffer for libssl to copy the OCSP response data into, and the function uses a relatively uncommon (for libssl) pattern for doing that copying. AFAICT, it is not really necessary to do any copying at all, if we allow SSL_GetStapledOCSPResponse to return a pointer to the internal SECItem that holds the response data, and document that the application must not access that SECItem outside of the callback function that it is calling SSL_GetStapledOCSPResponse in. This would be more efficient. The signature would be:

   const SECItem * SSL_GetStapledOCSPResponse(PRFileDesc * fd);

Does this seem reasonable to people?

Kai's patch fixes some bugs in CERT_CacheOCSPResponseFromSideChannel and provides a default OCSP stapling implementation in SSL_AuthCertificate. I suggest that we handle the fixes to CERT_CacheOCSPResponseFromSideChannel in separate bugs. I also suggest that we factor out his implementation of SSL_AuthCertificate into a separate bug, because I think there are some unresolved questions about the how OCSP stapling is really supposed to work, that require more clarification (documentation and testing) of the implemented semantics.

One high-level issue that came up: the CertificateStatus extension is not specific to OCSP. Do we want to hard-code OCSP into the API, or do we want the API to be more flexible and allow other status types? In the interest of expediency, and keeping with the YAGNI principle, it is better to just keep the hard-coding of a single OCSP response in the API, like both patches do. But, we should note that is a conscious decision that may result in a new more flexible CertificateStatus API later, that would make this OCSP-specific API somewhat redundant.

	Eric Rescorla (:ekr)
	Comment 80 • 13 years ago

" Also, I suspect that there will still be a lot of changes to the multi-stapling spec itself as it is still going through standardization. IMO, it is better to experiment with implementing it in its own bug, bug 611836. I think that most of Kai's patch will be useful for fixing that bug."

To elaborate on this topic, we *just* accepted it as a WG item, so I expect it will undergo the usual torturing by the WG and we won't have it in final form (which won't be the same as now) for at least a year. I think having regular stapling now would be a good thing.

	Kai Engert (:KaiE:) Assignee
	Comment 81 • 13 years ago

these obvious syntax fixes are necessary to build on windows

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 82 • 13 years ago

NOTE: I think there is a bug in AGL's original patch, and in this modified patch too: In the case of a renegotiation, where server stapled an OCSP response in the previous handshake, but did not staple an OCSP response in the current handshake, I believe that the OCSP response from the previous handshake still gets reported from SSL_GetStapledOCSPResponse/SSL_PeerStapledOCSPResponse. This would be confusing especially in the case where the server sent a different certificate chain in the current handshake than it sent in the previous handshake.

In the next comment, I will use the Splinter interface to explain the changes I made to AGL's patch. I recommend using the splinter tool to read the comments.

In the next patch, I will merge a simplified version of Kai's changes to the state machine, which avoids the problem of the OCSP response from the previous handshake on a connection being reported for subsequent handshakes that do not staple a response.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 83 • 13 years ago

It is easier to review these changes using this interdiff, which is slightly different than the version that I just posted.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 84 • 13 years ago

Comment on attachment 622680 [details] [diff] [review]
Part 1 - Cleanup of AGL's patch -- interdiff from AGL's patch in attachment 616669 [details] [diff] [review]

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

Like I mentioned above, it will probably be easier to review this interdiff with the comments I added via Splinter.

Also, like I mentioned above, I think there is a bug regarding renegotiations, but it is different than what I said above:

ssl3_HandleCertificateStatus contains this check:

    if (!ss->ssl3.hs.may_get_cert_status ||
	ss->ssl3.hs.ws != wait_server_cert ||
	!ss->ssl3.hs.pending_cert_msg.data ||
	ss->ssl3.hs.cert_status.data) {
	errCode = SSL_ERROR_RX_UNEXPECTED_CERT_STATUS;
	desc = unexpected_message;
	goto alert_loser;
    }

ss->ssl3.hs.cert_status.data will be non-NULL in a renegotiation that has a CertificateStatus that occurs after a previous handshake contained a CertificateStatus, because, as far as I can tell, ss->ssl3.hs.cert_status.data is not cleared between handshakes. If we were to remove this check, then we would run into the problem where a CertificateStatus from the previous handshake could get carried over to the next handshake, and we would have a memory leak. So, AFAICT, the simplest correct solution is to free cert_status between handshakes on a connection. (In the next patch, I do something different than that.)

::: ../ocsp-agl/mozilla/security/nss/lib/ssl/ssl.def
@@ +145,5 @@
>  ;+    global:
>  DTLS_GetTimeout;
>  DTLS_ImportFD;
>  SSL_ExportKeyingMaterial;
> +SSL_PeerStapledOCSPResponse;

I renamed the function to make it clearer that the OCSP response returned is the peer's OCSP response. If we implement OCSP stapling on the server side, then there may be a SSL_LocalStapledOCSPResponse. This naming follows the convention set by SSL_PeerCertificate/SSL_LocalCertificate.

::: ../ocsp-agl/mozilla/security/nss/lib/ssl/ssl.h
@@ +416,2 @@
>   */
> +SSL_IMPORT const SECItem * SSL_PeerStapledOCSPResponse(PRFileDesc *fd);

Extended documentation and simplified calling convention. Most callers are probably going to pass the response to CERT_CacheOCSPResponseFromSideChannel, so there's no need to do any allocations or copying.

Returning const SECItem* helps ensure that the caller doesn't try to free the SECItem.

::: ../ocsp-agl/mozilla/security/nss/lib/ssl/ssl3con.c
@@ +8194,5 @@
>  	goto format_loser;
>      }
>  
>      if (SECITEM_AllocItem(NULL, &ss->ssl3.hs.cert_status, length) == NULL) {
> +	return SECFailure;

replaced spaces with tabs.

@@ +8202,5 @@
>  
>      return SECSuccess;
>  
>  format_loser:
> +    return ssl3_DecodeError(ss);

The bad_certificate_status_response alert is supposed to be sent when there is a problem with parsing/verifying the OCSP response, not when there is a problem parsing the CertificateStatus message.

@@ +9181,5 @@
>  		(void)ssl_MapLowLevelError(SSL_ERROR_RX_UNEXPECTED_CERTIFICATE);
>  		return SECFailure;
>  	    }
>  	    if (SECITEM_AllocItem(NULL, &ss->ssl3.hs.pending_cert_msg,
> +				  length) == NULL) {

replaced spaces with tabs.

::: ../ocsp-agl/mozilla/security/nss/lib/ssl/ssl3ext.c
@@ -631,5 @@
>  {
> -    /* If we didn't request this extension, then the server may not echo it. */
> -    if (!ss->opt.enableOCSPStapling)
> -	return SECFailure;
> -

ssl3_HandleHelloExtensions already does this check via ssl3_ClientExtensionAdvertised:

http://mxr.mozilla.org/mozilla/source/security/nss/lib/ssl/ssl3ext.c#1458

::: ../ocsp-agl/mozilla/security/nss/lib/ssl/sslimpl.h
@@ +799,1 @@
>      SECItem               cert_status; /* an OCSP response */

Whitespace changes.

::: ../ocsp-agl/mozilla/security/nss/lib/ssl/sslsock.c
@@ -1796,4 @@
>      }
>  
> -    ssl_Get1stHandshakeLock(ss);
> -    ssl_GetSSL3HandshakeLock(ss);

We cannot acquire these locks in this function because this function will usually be called in the authenticate certificate callback, and we do that with these locks already held.

@@ -1805,5 @@
> -	*len = ss->ssl3.hs.cert_status.len;
> -	PORT_Memcpy(out_data, ss->ssl3.hs.cert_status.data, todo);
> -    } else {
> -	*len = 0;
> -    }

Simplified calling convention.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 85 • 13 years ago

I will explain the changes using Splinter in the next comment.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 86 • 13 years ago

Comment on attachment 622684 [details] [diff] [review]
Part 2 - Simplify state machine changes using logic from Kai's patch (simplified) and move OCSP response to session - Interdiff from attachment 622680 [details] [diff] [review]

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

::: ../ocsp-agl/mozilla/security/nss/lib/ssl/ssl3con.c
@@ +8174,2 @@
>  
> +    PORT_Assert(ss->ssl3.hs.ws == wait_certificate_status);

ssl3_HandleHandshakeMessage now ensures that ssl3_HandleCertificateStatus is only called when appropriate.

@@ +8189,4 @@
>  	return SECFailure;
>      }
> +    ss->sec.ci.sid->peerCertStatus.type = siBuffer;
> +    PORT_Memcpy(ss->sec.ci.sid->peerCertStatus.data, b, length);

Moved the OCSP response to the session object, so that it can be retrieved for resumed handshakes too, and because the session object automatically gets reset during a full handshake renegotation, so we don't have to worry about the state carrying over from one handshake on a connection to the next one.

@@ +8355,5 @@
> +    }
> +    
> +    return rv;
> +
> +ambiguous_err:

The actual change (hard to see from this diff) is that we've hoisted the code for ssl3_AuthCertificate out of this function. All the error handling logic here is unchanged from the original logic, except for one exception noted below.

I believe that ssl3_AuthCertificate as a standalone function separate from the parsing of the Certificate message will be useful in later patches. And, doing things this way lets us avoid needing the pending_cert_msg buffer.

@@ +8376,5 @@
> +
> +alert_loser:
> +    (void)SSL3_SendAlert(ss, alert_fatal, desc);
> +
> +loser:

See note below about what was removed from this label.

@@ -8493,5 @@
> -
> -    if (ss->sec.peerCert != NULL) {
> -	CERT_DestroyCertificate(ss->sec.peerCert);
> -	ss->sec.peerCert = NULL;
> -    }

There is no need to clean up these things here because we will do so when the socket is closed.

	Kai Engert (:KaiE:) Assignee
	Comment 87 • 13 years ago

Comment on attachment 619227 [details] [diff] [review]
Patch v20 [multiple bugs] [includes testing]

In order to allow for early testing of this proposed patch, I've made binaries available at http://flowerbeetle.org

	Ben Wilson
	Comment 88 • 12 years ago

I have voted in favor of this bug. I understand that proposed patch needs to be reviewed and approved, but would it help any if I were to get more people to vote?  I can email members of the CA/B Forum, etc.  If that would not be helpful, what other things could I do to move this forward? Thanks, Ben

	Kai Engert (:KaiE:) Assignee
	Comment 89 • 12 years ago

Voting is irrelevant if the developers block each other, because they cannot reach agreements.

	Gervase Markham [:gerv]
	Comment 90 • 12 years ago

I have emailed Wah-Teh to ask him to comment on the best way to make progress here.

Gerv

	Kai Engert (:KaiE:) Assignee
	Comment 91 • 12 years ago

We have an unclear mix of attachments.

I understand Brian's proposal involves the following set of patches attached on top of each other:
- Adam's attachment 616669 [details] [diff] [review]
- Brian's attachment 622680 [details] [diff] [review]
- Brian's attachment 622684 [details] [diff] [review]

The latter patches even remove stuff that was added by the first one, which makes it very difficult to review.

(I think contributors should do better when asking for reviews, and attach consolidated patches.)

I just spent one hour and, one after the other, manually merged the bitrotted patches into the latest NSS trunk. I'm attaching it here. (It builds, but I haven't tested it.) I'm marking the old patches as obsolete. 

Brian, feel free to re-request review on this consolidated patch from Wan-Teh.

I don't like this patch, because it's incomplete. It makes no attempt to inject the OCSP data received into the OCSP cache. I think it's inappropriate to require each NSS based application to handle that injection on their own. NSS should be a comfortable toolkit with default functionality, not require applications to do a lot of stuff by foot.

(The patch that I had proposed implements that injection.)

	Kai Engert (:KaiE:) Assignee
	Comment 92 • 12 years ago

The following decisions should be made by the NSS team in tomorrow's NSS conference call:

1:
(a) do what Brian suggests, and strictly focus on single-stapling for now, in order to use simpler patches
(b) do what Kai suggests, and use the multi-stapling preparation code that Kai has already implemented in the attached patches


2:
Do OCSP cache injection as part of this work from within libSSL.
(a) no, as omitted by Brian
(b) yes, as proposed by Kai in my latest patch


In addition, given the 5 months of review stalling that we've seen in this bug, I'd like to ask for a clarification:

3:
Given that Google Chrome has already implemented this feature using a locally patched version of NSS, are Google employees still interested in getting this feature to work in the shared NSS code (e.g. by reviewing patches) - or should we rather try to get this done without their help?

Thanks

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 93 • 12 years ago

(In reply to Kai Engert (:kaie) from comment #92)
> The following decisions should be made by the NSS team in tomorrow's NSS
> conference call:
> 
> 1:
> (a) do what Brian suggests, and strictly focus on single-stapling for now,
> in order to use simpler patches
> (b) do what Kai suggests, and use the multi-stapling preparation code that
> Kai has already implemented in the attached patches

FYI, I doubt that Firefox will ever offer the multi-stapling extension to servers because we are already planning different optimizations for revocation checking of intermediates (as discussed in the NSS workweek), and the currently-proposed multi-stapling extension would interfere with those optimizations and other ones. For the same reason, I doubt that Chromium would be better off offering the multi-stapling extension either. And, accordingly, I am not sure any servers will ever implement it, if browsers do not choose to send it.

> Do OCSP cache injection as part of this work from within libSSL.
> (a) no, as omitted by Brian
> (b) yes, as proposed by Kai in my latest patch

It is important that the application have direct access to the OCSP responses, and that the application can avoid using the NSS OCSP cache if it doesn't want to use it. In particular, it seems likely that Firefox may (eventually) not use the NSS OCSP cache at all, because it would need to implement its own (persistent) cache for other reasons.

As discussed above, the best solution is to make the SSL certificate auth hook responsible for dealing with the OCSP responses. Then, NSS's default certificate hook can put those responses in the OCSP cache if it wants, though (again, as discussed above) there are problems with doing so.

> In addition, given the 5 months of review stalling that we've seen in this
> bug, I'd like to ask for a clarification:

Rather, progress has been blocked on me updating the patches.

Note that I also raised some concern about whether all of these patches are doing the correct thing when an OCSP response is received during a renegotiation handshake. IIRC, there is such a problem with the patches that were posted before mine. In my patch, I attempted to addressed that issue, but I did so by storing the OCSP responses in the session state; IIRC, Wan-Teh had asked that we avoid storing the OCSP responses in the session cache.

	Gervase Markham [:gerv]
	Comment 94 • 12 years ago

Brian: if there are onlookers interested in helping this patch make progress, what can they do? Is there testing they can perform, perhaps using a copy of Flowerbeetle? Is it worth anyone else taking up the work of improving the patch, or is it close enough to done that this is unlikely to help?

Gerv

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 95 • 12 years ago

(In reply to Gervase Markham [:gerv] from comment #94)
> Brian: if there are onlookers interested in helping this patch make
> progress, what can they do? Is there testing they can perform, perhaps using
> a copy of Flowerbeetle? Is it worth anyone else taking up the work of
> improving the patch, or is it close enough to done that this is unlikely to
> help?

Previously, Kai asked me to implement my review comments on his patch myself, which I did. But, like Kai mentioned, there are some things that need to be agreed on that might require modifications of that patch.

For Gecko's use, we need to write the PSM-side code and we need to write the xpcshell tests that verify that Gecko is implementing OCSP stapling correctly. I'd speculate that fixing bug 466524 and bug 663733 would be very helpful for writing said tests. Having those tests already running and passing before we do the final reviews would also make the reviews easier. For example, having a test that we do the right thing during renegotation handshakes would be especially useful. Depending on who does what, it may also be better to get the changes in bug 754365 landed before attempting the PSM side of this.

	Kai Engert (:KaiE:) Assignee
	Comment 96 • 12 years ago

I'm not interested in work that requires PSM side pieces. The PSM pieces should be discussed and implemented elsewhere.

This is an NSS level bug. I am interested to see a solution that is complete inside NSS, thereby allowing any NSS based application to make use of the OCSP stapling feature.

	Kai Engert (:KaiE:) Assignee
	Comment 97 • 12 years ago

This NSS level bug shouldn't be blocked by Gecko/PSM requirements, be it PSM code or be it Gecko level testing. The Mozilla/PSM/Gecko teams can start writing their tests after we have finished an initial implementation in NSS.

	Ben Wilson
	Comment 98 • 12 years ago

Thanks.  Could I ask that Gerv be prepared to provide a status update to the CA/Browser Forum during a telephone call on Thursday?  Thanks again.

	Kai Engert (:KaiE:) Assignee
	Comment 99 • 12 years ago

For what it's worth...

I've also merged my own patches (which include the work from others) to latest trunk. (Includes a fix for an incorrect pointer that I discovered today.)

This matches what I described earlier as patch v20 (based on v16).

The test script passes, including the new tests against some known OCSP stapling servers. (Note the expired+revoked cert donated by a commercial CA for testing, which runs on my test server, will expire on December 8, after which we will have reduced CA testing coverage, unless we get new commercial certificate contributions.)

	Ben Wilson
	Comment 100 • 12 years ago

FWIW - the CAB/Forum also assigned a new EKU OID for "mustStaple". It is 2.23.140.16.1.  (2.23.140 is the "ca-browser-forum" and 16 is "ocsp").  Last I read, the EKU would mean "only use this key when it's accompanied by some stapled certificate status information."

	Robert Relyea
	Comment 101 • 12 years ago

OK, here's my take:

My assumptions.

1) IIRC only single stapling has been completely defined in the RFC.
2) There is a proposal for multi-stapling
3) Mozilla's current trajectory requires only single stapling.

My conclusions:

1) OCPS stapling patch should not be held up waiting for multi-stapling.
2) In the long run NSS should support both single stapling and multi-stapling, even if Mozilla only cares about the former (for now).

If we have patches that are complete for single stapling and does not interfere with the multi-stapling spec, we should get those patches in. If we have a choice between a set of complete single stapling patches and a complete set of multi-stapling patches, we should check in the former.

In all cases complete means both server and client side need to be implemented... at least to the point that we can run automated testing on the feature.

bob

	Kai Engert (:KaiE:) Assignee
	Comment 102 • 12 years ago

(In reply to Robert Relyea from comment #101)
> 1) OCPS stapling patch should not be held up waiting for multi-stapling.

Nobody proposed to wait for multi stapling.

My patches use a "list" instead of "single fixed item" in the APIs, thereby offering a simple way to be "prepared" for a potential later multi stapling implementation. That's all there is to say.

But Brian doesn't like this preparation and suggests to undo that work.

In my opinion, this preparation isn't harmful. But if that were the only area of dispute, I'd be fine to revert code to an always-single-item API.


> 2) In the long run NSS should support both single stapling and
> multi-stapling, even if Mozilla only cares about the former (for now).
> 
> If we have patches that are complete for single stapling and does not
> interfere with the multi-stapling spec,
> we should get those patches in.


I believe that's what the attached patch v24 does.

The client side code is complete and does cache injection.

It full supports single stapling.

It doesn't attept to implement multi stapling in any way, except by "being prepared" for potential future larger sets of OCSP data.



> If
> we have a choice between a set of complete single stapling patches and a
> complete set of multi-stapling patches, we should check in the former.


We currently have a choice between 

(a) incomplete single stapling as implemented by Adam/Brian, because that code is missing cache injection and requires the application to do additional work

and

(b) complete single stapling as put together by me (using earlier contributions), including basic automated testing of the client side, against known stapling servers.


> In all cases complete means both server and client side need to be
> implemented... at least to the point that we can run automated testing on
> the feature.


I'm not aware of any server side code in NSS for OCSP stapling, because we currently don't have any NSS code to produce OCSP responses.

We have client testing against known external servers.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 103 • 12 years ago

Comment on attachment 674688 [details] [diff] [review]
Patch v24

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

::: mozilla/security/nss/lib/ssl/ssl3con.c
@@ +8386,5 @@
> +static SECStatus ambiguous_err(sslSocket *ss, PRBool isTLS, SSL3AlertDescription desc);
> +static SECStatus decode_loser(sslSocket *ss, PRBool isTLS, SSL3AlertDescription desc, int errCode);
> +static SECStatus alert_loser(sslSocket *ss, SSL3AlertDescription desc, int errCode);
> +static SECStatus loser(sslSocket *ss, SSL3AlertDescription desc, int errCode);
> +static SECStatus check_loser(SECStatus rv, sslSocket *ss, SSL3AlertDescription desc);

It seems like this is very similar to the patch I r-d a long time ago. Please see the previous review comments about fixing things like this unnecessary splitting up of ssl3_HandleCertificate into many functions. It isn't necessary and it makes the code harder to understand, especially because there are similarly-named functions that work slightly differently in this file, and also because these are not general-purpose functions. The error handling logic for the change to support OCSP stapling can be made very simple when this is undone.

it seems most or all of my previous review comments still seem to apply to this patch. Please address them before you post a patch for review. Thanks.

	Kai Engert (:KaiE:) Assignee
	Comment 104 • 12 years ago

Brian, I know that you don't like my patch, and I didn't ask you for review. I simply attached the patch to demonstrate that we have working code, and maybe other people will like it.

	Kai Engert (:KaiE:) Assignee
	Comment 105 • 12 years ago

Besides, it's not strictly my patch. It combines a lot of code from the other bugs, too. It's an attempt go get something done.

Brian, when I suggested that you might rewrite the patch, I had hoped that you would produce a result that is similarly complete as the patch that I attached. I think you didn't do that. Therefore I think patch v24 is better than yours.

Feel free to use as much from patch v24 as you like, combine it with your own preferred code, and produce something that is similarly complete and that you like better.

But without a similarly complete alternative patch, I propose to take what we have, get it reviewed and checked in, and go from there. Let's not wait another couple of months for a new patch, simply because Brian is unhappy about stylistic details.

And instead of this continuing and stuck debate between Brian and me, it's time to hear opinions and get review comments from third parties. Regards.

	Ryan Sleevi
	Comment 106 • 12 years ago

Kai, it's not clear from your comment whether or not you want a detailed review, but I interpreted to be that you are not looking for a review at this time.

If you're asking for a preference between these two patches, I support Brian's change. I believe that we should keep the changes as minimal and as well-defined as possible, and build upwards, as I view Brian's patch as doing, rather than implementing a much larger area, but only half-implemented in some areas, as I see patch v24 doing.

This is nothing personal, I just believe that a smaller patch is easier to reason about, easier to spot bugs in, and easier to test. The fact that Brian's patch is 1/4 the size of v24 I believe highlights the amount of code changes that exist in v24. I agree with Brian that I do not think it's appropriate to tie clean up to this patch, and I think that's one thing holding it back.

	Kai Engert (:KaiE:) Assignee
	Comment 107 • 12 years ago

Guys, I really feel misunderstood.

Rejecting my patch based on the argument of size is funny.

My patch is larger because it contains additional functionality and testing.

How is Brian's patch actually enabling NSS applications to make use of the OCSP stapling data? It doesn't!

My patch implements OCSP cache injection in the default authentication code (and that requires some code for duplicating the cert-id data structure).

Also, all new code is supposed to contain testing, right? Brian's patch doesn't have any real testing!

Brian's patch only has a dummy test in tstclnt, that simply tests whether ANY data has been received in the stapling message. I don't call that a functional test.

A big set of my patch is an enhancement to tstclnt that actually tests whether the OCSP cache injection has worked, and whether the data was a good status or was a revoked status or invalid data. And my patch also extends the OCSP test scripts to invokes this testing.

Furthermore, my patch adds an implementation to ssltap that can dump the contents of the stapled data. That's trivial code fully contained in the ssltap, and shouldn't be seen as a disadvantage for reviewing the rest.

The real dispute here is that Brian doesn't like the portions in the core SSL protocol implementation, that I had put together based on the earlier code contributions.

But I'm making a step towards you. I appreciate that Brian has written the code in the way he prefers (even though I had to merge three patches before I was able to understand what he did).

I hereby give up on my SUBSET of my patch for the actual SSL protocol enhancement. I removed these parts from my patch, and I replaced it with Brian's patch.

Another step towards you, in order to simplify the patch, I give up argueing about the "multi stapling preparation", and I have removed the code that duplicates the secitem-array, and I'm accepting Brian's interface that takes only a single secitem with a single response. (Which IMHO is a bit shortsighted, because using a list and the array duplication code is trivial, but so be it, if it helps move this forward.)

I'm attaching a new patch v26 that is said combination, uses Brian's SSL protocol code, plus the code to enable testing.

	Kai Engert (:KaiE:) Assignee
	Comment 109 • 12 years ago

Output created by new tests:

ocsp.sh: startssl valid, supports OCSP stapling
ocsp.sh: #1596: startssl valid, supports OCSP stapling  - PASSED
ocsp.sh: startssl revoked, supports OCSP stapling
ocsp.sh: #1597: startssl revoked, supports OCSP stapling  - PASSED
ocsp.sh: comodo trial test expired revoked, supports OCSP stapling
ocsp.sh: #1598: comodo trial test expired revoked, supports OCSP stapling  - PASSED
ocsp.sh: thawte valid, supports OCSP stapling
ocsp.sh: #1599: thawte valid, supports OCSP stapling  - PASSED
ocsp.sh: thawte revoked, supports OCSP stapling
ocsp.sh: #1600: thawte revoked, supports OCSP stapling  - PASSED
ocsp.sh: live valid, supports OCSP stapling
ocsp.sh: #1601: live valid, supports OCSP stapling  - PASSED
ocsp.sh: startssl valid, don't support OCSP stapling
ocsp.sh: #1602: startssl valid, don't support OCSP stapling  - PASSED
ocsp.sh: cacert untrusted, don't support OCSP stapling
ocsp.sh: #1603: cacert untrusted, don't support OCSP stapling  - PASSED

	Kai Engert (:KaiE:) Assignee
	Comment 110 • 12 years ago

Just documenting what I see as I go along, Brian's patch is missing the check-for-max-sane-cert-status-length that you have suggested yourself in comment 45 (and was contained in my patch v24).

So, Brian, since you have asked that we drop my parts of the patch that implements the core protocol, you might want to double check if you have missed anything else that I had already fixed...

(I'll soon give an update on what has been discussed today, so please wait a little. This comment doesn't mean a change of plan.)