Skip to content

Fix CSR attribute ordering for SCEP enrollment#5909

Merged
rene merged 1 commit intolf-edge:masterfrom
milan-zededa:fix-csr-ordering
May 8, 2026
Merged

Fix CSR attribute ordering for SCEP enrollment#5909
rene merged 1 commit intolf-edge:masterfrom
milan-zededa:fix-csr-ordering

Conversation

@milan-zededa
Copy link
Copy Markdown
Contributor

Description

Some SCEP servers (e.g. Sectigo/cert-manager) require the challengePassword attribute to appear before all the Extension Request attributes (e.g. SAN attributes) in the CSR. The smallstep/scep's x509util.CreateCertificateRequest appends challengePassword after Extension Requests, causing enrollment to fail.

Fix by calling x509.CreateCertificateRequest directly (dropping the x509util wrapper) and prepending challengePassword manually to the raw CSR DER before the Extension Request, then re-signing the certificate request.

How to test and validate this PR

Not every SCEP server requires that the challengePassword precedes Extension Request attributes.
For example, https://github.com/micromdm/scep is fine with any order.
However, if you are for example using Certificate Manager from sectigo, and configure SCEP profile with some SAN attributes, the enrollment would fail without this patch with an error badRequest (2).

Changelog notes

Fixed SCEP certificate enrollment failing due to incorrect CSR attribute ordering: the challenge password attribute is now placed before the certificate extensions, as required by some SCEP servers.

PR Backports

- 16.0-stable: No, as the feature is not available there.
- 14.5-stable: No, as the feature is not available there.
- 13.4-stable: No, as the feature is not available there.

Checklist

  • I've provided a proper description
  • I've added the proper documentation
  • I've tested my PR on amd64 device
  • I've tested my PR on arm64 device
  • I've written the test verification instructions
  • I've set the proper labels to this PR
  • I've checked the boxes above, or I've provided a good reason why I didn't check them.

@milan-zededa milan-zededa added the bug Something isn't working label May 6, 2026
Comment thread pkg/pillar/cmd/scepclient/scep.go Outdated
// prependChallengePassword inserts the challengePassword attribute as the first
// attribute in the CSR DER, before the Extension Request attribute. Some SCEP
// servers require this ordering and reject CSRs where it appears after extensions.
func prependChallengePassword(derBytes []byte, challenge string,
Copy link
Copy Markdown
Contributor Author

@milan-zededa milan-zededa May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, I had to implement this because the Go x509 package does not support the challenge password (see: golang/go#15995).

I was previously using github.com/smallstep/scep/x509util, but it turned out to be incompatible with what the Sectigo SCEP server expects.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 6, 2026

Codecov Report

❌ Patch coverage is 45.45455% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 22.07%. Comparing base (63fa63d) to head (c14363c).
⚠️ Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
pkg/pillar/cmd/scepclient/scep.go 45.45% 29 Missing and 7 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5909      +/-   ##
==========================================
+ Coverage   21.62%   22.07%   +0.44%     
==========================================
  Files         464      474      +10     
  Lines       83994    85743    +1749     
==========================================
+ Hits        18166    18928     +762     
- Misses      64300    65099     +799     
- Partials     1528     1716     +188     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to rebasing on master (so we can get the smoke tests to pass) please consider these comments:

  1. Latent RSA-PSS signing bug in hashFuncForSignatureAlgorithm

hashFuncForSignatureAlgorithm lists SHA*WithRSAPSS cases as supported, but the
call site signs with:

sig, err := key.Sign(rand.Reader, h.Sum(nil), hashFunc)

For an RSA key, passing a bare crypto.Hash as opts makes *rsa.PrivateKey.Sign
produce a PKCS#1 v1.5 signature, not PSS. The resulting CSR would carry a PSS
SignatureAlgorithm OID (copied from req.SignatureAlgorithm) over a PKCS#1 v1.5
signature — silently malformed.

This is currently unreachable because selectSignatureAlgorithm (scep.go:1651)
only ever returns the non-PSS variants for RSA — but that makes the PSS cases
dead-code that looks supported. They're a landmine for the next maintainer who
decides to wire PSS into selectSignatureAlgorithm.

Fix: either drop the three PSS cases (YAGNI) or branch on PSS and pass
&rsa.PSSOptions{Hash: hashFunc, SaltLength: rsa.PSSSaltLengthEqualsHash}.

  1. No tests added

prependChallengePassword does surgical ASN.1 rewriting and re-signs the
result. It's worth at least one unit test that:

  • round-trips a CSR through it,
  • verifies the signature validates against the embedded public key,
  • verifies the challengePassword attribute appears before the extensionRequest
    attribute,
  • verifies SAN entries are preserved.

A small RSA-2048 + SHA-256 fixture-key test would catch any future regression
and the PSS issue above.

Some SCEP servers (e.g. Sectigo/cert-manager) require the
challengePassword attribute to appear before the Extension Request
attribute in the CSR. The smallstep/scep x509util.CreateCertificateRequest
appends challengePassword after the Extension Request, causing enrollment
to fail.

Fix by calling x509.CreateCertificateRequest directly (dropping the
x509util wrapper) and prepending challengePassword manually to the raw
CSR DER before the Extension Request, then re-signing the
TBSCertificateRequest.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Milan Lenco <milan@zededa.com>
@github-actions github-actions Bot requested a review from eriknordmark May 7, 2026 09:35
@milan-zededa
Copy link
Copy Markdown
Contributor Author

Rebased on the top of the latest master and also done all the suggestions from Erik.

Copy link
Copy Markdown
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rene rene merged commit 1fd59f8 into lf-edge:master May 8, 2026
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants