436414 - Change OCSP client code to try HTTP GET first, fall back to POST; Enhance httpserv to be an OCSP server; Test OCSP retrieval locally.

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

OCSP allows requests to be encoded as HTTP POST requests or HTTP GET requests.
NSS presently always uses POST requests.
Windows always uses GET.  Opera is converting from POST to GET.
GET has the advantage that it can be cached by an ordinary caching http server.
It is also possible for an OCSP responder to redirect a GET request, but not
a POST request.  

NSS should support using GET as well as POST, and when used by PSM, it should
use GET by default (though perhaps PSM could control this through an API).

Comment 1

[Kai Engert [:KaiE:]]

In order to achieve maximum compatibility, the following decision has been made:

If NSS is to switch its default to GET, it should fail gracefully on servers that support POST, only.

If the attempt to obtain a server response using GET fails, we should automatically fall back to using POST.

Comment 2

[Kai Engert [:KaiE:]]

If we want to land this new "feature" for Firefox 3.1, we must have the initial support working and delivered into Firefox within the next 6 weeks (feature complete date).

I expect we'll declare 3.12 RTM next week.

I'm setting the tentative target milestone of this bug to 3.12.1

Comment 3

[Kai Engert [:KaiE:]]

FYI, I'm running a little test OCSP environment at http://kuix.de/misc/test-ocsp
(If the OCSP test links are down, please ping me and I'll restart them)

It only supports OCSP POST requests.

Comment 4

[Kai Engert [:KaiE:]]

Assigning to Honza.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

I thought Julien was working on this.  Julien, are you working on this?

Comment 6

[Julien Pierre]

No, I am not. It was never on my radar.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Unsetting target milestone in unresolved bugs whose targets have passed.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Gerv suggested (and I agree) that this might be a great help in reducing OCSP latency using caching, and allow CAs to scale up much better using CDNs. However, not only would we need to fall back to POST when the server doesn't support GET, but we would also need to fall back to POST when we receive a response that is too old (as determined by the applications revocation status staleness policy), especially since (a) the CA's OCSP responder might have a different recommended staleness policy (as given in its responses' cache control headers), and (b) many proxy caches do not honor cache control headers.

Comment 9

[Rob Stradling]

Paul Tiemann suggested that "NSS should know which CAs need POST and which can take GETs so it doesn't have to experience a failed GET before sending a POST".  I suppose NSS could ship with a hard-coded list and/or keep a list of OCSP URLs that fail GETs at runtime.  Worth the development effort?

Comment 10

[Gervase Markham [:gerv]]

Brian: if an OCSP responder's cache control headers don't match the content, then that responder needs fixing - and if site access is denied because it's broken, the CA's clients are going to demand it be fixed pretty quick :-)

Do you have data on the extent of problem b)>

Rob: I think we'll need to look at Opera's experience for guidance. It's quite possible they have something like this.

Gerv

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to comment #10)
> Brian: if an OCSP responder's cache control headers don't match the content,
> then that responder needs fixing - and if site access is denied because it's
> broken, the CA's clients are going to demand it be fixed pretty quick :-)

The problem isn't with the CA's cache necessarily, but (also) proxy caches between the client and the server. We cannot assume that proxy caches are operating according to the spec, especially in the face of an attack on the client's network that OCSP defends against.

Comment 12

[Kai Engert [:KaiE:]]

Our list of CAs and GET/POST preference of their OCSP servers will:
- never be complete
- get out of date when CAs change their servers

An automatic fallback is necessary.

Comment 14

[sugi]

Only supporting OCSP-GET and then fail back to OCSP-POST is enough at first release?
Demand for supporting OCSP has grown every year.
Therefore, reducing the load of OCSP signing engine is an important task.

Comment 15

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

"The OCSP responder MUST support requests and responses over HTTP. When sending requests that are less than or equal to 255 bytes in total (after encoding) including the scheme and delimiters (http://), server name and base64-encoded OCSPRequest structure, clients MUST use the GET method (to enable OCSP response caching)."

- Standards Track RFC 5019, The Lightweight Online Certificate Status Protocol (OCSP) Profile for High-Volume Environments

Comment 16

[sugi]

Is someone preparing for implementing this scheme?

Comment 17

[Honza Bambas (:mayhemer)]

For now I'm releasing this.

Comment 18

[sugi]

Hello Honza,

Could you please let me investigate the releasing version?

Comment 19

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

sugi, if GobalSign wants to reduce the number of OCSP requests its servers receive, please have GobalSign sponsor the development and testing of OCSP stapling on open source products like Apache, nginx, etc., and help encourage server admins to enable that feature. We are planning to implement OCSP stapling soon, and OCSP stapling has advantages for users *and* CAs.

We will implement some kind of persistent revocation status caching, even for responses retrieved via POST; after that, the main advantage of GET over POST would be support for caching by proxy caches. This isn't enough of a justification for making requesting via GET a high priority.

Comment 20

[sugi]

Hello Brian,

Of course we'll help the test if required.
We have some enviroment for PKI field.

Comment 21

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Sugi, it would be great if you could test the OCSP stapling support for Apache and report the results back in bug 360420 and/or reply to Gerv's message on the mailing list: http://permalink.gmane.org/gmane.comp.mozilla.crypto/16169

Comment 22

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(Just to be clear, in order to test Apache's stapling support, you have to use IE9 or another client for now, until bug 360420 is resolved.)

Comment 23

[Kai Engert [:KaiE:]]

(In reply to comment #18)
> Hello Honza,
> 
> Could you please let me investigate the releasing version?

Sugi, I believe you misunderstood, Honza was not talking about creating a software release. Instead Honza removed himself as the assignee of this bug, so he released the bug, but not fixed the bug.

Comment 24

[sugi]

Hello Sirs,

I see.
Then, let's fix this bug.
Unfortunately, I have little experience with C and I've never posted to mozilla project yet.
I'm very happy if someone supports me.

I've glanced at the source code.
Then "ocsp_sendEncodedRequest" function in "ocsp.c" was hard coded with POST method.
Before the POST, GET method should be inserted if the size of the request is less than or equal to 255 bytes.
Maybe some complicated caching logic should be implemented, but the above is enough for the first release?

Regards,
Koichi Sugimoto.

Comment 25

[Gervase Markham [:gerv]]

sugi: I think the logic needs to be:

Try GET (for a request of any size)
If GET fails or response is out of date, try POST
If POST fails, follow the existing error path

It is the waiting and retrying which will probably make this patch more complicated. (I do not understand this code; someone like Nelson or Kai would need to help you with specific questions about it.)

Gerv

Comment 26

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Sugi, would it satisfy your goal if Gecko (Firefox) cached the results of the POST? Or, are you specifically trying to enable proxy caches?

I am very interested in getting Gecko (Firefox) to cache OCSP responses (regardless of method of retrieval) on disk. We don't need to change NSS to support GET to do that. If you would like to work on this, then I can set up a discussion with you, me, Kai, and our cache people.

Comment 27

[sugi]

Hello Brian,

My goal is proxy caches.
I think RFC 5019 should be supported.
Therefore, HTTP-GET based request (no nonce) is mandatory as most browsers (IE, Opera, Safari, Chrome ..) already support this scheme.
Proxy caching reduces the load of signing engine of OCSP server.
OCSP is vulnerable against DOS attack because request does not requre signing but response requres signing.
To avoid this kind of attack, IMHO, proxy caching is effective.


Regards,
Koichi Sugimoto.

Comment 28

[Ryan Hurst]

I wanted to add, without Mozilla supporting this CAs can not move to CDN based distribution of OCSP responses without relying on client behavior for falling back to CRL (I do not know if Mozilla does this).

Comment 29

[Kai Engert [:KaiE:]]

http://tools.ietf.org/html/rfc5019#section-5 is unclear regarding the "urlencode" detail.

The paragraph explaning the 255 bytes limit says "after encoding", it lists all the various pieces including base64 encoding.

However, it doesn't clarify whether the limit is supposed to be checked "before urlencoding" or "after urlencoding".

Let's say the OCSP URI is
    http://ocsp.some.ca/
which is 20 bytes

Let's say the base64 encoded request is 235 bytes in size, and contains exactly one "+" character, no "/" character.

Before urlencoding the total request will be 255 bytes.
After urlencoding the total request will be 257 bytes.

In this scenario, GET or POST?

Comment 30

[Gervase Markham [:gerv]]

"   The OCSP responder MUST support requests and responses over HTTP.
   When sending requests that are less than or equal to 255 bytes in
   total (after encoding) including the scheme and delimiters (http://),
   server name and base64-encoded OCSPRequest structure, clients MUST
   use the GET method (to enable OCSP response caching)."

I think that means 255 bytes before URL encoding.

However, requests less than 255 bytes MUST be GET; those over _SHOULD_ be POST. So you can comply with the strict terms of the RFC if you err on the side of GETting rather than POSTing.

If I were you, I would make the level at which we switch from GET to POST a configurable limit. After all, if every OCSP server supports GETs up to 512 bytes, why would we not up the limit, in order to get better caching behaviour?

Gerv

Comment 31

[Kai Engert [:KaiE:]]

Ok. Let me initially do:

  with base64 up to 255 => try GET, fall back to POST
  base64 up to 255, but with urlencode > 255 => try GET, fall back to POST
  with base64 > 255 => POST

Comment 32

[sugi]

As for fresh response described on https://wiki.mozilla.org/NSS_OCSP_Brainstorming, I think nonce should be used.
If an OCSP request includes nonce in it, then, conforming OCSP responder must include the same nonce value in the resopnse.
RFC 3161 specifies about nonce handling, I think it a good reference:
http://www.ietf.org/rfc/rfc3161.txt


Regards,
Koichi Sugimoto.

Comment 33

[Rob Stradling]

(In reply to sugi from comment #32)
> As for fresh response described on
> https://wiki.mozilla.org/NSS_OCSP_Brainstorming, I think nonce should be used.
> If an OCSP request includes nonce in it, then, conforming OCSP responder
> must include the same nonce value in the resopnse.

That's not true.

RFC 5019 says:
"...in the case where a responder only supports pre-produced responses and does not have the ability to respond to an OCSP request containing a nonce, it SHOULD return a response that does not include a nonce.
Clients SHOULD attempt to process a response even if the response does not include a nonce...
...Clients that opt to include a nonce in the request SHOULD NOT reject a corresponding OCSPResponse solely on the basis of the nonexistent expected nonce, but MUST fall back to validating the OCSPResponse based on time."

Comment 34

[Rob Stradling]

That wiki page identifies the following problem scenario:
"after 24 hours an NSS client receives the old response from a proxy cache and rejects the response as invalid, because the timestamp is not fresh"

The CABForum Baseline Requirements say "OCSP responses from this service MUST have a maximum expiration time of ten days".  But these are minimum, not maximum, requirements.

Mozilla could choose to update the Mozilla CA Certificate Policy to require OCSP Responses to have a maximum expiration time of 24 hours.

Lucky Green once wrote:
"learning in 80 ms that the certificate was good as of a week ago and to not hope for fresher information for another week seems of limited, if any, utility to us or our customers".

Comment 35

[sugi]

Hell Rob,

Was too omitted...

Now, the rfc2560bis (http://www.ietf.org/id/draft-ietf-pkix-rfc2560bis-04.txt) describes:

2.3.3.  Nonce Response Extension

   This document defines one standard extension that may appear in the
   responseExtensions field of OCSP response messages.  The nonce
   cryptographically binds a request and a response to prevent replay
   attacks.  The nonce extension in the response is identified using the
   same object identifier as is used in the request, id-pkix-ocsp-nonce.

   If the request included a nonce extension (Section 2.1.1.1) then the
   response MUST either omit the nonce extension or include a nonce
   extension that has the same value as the nonce extension in the
   request.

   An OCSP responder MAY include a nonce extension in a response even if
   no nonce extension was included in the corresponding request. 

   Clients that do not include a nonce in the request MUST ignore any
   nonce that may be present in the response.


Therefore, if a request includes a nonce, then,

1) response must include the same nonce value as the corresponding request, or,
2) response must omit the nonce.

I think 1) is better from interoperability point of view.


Regards,
Koichi Sugimoto.

Comment 36

[sugi]

(In reply to sugi from comment #35)

Hell -> Hello

Sorry mistaken...

Comment 37

[Kai Engert [:KaiE:]]

Attachment: Patch v1

Comment 38

[Kai Engert [:KaiE:]]

Attachment: Patch v1 ignoring whitespace changes

Who got time and interest to do a review?

Comment 39

[Kai Engert [:KaiE:]]

Regarding the lifetime.

Shouldn't we distinguish CDN and Proxy?

I would assume a CDN means, each CDN server gets a dump of the data that the CDN is about to serve. In my understanding this would require that the CA creates a dump of all expected OCSP request/response pairs. In this scenario, the CA has full control over cache expiration. The CAs knows at what time the entries on the CDN will become too old and should be replaced.

In the proxy scenario, the CA's OCSP server should have full control, too.

The decision made by NSS whether an OCSP response is fresh is based on the produced-at attribute inside the OCSP response.

An OCSP server that returns an OCSP response should set HTTP expiration/validity headers based according to the produced-at value inside the response.

If an OCSP server dynamically signs the response and can include a current produced-at timestamp, then the HTTP headers could indicate an expiration of 24 hours.

If an OCSP server uses a pool of pre-produced OCSP responses, that server should dynamically adjust the HTTP headers based on the freshness of the OCSP response. If the OCSP response is 21 hours old, then the HTTP headers could indicate an expiration of 3 hours.

I assume the proxy servers (sitting in front of OCSP servers) will respect the HTTP expiration headers from the original OCSP server, and we should be fine?

Comment 40

[Ryan Hurst]

Sugimoto-san -

It is my belief decision to include a nonce in a request should be based on local policy, let me explain.

Including the nonce makes requests unique which negatively effects a intermediaries ability to do caching / optimization.

Including it but not requiring it does not materially improve the security of the transaction.

By supporting OCSP stapling you are accepting the need for intermediary caches, therefor your default behavior should be in support of caching and not include a nonce.

In the case of Windows I believe we added a group policy setting to enable requesting unique responses for enterprise customers.

As for interoperability about a year and a half ago I did a non-scientific study of OCSP responder capabilities and found none that did not support both GETS and posts, they were also OK with no nonces in the request.



Kai-

I see no reason to differentiate CDNs from PROXYs.

The only difference between the two cases is in the case of a CDN how does the OCSP server get the pre-produced responses to the CDN and that is not a problem for Mozilla to consider.

For a OCSP client to support either it must support the appropriate retry logic with the no-cache header, for example RFC 5019 says:

   OCSP clients MUST NOT include a no-cache header in OCSP request
   messages, unless the client encounters an expired response which may
   be a result of an intermediate proxy caching stale data.  In this
   situation, clients SHOULD resend the request specifying that proxies
   should be bypassed by including an appropriate HTTP header in the
   request (i.e., Pragma: no-cache or Cache-Control: no-cache).

As far as proxy servers respecting expiration headers, unfortunately this was not always the case which is why we added the above text in; things may be better today but this retry behavior works either way.

Comment 41

[Paul Tiemann]

I'm very happy to see recent developments in this bug!  We'll review and provide feedback on the patch.  If we could get POSTs under 10% of the mix on our OCSP service, we'd be able to try a caching CDN, which would benefit all parties (well, we'd have to pay more for the bandwidth it uses, but we get other benefits)

General information from our OCSP service, taken this morning:

65% of requests are POSTs
35% are GETs

Of the POSTs, these are the user-agents.  We don't log user agent normally, so I had to make a temporary log file with "$request_method $http_user_agent" and wait an hour to collect a few million lines as a sample...

User agents:
92.05%	Firefox
5.72%	ocspd (Mac)
0.68%	Thunderbird
1.55%	Other: everything else

Comment 42

[Ryan Hurst]

So am I :)

We see a different mix, about 40% POST and 60% GETs.

I don't have access to production machines to enable logging of user agents so I don't know the mix but a quick glance at statcounter tells me our mix is roughly inline with what I would expect based on browser penetration / behavior.

I suspect DigiCert has some customers that support the Mozilla community directly as to why they have such a skew.

With that said as Paul mentioned without this patch it very well may be impractical to do CDN based distribution of OCSP responses to help with performance and get the industry to hard-stop revocation.

Keep up the good work guys,

Ryan

Comment 43

[Rick Roos]

Hey Kai,

I reviewed this patch and I did notice a possible buffer overrun  in ocsp_GetEncodedOCSPResponseFromRequest.  The OCSP URL from the certificate is copied into the urlBuf buffer without length validation.  A buffer overrun could also occur when the base64 encoded request is URL encoded and added to urlBuf.

Thanks,

Rick

Comment 44

[Justin Dolske [:Dolske]]

Is there a specific person who can/should review this? I'm going through unassigned reviews, and am unsure what to do with this one.

Comment 45

[Josh Aas]

(In reply to Rick Roos from comment #43)

> I reviewed this patch and I did notice a possible buffer overrun  in
> ocsp_GetEncodedOCSPResponseFromRequest.  The OCSP URL from the certificate
> is copied into the urlBuf buffer without length validation.  A buffer
> overrun could also occur when the base64 encoded request is URL encoded and
> added to urlBuf.

Kai - can you respond to this? Perhaps we need a new patch?

Comment 46

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Kai, please include a description of how this patch interacts with HTTP caching and provide automated tests for it. Then I can review it.

In particular, how do we ensure that if the HTTP cache returns a cached response that is too old, that we re-request the OCSP request using GET with "Cache-Control: no-cache" or POST.

It would also be helpful to know how the implemention differs from the way that IE and Chrome work. I have heard that Chrome already supports/supported GET for OCSP requests even though they use NSS. What did they do and why can't we just use that implementation?

Comment 47

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian Smith (:bsmith) from comment #46)
> Kai, please include a description of how this patch interacts with HTTP
> caching and provide automated tests for it. Then I can review it.

It is probably too difficult to write NSS-level tests for this.

It is better for Gecko's purposes to do the automated tests using Gecko's HTTP cache, so that we can ensure that changes to our HTTP cache don't regress this functionality--in particular, the handling of the case where we need to re-try the request using POST or with "Cache-control: no-cache" or similar.

Also, this behavior should be configurable and at least in mozilla-central we need to force it OFF until we have the necessary testing.

Comment 48

[Kai Engert [:KaiE:]]

Comment on attachment 599140 [details] [diff] [review]
Patch v1 ignoring whitespace changes

The patch is incomplete.
When using libpkix-based verification,
we run through pkix_pl_OcspResponse_Create,
which is still hardcoded to use POST.

Comment 49

[Kai Engert [:KaiE:]]

Attachment: Patch v3

This patch applies to NSS trunk, on top of patch v20 from bug 360420.

Comment 50

[Kai Engert [:KaiE:]]

Brian, this bug is about the initial implementation of OCSP GET.

I don't have answers regarding your questions about an application level HTTP cache, and in my opinion your questions are outside of the scope of this NSS bug. You should discuss and research them elsewhere.

My general opinion here is, that the operators of OCSP proxying/caching servers should do their homework and use appropriate HTTP headers to control the lifetime of OCSP status reponses in caches. Whatever happens at the HTTP level, we'll continue to make our decisions based on the freshness of the underlying OCSP data.

If you have further suggestions around the use of HTTP headers, I propose they should be tracked in a new, separate bug e.g. "Investigate OCSP GET and caching" - but I propose to get GET support implemented first, so we have something to use and experiment with.

Comment 51

[Ryan Hurst]

It is fair to say it is up to the OCSP responder to manage their headers appropriate, RFC 5019 documents the right mappings between the OCSP response fields and the HTTP headers.

Its also appropriate to say NSS rely on the fields in the OCSP response vs in the OCSP header.

However it needs to handle the case where a intermediary cache (say a HTTP proxy at the edge of an enterprise) is missconfigured and is keeping the OCSP responses longer than the headers specify.

In such cases at the NSS layer you will get a state OCSP response, if you do get a state response you should re-try with "Cache-control: no-cache" or similar.

I think its fine to simply assume that a retry is appropriate if you did a wire retrieval and got a stale object back without looking at the HTTP header since there is really no way to know by looking there if you are in one of these cases.

BTW what I describe is what Windows does.

BTW++ very excited to see this work done, keep up the good work.

Comment 52

[Kai Engert [:KaiE:]]

Comment on attachment 620500 [details] [diff] [review]
Patch v3

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

Comment 53

[Kai Engert [:KaiE:]]

Attachment: patch v5

merged. Patch applies on top of:
- NSS 3.14
- plus patch v29 from bug 360420

Comment 54

[Kai Engert [:KaiE:]]

Changing to depend on 360420. Not a technical dependency, just a work order dependency.

Comment 55

[Kai Engert [:KaiE:]]

Anyone volunteering to extend httpserv to be capable of acting as an OCSP responder, for example, by adding command line arguments for CA nicknames and CRL files (multiple pairs)?

Comment 56

[Kai Engert [:KaiE:]]

The missing piece is automated testing.

Automated testing is difficult, because we don't have an OCSP server yet as part of the NSS test tools.

Luckily, since bug 811317 got fixed, we now have the OCSP signature creation code that can allow us to implement an OCSP server.

I propose to use the existing httpserv executable, which is used by the NSS test suite to retrieve from a http server.

I propose to extend httpserv with a new option:
  -O enabled OCSP retrieval protocol [default: all]
     allowed values: [all | get | post]

I propose to hardcode the httpserv code, if a http requests asks for /ocsp then direct it to an internal function that provides an OCSP response, and keep the existing behaviour for all other urls.

In order for httpserv to sign an OCSP response, it must know which CA it shall use for signing. I propose to use the same command line parameter that has been added to the selfserv tool.
  -A <ca> Nickname of a CA used to sign a stapled cert status

One requirement for all new NSS testing is, we must not connect to the outside world, but rather restrict connections to the local system. Because the contents of a certificate define which OCSP server will be contacted, this makes it necessary that we dynamically create appropriate server certificates at test suite execution time. The test suite would use the known hostname of the local computer, and the port where our test httpserv will run, and add an appropriate OCSP AIA extension to the certificate.

The test suite scripts will have to be enhanced to run "httpserv" (http/ocsp server) in addition to the already being executed "selfserv" (ssl server tool), starting both in parallel, and terminating both when done after each test.

The test should ensure that we correctly receive both good and bad test results.

Instead of going the complicated way of generating CRLs, instead of extending httpserv to read CRLs, I propose a shortcut.

For testing purposes, I propose that we code the OCSP server behaviour to the serial numbers being used in certificates. For example, the httpserv test tool could return a "good" status whenever the (serial % 100) == 1, and return "revoked" whenever (serial % 100) == 2.

All of the above are preconditions for being able to test.

The actual test will have to be executed using the tstclnt client tool. The tstclnt tool would be executed against multiple httpserv configurations, to make sure that the NSS client code correctly functions with each kind of OCSP server.

tstclnt should be enhanced to report whether or not the OCSP communication had succeeded.

Comment 57

[Kai Engert [:KaiE:]]

I don't know how soon I will be able to work on this bug. Please feel free to work on it. Thanks.

Comment 58

[Honza Bambas (:mayhemer)]

Just for info: openssl can very easily run an OCSP server too, you may use it as a short therm workaround.

Comment 59

[Kai Engert [:KaiE:]]

I'm working on the enhancements I proposed in comment 56.

Comment 60

[Kai Engert [:KaiE:]]

Attachment: merged and with ocsp server

This patch is for backup purposes, not yet for review.
It implements an OCSP server.
Will work on cleanup soon.

Comment 61

[Kai Engert [:KaiE:]]

Attachment: updated patch backup

This patch adds testing. It replaces the outgoing connections to kuix.de, it dynamically generates certificates with OCSP AIA extensions that point to the local test computer, and it updates the tests to run a local ocsp server.
Attaching for backup purposes, not yet requesting review.

Comment 62

[Kai Engert [:KaiE:]]

Attachment: updated patch backup

Now with a libpkix implementation that does GET by default and falls back, too.
I should be ready to request reviews tomorrow.

Comment 63

[Kai Engert [:KaiE:]]

Attachment: v9 - httpserv (part 1 of 4)

This is a subset of the full patch for this bug.
It extends httpserv to be capable of running as an OCSP server.

The OCSP server needs access to an NSS database which contains the CA keys used to sign OCSP responses.

httpserv was originally based on selfserv, and I had earlier stripped everything that wasn't necessary.
Now I added back those parts that are required for using an NSS db.

Note you'll see some minor cleanup:
- some messages still printed "selfserv", changed to "httpserv"
- removed some old unnecessary selfserv specific pieces

How will the OCSP server decide, whether to send a good or a revoked status for a cert?
Answer: It will look at CRLs.

Therefore I added parameters to httpserv for specifying the CRLs that will be used, paired with the respective CA's nicknames.
Multiple CA/CRL pairs can be used.

On receiving an OCSP request, httpserv will find the correct pair based on the issuer name sent with the client's OCSP request.

As clients don't send the name and key of the CA cert, but rather the clients send hashes of the CA's name and the CA's key, we must create appropriate hashes of our OCSP CA certificates for comparison purposes.

Because the existing function CERT_CreateOCSPCertID is fixed to lookup issuer certificates, I've added a modified copy of that function to httpserv - CreateSelfCAID - which uses the given cert, rather than its issuer.

For HTTP GET/POST fallback testing on the client side, httpserv has a new option -O, which can be used to restrict which HTTP methods the OCSP server will support.
If the server receives an OCSP request using a HTTP method that has been disabled, the server will reply with a HTTP 400 error.

A single httpserv can support both standard HTTP requests (which will return the contents local file, just like a webserver) and HTTP OCSP requests. Both are distinguished by URL path. If request path begins with /ocsp then it will be processed as an OCSP request.

Note that OCSP GET requests are based on the binary OCSP request, then encoded as base64, and finally urlencoded.
The RFC defines that the request is sent as "http://host:port/path" then followed by "/" then followed by the encoded request.

Comment 64

[Kai Engert [:KaiE:]]

Attachment: v9 - ocsp get etc. (part 2 of 3)

This is a subset of the full patch for this bug.

Most of it are the changes to add client side support to the classic NSS engine for retrieving OCSP information using HTTP GET, plus the code to automatically fall back to HTTP POST on failure.

A subset of this patch are changes required for httpserv, for example I export two existing functions that are used by httpserv, CERT_GetSubjectNameDigest (renamed) and CERT_GetSPKIDigest.

Also, it exports a helper function (for urlencoding) that will be shared with the libpkix client code.

Comment 65

[Kai Engert [:KaiE:]]

Attachment: v9 - ocsp get (etc.) in classic NSS (part 2 of 4)

Comment 64 refers to this patch.

This is a subset of the full patch for this bug.

Most of it are the changes to add client side support to the classic NSS engine for retrieving OCSP information using HTTP GET, plus the code to automatically fall back to HTTP POST on failure.

A subset of this patch are changes required for httpserv, for example I export two existing functions that are used by httpserv, CERT_GetSubjectNameDigest (renamed) and CERT_GetSPKIDigest.

Also, it exports a helper function (for urlencoding) that will be shared with the libpkix client code.

Comment 66

[Kai Engert [:KaiE:]]

Attachment: v9 - ocsp get in libpkix (part 3 of 4)

This is a subset of the full patch for this bug.

It's the libpkix client code for retrieving OCSP information using HTTP GET, plus the code to automatically fall back to HTTP POST on failure.

I had to significantly rewrite/reorder function pkix_pl_OcspResponse_Create, in order to keep it compatible with the non-blocking API.

Also, because libpkix uses 8 space indent, and because I require multiple indend levels, I have changed the function to use 4 space indent.

As a result, reading the patch might be difficult. I recommend to rather read the new function pkix_pl_OcspResponse_Create.

Comment 67

[Kai Engert [:KaiE:]]

Attachment: new function pkix_pl_OcspResponse_Create (for easier review)

> As a result, reading the patch might be difficult. I recommend to rather
> read the new function pkix_pl_OcspResponse_Create.

... which I'm attaching here.

Comment 68

[Kai Engert [:KaiE:]]

Attachment: v9 - test changes (part 4 of 4)

This is a subset of the full patch for this bug.

It changes existing tests and adds new tests.

As of today, tests/libpkix/certs/ contains a set of certificates that have been statically created and checked in, containing OCSP AIA extensions that point to our global OCSP responder.
With the new httpserv functionality, we can start to use the local OCSP responder.

One part of this patch is to dynamically generate certificates during each test run, which will contain OCSP AIA extensions that point to the local test machine.
That's handled in scenario file ocspd.cfg which has been adjusted accordingly.

Because one httpserv process can provide multiple CAs at a single port, we no longer use multiple 2600, 2601 etc. port names. We only need to distinguish between online (available) OCSP responder and offline (not available) port.

We must run ocspd.cfg prior to executing the tests that depend on the certificates, that's why chains.sh has been changed to give special treatment to ocspd.cfg.

We'll prepare a combined server db (for running a single httpserv acting as multiple responders on a single port)
and a client db (which contains all certs, but no keys).

File ocsp.cfg had to be adjusted to use the correct database nicknames, instead of static filenames.

Scenario "method.cfg" is new. It's a minimal OCSP test, that will be run twice.

Once it will be run against a server that supports HTTP POST, only, 
and once against a server that supports HTTP GET, only. 
This will test that client side fallback from GET to POST method works correctly.

The method.cfg scenario uses the vfychain tool, which can use either the classic NSS engine or the libpkix engine.
Testing using both engines will make sure that we test both implementations of the OCSP retrieval and HTTP method fallback code.

(Note that most of the other preexisting chain tests run in libpkix mode, only, as they'll only work with libpkix.)

Comment 69

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Build fixes for part 1: fix Windows build

Kai, there are the minor fixes necessary to get the httpserv changes to build on Windows.

Comment 70

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: An example of how to merge part 2 to the NSS trunk

Kai, when I did the work on bug 853812 I did not realize that it was going to overlap so much with this work. (At the time, nobody was actively working on OCSP GET, IIRC). Before checking in the patch for bug 853812, I tried to make sure that there would be a reasonable way to merge your work in this bug on top of it. The result of that experiment is this patch. I did not review the logic of your changes carefully when I did the merge, but I did verify that your modified chains.sh test suite passes (on Windows). This gave me enough confidence to go ahead and check in my patch for bug 853812.

I noticed something that might be a bug in the patch. Search for "Added logic by bsmith" in this modified vesion. Basically, it seems like the original patch was not decoding the OCSP response in the case where we fell back from GET to POST, in order to populate the pOptionalDecodedResponse output parameter. Also, I adjusted the indention in this modified version of your patch to match the 4-space indention in the rest of the file.

I hope you find this modification of your patch helpful for the merge.

Comment 71

[Patrick McManus [:mcmanus]]

Comment on attachment 745034 [details] [diff] [review]
An example of how to merge part 2 to the NSS trunk

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

I applied this to the nss default hg and then to firefox nightly to check on OCSP HTTP GET, and specifically how it interacted with the necko cache between restarts.

For my two test cases that used EV certs (www.twitter.com and www.mozilla.com) I still saw POST. I guess those use libpkix.

Do EV certs need to use POST OCSP for a reason I'm unfamiliar with, or does this patch just not cover that the libpkix case?

Comment 72

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Patrick McManus [:mcmanus] from comment #71)
> For my two test cases that used EV certs (www.twitter.com and
> www.mozilla.com) I still saw POST. I guess those use libpkix.

Did you apply the patch "v9 - ocsp get in libpkix (part 3 of 4)"?

Comment 73

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 745034 [details] [diff] [review]
An example of how to merge part 2 to the NSS trunk

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

::: lib/certhigh/ocsp.c
@@ +3654,5 @@
> +        }
> +        else {
> +            switch (response->statusValue) {
> +              case ocspResponse_malformedRequest:
> +              case ocspResponse_internalError:

I only looked at this patch for a brief amount of time. But, it seems to me that we need to fall back to POST for anything/everything except     ocspResponse_successful. Otherwise, it seems like an attacker could force a bad OCSP response into a browser's HTTP cache and cause a (nearly-)permanent DoS of the site for which the OCSP response applies. It is really important that we prevent DoS issues, because these types of DoS could do things like permanently disabling a browser's ability to auto-update itself.

In addition, it isn't clear to me if/how the case of a "successful" but expired OCSP response is handled. Expired OCSP responses retrieved by GET also need to fall back to POST, to prevent the same kinds of DoS issues.

Concerns about these DoS issues and (and lack of tests for them in PSM) are what is preventing us from moving forward with this.

Comment 74

[Patrick McManus [:mcmanus]]

(In reply to Brian Smith (:bsmith) from comment #72)
> (In reply to Patrick McManus [:mcmanus] from comment #71)
> > For my two test cases that used EV certs (www.twitter.com and
> > www.mozilla.com) I still saw POST. I guess those use libpkix.
> 
> Did you apply the patch "v9 - ocsp get in libpkix (part 3 of 4)"?

excellent - thanks. that helps. I had received incorrect info on what patch was current here.

Comment 75

[Kai Engert [:KaiE:]]

Brian, since OCSP responses aren't retrieved using a secure channel, an attacker could provide bad responses for either GET or POST requests.

After the retrieval, the handling at the OCSP cache level in NSS is identical for both GET and POST requests - only those responses with a good signature will be cached for a longer duration.

The only difference I can see affect applications that cache HTTP responses.

Since NSS doesn't do any HTTP level caching, the problems you mentioned are outside the scope of this NSS bug and should be discussed in a PSM bug.

My comment for such a separate PSM bug:

I propose that Mozilla applications should never cache the raw HTTP OCSP responses, but rather rely on the functional OCSP cache in NSS.

Since NSS uses the SEC_RegisterDefaultHttpClient API, and since PSM provides an implementation of this callback API, it's already in PSM's control whether or not to enable caching, when using the Necko engine. If you require a flag, which allows PSM to distinguish OCSP HTTP requests from potentially non-OCSP HTTP requests made by NSS, then we could add such a flag in NSS, when assembling the request that gets passed to the HTTP callback API.

Comment 76

[Kai Engert [:KaiE:]]

> > Did you apply the patch "v9 - ocsp get in libpkix (part 3 of 4)"?
> 
> excellent - thanks. that helps. I had received incorrect info on what patch
> was current here.

You must apply 4 parts.

Comment 77

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Kai Engert (:kaie) from comment #75)
> Brian, since OCSP responses aren't retrieved using a secure channel, an
> attacker could provide bad responses for either GET or POST requests.

Yes, but there is a big difference between a temporary attack while you are on a compromised network vs. a permanent attack that persists even after you leave the compromised network. For example, it would be very bad if a MitM could disable Firefox's security update mechanism simply by substituting an Unknown response for a Good one with Cache-Control: max-age=2289600000 (100 years), or an old expired one. Also, it would be very bad if the CA made a mistake with their Cache-Control and accidentally had the max-age extend past the expiration time of the OCSP response, causing a similar DoS of a potentially-critical service.

> The only difference I can see affect applications that cache HTTP responses.
> 
> Since NSS doesn't do any HTTP level caching, the problems you mentioned are
> outside the scope of this NSS bug and should be discussed in a PSM bug.

If an application worked correctly (no exposure to a potential DoS) before this change, then it should work correctly after this change. That's the whole premise of NSS's backward compatibility goal. That means NSS has to handle the case where the application is using HTTP level caching for its HTTP GET requests.

> My comment for such a separate PSM bug:
> 
> I propose that Mozilla applications should never cache the raw HTTP OCSP
> responses, but rather rely on the functional OCSP cache in NSS.

The main reason we (Mozilla) are interested in this bug is specifically because we want a *persistent* OCSP cache (and one that works correctly with our private browsing mode).

> Since NSS uses the SEC_RegisterDefaultHttpClient API, and since PSM provides
> an implementation of this callback API, it's already in PSM's control
> whether or not to enable caching, when using the Necko engine. If you
> require a flag, which allows PSM to distinguish OCSP HTTP requests from
> potentially non-OCSP HTTP requests made by NSS, then we could add such a
> flag in NSS, when assembling the request that gets passed to the HTTP
> callback API.

Making such a change in any application should not be necessary, because of NSS's backward compatibility promise. I highly doubt that Firefox and Thunderbird are the only applications that would be negatively affected by this.

Anyway, the fix I suggested seems very simple to implement, and the negative consequences of not making that fix are severe, so I don't understand how we could justify not fixing it.

Comment 78

[Kai Engert [:KaiE:]]

(1)

You are worried about side effects of using GET by default.
I accept that complaint and your mentioning of backwards compatibility.
I conclude that NSS should continue to use POST (only) by default, and that we should have a global configuration function, that an application can use to enable the "try GET first" approach.

You said, NSS is responsible for not breaking an application. However, where is the code that would cause problems for the application? It's code in the application's HTTP callback function, where caching is being used.

I propose that we introduce a function
  CERT_EnableOCSPGET(PRBool)
that an application can use.

Thereby, only versions of PSM that have been adjusted to correctly deal with GET requests, can enable the GET feature.


(2)

In order to allow PSM to enhance the imlementation of the HTTP callback API, to make it compatible with OCSP GET requests, we can enhance NSS to set a flag in the existing API, by adding an HTTP header, e.g. x-nss-ocsp: yes
The PSM implementation can detect it, and act as appropriate.


(3)

The OCSP GET to POST fallback logic implemented in this patch is simple. The number of variables that influence the decision to retry or not are limited to the status of the HTTP transaction.

You suggest to increase the number of variables that can trigger a retry. Your proposal requires to decode and analyze the contents of the OCSP response.

This isn't as simple as you assume. It would require to restructure the code, because currently the decoding of the OCSP response happens at different scopes.

Your proposal would require to move the retry decision to a different place, and to use the HTTP mechanism as a parameter to the functions that execute the OCSP request.

I'm not convinced it's a good idea to rewrite the code in such a way. If the application doesn't cache the results of the HTTP OCSP GET requests, and relies on the OCSP CACHE only, it isn't necessary.

(4)

You say you want a persistent OCSP cache. This cannot be implement using an HTTP cache, it must be implemented in NSS, as a raw OCSP data cache, in order to cover information retrieved using either GET or POST.

Comment 79

[Kai Engert [:KaiE:]]

Bob, if you agreed to my arguments in comment 78, then you wouldn't have to wait for a new patch.

The additional changes I proposed in comment 78 are small, and could be done in a follow up patch:
- a new global function that controls a global config bool in OCSPGlobalStruct
- in the two places where we decide whether or not to try GET,
  we'd look at above configured bool in addition
- a simple call to SEC_HttpClientFcnV1->addHeaderFcn("x-nss-ocsp: yes")
  in both places where we contruct the HTTP request

Comment 80

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Kai Engert (:kaie) from comment #78)
> 
> The OCSP GET to POST fallback logic implemented in this patch is simple. The
> number of variables that influence the decision to retry or not are limited
> to the status of the HTTP transaction.
> 
> You suggest to increase the number of variables that can trigger a retry.
> Your proposal requires to decode and analyze the contents of the OCSP
> response.
> 
> This isn't as simple as you assume. It would require to restructure the
> code, because currently the decoding of the OCSP response happens at
> different scopes.
>
> Your proposal would require to move the retry decision to a different place,
> and to use the HTTP mechanism as a parameter to the functions that execute
> the OCSP request.

Kai, please look at attachment 745034 [details] [diff] [review] ("An example of how to merge part 2 to the NSS trunk"). It refactors your patch so that it applies cleanly on the current NSS head. See Comment 70 for more background. I think that a side-effect of that modification of your patch makes the change I suggested relatively simple.

> (4)
> 
> You say you want a persistent OCSP cache. This cannot be implement using an
> HTTP cache, it must be implemented in NSS, as a raw OCSP data cache, in
> order to cover information retrieved using either GET or POST.

We are fine with not having saving POST responses in the persistent cache, because, in particular, all CAs in Mozilla's CA program are required to support OCSP GET anyway. We see the persistent cache as purely a performance optimization, and we don't see a need to optimize POST at this time. Therefore, using our HTTP cache as the persistent OCSP cache is good enough. Also, our HTTP cache already correctly handles Firefox's private browsing mode without any extra effort on our part. It would be extra work to modify any kind of persistent caching that NSS would do to correctly handle browsers' private browsing modes.

Comment 81

[Kai Engert [:KaiE:]]

(In reply to Brian Smith (:bsmith) from comment #80)
> 
> We are fine with not having saving POST responses in the persistent cache,
> because, in particular, all CAs in Mozilla's CA program are required to
> support OCSP GET anyway. We see the persistent cache as purely a performance
> optimization, and we don't see a need to optimize POST at this time.
> Therefore, using our HTTP cache as the persistent OCSP cache is good enough.

Brian, please separate the discussion about NSS and Firefox. NSS doesn't have a HTTP cache. NSS already has a different cache and it's working. If you want a HTTP cache in your application instead, then that should be discussed in an application specific bug, and you could ask for an API to disable the NSS internal OCSP cache (in a separate bug).

As you know, NSS is used by a larger number of software, not just by Firefox. You argue that changing NSS behaviour is fine, because it will be sufficient for Mozilla. I don't follow your argument. Mozilla is an application with its own needs, and this is about implementing generic code in NSS, for applications with or without an HTTP cache.

Comment 82

[Kai Engert [:KaiE:]]

(In reply to Brian Smith (:bsmith) from comment #80)
> Kai, please look at attachment 745034 [details] [diff] [review] ("An example
> of how to merge part 2 to the NSS trunk"). It refactors your patch so that
> it applies cleanly on the current NSS head. See Comment 70 for more
> background. I think that a side-effect of that modification of your patch
> makes the change I suggested relatively simple.

The more complicated change is the libpkix code, which you don't touch here.

Comment 83

[Kai Engert [:KaiE:]]

Comment on attachment 745026 [details] [diff] [review]
Build fixes for part 1: fix Windows build

thanks, will merge these fixes

Comment 84

[Kai Engert [:KaiE:]]

Comment on attachment 745034 [details] [diff] [review]
An example of how to merge part 2 to the NSS trunk

Thanks. I'll use your changes. In the meantime even this newer patch required additional merging.

Comment 85

[Kai Engert [:KaiE:]]

Attachment: v10 part 1 of 4 (httpserv)

I've merged the 4 patches to latest nss trunk.

Comment 86

[Kai Engert [:KaiE:]]

Attachment: v10 part 2 of 4 (OCSP GET in classic NSS)

Comment 87

[Kai Engert [:KaiE:]]

Attachment: v10 part 3 of 4 (OCSP GET in libPKIX)

Comment 88

[Kai Engert [:KaiE:]]

Attachment: v10 part 4 of 4 (test changes)

Comment 89

[Kai Engert [:KaiE:]]

Status update:

In today's call, the majority of NSS team members have decided that NSS cannot ignore the GET HTTP caching problem. Even if an application handles the HTTP retrieval and disables all caching at the application level, the application could still talk to proxy servers which aren't controlled by the CA, and that might have a harmful GET result cached.

It was decided, rougly said, if NSS retrieves an OCSP response using GET, it should only accept it, if the response is correctly signed and if it's considered to be "fresh". For everything else (fine tuning pending) NSS should fall back to retry using POST.

This will further complicate the current patch, because currently, network retrieval and response processing happen at different places - in particular in the network code.

The patch will have to be refactored, and the HTTP mechanism will have to be added as a parameter to the internal network retrieval mechanism.

Because the current patches as is are considered not yet acceptable for Firefox and Chromium, I'm not sure whether it makes sense to review the current patches. Parts 2 and 3 of the patches might look much different after the refactoring.

Comment 90

[Kai Engert [:KaiE:]]

> This will further complicate the current patch, because currently, network
> retrieval and response processing happen at different places - in particular
> in the network code.

meant to say "in particular in the LIBPKIX code"

Comment 91

[Kai Engert [:KaiE:]]

At least during the next 2-3 weeks I won't be able to work on this bug.

Comment 92

[Kai Engert [:KaiE:]]

Attachment: v11 part 1 of 4 (httpserv)

minor merging to trunk

Comment 93

[Kai Engert [:KaiE:]]

Attachment: v11 part 2 of 4 (OCSP GET in classic NSS)

Refactored the code classic code. If OCSP response obtained using GET is a valid signed and fresh response, but response status isn't "good", then retry with POST. Passes tests.

Will work on libPKIX portion next.

Comment 94

[Kai Engert [:KaiE:]]

Attachment: v12 - httpserv - part 1 of 4

Comment 95

[Kai Engert [:KaiE:]]

Attachment: v12 - GET in classic - part 2 of 4

Comment 96

[Kai Engert [:KaiE:]]

Attachment: v12 - GET in libPKIX - part 3 of 4

Comment 97

[Kai Engert [:KaiE:]]

Attachment: v13 - httpserv - part 1-of-4

Comment 98

[Kai Engert [:KaiE:]]

Attachment: v13 - tests - part 2-of-4

This contains additional testing, enabled by another mode in httpserv.

With the earlier version of patch, the simulated failures of GET (to force a fallback to POST) were failures at an early stage of the HTTP connection.

The new httpserv mode "-O get-revoked" will cause all GET responses, that would otherwise have "valid and good OCSP status" to return a "valid and revoked OCSP status".

This allows us to test that our library client OCSP retrieval logic indeed falls back to POST after receiving a valid GET response with a non-good status.

Comment 99

[Kai Engert [:KaiE:]]

Attachment: v13 - GET for classic - part 3-of-4

Comment 100

[Kai Engert [:KaiE:]]

Attachment: v13 - GET for libPKIX - part 4-of-4

Comment 101

[Kai Engert [:KaiE:]]

The patches pass all tests and are ready for review.

Comment 102

[Robert Relyea]

Comment on attachment 795958 [details] [diff] [review]
v13 - GET for classic - part 3-of-4

r+ rrelyea

Comment 103

[Robert Relyea]

Comment on attachment 795952 [details] [diff] [review]
v13 - httpserv - part 1-of-4

r+ rrelyea

Comment 104

[Robert Relyea]

Comment on attachment 795957 [details] [diff] [review]
v13 - tests - part 2-of-4

r+ rrelyea

Comment 105

[Robert Relyea]

Kai, in the pkix case, it looks like we are retrying POST even if we get a valid revocation response from GET! If GET returns a valid revocation response we should return that response. I think you can accomplish this by changing the test at line 381 of pkis_OcspChecker_CheckExternal from 

  if (currentStage == stageGET && revStatus != PKIX_RevStatus_Success) {

to

  if (currentStage == stageGET && revStatus == PKIX_RevStatus_NoInfo) {

(you could remove line 386 which sets revStatus to NoInfo with this change as well).

bob

Comment 106

[Kai Engert [:KaiE:]]

(In reply to Robert Relyea from comment #105)
> Kai, in the pkix case, it looks like we are retrying POST even if we get a
> valid revocation response from GET! 

Yes. The classic patch does so, too!


> If GET returns a valid revocation response we should return that response.

Ok, that makes sense.

But if the valid signed GET status is "unknown", we'll retry with POST.


> I think you can accomplish this by.... [snip]

I agree. However, I'd prefer to explicitly test for "neither good nor revoked", in case the enum ever gets extended to allow additional values besides "no-info", "good", "revoked".

I'll attach an updated
- patch v14 libpkix part 4-of-4


In order to adjust the classic code in the same way for consistency, 
I'll also attach an
- incremental classic patch on top of the already reviewed part 3-of-4


Furthermore, your change request will require small updates for httpserv and for testing, too. I had added the testing mode "get-revoked" for testing the automatic fallback. Since you don't want to retry on revoked status, I'll change that to testing mode "get-unknown" to return an unknown status if GET is being used, and attach further small incremental patches.

Comment 107

[Kai Engert [:KaiE:]]

Attachment: v14 - GET for libPKIX - part 4-of-4

The slightly updated patch for libPKIX, as requested.

Comment 108

[Kai Engert [:KaiE:]]

Attachment: v4 - incremental for httpserv / tests / classic [merged with v13 patches at time of checkin]

This is an incremental patch on top of the already reviewed patches, parts 1, 2, and 3.

It changes the classic code to accept a GET response with status revoked.

It changes the testing code to test that we do fall back to POST, if GET said "unknown".

Comment 109

[Kai Engert [:KaiE:]]

(In reply to Kai Engert (:kaie) from comment #107)
> Created attachment 802458 [details] [diff] [review]
> v14 - GET for libPKIX - part 4-of-4
> 
> The slightly updated patch for libPKIX, as requested.

Use this link to see the interdiff of my changes to the libPKIX code:
https://bugzilla.mozilla.org/attachment.cgi?oldid=795960&action=interdiff&newid=802458&headers=1

Comment 110

[Robert Relyea]

Comment on attachment 802458 [details] [diff] [review]
v14 - GET for libPKIX - part 4-of-4

r+ rrelyea

Comment 111

[Kai Engert [:KaiE:]]

Comment on attachment 795952 [details] [diff] [review]
v13 - httpserv - part 1-of-4

https://hg.mozilla.org/projects/nss/rev/bfc9085d0676

Comment 112

[Kai Engert [:KaiE:]]

Comment on attachment 795957 [details] [diff] [review]
v13 - tests - part 2-of-4

https://hg.mozilla.org/projects/nss/rev/43bc35615a4e

Comment 113

[Kai Engert [:KaiE:]]

Comment on attachment 795958 [details] [diff] [review]
v13 - GET for classic - part 3-of-4

https://hg.mozilla.org/projects/nss/rev/654082cbd702
https://hg.mozilla.org/projects/nss/rev/4ebfd5d26867

(fixed nss.def to use version 3.15.3)

Comment 114

[Kai Engert [:KaiE:]]

Comment on attachment 802458 [details] [diff] [review]
v14 - GET for libPKIX - part 4-of-4

https://hg.mozilla.org/projects/nss/rev/a9ee920bd5cf

Comment 115

[Kai Engert [:KaiE:]]

checked in a bustage fix
https://hg.mozilla.org/projects/nss/rev/b7e765fda4cf

(regular expression to extract port number failed if hostname contains a digit)

Comment 116

[Kai Engert [:KaiE:]]

Attachment: bustage fix, add -w parameter to pp tool, to print output unwrapped

An additional bustage fix is required.

Our tests script attempt to dynamically extract the host name and port number from a certificate, by making use of the "pp" utility.

Unfortunately, when the hostname is long, as it's the case with some of Mozilla's build VMs, the pp utility wraps the URL. As a result, the grep command extracts only a subset of the URI, and the port number may be cut off.

As a fix, I propose to add a "-w" command line option to the "pp" utility, which means "don't wrap long lines", and change the URI extraction to use that parameter.

(In the past, NSS developers have usually asked for consistency, so I'm not simply fixing the output of the URI, but rather introduce a way to  consistenctly ask for one output format or the other (default = wrapped; -p = not wrapped.)

With the proposed fix, the default behaviour of "pp" won't change for most output types. (Only the output of the certificate-identity will change slightly, as it was previously hardcoded not to wrap some parts, and now is consistent, too.)

I've already checked this patch in, to fix the bustage.
https://hg.mozilla.org/projects/nss/rev/4be429189df6

Bob, can you please review?

Comment 117

[Kai Engert [:KaiE:]]

Attachment: memory leak fix

Automated testing found a new memory leak.
All reported stacks point to the same place.

I've checked in this trivial cleanup patch as a bustage fix, which fixed the leak in my local testing.

https://hg.mozilla.org/projects/nss/rev/c2e4eb8d2ede

Comment 118

[Kai Engert [:KaiE:]]

changing target milestone to 3.15.4

Comment 119

[Wan-Teh Chang]

Attachment: Fix compiler warnings about the arguments to PL_Base64Encode in cert_GetOCSPResponse

Comment 120

[Wan-Teh Chang]

Comment on attachment 8345457 [details] [diff] [review]
Fix compiler warnings about the arguments to PL_Base64Encode in cert_GetOCSPResponse

Patch checked in: https://hg.mozilla.org/projects/nss/rev/7915c49baf3d

Comment 121

[Wan-Teh Chang]

Attachment: Fix more compiler warnings

The compiler warns that 'mechanism' is uninitialized if we take the
"else" branch that has the PORT_Assert(0).

The compiler warns about a switch statement that doesn't handle all
cases of the enum type.

The compiler warns about wrong pointer argument types (char * vs.
unsigned char *) passed to PL_Base64Encode.

Comment 122

[Wan-Teh Chang]

Attachment: Rename functions and arguments, fix compiler warnings

1. "mechanism" is renamed "method" in function or variable/argument names
that refer to the HTTP GET and POST methods. They are "HTTP methods", not
"HTTP mechanisms".

2. Do not export CERT_GetEncodedOCSPResponseByMechanism (renamed
CERT_GetEncodedOCSPResponseByMethod) because it is not declared in the
ocsp.h header and is not being used.

3. CERT_GetSPKIDigest is renamed CERT_GetSubjectPublicKeyDigest because
it digests just the subjectPublicKey rather than the whole
subjectPublicKeyInfo. This difference is important.

Comment 123

[Wan-Teh Chang]

Comment on attachment 795958 [details] [diff] [review]
v13 - GET for classic - part 3-of-4

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

::: lib/nss/nss.def
@@ +1038,5 @@
>  ;+       *;
>  ;+};
> +;+NSS_3.15.2 { 	# NSS 3.15.2 release
> +;+    global:
> +CERT_DestroyCrl;

Bob:

CERT_DestroyCrl is a synonym of SEC_DestroyCrl:
http://mxr.mozilla.org/nss/ident?i=CERT_DestroyCrl

and SEC_DestroyCrl is already exported.

I just wanted to make sure you knew about this and you approved the
export of CERT_DestroyCrl for readability or API consistency.

In general it is confusing to have two functions that do exactly the
same thing. If we export both of them, we should mark one as preferred
and the other one as deprecated.

Comment 124

[Wan-Teh Chang]

Attachment: Don't export CERT_DestroyCrl. Use SEC_DestroyCrl instead

SEC_DestroyCrl is declared in "certdb.h", so I need to include that
header. I also removed unnecessary headers "certt.h" and "ocspt.h".
The NSS convention is that the "xxx.h" headers include the corresponding
"xxxt.h" headers.

Comment 125

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8349838 [details] [diff] [review]
Rename functions and arguments, fix compiler warnings

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

::: lib/certdb/cert.h
@@ +1506,5 @@
>   * Digest the cert's subject public key using the specified algorithm.
>   * The necessary storage for the digest data is allocated.  If "fill" is
>   * non-null, the data is put there, otherwise a SECItem is allocated.
>   * Allocation from "arena" if it is non-null, heap otherwise.  Any problem
>   * results in a NULL being returned (and an appropriate error set).

Please add a comment here emphasizing that this does NOT digest the whole SubjectPublicKeyInfo structure.

::: lib/certhigh/ocsp.c
@@ +3529,5 @@
>                                       signerCert);
>      if (!request)
>          return NULL;
>      return ocsp_GetEncodedOCSPResponseFromRequest(arena, request, location, 
> +                                                  method, time, addServiceLocator, 

nit: trailing whitespace on this line.

@@ +3650,3 @@
>          encodedResponse = cert_GetOCSPResponse(arena, location, encodedRequest);
>      }
> +    else if (!strcmp(method, "POST")) {

It is unfortunate that this function takes a string instead of a simple enum for the method parameter. strcmp() is less efficient and more dangerous than operator==(enum, enum). We can improve this in another bug, if we ever need to export this function.

@@ +3789,5 @@
>                                                 addServiceLocator, NULL);
>      if (!request)
>          return NULL;
>      return ocsp_GetEncodedOCSPResponseFromRequest(arena, request, location, 
> +                                                  method, time, addServiceLocator, 

nit: trailing whitespace on this line.

::: lib/certhigh/ocspsig.c
@@ +477,1 @@
>  		goto done;

nit: This is easier to read (aligning wrapped parameters at parentheses and using braces):

	if (!CERT_GetSubjectPublicKeyDigest(tmpArena, responderCert,
	                                    SEC_OID_SHA1,
					    &rid->responderIDValue.keyHash)) {
	    goto done;
	}

Comment 126

[Wan-Teh Chang]

Comment on attachment 8350178 [details] [diff] [review]
Don't export CERT_DestroyCrl. Use SEC_DestroyCrl instead

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

Patch checked in: https://hg.mozilla.org/projects/nss/rev/09ae2945caf7

Comment 127

[Wan-Teh Chang]

Attachment: Rename functions and arguments, fix compiler warnings, v2

I made all the changes Brian suggested (except for the one about argument
formatting, which would still exceed the 80-char line limit).

Patch checked in: https://hg.mozilla.org/projects/nss/rev/a3d86225543c