From 513ebc378f103057dcefdc075ee253c46d3a34d2 Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Thu, 2 Jul 2020 17:07:25 -0700 Subject: [PATCH 01/31] ocsp: add parsing utilities for ASN.1 OCSP responses Signed-off-by: Daniel Goldstein --- .../transport_sockets/tls/ocsp/BUILD | 33 ++ .../tls/ocsp/asn1_utility.cc | 138 +++++++ .../transport_sockets/tls/ocsp/asn1_utility.h | 207 +++++++++++ .../transport_sockets/tls/ocsp/ocsp.cc | 307 ++++++++++++++++ .../transport_sockets/tls/ocsp/ocsp.h | 324 +++++++++++++++++ .../transport_sockets/tls/ocsp/BUILD | 40 +++ .../tls/ocsp/asn1_utility_test.cc | 340 ++++++++++++++++++ .../tls/ocsp/gen_unittest_ocsp_data.sh | 156 ++++++++ .../transport_sockets/tls/ocsp/ocsp_test.cc | 291 +++++++++++++++ 9 files changed, 1836 insertions(+) create mode 100644 source/extensions/transport_sockets/tls/ocsp/BUILD create mode 100644 source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc create mode 100644 source/extensions/transport_sockets/tls/ocsp/asn1_utility.h create mode 100644 source/extensions/transport_sockets/tls/ocsp/ocsp.cc create mode 100644 source/extensions/transport_sockets/tls/ocsp/ocsp.h create mode 100644 test/extensions/transport_sockets/tls/ocsp/BUILD create mode 100644 test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc create mode 100755 test/extensions/transport_sockets/tls/ocsp/gen_unittest_ocsp_data.sh create mode 100644 test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc diff --git a/source/extensions/transport_sockets/tls/ocsp/BUILD b/source/extensions/transport_sockets/tls/ocsp/BUILD new file mode 100644 index 000000000000..4d2801a0be7d --- /dev/null +++ b/source/extensions/transport_sockets/tls/ocsp/BUILD @@ -0,0 +1,33 @@ +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_library", + "envoy_package", +) + +licenses(["notice"]) # Apache 2 + +envoy_package() + +envoy_cc_library( + name = "ocsp_lib", + srcs = ["ocsp.cc"], + hdrs = ["ocsp.h"], + repository = "", + deps = [ + ":asn1_utility_lib", + "//include/envoy/common:time_interface", + "//include/envoy/ssl:context_config_interface", + "//source/extensions/transport_sockets/tls:utility_lib", + ], +) + +envoy_cc_library( + name = "asn1_utility_lib", + srcs = ["asn1_utility.cc"], + hdrs = ["asn1_utility.h"], + repository = "", + deps = [ + "//include/envoy/common:time_interface", + "//include/envoy/ssl:context_config_interface", + ], +) diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc new file mode 100644 index 000000000000..fcb52b066532 --- /dev/null +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc @@ -0,0 +1,138 @@ +#include "extensions/transport_sockets/tls/ocsp/asn1_utility.h" + +#include "absl/strings/str_cat.h" + +namespace Envoy { +namespace Extensions { +namespace TransportSockets { +namespace Tls { +namespace Ocsp { + +std::string Asn1Utility::cbsToString(CBS& cbs) { + auto str_head = reinterpret_cast(CBS_data(&cbs)); + return {str_head, CBS_len(&cbs)}; +} + +bool Asn1Utility::isOptionalPresent(CBS& cbs, CBS* data, unsigned tag) { + int is_present; + if (!CBS_get_optional_asn1(&cbs, data, &is_present, tag)) { + throw Envoy::EnvoyException("Failed to parse ASN.1 element tag"); + } + + return is_present; +} + +std::string Asn1Utility::parseOid(CBS& cbs) { + CBS oid; + if (!CBS_get_asn1(&cbs, &oid, CBS_ASN1_OBJECT)) { + throw Envoy::EnvoyException("Input is not a well-formed ASN.1 OBJECT"); + } + char* oid_text = CBS_asn1_oid_to_text(&oid); + if (oid_text == nullptr) { + throw Envoy::EnvoyException("Failed to parse oid"); + } + + std::string oid_text_str(oid_text); + OPENSSL_free(oid_text); + return oid_text_str; +} + +Envoy::SystemTime Asn1Utility::parseGeneralizedTime(CBS& cbs) { + CBS elem; + if (!CBS_get_asn1(&cbs, &elem, CBS_ASN1_GENERALIZEDTIME)) { + throw Envoy::EnvoyException("Input is not a well-formed ASN.1 GENERALIZEDTIME"); + } + + auto time_str = cbsToString(elem); + // OCSP follows the RFC 5280 enforcement that GENERALIZEDTIME + // fields MUST be in UTC, so must be suffixed with a Z character. + // Local time or time differential, though a part of the ASN.1 + // GENERALIZEDTIME spec, are not supported. + // Reference: https://tools.ietf.org/html/rfc5280#section-4.1.2.5.2 + if (time_str.at(time_str.length() - 1) != 'Z') { + throw Envoy::EnvoyException("GENERALIZEDTIME must be in UTC"); + } + + absl::Time time; + std::string time_format = "%E4Y%m%d%H%M%SZ"; + std::string parse_error; + if (!absl::ParseTime(time_format, time_str, &time, &parse_error)) { + throw Envoy::EnvoyException(absl::StrCat("Error parsing timestamp ", time_str, " with format ", + time_format, ". Error: ", parse_error)); + } + return absl::ToChronoTime(time); +} + +// Performs the following conversions to go from bytestring to hex integer +// CBS -> ASN1_INTEGER -> BIGNUM -> String +std::string Asn1Utility::parseInteger(CBS& cbs) { + CBS num; + if (!CBS_get_asn1(&cbs, &num, CBS_ASN1_INTEGER)) { + throw Envoy::EnvoyException("Input is not a well-formed ASN.1 INTEGER"); + } + + auto head = CBS_data(&num); + const ASN1_INTEGER* asn1_serial_number = c2i_ASN1_INTEGER(nullptr, &head, CBS_len(&num)); + if (asn1_serial_number != nullptr) { + BIGNUM num_bn; + BN_init(&num_bn); + ASN1_INTEGER_to_BN(asn1_serial_number, &num_bn); + + char* char_serial_number = BN_bn2hex(&num_bn); + BN_free(&num_bn); + M_ASN1_INTEGER_free(asn1_serial_number); + + if (char_serial_number != nullptr) { + std::string serial_number(char_serial_number); + OPENSSL_free(char_serial_number); + return serial_number; + } + } + + throw Envoy::EnvoyException("Failed to parse ASN.1 INTEGER"); +} + +std::string Asn1Utility::parseOctetString(CBS& cbs) { + CBS value; + if (!CBS_get_asn1(&cbs, &value, CBS_ASN1_OCTETSTRING)) { + throw Envoy::EnvoyException("Input is not a well-formed ASN.1 OCTETSTRING"); + } + + return cbsToString(value); +} + +std::vector Asn1Utility::parseBitString(CBS& cbs) { + CBS value; + if (!CBS_get_asn1(&cbs, &value, CBS_ASN1_BITSTRING)) { + throw Envoy::EnvoyException("Input is not a well-formed ASN.1 BITSTRING"); + } + + auto data = reinterpret_cast(CBS_data(&value)); + return {data, data + CBS_len(&value)}; +} + +std::string Asn1Utility::parseAlgorithmIdentifier(CBS& cbs) { + // AlgorithmIdentifier ::= SEQUENCE { + // algorithm OBJECT IDENTIFIER, + // parameters ANY DEFINED BY algorithm OPTIONAL + // } + CBS elem; + if (!CBS_get_asn1(&cbs, &elem, CBS_ASN1_SEQUENCE)) { + throw Envoy::EnvoyException("AlgorithmIdentifier is not a well-formed ASN.1 SEQUENCE"); + } + + return parseOid(elem); + // ignore parameters +} + +void Asn1Utility::skipOptional(CBS& cbs, unsigned tag) { + if (!CBS_get_optional_asn1(&cbs, nullptr, nullptr, tag)) { + throw Envoy::EnvoyException("Failed to parse ASN.1 element tag"); + } +} + +} // namespace Ocsp +} // namespace Tls +} // namespace TransportSockets +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h new file mode 100644 index 000000000000..31a346d0737c --- /dev/null +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h @@ -0,0 +1,207 @@ +#pragma once + +#include +#include +#include + +#include "envoy/common/exception.h" +#include "envoy/common/time.h" + +#include "common/common/assert.h" + +#include "absl/types/optional.h" +#include "openssl/bn.h" +#include "openssl/bytestring.h" +#include "openssl/ssl.h" + +namespace Envoy { +namespace Extensions { +namespace TransportSockets { +namespace Tls { +namespace Ocsp { + +/** + * Construct a |T| from the data contained in the CBS&. Functions + * of this type must advance the input CBS& over the element. + */ +template using Asn1ParsingFunc = std::function; + +/** + * Utility functions for parsing DER-encoded ASN.1 objects. + * This relies heavily on the 'openssl/bytestring' API which + * is BoringSSL's recommended interface for parsing DER-encoded + * ASN.1 data when there is not an existing wrapper. + * This is not a complete library for ASN.1 parsing and primarily + * serves as abstractions for the OCSP module, but can be + * extended and moved into a general utility to support parsing of + * additional ASN.1 objects. + * + * Each function adheres to the invariant that given a reference + * to a crypto bytestring (CBS&), it will parse the specified + * ASN.1 element and advance |cbs| over it. + * + * An exception is thrown if the bytestring is malformed or does + * not match the specified ASN.1 object. The position + * of |cbs| is not reliable after an exception is thrown. + */ +class Asn1Utility { +public: + ~Asn1Utility() = default; + + /** + * Extracts the full contents of |cbs| as a string. + * This copies the data in |cbs|. + * + * @param cbs a CBS& that refers to the current document position + * @returns std::string containing the contents of |cbs| + */ + static std::string cbsToString(CBS& cbs); + + /** + * Parses all elements of an ASN.1 SEQUENCE OF. |parseElement| must + * advance its input CBS& over the entire element. + * + * @param cbs a CBS& that refers to an ASN.1 SEQUENCE OF object + * @param parseElement an Asn1ParsingFunc used to parse each element + * @returns std::vector containing the parsed elements of the sequence. + * @throws Envoy::EnvoyException if |cbs| does not point to a well-formed + * SEQUENCE OF object. + */ + template + static std::vector parseSequenceOf(CBS& cbs, Asn1ParsingFunc parseElement); + + /** + * Checks if an explicitly tagged optional element of |tag| is present and + * if so parses its value with |parseData|. If the element is not present, + * |cbs| is not advanced. + * + * @param cbs a CBS& that refers to the current document position + * @param parseData an Asn1ParsingFunc used to parse the data if present + * @return absl::optional with a T if |cbs| is of the specified tag, + * else nullopt + */ + template + static absl::optional parseOptional(CBS& cbs, Asn1ParsingFunc parseData, unsigned tag); + + /** + * Returns whether or not an element explicitly tagged with |tag| is present + * at |cbs|. If so, |cbs| is advanced over the optional and assigns + * |data| to the inner element, if |data| is not nullptr. + * If |cbs| does not contain |tag|, |cbs| remains at the same position. + * + * @param cbs a CBS& that refers to the current document position + * @param data a CBS& that is set to the contents of |cbs| + * @param an explicit tag indicating an optional value + * + * @returns bool whether |cbs| points to an element tagged with |tag| + * @throws Envoy::EnvoyException if |cbs| is a malformed TLV bytestring + */ + static bool isOptionalPresent(CBS& cbs, CBS* data, unsigned tag); + + /** + * @param cbs a CBS& that refers to an ASN.1 OBJECT IDENTIFIER element + * @returns std::string the OID encoded in |cbs| + * @throws Envoy::EnvoyException if |cbs| does not point to a well-formed + * OBJECT IDENTIFIER + */ + static std::string parseOid(CBS& cbs); + + /** + * @param cbs a CBS& that refers to an ASN.1 GENERALIZEDTIME element + * @returns Envoy::SystemTime the UTC timestamp encoded in |cbs| + * @throws Envoy::EnvoyException if |cbs| does not point to a well-formed + * GENERALIZEDTIME + */ + static Envoy::SystemTime parseGeneralizedTime(CBS& cbs); + + /** + * Parses an ASN.1 INTEGER type into its hex string representation. + * ASN.1 INTEGER types are arbitrary precision. + * If you're SURE the integer fits into a fixed-size int, + * use CBS_get_asn1_* functions for the given integer type instead. + * + * @param cbs a CBS& that refers to an ASN.1 INTEGER element + * @returns std::string a hex representation of the integer + * @throws Envoy::EnvoyException if |cbs| does not point to a well-formed + * INTEGER + */ + static std::string parseInteger(CBS& cbs); + + /** + * Parses an ASN.1 AlgorithmIdentifier. Currently ignores algorithm params + * and only returns the OID of the algorithm. + * + * @param cbs a CBS& that refers to an ASN.1 AlgorithmIdentifier element + * @returns std::string the OID of the algorithm + * @throws Envoy::EnvoyException if |cbs| does not point to a well-formed + * AlgorithmIdentifier + */ + static std::string parseAlgorithmIdentifier(CBS& cbs); + + /** + * @param cbs a CBS& that refers to an ASN.1 OCTETSTRING element + * @returns std::string of the octets in |cbs| + * @throws Envoy::EnvoyException if |cbs| does not point to a well-formed + * OCTETSTRING + */ + static std::string parseOctetString(CBS& cbs); + + /** + * Parses an ASN.1 BITSTRING into a byte vector. The first byte + * of the vector indicates the number of unused bits at the end of + * the last byte. The second byte up through part of the last byte + * contain the contents of the bit string. + * + * @param cbs a CBS& that refers to an ASN.1 BITSTRING element + * @returns std::vector of the bitstring packed into bytes. + * @throws Envoy::EnvoyException if |cbs| does not point to a well-formed BITSTRING + */ + static std::vector parseBitString(CBS& cbs); + + /** + * Advance |cbs| over an ASN.1 value of the class |tag| if that + * value is present. Otherwise, |cbs| stays in the same position. + * + * @param cbs a CBS& that refers to the current document position + * @param tag the tag of the value to skip + * @throws Envoy::EnvoyException if |cbs| is a malformed TLV bytestring + */ + static void skipOptional(CBS& cbs, unsigned tag); +}; + +template +std::vector Asn1Utility::parseSequenceOf(CBS& cbs, Asn1ParsingFunc parseElement) { + CBS seq_elem; + std::vector vec; + + // Initialize seq_elem to first element in sequence. + if (!CBS_get_asn1(&cbs, &seq_elem, CBS_ASN1_SEQUENCE)) { + throw Envoy::EnvoyException("Expected sequence of ASN.1 elements."); + } + + while (CBS_data(&seq_elem) < CBS_data(&cbs)) { + // parseElement MUST advance seq_elem + vec.push_back(parseElement(seq_elem)); + } + + RELEASE_ASSERT(CBS_data(&cbs) == CBS_data(&seq_elem), + "Sequence tag length must match actual length or element parsing would fail"); + + return vec; +} + +template +absl::optional Asn1Utility::parseOptional(CBS& cbs, Asn1ParsingFunc parseData, unsigned tag) { + CBS data; + if (isOptionalPresent(cbs, &data, tag)) { + return parseData(data); + } + + return absl::nullopt; +} + +} // namespace Ocsp +} // namespace Tls +} // namespace TransportSockets +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc new file mode 100644 index 000000000000..43aa82735223 --- /dev/null +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc @@ -0,0 +1,307 @@ +#include "extensions/transport_sockets/tls/ocsp/ocsp.h" + +#include "common/common/utility.h" + +#include "extensions/transport_sockets/tls/ocsp/asn1_utility.h" +#include "extensions/transport_sockets/tls/utility.h" + +namespace Envoy { +namespace Extensions { +namespace TransportSockets { +namespace Tls { +namespace Ocsp { + +namespace CertUtility = Envoy::Extensions::TransportSockets::Tls::Utility; + +static const unsigned CONS = CBS_ASN1_CONSTRUCTED; +static const unsigned CONT = CBS_ASN1_CONTEXT_SPECIFIC; + +namespace { + +unsigned parseTag(CBS& cbs) { + unsigned tag; + if (!CBS_get_any_asn1_element(&cbs, nullptr, &tag, nullptr)) { + throw EnvoyException("Failed to parse ASN.1 element tag"); + } + return tag; +} + +std::unique_ptr readDerEncodedOcspResponse(const std::string& der) { + CBS cbs; + CBS_init(&cbs, reinterpret_cast(der.c_str()), der.size()); + + auto resp = Asn1OcspUtility::parseOcspResponse(cbs); + if (CBS_len(&cbs) != 0) { + throw EnvoyException("Data contained more than a single OCSP response"); + } + + return resp; +} + +void skipResponderId(CBS& cbs) { + // ResponderID ::= CHOICE { + // byName [1] Name, + // byKey [2] KeyHash + // } + // + // KeyHash ::= OCTET STRING -- SHA-1 hash of responder's public key + // (excluding the tag and length fields) + + if (Asn1Utility::isOptionalPresent(cbs, nullptr, CONS | CONT | 1) || + Asn1Utility::isOptionalPresent(cbs, nullptr, CONS | CONT | 2)) { + return; + } + + throw EnvoyException(absl::StrCat("Unknown choice for Responder ID: ", parseTag(cbs))); +} + +} // namespace + +OcspResponse::OcspResponse(OcspResponseStatus status, ResponsePtr&& response) + : status_(status), response_(std::move(response)) {} + +BasicOcspResponse::BasicOcspResponse(ResponseData data, std::string signature_alg, + std::vector signature) + : data_(data), signature_alg_(signature_alg), signature_(std::move(signature)) {} + +const std::string BasicOcspResponse::OID = "1.3.6.1.5.5.7.48.1.1"; + +ResponseData::ResponseData(Envoy::SystemTime produced_at, + std::vector single_responses) + : produced_at_(produced_at), single_responses_(std::move(single_responses)) {} + +SingleResponse::SingleResponse(CertId cert_id, CertStatus status, Envoy::SystemTime this_update, + absl::optional next_update) + : cert_id_(cert_id), status_(status), this_update_(this_update), next_update_(next_update) {} + +CertId::CertId(std::string serial_number, std::string alg_oid, std::string issuer_name_hash, + std::string issuer_public_key_hash) + : serial_number_(serial_number), alg_oid_(alg_oid), issuer_name_hash_(issuer_name_hash), + issuer_public_key_hash_(issuer_public_key_hash) {} + +OcspResponseWrapper::OcspResponseWrapper(std::string der_response, TimeSource& time_source) + : raw_bytes_(std::move(der_response)), response_(readDerEncodedOcspResponse(raw_bytes_)), + time_source_(time_source) { + + if (response_->response_ == nullptr) { + throw EnvoyException("OCSP response has no body"); + } + + // We only permit a 1:1 of certificate to response + if (response_->response_->getNumCerts() != 1) { + throw EnvoyException("OCSP Response must be for one certificate only"); + } + + auto& this_update = response_->response_->getThisUpdate(); + if (time_source_.systemTime() < this_update) { + DateFormatter formatter("%E4Y%m%d%H%M%S"); + throw EnvoyException(absl::StrCat("OCSP Response thisUpdate field is set in the future: ", + formatter.fromTime(this_update))); + } +} + +// TODO(daniel-goldstein): This should also check the issuer +bool OcspResponseWrapper::matchesCertificate(X509& cert) { + std::string cert_serial_number = CertUtility::getSerialNumberFromCertificate(cert); + std::string resp_cert_serial_number = response_->response_->getCertSerialNumber(); + return resp_cert_serial_number == cert_serial_number; +} + +bool OcspResponseWrapper::isExpired() { + auto& next_update = response_->response_->getNextUpdate(); + return next_update == absl::nullopt || next_update < time_source_.systemTime(); +} + +std::unique_ptr Asn1OcspUtility::parseOcspResponse(CBS& cbs) { + // OCSPResponse ::= SEQUENCE { + // responseStatus OCSPResponseStatus, + // responseBytes [0] EXPLICIT ResponseBytes OPTIONAL + // } + + CBS elem; + if (!CBS_get_asn1(&cbs, &elem, CBS_ASN1_SEQUENCE)) { + throw EnvoyException("OCSP Response is not a well-formed ASN.1 SEQUENCE"); + } + + OcspResponseStatus status = Asn1OcspUtility::parseResponseStatus(elem); + + CBS bytes; + ResponsePtr resp = nullptr; + if (Asn1Utility::isOptionalPresent(elem, &bytes, CONS | CONT | 0)) { + resp = Asn1OcspUtility::parseResponseBytes(bytes); + } + + return std::make_unique(status, std::move(resp)); +} + +OcspResponseStatus Asn1OcspUtility::parseResponseStatus(CBS& cbs) { + // OCSPResponseStatus ::= ENUMERATED { + // successful (0), -- Response has valid confirmations + // malformedRequest (1), -- Illegal confirmation request + // internalError (2), -- Internal error in issuer + // tryLater (3), -- Try again later + // -- (4) is not used + // sigRequired (5), -- Must sign the request + // unauthorized (6) -- Request unauthorized + // } + CBS status; + if (!CBS_get_asn1(&cbs, &status, CBS_ASN1_ENUMERATED)) { + throw EnvoyException("OCSP ResponseStatus is not a well-formed ASN.1 ENUMERATED"); + } + + auto status_ordinal = *CBS_data(&status); + switch (status_ordinal) { + case 0: + return OcspResponseStatus::SUCCESSFUL; + case 1: + return OcspResponseStatus::MALFORMED_REQUEST; + case 2: + return OcspResponseStatus::INTERNAL_ERROR; + case 3: + return OcspResponseStatus::TRY_LATER; + case 5: + return OcspResponseStatus::SIG_REQUIRED; + case 6: + return OcspResponseStatus::UNAUTHORIZED; + default: + throw EnvoyException(absl::StrCat("Unknown OCSP Response Status variant: ", status_ordinal)); + } +} + +ResponsePtr Asn1OcspUtility::parseResponseBytes(CBS& cbs) { + // ResponseBytes ::= SEQUENCE { + // responseType RESPONSE. + // &id ({ResponseSet}), + // response OCTET STRING (CONTAINING RESPONSE. + // &Type({ResponseSet}{@responseType})) + // } + CBS elem, response; + if (!CBS_get_asn1(&cbs, &elem, CBS_ASN1_SEQUENCE)) { + throw EnvoyException("OCSP ResponseBytes is not a well-formed SEQUENCE"); + } + + auto oid_str = Asn1Utility::parseOid(elem); + if (!CBS_get_asn1(&elem, &response, CBS_ASN1_OCTETSTRING)) { + throw EnvoyException("Expected ASN.1 OCTETSTRING for response"); + } + + if (oid_str == BasicOcspResponse::OID) { + return Asn1OcspUtility::parseBasicOcspResponse(response); + } + throw EnvoyException(absl::StrCat("Unknown OCSP Response type with OID: ", oid_str)); +} + +std::unique_ptr Asn1OcspUtility::parseBasicOcspResponse(CBS& cbs) { + // BasicOCSPResponse ::= SEQUENCE { + // tbsResponseData ResponseData, + // signatureAlgorithm AlgorithmIdentifier{SIGNATURE-ALGORITHM, + // {sa-dsaWithSHA1 | sa-rsaWithSHA1 | + // sa-rsaWithMD5 | sa-rsaWithMD2, ...}}, + // signature BIT STRING, + // certs [0] EXPLICIT SEQUENCE OF Certificate OPTIONAL + // } + CBS elem; + if (!CBS_get_asn1(&cbs, &elem, CBS_ASN1_SEQUENCE)) { + throw EnvoyException("OCSP BasicOCSPResponse is not a wellf-formed ASN.1 SEQUENCE"); + } + auto response_data = Asn1OcspUtility::parseResponseData(elem); + auto signature_alg = Asn1Utility::parseAlgorithmIdentifier(elem); + auto signature = Asn1Utility::parseBitString(elem); + // TODO(daniel-goldstein): Verify this signature + // "The value for signature SHALL be computed on the hash of the DER + // encoding of ResponseData." + + // optional additional certs are ignored. + + return std::make_unique(response_data, signature_alg, std::move(signature)); +} + +ResponseData Asn1OcspUtility::parseResponseData(CBS& cbs) { + // ResponseData ::= SEQUENCE { + // version [0] EXPLICIT Version DEFAULT v1, + // responderID ResponderID, + // producedAt GeneralizedTime, + // responses SEQUENCE OF SingleResponse, + // responseExtensions [1] EXPLICIT Extensions OPTIONAL + // } + CBS elem; + if (!CBS_get_asn1(&cbs, &elem, CBS_ASN1_SEQUENCE)) { + throw EnvoyException("OCSP ResponseData is not a well-formed ASN.1 SEQUENCE"); + } + + Asn1Utility::skipOptional(elem, 0); + skipResponderId(elem); + auto produced_at = Asn1Utility::parseGeneralizedTime(elem); + auto responses = Asn1Utility::parseSequenceOf(elem, parseSingleResponse); + // Extensions currently ignored + + return {produced_at, std::move(responses)}; +} + +SingleResponse Asn1OcspUtility::parseSingleResponse(CBS& cbs) { + // SingleResponse ::= SEQUENCE { + // certID CertID, + // certStatus CertStatus, + // thisUpdate GeneralizedTime, + // nextUpdate [0] EXPLICIT GeneralizedTime OPTIONAL, + // singleExtensions [1] EXPLICIT Extensions OPTIONAL + // } + CBS elem; + if (!CBS_get_asn1(&cbs, &elem, CBS_ASN1_SEQUENCE)) { + throw EnvoyException("OCSP SingleResponse is not a well-formed ASN.1 SEQUENCE"); + } + + auto cert_id = Asn1OcspUtility::parseCertId(elem); + auto status = Asn1OcspUtility::parseCertStatus(elem); + auto this_update = Asn1Utility::parseGeneralizedTime(elem); + auto next_update = Asn1Utility::parseOptional( + elem, Asn1Utility::parseGeneralizedTime, CONS | CONT | 0); + // Extensions currently ignored + + return {cert_id, status, this_update, next_update}; +} + +CertId Asn1OcspUtility::parseCertId(CBS& cbs) { + // CertID ::= SEQUENCE { + // hashAlgorithm AlgorithmIdentifier, + // issuerNameHash OCTET STRING, -- Hash of issuer's DN + // issuerKeyHash OCTET STRING, -- Hash of issuer's public key + // serialNumber CertificateSerialNumber + // } + CBS elem; + if (!CBS_get_asn1(&cbs, &elem, CBS_ASN1_SEQUENCE)) { + throw EnvoyException("OCSP CertID is not a well-formed ASN.1 SEQUENCE"); + } + + auto alg = Asn1Utility::parseAlgorithmIdentifier(elem); + auto issuer_name_hash = Asn1Utility::parseOctetString(elem); + auto issuer_public_key_hash = Asn1Utility::parseOctetString(elem); + auto serial_number = Asn1Utility::parseInteger(elem); + + return {serial_number, alg, issuer_name_hash, issuer_public_key_hash}; +} + +CertStatus Asn1OcspUtility::parseCertStatus(CBS& cbs) { + // CertStatus ::= CHOICE { + // good [0] IMPLICIT NULL, + // revoked [1] IMPLICIT RevokedInfo, + // unknown [2] IMPLICIT UnknownInfo + // } + if (Asn1Utility::isOptionalPresent(cbs, nullptr, CONT | 0)) { + return CertStatus::GOOD; + } + if (Asn1Utility::isOptionalPresent(cbs, nullptr, CONS | CONT | 1)) { + return CertStatus::REVOKED; + } + if (Asn1Utility::isOptionalPresent(cbs, nullptr, CONT | 2)) { + return CertStatus::UNKNOWN; + } + + throw EnvoyException(absl::StrCat("Unknown OcspCertStatus tag: ", parseTag(cbs))); +} + +} // namespace Ocsp +} // namespace Tls +} // namespace TransportSockets +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.h b/source/extensions/transport_sockets/tls/ocsp/ocsp.h new file mode 100644 index 000000000000..8bbb848a42f8 --- /dev/null +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.h @@ -0,0 +1,324 @@ +#pragma once + +#include +#include +#include + +#include "envoy/common/exception.h" +#include "envoy/common/time.h" + +#include "absl/types/optional.h" +#include "openssl/bytestring.h" +#include "openssl/ssl.h" + +/** + * Data structures and functions for unmarshalling OCSP responses + * according to the RFC6960 B.2 spec. See: https://tools.ietf.org/html/rfc6960#appendix-B + */ + +namespace Envoy { +namespace Extensions { +namespace TransportSockets { +namespace Tls { +namespace Ocsp { + +/** + * Reflection of the ASN.1 OcspResponseStatus enumeration. + * The possible statuses that can accompany an OCSP response. + */ +enum class OcspResponseStatus { + // OCSPResponseStatus ::= ENUMERATED { + // successful (0), -- Response has valid confirmations + // malformedRequest (1), -- Illegal confirmation request + // internalError (2), -- Internal error in issuer + // tryLater (3), -- Try again later + // -- (4) is not used + // sigRequired (5), -- Must sign the request + // unauthorized (6) -- Request unauthorized + // } + SUCCESSFUL = 0, + MALFORMED_REQUEST = 1, + INTERNAL_ERROR = 2, + TRY_LATER = 3, + SIG_REQUIRED = 5, + UNAUTHORIZED = 6 +}; + +/** + * Reflection of the ASN.1 CertStatus enumeration. + * The status of a single SSL certificate in an OCSP response. + */ +enum class CertStatus { + // The certificate is known to be valid + GOOD, + // The certificate has been revoked + REVOKED, + // The responder has no record of the certificate and cannot confirm its validity + UNKNOWN +}; + +/** + * Reflection of the ASN.1 CertId structure. + * Contains the information to uniquely identify an SSL Certificate. + * Serial numbers are guaranteed to be + * unique per issuer but not necessarily universally. + */ +struct CertId { + CertId(std::string serial_number, std::string alg_oid, std::string issuer_name_hash, + std::string issuer_public_key_hash); + + std::string serial_number_; + std::string alg_oid_; + std::string issuer_name_hash_; + std::string issuer_public_key_hash_; +}; + +/** + * Reflection of the ASN.1 SingleResponse structure. + * Contains information about the OCSP status of a single certificate. + * An OCSP request may request the status of multiple certificates and + * therefore responses may contain multiple SingleResponses. + * + * this_update_ and next_update_ reflect the validity period for this response. + * If next_update_ is not present, the OCSP responder always has new information + * available. In this case the response would be considered immediately expired + * and invalid for stapling. + */ +struct SingleResponse { + SingleResponse(CertId cert_id, CertStatus status, Envoy::SystemTime this_update, + absl::optional next_update); + + const CertId cert_id_; + const CertStatus status_; + const Envoy::SystemTime this_update_; + const absl::optional next_update_; +}; + +/** + * Reflection of the ASN.1 ResponseData structure. + * Contains an OCSP response for each certificate in a given request + * as well as the time at which the response was produced. + */ +struct ResponseData { + ResponseData(Envoy::SystemTime produced_at, std::vector single_responses); + + const Envoy::SystemTime produced_at_; + const std::vector single_responses_; +}; + +/** + * An abstract type for OCSP response formats. Which variant of |Response| is + * used in an |OcspResponse| is indicated by the structure's OID. + * + * We currently enforce that OCSP responses must be for a single certificate + * only. The methods on this class extract the relevant information for the + * single certificate contained in the response. + */ +class Response { +public: + virtual ~Response() = default; + + /** + * @return The number of certs reported on by this response. + */ + virtual size_t getNumCerts() PURE; + + /** + * @return The revocation status of the certificate. + */ + virtual CertStatus getCertRevocationStatus() PURE; + + /** + * @return The serial number of the certificate. + */ + virtual const std::string& getCertSerialNumber() PURE; + + /** + * @return The beginning of the validity window for this response. + */ + virtual const Envoy::SystemTime& getThisUpdate() PURE; + + /** + * The time at which this response is considered to expire. If + * |nullopt|, then there is assumed to always be more up-to-date + * information available and the response is always considered expired. + * + * @return The end of the validity window for this response. + */ + virtual const absl::optional& getNextUpdate() PURE; +}; + +using ResponsePtr = std::unique_ptr; + +/** + * Reflection of the ASN.1 BasicOcspResponse structure. + * Contains the full data of an OCSP response and a signature/signature + * algorithm to verify the OCSP responder. + * + * BasicOcspResponse is the only supported Response type in RFC 6960. + */ +class BasicOcspResponse : public Response { +public: + BasicOcspResponse(ResponseData data, std::string signature_alg, std::vector signature); + + // Response + size_t getNumCerts() override { return data_.single_responses_.size(); } + CertStatus getCertRevocationStatus() override { return data_.single_responses_[0].status_; } + const std::string& getCertSerialNumber() override { + return data_.single_responses_[0].cert_id_.serial_number_; + } + const Envoy::SystemTime& getThisUpdate() override { + return data_.single_responses_[0].this_update_; + } + const absl::optional& getNextUpdate() override { + return data_.single_responses_[0].next_update_; + } + + const static std::string OID; + +private: + const ResponseData data_; + const std::string signature_alg_; + const std::vector signature_; +}; + +/** + * Reflection of the ASN.1 OcspResponse structure. + * This is the top-level data structure for OCSP responses. + */ +struct OcspResponse { + OcspResponse(OcspResponseStatus status, ResponsePtr&& response); + + OcspResponseStatus status_; + ResponsePtr response_; +}; + +/** + * A wrapper used to own and query an OCSP response in DER-encoded format. + */ +class OcspResponseWrapper { +public: + OcspResponseWrapper(std::string der_response, TimeSource& time_source); + + /** + * @return std::string& a reference to the underlying bytestring representation + * of the OCSP response + */ + const std::string& rawBytes() { return raw_bytes_; } + + /** + * @return OcspResponseStatus whether the OCSP response was successfully created + * or a status indicating an error in the OCSP process + */ + OcspResponseStatus getResponseStatus() { return response_->status_; } + + /** + * @returns CertStatus for the single SSL certificate reported on by this response + */ + CertStatus getCertRevocationStatus() { return response_->response_->getCertRevocationStatus(); } + + /** + * @param cert a X509& SSL certificate + * @returns bool whether this OCSP response contains the revocation status of |cert| + */ + bool matchesCertificate(X509& cert); + + /** + * Determines whether the OCSP response can no longer be considered valid. + * This can be true if the nextUpdate field of the response has passed + * or is not present, indicating that there is always more updated information + * available. + * + * @returns bool if the OCSP response is expired. + */ + bool isExpired(); + +private: + const std::string raw_bytes_; + const std::unique_ptr response_; + TimeSource& time_source_; +}; + +using OcspResponseWrapperPtr = std::unique_ptr; + +/** + * ASN.1 DER-encoded parsing functions similar to Asn1Utility but specifically + * for structures related to OCSP. + * + * Each function must advance |cbs| across the element it refers to. + */ +class Asn1OcspUtility { +public: + /** + * @param cbs a CBS& that refers to an ASN.1 OcspResponse element + * @returns std::unique_ptr the OCSP response encoded in |cbs| + * @throws Envoy::EnvoyException if |cbs| does not contain a well-formed OcspResponse + * element. + */ + static std::unique_ptr parseOcspResponse(CBS& cbs); + + /** + * @param cbs a CBS& that refers to an ASN.1 OcspResponseStatus element + * @returns OcspResponseStatus the OCSP response encoded in |cbs| + * @throws Envoy::EnvoyException if |cbs| does not contain a well-formed + * OcspResponseStatus element. + */ + static OcspResponseStatus parseResponseStatus(CBS& cbs); + + /** + * @param cbs a CBS& that refers to an ASN.1 Response element + * @returns Response containing the content of an OCSP response + * @throws Envoy::EnvoyException if |cbs| does not contain a well-formed + * structure that is a valid Response type. + */ + static ResponsePtr parseResponseBytes(CBS& cbs); + + /** + * @param cbs a CBS& that refers to an ASN.1 BasicOcspResponse element + * @returns BasicOcspResponse containing the content of an OCSP response + * @throws Envoy::EnvoyException if |cbs| does not contain a well-formed + * BasicOcspResponse element. + */ + static std::unique_ptr parseBasicOcspResponse(CBS& cbs); + + /** + * @param cbs a CBS& that refers to an ASN.1 ResponseData element + * @returns ResponseData containing the content of an OCSP response relating + * to certificate statuses. + * @throws Envoy::EnvoyException if |cbs| does not contain a well-formed + * ResponseData element. + */ + static ResponseData parseResponseData(CBS& cbs); + + /** + * @param cbs a CBS& that refers to an ASN.1 SingleResponse element + * @returns SingleResponse containing the id and revocation status of + * a single certificate. + * @throws Envoy::EnvoyException if |cbs| does not contain a well-formed + * SingleResponse element. + */ + static SingleResponse parseSingleResponse(CBS& cbs); + + /** + * @param cbs a CBS& that refers to an ASN.1 CertId element + * @returns CertId containing the information necessary to uniquely identify + * an SSL certificate. + * @throws Envoy::EnvoyException if |cbs| does not contain a well-formed + * CertId element. + */ + static CertId parseCertId(CBS& cbs); + + /** + * @param cbs a CBS& that refers to an ASN.1 CertStatus element + * @returns CertStatus the revocation status of a given certificate. + * @throws Envoy::EnvoyException if |cbs| does not contain a well-formed + * CertStatus element. + */ + static CertStatus parseCertStatus(CBS& cbs); +}; + +} // namespace Ocsp +} // namespace Tls +} // namespace TransportSockets +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/transport_sockets/tls/ocsp/BUILD b/test/extensions/transport_sockets/tls/ocsp/BUILD new file mode 100644 index 000000000000..6e64630e92d3 --- /dev/null +++ b/test/extensions/transport_sockets/tls/ocsp/BUILD @@ -0,0 +1,40 @@ +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_test", + "envoy_package", +) + +licenses(["notice"]) # Apache 2 + +envoy_package() + +envoy_cc_test( + name = "ocsp_test", + srcs = [ + "ocsp_test.cc", + ], + data = [ + "gen_unittest_ocsp_data.sh", + ], + external_deps = ["ssl"], + deps = [ + "//source/common/filesystem:filesystem_lib", + "//source/extensions/transport_sockets/tls:utility_lib", + "//source/extensions/transport_sockets/tls/ocsp:ocsp_lib", + "//test/extensions/transport_sockets/tls:ssl_test_utils", + "//test/test_common:environment_lib", + "//test/test_common:simulated_time_system_lib", + ], +) + +envoy_cc_test( + name = "asn1_utility_test", + srcs = [ + "asn1_utility_test.cc", + ], + external_deps = ["ssl"], + deps = [ + "//source/extensions/transport_sockets/tls/ocsp:asn1_utility_lib", + "//test/extensions/transport_sockets/tls:ssl_test_utils", + ], +) diff --git a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc new file mode 100644 index 000000000000..252d3e21ae98 --- /dev/null +++ b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc @@ -0,0 +1,340 @@ +#include + +#include "extensions/transport_sockets/tls/ocsp/asn1_utility.h" + +#include "test/test_common/utility.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace TransportSockets { +namespace Tls { +namespace Ocsp { + +class Asn1UtilityTest : public testing::Test { +public: + // DER encoding of a single TLV ASN.1 element. + // returns a pointer to the underlying buffer and transfers + // ownership to the caller. + uint8_t* asn1Encode(CBS& cbs, std::string& value, unsigned tag) { + bssl::ScopedCBB cbb; + CBB child; + auto data_head = reinterpret_cast(value.c_str()); + + EXPECT_TRUE(CBB_init(cbb.get(), 0)); + EXPECT_TRUE(CBB_add_asn1(cbb.get(), &child, tag)); + EXPECT_TRUE(CBB_add_bytes(&child, data_head, value.size())); + + uint8_t* buf; + size_t buf_len; + EXPECT_TRUE(CBB_finish(cbb.get(), &buf, &buf_len)); + + CBS_init(&cbs, buf, buf_len); + return buf; + } + + void expectThrowOnWrongTag(std::function parse) { + CBS cbs; + CBS_init(&cbs, asn1_true.data(), asn1_true.size()); + EXPECT_THROW(parse(cbs), EnvoyException); + } + + const std::vector asn1_true = {0x1u, 1, 0xff}; + const std::vector asn1_empty_seq = {0x30, 0}; +}; + +TEST_F(Asn1UtilityTest, ParseMethodsWrongTagTest) { + expectThrowOnWrongTag( + [](CBS& cbs) { Asn1Utility::parseSequenceOf(cbs, [](CBS&) { return ""; }); }); + expectThrowOnWrongTag(Asn1Utility::parseOid); + expectThrowOnWrongTag(Asn1Utility::parseGeneralizedTime); + expectThrowOnWrongTag(Asn1Utility::parseInteger); + expectThrowOnWrongTag(Asn1Utility::parseOctetString); + expectThrowOnWrongTag(Asn1Utility::parseBitString); + expectThrowOnWrongTag(Asn1Utility::parseAlgorithmIdentifier); +} + +TEST_F(Asn1UtilityTest, ToStringTest) { + CBS cbs; + std::string data = "test"; + CBS_init(&cbs, reinterpret_cast(data.c_str()), data.size()); + EXPECT_EQ(data, Asn1Utility::cbsToString(cbs)); +} + +TEST_F(Asn1UtilityTest, ParseSequenceOfEmptySequenceTest) { + CBS cbs; + CBS_init(&cbs, asn1_empty_seq.data(), asn1_empty_seq.size()); + + std::vector vec; + auto actual = Asn1Utility::parseSequenceOf(cbs, [](CBS&) { return ""; }); + EXPECT_EQ(vec, actual); +} + +TEST_F(Asn1UtilityTest, ParseSequenceOfMultipleElementSequenceTest) { + std::vector octet_seq = { + // SEQUENCE OF 3 2-byte elements + 0x30, + 3 * (2 + 2), + // 1st OCTET STRING + 0x4u, + 2, + 0x1, + 0x2, + // 2nd OCTET STRING + 0x4u, + 2, + 0x3, + 0x4, + // 3rd OCTET STRING + 0x4u, + 2, + 0x5, + 0x6, + }; + CBS cbs; + CBS_init(&cbs, octet_seq.data(), octet_seq.size()); + + std::vector vec = {"\x1\x2", "\x3\x4", "\x5\x6"}; + auto actual = Asn1Utility::parseSequenceOf(cbs, Asn1Utility::parseOctetString); + EXPECT_EQ(vec, actual); +} + +TEST_F(Asn1UtilityTest, SequenceOfLengthMismatchErrorTest) { + std::vector malformed = { + // SEQUENCE OF length wrongfully 2 instead of 4 bytes + 0x30, + 3, + // 1st OCTET STRING + 0x4u, + 2, + 0x1, + 0x2, + }; + CBS cbs; + CBS_init(&cbs, malformed.data(), malformed.size()); + + EXPECT_THROW_WITH_MESSAGE( + Asn1Utility::parseSequenceOf(cbs, Asn1Utility::parseOctetString), EnvoyException, + "Input is not a well-formed ASN.1 OCTETSTRING"); +} + +TEST_F(Asn1UtilityTest, SequenceOfMixedTypeErrorTest) { + std::vector mixed_type = { + // SEQUENCE OF 1 OCTET STRING and 1 BOOLEAN + 0x30, + 7, + // OCTET STRING + 0x4u, + 2, + 0x1, + 0x2, + // BOOLEAN true + 0x1u, + 1, + 0xff, + }; + CBS cbs; + CBS_init(&cbs, mixed_type.data(), mixed_type.size()); + + EXPECT_THROW_WITH_MESSAGE( + Asn1Utility::parseSequenceOf(cbs, Asn1Utility::parseOctetString), EnvoyException, + "Input is not a well-formed ASN.1 OCTETSTRING"); +} + +TEST_F(Asn1UtilityTest, IsOptionalPresentTest) { + CBS cbs, value; + CBS_init(&cbs, asn1_true.data(), asn1_true.size()); + + const uint8_t* start = CBS_data(&cbs); + EXPECT_FALSE(Asn1Utility::isOptionalPresent(cbs, nullptr, CBS_ASN1_INTEGER)); + EXPECT_EQ(start, CBS_data(&cbs)); + + EXPECT_TRUE(Asn1Utility::isOptionalPresent(cbs, &value, CBS_ASN1_BOOLEAN)); + EXPECT_EQ(0xff, *CBS_data(&value)); +} + +TEST_F(Asn1UtilityTest, IsOptionalPresentMissingValueTest) { + std::vector missing_val_bool = {0x1u, 1}; + CBS cbs, value; + CBS_init(&cbs, missing_val_bool.data(), missing_val_bool.size()); + + EXPECT_THROW_WITH_MESSAGE(Asn1Utility::isOptionalPresent(cbs, &value, CBS_ASN1_BOOLEAN), + EnvoyException, "Failed to parse ASN.1 element tag"); +} + +TEST_F(Asn1UtilityTest, ParseOptionalTest) { + std::vector nothing; + std::vector explicit_optional_true = {0, 3, 0x1u, 1, 0xff}; + + CBS cbs_true, cbs_explicit_optional_true, cbs_empty_seq, cbs_nothing; + CBS_init(&cbs_true, asn1_true.data(), asn1_true.size()); + CBS_init(&cbs_explicit_optional_true, explicit_optional_true.data(), + explicit_optional_true.size()); + CBS_init(&cbs_empty_seq, asn1_empty_seq.data(), asn1_empty_seq.size()); + CBS_init(&cbs_nothing, nothing.data(), nothing.size()); + + auto parseBool = [](CBS& cbs) -> bool { + int res; + CBS_get_asn1_bool(&cbs, &res); + return res; + }; + + absl::optional expected(true); + EXPECT_EQ(expected, Asn1Utility::parseOptional(cbs_explicit_optional_true, parseBool, 0)); + EXPECT_EQ(absl::nullopt, + Asn1Utility::parseOptional(cbs_empty_seq, parseBool, CBS_ASN1_BOOLEAN)); + EXPECT_EQ(absl::nullopt, + Asn1Utility::parseOptional(cbs_nothing, parseBool, CBS_ASN1_BOOLEAN)); +} + +TEST_F(Asn1UtilityTest, ParseOidTest) { + std::string oid = "1.1.1.1.1.1.1"; + + bssl::ScopedCBB cbb; + CBB child; + ASSERT_TRUE(CBB_init(cbb.get(), 0)); + ASSERT_TRUE(CBB_add_asn1(cbb.get(), &child, CBS_ASN1_OBJECT)); + ASSERT_TRUE(CBB_add_asn1_oid_from_text(&child, oid.c_str(), oid.size())); + + uint8_t* buf; + size_t buf_len; + CBS cbs; + ASSERT_TRUE(CBB_finish(cbb.get(), &buf, &buf_len)); + CBS_init(&cbs, buf, buf_len); + bssl::UniquePtr scoped(buf); + + EXPECT_EQ(oid, Asn1Utility::parseOid(cbs)); +} + +TEST_F(Asn1UtilityTest, ParseGeneralizedTimeWrongFormatErrorTest) { + std::string invalid_time = ""; + CBS cbs; + bssl::UniquePtr scoped(asn1Encode(cbs, invalid_time, CBS_ASN1_GENERALIZEDTIME)); + + EXPECT_ANY_THROW(Asn1Utility::parseGeneralizedTime(cbs)); +} + +TEST_F(Asn1UtilityTest, ParseGeneralizedTimeTest) { + std::string time = "20070614185900Z"; + + CBS cbs; + bssl::UniquePtr scoped(asn1Encode(cbs, time, CBS_ASN1_GENERALIZEDTIME)); + absl::Time expected = TestUtility::parseTime(time, "%E4Y%m%d%H%M%SZ"); + + EXPECT_EQ(absl::ToChronoTime(expected), Asn1Utility::parseGeneralizedTime(cbs)); +} + +TEST_F(Asn1UtilityTest, TestParseGeneralizedTimeRejectsNonUTCTime) { + std::string local_time = "20070601145918"; + CBS cbs; + bssl::UniquePtr scoped(asn1Encode(cbs, local_time, CBS_ASN1_GENERALIZEDTIME)); + + EXPECT_THROW_WITH_MESSAGE(Asn1Utility::parseGeneralizedTime(cbs), EnvoyException, + "GENERALIZEDTIME must be in UTC"); +} + +TEST_F(Asn1UtilityTest, TestParseGeneralizedTimeInvalidTime) { + std::string ymd = "20070601Z"; + CBS cbs; + bssl::UniquePtr scoped(asn1Encode(cbs, ymd, CBS_ASN1_GENERALIZEDTIME)); + + EXPECT_THROW_WITH_REGEX(Asn1Utility::parseGeneralizedTime(cbs), EnvoyException, + "Error parsing timestamp 20070601Z with format %E4Y%m%d%H%M%SZ"); +} + +TEST_F(Asn1UtilityTest, ParseIntegerTest) { + std::vector> integers = { + {1, "01"}, + {10, "0a"}, + {1000000, "0f4240"}, + {-1, "-01"}, + }; + bssl::ScopedCBB cbb; + CBS cbs; + uint8_t* buf; + size_t buf_len; + for (auto int_and_hex : integers) { + ASSERT_TRUE(CBB_init(cbb.get(), 0)); + ASSERT_TRUE(CBB_add_asn1_int64(cbb.get(), int_and_hex.first)); + ASSERT_TRUE(CBB_finish(cbb.get(), &buf, &buf_len)); + + CBS_init(&cbs, buf, buf_len); + bssl::UniquePtr scoped_buf(buf); + + EXPECT_EQ(int_and_hex.second, Asn1Utility::parseInteger(cbs)); + cbb.Reset(); + } +} + +TEST_F(Asn1UtilityTest, ParseOctetStringTest) { + std::string data = "example_string"; + CBS cbs; + bssl::UniquePtr scoped(asn1Encode(cbs, data, CBS_ASN1_OCTETSTRING)); + + EXPECT_EQ(data, Asn1Utility::parseOctetString(cbs)); +} + +TEST_F(Asn1UtilityTest, ParseBitStringTest) { + std::vector data = {0, 1, 2, 3}; + std::vector tlv = {0x3u, 4}; + tlv.insert(tlv.end(), data.begin(), data.end()); + + CBS cbs; + CBS_init(&cbs, tlv.data(), tlv.size()); + EXPECT_EQ(data, Asn1Utility::parseBitString(cbs)); +} + +TEST_F(Asn1UtilityTest, ParseAlgorithmIdentifierTest) { + std::string sha256 = "2.16.840.1.101.3.4.2.1"; + + bssl::ScopedCBB cbb; + CBB seq, oid; + ASSERT_TRUE(CBB_init(cbb.get(), 0)); + ASSERT_TRUE(CBB_add_asn1(cbb.get(), &seq, CBS_ASN1_SEQUENCE)); + ASSERT_TRUE(CBB_add_asn1(&seq, &oid, CBS_ASN1_OBJECT)); + ASSERT_TRUE(CBB_add_asn1_oid_from_text(&oid, sha256.c_str(), sha256.size())); + + uint8_t* buf; + size_t buf_len; + CBS cbs; + ASSERT_TRUE(CBB_finish(cbb.get(), &buf, &buf_len)); + CBS_init(&cbs, buf, buf_len); + bssl::UniquePtr scoped(buf); + + EXPECT_EQ(sha256, Asn1Utility::parseAlgorithmIdentifier(cbs)); +} + +TEST_F(Asn1UtilityTest, SkipOptionalPresentAdvancesTest) { + CBS cbs; + CBS_init(&cbs, asn1_empty_seq.data(), asn1_empty_seq.size()); + + const uint8_t* start = CBS_data(&cbs); + EXPECT_NO_THROW({ Asn1Utility::skipOptional(cbs, CBS_ASN1_SEQUENCE); }); + EXPECT_EQ(start + 2, CBS_data(&cbs)); +} + +TEST_F(Asn1UtilityTest, SkipOptionalNotPresentDoesNotAdvanceTest) { + CBS cbs; + CBS_init(&cbs, asn1_empty_seq.data(), asn1_empty_seq.size()); + + const uint8_t* start = CBS_data(&cbs); + EXPECT_NO_THROW({ Asn1Utility::skipOptional(cbs, CBS_ASN1_BOOLEAN); }); + EXPECT_EQ(start, CBS_data(&cbs)); +} + +TEST_F(Asn1UtilityTest, SkipOptionalMalformedTagTest) { + std::vector malformed_seq = {0x30}; + CBS cbs; + CBS_init(&cbs, malformed_seq.data(), malformed_seq.size()); + + EXPECT_THROW_WITH_MESSAGE(Asn1Utility::skipOptional(cbs, CBS_ASN1_SEQUENCE), EnvoyException, + "Failed to parse ASN.1 element tag"); +} + +} // namespace Ocsp +} // namespace Tls +} // namespace TransportSockets +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/transport_sockets/tls/ocsp/gen_unittest_ocsp_data.sh b/test/extensions/transport_sockets/tls/ocsp/gen_unittest_ocsp_data.sh new file mode 100755 index 000000000000..a2a8f8b3a4e6 --- /dev/null +++ b/test/extensions/transport_sockets/tls/ocsp/gen_unittest_ocsp_data.sh @@ -0,0 +1,156 @@ +#!/bin/bash +# +# Create test certificates and OCSP responses for them for unittests. + +set -e + +trap cleanup EXIT +cleanup() { + rm *_index* + rm *.csr + rm *.cnf + rm *_serial* +} + +[[ -z "${TEST_TMPDIR}" ]] && TEST_TMPDIR="$(cd $(dirname $0) && pwd)" + +TEST_OCSP_DIR="${TEST_TMPDIR}/ocsp_test_data" +mkdir -p "${TEST_OCSP_DIR}" + +cd $TEST_OCSP_DIR + +################################################## +# Make the configuration file +################################################## + +# $1= $2= +generate_config() { +(cat << EOF +[ req ] +default_bits = 2048 +distinguished_name = req_distinguished_name + +[ req_distinguished_name ] +countryName = US +countryName_default = US +stateOrProvinceName = California +stateOrProvinceName_default = California +localityName = San Francisco +localityName_default = San Francisco +organizationName = Lyft +organizationName_default = Lyft +organizationalUnitName = Lyft Engineering +organizationalUnitName_default = Lyft Engineering +commonName = $1 +commonName_default = $1 +commonName_max = 64 + +[ ca ] +default_ca = CA_default + +[ CA_default ] +dir = ${TEST_OCSP_DIR} +certs = ${TEST_OCSP_DIR} +new_certs_dir = ${TEST_OCSP_DIR} +serial = ${TEST_OCSP_DIR} +database = ${TEST_OCSP_DIR}/$2_index.txt +serial = ${TEST_OCSP_DIR}/$2_serial + +private_key = ${TEST_OCSP_DIR}/$2_key.pem +certificate = ${TEST_OCSP_DIR}/$2_cert.pem + +default_days = 375 +default_md = sha256 +preserve = no +policy = policy_default + +[ policy_default ] +countryName = optional +stateOrProvinceName = optional +organizationName = optional +organizationalUnitName = optional +commonName = supplied +emailAddress = optional + + +[ v3_ca ] +subjectKeyIdentifier = hash +authorityKeyIdentifier = keyid:always,issuer +basicConstraints = critical, CA:true +keyUsage = critical, digitalSignature, cRLSign, keyCertSign +EOF +) > $1.cnf +} + +# $1= $2=[issuer name] +generate_ca() { + if [[ "$2" != "" ]]; then local EXTRA_ARGS="-CA $2_cert.pem -CAkey $2_key.pem -CAcreateserial"; fi + openssl genrsa -out $1_key.pem 2048 + openssl req -new -key $1_key.pem -out $1_cert.csr \ + -config $1.cnf -batch -sha256 + openssl x509 -req \ + -in $1_cert.csr -signkey $1_key.pem -out $1_cert.pem \ + -extensions v3_ca -extfile $1.cnf $EXTRA_ARGS +} + +# $1= $2= +generate_x509_cert() { + openssl genrsa -out $1_key.pem 2048 + openssl req -new -key $1_key.pem -out $1_cert.csr -config $1.cnf -batch -sha256 + openssl ca -config $1.cnf -notext -batch -in $1_cert.csr -out $1_cert.pem +} + +# $1= $2= $3= $4=[extra args] +generate_ocsp_response() { + # Generate an OCSP request + openssl ocsp -CAfile $2_cert.pem -issuer $2_cert.pem \ + -cert $1_cert.pem -reqout $3_ocsp_req.der + + # Generate the OCSP response + openssl ocsp -CA $2_cert.pem \ + -rkey $2_key.pem -rsigner $2_cert.pem -index $2_index.txt \ + -reqin $3_ocsp_req.der -respout $3_ocsp_resp.der $4 +} + +# $1= $2= +revoke_certificate() { + openssl ca -revoke $1_cert.pem -keyfile $2_key.pem -cert $2_cert.pem -config $2.cnf +} + +# Set up the CA +touch ca_index.txt +echo "unique_subject = no" > ca_index.txt.attr +echo 1000 > ca_serial +generate_config ca ca +generate_ca ca + +# Set up an intermediate CA with a different database +touch intermediate_ca_index.txt +echo "unique_subject = no" > intermediate_ca_index.txt.attr +echo 1000 > intermediate_ca_serial +generate_config intermediate_ca intermediate_ca +generate_ca intermediate_ca ca + +# Generate valid cert and OCSP response +generate_config good ca +generate_x509_cert good ca +generate_ocsp_response good ca good "-ndays 7" + +# Generate OCSP response with the responder key hash instead of name +generate_ocsp_response good ca responder_key_hash -resp_key_id + +# Generate and revoke a cert and create OCSP response +generate_config revoked ca +generate_x509_cert revoked ca +revoke_certificate revoked ca +generate_ocsp_response revoked ca revoked + +# Create OCSP response for cert unknown to the CA +generate_ocsp_response good intermediate_ca unknown + +# Generate an OCSP request/response for multiple certs +openssl ocsp -CAfile ca_cert.pem -issuer ca_cert.pem \ + -cert good_cert.pem -cert revoked_cert.pem -reqout multiple_cert_ocsp_req.der +openssl ocsp -CA ca_cert.pem \ + -rkey ca_key.pem -rsigner ca_cert.pem -index ca_index.txt \ + -reqin multiple_cert_ocsp_req.der -respout multiple_cert_ocsp_resp.der diff --git a/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc b/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc new file mode 100644 index 000000000000..9ceb59537628 --- /dev/null +++ b/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc @@ -0,0 +1,291 @@ +#include "common/filesystem/filesystem_impl.h" + +#include "extensions/transport_sockets/tls/ocsp/ocsp.h" +#include "extensions/transport_sockets/tls/utility.h" + +#include "test/extensions/transport_sockets/tls/ssl_test_utility.h" +#include "test/test_common/environment.h" +#include "test/test_common/simulated_time_system.h" +#include "test/test_common/utility.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" +#include "openssl/x509v3.h" + +namespace Envoy { +namespace Extensions { +namespace TransportSockets { +namespace Tls { +namespace Ocsp { + +namespace CertUtility = Envoy::Extensions::TransportSockets::Tls::Utility; + +class OcspFullResponseParsingTest : public testing::Test { +public: + static void SetUpTestSuite() { // NOLINT(readability-identifier-naming) + TestEnvironment::exec({TestEnvironment::runfilesPath( + "test/extensions/transport_sockets/tls/ocsp/gen_unittest_ocsp_data.sh")}); + } + + std::string fullPath(std::string filename) { + return TestEnvironment::substitute("{{ test_tmpdir }}/ocsp_test_data/" + filename); + } + + std::string readFile(std::string filename) { + return TestEnvironment::readFileToStringForTest(fullPath(filename)); + } + + void setup(std::string response_filename) { + std::string der_response = readFile(response_filename); + response_ = std::make_unique(der_response, time_system_); + EXPECT_EQ(response_->rawBytes(), der_response); + } + + void expectCertStatus(CertStatus expected_status) { + EXPECT_EQ(OcspResponseStatus::SUCCESSFUL, response_->getResponseStatus()); + EXPECT_EQ(expected_status, response_->getCertRevocationStatus()); + } + + void expectCertificateMatches(std::string cert_filename) { + auto cert_ = readCertFromFile(fullPath(cert_filename)); + EXPECT_TRUE(response_->matchesCertificate(*cert_)); + } + +protected: + Event::SimulatedTimeSystem time_system_; + OcspResponseWrapperPtr response_; +}; + +TEST_F(OcspFullResponseParsingTest, GoodCertTest) { + setup("good_ocsp_resp.der"); + expectCertStatus(CertStatus::GOOD); + expectCertificateMatches("good_cert.pem"); + + auto cert = readCertFromFile(fullPath("revoked_cert.pem")); + EXPECT_FALSE(response_->matchesCertificate(*cert)); + + // Contains nextUpdate that is in the future + EXPECT_FALSE(response_->isExpired()); +} + +TEST_F(OcspFullResponseParsingTest, RevokedCertTest) { + setup("revoked_ocsp_resp.der"); + expectCertStatus(CertStatus::REVOKED); + expectCertificateMatches("revoked_cert.pem"); + EXPECT_TRUE(response_->isExpired()); +} + +TEST_F(OcspFullResponseParsingTest, UnknownCertTest) { + setup("unknown_ocsp_resp.der"); + expectCertStatus(CertStatus::UNKNOWN); + expectCertificateMatches("good_cert.pem"); + EXPECT_TRUE(response_->isExpired()); +} + +TEST_F(OcspFullResponseParsingTest, ExpiredResponseTest) { + auto next_week = time_system_.systemTime() + std::chrono::hours(8 * 24); + time_system_.setSystemTime(next_week); + setup("good_ocsp_resp.der"); + // nextUpdate is present but in the past + EXPECT_TRUE(response_->isExpired()); +} + +TEST_F(OcspFullResponseParsingTest, ThisUpdateAfterNowTest) { + auto past_time = TestUtility::parseTime("2000 01 01", "%Y %m %d"); + time_system_.setSystemTime(absl::ToChronoTime(past_time)); + EXPECT_THROW_WITH_REGEX(setup("good_ocsp_resp.der"), EnvoyException, + "OCSP Response thisUpdate field is set in the future"); +} + +TEST_F(OcspFullResponseParsingTest, ResponderIdKeyHashTest) { + setup("responder_key_hash_ocsp_resp.der"); + expectCertStatus(CertStatus::GOOD); + expectCertificateMatches("good_cert.pem"); + EXPECT_TRUE(response_->isExpired()); +} + +TEST_F(OcspFullResponseParsingTest, MultiCertResponseTest) { + std::string resp_bytes = readFile("multiple_cert_ocsp_resp.der"); + EXPECT_THROW_WITH_MESSAGE(OcspResponseWrapper response(resp_bytes, time_system_), EnvoyException, + "OCSP Response must be for one certificate only"); +} + +TEST_F(OcspFullResponseParsingTest, NoResponseBodyTest) { + std::vector data = { + // SEQUENCE + 0x30, 3, + // OcspResponseStatus - INTERNAL_ERROR + 0xau, 1, 2, + // no response bytes + }; + std::string byte_string(reinterpret_cast(data.data()), data.size()); + EXPECT_THROW_WITH_MESSAGE(OcspResponseWrapper response(byte_string, time_system_), EnvoyException, + "OCSP response has no body"); +} + +TEST_F(OcspFullResponseParsingTest, OnlyOneResponseInByteStringTest) { + auto resp1_bytes = readFile("good_ocsp_resp.der"); + auto resp2_bytes = readFile("revoked_ocsp_resp.der"); + auto two_responses = resp1_bytes + resp2_bytes; + + EXPECT_THROW_WITH_MESSAGE(OcspResponseWrapper response_wrapper(two_responses, time_system_), + EnvoyException, "Data contained more than a single OCSP response"); +} + +TEST_F(OcspFullResponseParsingTest, ParseOcspResponseWrongTagTest) { + std::string resp_bytes = readFile("good_ocsp_resp.der"); + // Change the SEQUENCE tag to an OCTETSTRING tag + resp_bytes.replace(0, 1, "\x4u"); + EXPECT_THROW_WITH_MESSAGE(OcspResponseWrapper response_wrapper(resp_bytes, time_system_), + EnvoyException, "OCSP Response is not a well-formed ASN.1 SEQUENCE"); +} + +class Asn1OcspUtilityTest : public testing::Test { +public: + void expectResponseStatus(uint8_t code, OcspResponseStatus expected) { + std::vector asn1_enum = {0xau, 1, code}; + CBS cbs; + CBS_init(&cbs, asn1_enum.data(), asn1_enum.size()); + + EXPECT_EQ(expected, Asn1OcspUtility::parseResponseStatus(cbs)); + } + + void expectThrowOnWrongTag(std::function parse) { + CBS cbs; + CBS_init(&cbs, asn1_true.data(), asn1_true.size()); + EXPECT_THROW(parse(cbs), EnvoyException); + } + + const std::vector asn1_true = {0x1u, 1, 0xff}; +}; + +TEST_F(Asn1OcspUtilityTest, ParseResponseStatusTest) { + expectResponseStatus(0, OcspResponseStatus::SUCCESSFUL); + expectResponseStatus(1, OcspResponseStatus::MALFORMED_REQUEST); + expectResponseStatus(2, OcspResponseStatus::INTERNAL_ERROR); + expectResponseStatus(3, OcspResponseStatus::TRY_LATER); + expectResponseStatus(5, OcspResponseStatus::SIG_REQUIRED); + expectResponseStatus(6, OcspResponseStatus::UNAUTHORIZED); +} + +TEST_F(Asn1OcspUtilityTest, ParseMethodWrongTagTest) { + expectThrowOnWrongTag(Asn1OcspUtility::parseResponseBytes); + expectThrowOnWrongTag(Asn1OcspUtility::parseBasicOcspResponse); + expectThrowOnWrongTag(Asn1OcspUtility::parseResponseData); + expectThrowOnWrongTag(Asn1OcspUtility::parseSingleResponse); + expectThrowOnWrongTag(Asn1OcspUtility::parseCertId); + expectThrowOnWrongTag(Asn1OcspUtility::parseResponseStatus); +} + +TEST_F(Asn1OcspUtilityTest, ParseResponseDataBadResponderIdVariantTest) { + std::vector data = { + // SEQUENCE + 0x30, + 6, + // version + 0, + 1, + 0, + // Invalid Responeder ID tag 3 + 3, + 1, + 0, + }; + CBS cbs; + CBS_init(&cbs, data.data(), data.size()); + EXPECT_THROW_WITH_MESSAGE(Asn1OcspUtility::parseResponseData(cbs), EnvoyException, + "Unknown choice for Responder ID: 3"); +} + +TEST_F(Asn1OcspUtilityTest, ParseOcspResponseBytesMissingTest) { + std::vector data = { + // SEQUENCE + 0x30, 3, + // OcspResponseStatus - INTERNAL_ERROR + 0xau, 1, 2, + // no response bytes + }; + CBS cbs; + CBS_init(&cbs, data.data(), data.size()); + auto response = Asn1OcspUtility::parseOcspResponse(cbs); + EXPECT_EQ(response->status_, OcspResponseStatus::INTERNAL_ERROR); + EXPECT_TRUE(response->response_ == nullptr); +} + +TEST_F(Asn1OcspUtilityTest, ParseResponseStatusUnknownVariantTest) { + std::vector bad_enum_variant = {0xau, 1, 4}; + CBS cbs; + CBS_init(&cbs, bad_enum_variant.data(), bad_enum_variant.size()); + EXPECT_THROW_WITH_MESSAGE(Asn1OcspUtility::parseResponseStatus(cbs), EnvoyException, + "Unknown OCSP Response Status variant: 4"); +} + +TEST_F(Asn1OcspUtilityTest, ParseResponseBytesNoOctetStringTest) { + std::string oid_str = "1.1.1.1.1.1.1"; + bssl::ScopedCBB cbb; + CBB seq, oid, obj; + uint8_t* buf; + size_t buf_len; + + ASSERT_TRUE(CBB_init(cbb.get(), 0)); + ASSERT_TRUE(CBB_add_asn1(cbb.get(), &seq, CBS_ASN1_SEQUENCE)); + ASSERT_TRUE(CBB_add_asn1(&seq, &oid, CBS_ASN1_OBJECT)); + ASSERT_TRUE(CBB_add_asn1_oid_from_text(&oid, oid_str.c_str(), oid_str.size())); + // Empty sequence instead of OCTETSTRING with the response + ASSERT_TRUE(CBB_add_asn1(&seq, &obj, CBS_ASN1_SEQUENCE)); + ASSERT_TRUE(CBB_finish(cbb.get(), &buf, &buf_len)); + + CBS cbs; + CBS_init(&cbs, buf, buf_len); + bssl::UniquePtr scoped(buf); + + EXPECT_THROW_WITH_MESSAGE(Asn1OcspUtility::parseResponseBytes(cbs), EnvoyException, + "Expected ASN.1 OCTETSTRING for response"); +} + +TEST_F(Asn1OcspUtilityTest, ParseResponseBytesUnknownResponseTypeTest) { + std::string oid_str = "1.1.1.1.1.1.1"; + bssl::ScopedCBB cbb; + CBB seq, oid, obj; + uint8_t* buf; + size_t buf_len; + + ASSERT_TRUE(CBB_init(cbb.get(), 0)); + ASSERT_TRUE(CBB_add_asn1(cbb.get(), &seq, CBS_ASN1_SEQUENCE)); + ASSERT_TRUE(CBB_add_asn1(&seq, &oid, CBS_ASN1_OBJECT)); + ASSERT_TRUE(CBB_add_asn1_oid_from_text(&oid, oid_str.c_str(), oid_str.size())); + ASSERT_TRUE(CBB_add_asn1(&seq, &obj, CBS_ASN1_OCTETSTRING)); + ASSERT_TRUE(CBB_add_bytes(&obj, reinterpret_cast("\x1\x2\x3"), 3)); + ASSERT_TRUE(CBB_finish(cbb.get(), &buf, &buf_len)); + + CBS cbs; + CBS_init(&cbs, buf, buf_len); + bssl::UniquePtr scoped(buf); + + EXPECT_THROW_WITH_MESSAGE(Asn1OcspUtility::parseResponseBytes(cbs), EnvoyException, + "Unknown OCSP Response type with OID: 1.1.1.1.1.1.1"); +} + +TEST_F(Asn1OcspUtilityTest, ParseCertStatusInvalidVariantTest) { + std::vector invalid_choice = {3, 0}; + CBS cbs; + CBS_init(&cbs, invalid_choice.data(), invalid_choice.size()); + + EXPECT_THROW_WITH_MESSAGE(Asn1OcspUtility::parseCertStatus(cbs), EnvoyException, + "Unknown OcspCertStatus tag: 3"); +} + +TEST_F(Asn1OcspUtilityTest, ParseCertStatusInvalidByteStringTest) { + std::vector invalid_choice; + CBS cbs; + CBS_init(&cbs, invalid_choice.data(), invalid_choice.size()); + + EXPECT_THROW_WITH_MESSAGE(Asn1OcspUtility::parseCertStatus(cbs), EnvoyException, + "Failed to parse ASN.1 element tag"); +} + +} // namespace Ocsp +} // namespace Tls +} // namespace TransportSockets +} // namespace Extensions +} // namespace Envoy From 4336a8c801b0d3882ea7219e5452a0e210b33915 Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Mon, 27 Jul 2020 11:18:05 -0700 Subject: [PATCH 02/31] fix spelling Signed-off-by: Daniel Goldstein --- api/BUILD | 1 - .../transport_sockets/tls/ocsp/asn1_utility.h | 50 +++++++++---------- .../transport_sockets/tls/ocsp/ocsp.cc | 2 +- .../transport_sockets/tls/ocsp/ocsp.h | 24 ++++----- .../transport_sockets/tls/ocsp/ocsp_test.cc | 2 +- tools/spelling/spelling_dictionary.txt | 8 +++ 6 files changed, 47 insertions(+), 40 deletions(-) diff --git a/api/BUILD b/api/BUILD index 8c608fdeca4a..3317f5a64c55 100644 --- a/api/BUILD +++ b/api/BUILD @@ -246,7 +246,6 @@ proto_library( "//envoy/service/discovery/v3:pkg", "//envoy/service/endpoint/v3:pkg", "//envoy/service/event_reporting/v3:pkg", - "//envoy/service/extension/v3:pkg", "//envoy/service/health/v3:pkg", "//envoy/service/listener/v3:pkg", "//envoy/service/load_stats/v3:pkg", diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h index 31a346d0737c..6436360726bf 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h @@ -38,22 +38,22 @@ template using Asn1ParsingFunc = std::function; * * Each function adheres to the invariant that given a reference * to a crypto bytestring (CBS&), it will parse the specified - * ASN.1 element and advance |cbs| over it. + * ASN.1 element and advance `cbs` over it. * * An exception is thrown if the bytestring is malformed or does * not match the specified ASN.1 object. The position - * of |cbs| is not reliable after an exception is thrown. + * of `cbs` is not reliable after an exception is thrown. */ class Asn1Utility { public: ~Asn1Utility() = default; /** - * Extracts the full contents of |cbs| as a string. - * This copies the data in |cbs|. + * Extracts the full contents of `cbs` as a string. + * This copies the data in `cbs`. * * @param cbs a CBS& that refers to the current document position - * @returns std::string containing the contents of |cbs| + * @returns std::string containing the contents of `cbs` */ static std::string cbsToString(CBS& cbs); @@ -64,7 +64,7 @@ class Asn1Utility { * @param cbs a CBS& that refers to an ASN.1 SEQUENCE OF object * @param parseElement an Asn1ParsingFunc used to parse each element * @returns std::vector containing the parsed elements of the sequence. - * @throws Envoy::EnvoyException if |cbs| does not point to a well-formed + * @throws Envoy::EnvoyException if `cbs` does not point to a well-formed * SEQUENCE OF object. */ template @@ -73,11 +73,11 @@ class Asn1Utility { /** * Checks if an explicitly tagged optional element of |tag| is present and * if so parses its value with |parseData|. If the element is not present, - * |cbs| is not advanced. + * `cbs` is not advanced. * * @param cbs a CBS& that refers to the current document position * @param parseData an Asn1ParsingFunc used to parse the data if present - * @return absl::optional with a T if |cbs| is of the specified tag, + * @return absl::optional with a T if `cbs` is of the specified tag, * else nullopt */ template @@ -85,31 +85,31 @@ class Asn1Utility { /** * Returns whether or not an element explicitly tagged with |tag| is present - * at |cbs|. If so, |cbs| is advanced over the optional and assigns + * at `cbs`. If so, `cbs` is advanced over the optional and assigns * |data| to the inner element, if |data| is not nullptr. - * If |cbs| does not contain |tag|, |cbs| remains at the same position. + * If `cbs` does not contain |tag|, `cbs` remains at the same position. * * @param cbs a CBS& that refers to the current document position - * @param data a CBS& that is set to the contents of |cbs| + * @param data a CBS& that is set to the contents of `cbs` * @param an explicit tag indicating an optional value * - * @returns bool whether |cbs| points to an element tagged with |tag| - * @throws Envoy::EnvoyException if |cbs| is a malformed TLV bytestring + * @returns bool whether `cbs` points to an element tagged with |tag| + * @throws Envoy::EnvoyException if `cbs` is a malformed TLV bytestring */ static bool isOptionalPresent(CBS& cbs, CBS* data, unsigned tag); /** * @param cbs a CBS& that refers to an ASN.1 OBJECT IDENTIFIER element - * @returns std::string the OID encoded in |cbs| - * @throws Envoy::EnvoyException if |cbs| does not point to a well-formed + * @returns std::string the OID encoded in `cbs` + * @throws Envoy::EnvoyException if `cbs` does not point to a well-formed * OBJECT IDENTIFIER */ static std::string parseOid(CBS& cbs); /** * @param cbs a CBS& that refers to an ASN.1 GENERALIZEDTIME element - * @returns Envoy::SystemTime the UTC timestamp encoded in |cbs| - * @throws Envoy::EnvoyException if |cbs| does not point to a well-formed + * @returns Envoy::SystemTime the UTC timestamp encoded in `cbs` + * @throws Envoy::EnvoyException if `cbs` does not point to a well-formed * GENERALIZEDTIME */ static Envoy::SystemTime parseGeneralizedTime(CBS& cbs); @@ -122,7 +122,7 @@ class Asn1Utility { * * @param cbs a CBS& that refers to an ASN.1 INTEGER element * @returns std::string a hex representation of the integer - * @throws Envoy::EnvoyException if |cbs| does not point to a well-formed + * @throws Envoy::EnvoyException if `cbs` does not point to a well-formed * INTEGER */ static std::string parseInteger(CBS& cbs); @@ -133,15 +133,15 @@ class Asn1Utility { * * @param cbs a CBS& that refers to an ASN.1 AlgorithmIdentifier element * @returns std::string the OID of the algorithm - * @throws Envoy::EnvoyException if |cbs| does not point to a well-formed + * @throws Envoy::EnvoyException if `cbs` does not point to a well-formed * AlgorithmIdentifier */ static std::string parseAlgorithmIdentifier(CBS& cbs); /** * @param cbs a CBS& that refers to an ASN.1 OCTETSTRING element - * @returns std::string of the octets in |cbs| - * @throws Envoy::EnvoyException if |cbs| does not point to a well-formed + * @returns std::string of the octets in `cbs` + * @throws Envoy::EnvoyException if `cbs` does not point to a well-formed * OCTETSTRING */ static std::string parseOctetString(CBS& cbs); @@ -154,17 +154,17 @@ class Asn1Utility { * * @param cbs a CBS& that refers to an ASN.1 BITSTRING element * @returns std::vector of the bitstring packed into bytes. - * @throws Envoy::EnvoyException if |cbs| does not point to a well-formed BITSTRING + * @throws Envoy::EnvoyException if `cbs` does not point to a well-formed BITSTRING */ static std::vector parseBitString(CBS& cbs); /** - * Advance |cbs| over an ASN.1 value of the class |tag| if that - * value is present. Otherwise, |cbs| stays in the same position. + * Advance `cbs` over an ASN.1 value of the class |tag| if that + * value is present. Otherwise, `cbs` stays in the same position. * * @param cbs a CBS& that refers to the current document position * @param tag the tag of the value to skip - * @throws Envoy::EnvoyException if |cbs| is a malformed TLV bytestring + * @throws Envoy::EnvoyException if `cbs` is a malformed TLV bytestring */ static void skipOptional(CBS& cbs, unsigned tag); }; diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc index 43aa82735223..b5b3aee04089 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc @@ -264,7 +264,7 @@ SingleResponse Asn1OcspUtility::parseSingleResponse(CBS& cbs) { CertId Asn1OcspUtility::parseCertId(CBS& cbs) { // CertID ::= SEQUENCE { // hashAlgorithm AlgorithmIdentifier, - // issuerNameHash OCTET STRING, -- Hash of issuer's DN + // issuerNameHash OCTET STRING, -- Hash of issuer's `DN` // issuerKeyHash OCTET STRING, -- Hash of issuer's public key // serialNumber CertificateSerialNumber // } diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.h b/source/extensions/transport_sockets/tls/ocsp/ocsp.h index 8bbb848a42f8..af283ad31cdf 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.h +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.h @@ -12,7 +12,7 @@ #include "openssl/ssl.h" /** - * Data structures and functions for unmarshalling OCSP responses + * Data structures and functions for unmarshaling OCSP responses * according to the RFC6960 B.2 spec. See: https://tools.ietf.org/html/rfc6960#appendix-B */ @@ -245,22 +245,22 @@ using OcspResponseWrapperPtr = std::unique_ptr; * ASN.1 DER-encoded parsing functions similar to Asn1Utility but specifically * for structures related to OCSP. * - * Each function must advance |cbs| across the element it refers to. + * Each function must advance `cbs` across the element it refers to. */ class Asn1OcspUtility { public: /** * @param cbs a CBS& that refers to an ASN.1 OcspResponse element - * @returns std::unique_ptr the OCSP response encoded in |cbs| - * @throws Envoy::EnvoyException if |cbs| does not contain a well-formed OcspResponse + * @returns std::unique_ptr the OCSP response encoded in `cbs` + * @throws Envoy::EnvoyException if `cbs` does not contain a well-formed OcspResponse * element. */ static std::unique_ptr parseOcspResponse(CBS& cbs); /** * @param cbs a CBS& that refers to an ASN.1 OcspResponseStatus element - * @returns OcspResponseStatus the OCSP response encoded in |cbs| - * @throws Envoy::EnvoyException if |cbs| does not contain a well-formed + * @returns OcspResponseStatus the OCSP response encoded in `cbs` + * @throws Envoy::EnvoyException if `cbs` does not contain a well-formed * OcspResponseStatus element. */ static OcspResponseStatus parseResponseStatus(CBS& cbs); @@ -268,7 +268,7 @@ class Asn1OcspUtility { /** * @param cbs a CBS& that refers to an ASN.1 Response element * @returns Response containing the content of an OCSP response - * @throws Envoy::EnvoyException if |cbs| does not contain a well-formed + * @throws Envoy::EnvoyException if `cbs` does not contain a well-formed * structure that is a valid Response type. */ static ResponsePtr parseResponseBytes(CBS& cbs); @@ -276,7 +276,7 @@ class Asn1OcspUtility { /** * @param cbs a CBS& that refers to an ASN.1 BasicOcspResponse element * @returns BasicOcspResponse containing the content of an OCSP response - * @throws Envoy::EnvoyException if |cbs| does not contain a well-formed + * @throws Envoy::EnvoyException if `cbs` does not contain a well-formed * BasicOcspResponse element. */ static std::unique_ptr parseBasicOcspResponse(CBS& cbs); @@ -285,7 +285,7 @@ class Asn1OcspUtility { * @param cbs a CBS& that refers to an ASN.1 ResponseData element * @returns ResponseData containing the content of an OCSP response relating * to certificate statuses. - * @throws Envoy::EnvoyException if |cbs| does not contain a well-formed + * @throws Envoy::EnvoyException if `cbs` does not contain a well-formed * ResponseData element. */ static ResponseData parseResponseData(CBS& cbs); @@ -294,7 +294,7 @@ class Asn1OcspUtility { * @param cbs a CBS& that refers to an ASN.1 SingleResponse element * @returns SingleResponse containing the id and revocation status of * a single certificate. - * @throws Envoy::EnvoyException if |cbs| does not contain a well-formed + * @throws Envoy::EnvoyException if `cbs` does not contain a well-formed * SingleResponse element. */ static SingleResponse parseSingleResponse(CBS& cbs); @@ -303,7 +303,7 @@ class Asn1OcspUtility { * @param cbs a CBS& that refers to an ASN.1 CertId element * @returns CertId containing the information necessary to uniquely identify * an SSL certificate. - * @throws Envoy::EnvoyException if |cbs| does not contain a well-formed + * @throws Envoy::EnvoyException if `cbs` does not contain a well-formed * CertId element. */ static CertId parseCertId(CBS& cbs); @@ -311,7 +311,7 @@ class Asn1OcspUtility { /** * @param cbs a CBS& that refers to an ASN.1 CertStatus element * @returns CertStatus the revocation status of a given certificate. - * @throws Envoy::EnvoyException if |cbs| does not contain a well-formed + * @throws Envoy::EnvoyException if `cbs` does not contain a well-formed * CertStatus element. */ static CertStatus parseCertStatus(CBS& cbs); diff --git a/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc b/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc index 9ceb59537628..28c721f25e02 100644 --- a/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc +++ b/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc @@ -186,7 +186,7 @@ TEST_F(Asn1OcspUtilityTest, ParseResponseDataBadResponderIdVariantTest) { 0, 1, 0, - // Invalid Responeder ID tag 3 + // Invalid Responder ID tag 3 3, 1, 0, diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index 64e5d768f114..322a0849ad25 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -15,6 +15,7 @@ API ARN ASAN ASCII +ASN ASSERTs AST AWS @@ -67,6 +68,7 @@ DNS DQUOTE DRYs DS +DSA DST DW EADDRINUSE @@ -119,6 +121,7 @@ GCE GCM GCOVR GCP +GENERALIZEDTIME GETting GLB GOAWAY @@ -210,6 +213,7 @@ NUL Nilsson Nonhashable OCSP +OID OK OOM OOMs @@ -417,8 +421,10 @@ bazel behaviour benchmarked bidi +bignum bitfield bitset +bitstring bitwise blackhole blackholed @@ -437,6 +443,7 @@ bulkstrings bursty bytecode bytestream +bytestring cacheable cacheability callee @@ -807,6 +814,7 @@ num numkeys observability ocagent +octetstring offsetof oneof oneway From 3e7d7bca65bb365c538271691964f52efb9f34e4 Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Mon, 27 Jul 2020 14:28:12 -0700 Subject: [PATCH 03/31] clang_tidy fix and undo deletion in BUILD Signed-off-by: Daniel Goldstein --- api/BUILD | 1 + .../tls/ocsp/asn1_utility.cc | 6 +++-- .../transport_sockets/tls/ocsp/asn1_utility.h | 22 +++++++++---------- .../transport_sockets/tls/ocsp/ocsp.cc | 12 +++++----- .../transport_sockets/tls/ocsp/ocsp.h | 12 +++++----- .../tls/ocsp/asn1_utility_test.cc | 2 +- .../transport_sockets/tls/ocsp/ocsp_test.cc | 20 ++++++++--------- 7 files changed, 39 insertions(+), 36 deletions(-) diff --git a/api/BUILD b/api/BUILD index 3317f5a64c55..8c608fdeca4a 100644 --- a/api/BUILD +++ b/api/BUILD @@ -246,6 +246,7 @@ proto_library( "//envoy/service/discovery/v3:pkg", "//envoy/service/endpoint/v3:pkg", "//envoy/service/event_reporting/v3:pkg", + "//envoy/service/extension/v3:pkg", "//envoy/service/health/v3:pkg", "//envoy/service/listener/v3:pkg", "//envoy/service/load_stats/v3:pkg", diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc index fcb52b066532..051a446c138d 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc @@ -72,7 +72,7 @@ std::string Asn1Utility::parseInteger(CBS& cbs) { } auto head = CBS_data(&num); - const ASN1_INTEGER* asn1_serial_number = c2i_ASN1_INTEGER(nullptr, &head, CBS_len(&num)); + ASN1_INTEGER* asn1_serial_number = c2i_ASN1_INTEGER(nullptr, &head, CBS_len(&num)); if (asn1_serial_number != nullptr) { BIGNUM num_bn; BN_init(&num_bn); @@ -80,7 +80,9 @@ std::string Asn1Utility::parseInteger(CBS& cbs) { char* char_serial_number = BN_bn2hex(&num_bn); BN_free(&num_bn); - M_ASN1_INTEGER_free(asn1_serial_number); + // M_ASN1_INTEGER_free performs a c-style cast which the linters don't + // like, so we're doing the equivalent here with a static_cast + ASN1_STRING_free(static_cast(asn1_serial_number)); if (char_serial_number != nullptr) { std::string serial_number(char_serial_number); diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h index 6436360726bf..52ce2d1675dc 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h @@ -58,30 +58,30 @@ class Asn1Utility { static std::string cbsToString(CBS& cbs); /** - * Parses all elements of an ASN.1 SEQUENCE OF. |parseElement| must + * Parses all elements of an ASN.1 SEQUENCE OF. |parse_element| must * advance its input CBS& over the entire element. * * @param cbs a CBS& that refers to an ASN.1 SEQUENCE OF object - * @param parseElement an Asn1ParsingFunc used to parse each element + * @param parse_element an Asn1ParsingFunc used to parse each element * @returns std::vector containing the parsed elements of the sequence. * @throws Envoy::EnvoyException if `cbs` does not point to a well-formed * SEQUENCE OF object. */ template - static std::vector parseSequenceOf(CBS& cbs, Asn1ParsingFunc parseElement); + static std::vector parseSequenceOf(CBS& cbs, Asn1ParsingFunc parse_element); /** * Checks if an explicitly tagged optional element of |tag| is present and - * if so parses its value with |parseData|. If the element is not present, + * if so parses its value with |parse_data|. If the element is not present, * `cbs` is not advanced. * * @param cbs a CBS& that refers to the current document position - * @param parseData an Asn1ParsingFunc used to parse the data if present + * @param parse_data an Asn1ParsingFunc used to parse the data if present * @return absl::optional with a T if `cbs` is of the specified tag, * else nullopt */ template - static absl::optional parseOptional(CBS& cbs, Asn1ParsingFunc parseData, unsigned tag); + static absl::optional parseOptional(CBS& cbs, Asn1ParsingFunc parse_data, unsigned tag); /** * Returns whether or not an element explicitly tagged with |tag| is present @@ -170,7 +170,7 @@ class Asn1Utility { }; template -std::vector Asn1Utility::parseSequenceOf(CBS& cbs, Asn1ParsingFunc parseElement) { +std::vector Asn1Utility::parseSequenceOf(CBS& cbs, Asn1ParsingFunc parse_element) { CBS seq_elem; std::vector vec; @@ -180,8 +180,8 @@ std::vector Asn1Utility::parseSequenceOf(CBS& cbs, Asn1ParsingFunc parseEl } while (CBS_data(&seq_elem) < CBS_data(&cbs)) { - // parseElement MUST advance seq_elem - vec.push_back(parseElement(seq_elem)); + // parse_element MUST advance seq_elem + vec.push_back(parse_element(seq_elem)); } RELEASE_ASSERT(CBS_data(&cbs) == CBS_data(&seq_elem), @@ -191,10 +191,10 @@ std::vector Asn1Utility::parseSequenceOf(CBS& cbs, Asn1ParsingFunc parseEl } template -absl::optional Asn1Utility::parseOptional(CBS& cbs, Asn1ParsingFunc parseData, unsigned tag) { +absl::optional Asn1Utility::parseOptional(CBS& cbs, Asn1ParsingFunc parse_data, unsigned tag) { CBS data; if (isOptionalPresent(cbs, &data, tag)) { - return parseData(data); + return parse_data(data); } return absl::nullopt; diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc index b5b3aee04089..b61c36532af8 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc @@ -152,17 +152,17 @@ OcspResponseStatus Asn1OcspUtility::parseResponseStatus(CBS& cbs) { auto status_ordinal = *CBS_data(&status); switch (status_ordinal) { case 0: - return OcspResponseStatus::SUCCESSFUL; + return OcspResponseStatus::Successful; case 1: - return OcspResponseStatus::MALFORMED_REQUEST; + return OcspResponseStatus::MalformedRequest; case 2: - return OcspResponseStatus::INTERNAL_ERROR; + return OcspResponseStatus::InternalError; case 3: - return OcspResponseStatus::TRY_LATER; + return OcspResponseStatus::TryLater; case 5: - return OcspResponseStatus::SIG_REQUIRED; + return OcspResponseStatus::SigRequired; case 6: - return OcspResponseStatus::UNAUTHORIZED; + return OcspResponseStatus::Unauthorized; default: throw EnvoyException(absl::StrCat("Unknown OCSP Response Status variant: ", status_ordinal)); } diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.h b/source/extensions/transport_sockets/tls/ocsp/ocsp.h index af283ad31cdf..160941789666 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.h +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.h @@ -36,12 +36,12 @@ enum class OcspResponseStatus { // sigRequired (5), -- Must sign the request // unauthorized (6) -- Request unauthorized // } - SUCCESSFUL = 0, - MALFORMED_REQUEST = 1, - INTERNAL_ERROR = 2, - TRY_LATER = 3, - SIG_REQUIRED = 5, - UNAUTHORIZED = 6 + Successful = 0, + MalformedRequest = 1, + InternalError = 2, + TryLater = 3, + SigRequired = 5, + Unauthorized = 6 }; /** diff --git a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc index 252d3e21ae98..752da6aa40fb 100644 --- a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc +++ b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc @@ -255,7 +255,7 @@ TEST_F(Asn1UtilityTest, ParseIntegerTest) { CBS cbs; uint8_t* buf; size_t buf_len; - for (auto int_and_hex : integers) { + for (auto const& int_and_hex : integers) { ASSERT_TRUE(CBB_init(cbb.get(), 0)); ASSERT_TRUE(CBB_add_asn1_int64(cbb.get(), int_and_hex.first)); ASSERT_TRUE(CBB_finish(cbb.get(), &buf, &buf_len)); diff --git a/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc b/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc index 28c721f25e02..2d247faaa26f 100644 --- a/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc +++ b/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc @@ -42,7 +42,7 @@ class OcspFullResponseParsingTest : public testing::Test { } void expectCertStatus(CertStatus expected_status) { - EXPECT_EQ(OcspResponseStatus::SUCCESSFUL, response_->getResponseStatus()); + EXPECT_EQ(OcspResponseStatus::Successful, response_->getResponseStatus()); EXPECT_EQ(expected_status, response_->getCertRevocationStatus()); } @@ -114,7 +114,7 @@ TEST_F(OcspFullResponseParsingTest, NoResponseBodyTest) { std::vector data = { // SEQUENCE 0x30, 3, - // OcspResponseStatus - INTERNAL_ERROR + // OcspResponseStatus - InternalError 0xau, 1, 2, // no response bytes }; @@ -160,12 +160,12 @@ class Asn1OcspUtilityTest : public testing::Test { }; TEST_F(Asn1OcspUtilityTest, ParseResponseStatusTest) { - expectResponseStatus(0, OcspResponseStatus::SUCCESSFUL); - expectResponseStatus(1, OcspResponseStatus::MALFORMED_REQUEST); - expectResponseStatus(2, OcspResponseStatus::INTERNAL_ERROR); - expectResponseStatus(3, OcspResponseStatus::TRY_LATER); - expectResponseStatus(5, OcspResponseStatus::SIG_REQUIRED); - expectResponseStatus(6, OcspResponseStatus::UNAUTHORIZED); + expectResponseStatus(0, OcspResponseStatus::Successful); + expectResponseStatus(1, OcspResponseStatus::MalformedRequest); + expectResponseStatus(2, OcspResponseStatus::InternalError); + expectResponseStatus(3, OcspResponseStatus::TryLater); + expectResponseStatus(5, OcspResponseStatus::SigRequired); + expectResponseStatus(6, OcspResponseStatus::Unauthorized); } TEST_F(Asn1OcspUtilityTest, ParseMethodWrongTagTest) { @@ -201,14 +201,14 @@ TEST_F(Asn1OcspUtilityTest, ParseOcspResponseBytesMissingTest) { std::vector data = { // SEQUENCE 0x30, 3, - // OcspResponseStatus - INTERNAL_ERROR + // OcspResponseStatus - InternalError 0xau, 1, 2, // no response bytes }; CBS cbs; CBS_init(&cbs, data.data(), data.size()); auto response = Asn1OcspUtility::parseOcspResponse(cbs); - EXPECT_EQ(response->status_, OcspResponseStatus::INTERNAL_ERROR); + EXPECT_EQ(response->status_, OcspResponseStatus::InternalError); EXPECT_TRUE(response->response_ == nullptr); } From 3d2aecc2782ed6a1a3a4a431bc7e4138a1f8d2af Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Mon, 27 Jul 2020 14:59:24 -0700 Subject: [PATCH 04/31] long line fix Signed-off-by: Daniel Goldstein --- source/extensions/transport_sockets/tls/ocsp/asn1_utility.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h index 52ce2d1675dc..89a3c3681005 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h @@ -191,7 +191,8 @@ std::vector Asn1Utility::parseSequenceOf(CBS& cbs, Asn1ParsingFunc parse_e } template -absl::optional Asn1Utility::parseOptional(CBS& cbs, Asn1ParsingFunc parse_data, unsigned tag) { +absl::optional Asn1Utility::parseOptional(CBS& cbs, Asn1ParsingFunc parse_data, + unsigned tag) { CBS data; if (isOptionalPresent(cbs, &data, tag)) { return parse_data(data); From a07736201a9da4ddc385df98612aab2088ce704a Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Mon, 27 Jul 2020 15:50:00 -0700 Subject: [PATCH 05/31] spell check Signed-off-by: Daniel Goldstein --- source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc index 051a446c138d..060830fa0a77 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc @@ -80,8 +80,7 @@ std::string Asn1Utility::parseInteger(CBS& cbs) { char* char_serial_number = BN_bn2hex(&num_bn); BN_free(&num_bn); - // M_ASN1_INTEGER_free performs a c-style cast which the linters don't - // like, so we're doing the equivalent here with a static_cast + // avoiding M_ASN1_INTEGER_free which performs a c-style cast ASN1_STRING_free(static_cast(asn1_serial_number)); if (char_serial_number != nullptr) { From 94bf777662af34e5bb5ba2710a7ddc453b4251f7 Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Tue, 28 Jul 2020 08:17:21 -0700 Subject: [PATCH 06/31] replace function not implemented in boringssl_fips Signed-off-by: Daniel Goldstein --- .../tls/ocsp/asn1_utility_test.cc | 29 ++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc index 752da6aa40fb..0bb8f88642e0 100644 --- a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc +++ b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc @@ -244,6 +244,33 @@ TEST_F(Asn1UtilityTest, TestParseGeneralizedTimeInvalidTime) { "Error parsing timestamp 20070601Z with format %E4Y%m%d%H%M%SZ"); } +// Taken from +// https://boringssl.googlesource.com/boringssl/+/master/crypto/bytestring/cbb.c#531 +// because boringssl_fips does not yet implement CBB_add_asn1_int64 +void cbbAddAsn1Int64(CBB* cbb, int64_t value) { + if (value >= 0) { + ASSERT_TRUE(CBB_add_asn1_uint64(cbb, value)); + } + + union { + int64_t i; + uint8_t bytes[sizeof(int64_t)]; + } u; + u.i = value; + int start = 7; + // Skip leading sign-extension bytes unless they are necessary. + while (start > 0 && (u.bytes[start] == 0xff && (u.bytes[start - 1] & 0x80))) { + start--; + } + + CBB child; + ASSERT_TRUE(CBB_add_asn1(cbb, &child, CBS_ASN1_INTEGER)); + for (int i = start; i >= 0; i--) { + ASSERT_TRUE(CBB_add_u8(&child, u.bytes[i])); + } + CBB_flush(cbb); +} + TEST_F(Asn1UtilityTest, ParseIntegerTest) { std::vector> integers = { {1, "01"}, @@ -257,7 +284,7 @@ TEST_F(Asn1UtilityTest, ParseIntegerTest) { size_t buf_len; for (auto const& int_and_hex : integers) { ASSERT_TRUE(CBB_init(cbb.get(), 0)); - ASSERT_TRUE(CBB_add_asn1_int64(cbb.get(), int_and_hex.first)); + cbbAddAsn1Int64(cbb.get(), int_and_hex.first); ASSERT_TRUE(CBB_finish(cbb.get(), &buf, &buf_len)); CBS_init(&cbs, buf, buf_len); From 23da9267c8097c07a98f9ab216828522849490b0 Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Tue, 28 Jul 2020 09:28:09 -0700 Subject: [PATCH 07/31] fix spelling pedantic Signed-off-by: Daniel Goldstein --- test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc | 2 +- tools/spelling/spelling_dictionary.txt | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc index 0bb8f88642e0..18f48dcdfbe9 100644 --- a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc +++ b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc @@ -246,7 +246,7 @@ TEST_F(Asn1UtilityTest, TestParseGeneralizedTimeInvalidTime) { // Taken from // https://boringssl.googlesource.com/boringssl/+/master/crypto/bytestring/cbb.c#531 -// because boringssl_fips does not yet implement CBB_add_asn1_int64 +// because boringssl_fips does not yet implement `CBB_add_asn1_int64` void cbbAddAsn1Int64(CBB* cbb, int64_t value) { if (value >= 0) { ASSERT_TRUE(CBB_add_asn1_uint64(cbb, value)); diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index 322a0849ad25..5310dd22d154 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -433,6 +433,7 @@ bool boolean booleans bools +boringssl borks broadcasted buf From 5f904ad4c4f0b64061aef8ede2e83e9414730bf4 Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Tue, 28 Jul 2020 12:57:35 -0700 Subject: [PATCH 08/31] wrap tests in anonymous namespace Signed-off-by: Daniel Goldstein --- .../transport_sockets/tls/ocsp/asn1_utility_test.cc | 4 ++++ test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc index 18f48dcdfbe9..a87ff29c742d 100644 --- a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc +++ b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc @@ -13,6 +13,8 @@ namespace TransportSockets { namespace Tls { namespace Ocsp { +namespace { + class Asn1UtilityTest : public testing::Test { public: // DER encoding of a single TLV ASN.1 element. @@ -360,6 +362,8 @@ TEST_F(Asn1UtilityTest, SkipOptionalMalformedTagTest) { "Failed to parse ASN.1 element tag"); } +} // namespace + } // namespace Ocsp } // namespace Tls } // namespace TransportSockets diff --git a/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc b/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc index 2d247faaa26f..e007f078894d 100644 --- a/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc +++ b/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc @@ -18,6 +18,8 @@ namespace TransportSockets { namespace Tls { namespace Ocsp { +namespace { + namespace CertUtility = Envoy::Extensions::TransportSockets::Tls::Utility; class OcspFullResponseParsingTest : public testing::Test { @@ -284,6 +286,8 @@ TEST_F(Asn1OcspUtilityTest, ParseCertStatusInvalidByteStringTest) { "Failed to parse ASN.1 element tag"); } +} // namespace + } // namespace Ocsp } // namespace Tls } // namespace TransportSockets From bc351d0dcdab846f41f2eae93efc8a9bf00d69f9 Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Mon, 3 Aug 2020 10:00:51 -0700 Subject: [PATCH 09/31] clean up raw pointers and unused signature fields Signed-off-by: Daniel Goldstein --- .../transport_sockets/tls/ocsp/BUILD | 1 + .../tls/ocsp/asn1_utility.cc | 67 ++++++++++--------- .../transport_sockets/tls/ocsp/asn1_utility.h | 27 ++------ .../transport_sockets/tls/ocsp/ocsp.cc | 37 ++++------ .../transport_sockets/tls/ocsp/ocsp.h | 24 +++---- .../tls/ocsp/asn1_utility_test.cc | 40 +++++------ 6 files changed, 84 insertions(+), 112 deletions(-) diff --git a/source/extensions/transport_sockets/tls/ocsp/BUILD b/source/extensions/transport_sockets/tls/ocsp/BUILD index 4d2801a0be7d..8118f5e46f35 100644 --- a/source/extensions/transport_sockets/tls/ocsp/BUILD +++ b/source/extensions/transport_sockets/tls/ocsp/BUILD @@ -29,5 +29,6 @@ envoy_cc_library( deps = [ "//include/envoy/common:time_interface", "//include/envoy/ssl:context_config_interface", + "//source/common/common:c_smart_ptr_lib", ], ) diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc index 060830fa0a77..e482d10ec3d6 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc @@ -1,6 +1,8 @@ #include "extensions/transport_sockets/tls/ocsp/asn1_utility.h" +#include "common/common/c_smart_ptr.h" #include "absl/strings/str_cat.h" +#include "absl/strings/ascii.h" namespace Envoy { namespace Extensions { @@ -8,12 +10,26 @@ namespace TransportSockets { namespace Tls { namespace Ocsp { -std::string Asn1Utility::cbsToString(CBS& cbs) { +namespace { + // A type adapter since OPENSSL_free accepts void*. + void freeOpensslString(char* str) { + OPENSSL_free(str); + } + + // ASN1_INTEGER is a type alias for ASN1_STRING. + // This static_cast is intentional to avoid the + // c-style cast performed in M_ASN1_INTEGER_free. + void freeAsn1Integer(ASN1_INTEGER* integer) { + ASN1_STRING_free(static_cast(integer)); + } +} + +absl::string_view Asn1Utility::cbsToString(CBS& cbs) { auto str_head = reinterpret_cast(CBS_data(&cbs)); return {str_head, CBS_len(&cbs)}; } -bool Asn1Utility::isOptionalPresent(CBS& cbs, CBS* data, unsigned tag) { +bool Asn1Utility::getOptional(CBS& cbs, CBS* data, unsigned tag) { int is_present; if (!CBS_get_optional_asn1(&cbs, data, &is_present, tag)) { throw Envoy::EnvoyException("Failed to parse ASN.1 element tag"); @@ -27,13 +43,12 @@ std::string Asn1Utility::parseOid(CBS& cbs) { if (!CBS_get_asn1(&cbs, &oid, CBS_ASN1_OBJECT)) { throw Envoy::EnvoyException("Input is not a well-formed ASN.1 OBJECT"); } - char* oid_text = CBS_asn1_oid_to_text(&oid); + CSmartPtr oid_text(CBS_asn1_oid_to_text(&oid)); if (oid_text == nullptr) { throw Envoy::EnvoyException("Failed to parse oid"); } - std::string oid_text_str(oid_text); - OPENSSL_free(oid_text); + std::string oid_text_str(oid_text.get()); return oid_text_str; } @@ -49,14 +64,15 @@ Envoy::SystemTime Asn1Utility::parseGeneralizedTime(CBS& cbs) { // Local time or time differential, though a part of the ASN.1 // GENERALIZEDTIME spec, are not supported. // Reference: https://tools.ietf.org/html/rfc5280#section-4.1.2.5.2 - if (time_str.at(time_str.length() - 1) != 'Z') { + if (absl::ascii_toupper(time_str.at(time_str.length() - 1)) != 'Z') { throw Envoy::EnvoyException("GENERALIZEDTIME must be in UTC"); } absl::Time time; - std::string time_format = "%E4Y%m%d%H%M%SZ"; + auto utc_time_str = time_str.substr(0, time_str.length() - 1); + std::string time_format = "%E4Y%m%d%H%M%S"; std::string parse_error; - if (!absl::ParseTime(time_format, time_str, &time, &parse_error)) { + if (!absl::ParseTime(time_format, utc_time_str, &time, &parse_error)) { throw Envoy::EnvoyException(absl::StrCat("Error parsing timestamp ", time_str, " with format ", time_format, ". Error: ", parse_error)); } @@ -72,46 +88,35 @@ std::string Asn1Utility::parseInteger(CBS& cbs) { } auto head = CBS_data(&num); - ASN1_INTEGER* asn1_serial_number = c2i_ASN1_INTEGER(nullptr, &head, CBS_len(&num)); - if (asn1_serial_number != nullptr) { + CSmartPtr asn1_integer(c2i_ASN1_INTEGER(nullptr, &head, CBS_len(&num))); + if (asn1_integer != nullptr) { BIGNUM num_bn; BN_init(&num_bn); - ASN1_INTEGER_to_BN(asn1_serial_number, &num_bn); + ASN1_INTEGER_to_BN(asn1_integer.get(), &num_bn); - char* char_serial_number = BN_bn2hex(&num_bn); + CSmartPtr char_hex_number(BN_bn2hex(&num_bn)); BN_free(&num_bn); - // avoiding M_ASN1_INTEGER_free which performs a c-style cast - ASN1_STRING_free(static_cast(asn1_serial_number)); - - if (char_serial_number != nullptr) { - std::string serial_number(char_serial_number); - OPENSSL_free(char_serial_number); - return serial_number; + if (char_hex_number != nullptr) { + std::string hex_number(char_hex_number.get()); + return hex_number; } } throw Envoy::EnvoyException("Failed to parse ASN.1 INTEGER"); } -std::string Asn1Utility::parseOctetString(CBS& cbs) { +absl::string_view Asn1Utility::parseOctetString(CBS& cbs) { CBS value; if (!CBS_get_asn1(&cbs, &value, CBS_ASN1_OCTETSTRING)) { throw Envoy::EnvoyException("Input is not a well-formed ASN.1 OCTETSTRING"); } + + /* auto data = reinterpret_cast(CBS_data(&value)); */ + /* return {data, data + CBS_len(&value)}; */ return cbsToString(value); } -std::vector Asn1Utility::parseBitString(CBS& cbs) { - CBS value; - if (!CBS_get_asn1(&cbs, &value, CBS_ASN1_BITSTRING)) { - throw Envoy::EnvoyException("Input is not a well-formed ASN.1 BITSTRING"); - } - - auto data = reinterpret_cast(CBS_data(&value)); - return {data, data + CBS_len(&value)}; -} - std::string Asn1Utility::parseAlgorithmIdentifier(CBS& cbs) { // AlgorithmIdentifier ::= SEQUENCE { // algorithm OBJECT IDENTIFIER, @@ -123,7 +128,7 @@ std::string Asn1Utility::parseAlgorithmIdentifier(CBS& cbs) { } return parseOid(elem); - // ignore parameters + // Ignore `parameters`. } void Asn1Utility::skipOptional(CBS& cbs, unsigned tag) { diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h index 89a3c3681005..798c84ba5368 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h @@ -50,12 +50,11 @@ class Asn1Utility { /** * Extracts the full contents of `cbs` as a string. - * This copies the data in `cbs`. * * @param cbs a CBS& that refers to the current document position - * @returns std::string containing the contents of `cbs` + * @returns absl::string_view containing the contents of `cbs` */ - static std::string cbsToString(CBS& cbs); + static absl::string_view cbsToString(CBS& cbs); /** * Parses all elements of an ASN.1 SEQUENCE OF. |parse_element| must @@ -90,13 +89,13 @@ class Asn1Utility { * If `cbs` does not contain |tag|, `cbs` remains at the same position. * * @param cbs a CBS& that refers to the current document position - * @param data a CBS& that is set to the contents of `cbs` + * @param data a CBS* that is set to the contents of `cbs` if present * @param an explicit tag indicating an optional value * * @returns bool whether `cbs` points to an element tagged with |tag| * @throws Envoy::EnvoyException if `cbs` is a malformed TLV bytestring */ - static bool isOptionalPresent(CBS& cbs, CBS* data, unsigned tag); + static bool getOptional(CBS& cbs, CBS* data, unsigned tag); /** * @param cbs a CBS& that refers to an ASN.1 OBJECT IDENTIFIER element @@ -140,23 +139,11 @@ class Asn1Utility { /** * @param cbs a CBS& that refers to an ASN.1 OCTETSTRING element - * @returns std::string of the octets in `cbs` + * @returns absl::string_view of the octets in `cbs` * @throws Envoy::EnvoyException if `cbs` does not point to a well-formed * OCTETSTRING */ - static std::string parseOctetString(CBS& cbs); - - /** - * Parses an ASN.1 BITSTRING into a byte vector. The first byte - * of the vector indicates the number of unused bits at the end of - * the last byte. The second byte up through part of the last byte - * contain the contents of the bit string. - * - * @param cbs a CBS& that refers to an ASN.1 BITSTRING element - * @returns std::vector of the bitstring packed into bytes. - * @throws Envoy::EnvoyException if `cbs` does not point to a well-formed BITSTRING - */ - static std::vector parseBitString(CBS& cbs); + static absl::string_view parseOctetString(CBS& cbs); /** * Advance `cbs` over an ASN.1 value of the class |tag| if that @@ -194,7 +181,7 @@ template absl::optional Asn1Utility::parseOptional(CBS& cbs, Asn1ParsingFunc parse_data, unsigned tag) { CBS data; - if (isOptionalPresent(cbs, &data, tag)) { + if (getOptional(cbs, &data, tag)) { return parse_data(data); } diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc index b61c36532af8..eefbff9cae5b 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc @@ -13,9 +13,6 @@ namespace Ocsp { namespace CertUtility = Envoy::Extensions::TransportSockets::Tls::Utility; -static const unsigned CONS = CBS_ASN1_CONSTRUCTED; -static const unsigned CONT = CBS_ASN1_CONTEXT_SPECIFIC; - namespace { unsigned parseTag(CBS& cbs) { @@ -47,8 +44,8 @@ void skipResponderId(CBS& cbs) { // KeyHash ::= OCTET STRING -- SHA-1 hash of responder's public key // (excluding the tag and length fields) - if (Asn1Utility::isOptionalPresent(cbs, nullptr, CONS | CONT | 1) || - Asn1Utility::isOptionalPresent(cbs, nullptr, CONS | CONT | 2)) { + if (Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 1) || + Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 2)) { return; } @@ -60,9 +57,7 @@ void skipResponderId(CBS& cbs) { OcspResponse::OcspResponse(OcspResponseStatus status, ResponsePtr&& response) : status_(status), response_(std::move(response)) {} -BasicOcspResponse::BasicOcspResponse(ResponseData data, std::string signature_alg, - std::vector signature) - : data_(data), signature_alg_(signature_alg), signature_(std::move(signature)) {} +BasicOcspResponse::BasicOcspResponse(ResponseData data) : data_(data) {} const std::string BasicOcspResponse::OID = "1.3.6.1.5.5.7.48.1.1"; @@ -74,8 +69,8 @@ SingleResponse::SingleResponse(CertId cert_id, CertStatus status, Envoy::SystemT absl::optional next_update) : cert_id_(cert_id), status_(status), this_update_(this_update), next_update_(next_update) {} -CertId::CertId(std::string serial_number, std::string alg_oid, std::string issuer_name_hash, - std::string issuer_public_key_hash) +CertId::CertId(std::string serial_number, std::string alg_oid, absl::string_view issuer_name_hash, + absl::string_view issuer_public_key_hash) : serial_number_(serial_number), alg_oid_(alg_oid), issuer_name_hash_(issuer_name_hash), issuer_public_key_hash_(issuer_public_key_hash) {} @@ -127,7 +122,7 @@ std::unique_ptr Asn1OcspUtility::parseOcspResponse(CBS& cbs) { CBS bytes; ResponsePtr resp = nullptr; - if (Asn1Utility::isOptionalPresent(elem, &bytes, CONS | CONT | 0)) { + if (Asn1Utility::getOptional(elem, &bytes, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0)) { resp = Asn1OcspUtility::parseResponseBytes(bytes); } @@ -205,15 +200,11 @@ std::unique_ptr Asn1OcspUtility::parseBasicOcspResponse(CBS& throw EnvoyException("OCSP BasicOCSPResponse is not a wellf-formed ASN.1 SEQUENCE"); } auto response_data = Asn1OcspUtility::parseResponseData(elem); - auto signature_alg = Asn1Utility::parseAlgorithmIdentifier(elem); - auto signature = Asn1Utility::parseBitString(elem); - // TODO(daniel-goldstein): Verify this signature - // "The value for signature SHALL be computed on the hash of the DER - // encoding of ResponseData." - - // optional additional certs are ignored. + // The `signatureAlgorithm` and `signature` are ignored because OCSP + // responses are expected to be delivered from a reliable source. + // Optional additional certs are ignored. - return std::make_unique(response_data, signature_alg, std::move(signature)); + return std::make_unique(response_data); } ResponseData Asn1OcspUtility::parseResponseData(CBS& cbs) { @@ -255,7 +246,7 @@ SingleResponse Asn1OcspUtility::parseSingleResponse(CBS& cbs) { auto status = Asn1OcspUtility::parseCertStatus(elem); auto this_update = Asn1Utility::parseGeneralizedTime(elem); auto next_update = Asn1Utility::parseOptional( - elem, Asn1Utility::parseGeneralizedTime, CONS | CONT | 0); + elem, Asn1Utility::parseGeneralizedTime, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0); // Extensions currently ignored return {cert_id, status, this_update, next_update}; @@ -287,13 +278,13 @@ CertStatus Asn1OcspUtility::parseCertStatus(CBS& cbs) { // revoked [1] IMPLICIT RevokedInfo, // unknown [2] IMPLICIT UnknownInfo // } - if (Asn1Utility::isOptionalPresent(cbs, nullptr, CONT | 0)) { + if (Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONTEXT_SPECIFIC | 0)) { return CertStatus::GOOD; } - if (Asn1Utility::isOptionalPresent(cbs, nullptr, CONS | CONT | 1)) { + if (Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 1)) { return CertStatus::REVOKED; } - if (Asn1Utility::isOptionalPresent(cbs, nullptr, CONT | 2)) { + if (Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONTEXT_SPECIFIC | 2)) { return CertStatus::UNKNOWN; } diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.h b/source/extensions/transport_sockets/tls/ocsp/ocsp.h index 160941789666..d8d0a1f6ffd5 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.h +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.h @@ -8,6 +8,7 @@ #include "envoy/common/time.h" #include "absl/types/optional.h" +#include "absl/strings/string_view.h" #include "openssl/bytestring.h" #include "openssl/ssl.h" @@ -64,13 +65,13 @@ enum class CertStatus { * unique per issuer but not necessarily universally. */ struct CertId { - CertId(std::string serial_number, std::string alg_oid, std::string issuer_name_hash, - std::string issuer_public_key_hash); + CertId(std::string serial_number, std::string alg_oid, absl::string_view issuer_name_hash, + absl::string_view issuer_public_key_hash); std::string serial_number_; std::string alg_oid_; - std::string issuer_name_hash_; - std::string issuer_public_key_hash_; + absl::string_view issuer_name_hash_; + absl::string_view issuer_public_key_hash_; }; /** @@ -107,8 +108,8 @@ struct ResponseData { }; /** - * An abstract type for OCSP response formats. Which variant of |Response| is - * used in an |OcspResponse| is indicated by the structure's OID. + * An abstract type for OCSP response formats. Which variant of `Response` is + * used in an `OcspResponse` is indicated by the structure's OID. * * We currently enforce that OCSP responses must be for a single certificate * only. The methods on this class extract the relevant information for the @@ -140,7 +141,7 @@ class Response { /** * The time at which this response is considered to expire. If - * |nullopt|, then there is assumed to always be more up-to-date + * `nullopt`, then there is assumed to always be more up-to-date * information available and the response is always considered expired. * * @return The end of the validity window for this response. @@ -152,14 +153,13 @@ using ResponsePtr = std::unique_ptr; /** * Reflection of the ASN.1 BasicOcspResponse structure. - * Contains the full data of an OCSP response and a signature/signature - * algorithm to verify the OCSP responder. + * Contains the full data of an OCSP response. * * BasicOcspResponse is the only supported Response type in RFC 6960. */ class BasicOcspResponse : public Response { public: - BasicOcspResponse(ResponseData data, std::string signature_alg, std::vector signature); + BasicOcspResponse(ResponseData data); // Response size_t getNumCerts() override { return data_.single_responses_.size(); } @@ -178,8 +178,6 @@ class BasicOcspResponse : public Response { private: const ResponseData data_; - const std::string signature_alg_; - const std::vector signature_; }; /** @@ -219,7 +217,7 @@ class OcspResponseWrapper { /** * @param cert a X509& SSL certificate - * @returns bool whether this OCSP response contains the revocation status of |cert| + * @returns bool whether this OCSP response contains the revocation status of `cert` */ bool matchesCertificate(X509& cert); diff --git a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc index a87ff29c742d..b783b59faa3c 100644 --- a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc +++ b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc @@ -49,20 +49,19 @@ class Asn1UtilityTest : public testing::Test { TEST_F(Asn1UtilityTest, ParseMethodsWrongTagTest) { expectThrowOnWrongTag( - [](CBS& cbs) { Asn1Utility::parseSequenceOf(cbs, [](CBS&) { return ""; }); }); + [](CBS& cbs) { Asn1Utility::parseSequenceOf(cbs, [](CBS&) { return ""; }); }); expectThrowOnWrongTag(Asn1Utility::parseOid); expectThrowOnWrongTag(Asn1Utility::parseGeneralizedTime); expectThrowOnWrongTag(Asn1Utility::parseInteger); expectThrowOnWrongTag(Asn1Utility::parseOctetString); - expectThrowOnWrongTag(Asn1Utility::parseBitString); expectThrowOnWrongTag(Asn1Utility::parseAlgorithmIdentifier); } TEST_F(Asn1UtilityTest, ToStringTest) { CBS cbs; - std::string data = "test"; - CBS_init(&cbs, reinterpret_cast(data.c_str()), data.size()); - EXPECT_EQ(data, Asn1Utility::cbsToString(cbs)); + absl::string_view str = "test"; + CBS_init(&cbs, reinterpret_cast(str.data()), str.size()); + EXPECT_EQ(str, Asn1Utility::cbsToString(cbs)); } TEST_F(Asn1UtilityTest, ParseSequenceOfEmptySequenceTest) { @@ -98,8 +97,8 @@ TEST_F(Asn1UtilityTest, ParseSequenceOfMultipleElementSequenceTest) { CBS cbs; CBS_init(&cbs, octet_seq.data(), octet_seq.size()); - std::vector vec = {"\x1\x2", "\x3\x4", "\x5\x6"}; - auto actual = Asn1Utility::parseSequenceOf(cbs, Asn1Utility::parseOctetString); + std::vector vec = {"\x1\x2", "\x3\x4", "\x5\x6"}; + auto actual = Asn1Utility::parseSequenceOf(cbs, Asn1Utility::parseOctetString); EXPECT_EQ(vec, actual); } @@ -118,7 +117,7 @@ TEST_F(Asn1UtilityTest, SequenceOfLengthMismatchErrorTest) { CBS_init(&cbs, malformed.data(), malformed.size()); EXPECT_THROW_WITH_MESSAGE( - Asn1Utility::parseSequenceOf(cbs, Asn1Utility::parseOctetString), EnvoyException, + Asn1Utility::parseSequenceOf(cbs, Asn1Utility::parseOctetString), EnvoyException, "Input is not a well-formed ASN.1 OCTETSTRING"); } @@ -141,7 +140,7 @@ TEST_F(Asn1UtilityTest, SequenceOfMixedTypeErrorTest) { CBS_init(&cbs, mixed_type.data(), mixed_type.size()); EXPECT_THROW_WITH_MESSAGE( - Asn1Utility::parseSequenceOf(cbs, Asn1Utility::parseOctetString), EnvoyException, + Asn1Utility::parseSequenceOf(cbs, Asn1Utility::parseOctetString), EnvoyException, "Input is not a well-formed ASN.1 OCTETSTRING"); } @@ -150,10 +149,10 @@ TEST_F(Asn1UtilityTest, IsOptionalPresentTest) { CBS_init(&cbs, asn1_true.data(), asn1_true.size()); const uint8_t* start = CBS_data(&cbs); - EXPECT_FALSE(Asn1Utility::isOptionalPresent(cbs, nullptr, CBS_ASN1_INTEGER)); + EXPECT_FALSE(Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_INTEGER)); EXPECT_EQ(start, CBS_data(&cbs)); - EXPECT_TRUE(Asn1Utility::isOptionalPresent(cbs, &value, CBS_ASN1_BOOLEAN)); + EXPECT_TRUE(Asn1Utility::getOptional(cbs, &value, CBS_ASN1_BOOLEAN)); EXPECT_EQ(0xff, *CBS_data(&value)); } @@ -162,7 +161,7 @@ TEST_F(Asn1UtilityTest, IsOptionalPresentMissingValueTest) { CBS cbs, value; CBS_init(&cbs, missing_val_bool.data(), missing_val_bool.size()); - EXPECT_THROW_WITH_MESSAGE(Asn1Utility::isOptionalPresent(cbs, &value, CBS_ASN1_BOOLEAN), + EXPECT_THROW_WITH_MESSAGE(Asn1Utility::getOptional(cbs, &value, CBS_ASN1_BOOLEAN), EnvoyException, "Failed to parse ASN.1 element tag"); } @@ -219,11 +218,12 @@ TEST_F(Asn1UtilityTest, ParseGeneralizedTimeWrongFormatErrorTest) { } TEST_F(Asn1UtilityTest, ParseGeneralizedTimeTest) { - std::string time = "20070614185900Z"; + std::string time = "20070614185900z"; + std::string expected_time = "20070614185900"; CBS cbs; bssl::UniquePtr scoped(asn1Encode(cbs, time, CBS_ASN1_GENERALIZEDTIME)); - absl::Time expected = TestUtility::parseTime(time, "%E4Y%m%d%H%M%SZ"); + absl::Time expected = TestUtility::parseTime(expected_time, "%E4Y%m%d%H%M%S"); EXPECT_EQ(absl::ToChronoTime(expected), Asn1Utility::parseGeneralizedTime(cbs)); } @@ -243,7 +243,7 @@ TEST_F(Asn1UtilityTest, TestParseGeneralizedTimeInvalidTime) { bssl::UniquePtr scoped(asn1Encode(cbs, ymd, CBS_ASN1_GENERALIZEDTIME)); EXPECT_THROW_WITH_REGEX(Asn1Utility::parseGeneralizedTime(cbs), EnvoyException, - "Error parsing timestamp 20070601Z with format %E4Y%m%d%H%M%SZ"); + "Error parsing timestamp 20070601Z with format %E4Y%m%d%H%M%S"); } // Taken from @@ -305,16 +305,6 @@ TEST_F(Asn1UtilityTest, ParseOctetStringTest) { EXPECT_EQ(data, Asn1Utility::parseOctetString(cbs)); } -TEST_F(Asn1UtilityTest, ParseBitStringTest) { - std::vector data = {0, 1, 2, 3}; - std::vector tlv = {0x3u, 4}; - tlv.insert(tlv.end(), data.begin(), data.end()); - - CBS cbs; - CBS_init(&cbs, tlv.data(), tlv.size()); - EXPECT_EQ(data, Asn1Utility::parseBitString(cbs)); -} - TEST_F(Asn1UtilityTest, ParseAlgorithmIdentifierTest) { std::string sha256 = "2.16.840.1.101.3.4.2.1"; From ab88172adfb966f12173316202b8f9952bb680e0 Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Mon, 3 Aug 2020 10:20:34 -0700 Subject: [PATCH 10/31] remove unused produced_at field Signed-off-by: Daniel Goldstein --- .../transport_sockets/tls/ocsp/asn1_utility.cc | 6 ++++++ .../transport_sockets/tls/ocsp/asn1_utility.h | 10 ++++++++++ .../extensions/transport_sockets/tls/ocsp/ocsp.cc | 13 +++++-------- source/extensions/transport_sockets/tls/ocsp/ocsp.h | 10 +++++----- 4 files changed, 26 insertions(+), 13 deletions(-) diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc index e482d10ec3d6..b170dc2225bf 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc @@ -137,6 +137,12 @@ void Asn1Utility::skipOptional(CBS& cbs, unsigned tag) { } } +void Asn1Utility::skip(CBS& cbs, unsigned tag) { + if (!CBS_get_asn1(&cbs, nullptr, tag)) { + throw Envoy::EnvoyException("Failed to parse ASN.1 element"); + } +} + } // namespace Ocsp } // namespace Tls } // namespace TransportSockets diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h index 798c84ba5368..f564a20ac1a9 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h @@ -154,6 +154,16 @@ class Asn1Utility { * @throws Envoy::EnvoyException if `cbs` is a malformed TLV bytestring */ static void skipOptional(CBS& cbs, unsigned tag); + + /** + * Advance `cbs` over an ASN.1 value of the class |tag|. + * + * @param cbs a CBS& that refers to the current document position + * @param tag the tag of the value to skip + * @throws Envoy::EnvoyException if `cbs` does not point to a well-formed + * element with tag `tag`. + */ + static void skip(CBS& cbs, unsigned tag); }; template diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc index eefbff9cae5b..6bcf578bd068 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc @@ -54,16 +54,13 @@ void skipResponderId(CBS& cbs) { } // namespace -OcspResponse::OcspResponse(OcspResponseStatus status, ResponsePtr&& response) +OcspResponse::OcspResponse(OcspResponseStatus status, ResponsePtr response) : status_(status), response_(std::move(response)) {} BasicOcspResponse::BasicOcspResponse(ResponseData data) : data_(data) {} -const std::string BasicOcspResponse::OID = "1.3.6.1.5.5.7.48.1.1"; - -ResponseData::ResponseData(Envoy::SystemTime produced_at, - std::vector single_responses) - : produced_at_(produced_at), single_responses_(std::move(single_responses)) {} +ResponseData::ResponseData(std::vector single_responses) + : single_responses_(std::move(single_responses)) {} SingleResponse::SingleResponse(CertId cert_id, CertStatus status, Envoy::SystemTime this_update, absl::optional next_update) @@ -222,11 +219,11 @@ ResponseData Asn1OcspUtility::parseResponseData(CBS& cbs) { Asn1Utility::skipOptional(elem, 0); skipResponderId(elem); - auto produced_at = Asn1Utility::parseGeneralizedTime(elem); + Asn1Utility::skip(elem, CBS_ASN1_GENERALIZEDTIME); auto responses = Asn1Utility::parseSequenceOf(elem, parseSingleResponse); // Extensions currently ignored - return {produced_at, std::move(responses)}; + return {std::move(responses)}; } SingleResponse Asn1OcspUtility::parseSingleResponse(CBS& cbs) { diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.h b/source/extensions/transport_sockets/tls/ocsp/ocsp.h index d8d0a1f6ffd5..309b290a6c8e 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.h +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.h @@ -101,9 +101,8 @@ struct SingleResponse { * as well as the time at which the response was produced. */ struct ResponseData { - ResponseData(Envoy::SystemTime produced_at, std::vector single_responses); + ResponseData(std::vector single_responses); - const Envoy::SystemTime produced_at_; const std::vector single_responses_; }; @@ -174,8 +173,9 @@ class BasicOcspResponse : public Response { return data_.single_responses_[0].next_update_; } - const static std::string OID; - + // Identified as `id-pkix-ocsp-basic` in + // https://tools.ietf.org/html/rfc6960#appendix-B.2 + constexpr static absl::string_view OID = "1.3.6.1.5.5.7.48.1.1"; private: const ResponseData data_; }; @@ -185,7 +185,7 @@ class BasicOcspResponse : public Response { * This is the top-level data structure for OCSP responses. */ struct OcspResponse { - OcspResponse(OcspResponseStatus status, ResponsePtr&& response); + OcspResponse(OcspResponseStatus status, ResponsePtr response); OcspResponseStatus status_; ResponsePtr response_; From 561bdd1dcdf2e38302c3e119c27f2ca89641fdab Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Mon, 3 Aug 2020 10:38:28 -0700 Subject: [PATCH 11/31] change raw bytes from std string to std vector Signed-off-by: Daniel Goldstein --- .../transport_sockets/tls/ocsp/ocsp.cc | 6 ++--- .../transport_sockets/tls/ocsp/ocsp.h | 8 +++---- .../transport_sockets/tls/ocsp/ocsp_test.cc | 22 +++++++++---------- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc index 6bcf578bd068..e410161d3a38 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc @@ -23,9 +23,9 @@ unsigned parseTag(CBS& cbs) { return tag; } -std::unique_ptr readDerEncodedOcspResponse(const std::string& der) { +std::unique_ptr readDerEncodedOcspResponse(const std::vector& der) { CBS cbs; - CBS_init(&cbs, reinterpret_cast(der.c_str()), der.size()); + CBS_init(&cbs, der.data(), der.size()); auto resp = Asn1OcspUtility::parseOcspResponse(cbs); if (CBS_len(&cbs) != 0) { @@ -71,7 +71,7 @@ CertId::CertId(std::string serial_number, std::string alg_oid, absl::string_view : serial_number_(serial_number), alg_oid_(alg_oid), issuer_name_hash_(issuer_name_hash), issuer_public_key_hash_(issuer_public_key_hash) {} -OcspResponseWrapper::OcspResponseWrapper(std::string der_response, TimeSource& time_source) +OcspResponseWrapper::OcspResponseWrapper(std::vector der_response, TimeSource& time_source) : raw_bytes_(std::move(der_response)), response_(readDerEncodedOcspResponse(raw_bytes_)), time_source_(time_source) { diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.h b/source/extensions/transport_sockets/tls/ocsp/ocsp.h index 309b290a6c8e..341b474af064 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.h +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.h @@ -196,13 +196,13 @@ struct OcspResponse { */ class OcspResponseWrapper { public: - OcspResponseWrapper(std::string der_response, TimeSource& time_source); + OcspResponseWrapper(std::vector der_response, TimeSource& time_source); /** - * @return std::string& a reference to the underlying bytestring representation + * @return std::vector& a reference to the underlying bytestring representation * of the OCSP response */ - const std::string& rawBytes() { return raw_bytes_; } + const std::vector& rawBytes() { return raw_bytes_; } /** * @return OcspResponseStatus whether the OCSP response was successfully created @@ -232,7 +232,7 @@ class OcspResponseWrapper { bool isExpired(); private: - const std::string raw_bytes_; + const std::vector raw_bytes_; const std::unique_ptr response_; TimeSource& time_source_; }; diff --git a/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc b/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc index e007f078894d..f3e37f54ec3c 100644 --- a/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc +++ b/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc @@ -33,12 +33,13 @@ class OcspFullResponseParsingTest : public testing::Test { return TestEnvironment::substitute("{{ test_tmpdir }}/ocsp_test_data/" + filename); } - std::string readFile(std::string filename) { - return TestEnvironment::readFileToStringForTest(fullPath(filename)); + std::vector readFile(std::string filename) { + auto str = TestEnvironment::readFileToStringForTest(fullPath(filename)); + return {str.begin(), str.end()}; } void setup(std::string response_filename) { - std::string der_response = readFile(response_filename); + auto der_response = readFile(response_filename); response_ = std::make_unique(der_response, time_system_); EXPECT_EQ(response_->rawBytes(), der_response); } @@ -107,7 +108,7 @@ TEST_F(OcspFullResponseParsingTest, ResponderIdKeyHashTest) { } TEST_F(OcspFullResponseParsingTest, MultiCertResponseTest) { - std::string resp_bytes = readFile("multiple_cert_ocsp_resp.der"); + auto resp_bytes = readFile("multiple_cert_ocsp_resp.der"); EXPECT_THROW_WITH_MESSAGE(OcspResponseWrapper response(resp_bytes, time_system_), EnvoyException, "OCSP Response must be for one certificate only"); } @@ -120,24 +121,23 @@ TEST_F(OcspFullResponseParsingTest, NoResponseBodyTest) { 0xau, 1, 2, // no response bytes }; - std::string byte_string(reinterpret_cast(data.data()), data.size()); - EXPECT_THROW_WITH_MESSAGE(OcspResponseWrapper response(byte_string, time_system_), EnvoyException, + EXPECT_THROW_WITH_MESSAGE(OcspResponseWrapper response(data, time_system_), EnvoyException, "OCSP response has no body"); } TEST_F(OcspFullResponseParsingTest, OnlyOneResponseInByteStringTest) { - auto resp1_bytes = readFile("good_ocsp_resp.der"); + auto resp_bytes = readFile("good_ocsp_resp.der"); auto resp2_bytes = readFile("revoked_ocsp_resp.der"); - auto two_responses = resp1_bytes + resp2_bytes; + resp_bytes.insert(resp_bytes.end(), resp2_bytes.begin(), resp2_bytes.end()); - EXPECT_THROW_WITH_MESSAGE(OcspResponseWrapper response_wrapper(two_responses, time_system_), + EXPECT_THROW_WITH_MESSAGE(OcspResponseWrapper response_wrapper(resp_bytes, time_system_), EnvoyException, "Data contained more than a single OCSP response"); } TEST_F(OcspFullResponseParsingTest, ParseOcspResponseWrongTagTest) { - std::string resp_bytes = readFile("good_ocsp_resp.der"); + auto resp_bytes = readFile("good_ocsp_resp.der"); // Change the SEQUENCE tag to an OCTETSTRING tag - resp_bytes.replace(0, 1, "\x4u"); + resp_bytes[0] = 0x4u; EXPECT_THROW_WITH_MESSAGE(OcspResponseWrapper response_wrapper(resp_bytes, time_system_), EnvoyException, "OCSP Response is not a well-formed ASN.1 SEQUENCE"); } From 33ab2bf8e69ae8a5a91ca95e28c9ea4633801403 Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Mon, 3 Aug 2020 10:49:02 -0700 Subject: [PATCH 12/31] change octet strings to be byte vectors Signed-off-by: Daniel Goldstein --- .../transport_sockets/tls/ocsp/asn1_utility.cc | 8 +++----- .../transport_sockets/tls/ocsp/asn1_utility.h | 2 +- source/extensions/transport_sockets/tls/ocsp/ocsp.cc | 12 ++++++------ source/extensions/transport_sockets/tls/ocsp/ocsp.h | 6 ++---- 4 files changed, 12 insertions(+), 16 deletions(-) diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc index b170dc2225bf..fb12a721a065 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc @@ -105,16 +105,14 @@ std::string Asn1Utility::parseInteger(CBS& cbs) { throw Envoy::EnvoyException("Failed to parse ASN.1 INTEGER"); } -absl::string_view Asn1Utility::parseOctetString(CBS& cbs) { +std::vector Asn1Utility::parseOctetString(CBS& cbs) { CBS value; if (!CBS_get_asn1(&cbs, &value, CBS_ASN1_OCTETSTRING)) { throw Envoy::EnvoyException("Input is not a well-formed ASN.1 OCTETSTRING"); } - - /* auto data = reinterpret_cast(CBS_data(&value)); */ - /* return {data, data + CBS_len(&value)}; */ - return cbsToString(value); + auto data = reinterpret_cast(CBS_data(&value)); + return {data, data + CBS_len(&value)}; } std::string Asn1Utility::parseAlgorithmIdentifier(CBS& cbs) { diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h index f564a20ac1a9..f9ce1138ca14 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h @@ -143,7 +143,7 @@ class Asn1Utility { * @throws Envoy::EnvoyException if `cbs` does not point to a well-formed * OCTETSTRING */ - static absl::string_view parseOctetString(CBS& cbs); + static std::vector parseOctetString(CBS& cbs); /** * Advance `cbs` over an ASN.1 value of the class |tag| if that diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc index e410161d3a38..ab754e65a2af 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc @@ -66,10 +66,8 @@ SingleResponse::SingleResponse(CertId cert_id, CertStatus status, Envoy::SystemT absl::optional next_update) : cert_id_(cert_id), status_(status), this_update_(this_update), next_update_(next_update) {} -CertId::CertId(std::string serial_number, std::string alg_oid, absl::string_view issuer_name_hash, - absl::string_view issuer_public_key_hash) - : serial_number_(serial_number), alg_oid_(alg_oid), issuer_name_hash_(issuer_name_hash), - issuer_public_key_hash_(issuer_public_key_hash) {} +CertId::CertId(std::string serial_number, std::string alg_oid, std::vector issuer_name_hash) + : serial_number_(serial_number), alg_oid_(alg_oid), issuer_name_hash_(issuer_name_hash) {} OcspResponseWrapper::OcspResponseWrapper(std::vector der_response, TimeSource& time_source) : raw_bytes_(std::move(der_response)), response_(readDerEncodedOcspResponse(raw_bytes_)), @@ -261,12 +259,14 @@ CertId Asn1OcspUtility::parseCertId(CBS& cbs) { throw EnvoyException("OCSP CertID is not a well-formed ASN.1 SEQUENCE"); } + // We use just the issuer name + the serial number to uniquely identify + // a certificate. auto alg = Asn1Utility::parseAlgorithmIdentifier(elem); auto issuer_name_hash = Asn1Utility::parseOctetString(elem); - auto issuer_public_key_hash = Asn1Utility::parseOctetString(elem); + Asn1Utility::skip(elem, CBS_ASN1_OCTETSTRING); auto serial_number = Asn1Utility::parseInteger(elem); - return {serial_number, alg, issuer_name_hash, issuer_public_key_hash}; + return {serial_number, alg, issuer_name_hash}; } CertStatus Asn1OcspUtility::parseCertStatus(CBS& cbs) { diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.h b/source/extensions/transport_sockets/tls/ocsp/ocsp.h index 341b474af064..b3552018a251 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.h +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.h @@ -65,13 +65,11 @@ enum class CertStatus { * unique per issuer but not necessarily universally. */ struct CertId { - CertId(std::string serial_number, std::string alg_oid, absl::string_view issuer_name_hash, - absl::string_view issuer_public_key_hash); + CertId(std::string serial_number, std::string alg_oid, std::vector issuer_name_hash); std::string serial_number_; std::string alg_oid_; - absl::string_view issuer_name_hash_; - absl::string_view issuer_public_key_hash_; + std::vector issuer_name_hash_; }; /** From ead48ce2d40f4e81dbb3e0d0aa048882b3a7f38b Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Mon, 3 Aug 2020 12:27:47 -0700 Subject: [PATCH 13/31] warn about minimal verification of OCSP responses Signed-off-by: Daniel Goldstein --- .../tls/ocsp/asn1_utility.cc | 5 ++--- .../transport_sockets/tls/ocsp/asn1_utility.h | 22 ++++++++++--------- .../transport_sockets/tls/ocsp/ocsp.cc | 3 ++- .../transport_sockets/tls/ocsp/ocsp.h | 5 +++++ 4 files changed, 21 insertions(+), 14 deletions(-) diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc index fb12a721a065..3a95e4ee189a 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc @@ -70,11 +70,10 @@ Envoy::SystemTime Asn1Utility::parseGeneralizedTime(CBS& cbs) { absl::Time time; auto utc_time_str = time_str.substr(0, time_str.length() - 1); - std::string time_format = "%E4Y%m%d%H%M%S"; std::string parse_error; - if (!absl::ParseTime(time_format, utc_time_str, &time, &parse_error)) { + if (!absl::ParseTime(GENERALIZED_TIME_FORMAT, utc_time_str, &time, &parse_error)) { throw Envoy::EnvoyException(absl::StrCat("Error parsing timestamp ", time_str, " with format ", - time_format, ". Error: ", parse_error)); + GENERALIZED_TIME_FORMAT, ". Error: ", parse_error)); } return absl::ToChronoTime(time); } diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h index f9ce1138ca14..a5a16372a5f8 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h @@ -20,8 +20,10 @@ namespace TransportSockets { namespace Tls { namespace Ocsp { +constexpr absl::string_view GENERALIZED_TIME_FORMAT = "%E4Y%m%d%H%M%S"; + /** - * Construct a |T| from the data contained in the CBS&. Functions + * Construct a `T` from the data contained in the CBS&. Functions * of this type must advance the input CBS& over the element. */ template using Asn1ParsingFunc = std::function; @@ -57,7 +59,7 @@ class Asn1Utility { static absl::string_view cbsToString(CBS& cbs); /** - * Parses all elements of an ASN.1 SEQUENCE OF. |parse_element| must + * Parses all elements of an ASN.1 SEQUENCE OF. `parse_element` must * advance its input CBS& over the entire element. * * @param cbs a CBS& that refers to an ASN.1 SEQUENCE OF object @@ -70,8 +72,8 @@ class Asn1Utility { static std::vector parseSequenceOf(CBS& cbs, Asn1ParsingFunc parse_element); /** - * Checks if an explicitly tagged optional element of |tag| is present and - * if so parses its value with |parse_data|. If the element is not present, + * Checks if an explicitly tagged optional element of `tag` is present and + * if so parses its value with `parse_data`. If the element is not present, * `cbs` is not advanced. * * @param cbs a CBS& that refers to the current document position @@ -83,16 +85,16 @@ class Asn1Utility { static absl::optional parseOptional(CBS& cbs, Asn1ParsingFunc parse_data, unsigned tag); /** - * Returns whether or not an element explicitly tagged with |tag| is present + * Returns whether or not an element explicitly tagged with `tag` is present * at `cbs`. If so, `cbs` is advanced over the optional and assigns - * |data| to the inner element, if |data| is not nullptr. - * If `cbs` does not contain |tag|, `cbs` remains at the same position. + * `data` to the inner element, if `data` is not nullptr. + * If `cbs` does not contain `tag`, `cbs` remains at the same position. * * @param cbs a CBS& that refers to the current document position * @param data a CBS* that is set to the contents of `cbs` if present * @param an explicit tag indicating an optional value * - * @returns bool whether `cbs` points to an element tagged with |tag| + * @returns bool whether `cbs` points to an element tagged with `tag` * @throws Envoy::EnvoyException if `cbs` is a malformed TLV bytestring */ static bool getOptional(CBS& cbs, CBS* data, unsigned tag); @@ -146,7 +148,7 @@ class Asn1Utility { static std::vector parseOctetString(CBS& cbs); /** - * Advance `cbs` over an ASN.1 value of the class |tag| if that + * Advance `cbs` over an ASN.1 value of the class `tag` if that * value is present. Otherwise, `cbs` stays in the same position. * * @param cbs a CBS& that refers to the current document position @@ -156,7 +158,7 @@ class Asn1Utility { static void skipOptional(CBS& cbs, unsigned tag); /** - * Advance `cbs` over an ASN.1 value of the class |tag|. + * Advance `cbs` over an ASN.1 value of the class `tag`. * * @param cbs a CBS& that refers to the current document position * @param tag the tag of the value to skip diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc index ab754e65a2af..f4eafc4f0033 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc @@ -84,7 +84,8 @@ OcspResponseWrapper::OcspResponseWrapper(std::vector der_response, Time auto& this_update = response_->response_->getThisUpdate(); if (time_source_.systemTime() < this_update) { - DateFormatter formatter("%E4Y%m%d%H%M%S"); + std::string time_format(GENERALIZED_TIME_FORMAT); + DateFormatter formatter(time_format); throw EnvoyException(absl::StrCat("OCSP Response thisUpdate field is set in the future: ", formatter.fromTime(this_update))); } diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.h b/source/extensions/transport_sockets/tls/ocsp/ocsp.h index b3552018a251..be93220c9e84 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.h +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.h @@ -15,6 +15,11 @@ /** * Data structures and functions for unmarshaling OCSP responses * according to the RFC6960 B.2 spec. See: https://tools.ietf.org/html/rfc6960#appendix-B + * + * WARNING: This module is meant to validate that OCSP responses are well-formed + * and extract useful fields for OCSP stapling. This assumes that responses are + * provided from configs or another trusted source and does not perform the + * necessary checks to verify responses coming from an upstream server. */ namespace Envoy { From 1ed6234a17d4cf6b1d8e2acbae4bfcc39168dd0f Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Wed, 5 Aug 2020 17:00:33 -0700 Subject: [PATCH 14/31] change parse generalized time to use result type Signed-off-by: Daniel Goldstein --- .../tls/ocsp/asn1_utility.cc | 13 ++-- .../transport_sockets/tls/ocsp/asn1_utility.h | 7 +- .../transport_sockets/tls/ocsp/ocsp.cc | 15 ++++- .../tls/ocsp/asn1_utility_test.cc | 67 ++++++++++--------- 4 files changed, 59 insertions(+), 43 deletions(-) diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc index 3a95e4ee189a..58cceaa98689 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc @@ -52,10 +52,10 @@ std::string Asn1Utility::parseOid(CBS& cbs) { return oid_text_str; } -Envoy::SystemTime Asn1Utility::parseGeneralizedTime(CBS& cbs) { +ParsingResult Asn1Utility::parseGeneralizedTime(CBS& cbs) { CBS elem; if (!CBS_get_asn1(&cbs, &elem, CBS_ASN1_GENERALIZEDTIME)) { - throw Envoy::EnvoyException("Input is not a well-formed ASN.1 GENERALIZEDTIME"); + return "Input is not a well-formed ASN.1 GENERALIZEDTIME"; } auto time_str = cbsToString(elem); @@ -64,16 +64,17 @@ Envoy::SystemTime Asn1Utility::parseGeneralizedTime(CBS& cbs) { // Local time or time differential, though a part of the ASN.1 // GENERALIZEDTIME spec, are not supported. // Reference: https://tools.ietf.org/html/rfc5280#section-4.1.2.5.2 - if (absl::ascii_toupper(time_str.at(time_str.length() - 1)) != 'Z') { - throw Envoy::EnvoyException("GENERALIZEDTIME must be in UTC"); + if (time_str.length() > 0 && + absl::ascii_toupper(time_str.at(time_str.length() - 1)) != 'Z') { + return "GENERALIZEDTIME must be in UTC"; } absl::Time time; auto utc_time_str = time_str.substr(0, time_str.length() - 1); std::string parse_error; if (!absl::ParseTime(GENERALIZED_TIME_FORMAT, utc_time_str, &time, &parse_error)) { - throw Envoy::EnvoyException(absl::StrCat("Error parsing timestamp ", time_str, " with format ", - GENERALIZED_TIME_FORMAT, ". Error: ", parse_error)); + return absl::StrCat("Error parsing timestamp ", time_str, " with format ", + GENERALIZED_TIME_FORMAT, ". Error: ", parse_error); } return absl::ToChronoTime(time); } diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h index a5a16372a5f8..ce2753bdda1c 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h @@ -10,6 +10,7 @@ #include "common/common/assert.h" #include "absl/types/optional.h" +#include "absl/types/variant.h" #include "openssl/bn.h" #include "openssl/bytestring.h" #include "openssl/ssl.h" @@ -22,11 +23,13 @@ namespace Ocsp { constexpr absl::string_view GENERALIZED_TIME_FORMAT = "%E4Y%m%d%H%M%S"; +template using ParsingResult = absl::variant; + /** * Construct a `T` from the data contained in the CBS&. Functions * of this type must advance the input CBS& over the element. */ -template using Asn1ParsingFunc = std::function; +template using Asn1ParsingFunc = std::function(CBS&)>; /** * Utility functions for parsing DER-encoded ASN.1 objects. @@ -113,7 +116,7 @@ class Asn1Utility { * @throws Envoy::EnvoyException if `cbs` does not point to a well-formed * GENERALIZEDTIME */ - static Envoy::SystemTime parseGeneralizedTime(CBS& cbs); + static ParsingResult parseGeneralizedTime(CBS& cbs); /** * Parses an ASN.1 INTEGER type into its hex string representation. diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc index f4eafc4f0033..7c23dd43fad3 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc @@ -15,6 +15,15 @@ namespace CertUtility = Envoy::Extensions::TransportSockets::Tls::Utility; namespace { +template +T unwrap(ParsingResult res) { + if (absl::holds_alternative(res)) { + return absl::get(res); + } + + throw EnvoyException(absl::get(res)); +} + unsigned parseTag(CBS& cbs) { unsigned tag; if (!CBS_get_any_asn1_element(&cbs, nullptr, &tag, nullptr)) { @@ -240,9 +249,9 @@ SingleResponse Asn1OcspUtility::parseSingleResponse(CBS& cbs) { auto cert_id = Asn1OcspUtility::parseCertId(elem); auto status = Asn1OcspUtility::parseCertStatus(elem); - auto this_update = Asn1Utility::parseGeneralizedTime(elem); - auto next_update = Asn1Utility::parseOptional( - elem, Asn1Utility::parseGeneralizedTime, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0); + auto this_update = unwrap(Asn1Utility::parseGeneralizedTime(elem)); + auto next_update = unwrap(Asn1Utility::parseOptional>( + elem, Asn1Utility::parseGeneralizedTime, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0)); // Extensions currently ignored return {cert_id, status, this_update, next_update}; diff --git a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc index b783b59faa3c..95389836eecb 100644 --- a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc +++ b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc @@ -37,25 +37,25 @@ class Asn1UtilityTest : public testing::Test { return buf; } - void expectThrowOnWrongTag(std::function parse) { - CBS cbs; - CBS_init(&cbs, asn1_true.data(), asn1_true.size()); - EXPECT_THROW(parse(cbs), EnvoyException); - } + /* void expectThrowOnWrongTag(std::function parse) { */ + /* CBS cbs; */ + /* CBS_init(&cbs, asn1_true.data(), asn1_true.size()); */ + /* EXPECT_THROW(parse(cbs), EnvoyException); */ + /* } */ const std::vector asn1_true = {0x1u, 1, 0xff}; const std::vector asn1_empty_seq = {0x30, 0}; }; -TEST_F(Asn1UtilityTest, ParseMethodsWrongTagTest) { - expectThrowOnWrongTag( - [](CBS& cbs) { Asn1Utility::parseSequenceOf(cbs, [](CBS&) { return ""; }); }); - expectThrowOnWrongTag(Asn1Utility::parseOid); - expectThrowOnWrongTag(Asn1Utility::parseGeneralizedTime); - expectThrowOnWrongTag(Asn1Utility::parseInteger); - expectThrowOnWrongTag(Asn1Utility::parseOctetString); - expectThrowOnWrongTag(Asn1Utility::parseAlgorithmIdentifier); -} +/* TEST_F(Asn1UtilityTest, ParseMethodsWrongTagTest) { */ +/* expectThrowOnWrongTag( */ +/* [](CBS& cbs) { Asn1Utility::parseSequenceOf(cbs, [](CBS&) { return ""; }); }); */ +/* expectThrowOnWrongTag(Asn1Utility::parseOid); */ +/* expectThrowOnWrongTag(Asn1Utility::parseGeneralizedTime); */ +/* expectThrowOnWrongTag(Asn1Utility::parseInteger); */ +/* expectThrowOnWrongTag(Asn1Utility::parseOctetString); */ +/* expectThrowOnWrongTag(Asn1Utility::parseAlgorithmIdentifier); */ +/* } */ TEST_F(Asn1UtilityTest, ToStringTest) { CBS cbs; @@ -97,8 +97,8 @@ TEST_F(Asn1UtilityTest, ParseSequenceOfMultipleElementSequenceTest) { CBS cbs; CBS_init(&cbs, octet_seq.data(), octet_seq.size()); - std::vector vec = {"\x1\x2", "\x3\x4", "\x5\x6"}; - auto actual = Asn1Utility::parseSequenceOf(cbs, Asn1Utility::parseOctetString); + std::vector> vec = {{0x1, 0x2}, {0x3, 0x4}, {0x5, 0x6}}; + auto actual = Asn1Utility::parseSequenceOf>(cbs, Asn1Utility::parseOctetString); EXPECT_EQ(vec, actual); } @@ -117,7 +117,7 @@ TEST_F(Asn1UtilityTest, SequenceOfLengthMismatchErrorTest) { CBS_init(&cbs, malformed.data(), malformed.size()); EXPECT_THROW_WITH_MESSAGE( - Asn1Utility::parseSequenceOf(cbs, Asn1Utility::parseOctetString), EnvoyException, + Asn1Utility::parseSequenceOf>(cbs, Asn1Utility::parseOctetString), EnvoyException, "Input is not a well-formed ASN.1 OCTETSTRING"); } @@ -140,7 +140,7 @@ TEST_F(Asn1UtilityTest, SequenceOfMixedTypeErrorTest) { CBS_init(&cbs, mixed_type.data(), mixed_type.size()); EXPECT_THROW_WITH_MESSAGE( - Asn1Utility::parseSequenceOf(cbs, Asn1Utility::parseOctetString), EnvoyException, + Asn1Utility::parseSequenceOf>(cbs, Asn1Utility::parseOctetString), EnvoyException, "Input is not a well-formed ASN.1 OCTETSTRING"); } @@ -213,8 +213,9 @@ TEST_F(Asn1UtilityTest, ParseGeneralizedTimeWrongFormatErrorTest) { std::string invalid_time = ""; CBS cbs; bssl::UniquePtr scoped(asn1Encode(cbs, invalid_time, CBS_ASN1_GENERALIZEDTIME)); - - EXPECT_ANY_THROW(Asn1Utility::parseGeneralizedTime(cbs)); + Asn1Utility::parseGeneralizedTime(cbs); + EXPECT_EQ("Input is not a well-formed ASN.1 GENERALIZEDTIME", + absl::get(Asn1Utility::parseGeneralizedTime(cbs))); } TEST_F(Asn1UtilityTest, ParseGeneralizedTimeTest) { @@ -224,8 +225,9 @@ TEST_F(Asn1UtilityTest, ParseGeneralizedTimeTest) { CBS cbs; bssl::UniquePtr scoped(asn1Encode(cbs, time, CBS_ASN1_GENERALIZEDTIME)); absl::Time expected = TestUtility::parseTime(expected_time, "%E4Y%m%d%H%M%S"); + auto actual = absl::get(Asn1Utility::parseGeneralizedTime(cbs)); - EXPECT_EQ(absl::ToChronoTime(expected), Asn1Utility::parseGeneralizedTime(cbs)); + EXPECT_EQ(absl::ToChronoTime(expected), actual); } TEST_F(Asn1UtilityTest, TestParseGeneralizedTimeRejectsNonUTCTime) { @@ -233,18 +235,18 @@ TEST_F(Asn1UtilityTest, TestParseGeneralizedTimeRejectsNonUTCTime) { CBS cbs; bssl::UniquePtr scoped(asn1Encode(cbs, local_time, CBS_ASN1_GENERALIZEDTIME)); - EXPECT_THROW_WITH_MESSAGE(Asn1Utility::parseGeneralizedTime(cbs), EnvoyException, - "GENERALIZEDTIME must be in UTC"); + EXPECT_EQ("GENERALIZEDTIME must be in UTC", absl::get(Asn1Utility::parseGeneralizedTime(cbs))); } -TEST_F(Asn1UtilityTest, TestParseGeneralizedTimeInvalidTime) { - std::string ymd = "20070601Z"; - CBS cbs; - bssl::UniquePtr scoped(asn1Encode(cbs, ymd, CBS_ASN1_GENERALIZEDTIME)); +// TODO(daniel-goldstein): wat +/* TEST_F(Asn1UtilityTest, TestParseGeneralizedTimeInvalidTime) { */ +/* std::string ymd = "20070601Z"; */ +/* CBS cbs; */ +/* bssl::UniquePtr scoped(asn1Encode(cbs, ymd, CBS_ASN1_GENERALIZEDTIME)); */ - EXPECT_THROW_WITH_REGEX(Asn1Utility::parseGeneralizedTime(cbs), EnvoyException, - "Error parsing timestamp 20070601Z with format %E4Y%m%d%H%M%S"); -} +/* EXPECT_EQ("Error parsing timestamp 20070601Z with format %E4Y%m%d%H%M%S. Error: Failed to parse input", */ +/* absl::get(Asn1Utility::parseGeneralizedTime(cbs))); */ +/* } */ // Taken from // https://boringssl.googlesource.com/boringssl/+/master/crypto/bytestring/cbb.c#531 @@ -298,9 +300,10 @@ TEST_F(Asn1UtilityTest, ParseIntegerTest) { } TEST_F(Asn1UtilityTest, ParseOctetStringTest) { - std::string data = "example_string"; + std::vector data = { 0x1, 0x2, 0x3 }; + std::string data_str(data.begin(), data.end()); CBS cbs; - bssl::UniquePtr scoped(asn1Encode(cbs, data, CBS_ASN1_OCTETSTRING)); + bssl::UniquePtr scoped(asn1Encode(cbs, data_str, CBS_ASN1_OCTETSTRING)); EXPECT_EQ(data, Asn1Utility::parseOctetString(cbs)); } From e97c9c37688f0507ec76a8e4e8953ccebb06d2ff Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Thu, 6 Aug 2020 06:13:04 -0700 Subject: [PATCH 15/31] convert parse optional and sequence of to return ParsingResult Signed-off-by: Daniel Goldstein --- .../tls/ocsp/asn1_utility.cc | 14 ------ .../transport_sockets/tls/ocsp/asn1_utility.h | 32 +++++++------- .../transport_sockets/tls/ocsp/ocsp.cc | 9 ++-- .../transport_sockets/tls/ocsp/ocsp.h | 4 +- .../tls/ocsp/asn1_utility_test.cc | 44 +++++-------------- 5 files changed, 31 insertions(+), 72 deletions(-) diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc index 58cceaa98689..8dac2208462f 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc @@ -115,20 +115,6 @@ std::vector Asn1Utility::parseOctetString(CBS& cbs) { return {data, data + CBS_len(&value)}; } -std::string Asn1Utility::parseAlgorithmIdentifier(CBS& cbs) { - // AlgorithmIdentifier ::= SEQUENCE { - // algorithm OBJECT IDENTIFIER, - // parameters ANY DEFINED BY algorithm OPTIONAL - // } - CBS elem; - if (!CBS_get_asn1(&cbs, &elem, CBS_ASN1_SEQUENCE)) { - throw Envoy::EnvoyException("AlgorithmIdentifier is not a well-formed ASN.1 SEQUENCE"); - } - - return parseOid(elem); - // Ignore `parameters`. -} - void Asn1Utility::skipOptional(CBS& cbs, unsigned tag) { if (!CBS_get_optional_asn1(&cbs, nullptr, nullptr, tag)) { throw Envoy::EnvoyException("Failed to parse ASN.1 element tag"); diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h index ce2753bdda1c..7b94037d1d84 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h @@ -72,7 +72,7 @@ class Asn1Utility { * SEQUENCE OF object. */ template - static std::vector parseSequenceOf(CBS& cbs, Asn1ParsingFunc parse_element); + static ParsingResult> parseSequenceOf(CBS& cbs, Asn1ParsingFunc parse_element); /** * Checks if an explicitly tagged optional element of `tag` is present and @@ -85,7 +85,7 @@ class Asn1Utility { * else nullopt */ template - static absl::optional parseOptional(CBS& cbs, Asn1ParsingFunc parse_data, unsigned tag); + static ParsingResult> parseOptional(CBS& cbs, Asn1ParsingFunc parse_data, unsigned tag); /** * Returns whether or not an element explicitly tagged with `tag` is present @@ -131,17 +131,6 @@ class Asn1Utility { */ static std::string parseInteger(CBS& cbs); - /** - * Parses an ASN.1 AlgorithmIdentifier. Currently ignores algorithm params - * and only returns the OID of the algorithm. - * - * @param cbs a CBS& that refers to an ASN.1 AlgorithmIdentifier element - * @returns std::string the OID of the algorithm - * @throws Envoy::EnvoyException if `cbs` does not point to a well-formed - * AlgorithmIdentifier - */ - static std::string parseAlgorithmIdentifier(CBS& cbs); - /** * @param cbs a CBS& that refers to an ASN.1 OCTETSTRING element * @returns absl::string_view of the octets in `cbs` @@ -172,7 +161,7 @@ class Asn1Utility { }; template -std::vector Asn1Utility::parseSequenceOf(CBS& cbs, Asn1ParsingFunc parse_element) { +ParsingResult> Asn1Utility::parseSequenceOf(CBS& cbs, Asn1ParsingFunc parse_element) { CBS seq_elem; std::vector vec; @@ -183,7 +172,12 @@ std::vector Asn1Utility::parseSequenceOf(CBS& cbs, Asn1ParsingFunc parse_e while (CBS_data(&seq_elem) < CBS_data(&cbs)) { // parse_element MUST advance seq_elem - vec.push_back(parse_element(seq_elem)); + auto elem_res = parse_element(seq_elem); + if (absl::holds_alternative(elem_res)) { + vec.push_back(absl::get<0>(parse_element(seq_elem))); + } else { + return absl::get<1>(elem_res); + } } RELEASE_ASSERT(CBS_data(&cbs) == CBS_data(&seq_elem), @@ -193,11 +187,15 @@ std::vector Asn1Utility::parseSequenceOf(CBS& cbs, Asn1ParsingFunc parse_e } template -absl::optional Asn1Utility::parseOptional(CBS& cbs, Asn1ParsingFunc parse_data, +ParsingResult> Asn1Utility::parseOptional(CBS& cbs, Asn1ParsingFunc parse_data, unsigned tag) { CBS data; if (getOptional(cbs, &data, tag)) { - return parse_data(data); + auto res = parse_data(data); + if (absl::holds_alternative(res)) { + return absl::get<0>(res); + } + return absl::get<1>(res); } return absl::nullopt; diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc index 7c23dd43fad3..564c68df7536 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc @@ -269,14 +269,13 @@ CertId Asn1OcspUtility::parseCertId(CBS& cbs) { throw EnvoyException("OCSP CertID is not a well-formed ASN.1 SEQUENCE"); } - // We use just the issuer name + the serial number to uniquely identify - // a certificate. - auto alg = Asn1Utility::parseAlgorithmIdentifier(elem); - auto issuer_name_hash = Asn1Utility::parseOctetString(elem); + // We use just the serial number to uniquely identify a certificate. + Asn1Utility::skip(elem, CBS_ASN1_SEQUENCE); + Asn1Utility::skip(elem, CBS_ASN1_OCTETSTRING); Asn1Utility::skip(elem, CBS_ASN1_OCTETSTRING); auto serial_number = Asn1Utility::parseInteger(elem); - return {serial_number, alg, issuer_name_hash}; + return {serial_number}; } CertStatus Asn1OcspUtility::parseCertStatus(CBS& cbs) { diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.h b/source/extensions/transport_sockets/tls/ocsp/ocsp.h index be93220c9e84..726936498503 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.h +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.h @@ -70,11 +70,9 @@ enum class CertStatus { * unique per issuer but not necessarily universally. */ struct CertId { - CertId(std::string serial_number, std::string alg_oid, std::vector issuer_name_hash); + CertId(std::string serial_number); std::string serial_number_; - std::string alg_oid_; - std::vector issuer_name_hash_; }; /** diff --git a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc index 95389836eecb..a6e463d1cde7 100644 --- a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc +++ b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc @@ -54,7 +54,6 @@ class Asn1UtilityTest : public testing::Test { /* expectThrowOnWrongTag(Asn1Utility::parseGeneralizedTime); */ /* expectThrowOnWrongTag(Asn1Utility::parseInteger); */ /* expectThrowOnWrongTag(Asn1Utility::parseOctetString); */ -/* expectThrowOnWrongTag(Asn1Utility::parseAlgorithmIdentifier); */ /* } */ TEST_F(Asn1UtilityTest, ToStringTest) { @@ -68,8 +67,9 @@ TEST_F(Asn1UtilityTest, ParseSequenceOfEmptySequenceTest) { CBS cbs; CBS_init(&cbs, asn1_empty_seq.data(), asn1_empty_seq.size()); - std::vector vec; - auto actual = Asn1Utility::parseSequenceOf(cbs, [](CBS&) { return ""; }); + std::vector> vec; + auto actual = absl::get<0>(Asn1Utility::parseSequenceOf>(cbs, + Asn1Utility::parseOctetString)); EXPECT_EQ(vec, actual); } @@ -98,7 +98,7 @@ TEST_F(Asn1UtilityTest, ParseSequenceOfMultipleElementSequenceTest) { CBS_init(&cbs, octet_seq.data(), octet_seq.size()); std::vector> vec = {{0x1, 0x2}, {0x3, 0x4}, {0x5, 0x6}}; - auto actual = Asn1Utility::parseSequenceOf>(cbs, Asn1Utility::parseOctetString); + auto actual = absl::get<0>(Asn1Utility::parseSequenceOf>(cbs, Asn1Utility::parseOctetString)); EXPECT_EQ(vec, actual); } @@ -116,9 +116,8 @@ TEST_F(Asn1UtilityTest, SequenceOfLengthMismatchErrorTest) { CBS cbs; CBS_init(&cbs, malformed.data(), malformed.size()); - EXPECT_THROW_WITH_MESSAGE( - Asn1Utility::parseSequenceOf>(cbs, Asn1Utility::parseOctetString), EnvoyException, - "Input is not a well-formed ASN.1 OCTETSTRING"); + EXPECT_EQ("Input is not a well-formed ASN.1 OCTETSTRING", + absl::get<1>(Asn1Utility::parseSequenceOf>(cbs, Asn1Utility::parseOctetString))); } TEST_F(Asn1UtilityTest, SequenceOfMixedTypeErrorTest) { @@ -139,9 +138,8 @@ TEST_F(Asn1UtilityTest, SequenceOfMixedTypeErrorTest) { CBS cbs; CBS_init(&cbs, mixed_type.data(), mixed_type.size()); - EXPECT_THROW_WITH_MESSAGE( - Asn1Utility::parseSequenceOf>(cbs, Asn1Utility::parseOctetString), EnvoyException, - "Input is not a well-formed ASN.1 OCTETSTRING"); + EXPECT_EQ("Input is not a well-formed ASN.1 OCTETSTRING", + absl::get<1>(Asn1Utility::parseSequenceOf>(cbs, Asn1Utility::parseOctetString))); } TEST_F(Asn1UtilityTest, IsOptionalPresentTest) { @@ -183,11 +181,11 @@ TEST_F(Asn1UtilityTest, ParseOptionalTest) { }; absl::optional expected(true); - EXPECT_EQ(expected, Asn1Utility::parseOptional(cbs_explicit_optional_true, parseBool, 0)); + EXPECT_EQ(expected, absl::get<0>(Asn1Utility::parseOptional(cbs_explicit_optional_true, parseBool, 0))); EXPECT_EQ(absl::nullopt, - Asn1Utility::parseOptional(cbs_empty_seq, parseBool, CBS_ASN1_BOOLEAN)); + absl::get<0>(Asn1Utility::parseOptional(cbs_empty_seq, parseBool, CBS_ASN1_BOOLEAN))); EXPECT_EQ(absl::nullopt, - Asn1Utility::parseOptional(cbs_nothing, parseBool, CBS_ASN1_BOOLEAN)); + absl::get<0>(Asn1Utility::parseOptional(cbs_nothing, parseBool, CBS_ASN1_BOOLEAN))); } TEST_F(Asn1UtilityTest, ParseOidTest) { @@ -308,26 +306,6 @@ TEST_F(Asn1UtilityTest, ParseOctetStringTest) { EXPECT_EQ(data, Asn1Utility::parseOctetString(cbs)); } -TEST_F(Asn1UtilityTest, ParseAlgorithmIdentifierTest) { - std::string sha256 = "2.16.840.1.101.3.4.2.1"; - - bssl::ScopedCBB cbb; - CBB seq, oid; - ASSERT_TRUE(CBB_init(cbb.get(), 0)); - ASSERT_TRUE(CBB_add_asn1(cbb.get(), &seq, CBS_ASN1_SEQUENCE)); - ASSERT_TRUE(CBB_add_asn1(&seq, &oid, CBS_ASN1_OBJECT)); - ASSERT_TRUE(CBB_add_asn1_oid_from_text(&oid, sha256.c_str(), sha256.size())); - - uint8_t* buf; - size_t buf_len; - CBS cbs; - ASSERT_TRUE(CBB_finish(cbb.get(), &buf, &buf_len)); - CBS_init(&cbs, buf, buf_len); - bssl::UniquePtr scoped(buf); - - EXPECT_EQ(sha256, Asn1Utility::parseAlgorithmIdentifier(cbs)); -} - TEST_F(Asn1UtilityTest, SkipOptionalPresentAdvancesTest) { CBS cbs; CBS_init(&cbs, asn1_empty_seq.data(), asn1_empty_seq.size()); From 4e7dc5d35d7e811527135c7691a2006a8b77868d Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Thu, 6 Aug 2020 06:42:35 -0700 Subject: [PATCH 16/31] convert octet string parsing to use ParseResult Signed-off-by: Daniel Goldstein --- .../transport_sockets/tls/ocsp/asn1_utility.cc | 6 +++--- .../extensions/transport_sockets/tls/ocsp/asn1_utility.h | 9 ++++----- source/extensions/transport_sockets/tls/ocsp/ocsp.cc | 2 +- .../transport_sockets/tls/ocsp/asn1_utility_test.cc | 2 +- 4 files changed, 9 insertions(+), 10 deletions(-) diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc index 8dac2208462f..802308aa4d74 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc @@ -105,14 +105,14 @@ std::string Asn1Utility::parseInteger(CBS& cbs) { throw Envoy::EnvoyException("Failed to parse ASN.1 INTEGER"); } -std::vector Asn1Utility::parseOctetString(CBS& cbs) { +ParsingResult> Asn1Utility::parseOctetString(CBS& cbs) { CBS value; if (!CBS_get_asn1(&cbs, &value, CBS_ASN1_OCTETSTRING)) { - throw Envoy::EnvoyException("Input is not a well-formed ASN.1 OCTETSTRING"); + return "Input is not a well-formed ASN.1 OCTETSTRING"; } auto data = reinterpret_cast(CBS_data(&value)); - return {data, data + CBS_len(&value)}; + return std::vector{data, data + CBS_len(&value)}; } void Asn1Utility::skipOptional(CBS& cbs, unsigned tag) { diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h index 7b94037d1d84..b2a47c479e58 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h @@ -137,7 +137,7 @@ class Asn1Utility { * @throws Envoy::EnvoyException if `cbs` does not point to a well-formed * OCTETSTRING */ - static std::vector parseOctetString(CBS& cbs); + static ParsingResult> parseOctetString(CBS& cbs); /** * Advance `cbs` over an ASN.1 value of the class `tag` if that @@ -167,14 +167,14 @@ ParsingResult> Asn1Utility::parseSequenceOf(CBS& cbs, Asn1Parsing // Initialize seq_elem to first element in sequence. if (!CBS_get_asn1(&cbs, &seq_elem, CBS_ASN1_SEQUENCE)) { - throw Envoy::EnvoyException("Expected sequence of ASN.1 elements."); + return "Expected sequence of ASN.1 elements."; } while (CBS_data(&seq_elem) < CBS_data(&cbs)) { // parse_element MUST advance seq_elem auto elem_res = parse_element(seq_elem); if (absl::holds_alternative(elem_res)) { - vec.push_back(absl::get<0>(parse_element(seq_elem))); + vec.push_back(absl::get<0>(elem_res)); } else { return absl::get<1>(elem_res); } @@ -187,8 +187,7 @@ ParsingResult> Asn1Utility::parseSequenceOf(CBS& cbs, Asn1Parsing } template -ParsingResult> Asn1Utility::parseOptional(CBS& cbs, Asn1ParsingFunc parse_data, - unsigned tag) { +ParsingResult> Asn1Utility::parseOptional(CBS& cbs, Asn1ParsingFunc parse_data, unsigned tag) { CBS data; if (getOptional(cbs, &data, tag)) { auto res = parse_data(data); diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc index 564c68df7536..467598760b86 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc @@ -228,7 +228,7 @@ ResponseData Asn1OcspUtility::parseResponseData(CBS& cbs) { Asn1Utility::skipOptional(elem, 0); skipResponderId(elem); Asn1Utility::skip(elem, CBS_ASN1_GENERALIZEDTIME); - auto responses = Asn1Utility::parseSequenceOf(elem, parseSingleResponse); + auto responses = unwrap(Asn1Utility::parseSequenceOf(elem, parseSingleResponse)); // Extensions currently ignored return {std::move(responses)}; diff --git a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc index a6e463d1cde7..9fa7927fa1a4 100644 --- a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc +++ b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc @@ -303,7 +303,7 @@ TEST_F(Asn1UtilityTest, ParseOctetStringTest) { CBS cbs; bssl::UniquePtr scoped(asn1Encode(cbs, data_str, CBS_ASN1_OCTETSTRING)); - EXPECT_EQ(data, Asn1Utility::parseOctetString(cbs)); + EXPECT_EQ(data, absl::get<0>(Asn1Utility::parseOctetString(cbs))); } TEST_F(Asn1UtilityTest, SkipOptionalPresentAdvancesTest) { From a3bd2f66f517a103fa76076c1e179ae668635564 Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Thu, 6 Aug 2020 07:25:49 -0700 Subject: [PATCH 17/31] change skip methods to use ParsingResult Signed-off-by: Daniel Goldstein --- .../tls/ocsp/asn1_utility.cc | 11 +++++---- .../transport_sockets/tls/ocsp/asn1_utility.h | 4 ++-- .../transport_sockets/tls/ocsp/ocsp.cc | 23 ++++++++++--------- .../tls/ocsp/asn1_utility_test.cc | 8 +++---- 4 files changed, 25 insertions(+), 21 deletions(-) diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc index 802308aa4d74..04bab99bde5b 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc @@ -115,16 +115,19 @@ ParsingResult> Asn1Utility::parseOctetString(CBS& cbs) { return std::vector{data, data + CBS_len(&value)}; } -void Asn1Utility::skipOptional(CBS& cbs, unsigned tag) { +ParsingResult Asn1Utility::skipOptional(CBS& cbs, unsigned tag) { if (!CBS_get_optional_asn1(&cbs, nullptr, nullptr, tag)) { - throw Envoy::EnvoyException("Failed to parse ASN.1 element tag"); + return "Failed to parse ASN.1 element tag"; } + return absl::monostate(); } -void Asn1Utility::skip(CBS& cbs, unsigned tag) { +ParsingResult Asn1Utility::skip(CBS& cbs, unsigned tag) { if (!CBS_get_asn1(&cbs, nullptr, tag)) { - throw Envoy::EnvoyException("Failed to parse ASN.1 element"); + return "Failed to parse ASN.1 element"; } + + return absl::monostate(); } } // namespace Ocsp diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h index b2a47c479e58..2e3490579371 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h @@ -147,7 +147,7 @@ class Asn1Utility { * @param tag the tag of the value to skip * @throws Envoy::EnvoyException if `cbs` is a malformed TLV bytestring */ - static void skipOptional(CBS& cbs, unsigned tag); + static ParsingResult skipOptional(CBS& cbs, unsigned tag); /** * Advance `cbs` over an ASN.1 value of the class `tag`. @@ -157,7 +157,7 @@ class Asn1Utility { * @throws Envoy::EnvoyException if `cbs` does not point to a well-formed * element with tag `tag`. */ - static void skip(CBS& cbs, unsigned tag); + static ParsingResult skip(CBS& cbs, unsigned tag); }; template diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc index 467598760b86..3461e0b9ceb5 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc @@ -18,10 +18,10 @@ namespace { template T unwrap(ParsingResult res) { if (absl::holds_alternative(res)) { - return absl::get(res); + return absl::get<0>(res); } - throw EnvoyException(absl::get(res)); + throw EnvoyException(std::string(absl::get<1>(res))); } unsigned parseTag(CBS& cbs) { @@ -75,8 +75,7 @@ SingleResponse::SingleResponse(CertId cert_id, CertStatus status, Envoy::SystemT absl::optional next_update) : cert_id_(cert_id), status_(status), this_update_(this_update), next_update_(next_update) {} -CertId::CertId(std::string serial_number, std::string alg_oid, std::vector issuer_name_hash) - : serial_number_(serial_number), alg_oid_(alg_oid), issuer_name_hash_(issuer_name_hash) {} +CertId::CertId(std::string serial_number) : serial_number_(serial_number) {} OcspResponseWrapper::OcspResponseWrapper(std::vector der_response, TimeSource& time_source) : raw_bytes_(std::move(der_response)), response_(readDerEncodedOcspResponse(raw_bytes_)), @@ -225,10 +224,12 @@ ResponseData Asn1OcspUtility::parseResponseData(CBS& cbs) { throw EnvoyException("OCSP ResponseData is not a well-formed ASN.1 SEQUENCE"); } - Asn1Utility::skipOptional(elem, 0); + unwrap(Asn1Utility::skipOptional(elem, 0)); skipResponderId(elem); - Asn1Utility::skip(elem, CBS_ASN1_GENERALIZEDTIME); - auto responses = unwrap(Asn1Utility::parseSequenceOf(elem, parseSingleResponse)); + unwrap(Asn1Utility::skip(elem, CBS_ASN1_GENERALIZEDTIME)); + auto responses = unwrap(Asn1Utility::parseSequenceOf(elem, [](CBS& cbs) -> ParsingResult { + return parseSingleResponse(cbs); + })); // Extensions currently ignored return {std::move(responses)}; @@ -250,7 +251,7 @@ SingleResponse Asn1OcspUtility::parseSingleResponse(CBS& cbs) { auto cert_id = Asn1OcspUtility::parseCertId(elem); auto status = Asn1OcspUtility::parseCertStatus(elem); auto this_update = unwrap(Asn1Utility::parseGeneralizedTime(elem)); - auto next_update = unwrap(Asn1Utility::parseOptional>( + auto next_update = unwrap(Asn1Utility::parseOptional( elem, Asn1Utility::parseGeneralizedTime, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0)); // Extensions currently ignored @@ -270,9 +271,9 @@ CertId Asn1OcspUtility::parseCertId(CBS& cbs) { } // We use just the serial number to uniquely identify a certificate. - Asn1Utility::skip(elem, CBS_ASN1_SEQUENCE); - Asn1Utility::skip(elem, CBS_ASN1_OCTETSTRING); - Asn1Utility::skip(elem, CBS_ASN1_OCTETSTRING); + unwrap(Asn1Utility::skip(elem, CBS_ASN1_SEQUENCE)); + unwrap(Asn1Utility::skip(elem, CBS_ASN1_OCTETSTRING)); + unwrap(Asn1Utility::skip(elem, CBS_ASN1_OCTETSTRING)); auto serial_number = Asn1Utility::parseInteger(elem); return {serial_number}; diff --git a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc index 9fa7927fa1a4..6f0d5a130135 100644 --- a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc +++ b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc @@ -311,7 +311,7 @@ TEST_F(Asn1UtilityTest, SkipOptionalPresentAdvancesTest) { CBS_init(&cbs, asn1_empty_seq.data(), asn1_empty_seq.size()); const uint8_t* start = CBS_data(&cbs); - EXPECT_NO_THROW({ Asn1Utility::skipOptional(cbs, CBS_ASN1_SEQUENCE); }); + EXPECT_NO_THROW(absl::get<0>(Asn1Utility::skipOptional(cbs, CBS_ASN1_SEQUENCE))); EXPECT_EQ(start + 2, CBS_data(&cbs)); } @@ -320,7 +320,7 @@ TEST_F(Asn1UtilityTest, SkipOptionalNotPresentDoesNotAdvanceTest) { CBS_init(&cbs, asn1_empty_seq.data(), asn1_empty_seq.size()); const uint8_t* start = CBS_data(&cbs); - EXPECT_NO_THROW({ Asn1Utility::skipOptional(cbs, CBS_ASN1_BOOLEAN); }); + EXPECT_NO_THROW(absl::get<0>(Asn1Utility::skipOptional(cbs, CBS_ASN1_BOOLEAN))); EXPECT_EQ(start, CBS_data(&cbs)); } @@ -329,8 +329,8 @@ TEST_F(Asn1UtilityTest, SkipOptionalMalformedTagTest) { CBS cbs; CBS_init(&cbs, malformed_seq.data(), malformed_seq.size()); - EXPECT_THROW_WITH_MESSAGE(Asn1Utility::skipOptional(cbs, CBS_ASN1_SEQUENCE), EnvoyException, - "Failed to parse ASN.1 element tag"); + EXPECT_EQ("Failed to parse ASN.1 element tag", + absl::get<1>(Asn1Utility::skipOptional(cbs, CBS_ASN1_SEQUENCE))); } } // namespace From fe8589bed2810a2c2d46d399595411e2b189765a Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Thu, 6 Aug 2020 08:24:55 -0700 Subject: [PATCH 18/31] complete all asn1 utility methods moving to ParsingResult Signed-off-by: Daniel Goldstein --- .../tls/ocsp/asn1_utility.cc | 22 ++++---- .../transport_sockets/tls/ocsp/asn1_utility.h | 14 +++-- .../transport_sockets/tls/ocsp/ocsp.cc | 24 +++++---- .../tls/ocsp/asn1_utility_test.cc | 54 +++++++++---------- 4 files changed, 60 insertions(+), 54 deletions(-) diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc index 04bab99bde5b..52b42da41cb5 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc @@ -1,7 +1,6 @@ #include "extensions/transport_sockets/tls/ocsp/asn1_utility.h" #include "common/common/c_smart_ptr.h" -#include "absl/strings/str_cat.h" #include "absl/strings/ascii.h" namespace Envoy { @@ -29,23 +28,23 @@ absl::string_view Asn1Utility::cbsToString(CBS& cbs) { return {str_head, CBS_len(&cbs)}; } -bool Asn1Utility::getOptional(CBS& cbs, CBS* data, unsigned tag) { +ParsingResult Asn1Utility::getOptional(CBS& cbs, CBS* data, unsigned tag) { int is_present; if (!CBS_get_optional_asn1(&cbs, data, &is_present, tag)) { - throw Envoy::EnvoyException("Failed to parse ASN.1 element tag"); + return "Failed to parse ASN.1 element tag"; } - return is_present; + return static_cast(is_present); } -std::string Asn1Utility::parseOid(CBS& cbs) { +ParsingResult Asn1Utility::parseOid(CBS& cbs) { CBS oid; if (!CBS_get_asn1(&cbs, &oid, CBS_ASN1_OBJECT)) { - throw Envoy::EnvoyException("Input is not a well-formed ASN.1 OBJECT"); + return absl::string_view("Input is not a well-formed ASN.1 OBJECT"); } CSmartPtr oid_text(CBS_asn1_oid_to_text(&oid)); if (oid_text == nullptr) { - throw Envoy::EnvoyException("Failed to parse oid"); + return absl::string_view("Failed to parse oid"); } std::string oid_text_str(oid_text.get()); @@ -73,18 +72,17 @@ ParsingResult Asn1Utility::parseGeneralizedTime(CBS& cbs) { auto utc_time_str = time_str.substr(0, time_str.length() - 1); std::string parse_error; if (!absl::ParseTime(GENERALIZED_TIME_FORMAT, utc_time_str, &time, &parse_error)) { - return absl::StrCat("Error parsing timestamp ", time_str, " with format ", - GENERALIZED_TIME_FORMAT, ". Error: ", parse_error); + return "Error parsing string of GENERALIZEDTIME format"; } return absl::ToChronoTime(time); } // Performs the following conversions to go from bytestring to hex integer // CBS -> ASN1_INTEGER -> BIGNUM -> String -std::string Asn1Utility::parseInteger(CBS& cbs) { +ParsingResult Asn1Utility::parseInteger(CBS& cbs) { CBS num; if (!CBS_get_asn1(&cbs, &num, CBS_ASN1_INTEGER)) { - throw Envoy::EnvoyException("Input is not a well-formed ASN.1 INTEGER"); + return absl::string_view("Input is not a well-formed ASN.1 INTEGER"); } auto head = CBS_data(&num); @@ -102,7 +100,7 @@ std::string Asn1Utility::parseInteger(CBS& cbs) { } } - throw Envoy::EnvoyException("Failed to parse ASN.1 INTEGER"); + return absl::string_view("Failed to parse ASN.1 INTEGER"); } ParsingResult> Asn1Utility::parseOctetString(CBS& cbs) { diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h index 2e3490579371..9c7935bbe3bf 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h @@ -100,7 +100,7 @@ class Asn1Utility { * @returns bool whether `cbs` points to an element tagged with `tag` * @throws Envoy::EnvoyException if `cbs` is a malformed TLV bytestring */ - static bool getOptional(CBS& cbs, CBS* data, unsigned tag); + static ParsingResult getOptional(CBS& cbs, CBS* data, unsigned tag); /** * @param cbs a CBS& that refers to an ASN.1 OBJECT IDENTIFIER element @@ -108,7 +108,7 @@ class Asn1Utility { * @throws Envoy::EnvoyException if `cbs` does not point to a well-formed * OBJECT IDENTIFIER */ - static std::string parseOid(CBS& cbs); + static ParsingResult parseOid(CBS& cbs); /** * @param cbs a CBS& that refers to an ASN.1 GENERALIZEDTIME element @@ -129,7 +129,7 @@ class Asn1Utility { * @throws Envoy::EnvoyException if `cbs` does not point to a well-formed * INTEGER */ - static std::string parseInteger(CBS& cbs); + static ParsingResult parseInteger(CBS& cbs); /** * @param cbs a CBS& that refers to an ASN.1 OCTETSTRING element @@ -189,7 +189,13 @@ ParsingResult> Asn1Utility::parseSequenceOf(CBS& cbs, Asn1Parsing template ParsingResult> Asn1Utility::parseOptional(CBS& cbs, Asn1ParsingFunc parse_data, unsigned tag) { CBS data; - if (getOptional(cbs, &data, tag)) { + auto is_present = getOptional(cbs, &data, tag); + + if (absl::holds_alternative(is_present)) { + return absl::get(is_present); + } + + if (absl::get(is_present)) { auto res = parse_data(data); if (absl::holds_alternative(res)) { return absl::get<0>(res); diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc index 3461e0b9ceb5..a52b1b29232d 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc @@ -53,8 +53,8 @@ void skipResponderId(CBS& cbs) { // KeyHash ::= OCTET STRING -- SHA-1 hash of responder's public key // (excluding the tag and length fields) - if (Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 1) || - Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 2)) { + if (unwrap(Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 1)) || + unwrap(Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 2))) { return; } @@ -99,7 +99,10 @@ OcspResponseWrapper::OcspResponseWrapper(std::vector der_response, Time } } -// TODO(daniel-goldstein): This should also check the issuer +// We use just the serial number to uniquely identify a certificate. +// Though different issuers could produce certificates with the same serial +// number, this is check is to prevent operator error and a collision in this +// case is unlikely. bool OcspResponseWrapper::matchesCertificate(X509& cert) { std::string cert_serial_number = CertUtility::getSerialNumberFromCertificate(cert); std::string resp_cert_serial_number = response_->response_->getCertSerialNumber(); @@ -126,7 +129,7 @@ std::unique_ptr Asn1OcspUtility::parseOcspResponse(CBS& cbs) { CBS bytes; ResponsePtr resp = nullptr; - if (Asn1Utility::getOptional(elem, &bytes, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0)) { + if (unwrap(Asn1Utility::getOptional(elem, &bytes, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0))) { resp = Asn1OcspUtility::parseResponseBytes(bytes); } @@ -179,7 +182,7 @@ ResponsePtr Asn1OcspUtility::parseResponseBytes(CBS& cbs) { throw EnvoyException("OCSP ResponseBytes is not a well-formed SEQUENCE"); } - auto oid_str = Asn1Utility::parseOid(elem); + auto oid_str = unwrap(Asn1Utility::parseOid(elem)); if (!CBS_get_asn1(&elem, &response, CBS_ASN1_OCTETSTRING)) { throw EnvoyException("Expected ASN.1 OCTETSTRING for response"); } @@ -228,7 +231,7 @@ ResponseData Asn1OcspUtility::parseResponseData(CBS& cbs) { skipResponderId(elem); unwrap(Asn1Utility::skip(elem, CBS_ASN1_GENERALIZEDTIME)); auto responses = unwrap(Asn1Utility::parseSequenceOf(elem, [](CBS& cbs) -> ParsingResult { - return parseSingleResponse(cbs); + return ParsingResult(parseSingleResponse(cbs)); })); // Extensions currently ignored @@ -270,11 +273,10 @@ CertId Asn1OcspUtility::parseCertId(CBS& cbs) { throw EnvoyException("OCSP CertID is not a well-formed ASN.1 SEQUENCE"); } - // We use just the serial number to uniquely identify a certificate. unwrap(Asn1Utility::skip(elem, CBS_ASN1_SEQUENCE)); unwrap(Asn1Utility::skip(elem, CBS_ASN1_OCTETSTRING)); unwrap(Asn1Utility::skip(elem, CBS_ASN1_OCTETSTRING)); - auto serial_number = Asn1Utility::parseInteger(elem); + auto serial_number = unwrap(Asn1Utility::parseInteger(elem)); return {serial_number}; } @@ -285,13 +287,13 @@ CertStatus Asn1OcspUtility::parseCertStatus(CBS& cbs) { // revoked [1] IMPLICIT RevokedInfo, // unknown [2] IMPLICIT UnknownInfo // } - if (Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONTEXT_SPECIFIC | 0)) { + if (unwrap(Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONTEXT_SPECIFIC | 0))) { return CertStatus::GOOD; } - if (Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 1)) { + if (unwrap(Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 1))) { return CertStatus::REVOKED; } - if (Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONTEXT_SPECIFIC | 2)) { + if (unwrap(Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONTEXT_SPECIFIC | 2))) { return CertStatus::UNKNOWN; } diff --git a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc index 6f0d5a130135..9d50736dbf28 100644 --- a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc +++ b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc @@ -37,24 +37,25 @@ class Asn1UtilityTest : public testing::Test { return buf; } - /* void expectThrowOnWrongTag(std::function parse) { */ - /* CBS cbs; */ - /* CBS_init(&cbs, asn1_true.data(), asn1_true.size()); */ - /* EXPECT_THROW(parse(cbs), EnvoyException); */ - /* } */ + template + void expectParseResultErrorOnWrongTag(std::function(CBS&)> parse) { + CBS cbs; + CBS_init(&cbs, asn1_true.data(), asn1_true.size()); + EXPECT_NO_THROW(absl::get<1>(parse(cbs))); + } const std::vector asn1_true = {0x1u, 1, 0xff}; const std::vector asn1_empty_seq = {0x30, 0}; }; -/* TEST_F(Asn1UtilityTest, ParseMethodsWrongTagTest) { */ -/* expectThrowOnWrongTag( */ -/* [](CBS& cbs) { Asn1Utility::parseSequenceOf(cbs, [](CBS&) { return ""; }); }); */ -/* expectThrowOnWrongTag(Asn1Utility::parseOid); */ -/* expectThrowOnWrongTag(Asn1Utility::parseGeneralizedTime); */ -/* expectThrowOnWrongTag(Asn1Utility::parseInteger); */ -/* expectThrowOnWrongTag(Asn1Utility::parseOctetString); */ -/* } */ +TEST_F(Asn1UtilityTest, ParseMethodsWrongTagTest) { + expectParseResultErrorOnWrongTag>>( + [](CBS& cbs) { return Asn1Utility::parseSequenceOf>(cbs, Asn1Utility::parseOctetString); }); + expectParseResultErrorOnWrongTag(Asn1Utility::parseOid); + expectParseResultErrorOnWrongTag(Asn1Utility::parseGeneralizedTime); + expectParseResultErrorOnWrongTag(Asn1Utility::parseInteger); + expectParseResultErrorOnWrongTag>(Asn1Utility::parseOctetString); +} TEST_F(Asn1UtilityTest, ToStringTest) { CBS cbs; @@ -147,10 +148,10 @@ TEST_F(Asn1UtilityTest, IsOptionalPresentTest) { CBS_init(&cbs, asn1_true.data(), asn1_true.size()); const uint8_t* start = CBS_data(&cbs); - EXPECT_FALSE(Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_INTEGER)); + EXPECT_FALSE(absl::get<0>(Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_INTEGER))); EXPECT_EQ(start, CBS_data(&cbs)); - EXPECT_TRUE(Asn1Utility::getOptional(cbs, &value, CBS_ASN1_BOOLEAN)); + EXPECT_TRUE(absl::get<0>(Asn1Utility::getOptional(cbs, &value, CBS_ASN1_BOOLEAN))); EXPECT_EQ(0xff, *CBS_data(&value)); } @@ -159,8 +160,8 @@ TEST_F(Asn1UtilityTest, IsOptionalPresentMissingValueTest) { CBS cbs, value; CBS_init(&cbs, missing_val_bool.data(), missing_val_bool.size()); - EXPECT_THROW_WITH_MESSAGE(Asn1Utility::getOptional(cbs, &value, CBS_ASN1_BOOLEAN), - EnvoyException, "Failed to parse ASN.1 element tag"); + EXPECT_EQ("Failed to parse ASN.1 element tag", + absl::get<1>(Asn1Utility::getOptional(cbs, &value, CBS_ASN1_BOOLEAN))); } TEST_F(Asn1UtilityTest, ParseOptionalTest) { @@ -204,7 +205,7 @@ TEST_F(Asn1UtilityTest, ParseOidTest) { CBS_init(&cbs, buf, buf_len); bssl::UniquePtr scoped(buf); - EXPECT_EQ(oid, Asn1Utility::parseOid(cbs)); + EXPECT_EQ(oid, absl::get<0>(Asn1Utility::parseOid(cbs))); } TEST_F(Asn1UtilityTest, ParseGeneralizedTimeWrongFormatErrorTest) { @@ -236,15 +237,14 @@ TEST_F(Asn1UtilityTest, TestParseGeneralizedTimeRejectsNonUTCTime) { EXPECT_EQ("GENERALIZEDTIME must be in UTC", absl::get(Asn1Utility::parseGeneralizedTime(cbs))); } -// TODO(daniel-goldstein): wat -/* TEST_F(Asn1UtilityTest, TestParseGeneralizedTimeInvalidTime) { */ -/* std::string ymd = "20070601Z"; */ -/* CBS cbs; */ -/* bssl::UniquePtr scoped(asn1Encode(cbs, ymd, CBS_ASN1_GENERALIZEDTIME)); */ +TEST_F(Asn1UtilityTest, TestParseGeneralizedTimeInvalidTime) { + std::string ymd = "20070601Z"; + CBS cbs; + bssl::UniquePtr scoped(asn1Encode(cbs, ymd, CBS_ASN1_GENERALIZEDTIME)); -/* EXPECT_EQ("Error parsing timestamp 20070601Z with format %E4Y%m%d%H%M%S. Error: Failed to parse input", */ -/* absl::get(Asn1Utility::parseGeneralizedTime(cbs))); */ -/* } */ + EXPECT_EQ("Error parsing string of GENERALIZEDTIME format", + absl::get<1>(Asn1Utility::parseGeneralizedTime(cbs))); +} // Taken from // https://boringssl.googlesource.com/boringssl/+/master/crypto/bytestring/cbb.c#531 @@ -292,7 +292,7 @@ TEST_F(Asn1UtilityTest, ParseIntegerTest) { CBS_init(&cbs, buf, buf_len); bssl::UniquePtr scoped_buf(buf); - EXPECT_EQ(int_and_hex.second, Asn1Utility::parseInteger(cbs)); + EXPECT_EQ(int_and_hex.second, absl::get<0>(Asn1Utility::parseInteger(cbs))); cbb.Reset(); } } From 26f591521dd808ca103082add5d3a31c0ed6b31f Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Thu, 6 Aug 2020 09:36:48 -0700 Subject: [PATCH 19/31] skip cert status Signed-off-by: Daniel Goldstein --- .../transport_sockets/tls/ocsp/ocsp.cc | 40 ++++++++----------- .../transport_sockets/tls/ocsp/ocsp.h | 35 +--------------- .../transport_sockets/tls/ocsp/ocsp_test.cc | 29 +++----------- 3 files changed, 23 insertions(+), 81 deletions(-) diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc index a52b1b29232d..29903fb77991 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc @@ -61,6 +61,19 @@ void skipResponderId(CBS& cbs) { throw EnvoyException(absl::StrCat("Unknown choice for Responder ID: ", parseTag(cbs))); } +void skipCertStatus(CBS& cbs) { + // CertStatus ::= CHOICE { + // good [0] IMPLICIT NULL, + // revoked [1] IMPLICIT RevokedInfo, + // unknown [2] IMPLICIT UnknownInfo + // } + if (!(unwrap(Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONTEXT_SPECIFIC | 0)) || + unwrap(Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 1)) || + unwrap(Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONTEXT_SPECIFIC | 2)))) { + throw EnvoyException(absl::StrCat("Unknown OcspCertStatus tag: ", parseTag(cbs))); + } +} + } // namespace OcspResponse::OcspResponse(OcspResponseStatus status, ResponsePtr response) @@ -71,9 +84,9 @@ BasicOcspResponse::BasicOcspResponse(ResponseData data) : data_(data) {} ResponseData::ResponseData(std::vector single_responses) : single_responses_(std::move(single_responses)) {} -SingleResponse::SingleResponse(CertId cert_id, CertStatus status, Envoy::SystemTime this_update, +SingleResponse::SingleResponse(CertId cert_id, Envoy::SystemTime this_update, absl::optional next_update) - : cert_id_(cert_id), status_(status), this_update_(this_update), next_update_(next_update) {} + : cert_id_(cert_id), this_update_(this_update), next_update_(next_update) {} CertId::CertId(std::string serial_number) : serial_number_(serial_number) {} @@ -252,13 +265,13 @@ SingleResponse Asn1OcspUtility::parseSingleResponse(CBS& cbs) { } auto cert_id = Asn1OcspUtility::parseCertId(elem); - auto status = Asn1OcspUtility::parseCertStatus(elem); + skipCertStatus(elem); auto this_update = unwrap(Asn1Utility::parseGeneralizedTime(elem)); auto next_update = unwrap(Asn1Utility::parseOptional( elem, Asn1Utility::parseGeneralizedTime, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0)); // Extensions currently ignored - return {cert_id, status, this_update, next_update}; + return {cert_id, this_update, next_update}; } CertId Asn1OcspUtility::parseCertId(CBS& cbs) { @@ -281,25 +294,6 @@ CertId Asn1OcspUtility::parseCertId(CBS& cbs) { return {serial_number}; } -CertStatus Asn1OcspUtility::parseCertStatus(CBS& cbs) { - // CertStatus ::= CHOICE { - // good [0] IMPLICIT NULL, - // revoked [1] IMPLICIT RevokedInfo, - // unknown [2] IMPLICIT UnknownInfo - // } - if (unwrap(Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONTEXT_SPECIFIC | 0))) { - return CertStatus::GOOD; - } - if (unwrap(Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 1))) { - return CertStatus::REVOKED; - } - if (unwrap(Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONTEXT_SPECIFIC | 2))) { - return CertStatus::UNKNOWN; - } - - throw EnvoyException(absl::StrCat("Unknown OcspCertStatus tag: ", parseTag(cbs))); -} - } // namespace Ocsp } // namespace Tls } // namespace TransportSockets diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.h b/source/extensions/transport_sockets/tls/ocsp/ocsp.h index 726936498503..b3e84ad033b0 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.h +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.h @@ -50,19 +50,6 @@ enum class OcspResponseStatus { Unauthorized = 6 }; -/** - * Reflection of the ASN.1 CertStatus enumeration. - * The status of a single SSL certificate in an OCSP response. - */ -enum class CertStatus { - // The certificate is known to be valid - GOOD, - // The certificate has been revoked - REVOKED, - // The responder has no record of the certificate and cannot confirm its validity - UNKNOWN -}; - /** * Reflection of the ASN.1 CertId structure. * Contains the information to uniquely identify an SSL Certificate. @@ -87,11 +74,10 @@ struct CertId { * and invalid for stapling. */ struct SingleResponse { - SingleResponse(CertId cert_id, CertStatus status, Envoy::SystemTime this_update, + SingleResponse(CertId cert_id, Envoy::SystemTime this_update, absl::optional next_update); const CertId cert_id_; - const CertStatus status_; const Envoy::SystemTime this_update_; const absl::optional next_update_; }; @@ -124,11 +110,6 @@ class Response { */ virtual size_t getNumCerts() PURE; - /** - * @return The revocation status of the certificate. - */ - virtual CertStatus getCertRevocationStatus() PURE; - /** * @return The serial number of the certificate. */ @@ -163,7 +144,6 @@ class BasicOcspResponse : public Response { // Response size_t getNumCerts() override { return data_.single_responses_.size(); } - CertStatus getCertRevocationStatus() override { return data_.single_responses_[0].status_; } const std::string& getCertSerialNumber() override { return data_.single_responses_[0].cert_id_.serial_number_; } @@ -211,11 +191,6 @@ class OcspResponseWrapper { */ OcspResponseStatus getResponseStatus() { return response_->status_; } - /** - * @returns CertStatus for the single SSL certificate reported on by this response - */ - CertStatus getCertRevocationStatus() { return response_->response_->getCertRevocationStatus(); } - /** * @param cert a X509& SSL certificate * @returns bool whether this OCSP response contains the revocation status of `cert` @@ -306,14 +281,6 @@ class Asn1OcspUtility { * CertId element. */ static CertId parseCertId(CBS& cbs); - - /** - * @param cbs a CBS& that refers to an ASN.1 CertStatus element - * @returns CertStatus the revocation status of a given certificate. - * @throws Envoy::EnvoyException if `cbs` does not contain a well-formed - * CertStatus element. - */ - static CertStatus parseCertStatus(CBS& cbs); }; } // namespace Ocsp diff --git a/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc b/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc index f3e37f54ec3c..e607653f9b4d 100644 --- a/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc +++ b/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc @@ -44,9 +44,8 @@ class OcspFullResponseParsingTest : public testing::Test { EXPECT_EQ(response_->rawBytes(), der_response); } - void expectCertStatus(CertStatus expected_status) { + void expectSuccessful() { EXPECT_EQ(OcspResponseStatus::Successful, response_->getResponseStatus()); - EXPECT_EQ(expected_status, response_->getCertRevocationStatus()); } void expectCertificateMatches(std::string cert_filename) { @@ -61,7 +60,7 @@ class OcspFullResponseParsingTest : public testing::Test { TEST_F(OcspFullResponseParsingTest, GoodCertTest) { setup("good_ocsp_resp.der"); - expectCertStatus(CertStatus::GOOD); + expectSuccessful(); expectCertificateMatches("good_cert.pem"); auto cert = readCertFromFile(fullPath("revoked_cert.pem")); @@ -73,14 +72,14 @@ TEST_F(OcspFullResponseParsingTest, GoodCertTest) { TEST_F(OcspFullResponseParsingTest, RevokedCertTest) { setup("revoked_ocsp_resp.der"); - expectCertStatus(CertStatus::REVOKED); + expectSuccessful(); expectCertificateMatches("revoked_cert.pem"); EXPECT_TRUE(response_->isExpired()); } TEST_F(OcspFullResponseParsingTest, UnknownCertTest) { setup("unknown_ocsp_resp.der"); - expectCertStatus(CertStatus::UNKNOWN); + expectSuccessful(); expectCertificateMatches("good_cert.pem"); EXPECT_TRUE(response_->isExpired()); } @@ -102,7 +101,7 @@ TEST_F(OcspFullResponseParsingTest, ThisUpdateAfterNowTest) { TEST_F(OcspFullResponseParsingTest, ResponderIdKeyHashTest) { setup("responder_key_hash_ocsp_resp.der"); - expectCertStatus(CertStatus::GOOD); + expectSuccessful(); expectCertificateMatches("good_cert.pem"); EXPECT_TRUE(response_->isExpired()); } @@ -268,24 +267,6 @@ TEST_F(Asn1OcspUtilityTest, ParseResponseBytesUnknownResponseTypeTest) { "Unknown OCSP Response type with OID: 1.1.1.1.1.1.1"); } -TEST_F(Asn1OcspUtilityTest, ParseCertStatusInvalidVariantTest) { - std::vector invalid_choice = {3, 0}; - CBS cbs; - CBS_init(&cbs, invalid_choice.data(), invalid_choice.size()); - - EXPECT_THROW_WITH_MESSAGE(Asn1OcspUtility::parseCertStatus(cbs), EnvoyException, - "Unknown OcspCertStatus tag: 3"); -} - -TEST_F(Asn1OcspUtilityTest, ParseCertStatusInvalidByteStringTest) { - std::vector invalid_choice; - CBS cbs; - CBS_init(&cbs, invalid_choice.data(), invalid_choice.size()); - - EXPECT_THROW_WITH_MESSAGE(Asn1OcspUtility::parseCertStatus(cbs), EnvoyException, - "Failed to parse ASN.1 element tag"); -} - } // namespace } // namespace Ocsp From 5731d84a032a28c4e223a3c8cff0119583b519c0 Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Thu, 6 Aug 2020 09:51:34 -0700 Subject: [PATCH 20/31] format Signed-off-by: Daniel Goldstein --- .../tls/ocsp/asn1_utility.cc | 27 ++++++------ .../transport_sockets/tls/ocsp/asn1_utility.h | 9 ++-- .../transport_sockets/tls/ocsp/ocsp.cc | 41 ++++++++++-------- .../transport_sockets/tls/ocsp/ocsp.h | 3 +- .../tls/ocsp/asn1_utility_test.cc | 42 +++++++++++-------- 5 files changed, 68 insertions(+), 54 deletions(-) diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc index 52b42da41cb5..fcb9e2a7d6be 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc @@ -1,4 +1,5 @@ #include "extensions/transport_sockets/tls/ocsp/asn1_utility.h" + #include "common/common/c_smart_ptr.h" #include "absl/strings/ascii.h" @@ -10,18 +11,16 @@ namespace Tls { namespace Ocsp { namespace { - // A type adapter since OPENSSL_free accepts void*. - void freeOpensslString(char* str) { - OPENSSL_free(str); - } - - // ASN1_INTEGER is a type alias for ASN1_STRING. - // This static_cast is intentional to avoid the - // c-style cast performed in M_ASN1_INTEGER_free. - void freeAsn1Integer(ASN1_INTEGER* integer) { - ASN1_STRING_free(static_cast(integer)); - } +// A type adapter since OPENSSL_free accepts void*. +void freeOpensslString(char* str) { OPENSSL_free(str); } + +// ASN1_INTEGER is a type alias for ASN1_STRING. +// This static_cast is intentional to avoid the +// c-style cast performed in M_ASN1_INTEGER_free. +void freeAsn1Integer(ASN1_INTEGER* integer) { + ASN1_STRING_free(static_cast(integer)); } +} // namespace absl::string_view Asn1Utility::cbsToString(CBS& cbs) { auto str_head = reinterpret_cast(CBS_data(&cbs)); @@ -63,8 +62,7 @@ ParsingResult Asn1Utility::parseGeneralizedTime(CBS& cbs) { // Local time or time differential, though a part of the ASN.1 // GENERALIZEDTIME spec, are not supported. // Reference: https://tools.ietf.org/html/rfc5280#section-4.1.2.5.2 - if (time_str.length() > 0 && - absl::ascii_toupper(time_str.at(time_str.length() - 1)) != 'Z') { + if (time_str.length() > 0 && absl::ascii_toupper(time_str.at(time_str.length() - 1)) != 'Z') { return "GENERALIZEDTIME must be in UTC"; } @@ -86,7 +84,8 @@ ParsingResult Asn1Utility::parseInteger(CBS& cbs) { } auto head = CBS_data(&num); - CSmartPtr asn1_integer(c2i_ASN1_INTEGER(nullptr, &head, CBS_len(&num))); + CSmartPtr asn1_integer( + c2i_ASN1_INTEGER(nullptr, &head, CBS_len(&num))); if (asn1_integer != nullptr) { BIGNUM num_bn; BN_init(&num_bn); diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h index 9c7935bbe3bf..e083b30cfbf6 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h @@ -85,7 +85,8 @@ class Asn1Utility { * else nullopt */ template - static ParsingResult> parseOptional(CBS& cbs, Asn1ParsingFunc parse_data, unsigned tag); + static ParsingResult> parseOptional(CBS& cbs, Asn1ParsingFunc parse_data, + unsigned tag); /** * Returns whether or not an element explicitly tagged with `tag` is present @@ -161,7 +162,8 @@ class Asn1Utility { }; template -ParsingResult> Asn1Utility::parseSequenceOf(CBS& cbs, Asn1ParsingFunc parse_element) { +ParsingResult> Asn1Utility::parseSequenceOf(CBS& cbs, + Asn1ParsingFunc parse_element) { CBS seq_elem; std::vector vec; @@ -187,7 +189,8 @@ ParsingResult> Asn1Utility::parseSequenceOf(CBS& cbs, Asn1Parsing } template -ParsingResult> Asn1Utility::parseOptional(CBS& cbs, Asn1ParsingFunc parse_data, unsigned tag) { +ParsingResult> Asn1Utility::parseOptional(CBS& cbs, Asn1ParsingFunc parse_data, + unsigned tag) { CBS data; auto is_present = getOptional(cbs, &data, tag); diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc index 29903fb77991..c332bb434ab8 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc @@ -15,8 +15,7 @@ namespace CertUtility = Envoy::Extensions::TransportSockets::Tls::Utility; namespace { -template -T unwrap(ParsingResult res) { +template T unwrap(ParsingResult res) { if (absl::holds_alternative(res)) { return absl::get<0>(res); } @@ -53,8 +52,10 @@ void skipResponderId(CBS& cbs) { // KeyHash ::= OCTET STRING -- SHA-1 hash of responder's public key // (excluding the tag and length fields) - if (unwrap(Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 1)) || - unwrap(Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 2))) { + if (unwrap(Asn1Utility::getOptional(cbs, nullptr, + CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 1)) || + unwrap(Asn1Utility::getOptional(cbs, nullptr, + CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 2))) { return; } @@ -62,16 +63,17 @@ void skipResponderId(CBS& cbs) { } void skipCertStatus(CBS& cbs) { - // CertStatus ::= CHOICE { - // good [0] IMPLICIT NULL, - // revoked [1] IMPLICIT RevokedInfo, - // unknown [2] IMPLICIT UnknownInfo - // } - if (!(unwrap(Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONTEXT_SPECIFIC | 0)) || - unwrap(Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 1)) || - unwrap(Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONTEXT_SPECIFIC | 2)))) { - throw EnvoyException(absl::StrCat("Unknown OcspCertStatus tag: ", parseTag(cbs))); - } + // CertStatus ::= CHOICE { + // good [0] IMPLICIT NULL, + // revoked [1] IMPLICIT RevokedInfo, + // unknown [2] IMPLICIT UnknownInfo + // } + if (!(unwrap(Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONTEXT_SPECIFIC | 0)) || + unwrap(Asn1Utility::getOptional(cbs, nullptr, + CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 1)) || + unwrap(Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONTEXT_SPECIFIC | 2)))) { + throw EnvoyException(absl::StrCat("Unknown OcspCertStatus tag: ", parseTag(cbs))); + } } } // namespace @@ -142,7 +144,8 @@ std::unique_ptr Asn1OcspUtility::parseOcspResponse(CBS& cbs) { CBS bytes; ResponsePtr resp = nullptr; - if (unwrap(Asn1Utility::getOptional(elem, &bytes, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0))) { + if (unwrap(Asn1Utility::getOptional(elem, &bytes, + CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0))) { resp = Asn1OcspUtility::parseResponseBytes(bytes); } @@ -243,9 +246,10 @@ ResponseData Asn1OcspUtility::parseResponseData(CBS& cbs) { unwrap(Asn1Utility::skipOptional(elem, 0)); skipResponderId(elem); unwrap(Asn1Utility::skip(elem, CBS_ASN1_GENERALIZEDTIME)); - auto responses = unwrap(Asn1Utility::parseSequenceOf(elem, [](CBS& cbs) -> ParsingResult { + auto responses = unwrap(Asn1Utility::parseSequenceOf( + elem, [](CBS& cbs) -> ParsingResult { return ParsingResult(parseSingleResponse(cbs)); - })); + })); // Extensions currently ignored return {std::move(responses)}; @@ -268,7 +272,8 @@ SingleResponse Asn1OcspUtility::parseSingleResponse(CBS& cbs) { skipCertStatus(elem); auto this_update = unwrap(Asn1Utility::parseGeneralizedTime(elem)); auto next_update = unwrap(Asn1Utility::parseOptional( - elem, Asn1Utility::parseGeneralizedTime, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0)); + elem, Asn1Utility::parseGeneralizedTime, + CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0)); // Extensions currently ignored return {cert_id, this_update, next_update}; diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.h b/source/extensions/transport_sockets/tls/ocsp/ocsp.h index b3e84ad033b0..12ce73d354c3 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.h +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.h @@ -7,8 +7,8 @@ #include "envoy/common/exception.h" #include "envoy/common/time.h" -#include "absl/types/optional.h" #include "absl/strings/string_view.h" +#include "absl/types/optional.h" #include "openssl/bytestring.h" #include "openssl/ssl.h" @@ -157,6 +157,7 @@ class BasicOcspResponse : public Response { // Identified as `id-pkix-ocsp-basic` in // https://tools.ietf.org/html/rfc6960#appendix-B.2 constexpr static absl::string_view OID = "1.3.6.1.5.5.7.48.1.1"; + private: const ResponseData data_; }; diff --git a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc index 9d50736dbf28..afa8db10abb8 100644 --- a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc +++ b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc @@ -49,8 +49,9 @@ class Asn1UtilityTest : public testing::Test { }; TEST_F(Asn1UtilityTest, ParseMethodsWrongTagTest) { - expectParseResultErrorOnWrongTag>>( - [](CBS& cbs) { return Asn1Utility::parseSequenceOf>(cbs, Asn1Utility::parseOctetString); }); + expectParseResultErrorOnWrongTag>>([](CBS& cbs) { + return Asn1Utility::parseSequenceOf>(cbs, Asn1Utility::parseOctetString); + }); expectParseResultErrorOnWrongTag(Asn1Utility::parseOid); expectParseResultErrorOnWrongTag(Asn1Utility::parseGeneralizedTime); expectParseResultErrorOnWrongTag(Asn1Utility::parseInteger); @@ -69,8 +70,8 @@ TEST_F(Asn1UtilityTest, ParseSequenceOfEmptySequenceTest) { CBS_init(&cbs, asn1_empty_seq.data(), asn1_empty_seq.size()); std::vector> vec; - auto actual = absl::get<0>(Asn1Utility::parseSequenceOf>(cbs, - Asn1Utility::parseOctetString)); + auto actual = absl::get<0>( + Asn1Utility::parseSequenceOf>(cbs, Asn1Utility::parseOctetString)); EXPECT_EQ(vec, actual); } @@ -99,7 +100,8 @@ TEST_F(Asn1UtilityTest, ParseSequenceOfMultipleElementSequenceTest) { CBS_init(&cbs, octet_seq.data(), octet_seq.size()); std::vector> vec = {{0x1, 0x2}, {0x3, 0x4}, {0x5, 0x6}}; - auto actual = absl::get<0>(Asn1Utility::parseSequenceOf>(cbs, Asn1Utility::parseOctetString)); + auto actual = absl::get<0>( + Asn1Utility::parseSequenceOf>(cbs, Asn1Utility::parseOctetString)); EXPECT_EQ(vec, actual); } @@ -118,7 +120,8 @@ TEST_F(Asn1UtilityTest, SequenceOfLengthMismatchErrorTest) { CBS_init(&cbs, malformed.data(), malformed.size()); EXPECT_EQ("Input is not a well-formed ASN.1 OCTETSTRING", - absl::get<1>(Asn1Utility::parseSequenceOf>(cbs, Asn1Utility::parseOctetString))); + absl::get<1>(Asn1Utility::parseSequenceOf>( + cbs, Asn1Utility::parseOctetString))); } TEST_F(Asn1UtilityTest, SequenceOfMixedTypeErrorTest) { @@ -140,7 +143,8 @@ TEST_F(Asn1UtilityTest, SequenceOfMixedTypeErrorTest) { CBS_init(&cbs, mixed_type.data(), mixed_type.size()); EXPECT_EQ("Input is not a well-formed ASN.1 OCTETSTRING", - absl::get<1>(Asn1Utility::parseSequenceOf>(cbs, Asn1Utility::parseOctetString))); + absl::get<1>(Asn1Utility::parseSequenceOf>( + cbs, Asn1Utility::parseOctetString))); } TEST_F(Asn1UtilityTest, IsOptionalPresentTest) { @@ -161,7 +165,7 @@ TEST_F(Asn1UtilityTest, IsOptionalPresentMissingValueTest) { CBS_init(&cbs, missing_val_bool.data(), missing_val_bool.size()); EXPECT_EQ("Failed to parse ASN.1 element tag", - absl::get<1>(Asn1Utility::getOptional(cbs, &value, CBS_ASN1_BOOLEAN))); + absl::get<1>(Asn1Utility::getOptional(cbs, &value, CBS_ASN1_BOOLEAN))); } TEST_F(Asn1UtilityTest, ParseOptionalTest) { @@ -182,11 +186,12 @@ TEST_F(Asn1UtilityTest, ParseOptionalTest) { }; absl::optional expected(true); - EXPECT_EQ(expected, absl::get<0>(Asn1Utility::parseOptional(cbs_explicit_optional_true, parseBool, 0))); - EXPECT_EQ(absl::nullopt, - absl::get<0>(Asn1Utility::parseOptional(cbs_empty_seq, parseBool, CBS_ASN1_BOOLEAN))); - EXPECT_EQ(absl::nullopt, - absl::get<0>(Asn1Utility::parseOptional(cbs_nothing, parseBool, CBS_ASN1_BOOLEAN))); + EXPECT_EQ(expected, absl::get<0>(Asn1Utility::parseOptional(cbs_explicit_optional_true, + parseBool, 0))); + EXPECT_EQ(absl::nullopt, absl::get<0>(Asn1Utility::parseOptional(cbs_empty_seq, parseBool, + CBS_ASN1_BOOLEAN))); + EXPECT_EQ(absl::nullopt, absl::get<0>(Asn1Utility::parseOptional(cbs_nothing, parseBool, + CBS_ASN1_BOOLEAN))); } TEST_F(Asn1UtilityTest, ParseOidTest) { @@ -214,7 +219,7 @@ TEST_F(Asn1UtilityTest, ParseGeneralizedTimeWrongFormatErrorTest) { bssl::UniquePtr scoped(asn1Encode(cbs, invalid_time, CBS_ASN1_GENERALIZEDTIME)); Asn1Utility::parseGeneralizedTime(cbs); EXPECT_EQ("Input is not a well-formed ASN.1 GENERALIZEDTIME", - absl::get(Asn1Utility::parseGeneralizedTime(cbs))); + absl::get(Asn1Utility::parseGeneralizedTime(cbs))); } TEST_F(Asn1UtilityTest, ParseGeneralizedTimeTest) { @@ -234,7 +239,8 @@ TEST_F(Asn1UtilityTest, TestParseGeneralizedTimeRejectsNonUTCTime) { CBS cbs; bssl::UniquePtr scoped(asn1Encode(cbs, local_time, CBS_ASN1_GENERALIZEDTIME)); - EXPECT_EQ("GENERALIZEDTIME must be in UTC", absl::get(Asn1Utility::parseGeneralizedTime(cbs))); + EXPECT_EQ("GENERALIZEDTIME must be in UTC", + absl::get(Asn1Utility::parseGeneralizedTime(cbs))); } TEST_F(Asn1UtilityTest, TestParseGeneralizedTimeInvalidTime) { @@ -243,7 +249,7 @@ TEST_F(Asn1UtilityTest, TestParseGeneralizedTimeInvalidTime) { bssl::UniquePtr scoped(asn1Encode(cbs, ymd, CBS_ASN1_GENERALIZEDTIME)); EXPECT_EQ("Error parsing string of GENERALIZEDTIME format", - absl::get<1>(Asn1Utility::parseGeneralizedTime(cbs))); + absl::get<1>(Asn1Utility::parseGeneralizedTime(cbs))); } // Taken from @@ -298,7 +304,7 @@ TEST_F(Asn1UtilityTest, ParseIntegerTest) { } TEST_F(Asn1UtilityTest, ParseOctetStringTest) { - std::vector data = { 0x1, 0x2, 0x3 }; + std::vector data = {0x1, 0x2, 0x3}; std::string data_str(data.begin(), data.end()); CBS cbs; bssl::UniquePtr scoped(asn1Encode(cbs, data_str, CBS_ASN1_OCTETSTRING)); @@ -330,7 +336,7 @@ TEST_F(Asn1UtilityTest, SkipOptionalMalformedTagTest) { CBS_init(&cbs, malformed_seq.data(), malformed_seq.size()); EXPECT_EQ("Failed to parse ASN.1 element tag", - absl::get<1>(Asn1Utility::skipOptional(cbs, CBS_ASN1_SEQUENCE))); + absl::get<1>(Asn1Utility::skipOptional(cbs, CBS_ASN1_SEQUENCE))); } } // namespace From 07afafcf6a143d5a49338f793e3d360614e658a9 Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Thu, 6 Aug 2020 11:01:48 -0700 Subject: [PATCH 21/31] update docs Signed-off-by: Daniel Goldstein --- .../transport_sockets/tls/ocsp/asn1_utility.h | 46 ++++++++++--------- .../transport_sockets/tls/ocsp/ocsp.h | 18 ++++---- 2 files changed, 34 insertions(+), 30 deletions(-) diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h index e083b30cfbf6..823920ee6c2e 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h @@ -23,6 +23,10 @@ namespace Ocsp { constexpr absl::string_view GENERALIZED_TIME_FORMAT = "%E4Y%m%d%H%M%S"; +/** + * The result of parsing an ASN.1 structure or an `absl::string_view` error + * description. + */ template using ParsingResult = absl::variant; /** @@ -56,7 +60,7 @@ class Asn1Utility { /** * Extracts the full contents of `cbs` as a string. * - * @param cbs a CBS& that refers to the current document position + * @param `cbs` a CBS& that refers to the current document position * @returns absl::string_view containing the contents of `cbs` */ static absl::string_view cbsToString(CBS& cbs); @@ -67,8 +71,8 @@ class Asn1Utility { * * @param cbs a CBS& that refers to an ASN.1 SEQUENCE OF object * @param parse_element an Asn1ParsingFunc used to parse each element - * @returns std::vector containing the parsed elements of the sequence. - * @throws Envoy::EnvoyException if `cbs` does not point to a well-formed + * @returns ParsingResult> containing the parsed elements of the sequence + * or an error string if `cbs` does not point to a well-formed * SEQUENCE OF object. */ template @@ -81,8 +85,8 @@ class Asn1Utility { * * @param cbs a CBS& that refers to the current document position * @param parse_data an Asn1ParsingFunc used to parse the data if present - * @return absl::optional with a T if `cbs` is of the specified tag, - * else nullopt + * @return ParsingResult> with a `T` if `cbs` is of the specified tag, + * nullopt if the element has a different tag, or an error string if parsing fails. */ template static ParsingResult> parseOptional(CBS& cbs, Asn1ParsingFunc parse_data, @@ -96,25 +100,24 @@ class Asn1Utility { * * @param cbs a CBS& that refers to the current document position * @param data a CBS* that is set to the contents of `cbs` if present - * @param an explicit tag indicating an optional value + * @param an unsigned explicit tag indicating an optional value * - * @returns bool whether `cbs` points to an element tagged with `tag` - * @throws Envoy::EnvoyException if `cbs` is a malformed TLV bytestring + * @returns ParsingResult whether `cbs` points to an element tagged with `tag` or + * an error string if parsing fails. */ static ParsingResult getOptional(CBS& cbs, CBS* data, unsigned tag); /** * @param cbs a CBS& that refers to an ASN.1 OBJECT IDENTIFIER element - * @returns std::string the OID encoded in `cbs` - * @throws Envoy::EnvoyException if `cbs` does not point to a well-formed - * OBJECT IDENTIFIER + * @returns ParsingResult the OID encoded in `cbs` or an error + * string if `cbs` does not point to a well-formed OBJECT IDENTIFIER */ static ParsingResult parseOid(CBS& cbs); /** * @param cbs a CBS& that refers to an ASN.1 GENERALIZEDTIME element - * @returns Envoy::SystemTime the UTC timestamp encoded in `cbs` - * @throws Envoy::EnvoyException if `cbs` does not point to a well-formed + * @returns ParsingResult the UTC timestamp encoded in `cbs` + * or an error string if `cbs` does not point to a well-formed * GENERALIZEDTIME */ static ParsingResult parseGeneralizedTime(CBS& cbs); @@ -126,17 +129,15 @@ class Asn1Utility { * use CBS_get_asn1_* functions for the given integer type instead. * * @param cbs a CBS& that refers to an ASN.1 INTEGER element - * @returns std::string a hex representation of the integer - * @throws Envoy::EnvoyException if `cbs` does not point to a well-formed - * INTEGER + * @returns ParsingResult a hex representation of the integer + * or an error string if `cbs` does not point to a well-formed INTEGER */ static ParsingResult parseInteger(CBS& cbs); /** * @param cbs a CBS& that refers to an ASN.1 OCTETSTRING element - * @returns absl::string_view of the octets in `cbs` - * @throws Envoy::EnvoyException if `cbs` does not point to a well-formed - * OCTETSTRING + * @returns ParsingResult> the octets in `cbs` or + * an error string if `cbs` does not point to a well-formed OCTETSTRING */ static ParsingResult> parseOctetString(CBS& cbs); @@ -146,7 +147,8 @@ class Asn1Utility { * * @param cbs a CBS& that refers to the current document position * @param tag the tag of the value to skip - * @throws Envoy::EnvoyException if `cbs` is a malformed TLV bytestring + * @returns ParsingResult a unit type denoting success + * or an error string if parsing fails. */ static ParsingResult skipOptional(CBS& cbs, unsigned tag); @@ -155,8 +157,8 @@ class Asn1Utility { * * @param cbs a CBS& that refers to the current document position * @param tag the tag of the value to skip - * @throws Envoy::EnvoyException if `cbs` does not point to a well-formed - * element with tag `tag`. + * @returns ParsingResult a unit type denoting success + * or an error string if parsing fails. */ static ParsingResult skip(CBS& cbs, unsigned tag); }; diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.h b/source/extensions/transport_sockets/tls/ocsp/ocsp.h index 12ce73d354c3..ec1c2c9b18c2 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.h +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.h @@ -18,8 +18,8 @@ * * WARNING: This module is meant to validate that OCSP responses are well-formed * and extract useful fields for OCSP stapling. This assumes that responses are - * provided from configs or another trusted source and does not perform the - * necessary checks to verify responses coming from an upstream server. + * provided from configs or another trusted source and does not perform + * checks necessary to verify responses coming from an upstream server. */ namespace Envoy { @@ -51,8 +51,8 @@ enum class OcspResponseStatus { }; /** - * Reflection of the ASN.1 CertId structure. - * Contains the information to uniquely identify an SSL Certificate. + * Partial reflection of the ASN.1 CertId structure. + * Contains the information to identify an SSL Certificate. * Serial numbers are guaranteed to be * unique per issuer but not necessarily universally. */ @@ -63,7 +63,7 @@ struct CertId { }; /** - * Reflection of the ASN.1 SingleResponse structure. + * Partial reflection of the ASN.1 SingleResponse structure. * Contains information about the OCSP status of a single certificate. * An OCSP request may request the status of multiple certificates and * therefore responses may contain multiple SingleResponses. @@ -83,7 +83,7 @@ struct SingleResponse { }; /** - * Reflection of the ASN.1 ResponseData structure. + * Partial reflection of the ASN.1 ResponseData structure. * Contains an OCSP response for each certificate in a given request * as well as the time at which the response was produced. */ @@ -97,7 +97,7 @@ struct ResponseData { * An abstract type for OCSP response formats. Which variant of `Response` is * used in an `OcspResponse` is indicated by the structure's OID. * - * We currently enforce that OCSP responses must be for a single certificate + * Envoy enforces that OCSP responses must be for a single certificate * only. The methods on this class extract the relevant information for the * single certificate contained in the response. */ @@ -135,6 +135,8 @@ using ResponsePtr = std::unique_ptr; /** * Reflection of the ASN.1 BasicOcspResponse structure. * Contains the full data of an OCSP response. + * Envoy enforces that OCSP responses contain a response for only + * a single certificate. * * BasicOcspResponse is the only supported Response type in RFC 6960. */ @@ -225,7 +227,7 @@ using OcspResponseWrapperPtr = std::unique_ptr; class Asn1OcspUtility { public: /** - * @param cbs a CBS& that refers to an ASN.1 OcspResponse element + * @param `cbs` a CBS& that refers to an ASN.1 OcspResponse element * @returns std::unique_ptr the OCSP response encoded in `cbs` * @throws Envoy::EnvoyException if `cbs` does not contain a well-formed OcspResponse * element. From 8a5ba3ab8df59e7e23526794cb05a624c2979062 Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Thu, 6 Aug 2020 11:10:13 -0700 Subject: [PATCH 22/31] fix spelling Signed-off-by: Daniel Goldstein --- .../extensions/transport_sockets/tls/ocsp/asn1_utility.cc | 2 +- source/extensions/transport_sockets/tls/ocsp/ocsp.cc | 6 +++--- tools/spelling/spelling_dictionary.txt | 1 + 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc index fcb9e2a7d6be..19f481b4b1a2 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc @@ -76,7 +76,7 @@ ParsingResult Asn1Utility::parseGeneralizedTime(CBS& cbs) { } // Performs the following conversions to go from bytestring to hex integer -// CBS -> ASN1_INTEGER -> BIGNUM -> String +// CBS -> ASN1_INTEGER -> BIGNUM -> String. ParsingResult Asn1Utility::parseInteger(CBS& cbs) { CBS num; if (!CBS_get_asn1(&cbs, &num, CBS_ASN1_INTEGER)) { diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc index c332bb434ab8..f0e477b2d588 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc @@ -100,7 +100,7 @@ OcspResponseWrapper::OcspResponseWrapper(std::vector der_response, Time throw EnvoyException("OCSP response has no body"); } - // We only permit a 1:1 of certificate to response + // We only permit a 1:1 of certificate to response. if (response_->response_->getNumCerts() != 1) { throw EnvoyException("OCSP Response must be for one certificate only"); } @@ -250,7 +250,7 @@ ResponseData Asn1OcspUtility::parseResponseData(CBS& cbs) { elem, [](CBS& cbs) -> ParsingResult { return ParsingResult(parseSingleResponse(cbs)); })); - // Extensions currently ignored + // Extensions currently ignored. return {std::move(responses)}; } @@ -274,7 +274,7 @@ SingleResponse Asn1OcspUtility::parseSingleResponse(CBS& cbs) { auto next_update = unwrap(Asn1Utility::parseOptional( elem, Asn1Utility::parseGeneralizedTime, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0)); - // Extensions currently ignored + // Extensions currently ignored. return {cert_id, this_update, next_update}; } diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index 5310dd22d154..e832e9ce7a1b 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -186,6 +186,7 @@ MB MD MERCHANTABILITY MGET +MONOSTATE MQ MSET MSVC From 4f3d4058bb9c6d273e267d50ddc96b612b2a5e93 Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Thu, 6 Aug 2020 12:38:27 -0700 Subject: [PATCH 23/31] add envoy extension package to BUILD Signed-off-by: Daniel Goldstein --- source/extensions/transport_sockets/tls/ocsp/BUILD | 3 +++ 1 file changed, 3 insertions(+) diff --git a/source/extensions/transport_sockets/tls/ocsp/BUILD b/source/extensions/transport_sockets/tls/ocsp/BUILD index 8118f5e46f35..8934311abf7f 100644 --- a/source/extensions/transport_sockets/tls/ocsp/BUILD +++ b/source/extensions/transport_sockets/tls/ocsp/BUILD @@ -1,11 +1,14 @@ load( "//bazel:envoy_build_system.bzl", "envoy_cc_library", + "envoy_extension_package", "envoy_package", ) licenses(["notice"]) # Apache 2 +envoy_extension_package() + envoy_package() envoy_cc_library( From 57b5ff4a0f90cf498bc3e8e22be32f48cc84348a Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Thu, 6 Aug 2020 13:01:04 -0700 Subject: [PATCH 24/31] remove envoy_package and replace with envoy_extension_package Signed-off-by: Daniel Goldstein --- source/extensions/transport_sockets/tls/ocsp/BUILD | 2 -- 1 file changed, 2 deletions(-) diff --git a/source/extensions/transport_sockets/tls/ocsp/BUILD b/source/extensions/transport_sockets/tls/ocsp/BUILD index 8934311abf7f..dc2af6edff1a 100644 --- a/source/extensions/transport_sockets/tls/ocsp/BUILD +++ b/source/extensions/transport_sockets/tls/ocsp/BUILD @@ -9,8 +9,6 @@ licenses(["notice"]) # Apache 2 envoy_extension_package() -envoy_package() - envoy_cc_library( name = "ocsp_lib", srcs = ["ocsp.cc"], From bc880d36e69c754920646813954c3062c91fdc2f Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Thu, 6 Aug 2020 13:31:25 -0700 Subject: [PATCH 25/31] removee envoy_package import Signed-off-by: Daniel Goldstein --- source/extensions/transport_sockets/tls/ocsp/BUILD | 1 - 1 file changed, 1 deletion(-) diff --git a/source/extensions/transport_sockets/tls/ocsp/BUILD b/source/extensions/transport_sockets/tls/ocsp/BUILD index dc2af6edff1a..e2995afd45cd 100644 --- a/source/extensions/transport_sockets/tls/ocsp/BUILD +++ b/source/extensions/transport_sockets/tls/ocsp/BUILD @@ -2,7 +2,6 @@ load( "//bazel:envoy_build_system.bzl", "envoy_cc_library", "envoy_extension_package", - "envoy_package", ) licenses(["notice"]) # Apache 2 From 51988e577293a140a783a4d7ff9feaf411ae41ca Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Thu, 6 Aug 2020 14:46:08 -0700 Subject: [PATCH 26/31] backtick most new words instead of adding to the dictionary Signed-off-by: Daniel Goldstein --- .../tls/ocsp/asn1_utility.cc | 12 ++-- .../transport_sockets/tls/ocsp/asn1_utility.h | 55 +++++++++---------- .../transport_sockets/tls/ocsp/ocsp.cc | 4 +- .../transport_sockets/tls/ocsp/ocsp.h | 30 +++++----- .../tls/ocsp/asn1_utility_test.cc | 6 +- .../transport_sockets/tls/ocsp/ocsp_test.cc | 4 +- tools/spelling/spelling_dictionary.txt | 7 --- 7 files changed, 55 insertions(+), 63 deletions(-) diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc index 19f481b4b1a2..8000294b39fd 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc @@ -14,9 +14,9 @@ namespace { // A type adapter since OPENSSL_free accepts void*. void freeOpensslString(char* str) { OPENSSL_free(str); } -// ASN1_INTEGER is a type alias for ASN1_STRING. +// `ASN1_INTEGER` is a type alias for `ASN1_STRING`. // This static_cast is intentional to avoid the -// c-style cast performed in M_ASN1_INTEGER_free. +// c-style cast performed in `M_ASN1_INTEGER_free`. void freeAsn1Integer(ASN1_INTEGER* integer) { ASN1_STRING_free(static_cast(integer)); } @@ -57,10 +57,10 @@ ParsingResult Asn1Utility::parseGeneralizedTime(CBS& cbs) { } auto time_str = cbsToString(elem); - // OCSP follows the RFC 5280 enforcement that GENERALIZEDTIME + // OCSP follows the RFC 5280 enforcement that `GENERALIZEDTIME` // fields MUST be in UTC, so must be suffixed with a Z character. - // Local time or time differential, though a part of the ASN.1 - // GENERALIZEDTIME spec, are not supported. + // Local time or time differential, though a part of the `ASN.1` + // `GENERALIZEDTIME` spec, are not supported. // Reference: https://tools.ietf.org/html/rfc5280#section-4.1.2.5.2 if (time_str.length() > 0 && absl::ascii_toupper(time_str.at(time_str.length() - 1)) != 'Z') { return "GENERALIZEDTIME must be in UTC"; @@ -76,7 +76,7 @@ ParsingResult Asn1Utility::parseGeneralizedTime(CBS& cbs) { } // Performs the following conversions to go from bytestring to hex integer -// CBS -> ASN1_INTEGER -> BIGNUM -> String. +// `CBS` -> `ASN1_INTEGER` -> `BIGNUM` -> String. ParsingResult Asn1Utility::parseInteger(CBS& cbs) { CBS num; if (!CBS_get_asn1(&cbs, &num, CBS_ASN1_INTEGER)) { diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h index 823920ee6c2e..63040b5a0efd 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h @@ -24,7 +24,7 @@ namespace Ocsp { constexpr absl::string_view GENERALIZED_TIME_FORMAT = "%E4Y%m%d%H%M%S"; /** - * The result of parsing an ASN.1 structure or an `absl::string_view` error + * The result of parsing an `ASN.1` structure or an `absl::string_view` error * description. */ template using ParsingResult = absl::variant; @@ -36,21 +36,21 @@ template using ParsingResult = absl::variant; template using Asn1ParsingFunc = std::function(CBS&)>; /** - * Utility functions for parsing DER-encoded ASN.1 objects. + * Utility functions for parsing DER-encoded `ASN.1` objects. * This relies heavily on the 'openssl/bytestring' API which * is BoringSSL's recommended interface for parsing DER-encoded - * ASN.1 data when there is not an existing wrapper. - * This is not a complete library for ASN.1 parsing and primarily + * `ASN.1` data when there is not an existing wrapper. + * This is not a complete library for `ASN.1` parsing and primarily * serves as abstractions for the OCSP module, but can be * extended and moved into a general utility to support parsing of - * additional ASN.1 objects. + * additional `ASN.1` objects. * * Each function adheres to the invariant that given a reference - * to a crypto bytestring (CBS&), it will parse the specified - * ASN.1 element and advance `cbs` over it. + * to a crypto `bytestring` (CBS&), it will parse the specified + * `ASN.1` element and advance `cbs` over it. * - * An exception is thrown if the bytestring is malformed or does - * not match the specified ASN.1 object. The position + * An exception is thrown if the `bytestring` is malformed or does + * not match the specified `ASN.1` object. The position * of `cbs` is not reliable after an exception is thrown. */ class Asn1Utility { @@ -66,11 +66,11 @@ class Asn1Utility { static absl::string_view cbsToString(CBS& cbs); /** - * Parses all elements of an ASN.1 SEQUENCE OF. `parse_element` must + * Parses all elements of an `ASN.1` SEQUENCE OF. `parse_element` must * advance its input CBS& over the entire element. * - * @param cbs a CBS& that refers to an ASN.1 SEQUENCE OF object - * @param parse_element an Asn1ParsingFunc used to parse each element + * @param cbs a CBS& that refers to an `ASN.1` SEQUENCE OF object + * @param parse_element an `Asn1ParsingFunc` used to parse each element * @returns ParsingResult> containing the parsed elements of the sequence * or an error string if `cbs` does not point to a well-formed * SEQUENCE OF object. @@ -84,7 +84,7 @@ class Asn1Utility { * `cbs` is not advanced. * * @param cbs a CBS& that refers to the current document position - * @param parse_data an Asn1ParsingFunc used to parse the data if present + * @param parse_data an `Asn1ParsingFunc` used to parse the data if present * @return ParsingResult> with a `T` if `cbs` is of the specified tag, * nullopt if the element has a different tag, or an error string if parsing fails. */ @@ -99,7 +99,6 @@ class Asn1Utility { * If `cbs` does not contain `tag`, `cbs` remains at the same position. * * @param cbs a CBS& that refers to the current document position - * @param data a CBS* that is set to the contents of `cbs` if present * @param an unsigned explicit tag indicating an optional value * * @returns ParsingResult whether `cbs` points to an element tagged with `tag` or @@ -108,56 +107,56 @@ class Asn1Utility { static ParsingResult getOptional(CBS& cbs, CBS* data, unsigned tag); /** - * @param cbs a CBS& that refers to an ASN.1 OBJECT IDENTIFIER element - * @returns ParsingResult the OID encoded in `cbs` or an error + * @param cbs a CBS& that refers to an `ASN.1` OBJECT IDENTIFIER element + * @returns ParsingResult the `OID` encoded in `cbs` or an error * string if `cbs` does not point to a well-formed OBJECT IDENTIFIER */ static ParsingResult parseOid(CBS& cbs); /** - * @param cbs a CBS& that refers to an ASN.1 GENERALIZEDTIME element + * @param cbs a CBS& that refers to an `ASN.1` `GENERALIZEDTIME` element * @returns ParsingResult the UTC timestamp encoded in `cbs` * or an error string if `cbs` does not point to a well-formed - * GENERALIZEDTIME + * `GENERALIZEDTIME` */ static ParsingResult parseGeneralizedTime(CBS& cbs); /** - * Parses an ASN.1 INTEGER type into its hex string representation. - * ASN.1 INTEGER types are arbitrary precision. + * Parses an `ASN.1` INTEGER type into its hex string representation. + * `ASN.1` INTEGER types are arbitrary precision. * If you're SURE the integer fits into a fixed-size int, - * use CBS_get_asn1_* functions for the given integer type instead. + * use `CBS_get_asn1_*` functions for the given integer type instead. * - * @param cbs a CBS& that refers to an ASN.1 INTEGER element + * @param cbs a CBS& that refers to an `ASN.1` INTEGER element * @returns ParsingResult a hex representation of the integer * or an error string if `cbs` does not point to a well-formed INTEGER */ static ParsingResult parseInteger(CBS& cbs); /** - * @param cbs a CBS& that refers to an ASN.1 OCTETSTRING element + * @param cbs a CBS& that refers to an `ASN.1` `OCTETSTRING` element * @returns ParsingResult> the octets in `cbs` or - * an error string if `cbs` does not point to a well-formed OCTETSTRING + * an error string if `cbs` does not point to a well-formed `OCTETSTRING` */ static ParsingResult> parseOctetString(CBS& cbs); /** - * Advance `cbs` over an ASN.1 value of the class `tag` if that + * Advance `cbs` over an `ASN.1` value of the class `tag` if that * value is present. Otherwise, `cbs` stays in the same position. * * @param cbs a CBS& that refers to the current document position * @param tag the tag of the value to skip - * @returns ParsingResult a unit type denoting success + * @returns `ParsingResult` a unit type denoting success * or an error string if parsing fails. */ static ParsingResult skipOptional(CBS& cbs, unsigned tag); /** - * Advance `cbs` over an ASN.1 value of the class `tag`. + * Advance `cbs` over an `ASN.1` value of the class `tag`. * * @param cbs a CBS& that refers to the current document position * @param tag the tag of the value to skip - * @returns ParsingResult a unit type denoting success + * @returns `ParsingResult` a unit type denoting success * or an error string if parsing fails. */ static ParsingResult skip(CBS& cbs, unsigned tag); diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc index f0e477b2d588..097acaabbf6c 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc @@ -213,8 +213,8 @@ std::unique_ptr Asn1OcspUtility::parseBasicOcspResponse(CBS& // BasicOCSPResponse ::= SEQUENCE { // tbsResponseData ResponseData, // signatureAlgorithm AlgorithmIdentifier{SIGNATURE-ALGORITHM, - // {sa-dsaWithSHA1 | sa-rsaWithSHA1 | - // sa-rsaWithMD5 | sa-rsaWithMD2, ...}}, + // {`sa-dsaWithSHA1` | `sa-rsaWithSHA1` | + // `sa-rsaWithMD5` | `sa-rsaWithMD2`, ...}}, // signature BIT STRING, // certs [0] EXPLICIT SEQUENCE OF Certificate OPTIONAL // } diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.h b/source/extensions/transport_sockets/tls/ocsp/ocsp.h index ec1c2c9b18c2..35c274ee1579 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.h +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.h @@ -29,7 +29,7 @@ namespace Tls { namespace Ocsp { /** - * Reflection of the ASN.1 OcspResponseStatus enumeration. + * Reflection of the `ASN.1` OcspResponseStatus enumeration. * The possible statuses that can accompany an OCSP response. */ enum class OcspResponseStatus { @@ -51,7 +51,7 @@ enum class OcspResponseStatus { }; /** - * Partial reflection of the ASN.1 CertId structure. + * Partial reflection of the `ASN.1` CertId structure. * Contains the information to identify an SSL Certificate. * Serial numbers are guaranteed to be * unique per issuer but not necessarily universally. @@ -63,7 +63,7 @@ struct CertId { }; /** - * Partial reflection of the ASN.1 SingleResponse structure. + * Partial reflection of the `ASN.1` SingleResponse structure. * Contains information about the OCSP status of a single certificate. * An OCSP request may request the status of multiple certificates and * therefore responses may contain multiple SingleResponses. @@ -83,7 +83,7 @@ struct SingleResponse { }; /** - * Partial reflection of the ASN.1 ResponseData structure. + * Partial reflection of the `ASN.1` ResponseData structure. * Contains an OCSP response for each certificate in a given request * as well as the time at which the response was produced. */ @@ -95,7 +95,7 @@ struct ResponseData { /** * An abstract type for OCSP response formats. Which variant of `Response` is - * used in an `OcspResponse` is indicated by the structure's OID. + * used in an `OcspResponse` is indicated by the structure's `OID`. * * Envoy enforces that OCSP responses must be for a single certificate * only. The methods on this class extract the relevant information for the @@ -133,7 +133,7 @@ class Response { using ResponsePtr = std::unique_ptr; /** - * Reflection of the ASN.1 BasicOcspResponse structure. + * Reflection of the `ASN.1` BasicOcspResponse structure. * Contains the full data of an OCSP response. * Envoy enforces that OCSP responses contain a response for only * a single certificate. @@ -165,7 +165,7 @@ class BasicOcspResponse : public Response { }; /** - * Reflection of the ASN.1 OcspResponse structure. + * Reflection of the `ASN.1` OcspResponse structure. * This is the top-level data structure for OCSP responses. */ struct OcspResponse { @@ -219,7 +219,7 @@ class OcspResponseWrapper { using OcspResponseWrapperPtr = std::unique_ptr; /** - * ASN.1 DER-encoded parsing functions similar to Asn1Utility but specifically + * `ASN.1` DER-encoded parsing functions similar to `Asn1Utility` but specifically * for structures related to OCSP. * * Each function must advance `cbs` across the element it refers to. @@ -227,7 +227,7 @@ using OcspResponseWrapperPtr = std::unique_ptr; class Asn1OcspUtility { public: /** - * @param `cbs` a CBS& that refers to an ASN.1 OcspResponse element + * @param `cbs` a CBS& that refers to an `ASN.1` OcspResponse element * @returns std::unique_ptr the OCSP response encoded in `cbs` * @throws Envoy::EnvoyException if `cbs` does not contain a well-formed OcspResponse * element. @@ -235,7 +235,7 @@ class Asn1OcspUtility { static std::unique_ptr parseOcspResponse(CBS& cbs); /** - * @param cbs a CBS& that refers to an ASN.1 OcspResponseStatus element + * @param cbs a CBS& that refers to an `ASN.1` OcspResponseStatus element * @returns OcspResponseStatus the OCSP response encoded in `cbs` * @throws Envoy::EnvoyException if `cbs` does not contain a well-formed * OcspResponseStatus element. @@ -243,7 +243,7 @@ class Asn1OcspUtility { static OcspResponseStatus parseResponseStatus(CBS& cbs); /** - * @param cbs a CBS& that refers to an ASN.1 Response element + * @param cbs a CBS& that refers to an `ASN.1` Response element * @returns Response containing the content of an OCSP response * @throws Envoy::EnvoyException if `cbs` does not contain a well-formed * structure that is a valid Response type. @@ -251,7 +251,7 @@ class Asn1OcspUtility { static ResponsePtr parseResponseBytes(CBS& cbs); /** - * @param cbs a CBS& that refers to an ASN.1 BasicOcspResponse element + * @param cbs a CBS& that refers to an `ASN.1` BasicOcspResponse element * @returns BasicOcspResponse containing the content of an OCSP response * @throws Envoy::EnvoyException if `cbs` does not contain a well-formed * BasicOcspResponse element. @@ -259,7 +259,7 @@ class Asn1OcspUtility { static std::unique_ptr parseBasicOcspResponse(CBS& cbs); /** - * @param cbs a CBS& that refers to an ASN.1 ResponseData element + * @param cbs a CBS& that refers to an `ASN.1` ResponseData element * @returns ResponseData containing the content of an OCSP response relating * to certificate statuses. * @throws Envoy::EnvoyException if `cbs` does not contain a well-formed @@ -268,7 +268,7 @@ class Asn1OcspUtility { static ResponseData parseResponseData(CBS& cbs); /** - * @param cbs a CBS& that refers to an ASN.1 SingleResponse element + * @param cbs a CBS& that refers to an `ASN.1` SingleResponse element * @returns SingleResponse containing the id and revocation status of * a single certificate. * @throws Envoy::EnvoyException if `cbs` does not contain a well-formed @@ -277,7 +277,7 @@ class Asn1OcspUtility { static SingleResponse parseSingleResponse(CBS& cbs); /** - * @param cbs a CBS& that refers to an ASN.1 CertId element + * @param cbs a CBS& that refers to an `ASN.1` CertId element * @returns CertId containing the information necessary to uniquely identify * an SSL certificate. * @throws Envoy::EnvoyException if `cbs` does not contain a well-formed diff --git a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc index afa8db10abb8..51785b413a41 100644 --- a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc +++ b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc @@ -17,7 +17,7 @@ namespace { class Asn1UtilityTest : public testing::Test { public: - // DER encoding of a single TLV ASN.1 element. + // DER encoding of a single TLV `ASN.1` element. // returns a pointer to the underlying buffer and transfers // ownership to the caller. uint8_t* asn1Encode(CBS& cbs, std::string& value, unsigned tag) { @@ -147,7 +147,7 @@ TEST_F(Asn1UtilityTest, SequenceOfMixedTypeErrorTest) { cbs, Asn1Utility::parseOctetString))); } -TEST_F(Asn1UtilityTest, IsOptionalPresentTest) { +TEST_F(Asn1UtilityTest, GetOptionalTest) { CBS cbs, value; CBS_init(&cbs, asn1_true.data(), asn1_true.size()); @@ -159,7 +159,7 @@ TEST_F(Asn1UtilityTest, IsOptionalPresentTest) { EXPECT_EQ(0xff, *CBS_data(&value)); } -TEST_F(Asn1UtilityTest, IsOptionalPresentMissingValueTest) { +TEST_F(Asn1UtilityTest, GetOptionalMissingValueTest) { std::vector missing_val_bool = {0x1u, 1}; CBS cbs, value; CBS_init(&cbs, missing_val_bool.data(), missing_val_bool.size()); diff --git a/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc b/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc index e607653f9b4d..1fd17eea0b1f 100644 --- a/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc +++ b/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc @@ -135,7 +135,7 @@ TEST_F(OcspFullResponseParsingTest, OnlyOneResponseInByteStringTest) { TEST_F(OcspFullResponseParsingTest, ParseOcspResponseWrongTagTest) { auto resp_bytes = readFile("good_ocsp_resp.der"); - // Change the SEQUENCE tag to an OCTETSTRING tag + // Change the SEQUENCE tag to an `OCTETSTRING` tag resp_bytes[0] = 0x4u; EXPECT_THROW_WITH_MESSAGE(OcspResponseWrapper response_wrapper(resp_bytes, time_system_), EnvoyException, "OCSP Response is not a well-formed ASN.1 SEQUENCE"); @@ -232,7 +232,7 @@ TEST_F(Asn1OcspUtilityTest, ParseResponseBytesNoOctetStringTest) { ASSERT_TRUE(CBB_add_asn1(cbb.get(), &seq, CBS_ASN1_SEQUENCE)); ASSERT_TRUE(CBB_add_asn1(&seq, &oid, CBS_ASN1_OBJECT)); ASSERT_TRUE(CBB_add_asn1_oid_from_text(&oid, oid_str.c_str(), oid_str.size())); - // Empty sequence instead of OCTETSTRING with the response + // Empty sequence instead of `OCTETSTRING` with the response ASSERT_TRUE(CBB_add_asn1(&seq, &obj, CBS_ASN1_SEQUENCE)); ASSERT_TRUE(CBB_finish(cbb.get(), &buf, &buf_len)); diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index cf20436ca153..3cac9bb8fcd7 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -15,7 +15,6 @@ API ARN ASAN ASCII -ASN ASSERTs AST AWS @@ -68,7 +67,6 @@ DNS DQUOTE DRYs DS -DSA DST DW EADDRINUSE @@ -121,7 +119,6 @@ GCE GCM GCOVR GCP -GENERALIZEDTIME GETting GLB GOAWAY @@ -186,7 +183,6 @@ MB MD MERCHANTABILITY MGET -MONOSTATE MQ MSET MSVC @@ -214,7 +210,6 @@ NUL Nilsson Nonhashable OCSP -OID OK OOM OOMs @@ -426,7 +421,6 @@ bidi bignum bitfield bitset -bitstring bitwise blackhole blackholed @@ -817,7 +811,6 @@ num numkeys observability ocagent -octetstring offsetof oneof oneway From 6d5c9d59c59357505615327f7c041cc70b14fa8c Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Fri, 7 Aug 2020 07:53:42 -0700 Subject: [PATCH 27/31] change get optional to return an optional CBS Signed-off-by: Daniel Goldstein --- .../tls/ocsp/asn1_utility.cc | 7 ++++--- .../transport_sockets/tls/ocsp/asn1_utility.h | 2 +- .../transport_sockets/tls/ocsp/ocsp.cc | 19 +++++++++---------- .../tls/ocsp/asn1_utility_test.cc | 10 +++++----- 4 files changed, 19 insertions(+), 19 deletions(-) diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc index 8000294b39fd..f07e43621ad7 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc @@ -27,13 +27,14 @@ absl::string_view Asn1Utility::cbsToString(CBS& cbs) { return {str_head, CBS_len(&cbs)}; } -ParsingResult Asn1Utility::getOptional(CBS& cbs, CBS* data, unsigned tag) { +ParsingResult> Asn1Utility::getOptional(CBS& cbs, unsigned tag) { int is_present; - if (!CBS_get_optional_asn1(&cbs, data, &is_present, tag)) { + CBS data; + if (!CBS_get_optional_asn1(&cbs, &data, &is_present, tag)) { return "Failed to parse ASN.1 element tag"; } - return static_cast(is_present); + return is_present ? data : absl::nullopt; } ParsingResult Asn1Utility::parseOid(CBS& cbs) { diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h index 63040b5a0efd..46d3e829f88e 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h @@ -104,7 +104,7 @@ class Asn1Utility { * @returns ParsingResult whether `cbs` points to an element tagged with `tag` or * an error string if parsing fails. */ - static ParsingResult getOptional(CBS& cbs, CBS* data, unsigned tag); + static ParsingResult> getOptional(CBS& cbs, unsigned tag); /** * @param cbs a CBS& that refers to an `ASN.1` OBJECT IDENTIFIER element diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc index 097acaabbf6c..87b9572e1498 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc @@ -52,9 +52,9 @@ void skipResponderId(CBS& cbs) { // KeyHash ::= OCTET STRING -- SHA-1 hash of responder's public key // (excluding the tag and length fields) - if (unwrap(Asn1Utility::getOptional(cbs, nullptr, + if (unwrap(Asn1Utility::getOptional(cbs, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 1)) || - unwrap(Asn1Utility::getOptional(cbs, nullptr, + unwrap(Asn1Utility::getOptional(cbs, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 2))) { return; } @@ -68,10 +68,10 @@ void skipCertStatus(CBS& cbs) { // revoked [1] IMPLICIT RevokedInfo, // unknown [2] IMPLICIT UnknownInfo // } - if (!(unwrap(Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONTEXT_SPECIFIC | 0)) || - unwrap(Asn1Utility::getOptional(cbs, nullptr, + if (!(unwrap(Asn1Utility::getOptional(cbs, CBS_ASN1_CONTEXT_SPECIFIC | 0)) || + unwrap(Asn1Utility::getOptional(cbs, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 1)) || - unwrap(Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_CONTEXT_SPECIFIC | 2)))) { + unwrap(Asn1Utility::getOptional(cbs, CBS_ASN1_CONTEXT_SPECIFIC | 2)))) { throw EnvoyException(absl::StrCat("Unknown OcspCertStatus tag: ", parseTag(cbs))); } } @@ -141,12 +141,11 @@ std::unique_ptr Asn1OcspUtility::parseOcspResponse(CBS& cbs) { } OcspResponseStatus status = Asn1OcspUtility::parseResponseStatus(elem); - - CBS bytes; + auto maybe_bytes = unwrap(Asn1Utility::getOptional(elem, + CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0)); ResponsePtr resp = nullptr; - if (unwrap(Asn1Utility::getOptional(elem, &bytes, - CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0))) { - resp = Asn1OcspUtility::parseResponseBytes(bytes); + if (maybe_bytes) { + resp = Asn1OcspUtility::parseResponseBytes(maybe_bytes.get()); } return std::make_unique(status, std::move(resp)); diff --git a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc index 51785b413a41..b22d5e44d28c 100644 --- a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc +++ b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc @@ -148,24 +148,24 @@ TEST_F(Asn1UtilityTest, SequenceOfMixedTypeErrorTest) { } TEST_F(Asn1UtilityTest, GetOptionalTest) { - CBS cbs, value; + CBS cbs; CBS_init(&cbs, asn1_true.data(), asn1_true.size()); const uint8_t* start = CBS_data(&cbs); - EXPECT_FALSE(absl::get<0>(Asn1Utility::getOptional(cbs, nullptr, CBS_ASN1_INTEGER))); + EXPECT_EQ(absl::nullopt, absl::get<0>(Asn1Utility::getOptional(cbs, CBS_ASN1_INTEGER))); EXPECT_EQ(start, CBS_data(&cbs)); - EXPECT_TRUE(absl::get<0>(Asn1Utility::getOptional(cbs, &value, CBS_ASN1_BOOLEAN))); + EXPECT_TRUE(absl::get<0>(Asn1Utility::getOptional(cbs, CBS_ASN1_BOOLEAN))); EXPECT_EQ(0xff, *CBS_data(&value)); } TEST_F(Asn1UtilityTest, GetOptionalMissingValueTest) { std::vector missing_val_bool = {0x1u, 1}; - CBS cbs, value; + CBS cbs; CBS_init(&cbs, missing_val_bool.data(), missing_val_bool.size()); EXPECT_EQ("Failed to parse ASN.1 element tag", - absl::get<1>(Asn1Utility::getOptional(cbs, &value, CBS_ASN1_BOOLEAN))); + absl::get<1>(Asn1Utility::getOptional(cbs, CBS_ASN1_BOOLEAN))); } TEST_F(Asn1UtilityTest, ParseOptionalTest) { From db03fb35dc6ba633f47201cb9f406b2ecc94c94a Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Mon, 10 Aug 2020 06:41:04 -0700 Subject: [PATCH 28/31] fix optional implementation Signed-off-by: Daniel Goldstein --- .../transport_sockets/tls/ocsp/asn1_utility.cc | 2 +- .../transport_sockets/tls/ocsp/asn1_utility.h | 12 ++++++------ source/extensions/transport_sockets/tls/ocsp/ocsp.cc | 2 +- .../transport_sockets/tls/ocsp/asn1_utility_test.cc | 2 +- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc index f07e43621ad7..82cf430a7fd2 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.cc @@ -34,7 +34,7 @@ ParsingResult> Asn1Utility::getOptional(CBS& cbs, unsigned t return "Failed to parse ASN.1 element tag"; } - return is_present ? data : absl::nullopt; + return is_present ? absl::optional(data) : absl::nullopt; } ParsingResult Asn1Utility::parseOid(CBS& cbs) { diff --git a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h index 46d3e829f88e..05bea0c2d19b 100644 --- a/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h +++ b/source/extensions/transport_sockets/tls/ocsp/asn1_utility.h @@ -192,15 +192,15 @@ ParsingResult> Asn1Utility::parseSequenceOf(CBS& cbs, template ParsingResult> Asn1Utility::parseOptional(CBS& cbs, Asn1ParsingFunc parse_data, unsigned tag) { - CBS data; - auto is_present = getOptional(cbs, &data, tag); + auto maybe_data_res = getOptional(cbs, tag); - if (absl::holds_alternative(is_present)) { - return absl::get(is_present); + if (absl::holds_alternative(maybe_data_res)) { + return absl::get(maybe_data_res); } - if (absl::get(is_present)) { - auto res = parse_data(data); + auto maybe_data = absl::get>(maybe_data_res); + if (maybe_data) { + auto res = parse_data(maybe_data.value()); if (absl::holds_alternative(res)) { return absl::get<0>(res); } diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc index 87b9572e1498..cd6fb00cc0da 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc @@ -145,7 +145,7 @@ std::unique_ptr Asn1OcspUtility::parseOcspResponse(CBS& cbs) { CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0)); ResponsePtr resp = nullptr; if (maybe_bytes) { - resp = Asn1OcspUtility::parseResponseBytes(maybe_bytes.get()); + resp = Asn1OcspUtility::parseResponseBytes(maybe_bytes.value()); } return std::make_unique(status, std::move(resp)); diff --git a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc index b22d5e44d28c..1f5ac34625b2 100644 --- a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc +++ b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc @@ -155,7 +155,7 @@ TEST_F(Asn1UtilityTest, GetOptionalTest) { EXPECT_EQ(absl::nullopt, absl::get<0>(Asn1Utility::getOptional(cbs, CBS_ASN1_INTEGER))); EXPECT_EQ(start, CBS_data(&cbs)); - EXPECT_TRUE(absl::get<0>(Asn1Utility::getOptional(cbs, CBS_ASN1_BOOLEAN))); + CBS value = absl::get<0>(Asn1Utility::getOptional(cbs, CBS_ASN1_BOOLEAN)).value(); EXPECT_EQ(0xff, *CBS_data(&value)); } From 7398b7b8bcb543577af27b74455954572ef08549 Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Mon, 10 Aug 2020 08:12:59 -0700 Subject: [PATCH 29/31] add runtime flag for checking the ocsp validity window start time Signed-off-by: Daniel Goldstein --- source/common/runtime/runtime_features.cc | 1 + source/extensions/transport_sockets/tls/ocsp/BUILD | 1 + source/extensions/transport_sockets/tls/ocsp/ocsp.cc | 4 +++- test/extensions/transport_sockets/tls/ocsp/BUILD | 1 + .../transport_sockets/tls/ocsp/asn1_utility_test.cc | 5 +++-- .../transport_sockets/tls/ocsp/ocsp_test.cc | 11 +++++++++++ 6 files changed, 20 insertions(+), 3 deletions(-) diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index ae0602138651..fffaeaccdbf9 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -81,6 +81,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.stop_faking_paths", "envoy.reloadable_features.hcm_stream_error_on_invalid_message", "envoy.reloadable_features.strict_1xx_and_204_response_headers", + "envoy.reloadable_features.check_ocsp_response_validity_start_time", }; // This is a section for officially sanctioned runtime features which are too diff --git a/source/extensions/transport_sockets/tls/ocsp/BUILD b/source/extensions/transport_sockets/tls/ocsp/BUILD index e2995afd45cd..6b6ad9e328d3 100644 --- a/source/extensions/transport_sockets/tls/ocsp/BUILD +++ b/source/extensions/transport_sockets/tls/ocsp/BUILD @@ -17,6 +17,7 @@ envoy_cc_library( ":asn1_utility_lib", "//include/envoy/common:time_interface", "//include/envoy/ssl:context_config_interface", + "//source/common/runtime:runtime_features_lib", "//source/extensions/transport_sockets/tls:utility_lib", ], ) diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc index cd6fb00cc0da..98a33c7f0703 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc @@ -1,6 +1,7 @@ #include "extensions/transport_sockets/tls/ocsp/ocsp.h" #include "common/common/utility.h" +#include "common/runtime/runtime_features.h" #include "extensions/transport_sockets/tls/ocsp/asn1_utility.h" #include "extensions/transport_sockets/tls/utility.h" @@ -106,7 +107,8 @@ OcspResponseWrapper::OcspResponseWrapper(std::vector der_response, Time } auto& this_update = response_->response_->getThisUpdate(); - if (time_source_.systemTime() < this_update) { + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.check_ocsp_response_validity_start_time") && + time_source_.systemTime() < this_update) { std::string time_format(GENERALIZED_TIME_FORMAT); DateFormatter formatter(time_format); throw EnvoyException(absl::StrCat("OCSP Response thisUpdate field is set in the future: ", diff --git a/test/extensions/transport_sockets/tls/ocsp/BUILD b/test/extensions/transport_sockets/tls/ocsp/BUILD index 6e64630e92d3..e37fb8e6d4d0 100644 --- a/test/extensions/transport_sockets/tls/ocsp/BUILD +++ b/test/extensions/transport_sockets/tls/ocsp/BUILD @@ -23,6 +23,7 @@ envoy_cc_test( "//source/extensions/transport_sockets/tls/ocsp:ocsp_lib", "//test/extensions/transport_sockets/tls:ssl_test_utils", "//test/test_common:environment_lib", + "//test/test_common:test_runtime_lib", "//test/test_common:simulated_time_system_lib", ], ) diff --git a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc index 1f5ac34625b2..e3299c39bd22 100644 --- a/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc +++ b/test/extensions/transport_sockets/tls/ocsp/asn1_utility_test.cc @@ -164,8 +164,9 @@ TEST_F(Asn1UtilityTest, GetOptionalMissingValueTest) { CBS cbs; CBS_init(&cbs, missing_val_bool.data(), missing_val_bool.size()); - EXPECT_EQ("Failed to parse ASN.1 element tag", - absl::get<1>(Asn1Utility::getOptional(cbs, CBS_ASN1_BOOLEAN))); + auto res = Asn1Utility::getOptional(cbs, CBS_ASN1_BOOLEAN); + EXPECT_TRUE(absl::holds_alternative(res)); + EXPECT_EQ("Failed to parse ASN.1 element tag", absl::get<1>(res)); } TEST_F(Asn1UtilityTest, ParseOptionalTest) { diff --git a/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc b/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc index 1fd17eea0b1f..438a7afcaac9 100644 --- a/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc +++ b/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc @@ -6,6 +6,7 @@ #include "test/extensions/transport_sockets/tls/ssl_test_utility.h" #include "test/test_common/environment.h" #include "test/test_common/simulated_time_system.h" +#include "test/test_common/test_runtime.h" #include "test/test_common/utility.h" #include "gmock/gmock.h" @@ -99,6 +100,16 @@ TEST_F(OcspFullResponseParsingTest, ThisUpdateAfterNowTest) { "OCSP Response thisUpdate field is set in the future"); } +TEST_F(OcspFullResponseParsingTest, ThisUpdateAfterNowCheckDisabledTest) { + auto past_time = TestUtility::parseTime("2000 01 01", "%Y %m %d"); + time_system_.setSystemTime(absl::ToChronoTime(past_time)); + + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.check_ocsp_response_validity_start_time", "false"}}); + setup("good_ocsp_resp.der"); +} + TEST_F(OcspFullResponseParsingTest, ResponderIdKeyHashTest) { setup("responder_key_hash_ocsp_resp.der"); expectSuccessful(); From 4cd7d3c113baee1c8ccf75919bc91d734c33c226 Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Mon, 10 Aug 2020 08:29:54 -0700 Subject: [PATCH 30/31] format Signed-off-by: Daniel Goldstein --- .../transport_sockets/tls/ocsp/ocsp.cc | 17 ++++++++--------- .../extensions/transport_sockets/tls/ocsp/BUILD | 2 +- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc index 98a33c7f0703..04cc0a913785 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc @@ -53,10 +53,8 @@ void skipResponderId(CBS& cbs) { // KeyHash ::= OCTET STRING -- SHA-1 hash of responder's public key // (excluding the tag and length fields) - if (unwrap(Asn1Utility::getOptional(cbs, - CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 1)) || - unwrap(Asn1Utility::getOptional(cbs, - CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 2))) { + if (unwrap(Asn1Utility::getOptional(cbs, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 1)) || + unwrap(Asn1Utility::getOptional(cbs, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 2))) { return; } @@ -70,8 +68,8 @@ void skipCertStatus(CBS& cbs) { // unknown [2] IMPLICIT UnknownInfo // } if (!(unwrap(Asn1Utility::getOptional(cbs, CBS_ASN1_CONTEXT_SPECIFIC | 0)) || - unwrap(Asn1Utility::getOptional(cbs, - CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 1)) || + unwrap( + Asn1Utility::getOptional(cbs, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 1)) || unwrap(Asn1Utility::getOptional(cbs, CBS_ASN1_CONTEXT_SPECIFIC | 2)))) { throw EnvoyException(absl::StrCat("Unknown OcspCertStatus tag: ", parseTag(cbs))); } @@ -107,7 +105,8 @@ OcspResponseWrapper::OcspResponseWrapper(std::vector der_response, Time } auto& this_update = response_->response_->getThisUpdate(); - if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.check_ocsp_response_validity_start_time") && + if (Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.check_ocsp_response_validity_start_time") && time_source_.systemTime() < this_update) { std::string time_format(GENERALIZED_TIME_FORMAT); DateFormatter formatter(time_format); @@ -143,8 +142,8 @@ std::unique_ptr Asn1OcspUtility::parseOcspResponse(CBS& cbs) { } OcspResponseStatus status = Asn1OcspUtility::parseResponseStatus(elem); - auto maybe_bytes = unwrap(Asn1Utility::getOptional(elem, - CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0)); + auto maybe_bytes = + unwrap(Asn1Utility::getOptional(elem, CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0)); ResponsePtr resp = nullptr; if (maybe_bytes) { resp = Asn1OcspUtility::parseResponseBytes(maybe_bytes.value()); diff --git a/test/extensions/transport_sockets/tls/ocsp/BUILD b/test/extensions/transport_sockets/tls/ocsp/BUILD index e37fb8e6d4d0..83a0cf54ab81 100644 --- a/test/extensions/transport_sockets/tls/ocsp/BUILD +++ b/test/extensions/transport_sockets/tls/ocsp/BUILD @@ -23,8 +23,8 @@ envoy_cc_test( "//source/extensions/transport_sockets/tls/ocsp:ocsp_lib", "//test/extensions/transport_sockets/tls:ssl_test_utils", "//test/test_common:environment_lib", - "//test/test_common:test_runtime_lib", "//test/test_common:simulated_time_system_lib", + "//test/test_common:test_runtime_lib", ], ) From 1dcb401cd5d57e52c1ea6d7455bb312c70c9e00c Mon Sep 17 00:00:00 2001 From: Daniel Goldstein Date: Mon, 10 Aug 2020 10:11:34 -0700 Subject: [PATCH 31/31] change thisUpdate check to log a warning instead of throw Signed-off-by: Daniel Goldstein --- source/common/runtime/runtime_features.cc | 1 - .../extensions/transport_sockets/tls/ocsp/BUILD | 1 - .../transport_sockets/tls/ocsp/ocsp.cc | 9 +++------ test/extensions/transport_sockets/tls/ocsp/BUILD | 2 +- .../transport_sockets/tls/ocsp/ocsp_test.cc | 16 +++------------- 5 files changed, 7 insertions(+), 22 deletions(-) diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index fffaeaccdbf9..ae0602138651 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -81,7 +81,6 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.stop_faking_paths", "envoy.reloadable_features.hcm_stream_error_on_invalid_message", "envoy.reloadable_features.strict_1xx_and_204_response_headers", - "envoy.reloadable_features.check_ocsp_response_validity_start_time", }; // This is a section for officially sanctioned runtime features which are too diff --git a/source/extensions/transport_sockets/tls/ocsp/BUILD b/source/extensions/transport_sockets/tls/ocsp/BUILD index 6b6ad9e328d3..e2995afd45cd 100644 --- a/source/extensions/transport_sockets/tls/ocsp/BUILD +++ b/source/extensions/transport_sockets/tls/ocsp/BUILD @@ -17,7 +17,6 @@ envoy_cc_library( ":asn1_utility_lib", "//include/envoy/common:time_interface", "//include/envoy/ssl:context_config_interface", - "//source/common/runtime:runtime_features_lib", "//source/extensions/transport_sockets/tls:utility_lib", ], ) diff --git a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc index 04cc0a913785..11f7d0aa2c90 100644 --- a/source/extensions/transport_sockets/tls/ocsp/ocsp.cc +++ b/source/extensions/transport_sockets/tls/ocsp/ocsp.cc @@ -1,7 +1,6 @@ #include "extensions/transport_sockets/tls/ocsp/ocsp.h" #include "common/common/utility.h" -#include "common/runtime/runtime_features.h" #include "extensions/transport_sockets/tls/ocsp/asn1_utility.h" #include "extensions/transport_sockets/tls/utility.h" @@ -105,13 +104,11 @@ OcspResponseWrapper::OcspResponseWrapper(std::vector der_response, Time } auto& this_update = response_->response_->getThisUpdate(); - if (Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.check_ocsp_response_validity_start_time") && - time_source_.systemTime() < this_update) { + if (time_source_.systemTime() < this_update) { std::string time_format(GENERALIZED_TIME_FORMAT); DateFormatter formatter(time_format); - throw EnvoyException(absl::StrCat("OCSP Response thisUpdate field is set in the future: ", - formatter.fromTime(this_update))); + ENVOY_LOG_MISC(warn, "OCSP Response thisUpdate field is set in the future: {}", + formatter.fromTime(this_update)); } } diff --git a/test/extensions/transport_sockets/tls/ocsp/BUILD b/test/extensions/transport_sockets/tls/ocsp/BUILD index 83a0cf54ab81..9f42f8d2ad05 100644 --- a/test/extensions/transport_sockets/tls/ocsp/BUILD +++ b/test/extensions/transport_sockets/tls/ocsp/BUILD @@ -23,8 +23,8 @@ envoy_cc_test( "//source/extensions/transport_sockets/tls/ocsp:ocsp_lib", "//test/extensions/transport_sockets/tls:ssl_test_utils", "//test/test_common:environment_lib", + "//test/test_common:logging_lib", "//test/test_common:simulated_time_system_lib", - "//test/test_common:test_runtime_lib", ], ) diff --git a/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc b/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc index 438a7afcaac9..aebca52c04ef 100644 --- a/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc +++ b/test/extensions/transport_sockets/tls/ocsp/ocsp_test.cc @@ -5,8 +5,8 @@ #include "test/extensions/transport_sockets/tls/ssl_test_utility.h" #include "test/test_common/environment.h" +#include "test/test_common/logging.h" #include "test/test_common/simulated_time_system.h" -#include "test/test_common/test_runtime.h" #include "test/test_common/utility.h" #include "gmock/gmock.h" @@ -96,18 +96,8 @@ TEST_F(OcspFullResponseParsingTest, ExpiredResponseTest) { TEST_F(OcspFullResponseParsingTest, ThisUpdateAfterNowTest) { auto past_time = TestUtility::parseTime("2000 01 01", "%Y %m %d"); time_system_.setSystemTime(absl::ToChronoTime(past_time)); - EXPECT_THROW_WITH_REGEX(setup("good_ocsp_resp.der"), EnvoyException, - "OCSP Response thisUpdate field is set in the future"); -} - -TEST_F(OcspFullResponseParsingTest, ThisUpdateAfterNowCheckDisabledTest) { - auto past_time = TestUtility::parseTime("2000 01 01", "%Y %m %d"); - time_system_.setSystemTime(absl::ToChronoTime(past_time)); - - TestScopedRuntime scoped_runtime; - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.check_ocsp_response_validity_start_time", "false"}}); - setup("good_ocsp_resp.der"); + EXPECT_LOG_CONTAINS("warning", "OCSP Response thisUpdate field is set in the future", + setup("good_ocsp_resp.der")); } TEST_F(OcspFullResponseParsingTest, ResponderIdKeyHashTest) {