Skip to content

Don't stop reading CSR extensions at the SAN extension - #57

Open
datrixlab wants to merge 1 commit into
mholt:masterfrom
datrixlab:fix/tnauthlist-lost-after-san
Open

datrixlab wants to merge 1 commit into
mholt:masterfrom
datrixlab:fix/tnauthlist-lost-after-san

Conversation

@datrixlab

Copy link
Copy Markdown

The bug

createIdentifiersUsingCSR walks csr.Extensions to pick up the TNAuthList, permanent-identifier and hardware-module identifiers (csr.go:295, "Extract TNAuthList, permanent identifiers and hardware module values"). The branch that reads the SAN extension ends in break, which leaves the loop, not the branch. The TNAuthList check sits earlier in the same iteration, so it only ever sees extensions that come before the SAN one.

x509.CreateCertificateRequest writes the SAN extension (2.5.29.17) before anything in ExtraExtensions, and ExtraExtensions is where a TNAuthList (1.3.6.1.5.5.7.1.26) has to go. So in any CSR that carries both, the TNAuthList identifier is dropped.

Measured on master (e289d7f), building the CSR with x509.CreateCertificateRequest and parsing it back:

CSR extensions, in order identifiers
TNAuthList only 1.3.6.1.5.5.7.1.26 [{TNAuthList MASgAgwA}]
TNAuthList + DNSNames: ["example.com"] 2.5.29.17, 1.3.6.1.5.5.7.1.26 [{dns example.com}] — TNAuthList lost

Both callers see it:

  • OrderParametersFromCSR (csr.go:142, reached from ObtainCertificateForSANs) builds an order that is short one identifier, so the CA has nothing to authorize it against.

  • validateOrderIdentifiers (client.go:235, called unconditionally from the finalize path) rejects an otherwise correct 2-identifier order with:

    number of identifiers in Order [...] (2) does not match the number of
    identifiers extracted from CSR [{dns example.com}] (1)
    

The fix

Drop the break. Nothing after it depends on leaving the loop early; a CSR has at most one SAN extension, so the remaining iterations only look at the other extensions this block is meant to read. With it gone, the same CSR yields [{dns example.com} {TNAuthList MASgAgwA}] and validateOrderIdentifiers returns nil.

Tests

New csr_test.go (csr.go had no test file). Test_createIdentifiersUsingCSR_tnAuthList builds real CSRs through x509.CreateCertificateRequest so the extension order is the one a caller actually produces, and covers TNAuthList alone and TNAuthList next to a SAN.

  • go test ./... passes on this branch (both packages).
  • Reverting only csr.go leaves the "TNAuthList alongside a SAN" subtest failing with [{dns example.com}], and the "TNAuthList alone" subtest passing — so the test pins the bug, not the feature.
  • go vet ./... clean. gofmt reports no diff for either file beyond the repo's existing CRLF checkout (gofmt -l flags untouched files such as client.go the same way on my machine).

No existing test referenced TNAuthList, which is how this survived #35 and #37.

createIdentifiersUsingCSR walks csr.Extensions to collect the TNAuthList,
permanent-identifier and hardware-module identifiers, but the branch that
reads the SAN extension ends in `break`, which leaves the loop rather than
the branch. The TNAuthList check runs earlier in the same iteration, so it
only sees extensions that precede the SAN one.

x509.CreateCertificateRequest writes the SAN extension before anything in
ExtraExtensions, which is where a TNAuthList has to go. So in a CSR that
carries both, the TNAuthList identifier is silently dropped:

  CSR with TNAuthList only                 -> [{TNAuthList ...}]
  CSR with TNAuthList and DNSNames ["..."] -> [{dns ...}]

Both callers are affected. OrderParametersFromCSR builds an order that is
short one identifier, and validateOrderIdentifiers rejects an otherwise
correct order with "number of identifiers in Order ... (2) does not match
the number of identifiers extracted from CSR ... (1)".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant