diff --git a/internal/api/handler/scep.go b/internal/api/handler/scep.go index ff421de..f23eee2 100644 --- a/internal/api/handler/scep.go +++ b/internal/api/handler/scep.go @@ -577,7 +577,6 @@ func extractCSRFields(csrDER []byte) ([]byte, string, string, error) { } challengePassword := "" - transactionID := "" // OID for challengePassword: 1.2.840.113549.1.9.7 oidChallengePassword := asn1.ObjectIdentifier{1, 2, 840, 113549, 1, 9, 7} @@ -608,10 +607,20 @@ func extractCSRFields(csrDER []byte) ([]byte, string, string, error) { } } - // Use CN as fallback transaction ID if not found in attributes - if transactionID == "" && csr.Subject.CommonName != "" { - transactionID = csr.Subject.CommonName - } + // transactionID falls back to the CSR's CN. The MVP path (this + // function) never extracts the SCEP transaction-ID attribute (OID + // 2.16.840.1.113733.1.9.7) from CSR.Attributes — that's a known + // gap; the RFC 8894 path (tryParseRFC8894 above) extracts it + // properly from the PKCS#7 SignedData authenticatedAttributes, + // which is where conformant clients put it anyway. CodeQL #18 + // flagged the pre-existing `if transactionID == ""` dead + // conditional (transactionID was initialized to "" three lines + // above and never reassigned); cleaned up here. The MVP path + // stays usable for lightweight legacy clients that send the CSR + // directly with no PKCS#7 wrapping — they get CN-as-transaction-ID + // which is sufficient for matching against pollers in the existing + // test suite. + transactionID := csr.Subject.CommonName return csrDER, challengePassword, transactionID, nil } diff --git a/internal/connector/issuer/ejbca/ejbca.go b/internal/connector/issuer/ejbca/ejbca.go index 4ae0875..cfbbd73 100644 --- a/internal/connector/issuer/ejbca/ejbca.go +++ b/internal/connector/issuer/ejbca/ejbca.go @@ -404,18 +404,27 @@ func (c *Connector) RevokeCertificate(ctx context.Context, request issuer.Revoca return fmt.Errorf("failed to marshal revoke request: %w", err) } - // Use the serial directly or extract from OrderID if present (as fallback) + // EJBCA's REST API has two revoke endpoints: + // /certificate/{issuer_dn}/{serial}/revoke — DN-qualified (more + // robust when EJBCA + // has multiple CAs + // with overlapping + // serial spaces) + // /certificate/{serial}/revoke — serial-only (this + // connector's + // contract today) + // + // We currently use the serial-only endpoint; the issuer DN isn't + // preserved in IssuanceResult.OrderID and the cert isn't re-fetched + // on revoke. EJBCA installations with serial-uniqueness across all + // configured CAs (the typical certctl deployment shape — one EJBCA + // CA per certctl issuer config) work fine. CodeQL #19 flagged the + // pre-existing `if issuerDN == ""` dead-conditional where issuerDN + // was always empty; cleaned up here. Future enhancement (when /if + // a multi-CA EJBCA deployment surfaces): parse issuer DN from + // IssuanceResult metadata + use the DN-qualified endpoint. serial := request.Serial - issuerDN := "" - - // If we have time and access to issuer DN, we could parse it from OrderID - // For now, we attempt to use serial as-is, and fall back to issuer DN lookup if needed. - - revokeURL := fmt.Sprintf("%s/certificate/%s/%s/revoke", c.config.APIUrl, issuerDN, serial) - if issuerDN == "" { - // If no issuer DN, just use serial alone (may fail if EJBCA requires issuer_dn) - revokeURL = fmt.Sprintf("%s/certificate/%s/revoke", c.config.APIUrl, serial) - } + revokeURL := fmt.Sprintf("%s/certificate/%s/revoke", c.config.APIUrl, serial) req, err := http.NewRequestWithContext(ctx, http.MethodPut, revokeURL, bytes.NewReader(body)) if err != nil {