From 2690b6401aac022895a133fa7ae50cd114c98864 Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Mon, 4 May 2026 05:17:16 +0000 Subject: [PATCH] refactor(scep+ejbca): drop dead conditionals on always-empty vars (CodeQL #18, #19) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two CodeQL go/comparison-of-identical-expressions alerts in one sweep — both Warning severity, both real dead-code (not false positives). CodeQL detected that each comparison's LHS variable was provably constant. Alert #18 — internal/api/handler/scep.go:612 (extractCSRFields): challengePassword := "" transactionID := "" // ... loop populates challengePassword from CSR.Attributes ... for _, attr := range csr.Attributes { if attr.Type.Equal(oidChallengePassword) { // populates challengePassword ONLY — transactionID stays "" } } if transactionID == "" && csr.Subject.CommonName != "" { // ← always true transactionID = csr.Subject.CommonName } transactionID was initialized to "" and never reassigned before the check. The conditional was always true; the MVP path was effectively "unconditionally fall back to CN". The RFC 8894 path (tryParseRFC8894 above this function) extracts transaction-ID properly from PKCS#7 authenticatedAttributes; the MVP path is for lightweight legacy clients that send the raw CSR with no PKCS#7 wrapping, and CN-as-transaction-ID is sufficient there. Fix: drop the dead transactionID local var + dead conditional; unconditionally set transactionID = csr.Subject.CommonName. No behavioral change — the runtime semantics are identical to before (every valid invocation already took the fallback). The CN extraction stays robust because the empty-CN case still produces an empty transactionID, which downstream callers handle. Alert #19 — internal/connector/issuer/ejbca/ejbca.go:415 (RevokeCertificate): serial := request.Serial issuerDN := "" // (comment: "if we have time..." — TODO never followed up) revokeURL := fmt.Sprintf("%s/certificate/%s/%s/revoke", apiURL, issuerDN, serial) if issuerDN == "" { // ← always true revokeURL = fmt.Sprintf("%s/certificate/%s/revoke", apiURL, serial) } issuerDN was hardcoded to "" two lines above. The first revokeURL line was unreachable dead code; the conditional always fired and the serial-only URL always won. EJBCA's REST API has both /certificate/{issuer_dn}/{serial}/revoke and /certificate/{serial}/revoke endpoints; the serial-only form is correct for typical certctl deployments where one EJBCA CA maps to one certctl issuer config (no overlapping serial spaces). Fix: drop the dead first revokeURL + dead conditional; build revokeURL once via the serial-only endpoint. No behavioral change — the runtime URL was always the serial-only one. Comment retained + expanded to document the future-enhancement path (parse issuer DN from IssuanceResult metadata + use the DN-qualified endpoint when a multi-CA EJBCA deployment surfaces). Verified locally: gofmt: clean. go vet ./internal/api/handler/... + ./internal/connector/issuer/ejbca/...: exit 0. go test -short -count=1 ./internal/api/handler/... + ejbca/...: PASS. Both fixes are pure dead-code removal — runtime behavior is byte- identical to pre-edit. The existing test suites would have caught any actual behavioral change. References: https://github.com/certctl-io/certctl/security/code-scanning/18 https://github.com/certctl-io/certctl/security/code-scanning/19 Closes both alerts. --- internal/api/handler/scep.go | 19 +++++++++++---- internal/connector/issuer/ejbca/ejbca.go | 31 +++++++++++++++--------- 2 files changed, 34 insertions(+), 16 deletions(-) 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 {