From 1f0cf558a98c9b5989328863156a3c02fd2839f5 Mon Sep 17 00:00:00 2001
From: Ruben De Visscher <github@rubendv.be>
Date: Fri, 7 Oct 2022 18:19:08 +0200
Subject: [PATCH] Fix for duplicate SANs in signed certificates (#16700)

* Fix for duplicate SANs in signed certificates when othernames are present 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 https://github.com/golang/go/issues/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.

* Added unittest and changelog entry.
---
 builtin/logical/pki/backend_test.go | 24 ++++++++++++++++++++++++
 changelog/16700.txt                 |  3 +++
 sdk/helper/certutil/helpers.go      |  7 +++++--
 3 files changed, 32 insertions(+), 2 deletions(-)
 create mode 100644 changelog/16700.txt

diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go
index ba858345d..dbd2be2e3 100644
--- a/builtin/logical/pki/backend_test.go
+++ b/builtin/logical/pki/backend_test.go
@@ -2131,10 +2131,21 @@ func runTestSignVerbatim(t *testing.T, keyType string) {
 	if err != nil {
 		t.Fatal(err)
 	}
+	oidExtensionSubjectAltName = []int{2, 5, 29, 17}
 	csrReq := &x509.CertificateRequest{
 		Subject: pkix.Name{
 			CommonName: "foo.bar.com",
 		},
+		// Check that otherName extensions are not duplicated (see hashicorp/vault#16700).
+		// If these extensions are duplicated, sign-verbatim will fail when parsing the signed certificate on Go 1.19+ (see golang/go#50988).
+		// On older versions of Go this test will fail due to an explicit check for duplicate otherNames later in this test.
+		ExtraExtensions: []pkix.Extension{
+			{
+				Id:       oidExtensionSubjectAltName,
+				Critical: false,
+				Value:    []byte{0x30, 0x26, 0xA0, 0x24, 0x06, 0x0A, 0x2B, 0x06, 0x01, 0x04, 0x01, 0x82, 0x37, 0x14, 0x02, 0x03, 0xA0, 0x16, 0x0C, 0x14, 0x75, 0x73, 0x65, 0x72, 0x6E, 0x61, 0x6D, 0x65, 0x40, 0x65, 0x78, 0x61, 0x6D, 0x70, 0x6C, 0x65, 0x2E, 0x63, 0x6F, 0x6D},
+			},
+		},
 	}
 	csr, err := x509.CreateCertificateRequest(rand.Reader, csrReq, key)
 	if err != nil {
@@ -2285,6 +2296,19 @@ func runTestSignVerbatim(t *testing.T, keyType string) {
 		t.Fatalf("expected a single cert, got %d", len(certs))
 	}
 	cert = certs[0]
+
+	// Fallback check for duplicate otherName, necessary on Go versions before 1.19.
+	// We assume that there is only one SAN in the original CSR and that it is an otherName.
+	san_count := 0
+	for _, ext := range cert.Extensions {
+		if ext.Id.Equal(oidExtensionSubjectAltName) {
+			san_count += 1
+		}
+	}
+	if san_count != 1 {
+		t.Fatalf("expected one SAN extension, got %d", san_count)
+	}
+
 	notAfter := cert.NotAfter.Format(time.RFC3339)
 	if notAfter != "9999-12-31T23:59:59Z" {
 		t.Fatal(fmt.Errorf("not after from certificate is not matching with input parameter"))
diff --git a/changelog/16700.txt b/changelog/16700.txt
new file mode 100644
index 000000000..9dc8d1e2d
--- /dev/null
+++ b/changelog/16700.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+secrets/pki: Fixes duplicate otherName in certificates created by the sign-verbatim endpoint.
+```
diff --git a/sdk/helper/certutil/helpers.go b/sdk/helper/certutil/helpers.go
index 0971fefe8..58ebc06f2 100644
--- a/sdk/helper/certutil/helpers.go
+++ b/sdk/helper/certutil/helpers.go
@@ -1033,7 +1033,10 @@ func selectSignatureAlgorithmForECDSA(pub crypto.PublicKey, signatureBits int) x
 	}
 }
 
-var oidExtensionBasicConstraints = []int{2, 5, 29, 19}
+var (
+	oidExtensionBasicConstraints = []int{2, 5, 29, 19}
+	oidExtensionSubjectAltName   = []int{2, 5, 29, 17}
+)
 
 // CreateCSR creates a CSR with the default rand.Reader to
 // generate a cert/keypair. This is currently only meant
@@ -1208,7 +1211,7 @@ func signCertificate(data *CreationBundle, randReader io.Reader) (*ParsedCertBun
 		certTemplate.URIs = data.CSR.URIs
 
 		for _, name := range data.CSR.Extensions {
-			if !name.Id.Equal(oidExtensionBasicConstraints) {
+			if !name.Id.Equal(oidExtensionBasicConstraints) && !(len(data.Params.OtherSANs) > 0 && name.Id.Equal(oidExtensionSubjectAltName)) {
 				certTemplate.ExtraExtensions = append(certTemplate.ExtraExtensions, name)
 			}
 		}