da_revocation: Follow ups in TestDACRevocationDelegateImpl and revocation set generation script (#36680)

* serialize the crl signer and crl signer delegator cert as DER

* use enum instead of bool for keyid and rdns

* use std::string in internal APIs

* update impl and tests to use pure base64 DER encoded data

* restyler changes

* Apply suggestions from code review

Co-authored-by: Andrei Litvin <[email protected]>

---------

Co-authored-by: Andrei Litvin <[email protected]>

Author: shubhamdp

credentials/generate-revocation-set.py

@@ -32,6 +32,7 @@
 import requests
 from click_option_group import RequiredMutuallyExclusiveOptionGroup, optgroup
 from cryptography import x509
+from cryptography.hazmat.primitives import serialization
 from cryptography.hazmat.primitives.asymmetric import ec
 from cryptography.x509.oid import NameOID
 
@@ -510,12 +511,12 @@ def main(use_main_net_dcld: str, use_test_net_dcld: str, use_main_net_http: bool
             "issuer_subject_key_id": certificate_akid_hex,
             "issuer_name": certificate_authority_name_b64,
             "revoked_serial_numbers": serialnumber_list,
-            "crl_signer_cert": revocation_point["crlSignerCertificate"],
+            "crl_signer_cert": base64.b64encode(crl_signer_certificate.public_bytes(serialization.Encoding.DER)).decode('utf-8'),
         }
 
-        if "crlSignerDelegator" in revocation_point:
-            entry["crl_signer_delegator"] = revocation_point["crlSignerDelegator"]
-
+        if crl_signer_delegator_cert:
+            entry["crl_signer_delegator"] = base64.b64encode(
+                crl_signer_delegator_cert.public_bytes(serialization.Encoding.DER)).decode('utf-8'),
         logging.debug(f"Entry to append: {entry}")
         revocation_set.append(entry)
 

src/credentials/attestation_verifier/TestDACRevocationDelegateImpl.cpp

@@ -32,49 +32,13 @@ namespace Credentials {
 
 namespace {
 
-static constexpr uint32_t kMaxIssuerBase64Len = BASE64_ENCODED_LEN(kMaxCertificateDistinguishedNameLength);
-
-CHIP_ERROR BytesToHexStr(const ByteSpan & bytes, MutableCharSpan & outHexStr)
+CHIP_ERROR BytesToHexStr(const ByteSpan & bytes, std::string & outHexStr)
 {
-    Encoding::HexFlags flags = Encoding::HexFlags::kUppercase;
-    ReturnErrorOnFailure(BytesToHex(bytes.data(), bytes.size(), outHexStr.data(), outHexStr.size(), flags));
-    outHexStr.reduce_size(2 * bytes.size());
-    return CHIP_NO_ERROR;
-}
-
-CHIP_ERROR X509_PemToDer(const std::string & pemCert, MutableByteSpan & derCert)
-{
-    std::string beginMarker = "-----BEGIN CERTIFICATE-----";
-    std::string endMarker   = "-----END CERTIFICATE-----";
-
-    std::size_t beginPos = pemCert.find(beginMarker);
-    VerifyOrReturnError(beginPos != std::string::npos, CHIP_ERROR_INVALID_ARGUMENT);
+    size_t hexLength = bytes.size() * 2;
+    outHexStr.resize(hexLength);
 
-    std::size_t endPos = pemCert.find(endMarker);
-    VerifyOrReturnError(endPos != std::string::npos, CHIP_ERROR_INVALID_ARGUMENT);
-
-    VerifyOrReturnError(beginPos < endPos, CHIP_ERROR_INVALID_ARGUMENT);
-
-    // Extract content between markers
-    std::string plainB64Str = pemCert.substr(beginPos + beginMarker.length(), endPos - (beginPos + beginMarker.length()));
-
-    // Remove all newline characters '\n' and '\r'
-    plainB64Str.erase(std::remove(plainB64Str.begin(), plainB64Str.end(), '\n'), plainB64Str.end());
-    plainB64Str.erase(std::remove(plainB64Str.begin(), plainB64Str.end(), '\r'), plainB64Str.end());
-
-    VerifyOrReturnError(!plainB64Str.empty(), CHIP_ERROR_INVALID_ARGUMENT);
-
-    // Verify we have enough room to store the decoded certificate
-    size_t maxDecodeLen = BASE64_MAX_DECODED_LEN(plainB64Str.size());
-    VerifyOrReturnError(derCert.size() >= maxDecodeLen, CHIP_ERROR_BUFFER_TOO_SMALL);
-
-    // decode b64
-    uint16_t derLen = Base64Decode(plainB64Str.c_str(), static_cast<uint16_t>(plainB64Str.size()), derCert.data());
-    VerifyOrReturnError(derLen != UINT16_MAX, CHIP_ERROR_INVALID_ARGUMENT);
-
-    derCert.reduce_size(derLen);
-
-    return CHIP_NO_ERROR;
+    Encoding::HexFlags flags = Encoding::HexFlags::kUppercase;
+    return BytesToHex(bytes.data(), bytes.size(), &outHexStr[0], hexLength, flags);
 }
 
 } // anonymous namespace
@@ -92,51 +56,40 @@ void TestDACRevocationDelegateImpl::ClearDeviceAttestationRevocationSetPath()
     mDeviceAttestationRevocationSetPath = mDeviceAttestationRevocationSetPath.substr(0, 0);
 }
 
-// outSubject is subject encoded as base64 string
-// outKeyId is SKID encoded as hex string
-CHIP_ERROR TestDACRevocationDelegateImpl::GetSubjectAndKeyIdFromPEMCert(const std::string & certPEM, std::string & outSubject,
-                                                                        std::string & outKeyId)
-{
-    // buffers and spans for storing crl signer delegator OR crl signer cert info
-    char subjectBuf[kMaxIssuerBase64Len]             = { 0 };
-    char skidBuf[2 * kAuthorityKeyIdentifierLength]  = { 0 };
-    uint8_t certDerBuf[kMax_x509_Certificate_Length] = { 0 };
-
-    MutableCharSpan subject(subjectBuf);
-    MutableCharSpan keyId(skidBuf);
-    MutableByteSpan certDER(certDerBuf);
-
-    ReturnLogErrorOnFailure(X509_PemToDer(certPEM, certDER));
-    ReturnErrorOnFailure(GetSubjectNameBase64Str(certDER, subject));
-    ReturnErrorOnFailure(GetSKIDHexStr(certDER, keyId));
-
-    outSubject = std::string(subject.data(), subject.size());
-    outKeyId   = std::string(keyId.data(), keyId.size());
-
-    return CHIP_NO_ERROR;
-}
-
 // Check if issuer and AKID matches with the crl signer OR crl signer delegator's subject and SKID
 bool TestDACRevocationDelegateImpl::CrossValidateCert(const Json::Value & revokedSet, const std::string & akidHexStr,
                                                       const std::string & issuerNameBase64Str)
 {
-    std::string certPEM;
+    std::string certBase64;
     [[maybe_unused]] std::string certType;
 
     if (revokedSet.isMember("crl_signer_delegator"))
     {
-        certPEM  = revokedSet["crl_signer_delegator"].asString();
-        certType = "CRL Signer delegator";
+        certBase64 = revokedSet["crl_signer_delegator"].asString();
+        certType   = "CRL Signer delegator";
     }
     else
     {
-        certPEM  = revokedSet["crl_signer_cert"].asString();
-        certType = "CRL Signer";
+        certBase64 = revokedSet["crl_signer_cert"].asString();
+        certType   = "CRL Signer";
     }
 
-    std::string subject; // crl signer or crl signer delegator subject
-    std::string keyId;   // crl signer or crl signer delegator SKID
-    VerifyOrReturnValue(CHIP_NO_ERROR == GetSubjectAndKeyIdFromPEMCert(certPEM, subject, keyId), false);
+    uint8_t certDerBuf[kMax_x509_Certificate_Length] = { 0 };
+    MutableByteSpan certDER(certDerBuf);
+
+    // Verify we have enough room to store the decoded certificate
+    size_t maxDecodeLen = BASE64_MAX_DECODED_LEN(certBase64.size());
+    VerifyOrReturnValue(certDER.size() >= maxDecodeLen, false);
+
+    uint16_t derLen = Base64Decode(certBase64.c_str(), static_cast<uint16_t>(certBase64.size()), certDER.data());
+    VerifyOrReturnValue(derLen != UINT16_MAX, false);
+    certDER.reduce_size(derLen);
+
+    std::string subject;
+    std::string keyId;
+
+    VerifyOrReturnValue(CHIP_NO_ERROR == GetSubjectNameBase64Str(certDER, subject), false);
+    VerifyOrReturnValue(CHIP_NO_ERROR == GetSKIDHexStr(certDER, keyId), false);
 
     ChipLogDetail(NotSpecified, "%s: Subject: %s", certType.c_str(), subject.c_str());
     ChipLogDetail(NotSpecified, "%s: SKID: %s", certType.c_str(), keyId.c_str());
@@ -154,13 +107,13 @@ bool TestDACRevocationDelegateImpl::CrossValidateCert(const Json::Value & revoke
 //       "serial1 bytes as base64",
 //       "serial2 bytes as base64"
 //     ]
-//     "crl_signer_cert": "<PEM incoded CRL signer certificate>",
-//     "crl_signer_delegator": <PEM incoded CRL signer delegator certificate>,
+//     "crl_signer_cert": "<base64 encoded DER certificate>",
+//     "crl_signer_delegator": "<base64 encoded DER certificate>",
 //   }
 // ]
 //
-bool TestDACRevocationDelegateImpl::IsEntryInRevocationSet(const CharSpan & akidHexStr, const CharSpan & issuerNameBase64Str,
-                                                           const CharSpan & serialNumberHexStr)
+bool TestDACRevocationDelegateImpl::IsEntryInRevocationSet(const std::string & akidHexStr, const std::string & issuerNameBase64Str,
+                                                           const std::string & serialNumberHexStr)
 {
     std::ifstream file(mDeviceAttestationRevocationSetPath.c_str());
     if (!file.is_open())
@@ -185,30 +138,26 @@ bool TestDACRevocationDelegateImpl::IsEntryInRevocationSet(const CharSpan & akid
         return false;
     }
 
-    std::string issuerName   = std::string(issuerNameBase64Str.data(), issuerNameBase64Str.size());
-    std::string serialNumber = std::string(serialNumberHexStr.data(), serialNumberHexStr.size());
-    std::string akid         = std::string(akidHexStr.data(), akidHexStr.size());
-
     // 6.2.4.2. Determining Revocation Status of an Entity
     for (const auto & revokedSet : jsonData)
     {
-        if (revokedSet["issuer_name"].asString() != issuerName)
+        if (revokedSet["issuer_name"].asString() != issuerNameBase64Str)
         {
             continue;
         }
-        if (revokedSet["issuer_subject_key_id"].asString() != akid)
+        if (revokedSet["issuer_subject_key_id"].asString() != akidHexStr)
         {
             continue;
         }
 
         // 4.a cross validate PAI with crl signer OR crl signer delegator
         // 4.b cross validate DAC with crl signer OR crl signer delegator
-        VerifyOrReturnValue(CrossValidateCert(revokedSet, akid, issuerName), false);
+        VerifyOrReturnValue(CrossValidateCert(revokedSet, akidHexStr, issuerNameBase64Str), false);
 
         // 4.c check if serial number is revoked
         for (const auto & revokedSerialNumber : revokedSet["revoked_serial_numbers"])
         {
-            if (revokedSerialNumber.asString() == serialNumber)
+            if (revokedSerialNumber.asString() == serialNumberHexStr)
             {
                 return true;
             }
@@ -217,36 +166,42 @@ bool TestDACRevocationDelegateImpl::IsEntryInRevocationSet(const CharSpan & akid
     return false;
 }
 
-CHIP_ERROR TestDACRevocationDelegateImpl::GetKeyIDHexStr(const ByteSpan & certDer, MutableCharSpan & outKeyIDHexStr, bool isAKID)
+CHIP_ERROR TestDACRevocationDelegateImpl::GetKeyIDHexStr(const ByteSpan & certDer, std::string & outKeyIDHexStr,
+                                                         KeyIdType keyIdType)
 {
     static_assert(kAuthorityKeyIdentifierLength == kSubjectKeyIdentifierLength, "AKID and SKID length mismatch");
 
     uint8_t keyIdBuf[kAuthorityKeyIdentifierLength];
     MutableByteSpan keyId(keyIdBuf);
 
-    if (isAKID)
+    switch (keyIdType)
     {
+    case KeyIdType::kAKID:
         ReturnErrorOnFailure(ExtractAKIDFromX509Cert(certDer, keyId));
-    }
-    else
-    {
+        break;
+
+    case KeyIdType::kSKID:
         ReturnErrorOnFailure(ExtractSKIDFromX509Cert(certDer, keyId));
+        break;
+
+    default:
+        return CHIP_ERROR_INVALID_ARGUMENT;
     }
 
     return BytesToHexStr(keyId, outKeyIDHexStr);
 }
 
-CHIP_ERROR TestDACRevocationDelegateImpl::GetAKIDHexStr(const ByteSpan & certDer, MutableCharSpan & outAKIDHexStr)
+CHIP_ERROR TestDACRevocationDelegateImpl::GetAKIDHexStr(const ByteSpan & certDer, std::string & outAKIDHexStr)
 {
-    return GetKeyIDHexStr(certDer, outAKIDHexStr, true);
+    return GetKeyIDHexStr(certDer, outAKIDHexStr, KeyIdType::kAKID);
 }
 
-CHIP_ERROR TestDACRevocationDelegateImpl::GetSKIDHexStr(const ByteSpan & certDer, MutableCharSpan & outSKIDHexStr)
+CHIP_ERROR TestDACRevocationDelegateImpl::GetSKIDHexStr(const ByteSpan & certDer, std::string & outSKIDHexStr)
 {
-    return GetKeyIDHexStr(certDer, outSKIDHexStr, false /* isAKID */);
+    return GetKeyIDHexStr(certDer, outSKIDHexStr, KeyIdType::kSKID);
 }
 
-CHIP_ERROR TestDACRevocationDelegateImpl::GetSerialNumberHexStr(const ByteSpan & certDer, MutableCharSpan & outSerialNumberHexStr)
+CHIP_ERROR TestDACRevocationDelegateImpl::GetSerialNumberHexStr(const ByteSpan & certDer, std::string & outSerialNumberHexStr)
 {
     uint8_t serialNumberBuf[kMaxCertificateSerialNumberLength] = { 0 };
     MutableByteSpan serialNumber(serialNumberBuf);
@@ -255,50 +210,55 @@ CHIP_ERROR TestDACRevocationDelegateImpl::GetSerialNumberHexStr(const ByteSpan &
     return BytesToHexStr(serialNumber, outSerialNumberHexStr);
 }
 
-CHIP_ERROR TestDACRevocationDelegateImpl::GetRDNBase64Str(const ByteSpan & certDer, MutableCharSpan & outRDNBase64String,
-                                                          bool isIssuer)
+CHIP_ERROR TestDACRevocationDelegateImpl::GetRDNBase64Str(const ByteSpan & certDer, std::string & outRDNBase64String,
+                                                          RDNType rdnType)
 {
     uint8_t rdnBuf[kMaxCertificateDistinguishedNameLength] = { 0 };
     MutableByteSpan rdn(rdnBuf);
 
-    if (isIssuer)
+    switch (rdnType)
     {
+    case RDNType::kIssuer:
         ReturnErrorOnFailure(ExtractIssuerFromX509Cert(certDer, rdn));
-    }
-    else
-    {
+        break;
+
+    case RDNType::kSubject:
         ReturnErrorOnFailure(ExtractSubjectFromX509Cert(certDer, rdn));
+        break;
+
+    default:
+        return CHIP_ERROR_INVALID_ARGUMENT;
     }
 
-    VerifyOrReturnError(outRDNBase64String.size() >= BASE64_ENCODED_LEN(rdn.size()), CHIP_ERROR_BUFFER_TOO_SMALL);
+    // calculate the b64 length needed
+    size_t b64LenNeeded = BASE64_ENCODED_LEN(rdn.size());
+
+    // Ensure string has enough capacity for base64 encoded data
+    outRDNBase64String.resize(b64LenNeeded);
+
+    uint16_t encodedLen = Base64Encode(rdn.data(), static_cast<uint16_t>(rdn.size()), &outRDNBase64String[0]);
+    outRDNBase64String.resize(encodedLen);
 
-    uint16_t encodedLen = Base64Encode(rdn.data(), static_cast<uint16_t>(rdn.size()), outRDNBase64String.data());
-    outRDNBase64String.reduce_size(encodedLen);
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR TestDACRevocationDelegateImpl::GetIssuerNameBase64Str(const ByteSpan & certDer,
-                                                                 MutableCharSpan & outIssuerNameBase64String)
+CHIP_ERROR TestDACRevocationDelegateImpl::GetIssuerNameBase64Str(const ByteSpan & certDer, std::string & outIssuerNameBase64String)
 {
-    return GetRDNBase64Str(certDer, outIssuerNameBase64String, true /* isIssuer */);
+    return GetRDNBase64Str(certDer, outIssuerNameBase64String, RDNType::kIssuer);
 }
 
 CHIP_ERROR TestDACRevocationDelegateImpl::GetSubjectNameBase64Str(const ByteSpan & certDer,
-                                                                  MutableCharSpan & outSubjectNameBase64String)
+                                                                  std::string & outSubjectNameBase64String)
 {
-    return GetRDNBase64Str(certDer, outSubjectNameBase64String, false /* isIssuer */);
+    return GetRDNBase64Str(certDer, outSubjectNameBase64String, RDNType::kSubject);
 }
 
 // @param certDer Certificate, in DER format, to check for revocation
 bool TestDACRevocationDelegateImpl::IsCertificateRevoked(const ByteSpan & certDer)
 {
-    char issuerNameBuffer[kMaxIssuerBase64Len]                           = { 0 };
-    char serialNumberHexStrBuffer[2 * kMaxCertificateSerialNumberLength] = { 0 };
-    char akidHexStrBuffer[2 * kAuthorityKeyIdentifierLength]             = { 0 };
-
-    MutableCharSpan issuerName(issuerNameBuffer);
-    MutableCharSpan serialNumber(serialNumberHexStrBuffer);
-    MutableCharSpan akid(akidHexStrBuffer);
+    std::string serialNumber;
+    std::string akid;
+    std::string issuerName;
 
     VerifyOrReturnValue(CHIP_NO_ERROR == GetIssuerNameBase64Str(certDer, issuerName), false);
     ChipLogDetail(NotSpecified, "Issuer: %.*s", static_cast<int>(issuerName.size()), issuerName.data());

src/credentials/attestation_verifier/TestDACRevocationDelegateImpl.h

@@ -53,22 +53,32 @@ class TestDACRevocationDelegateImpl : public DeviceAttestationRevocationDelegate
     void ClearDeviceAttestationRevocationSetPath();
 
 private:
-    bool CrossValidateCert(const Json::Value & revokedSet, const std::string & akIdHexStr, const std::string & issuerNameBase64Str);
+    enum class KeyIdType : uint8_t
+    {
+        kSKID = 0,
+        kAKID = 1,
+    };
+
+    enum class RDNType : uint8_t
+    {
+        kIssuer  = 0,
+        kSubject = 1,
+    };
 
-    CHIP_ERROR GetKeyIDHexStr(const ByteSpan & certDer, MutableCharSpan & outKeyIDHexStr, bool isAKID);
-    CHIP_ERROR GetAKIDHexStr(const ByteSpan & certDer, MutableCharSpan & outAKIDHexStr);
-    CHIP_ERROR GetSKIDHexStr(const ByteSpan & certDer, MutableCharSpan & outSKIDHexStr);
+    bool CrossValidateCert(const Json::Value & revokedSet, const std::string & akIdHexStr, const std::string & issuerNameBase64Str);
 
-    CHIP_ERROR GetSerialNumberHexStr(const ByteSpan & certDer, MutableCharSpan & outSerialNumberHexStr);
+    CHIP_ERROR GetKeyIDHexStr(const ByteSpan & certDer, std::string & outKeyIDHexStr, KeyIdType keyIdType);
+    CHIP_ERROR GetAKIDHexStr(const ByteSpan & certDer, std::string & outAKIDHexStr);
+    CHIP_ERROR GetSKIDHexStr(const ByteSpan & certDer, std::string & outSKIDHexStr);
 
-    CHIP_ERROR GetRDNBase64Str(const ByteSpan & certDer, MutableCharSpan & outRDNBase64String, bool isIssuer);
-    CHIP_ERROR GetIssuerNameBase64Str(const ByteSpan & certDer, MutableCharSpan & outIssuerNameBase64String);
-    CHIP_ERROR GetSubjectNameBase64Str(const ByteSpan & certDer, MutableCharSpan & outSubjectNameBase64String);
+    CHIP_ERROR GetSerialNumberHexStr(const ByteSpan & certDer, std::string & outSerialNumberHexStr);
 
-    CHIP_ERROR GetSubjectAndKeyIdFromPEMCert(const std::string & certPEM, std::string & outSubject, std::string & outKeyId);
+    CHIP_ERROR GetRDNBase64Str(const ByteSpan & certDer, std::string & outRDNBase64String, RDNType rdnType);
+    CHIP_ERROR GetIssuerNameBase64Str(const ByteSpan & certDer, std::string & outIssuerNameBase64String);
+    CHIP_ERROR GetSubjectNameBase64Str(const ByteSpan & certDer, std::string & outSubjectNameBase64String);
 
-    bool IsEntryInRevocationSet(const CharSpan & akidHexStr, const CharSpan & issuerNameBase64Str,
-                                const CharSpan & serialNumberHexStr);
+    bool IsEntryInRevocationSet(const std::string & akidHexStr, const std::string & issuerNameBase64Str,
+                                const std::string & serialNumberHexStr);
 
     bool IsCertificateRevoked(const ByteSpan & certDer);