gsadmin/certctl
1
0
Fork 0
mirror of https://github.com/shankar0123/certctl.git synced 2026-06-07 13:51:36 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor(scep+ejbca): drop dead conditionals on always-empty vars (CodeQL #18, #19)

Browse Source 
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.
This commit is contained in:
shankar0123
2026-05-04 05:17:16 +00:00
parent a00b20cc97
commit 9ef9f3cde3
2 changed files with 34 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files
internal/api/handler/scep.go
+14 -5
View File
@@ -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
}
internal/connector/issuer/ejbca/ejbca.go
+20 -11
View File
@@ -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 {