Adding OCSP Parser

net/cert/ocsp_parser.h

+// Copyright 2015 The Chromium Authors. All rights reserved.

[eroman, 2016/02/03 22:54:45]

can change to 2016

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

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef NET_CERT_OCSP_PARSER_H_

[eroman, 2016/02/03 22:54:45]

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

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

Done.

+#define NET_CERT_OCSP_PARSER_H_
+
+#include <string>
+#include <vector>
+
+#include "base/memory/scoped_ptr.h"
+#include "net/base/hash_value.h"
+#include "net/cert/internal/parse_certificate.h"
+#include "net/cert/internal/signature_algorithm.h"
+#include "net/der/input.h"
+#include "net/der/parse_values.h"
+#include "net/der/parser.h"
+#include "net/der/tag.h"
+
+namespace net {
+
+namespace cert {
+
+struct OCSPSingleResponse {
+  enum CertStatus {

[eroman, 2016/02/03 22:54:45]

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).

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

Done.

+    CERT_GOOD,
+    CERT_REVOKED,
+    CERT_UNKNOWN,
+  };
+
+  enum RevocationReason {
+    UNSPECIFIED,
+    KEY_COMPROMISE,
+    CA_COMPROMISE,
+    AFFILIATION_CHANGED,
+    SUPERSEDED,
+    CESSATION_OF_OPERATION,
+    CERTIFICATE_HOLD,
+    UNUSED,
+    REMOVE_FROM_CRL,
+    PRIVILEGE_WITHDRAWN,
+    A_COMPROMISE,
+  };
+
+  OCSPSingleResponse();
+  ~OCSPSingleResponse();
+
+  std::string cert_id;

[eroman, 2016/02/03 22:54:45]

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.

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

Done.

+  CertStatus cert_status;
+  der::GeneralizedTime revocation_time;
+  RevocationReason revocation_reason;
+  der::GeneralizedTime this_update;
+  der::GeneralizedTime next_update;
+  der::Input extensions;
+};
+
+struct OCSPResponseData {

[eroman, 2016/02/03 22:54:45]

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).

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

Done.

+  enum ResponderType { NAME, KEY_HASH };
+
+  struct ResponderID {
+    ResponderType type;
+    der::Input name;
+    HashValue key_hash;
+  };
+
+  OCSPResponseData();
+  ~OCSPResponseData();
+
+  uint8_t version;
+  OCSPResponseData::ResponderID responder_id;
+  der::GeneralizedTime produced_at;
+  std::vector<der::Input> responses;
+  der::Input extensions;
+};
+
+struct NET_EXPORT OCSPResponse {
+  enum ResponseStatus {
+    SUCCESSFUL,
+    MALFORMED_REQUEST,
+    INTERNAL_ERROR,
+    TRY_LATER,
+    SIG_REQUIRED,
+    UNAUTHORIZED,
+  };
+
+  OCSPResponse();
+  ~OCSPResponse();
+
+  ResponseStatus status;
+  der::Input data;
+  scoped_ptr<SignatureAlgorithm> signature_algorithm;
+  der::BitString signature;
+  std::vector<ParsedCertificate> certs;
+};
+
+// From RFC 6960:
+//
+// id-pkix-ocsp           OBJECT IDENTIFIER ::= { id-ad-ocsp }
+// id-pkix-ocsp-basic     OBJECT IDENTIFIER ::= { id-pkix-ocsp 1 }
+//
+// In dotted notation: 1.3.6.1.5.5.7.48.1.1
+NET_EXPORT der::Input BasicOCSPResponseOid();
+
+// Parses a DER-encoded OCSP "SingleResponse" as specified by RFC 6960. Returns
+// true on success and sets the results in |out|.
+//
+// On failure |out| has an undefined state. Some of its fields may have been
+// updated during parsing, whereas others may not have been changed.

[eroman, 2016/02/03 22:54:45]

Worth mentioning that |out| references data from |raw_tlv|, so it must remain
alive.

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

Done.

+NET_EXPORT_PRIVATE bool ParseOCSPSingleResponse(der::Input raw_tlv,
+                                                OCSPSingleResponse* out);
+
+// Parses a DER-encoded OCSP "ResponseData" as specified by RFC 6960. Returns
+// true on success and sets the results in |out|.
+//
+// On failure |out| has an undefined state. Some of its fields may have been
+// updated during parsing, whereas others may not have been changed.
+NET_EXPORT_PRIVATE bool ParseOCSPResponseData(der::Input raw_tlv,
+                                              OCSPResponseData* out);
+
+// Parses a DER-encoded "OCSPResponse" as specified by RFC 6960. Returns true
+// on success and sets the results in |out|.
+//
+// On failure |out| has an undefined state. Some of its fields may have been
+// updated during parsing, whereas others may not have been changed.
+NET_EXPORT_PRIVATE bool ParseOCSPResponse(der::Input ocsp_response,
+                                          OCSPResponse* out);
+
+// Verifies that the OCSP Response |response| is signed and has a valid trust
+// path to the issuer |cert|.
+NET_EXPORT_PRIVATE bool VerifyOCSPResponse(OCSPResponse* response,

[eroman, 2016/02/03 22:54:45]

Why are these non-const pointers? Are they expected to be mutated by the
invocation?

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

Done.

+                                           ParsedCertificate* cert);
+
+}  // namespace cert
+
+}  // namespace net
+
+#endif  // NET_CERT_OCSP_PARSER_H_