Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for duplicate SANs in signed certificates #16700

Merged
merged 2 commits into from
Oct 7, 2022

Conversation

rubendv
Copy link
Contributor

@rubendv rubendv commented Aug 12, 2022

When UseCSRValues is true (as is the case on the sign-verbatim endpoint), all extensions including Subject Alternative Names are copied from the CSR to the final certificate.

If the Subject Alternative Name in question contains any othernames (such as a Microsoft UPN) the SAN extension is added again a few lines later as a workaround for an encoding issue (by function HandleOtherSANs).
This way you end up with two copies of the SAN extension in the final certificate.

Having duplicate x509v3 extensions is invalid and is rejected by openssl on Ubuntu 20.04 (but not on Ubuntu 18.04), and is also rejected by Go in x509.ParseCertificate since golang/go#50988 (part of Go 1.19).

In this fix I do not copy the extension from the CSR if it will already be added by HandleOtherSANs.

@hashicorp-cla
Copy link

hashicorp-cla commented Aug 12, 2022

CLA assistant check
All committers have signed the CLA.

@rubendv rubendv changed the title Fix for duplicate SANs in signed certificates when othernames are present in the CSR SAN extension and UseCSRValues is true. Fix for duplicate SANs in signed certificates Aug 12, 2022
@heatherezell heatherezell added auth/cert Authentication - certificates bug Used to indicate a potential bug labels Aug 12, 2022
@cipherboy cipherboy added secret/pki cryptosec and removed auth/cert Authentication - certificates labels Sep 22, 2022
@cipherboy
Copy link
Contributor

\o Hey @rubendv, thanks for the PR! :-) Do you mind adding a test for this? It'd be great to make sure we don't break this again in the future.

@cipherboy cipherboy added this to the 1.13.0-rc1 milestone Sep 22, 2022
@rubendv
Copy link
Contributor Author

rubendv commented Sep 22, 2022

\o Hey @rubendv, thanks for the PR! :-) Do you mind adding a test for this? It'd be great to make sure we don't break this again in the future.

Sure! I will look into it.

@rubendv
Copy link
Contributor Author

rubendv commented Oct 7, 2022

I added tests @cipherboy. I noticed in the contributing guide it says that PRs should reference issues, would you like me to still create an issue for this PR?

@cipherboy
Copy link
Contributor

@rubendv Nah, no point IMO. I understand the problem well enough. Thank you!

@cipherboy
Copy link
Contributor

@rubendv Actually, could I bother you to rebase this on top of a newer main? I think test-go-remote-docker failed because you happened to base it off the point in time where we were trying to fix the CI's container network provisioning.

Looks like rerunning it didn't help:

+ docker exec -w /home/circleci/go/src/github.com/hashicorp/vault/ -e CIRCLECI -e VAULT_CI_GO_TEST_RACE -e GOCACHE=/tmp/gocache -e GO_TAGS -e GOPROXY=off -e VAULT_LICENSE_CI -e GOARCH=amd64 testcontainer gotestsum --format=short-verbose --junitfile test-results/go-test/results.xml --jsonfile test-results/go-test/results.json -- -tags ' deadlock' -timeout=60m -parallel=20 github.com/hashicorp/vault/plugins/database/mongodb
=== RUN   TestMongoDB_Initialize
    mongodbhelper.go:68: could not start docker mongo: no reachable servers
--- FAIL: TestMongoDB_Initialize (135.77s)
FAIL plugins/database/mongodb.TestMongoDB_Initialize (135.77s)

…sent in the CSR SAN extension and UseCSRValues is true.

When UseCSRValues is true (as is the case on the sign-verbatim endpoint), all extensions including Subject Alternative Names are copied from the CSR to the final certificate.
If the Subject Alternative Name in question contains any othernames (such as a Microsoft UPN) the SAN extension is added again as a workaround for an encoding issue (in function HandleOtherSANs).
Having duplicate x509v3 extensions is invalid and is rejected by openssl on Ubuntu 20.04, and also by Go since golang/go#50988 (including in Go 1.19).

In this fix I do not add the extension from the CSR if it will be added during HandleOtherSANs.
@cipherboy cipherboy enabled auto-merge (squash) October 7, 2022 16:01
@cipherboy cipherboy merged commit 4a2e014 into hashicorp:main Oct 7, 2022
@cipherboy
Copy link
Contributor

Thanks @rubendv, that worked!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Used to indicate a potential bug cryptosec secret/pki
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants