From 946a27a64e4ac00a5dc9ba581397245e43f4a82b Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Mon, 19 Dec 2022 11:15:59 -0500 Subject: [PATCH 1/3] Correctly distinguish empty issuer names When using client.Logical().JSONMergePatch(...) with an empty issuer name, patch incorrectly reports: > issuer name contained invalid characters In this case, both the error in getIssuerName(...) is incorrect and patch should allow setting an empty issuer name explicitly. Signed-off-by: Alexander Scheel --- builtin/logical/pki/path_fetch_issuers.go | 2 +- builtin/logical/pki/util.go | 12 +++++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/builtin/logical/pki/path_fetch_issuers.go b/builtin/logical/pki/path_fetch_issuers.go index 753058d2e0d0..0d8a8ab220da 100644 --- a/builtin/logical/pki/path_fetch_issuers.go +++ b/builtin/logical/pki/path_fetch_issuers.go @@ -548,7 +548,7 @@ func (b *backend) pathPatchIssuer(ctx context.Context, req *logical.Request, dat var newName string if ok { newName, err = getIssuerName(sc, data) - if err != nil && err != errIssuerNameInUse { + if err != nil && err != errIssuerNameInUse && err != errIssuerNameIsEmpty { // If the error is name already in use, and the new name is the // old name for this issuer, we're not actually updating the // issuer name (or causing a conflict) -- so don't err out. Other diff --git a/builtin/logical/pki/util.go b/builtin/logical/pki/util.go index 23c3111fbecb..a71a4d01734f 100644 --- a/builtin/logical/pki/util.go +++ b/builtin/logical/pki/util.go @@ -28,9 +28,10 @@ const ( ) var ( - nameMatcher = regexp.MustCompile("^" + framework.GenericNameRegex(issuerRefParam) + "$") - errIssuerNameInUse = errutil.UserError{Err: "issuer name already in use"} - errKeyNameInUse = errutil.UserError{Err: "key name already in use"} + nameMatcher = regexp.MustCompile("^" + framework.GenericNameRegex(issuerRefParam) + "$") + errIssuerNameInUse = errutil.UserError{Err: "issuer name already in use"} + errIssuerNameIsEmpty = errutil.UserError{Err: "expected non-empty issuer name"} + errKeyNameInUse = errutil.UserError{Err: "key name already in use"} ) func serialFromCert(cert *x509.Certificate) string { @@ -159,11 +160,12 @@ func getIssuerName(sc *storageContext, data *framework.FieldData) (string, error issuerNameIface, ok := data.GetOk("issuer_name") if ok { issuerName = strings.TrimSpace(issuerNameIface.(string)) - + if len(issuerName) == 0 { + return issuerName, errIssuerNameIsEmpty + } if strings.ToLower(issuerName) == defaultRef { return issuerName, errutil.UserError{Err: "reserved keyword 'default' can not be used as issuer name"} } - if !nameMatcher.MatchString(issuerName) { return issuerName, errutil.UserError{Err: "issuer name contained invalid characters"} } From 58a5a6a3acc05590d5a87b131826544b55e52ce5 Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Mon, 19 Dec 2022 11:19:00 -0500 Subject: [PATCH 2/3] Add changelog Signed-off-by: Alexander Scheel --- changelog/18466.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/18466.txt diff --git a/changelog/18466.txt b/changelog/18466.txt new file mode 100644 index 000000000000..220e058a16bb --- /dev/null +++ b/changelog/18466.txt @@ -0,0 +1,3 @@ +```release-note:bug +secrets/pki: Allow patching issuer to set an empty issuer name. +``` From 118f82991185416b8d2137c06beff76b363ae7b6 Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Mon, 19 Dec 2022 12:41:43 -0500 Subject: [PATCH 3/3] Add tests Signed-off-by: Alexander Scheel --- builtin/logical/pki/backend_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index c67999984e9a..2c7760cb2ec9 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -5082,6 +5082,16 @@ func TestPerIssuerAIA(t *testing.T) { require.Equal(t, leafCert.IssuingCertificateURL, []string{"https://example.com/ca", "https://backup.example.com/ca"}) require.Equal(t, leafCert.OCSPServer, []string{"https://example.com/ocsp", "https://backup.example.com/ocsp"}) require.Equal(t, leafCert.CRLDistributionPoints, []string{"https://example.com/crl", "https://backup.example.com/crl"}) + + // Validate that we can set an issuer name and remove it. + _, err = CBPatch(b, s, "issuer/default", map[string]interface{}{ + "issuer_name": "my-issuer", + }) + require.NoError(t, err) + _, err = CBPatch(b, s, "issuer/default", map[string]interface{}{ + "issuer_name": "", + }) + require.NoError(t, err) } func TestIssuersWithoutCRLBits(t *testing.T) {