Adding OCSP Parser

Addition of an OCSP parser using the net der code.

BUG=

Committed: https://crrev.com/be4817807690ad47baa4e87a9fb538bca04649f8
Cr-Commit-Position: refs/heads/master@{#383085}

[svaldez, 2015-12-30 18:18:43]

Mostly FYI for now. Still working on tests and adding comments.

[svaldez, 2015-12-30 18:19:04]

Description was changed from

==========
Adding initial OCSP Parser code

BUG=
==========

to

==========
Addition of an OCSP parser using the net der code.

BUG=
==========

[Ryan Sleevi, 2015-12-30 18:55:17]

rsleevi@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2015-12-30 18:55:18]

Quick drive by.

The biggest question will be the high-level approach to whether or not we parse
everything up front, or defer parsing until necessary/instantiated. Much of the
goal here has been to avoid parsing too much into the structure unnecessarily,
and only do so when requested/on-demand, which will generally only be once and
can be done via stack objects.

This makes me wonder whether it's more desirable to have a class-level
abstraction which digs in to the subsequent objects, but at a high level treats
everything as StringPieces and just records the offsets necessary for
conversions.

Hopefully David and Eric will chime in.

Also, suggests we should write up some markdown documentation for the internal
ASN.1 bits so that we have a common approach, since there are many (entirely
valid) ways to do it.

https://codereview.chromium.org/1541213002/diff/20001/net/cert/ocsp_parser.cc
File net/cert/ocsp_parser.cc (right):

https://codereview.chromium.org/1541213002/diff/20001/net/cert/ocsp_parser.cc...
net/cert/ocsp_parser.cc:42: }
BUG: Fail to handle other status tags.

https://codereview.chromium.org/1541213002/diff/20001/net/cert/ocsp_parser.cc...
net/cert/ocsp_parser.cc:96: if (version_present) {
newline

https://codereview.chromium.org/1541213002/diff/20001/net/cert/ocsp_parser.h
File net/cert/ocsp_parser.h (right):

https://codereview.chromium.org/1541213002/diff/20001/net/cert/ocsp_parser.h#...
net/cert/ocsp_parser.h:21: namespace ct {
wrong namespace ;)

https://codereview.chromium.org/1541213002/diff/20001/net/cert/ocsp_parser.h#...
net/cert/ocsp_parser.h:24: const uint8_t kOidPkixOcspBasic[] = {0x2b, 0x06,
0x01, 0x05, 0x05,
Don't want this in a header (will end up duplicating the storage into every TU)

https://codereview.chromium.org/1541213002/diff/20001/net/cert/ocsp_parser.h#...
net/cert/ocsp_parser.h:27: enum OCSPRevocationReason {
Inline to single response?

https://codereview.chromium.org/1541213002/diff/20001/net/cert/ocsp_parser.h#...
net/cert/ocsp_parser.h:41: enum OCSPCertStatus {
Inline to single response?

https://codereview.chromium.org/1541213002/diff/20001/net/cert/ocsp_parser.h#...
net/cert/ocsp_parser.h:47: enum OCSPResponseStatus {
inline to Response

https://codereview.chromium.org/1541213002/diff/20001/net/cert/ocsp_parser.h#...
net/cert/ocsp_parser.h:78: std::vector<ParsedExtension> extensions;
Not sure if we want to fully parse these; much of the other parsing code defers
parsing substructures to be done lazily.

[svaldez, 2015-12-30 19:31:37]

Yeah, we should probably have similar patterns in how we parse ASN.1.

My original reason for not going with lazy parsing is thinking that we should
probably be making sure that the OCSPResponse is correctly formed, and whether
we should be accepting signed responses that are ill-formed, possibly as a
result of a dumb server that isn't doing DER encoding correctly or some other
issue.

https://codereview.chromium.org/1541213002/diff/20001/net/cert/ocsp_parser.cc
File net/cert/ocsp_parser.cc (right):

https://codereview.chromium.org/1541213002/diff/20001/net/cert/ocsp_parser.cc...
net/cert/ocsp_parser.cc:42: }
On 2015/12/30 18:55:18, Ryan Sleevi wrote:
> BUG: Fail to handle other status tags.

Done.

https://codereview.chromium.org/1541213002/diff/20001/net/cert/ocsp_parser.cc...
net/cert/ocsp_parser.cc:96: if (version_present) {
On 2015/12/30 18:55:18, Ryan Sleevi wrote:
> newline

Done.

https://codereview.chromium.org/1541213002/diff/20001/net/cert/ocsp_parser.h
File net/cert/ocsp_parser.h (right):

https://codereview.chromium.org/1541213002/diff/20001/net/cert/ocsp_parser.h#...
net/cert/ocsp_parser.h:21: namespace ct {
On 2015/12/30 18:55:18, Ryan Sleevi wrote:
> wrong namespace ;)

Done.

https://codereview.chromium.org/1541213002/diff/20001/net/cert/ocsp_parser.h#...
net/cert/ocsp_parser.h:24: const uint8_t kOidPkixOcspBasic[] = {0x2b, 0x06,
0x01, 0x05, 0x05,
On 2015/12/30 18:55:18, Ryan Sleevi wrote:
> Don't want this in a header (will end up duplicating the storage into every
TU)

Done.

https://codereview.chromium.org/1541213002/diff/20001/net/cert/ocsp_parser.h#...
net/cert/ocsp_parser.h:27: enum OCSPRevocationReason {
On 2015/12/30 18:55:18, Ryan Sleevi wrote:
> Inline to single response?

Done.

https://codereview.chromium.org/1541213002/diff/20001/net/cert/ocsp_parser.h#...
net/cert/ocsp_parser.h:41: enum OCSPCertStatus {
On 2015/12/30 18:55:18, Ryan Sleevi wrote:
> Inline to single response?

Done.

https://codereview.chromium.org/1541213002/diff/20001/net/cert/ocsp_parser.h#...
net/cert/ocsp_parser.h:47: enum OCSPResponseStatus {
On 2015/12/30 18:55:18, Ryan Sleevi wrote:
> inline to Response

Done.

[svaldez, 2016-01-11 18:01:40]

Fixed the issues.

[svaldez, 2016-01-11 18:02:11]

On 2016/01/11 18:01:40, svaldez wrote:
> Fixed the issues.

Commented on the wrong CL.

[svaldez, 2016-01-12 19:25:35]

On 2016/01/11 18:02:11, svaldez wrote:
> On 2016/01/11 18:01:40, svaldez wrote:
> > Fixed the issues.
> 
> Commented on the wrong CL.

eroman: Any thoughts on how much parsing we should be doing?

We probably don't need to initially parse the extensions, though we might want
to parse out all the way through to OCSPSingleResponse, so that we don't have to
keep re-parsing that whenever we want to check the revocation status of a cert
against the OCSPResponse.

[svaldez, 2016-01-20 18:05:38]

On 2016/01/12 19:25:35, svaldez wrote:
> On 2016/01/11 18:02:11, svaldez wrote:
> > On 2016/01/11 18:01:40, svaldez wrote:
> > > Fixed the issues.
> > 
> > Commented on the wrong CL.
> 
> eroman: Any thoughts on how much parsing we should be doing?
> 
> We probably don't need to initially parse the extensions, though we might want
> to parse out all the way through to OCSPSingleResponse, so that we don't have
to
> keep re-parsing that whenever we want to check the revocation status of a cert
> against the OCSPResponse.

Split out into 3 parsing passes and added initial (broken) signature
verification. If you could take a look at the parsing stuff, I don't know
whether we want both the OCSP parsing and signature/chain verification in the
same CL or separate CLs.

[svaldez, 2016-01-25 21:48:05]

svaldez@chromium.org changed reviewers:
+ eroman@chromium.org

[eroman, 2016-02-03 22:54:46]

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.cc
File net/cert/ocsp_parser.cc (right):

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:1: // Copyright 2015 The Chromium Authors. All rights
reserved.
can change to 2016

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:40: bool ParseOCSPSingleResponse(der::Input raw_tlv,
OCSPSingleResponse* out) {
Please provide comments somewhere that reflect the structure being parsed.
Preferably at the top of the function, and optionally also inline when reading
the various parts:

SingleResponse ::= SEQUENCE {
   certID                  CertID,
   certStatus              CertStatus,
   thisUpdate              GeneralizedTime,
   nextUpdate          [0] EXPLICIT GeneralizedTime OPTIONAL,
   singleExtensions    [1] EXPLICIT Extensions OPTIONAL }

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:43: if (!response_parser.ReadSequence(&parser))
It doesn't look like unconsumed data is being checked for. In other words
shouldn't ParseOCSPSingleResponse fail if:

  * There is something after the sequence in |raw_tlv|
  * There is something after singleExtensions

Other code handles this by checking HasMore() in both places, unless there are
explicit extension points in the structure.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:47: if (!parser.ReadTag(der::kSequence, &cert_id))
The status parsing is a good candidate to move to a separate helper function.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:58: der::Parser revoked_info_parser(status);
Same issue here, what if there is unconsumed data at the end?

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:68: out->revocation_reason =
static_cast<OCSPSingleResponse::RevocationReason>(
This needs to ensure that the number is in range, or else use a generic integer
instead of a RevocationReason (since it is unexpected to end up with a value
that isn't part of enum).

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:71: out->cert_status = OCSPSingleResponse::CERT_UNKNOWN;
In the case of !CERT_REVOKED would be a good idea to reset the fields that are
not applicable in case they are mistakenly accessed. The documentation should
also make clear that those fields are not to be used unless if the status is
revoked.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:87: der::Parser next_update_parser(next_update_input);
Same issue here, this should fail if there was unconsumed data after the
generalized time

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:89: if
(!next_update_parser.ReadGeneralizedTime(&(out->next_update)))
How does a caller distinguish when it was present? I don't see that next_update
is reset in the case where it was not parsed.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:94: &(out->extensions), &extensions_present)) {
Same question here: how do callers of the function distinguish when there were
extensions or not? Also unless the expectation is that |out| is a default
initialized value, then out->extensions probably needs to be reset in the case
where it is not present. An empty input though would still be ambiguous as to
whether it was simply not present, or whether it was present but empty (which I
believe is invalid per the definition of extensions)

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:101: bool ParseOCSPResponseData(der::Input raw_tlv,
OCSPResponseData* out) {
Document the structure being parsed, referencing the RFC

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:103: if (!der::Parser(raw_tlv).ReadSequence(&parser))
Fail on unconsumed data?

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:118: if (!der::ParseUint8(version, &(out->version)))
Because the version is DEFAULT v1, a strict DER parser should also reject if the
value was provided as 0 (since DEFAULT values are not to be encoded.

As I recall there are a number of bugs in the wild around this though so
compatibility wise will likely need to allow it. But should call this out in the
code at least.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:138: if (responder_key.Length() != base::kSHA1Length)
given the memcpy below, I would say compare the length against
sizeof(key_hash.data) instead to make it clear there isn't possibility of
overrun.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:141: memcpy(key_hash.data,
responder_key.AsString().data(), base::kSHA1Length);
Unnecessary to call AsString() that will construct a temporary throw-away
string. Instead I guess UnsafeData().

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:160: out->responses.push_back(single_response);
Should out->responses not be cleared before starting? (Documentation doesn't say
that the output must be default initialized.) Implication will be if re-using an
object while parsing, will get left-over goop from previous run.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:165: &(out->extensions), &extensions_present)) {
Same question here about disambiguating present and empty vs omitted.

Is Extensions here the same as from RFC  5280? Because we already expose a
function for parsing that.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:171: bool ParseOCSPResponse(der::Input ocsp_response,
OCSPResponse* out) {
document the structure being parsed and reference to RFC

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:172: der::Parser parser(ocsp_response);
What about unconsumed data?

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:184: if (out->status != OCSPResponse::SUCCESSFUL)
Why is this a failure?

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:192: &response_bytes_input_parser)) {
What about unconsumed data?

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:204: der::Parser response_parser(response_string);
What about unconsumed data?

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:207: der::Input sigalg_tlv;
This function is big. It needs some documentation to guide what parts are being
parsed and what the corresponding ASN.1 is, or break it up into smaller
functions for the sub-parts.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:227: der::Parser certs_input_parser(certs_input);
Same comment as previously. (It is hard to follow which sub-parts are being
parsed in this function)

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:227: der::Parser certs_input_parser(certs_input);
Unconsumed data?

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:236: if (!ParseCertificate(cert_tlv, &parsed_cert))
I am not convinced that parsing the certificates should be done here.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:269: bool VerifyOCSPResponse(OCSPResponse* response,
ParsedCertificate* cert) {
I didn't review this function yet, but I am skeptical about how it re-implements
stuff from VerifyCertificateChain. I imagine it should be calling that instead
with a mode for indicating it is a CRL.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.h
File net/cert/ocsp_parser.h (right):

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.h...
net/cert/ocsp_parser.h:1: // Copyright 2015 The Chromium Authors. All rights
reserved.
can change to 2016

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.h...
net/cert/ocsp_parser.h:5: #ifndef NET_CERT_OCSP_PARSER_H_
Any reason to put this in cert as opposed to cert/internal?

Mostly we have been dumping the new stuff into cert/internal while the APIs
shake out, and this does depend on other files in cert/internal

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.h...
net/cert/ocsp_parser.h:25: enum CertStatus {
Not sure that our style guide enshrines one way or another, but have you
considered making these enum class instead? The main advantage IMO is that it
won't then implicitly promote integers or other enums to this type (i.e. catch
more misuse at compile time).

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.h...
net/cert/ocsp_parser.h:48: std::string cert_id;
Why a std::string for cert_id but der::Input for extensions? (i.e. one copies
the data, the other refereces it).

Also it is unclear what cert_id is (i.e. is it the TLV, is it the contents of
the sequence, or something else).

Lastly it is not clear that cert_id has not been validated.

My suggestion is to keep the stuff that has not been validated yet as a
der::Input, and call this out in the documentation.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.h...
net/cert/ocsp_parser.h:57: struct OCSPResponseData {
Please provide documentation for these structures that reference the RFC where
these are defined, and/or the mapping between these types and those in the RFC
(most of which are implied by the name).

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.h...
net/cert/ocsp_parser.h:108: // updated during parsing, whereas others may not
have been changed.
Worth mentioning that |out| references data from |raw_tlv|, so it must remain
alive.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.h...
net/cert/ocsp_parser.h:130: NET_EXPORT_PRIVATE bool
VerifyOCSPResponse(OCSPResponse* response,
Why are these non-const pointers? Are they expected to be mutated by the
invocation?

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser_u...
File net/cert/ocsp_parser_unittest.cc (right):

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser_u...
net/cert/ocsp_parser_unittest.cc:25: TEST_F(OCSPParserTest, GoodOCSPResponse) {
Move this data to .pem data files instead, and put in in net/data.

See the various examples in for instance net/data/parse_certificate_unittest

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser_u...
net/cert/ocsp_parser_unittest.cc:236: 0x30, 0x64, 0x0a, 0x01, 0x00, 0xa0, 0x5f,
0x30, 0x5d, 0x06, 0x09, 0x2b,
Same here.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser_u...
net/cert/ocsp_parser_unittest.cc:252: }
Given just how complicated the parsing of OCSP responses are, we will need a lot
more test cases to exercise the various paths and combinations.

https://codereview.chromium.org/1541213002/diff/260001/net/der/input.h
File net/der/input.h (right):

https://codereview.chromium.org/1541213002/diff/260001/net/der/input.h#newcode58
net/der/input.h:58: bool Equals(const Input& other) const;
Should delete Equals if introducing operator== so there is just 1 way to do it.

[svaldez, 2016-02-04 19:03:25]

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.cc
File net/cert/ocsp_parser.cc (right):

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:1: // Copyright 2015 The Chromium Authors. All rights
reserved.
On 2016/02/03 22:54:44, eroman wrote:
> can change to 2016

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:40: bool ParseOCSPSingleResponse(der::Input raw_tlv,
OCSPSingleResponse* out) {
On 2016/02/03 22:54:45, eroman wrote:
> Please provide comments somewhere that reflect the structure being parsed.
> Preferably at the top of the function, and optionally also inline when reading
> the various parts:
> 
> SingleResponse ::= SEQUENCE {
>    certID                  CertID,
>    certStatus              CertStatus,
>    thisUpdate              GeneralizedTime,
>    nextUpdate          [0] EXPLICIT GeneralizedTime OPTIONAL,
>    singleExtensions    [1] EXPLICIT Extensions OPTIONAL }

Should this be both here and in the header file, or is the header sufficient?

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:43: if (!response_parser.ReadSequence(&parser))
On 2016/02/03 22:54:45, eroman wrote:
> It doesn't look like unconsumed data is being checked for. In other words
> shouldn't ParseOCSPSingleResponse fail if:
> 
>   * There is something after the sequence in |raw_tlv|
>   * There is something after singleExtensions
> 
> Other code handles this by checking HasMore() in both places, unless there are
> explicit extension points in the structure.

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:47: if (!parser.ReadTag(der::kSequence, &cert_id))
On 2016/02/03 22:54:45, eroman wrote:
> The status parsing is a good candidate to move to a separate helper function.

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:58: der::Parser revoked_info_parser(status);
On 2016/02/03 22:54:45, eroman wrote:
> Same issue here, what if there is unconsumed data at the end?

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:68: out->revocation_reason =
static_cast<OCSPSingleResponse::RevocationReason>(
On 2016/02/03 22:54:45, eroman wrote:
> This needs to ensure that the number is in range, or else use a generic
integer
> instead of a RevocationReason (since it is unexpected to end up with a value
> that isn't part of enum).

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:71: out->cert_status = OCSPSingleResponse::CERT_UNKNOWN;
On 2016/02/03 22:54:44, eroman wrote:
> In the case of !CERT_REVOKED would be a good idea to reset the fields that are
> not applicable in case they are mistakenly accessed. The documentation should
> also make clear that those fields are not to be used unless if the status is
> revoked.

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:87: der::Parser next_update_parser(next_update_input);
On 2016/02/03 22:54:45, eroman wrote:
> Same issue here, this should fail if there was unconsumed data after the
> generalized time

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:89: if
(!next_update_parser.ReadGeneralizedTime(&(out->next_update)))
On 2016/02/03 22:54:45, eroman wrote:
> How does a caller distinguish when it was present? I don't see that
next_update
> is reset in the case where it was not parsed.

Done.

Adding has_... for all optional fields.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:94: &(out->extensions), &extensions_present)) {
On 2016/02/03 22:54:45, eroman wrote:
> Same question here: how do callers of the function distinguish when there were
> extensions or not? Also unless the expectation is that |out| is a default
> initialized value, then out->extensions probably needs to be reset in the case
> where it is not present. An empty input though would still be ambiguous as to
> whether it was simply not present, or whether it was present but empty (which
I
> believe is invalid per the definition of extensions)

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:101: bool ParseOCSPResponseData(der::Input raw_tlv,
OCSPResponseData* out) {
On 2016/02/03 22:54:45, eroman wrote:
> Document the structure being parsed, referencing the RFC

Acknowledged.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:103: if (!der::Parser(raw_tlv).ReadSequence(&parser))
On 2016/02/03 22:54:44, eroman wrote:
> Fail on unconsumed data?

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:118: if (!der::ParseUint8(version, &(out->version)))
On 2016/02/03 22:54:44, eroman wrote:
> Because the version is DEFAULT v1, a strict DER parser should also reject if
the
> value was provided as 0 (since DEFAULT values are not to be encoded.
> 
> As I recall there are a number of bugs in the wild around this though so
> compatibility wise will likely need to allow it. But should call this out in
the
> code at least.

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:138: if (responder_key.Length() != base::kSHA1Length)
On 2016/02/03 22:54:44, eroman wrote:
> given the memcpy below, I would say compare the length against
> sizeof(key_hash.data) instead to make it clear there isn't possibility of
> overrun.

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:141: memcpy(key_hash.data,
responder_key.AsString().data(), base::kSHA1Length);
On 2016/02/03 22:54:44, eroman wrote:
> Unnecessary to call AsString() that will construct a temporary throw-away
> string. Instead I guess UnsafeData().

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:160: out->responses.push_back(single_response);
On 2016/02/03 22:54:44, eroman wrote:
> Should out->responses not be cleared before starting? (Documentation doesn't
say
> that the output must be default initialized.) Implication will be if re-using
an
> object while parsing, will get left-over goop from previous run.

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:165: &(out->extensions), &extensions_present)) {
On 2016/02/03 22:54:45, eroman wrote:
> Same question here about disambiguating present and empty vs omitted.
> 
> Is Extensions here the same as from RFC  5280? Because we already expose a
> function for parsing that.

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:171: bool ParseOCSPResponse(der::Input ocsp_response,
OCSPResponse* out) {
On 2016/02/03 22:54:45, eroman wrote:
> document the structure being parsed and reference to RFC

Acknowledged.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:172: der::Parser parser(ocsp_response);
On 2016/02/03 22:54:45, eroman wrote:
> What about unconsumed data?

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:184: if (out->status != OCSPResponse::SUCCESSFUL)
On 2016/02/03 22:54:44, eroman wrote:
> Why is this a failure?

It isn't. "return true" since we don't have to parse the rest of the data.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:192: &response_bytes_input_parser)) {
On 2016/02/03 22:54:44, eroman wrote:
> What about unconsumed data?

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:204: der::Parser response_parser(response_string);
On 2016/02/03 22:54:45, eroman wrote:
> What about unconsumed data?

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:227: der::Parser certs_input_parser(certs_input);
On 2016/02/03 22:54:44, eroman wrote:
> Unconsumed data?

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.c...
net/cert/ocsp_parser.cc:269: bool VerifyOCSPResponse(OCSPResponse* response,
ParsedCertificate* cert) {
On 2016/02/03 22:54:44, eroman wrote:
> I didn't review this function yet, but I am skeptical about how it
re-implements
> stuff from VerifyCertificateChain. I imagine it should be calling that instead
> with a mode for indicating it is a CRL.

Unfortunately the certificate "chain" used for OCSP handling has very different
rules from normal chain verification. The chain attached to the OCSPResponse is
allowed to be an arbitrary set of certs, and we are only allowed to chain one
step from the issuer of the certificate.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.h
File net/cert/ocsp_parser.h (right):

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.h...
net/cert/ocsp_parser.h:1: // Copyright 2015 The Chromium Authors. All rights
reserved.
On 2016/02/03 22:54:45, eroman wrote:
> can change to 2016

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.h...
net/cert/ocsp_parser.h:5: #ifndef NET_CERT_OCSP_PARSER_H_
On 2016/02/03 22:54:45, eroman wrote:
> Any reason to put this in cert as opposed to cert/internal?
> 
> Mostly we have been dumping the new stuff into cert/internal while the APIs
> shake out, and this does depend on other files in cert/internal

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.h...
net/cert/ocsp_parser.h:25: enum CertStatus {
On 2016/02/03 22:54:45, eroman wrote:
> Not sure that our style guide enshrines one way or another, but have you
> considered making these enum class instead? The main advantage IMO is that it
> won't then implicitly promote integers or other enums to this type (i.e. catch
> more misuse at compile time).

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.h...
net/cert/ocsp_parser.h:48: std::string cert_id;
On 2016/02/03 22:54:45, eroman wrote:
> Why a std::string for cert_id but der::Input for extensions? (i.e. one copies
> the data, the other refereces it).
> 
> Also it is unclear what cert_id is (i.e. is it the TLV, is it the contents of
> the sequence, or something else).
> 
> Lastly it is not clear that cert_id has not been validated.
> 
> My suggestion is to keep the stuff that has not been validated yet as a
> der::Input, and call this out in the documentation.

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.h...
net/cert/ocsp_parser.h:57: struct OCSPResponseData {
On 2016/02/03 22:54:45, eroman wrote:
> Please provide documentation for these structures that reference the RFC where
> these are defined, and/or the mapping between these types and those in the RFC
> (most of which are implied by the name).

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.h...
net/cert/ocsp_parser.h:108: // updated during parsing, whereas others may not
have been changed.
On 2016/02/03 22:54:45, eroman wrote:
> Worth mentioning that |out| references data from |raw_tlv|, so it must remain
> alive.

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser.h...
net/cert/ocsp_parser.h:130: NET_EXPORT_PRIVATE bool
VerifyOCSPResponse(OCSPResponse* response,
On 2016/02/03 22:54:45, eroman wrote:
> Why are these non-const pointers? Are they expected to be mutated by the
> invocation?

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser_u...
File net/cert/ocsp_parser_unittest.cc (right):

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser_u...
net/cert/ocsp_parser_unittest.cc:25: TEST_F(OCSPParserTest, GoodOCSPResponse) {
On 2016/02/03 22:54:46, eroman wrote:
> Move this data to .pem data files instead, and put in in net/data.
> 
> See the various examples in for instance net/data/parse_certificate_unittest

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/cert/ocsp_parser_u...
net/cert/ocsp_parser_unittest.cc:236: 0x30, 0x64, 0x0a, 0x01, 0x00, 0xa0, 0x5f,
0x30, 0x5d, 0x06, 0x09, 0x2b,
On 2016/02/03 22:54:45, eroman wrote:
> Same here.

Done.

https://codereview.chromium.org/1541213002/diff/260001/net/der/input.h
File net/der/input.h (right):

https://codereview.chromium.org/1541213002/diff/260001/net/der/input.h#newcode58
net/der/input.h:58: bool Equals(const Input& other) const;
On 2016/02/03 22:54:46, eroman wrote:
> Should delete Equals if introducing operator== so there is just 1 way to do
it.

Splitting out into separate CL.

[svaldez, 2016-02-09 21:01:37]

Added a bunch of tests and fixed resulting bugs.

[eroman, 2016-02-10 20:37:15]

thanks, will give it another review pass this afternoon ...

[eroman, 2016-02-13 00:56:51]

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
File net/cert/internal/parse_ocsp.cc (right):

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:22: bool ParseOCSPStatus(der::Parser* parser,
OCSPCertStatus* out) {
I would find it useful when reading this to duplicate the ASN.1 documentations
here, or inline. I realize it is duplicated with the header, but der/asn.1
parsing is incredibly subtle when written manually.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:40: &(out->has_reason))) {
How about initializing has_reason to false at the start of this function, to
catch potential abuses of the API.

The header documentation should also call out which fields are valid to access,
since they depend on the Status and are otherwise un-initialized.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:46: return true;
From a future-proofing perspective, earlier returning success scares me.

This looks correct, but I would prefer if the parsing of RevokedInfo was in its
own function, so it is more clear what this is skipping past.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:49: if (!reason_parser.ReadTag(der::kEnumerated,
&revocation_reason))
Why is this der::kEnumerated?

I assumed the definition for CRLReason comes from RFC 5912:

   CRLReason ::= INTEGER

In which case shouldn't the tag be 0x02.

Please provide the definition and reference for what you are using.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:55:
(uint8_t)OCSPCertStatus::RevocationReason::REVOCATION_REASON_MAX) {
static_cast<>

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:59:
static_cast<OCSPCertStatus::RevocationReason>(revocation_reason_value);
I don't see that reason_parser.HasMore() is checked anywhere.
I believe unconsumed data in here should not be permitted.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:61: out->status =
OCSPCertStatus::Status::UNKNOWN;
For sanity it would be a good idea to reset all the unused fields for these
other cases.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:78: OCSPResponseData::ResponderID responder_id;
Why this temporary rather than writing directly to |out| ?

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:93: memcpy(key_hash.data,
responder_key.UnsafeData(), base::kSHA1Length);
can you use sizeof(key_hash.data) instead of base::kSHA1Length ?

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:125: if (parser->HasMore())
I think this would be more appropriate in the caller.

Based on the streaming API for this (consumes a BasicOCSPResponse from
|parser|), checking for remaining data seems like the responsibility of the
caller (ParseOCSPResponse) instead.

(If this function took a der::Input instead then it would make sense to verify
everything had been consumed).

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:129: return true;
Same comment as earlier - "return true" short cuicuits scare me, as they can
easily introduce bugs when code is added to a function. I would rather structure
this as an if statement, or smaller helper functions.

In fact the code below should be re-setting out->certs.clear() before looping.
In which case a return true here would effectively skip over that and be bad

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:140: out->certs.push_back(cert_tlv);
Where is the array first initialized?

In other words, should there be a call to out->certs.clear() before entering
this loop?

That would be consistent with what you did for out->responses.clear() later in
this file, which I agree is a good approach.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:186: if (!parser.ReadTag(der::kInteger,
&(out->serial_number)))
This is more lenient then the certificate serial number parsing we do, is that
intentional?

https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/internal/...

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:243: // We ignore the restriction that the
DEFAULT Version v1 shouldn't be
Per previous comment, try to avoid "We" when possible (see style thread)

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:285: bool ParseOCSPResponse(der::Input
ocsp_response, OCSPResponse* out) {
Would be useful to see the ASN.1 structure duplicated in comments here

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:299:
(uint8_t)OCSPResponse::ResponseStatus::RESPONSE_STATUS_MAX) {
static_cast

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:305: return true;
Same comment as earlier regarding "return true;" short-circuits.

Also this doesn't seem right -- need to still validate the rest of the DER.

The RFC says:

   If the value of responseStatus is one of the error conditions, the
responseBytes
   field is not set.

So should be checking !ocsp_response_parser.HasMore() before succeeding.
Or if this is a "should" rather than a "must" condition, need to validate that
it is at least the right stuff following.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:311: if
(!ocsp_response_parser.ReadConstructed(der::ContextSpecificConstructed(0),
Is this allowed to be omitted in the case of success? (not sure what that would
mean, but based on the ASN.1 at least it is optional)

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:317: if
(!response_bytes_input_parser.ReadSequence(&response_bytes_parser))
I would find it easier to read if each sequence is broken out into its own
function (i.e. have a separate function for parsing ResponseBytes here). Yes
some will be small, but easier to identify and verify each piece. I get a bit
lost when reading a larger stream of instructions, especially:
  * the variable names for the "parser" become longer
  * harder to verify the HasMore() calls

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:323: if (response_type_oid !=
BasicOCSPResponseOid())
(Note: Not familiar enough if this is the only OID we should support)

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:325: if
(!response_bytes_parser.ReadTag(der::kOctetString, &response_string))
Please provide documentation here that the octet string is being treated as a
BasicOCSPResponse (i.e. reference the RFC, otherwise this looks a bit magical
when reading). RFC 6960 section 4.2.1 ?

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:340: bool
CheckResponder(OCSPResponseData::ResponderID id,
The non-parsing functions feel like they belong in a different CL and file.

I have not reviewed the non-parsing bits in this review pass

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
File net/cert/internal/parse_ocsp.h (right):

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:62: enum class RevocationReason {
Where is this specified?

Same comment as the other enum: When relying on explicit values of the enum
provide them in the definition rather than relying on their definition order.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:85: // "SingleResponse". The 'certID' and
'singleExtensions' fields are pointers
Would be clearer to reference them by name |cert_id_tlv|, |extensions|

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:110: // "ResponseData". The 'responses' and
'extensions' fields are pointers to the
Are these referring to the RFC 6960 names, or the code names? Because either
'responses' or 'extensions' is inconsistent in that case. If referencing
variable names surround with |bars|.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:142: // "OCSPResponse" and the corresponding
"BasicOCSPResponse". The 'data' field
'data' --> |data| ?

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:144: // alive. The 'data' field isn't verified
until it is parsed into an
same

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:167: RESPONSE_STATUS_MAX,
Several things here:
  (1) Since this relies on the enum value for de-serialization, write the values
explicitly (protects against someone deciding to re-order these, and makes it
clear the correspondence).

  (2) Use a LAST value rather than *_MAX. I generally don't like introducing
dummy enum values since it means they have to be accounted for in a switch()
statement. Whereas aliasing the same value doesn't have that issue, albeit
requires a manual update.

   LAST = UNAUTHORIZED,

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:202: NET_EXPORT_PRIVATE bool
ParseOCSPSingleResponse(der::Input raw_tlv,
Plz change to |const der::Input&| for consistency with the other callsites that
largely use const der::Input&

(Performance difference is pretty "meh", so I won't argue on those grounds.)

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:228: const OCSPResponse* response,
(1) Why pointers instead of references? What is the relevance of nullptr?
(2) I don't think this belongs in parse_ocsp.h as it is not a parsing routine
(3) Please split this off to a separate changelist. It will need tests of a
different nature, and I would prefer to review it separately.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:236: NET_EXPORT_PRIVATE bool
GetOCSPCertStatus(const OCSPResponseData* response_data,
Why are the inputs pointers rather than references? If null is being interpreted
as having special significance it should be called out in the documentation.

https://codereview.chromium.org/1541213002/diff/340001/net/data/parse_ocsp_un...
File net/data/parse_ocsp_unittest/bad_ocsp_type.pem (right):

https://codereview.chromium.org/1541213002/diff/340001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/bad_ocsp_type.pem:1: -----BEGIN OCSP RESPONSE-----
I suggest annotating these with the ASN.1 dumps of the data, so can better see
what is going on here:

https://code.google.com/p/chromium/codesearch#chromium/src/net/data/verify_si...

(I expect if you copy that file into here, and then "python
annotatioe_test_data.py" from the directory should do the trick).

Otherwise I can't provide any real review feedback for these tests as it is
fairly opaque to me.

https://codereview.chromium.org/1541213002/diff/340001/net/data/parse_ocsp_un...
File net/data/parse_ocsp_unittest/malformed.pem (right):

https://codereview.chromium.org/1541213002/diff/340001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/malformed.pem:1: -----BEGIN OCSP RESPONSE-----
in what way is this malformed?

https://codereview.chromium.org/1541213002/diff/340001/net/data/parse_ocsp_un...
File net/data/parse_ocsp_unittest/no_response.pem (right):

https://codereview.chromium.org/1541213002/diff/340001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/no_response.pem:20: 
nit: These tests all have 2 lines between CA CERTIFICATE and CERTIFICATE, but 1
lines between the others.

[svaldez, 2016-02-16 17:25:12]

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
File net/cert/internal/parse_ocsp.cc (right):

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:22: bool ParseOCSPStatus(der::Parser* parser,
OCSPCertStatus* out) {
On 2016/02/13 00:56:50, eroman wrote:
> I would find it useful when reading this to duplicate the ASN.1 documentations
> here, or inline. I realize it is duplicated with the header, but der/asn.1
> parsing is incredibly subtle when written manually.

Acknowledged.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:40: &(out->has_reason))) {
On 2016/02/13 00:56:49, eroman wrote:
> How about initializing has_reason to false at the start of this function, to
> catch potential abuses of the API.
> 
> The header documentation should also call out which fields are valid to
access,
> since they depend on the Status and are otherwise un-initialized.

Done.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:46: return true;
On 2016/02/13 00:56:49, eroman wrote:
> From a future-proofing perspective, earlier returning success scares me.
> 
> This looks correct, but I would prefer if the parsing of RevokedInfo was in
its
> own function, so it is more clear what this is skipping past.

Done.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:49: if (!reason_parser.ReadTag(der::kEnumerated,
&revocation_reason))
On 2016/02/13 00:56:49, eroman wrote:
> Why is this der::kEnumerated?
> 
> I assumed the definition for CRLReason comes from RFC 5912:
> 
>    CRLReason ::= INTEGER
> 
> In which case shouldn't the tag be 0x02.
> 
> Please provide the definition and reference for what you are using.

CRLReason is defined as an ENUMERATED in RFC5912 (In the appendix) and
PKIX1Implicit-2009. There is errata for the section 4 definition.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:55:
(uint8_t)OCSPCertStatus::RevocationReason::REVOCATION_REASON_MAX) {
On 2016/02/13 00:56:49, eroman wrote:
> static_cast<>

Done.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:59:
static_cast<OCSPCertStatus::RevocationReason>(revocation_reason_value);
On 2016/02/13 00:56:49, eroman wrote:
> I don't see that reason_parser.HasMore() is checked anywhere.
> I believe unconsumed data in here should not be permitted.

Done.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:61: out->status =
OCSPCertStatus::Status::UNKNOWN;
On 2016/02/13 00:56:49, eroman wrote:
> For sanity it would be a good idea to reset all the unused fields for these
> other cases.

Done.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:78: OCSPResponseData::ResponderID responder_id;
On 2016/02/13 00:56:50, eroman wrote:
> Why this temporary rather than writing directly to |out| ?

Done.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:93: memcpy(key_hash.data,
responder_key.UnsafeData(), base::kSHA1Length);
On 2016/02/13 00:56:49, eroman wrote:
> can you use sizeof(key_hash.data) instead of base::kSHA1Length ?

Done.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:125: if (parser->HasMore())
On 2016/02/13 00:56:50, eroman wrote:
> I think this would be more appropriate in the caller.
> 
> Based on the streaming API for this (consumes a BasicOCSPResponse from
> |parser|), checking for remaining data seems like the responsibility of the
> caller (ParseOCSPResponse) instead.
> 
> (If this function took a der::Input instead then it would make sense to verify
> everything had been consumed).

Done.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:129: return true;
On 2016/02/13 00:56:49, eroman wrote:
> Same comment as earlier - "return true" short cuicuits scare me, as they can
> easily introduce bugs when code is added to a function. I would rather
structure
> this as an if statement, or smaller helper functions.
> 
> In fact the code below should be re-setting out->certs.clear() before looping.
> In which case a return true here would effectively skip over that and be bad

Done.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:140: out->certs.push_back(cert_tlv);
On 2016/02/13 00:56:50, eroman wrote:
> Where is the array first initialized?
> 
> In other words, should there be a call to out->certs.clear() before entering
> this loop?
> 
> That would be consistent with what you did for out->responses.clear() later in
> this file, which I agree is a good approach.

Done.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:186: if (!parser.ReadTag(der::kInteger,
&(out->serial_number)))
On 2016/02/13 00:56:50, eroman wrote:
> This is more lenient then the certificate serial number parsing we do, is that
> intentional?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/internal/...

Done.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:243: // We ignore the restriction that the
DEFAULT Version v1 shouldn't be
On 2016/02/13 00:56:49, eroman wrote:
> Per previous comment, try to avoid "We" when possible (see style thread)

Done.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:285: bool ParseOCSPResponse(der::Input
ocsp_response, OCSPResponse* out) {
On 2016/02/13 00:56:49, eroman wrote:
> Would be useful to see the ASN.1 structure duplicated in comments here

Done.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:299:
(uint8_t)OCSPResponse::ResponseStatus::RESPONSE_STATUS_MAX) {
On 2016/02/13 00:56:49, eroman wrote:
> static_cast

Done.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:305: return true;
On 2016/02/13 00:56:50, eroman wrote:
> Same comment as earlier regarding "return true;" short-circuits.
> 
> Also this doesn't seem right -- need to still validate the rest of the DER.
> 
> The RFC says:
> 
>    If the value of responseStatus is one of the error conditions, the
> responseBytes
>    field is not set.
> 
> So should be checking !ocsp_response_parser.HasMore() before succeeding.
> Or if this is a "should" rather than a "must" condition, need to validate that
> it is at least the right stuff following.

Done.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:311: if
(!ocsp_response_parser.ReadConstructed(der::ContextSpecificConstructed(0),
On 2016/02/13 00:56:50, eroman wrote:
> Is this allowed to be omitted in the case of success? (not sure what that
would
> mean, but based on the ASN.1 at least it is optional)

responseBytes must be set if the status is SUCCESSFUL.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:317: if
(!response_bytes_input_parser.ReadSequence(&response_bytes_parser))
On 2016/02/13 00:56:49, eroman wrote:
> I would find it easier to read if each sequence is broken out into its own
> function (i.e. have a separate function for parsing ResponseBytes here). Yes
> some will be small, but easier to identify and verify each piece. I get a bit
> lost when reading a larger stream of instructions, especially:
>   * the variable names for the "parser" become longer
>   * harder to verify the HasMore() calls

Moved the sequence parsing into BasicResponse parsing which should make it more
straightforward.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:323: if (response_type_oid !=
BasicOCSPResponseOid())
On 2016/02/13 00:56:50, eroman wrote:
> (Note: Not familiar enough if this is the only OID we should support)

Its the only defined OID currently.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:325: if
(!response_bytes_parser.ReadTag(der::kOctetString, &response_string))
On 2016/02/13 00:56:50, eroman wrote:
> Please provide documentation here that the octet string is being treated as a
> BasicOCSPResponse (i.e. reference the RFC, otherwise this looks a bit magical
> when reading). RFC 6960 section 4.2.1 ?

Done.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:340: bool
CheckResponder(OCSPResponseData::ResponderID id,
On 2016/02/13 00:56:50, eroman wrote:
> The non-parsing functions feel like they belong in a different CL and file.
> 
> I have not reviewed the non-parsing bits in this review pass

I'll split out the non-parsing bits after finishing running the parser/verifier
against all the Alexa sites.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
File net/cert/internal/parse_ocsp.h (right):

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:62: enum class RevocationReason {
On 2016/02/13 00:56:50, eroman wrote:
> Where is this specified?
> 
> Same comment as the other enum: When relying on explicit values of the enum
> provide them in the definition rather than relying on their definition order.

Done.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:85: // "SingleResponse". The 'certID' and
'singleExtensions' fields are pointers
On 2016/02/13 00:56:50, eroman wrote:
> Would be clearer to reference them by name |cert_id_tlv|, |extensions|

Done.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:110: // "ResponseData". The 'responses' and
'extensions' fields are pointers to the
On 2016/02/13 00:56:50, eroman wrote:
> Are these referring to the RFC 6960 names, or the code names? Because either
> 'responses' or 'extensions' is inconsistent in that case. If referencing
> variable names surround with |bars|.

Done.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:142: // "OCSPResponse" and the corresponding
"BasicOCSPResponse". The 'data' field
On 2016/02/13 00:56:50, eroman wrote:
> 'data' --> |data| ?

Done.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:144: // alive. The 'data' field isn't verified
until it is parsed into an
On 2016/02/13 00:56:50, eroman wrote:
> same

Done.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:167: RESPONSE_STATUS_MAX,
On 2016/02/13 00:56:50, eroman wrote:
> Several things here:
>   (1) Since this relies on the enum value for de-serialization, write the
values
> explicitly (protects against someone deciding to re-order these, and makes it
> clear the correspondence).
> 
>   (2) Use a LAST value rather than *_MAX. I generally don't like introducing
> dummy enum values since it means they have to be accounted for in a switch()
> statement. Whereas aliasing the same value doesn't have that issue, albeit
> requires a manual update.
> 
>    LAST = UNAUTHORIZED,

Done.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:202: NET_EXPORT_PRIVATE bool
ParseOCSPSingleResponse(der::Input raw_tlv,
On 2016/02/13 00:56:50, eroman wrote:
> Plz change to |const der::Input&| for consistency with the other callsites
that
> largely use const der::Input&
> 
> (Performance difference is pretty "meh", so I won't argue on those grounds.)

Done.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:228: const OCSPResponse* response,
On 2016/02/13 00:56:50, eroman wrote:
> (1) Why pointers instead of references? What is the relevance of nullptr?
> (2) I don't think this belongs in parse_ocsp.h as it is not a parsing routine
> (3) Please split this off to a separate changelist. It will need tests of a
> different nature, and I would prefer to review it separately.

I'll split it off to a separate CL once I'm done running/fixing all the Alexa
sites through the parser. I'm not a fan of having both a parse_ocsp and
verify_ocsp file, since users should never be able to do one without the other,
and parsing will still need an issuer and certificate to be provided, in order
to parse out the correct OCSP responses. Would it make more sense to rename the
files to be ocsp_response and then just have all the OCSP related code in there?

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:236: NET_EXPORT_PRIVATE bool
GetOCSPCertStatus(const OCSPResponseData* response_data,
On 2016/02/13 00:56:50, eroman wrote:
> Why are the inputs pointers rather than references? If null is being
interpreted
> as having special significance it should be called out in the documentation.

Done.

https://codereview.chromium.org/1541213002/diff/340001/net/data/parse_ocsp_un...
File net/data/parse_ocsp_unittest/bad_ocsp_type.pem (right):

https://codereview.chromium.org/1541213002/diff/340001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/bad_ocsp_type.pem:1: -----BEGIN OCSP RESPONSE-----
On 2016/02/13 00:56:50, eroman wrote:
> I suggest annotating these with the ASN.1 dumps of the data, so can better see
> what is going on here:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/data/verify_si...
> 
> (I expect if you copy that file into here, and then "python
> annotatioe_test_data.py" from the directory should do the trick).
> 
> Otherwise I can't provide any real review feedback for these tests as it is
> fairly opaque to me.

Done.

https://codereview.chromium.org/1541213002/diff/340001/net/data/parse_ocsp_un...
File net/data/parse_ocsp_unittest/malformed.pem (right):

https://codereview.chromium.org/1541213002/diff/340001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/malformed.pem:1: -----BEGIN OCSP RESPONSE-----
On 2016/02/13 00:56:50, eroman wrote:
> in what way is this malformed?

Done.

https://codereview.chromium.org/1541213002/diff/340001/net/data/parse_ocsp_un...
File net/data/parse_ocsp_unittest/no_response.pem (right):

https://codereview.chromium.org/1541213002/diff/340001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/no_response.pem:20: 
On 2016/02/13 00:56:50, eroman wrote:
> nit: These tests all have 2 lines between CA CERTIFICATE and CERTIFICATE, but
1
> lines between the others.

Done.

[eroman, 2016-02-16 23:42:26]

the parsing code looks good.

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
File net/cert/internal/parse_ocsp.h (right):

https://codereview.chromium.org/1541213002/diff/340001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:228: const OCSPResponse* response,
On 2016/02/16 17:25:11, svaldez wrote:
> On 2016/02/13 00:56:50, eroman wrote:
> > (1) Why pointers instead of references? What is the relevance of nullptr?
> > (2) I don't think this belongs in parse_ocsp.h as it is not a parsing
routine
> > (3) Please split this off to a separate changelist. It will need tests of a
> > different nature, and I would prefer to review it separately.
> 
> I'll split it off to a separate CL once I'm done running/fixing all the Alexa
> sites through the parser. I'm not a fan of having both a parse_ocsp and
> verify_ocsp file, since users should never be able to do one without the
other,
> and parsing will still need an issuer and certificate to be provided, in order
> to parse out the correct OCSP responses. Would it make more sense to rename
the
> files to be ocsp_response and then just have all the OCSP related code in
there?

I think there can be a logical separation between parsing and verification. But
we can discuss that more on the CL that adds the verification. I am fine with
changing the filenames in a later step.

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
File net/cert/internal/parse_ocsp.cc (right):

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:70: if (out->serial_number.Length() == 21 &&
Any plan to re-use VerifySerialNumber?
Also any deviation from the spec needs to be documented and justified. 21 byte
serial numbers are invalid. There are certificates which violate this, but it is
important to note that this is not spec compliant.

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:80: namespace {
nit: Unnamed namespaces are typically put at the start of the file in our code.

This isn't required by the style guide AFAIK, but is consistent with other stuff
in //net

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:133: bool ParseCertStatus(const der::Input&
raw_tlv, OCSPCertStatus* out) {
Thanks, this is easier to follow now IMO

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:205: namespace {
[optional] Same comment as previously. I think it is more typical to have all
the file-static functions in a single unnamed namespace at the top. I will defer
to your judgement if you think it is more valuable to keep it closer to where it
is used..

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:270: // ignored since many implementations
include them.
Can you include a TODO or some other way of finding this? (I would like to
annotate all of the places where we are not strict with warnings, once we have
error objects being passed to these functions).

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:351: out->certs.clear();
I suggest doing this unconditionally outside of "if (out->has_certs)"

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:431: bool VerifyHash(HashValueTag type,
Are you going to remove the other stuff from this CL, or do I need to review it?

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:469: default:
Please remove the default case, and instead explicitly enumerate the unused
values.

This has the advantage of causing a compile fail whenever new values are added,
forcing consideration to be made for call-sites.

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:492: return id.serial_number == serial_number;
Why this particular order of checks?

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
File net/cert/internal/parse_ocsp.h (right):

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:64: enum class RevocationReason {
Please provide a reference to the RFC and section this comes from. And also
indicate that these enum numbers correspond with the RFC.

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:138: uint64_t version;
Why change this to a uint64_t?

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:152: // if |has_certs| is true.
Thanks, this extra comment helps.

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:171: enum class ResponseStatus {
Please indicate that these numbers correspond with the RFC

https://codereview.chromium.org/1541213002/diff/400001/net/data/parse_ocsp_un...
File net/data/parse_ocsp_unittest/annotate_test_data.py (right):

https://codereview.chromium.org/1541213002/diff/400001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/annotate_test_data.py:10: block sections (except
for DATA), and add that output to the comment.
This comment is not correct anymore.

https://codereview.chromium.org/1541213002/diff/400001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/annotate_test_data.py:23: """Returns a transformed
(formatted) version of file_data"""
Should probably have a TODO somewhere about not duplicating this file, and
instead sharing stuff.

https://codereview.chromium.org/1541213002/diff/400001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/annotate_test_data.py:41: if block.name != 'DATA':
Is this relevant?

https://codereview.chromium.org/1541213002/diff/400001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/annotate_test_data.py:61: if block_name == 'OCSP
RESPONSE':
Add a comment explaining this.

https://codereview.chromium.org/1541213002/diff/400001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/annotate_test_data.py:63: (generated_comment,
response) = generated_comment.split('[HEX DUMP]:', 1)
I recall encountering a bug with OpenSSL's hex dumping where it would not escape
0x0A and instead just print a newline. I don't recall the specifics though, but
I do remember I changed some test data to avoid that sequence.

https://codereview.chromium.org/1541213002/diff/400001/net/data/parse_ocsp_un...
File net/data/parse_ocsp_unittest/bad_ocsp_type.pem (right):

https://codereview.chromium.org/1541213002/diff/400001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/bad_ocsp_type.pem:1: $ openssl asn1parse -i < [OCSP
RESPONSE]
Comments in these files explaining the failures would be helpful.

[svaldez, 2016-02-17 16:46:47]

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
File net/cert/internal/parse_ocsp.cc (right):

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:70: if (out->serial_number.Length() == 21 &&
On 2016/02/16 23:42:26, eroman wrote:
> Any plan to re-use VerifySerialNumber?
> Also any deviation from the spec needs to be documented and justified. 21 byte
> serial numbers are invalid. There are certificates which violate this, but it
is
> important to note that this is not spec compliant.

Fixing to use upstream CL changes.

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:80: namespace {
On 2016/02/16 23:42:25, eroman wrote:
> nit: Unnamed namespaces are typically put at the start of the file in our
code.
> 
> This isn't required by the style guide AFAIK, but is consistent with other
stuff
> in //net

I think its slightly more straightforward to keep the functions for parsing
subsections of the different structures close to the code that parses the larger
structs, but can move it if that makes more sense.

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:133: bool ParseCertStatus(const der::Input&
raw_tlv, OCSPCertStatus* out) {
On 2016/02/16 23:42:26, eroman wrote:
> Thanks, this is easier to follow now IMO

Acknowledged.

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:205: namespace {
On 2016/02/16 23:42:26, eroman wrote:
> [optional] Same comment as previously. I think it is more typical to have all
> the file-static functions in a single unnamed namespace at the top. I will
defer
> to your judgement if you think it is more valuable to keep it closer to where
it
> is used..

See above.

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:270: // ignored since many implementations
include them.
On 2016/02/16 23:42:26, eroman wrote:
> Can you include a TODO or some other way of finding this? (I would like to
> annotate all of the places where we are not strict with warnings, once we have
> error objects being passed to these functions).

I no longer believe this restriction to be true, from reading the ITU specs.

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:351: out->certs.clear();
On 2016/02/16 23:42:26, eroman wrote:
> I suggest doing this unconditionally outside of "if (out->has_certs)"

Done.

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:431: bool VerifyHash(HashValueTag type,
On 2016/02/16 23:42:25, eroman wrote:
> Are you going to remove the other stuff from this CL, or do I need to review
it?

I think it probably makes more sense to keep these bits in this CL, since this
is just code for parsing the SingleResponse and finding the correct one based on
the certificate details.

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:469: default:
On 2016/02/16 23:42:25, eroman wrote:
> Please remove the default case, and instead explicitly enumerate the unused
> values.
> 
> This has the advantage of causing a compile fail whenever new values are
added,
> forcing consideration to be made for call-sites.

Done.

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:492: return id.serial_number == serial_number;
On 2016/02/16 23:42:26, eroman wrote:
> Why this particular order of checks?

Doing it in the order of the CertID struct.

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
File net/cert/internal/parse_ocsp.h (right):

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:64: enum class RevocationReason {
On 2016/02/16 23:42:26, eroman wrote:
> Please provide a reference to the RFC and section this comes from. And also
> indicate that these enum numbers correspond with the RFC.

Done.

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:138: uint64_t version;
On 2016/02/16 23:42:26, eroman wrote:
> Why change this to a uint64_t?

It makes the code simpler, but adding a ReadUint8 to the net/der/ parser
instead.

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:152: // if |has_certs| is true.
On 2016/02/16 23:42:26, eroman wrote:
> Thanks, this extra comment helps.

Acknowledged.

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:171: enum class ResponseStatus {
On 2016/02/16 23:42:26, eroman wrote:
> Please indicate that these numbers correspond with the RFC

Done.

https://codereview.chromium.org/1541213002/diff/400001/net/data/parse_ocsp_un...
File net/data/parse_ocsp_unittest/annotate_test_data.py (right):

https://codereview.chromium.org/1541213002/diff/400001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/annotate_test_data.py:10: block sections (except
for DATA), and add that output to the comment.
On 2016/02/16 23:42:26, eroman wrote:
> This comment is not correct anymore.

Done.

https://codereview.chromium.org/1541213002/diff/400001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/annotate_test_data.py:23: """Returns a transformed
(formatted) version of file_data"""
On 2016/02/16 23:42:26, eroman wrote:
> Should probably have a TODO somewhere about not duplicating this file, and
> instead sharing stuff.

Done.

https://codereview.chromium.org/1541213002/diff/400001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/annotate_test_data.py:41: if block.name != 'DATA':
On 2016/02/16 23:42:26, eroman wrote:
> Is this relevant?

Done.

https://codereview.chromium.org/1541213002/diff/400001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/annotate_test_data.py:61: if block_name == 'OCSP
RESPONSE':
On 2016/02/16 23:42:26, eroman wrote:
> Add a comment explaining this.

Done.

https://codereview.chromium.org/1541213002/diff/400001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/annotate_test_data.py:63: (generated_comment,
response) = generated_comment.split('[HEX DUMP]:', 1)
On 2016/02/16 23:42:26, eroman wrote:
> I recall encountering a bug with OpenSSL's hex dumping where it would not
escape
> 0x0A and instead just print a newline. I don't recall the specifics though,
but
> I do remember I changed some test data to avoid that sequence.

Acknowledged.

https://codereview.chromium.org/1541213002/diff/400001/net/data/parse_ocsp_un...
File net/data/parse_ocsp_unittest/bad_ocsp_type.pem (right):

https://codereview.chromium.org/1541213002/diff/400001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/bad_ocsp_type.pem:1: $ openssl asn1parse -i < [OCSP
RESPONSE]
On 2016/02/16 23:42:26, eroman wrote:
> Comments in these files explaining the failures would be helpful.

Done.

[svaldez, 2016-02-17 18:12:20]

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
File net/cert/internal/parse_ocsp.cc (right):

https://codereview.chromium.org/1541213002/diff/400001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:270: // ignored since many implementations
include them.
On 2016/02/17 16:46:46, svaldez wrote:
> On 2016/02/16 23:42:26, eroman wrote:
> > Can you include a TODO or some other way of finding this? (I would like to
> > annotate all of the places where we are not strict with warnings, once we
have
> > error objects being passed to these functions).
> 
> I no longer believe this restriction to be true, from reading the ITU specs.

Followed by more reading of the ITU specs which lead me to believe my original
thoughts were right. Adding a TODO.

[eroman, 2016-02-19 02:27:34]

https://codereview.chromium.org/1541213002/diff/440001/net/cert/internal/pars...
File net/cert/internal/parse_ocsp.h (right):

https://codereview.chromium.org/1541213002/diff/440001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:91: A_COMPROMISE = 10,
why not AA_COMPROMISE? the aa i believe means "attribute authority".

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
File net/data/parse_ocsp_unittest/annotate_test_data.py (right):

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/annotate_test_data.py:5: # TODO: Deduplicate
various annotate_test_data.
TODO(svaldez)

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/annotate_test_data.py:57: # We also run 'openssl
asn1parse' against the OCTET STRING encoded
I noticed another comment that used "we". Typically best to avoid pronouns.

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
File net/data/parse_ocsp_unittest/bad_status.pem (right):

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/bad_status.pem:1: # Has an invalid status
please include in comment why it is invalid (i.e. > 10).

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
File net/data/parse_ocsp_unittest/good_response_next_update.pem (right):

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/good_response_next_update.pem:1: # Is a valid
response for the cert until nextUpdate
nit: No need for leading # in these.

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
File net/data/parse_ocsp_unittest/has_single_extension.pem (right):

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/has_single_extension.pem:1: # Has a extension in
the SingleResponse
a extension --> an extension

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
File net/data/parse_ocsp_unittest/has_version.pem (right):

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/has_version.pem:1: # Includes a default version
I don't think this description is correct.

The version number here is 1, which is NOT the default (0 is).

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
File net/data/parse_ocsp_unittest/malformed_status.pem (right):

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/malformed_status.pem:1: # Has a MALFORMED status
This reads funny. I expected this means the status is malformed (i.e. this is an
invalid input), but really it means malformedRequest enumerated value right?

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
File net/data/parse_ocsp_unittest/multiple_response.pem (right):

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/multiple_response.pem:1: # Has multiple responses
for the cert
Where are the multiple responses?

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
File net/data/parse_ocsp_unittest/ocsp_extra_certs.pem (right):

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/ocsp_extra_certs.pem:12: 8:d=3  hl=2 l=   1 prim:  
      INTEGER           :01
Am I reading it right, and all of the versions used are v2 ?

https://codereview.chromium.org/1541213002/diff/440001/net/der/parser.h
File net/der/parser.h (right):

https://codereview.chromium.org/1541213002/diff/440001/net/der/parser.h#newco...
net/der/parser.h:153: // outside the range of an int8_t.
uint8_t right?

[svaldez, 2016-02-19 15:13:55]

https://codereview.chromium.org/1541213002/diff/440001/net/cert/internal/pars...
File net/cert/internal/parse_ocsp.h (right):

https://codereview.chromium.org/1541213002/diff/440001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.h:91: A_COMPROMISE = 10,
On 2016/02/19 02:27:33, eroman wrote:
> why not AA_COMPROMISE? the aa i believe means "attribute authority".

Done.

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
File net/data/parse_ocsp_unittest/annotate_test_data.py (right):

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/annotate_test_data.py:5: # TODO: Deduplicate
various annotate_test_data.
On 2016/02/19 02:27:33, eroman wrote:
> TODO(svaldez)

Done.

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/annotate_test_data.py:57: # We also run 'openssl
asn1parse' against the OCTET STRING encoded
On 2016/02/19 02:27:33, eroman wrote:
> I noticed another comment that used "we". Typically best to avoid pronouns.

Done.

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
File net/data/parse_ocsp_unittest/bad_status.pem (right):

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/bad_status.pem:1: # Has an invalid status
On 2016/02/19 02:27:34, eroman wrote:
> please include in comment why it is invalid (i.e. > 10).

Done.

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
File net/data/parse_ocsp_unittest/good_response_next_update.pem (right):

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/good_response_next_update.pem:1: # Is a valid
response for the cert until nextUpdate
On 2016/02/19 02:27:34, eroman wrote:
> nit: No need for leading # in these.

Done.

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
File net/data/parse_ocsp_unittest/has_single_extension.pem (right):

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/has_single_extension.pem:1: # Has a extension in
the SingleResponse
On 2016/02/19 02:27:34, eroman wrote:
> a extension --> an extension

Done.

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
File net/data/parse_ocsp_unittest/has_version.pem (right):

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/has_version.pem:1: # Includes a default version
On 2016/02/19 02:27:34, eroman wrote:
> I don't think this description is correct.
> 
> The version number here is 1, which is NOT the default (0 is).

Done.

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
File net/data/parse_ocsp_unittest/malformed_status.pem (right):

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/malformed_status.pem:1: # Has a MALFORMED status
On 2016/02/19 02:27:34, eroman wrote:
> This reads funny. I expected this means the status is malformed (i.e. this is
an
> invalid input), but really it means malformedRequest enumerated value right?

Done.

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
File net/data/parse_ocsp_unittest/multiple_response.pem (right):

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/multiple_response.pem:1: # Has multiple responses
for the cert
On 2016/02/19 02:27:34, eroman wrote:
> Where are the multiple responses?

Line 21 and 30 start two responses.

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
File net/data/parse_ocsp_unittest/ocsp_extra_certs.pem (right):

https://codereview.chromium.org/1541213002/diff/440001/net/data/parse_ocsp_un...
net/data/parse_ocsp_unittest/ocsp_extra_certs.pem:12: 8:d=3  hl=2 l=   1 prim:  
      INTEGER           :01
On 2016/02/19 02:27:34, eroman wrote:
> Am I reading it right, and all of the versions used are v2 ?

Acknowledged.

https://codereview.chromium.org/1541213002/diff/440001/net/der/parser.h
File net/der/parser.h (right):

https://codereview.chromium.org/1541213002/diff/440001/net/der/parser.h#newco...
net/der/parser.h:153: // outside the range of an int8_t.
On 2016/02/19 02:27:34, eroman wrote:
> uint8_t right?

Done.

[eroman, 2016-02-23 22:29:03]

https://codereview.chromium.org/1541213002/diff/460001/net/cert/internal/pars...
File net/cert/internal/parse_ocsp.cc (right):

https://codereview.chromium.org/1541213002/diff/460001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:266: if
(!version_parser.ReadUint8(&(out->version)))
Where is the version number being checked?
RFC 6960 only defines v1(0), and that is the version for protocol/structures it
describes.
Theoretically the interpretation of everything else could change dramatically
based on the version number.

Rather than leave this up to the caller can it be checked here?

What about the existing OCSP responders, are they buggy and returning the wrong
version? (For X.509 certificates for instance there are some which return the
wrong value because they did 1-based indexing instead of 0-based).

https://codereview.chromium.org/1541213002/diff/460001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:519: // Verify Code Below (MOVING TO SEPARATE
CL)
What is the resolution to this comment?

[svaldez, 2016-02-24 16:44:38]

https://codereview.chromium.org/1541213002/diff/460001/net/cert/internal/pars...
File net/cert/internal/parse_ocsp.cc (right):

https://codereview.chromium.org/1541213002/diff/460001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:266: if
(!version_parser.ReadUint8(&(out->version)))
On 2016/02/23 22:29:03, eroman wrote:
> Where is the version number being checked?
> RFC 6960 only defines v1(0), and that is the version for protocol/structures
it
> describes.
> Theoretically the interpretation of everything else could change dramatically
> based on the version number.
> 
> Rather than leave this up to the caller can it be checked here?
> 
> What about the existing OCSP responders, are they buggy and returning the
wrong
> version? (For X.509 certificates for instance there are some which return the
> wrong value because they did 1-based indexing instead of 0-based).

Adding a check. It looks like we don't have any cases of invalid version in the
corpus of OCSP responses I'm using.

https://codereview.chromium.org/1541213002/diff/460001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:519: // Verify Code Below (MOVING TO SEPARATE
CL)
On 2016/02/23 22:29:03, eroman wrote:
> What is the resolution to this comment?

Will end up removing right before committing, since I need to keep re-adding
this code everytime I run metrics.

[eroman, 2016-02-25 00:15:59]

https://codereview.chromium.org/1541213002/diff/460001/net/cert/internal/pars...
File net/cert/internal/parse_ocsp.cc (right):

https://codereview.chromium.org/1541213002/diff/460001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:519: // Verify Code Below (MOVING TO SEPARATE
CL)
On 2016/02/24 16:44:37, svaldez wrote:
> On 2016/02/23 22:29:03, eroman wrote:
> > What is the resolution to this comment?
> 
> Will end up removing right before committing, since I need to keep re-adding
> this code everytime I run metrics.

This is easily managed through git.
Create a new branch that has this code, and whose upstream is this current CL.
For instance:

(1) Delete the stuff that is not for inclusion in this CL.
$ git checkout thisCL
...

(2) Commit the change that removes the stuff.
$ git commit -am 'Remove stuff'

(3) Create a new branch "newB" for the new stuff
$ git new-branch --upstream_current newB

(4) Patch back in the extra stuff
$ git revert HEAD

Then you run the tests by compiling this branch, and upload a clean CL in this
one.

[svaldez, 2016-02-25 16:03:45]

On 2016/02/25 00:15:59, eroman wrote:
>
https://codereview.chromium.org/1541213002/diff/460001/net/cert/internal/pars...
> File net/cert/internal/parse_ocsp.cc (right):
> 
>
https://codereview.chromium.org/1541213002/diff/460001/net/cert/internal/pars...
> net/cert/internal/parse_ocsp.cc:519: // Verify Code Below (MOVING TO SEPARATE
> CL)
> On 2016/02/24 16:44:37, svaldez wrote:
> > On 2016/02/23 22:29:03, eroman wrote:
> > > What is the resolution to this comment?
> > 
> > Will end up removing right before committing, since I need to keep re-adding
> > this code everytime I run metrics.
> 
> This is easily managed through git.
> Create a new branch that has this code, and whose upstream is this current CL.
> For instance:
> 
> (1) Delete the stuff that is not for inclusion in this CL.
> $ git checkout thisCL
> ...
> 
> (2) Commit the change that removes the stuff.
> $ git commit -am 'Remove stuff'
> 
> (3) Create a new branch "newB" for the new stuff
> $ git new-branch --upstream_current newB
> 
> (4) Patch back in the extra stuff
> $ git revert HEAD
> 
> Then you run the tests by compiling this branch, and upload a clean CL in this
> one.

Making a new branch is difficult since I have a stack of other branches off of
this one, but I've removed the code for verification from here.

[eroman, 2016-03-04 02:23:36]

https://codereview.chromium.org/1541213002/diff/500001/net/cert/internal/pars...
File net/cert/internal/parse_ocsp.cc (right):

https://codereview.chromium.org/1541213002/diff/500001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:445: // issuer |issuer| and serial number
|serial_number|.
This would be clearer if it spelled out what it checks for issuer (the name and
key tlv). But it doesn't verify the signature.

https://codereview.chromium.org/1541213002/diff/500001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:463: NOTIMPLEMENTED();
Is this a follow-up change?

https://codereview.chromium.org/1541213002/diff/500001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:467: if (!VerifyHash(type, id.issuer_name_hash,
issuer.subject_tlv))
I don't think this is correct. Can you provide a reference to the RFC?

From my reading:

  The hash shall be calculated over the DER encoding of the
  issuer's name field in the certificate being checked.

So then this should be using a hash over TBSCertificate.issuer, not the issuer's
TBSCertificate.subject.

Although they need to "match" they needn't be byte-for-byte identical, so I
don't think this is equivalent.

https://codereview.chromium.org/1541213002/diff/500001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:470: der::Parser outer_parser(issuer.spki_tlv);
Please provide a reference for the ASN.1 that it is parsing here, and the
intent.

https://codereview.chromium.org/1541213002/diff/500001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:503: for (const auto& response :
response_data.responses) {
What about possibility of duplicate responses. What if they conflict?

https://codereview.chromium.org/1541213002/diff/500001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:515: return true;
Why return true (and status unknown) if there is no response for |cert|?

Presumably this is a response for a request that included |cert|, in which case
isn't it an error for the responder not to not include a response for it? (i.e.
shouldn't it just include status unknown).

[svaldez, 2016-03-04 15:51:07]

https://codereview.chromium.org/1541213002/diff/500001/net/cert/internal/pars...
File net/cert/internal/parse_ocsp.cc (right):

https://codereview.chromium.org/1541213002/diff/500001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:445: // issuer |issuer| and serial number
|serial_number|.
On 2016/03/04 02:23:35, eroman wrote:
> This would be clearer if it spelled out what it checks for issuer (the name
and
> key tlv). But it doesn't verify the signature.

Done.

https://codereview.chromium.org/1541213002/diff/500001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:463: NOTIMPLEMENTED();
On 2016/03/04 02:23:35, eroman wrote:
> Is this a follow-up change?

Not short-term. The spec only requires Sha1/256 support, and we don't have a
nice way of doing 384/512.

https://codereview.chromium.org/1541213002/diff/500001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:467: if (!VerifyHash(type, id.issuer_name_hash,
issuer.subject_tlv))
On 2016/03/04 02:23:35, eroman wrote:
> I don't think this is correct. Can you provide a reference to the RFC?
> 
> From my reading:
> 
>   The hash shall be calculated over the DER encoding of the
>   issuer's name field in the certificate being checked.
> 
> So then this should be using a hash over TBSCertificate.issuer, not the
issuer's
> TBSCertificate.subject.
> 
> Although they need to "match" they needn't be byte-for-byte identical, so I
> don't think this is equivalent.

Done.

https://codereview.chromium.org/1541213002/diff/500001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:470: der::Parser outer_parser(issuer.spki_tlv);
On 2016/03/04 02:23:35, eroman wrote:
> Please provide a reference for the ASN.1 that it is parsing here, and the
> intent.

Done.

https://codereview.chromium.org/1541213002/diff/500001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:503: for (const auto& response :
response_data.responses) {
On 2016/03/04 02:23:35, eroman wrote:
> What about possibility of duplicate responses. What if they conflict?

That's why this code doesn't exit early. If we get duplicate GOOD responses,
then we return GOOD. If we ever get a single bad response, then we return a BAD
status.

https://codereview.chromium.org/1541213002/diff/500001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:515: return true;
On 2016/03/04 02:23:35, eroman wrote:
> Why return true (and status unknown) if there is no response for |cert|?
> 
> Presumably this is a response for a request that included |cert|, in which
case
> isn't it an error for the responder not to not include a response for it?
(i.e.
> shouldn't it just include status unknown).

Done.

[eroman, 2016-03-04 17:49:00]

https://codereview.chromium.org/1541213002/diff/500001/net/cert/internal/pars...
File net/cert/internal/parse_ocsp.cc (right):

https://codereview.chromium.org/1541213002/diff/500001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:503: for (const auto& response :
response_data.responses) {
On 2016/03/04 15:51:06, svaldez wrote:
> On 2016/03/04 02:23:35, eroman wrote:
> > What about possibility of duplicate responses. What if they conflict?
> 
> That's why this code doesn't exit early. If we get duplicate GOOD responses,
> then we return GOOD. If we ever get a single bad response, then we return a
BAD
> status.

Why just pick one rather than fail? What does the specification say about this

Seems like a problem to just pick one.
What if a different implementation picked a different response (which might
contradict the one we picked), then there would be an incompatibility.

Without seeing a normative spec reference for this, it sounds like invalid
behavior.

Also with this approach, we are breaking out when the status is "unknown", and
possibly missing a response that says it is bad. In other words, if there are
two responses, one that says unknown, and the other that says bad, we may treat
it as unknown and allow it.

[svaldez, 2016-03-04 17:54:47]

On 2016/03/04 17:49:00, eroman wrote:
>
https://codereview.chromium.org/1541213002/diff/500001/net/cert/internal/pars...
> File net/cert/internal/parse_ocsp.cc (right):
> 
>
https://codereview.chromium.org/1541213002/diff/500001/net/cert/internal/pars...
> net/cert/internal/parse_ocsp.cc:503: for (const auto& response :
> response_data.responses) {
> On 2016/03/04 15:51:06, svaldez wrote:
> > On 2016/03/04 02:23:35, eroman wrote:
> > > What about possibility of duplicate responses. What if they conflict?
> > 
> > That's why this code doesn't exit early. If we get duplicate GOOD responses,
> > then we return GOOD. If we ever get a single bad response, then we return a
> BAD
> > status.
> 
> Why just pick one rather than fail? What does the specification say about this
> 
> Seems like a problem to just pick one.
> What if a different implementation picked a different response (which might
> contradict the one we picked), then there would be an incompatibility.
> 
> Without seeing a normative spec reference for this, it sounds like invalid
> behavior.
> 
> Also with this approach, we are breaking out when the status is "unknown", and
> possibly missing a response that says it is bad. In other words, if there are
> two responses, one that says unknown, and the other that says bad, we may
treat
> it as unknown and allow it.

The spec seems to indicate that while a responder SHOULD NOT have extra replies,
it is allowed to. Any consumer should probably be treating an UNKNOWN status the
same way as a REVOKED status, since an attacker should be assumed to be able to
force an UNKNOWN status at various points in the OCSP response. A simpler API
might be to return either "GOOD" or "BAD" for a certificate, but doesn't match
the OCSP status as closely.

[eroman, 2016-03-04 17:56:11]

Can you add documentation to that function, and references to the spec to
justify its behavior?

On Fri, Mar 4, 2016 at 9:54 AM, <svaldez@chromium.org> wrote:

> On 2016/03/04 17:49:00, eroman wrote:
> >
>
>
https://codereview.chromium.org/1541213002/diff/500001/net/cert/internal/pars...
> > File net/cert/internal/parse_ocsp.cc (right):
> >
> >
>
>
https://codereview.chromium.org/1541213002/diff/500001/net/cert/internal/pars...
> > net/cert/internal/parse_ocsp.cc:503: for (const auto& response :
> > response_data.responses) {
> > On 2016/03/04 15:51:06, svaldez wrote:
> > > On 2016/03/04 02:23:35, eroman wrote:
> > > > What about possibility of duplicate responses. What if they conflict?
> > >
> > > That's why this code doesn't exit early. If we get duplicate GOOD
> responses,
> > > then we return GOOD. If we ever get a single bad response, then we
> return a
> > BAD
> > > status.
> >
> > Why just pick one rather than fail? What does the specification say
> about this
> >
> > Seems like a problem to just pick one.
> > What if a different implementation picked a different response (which
> might
> > contradict the one we picked), then there would be an incompatibility.
> >
> > Without seeing a normative spec reference for this, it sounds like
> invalid
> > behavior.
> >
> > Also with this approach, we are breaking out when the status is
> "unknown", and
> > possibly missing a response that says it is bad. In other words, if
> there are
> > two responses, one that says unknown, and the other that says bad, we may
> treat
> > it as unknown and allow it.
>
> The spec seems to indicate that while a responder SHOULD NOT have extra
> replies,
> it is allowed to. Any consumer should probably be treating an UNKNOWN
> status the
> same way as a REVOKED status, since an attacker should be assumed to be
> able to
> force an UNKNOWN status at various points in the OCSP response. A simpler
> API
> might be to return either "GOOD" or "BAD" for a certificate, but doesn't
> match
> the OCSP status as closely.
>
>
>
> https://codereview.chromium.org/1541213002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[svaldez, 2016-03-04 21:46:09]

Done.

[svaldez, 2016-03-22 17:14:09]

Just checking whether this was waiting on me?

[eroman, 2016-03-22 17:26:31]

Forgot about this, let me give it a final pass

[eroman, 2016-03-22 21:57:02]

lgtm

https://codereview.chromium.org/1541213002/diff/540001/net/cert/internal/pars...
File net/cert/internal/parse_ocsp.cc (right):

https://codereview.chromium.org/1541213002/diff/540001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:436: memcpy(value_hash.data(),
hash_string.data(), value_hash.size());
Could avoid a memcpy here, but not a big deal I guess.

https://codereview.chromium.org/1541213002/diff/540001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:517: if (out->status ==
OCSPCertStatus::Status::GOOD ||
This line deserves a comment, similar to what you described in the header.

https://codereview.chromium.org/1541213002/diff/540001/net/cert/internal/pars...
File net/cert/internal/parse_ocsp_unittest.cc (right):

https://codereview.chromium.org/1541213002/diff/540001/net/cert/internal/pars...
net/cert/internal/parse_ocsp_unittest.cc:22: void ReadOCSPFromFile(const
std::string& file_name,
This function is only used in one place, I imagine it would be simpler to inline
it there.

https://codereview.chromium.org/1541213002/diff/540001/net/cert/internal/pars...
net/cert/internal/parse_ocsp_unittest.cc:42: OCSP_SUCCESS,
nit: I suggest moving this to the top, since the rest are errors.

[svaldez, 2016-03-23 15:10:12]

https://codereview.chromium.org/1541213002/diff/540001/net/cert/internal/pars...
File net/cert/internal/parse_ocsp.cc (right):

https://codereview.chromium.org/1541213002/diff/540001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:436: memcpy(value_hash.data(),
hash_string.data(), value_hash.size());
On 2016/03/22 21:57:01, eroman wrote:
> Could avoid a memcpy here, but not a big deal I guess.

Acknowledged.

https://codereview.chromium.org/1541213002/diff/540001/net/cert/internal/pars...
net/cert/internal/parse_ocsp.cc:517: if (out->status ==
OCSPCertStatus::Status::GOOD ||
On 2016/03/22 21:57:02, eroman wrote:
> This line deserves a comment, similar to what you described in the header.

Done.

https://codereview.chromium.org/1541213002/diff/540001/net/cert/internal/pars...
File net/cert/internal/parse_ocsp_unittest.cc (right):

https://codereview.chromium.org/1541213002/diff/540001/net/cert/internal/pars...
net/cert/internal/parse_ocsp_unittest.cc:22: void ReadOCSPFromFile(const
std::string& file_name,
On 2016/03/22 21:57:02, eroman wrote:
> This function is only used in one place, I imagine it would be simpler to
inline
> it there.

Done.

https://codereview.chromium.org/1541213002/diff/540001/net/cert/internal/pars...
net/cert/internal/parse_ocsp_unittest.cc:42: OCSP_SUCCESS,
On 2016/03/22 21:57:02, eroman wrote:
> nit: I suggest moving this to the top, since the rest are errors.

Done.

[svaldez, 2016-03-24 14:25:46]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2016-03-24 14:25:47]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1541213002/#ps650001
(title: "Fixing bad path checking in compilers.")

[commit-bot: I haz the power, 2016-03-24 14:26:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1541213002/650001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1541213002/650001

[commit-bot: I haz the power, 2016-03-24 14:36:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-24 14:37:00]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[svaldez, 2016-03-24 15:29:37]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2016-03-24 15:29:38]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1541213002/#ps670001
(title: "More fixing.")

[commit-bot: I haz the power, 2016-03-24 15:29:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1541213002/670001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1541213002/670001

[commit-bot: I haz the power, 2016-03-24 15:52:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-24 15:52:35]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[svaldez, 2016-03-24 15:56:56]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2016-03-24 15:56:57]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1541213002/#ps690001
(title: "Fix more null checks.")

[commit-bot: I haz the power, 2016-03-24 15:57:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1541213002/690001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1541213002/690001

[commit-bot: I haz the power, 2016-03-24 17:16:40]

Description was changed from

==========
Addition of an OCSP parser using the net der code.

BUG=
==========

to

==========
Addition of an OCSP parser using the net der code.

BUG=
==========

[commit-bot: I haz the power, 2016-03-24 17:16:42]

Committed patchset #36 (id:690001)

[commit-bot: I haz the power, 2016-03-24 17:18:27]

Description was changed from

==========
Addition of an OCSP parser using the net der code.

BUG=
==========

to

==========
Addition of an OCSP parser using the net der code.

BUG=

Committed: https://crrev.com/be4817807690ad47baa4e87a9fb538bca04649f8
Cr-Commit-Position: refs/heads/master@{#383085}
==========

[commit-bot: I haz the power, 2016-03-24 17:18:29]

Patchset 36 (id:??) landed as
https://crrev.com/be4817807690ad47baa4e87a9fb538bca04649f8
Cr-Commit-Position: refs/heads/master@{#383085}