diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 6b4833c306..acf907defd 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -936,7 +936,7 @@ func generateTestCsr(t *testing.T, keyType certutil.PrivateKeyType, keyBits int) csrTemplate := x509.CertificateRequest{ Subject: pkix.Name{ - Country: []string{"MyCountry"}, + Country: []string{"MC"}, PostalCode: []string{"MyPostalCode"}, SerialNumber: "MySerialNumber", CommonName: "my@example.com", @@ -1561,10 +1561,10 @@ func generateRoleSteps(t *testing.T, useCSRs bool) []logicaltest.TestStep { } // Country tests { - roleVals.Country = []string{"foo"} + roleVals.Country = []string{"jp"} addTests(getCountryCheck(roleVals)) - roleVals.Country = []string{"foo", "bar"} + roleVals.Country = []string{"us", "ca"} addTests(getCountryCheck(roleVals)) } // OU tests diff --git a/builtin/logical/pki/ca_util.go b/builtin/logical/pki/ca_util.go index 744116e35e..bec44e1527 100644 --- a/builtin/logical/pki/ca_util.go +++ b/builtin/logical/pki/ca_util.go @@ -21,11 +21,17 @@ import ( "golang.org/x/crypto/ed25519" ) -func getGenerationParams(sc *storageContext, data *framework.FieldData, isRoot bool) (exported bool, format string, role *issuing.RoleEntry, errorResp *logical.Response) { +type generationParams struct { + exported bool + format string + role *issuing.RoleEntry +} + +func getGenerationParams(sc *storageContext, data *framework.FieldData, isRoot bool) (params generationParams, warnings []string, errorResp *logical.Response) { exportedStr := data.Get("exported").(string) switch exportedStr { case "exported": - exported = true + params.exported = true case "internal": case "existing": case "kms": @@ -35,8 +41,8 @@ func getGenerationParams(sc *storageContext, data *framework.FieldData, isRoot b return } - format = getFormat(data) - if format == "" { + params.format = getFormat(data) + if params.format == "" { errorResp = logical.ErrorResponse( `the "format" path parameter must be "pem", "der", or "pem_bundle"`) return @@ -48,7 +54,12 @@ func getGenerationParams(sc *storageContext, data *framework.FieldData, isRoot b return } - role = &issuing.RoleEntry{ + country := data.Get("country").([]string) + if err := validateCountry(country); err != nil { + warnings = append(warnings, err.Error()) + } + + role := &issuing.RoleEntry{ TTL: time.Duration(data.Get("ttl").(int)) * time.Second, KeyType: keyType, KeyBits: keyBits, @@ -65,7 +76,7 @@ func getGenerationParams(sc *storageContext, data *framework.FieldData, isRoot b AllowedUserIDs: []string{"*"}, OU: data.Get("ou").([]string), Organization: data.Get("organization").([]string), - Country: data.Get("country").([]string), + Country: country, Locality: data.Get("locality").([]string), Province: data.Get("province").([]string), StreetAddress: data.Get("street_address").([]string), @@ -74,6 +85,7 @@ func getGenerationParams(sc *storageContext, data *framework.FieldData, isRoot b CNValidations: []string{"disabled"}, KeyUsage: data.Get("key_usage").([]string), } + params.role = role *role.AllowWildcardCertificates = true if role.KeyBits, err = certutil.ValidateDefaultOrValueKeyType(role.KeyType, role.KeyBits); err != nil { diff --git a/builtin/logical/pki/cert_util.go b/builtin/logical/pki/cert_util.go index ebe10fd184..7f4f8e7517 100644 --- a/builtin/logical/pki/cert_util.go +++ b/builtin/logical/pki/cert_util.go @@ -18,6 +18,7 @@ import ( "strconv" "strings" "time" + "unicode" "github.com/hashicorp/vault/builtin/logical/pki/issuing" "github.com/hashicorp/vault/builtin/logical/pki/parsing" @@ -769,3 +770,18 @@ func (i CertNotAfterInputFromFieldData) GetTTL() int { func (i CertNotAfterInputFromFieldData) GetOptionalNotAfter() (interface{}, bool) { return i.data.GetOk("not_after") } + +func validateCountry(country []string) error { + for _, c := range country { + if len(c) != 2 { + return fmt.Errorf("country must be a 2 character ISO 3166 code, have: %v", c) + } + for _, cc := range c { + cc = unicode.ToUpper(cc) + if cc < 'A' || cc > 'Z' { + return fmt.Errorf("country code characters must be between A and Z, have: %v", c) + } + } + } + return nil +} diff --git a/builtin/logical/pki/path_generate_root_test.go b/builtin/logical/pki/path_generate_root_test.go index c9ba56d250..28a5abb496 100644 --- a/builtin/logical/pki/path_generate_root_test.go +++ b/builtin/logical/pki/path_generate_root_test.go @@ -10,21 +10,23 @@ import ( "github.com/stretchr/testify/require" ) -// TestGenerateRoot_MaxPathLengthValidation validates the behavior of the max_path_length -// parameter on the POST /root/generate/:exported API handler. -// -// The handler logic (pathCAGenerateRoot in path_root.go) is: -// - If max_path_length is not provided in the request, the field is left -// unset on the role and the certificate generation code picks a default -// (no BasicConstraints pathLenConstraint, i.e. MaxPathLen == -1 in Go's -// x509 representation). -// - If max_path_length is provided and is < -1, the request is rejected with -// an error. -// - If max_path_length is provided and is >= -1, the generated certificate -// will carry that pathLenConstraint. -// - When the generated certificate has MaxPathLen == 0 (pathLenConstraint=0), -// Vault adds a warning that the certificate cannot be used to issue -// intermediate CAs. +// TestGenerateRoot_InvalidCountry validates that ISO 3166 is followed for the Country field +func TestGenerateRoot_InvalidCountry(t *testing.T) { + t.Parallel() + b, s := CreateBackendWithStorage(t) + params := map[string]interface{}{ + "common_name": "root.example.com", + "ttl": "87600h", + "key_type": "ec", + "country": "Japan", + } + + resp, err := CBWrite(b, s, "root/generate/internal", params) + require.NoError(t, err) + + require.True(t, stringSliceContainsAny(resp.Warnings, "3166")) +} + func TestGenerateRoot_MaxPathLengthValidation(t *testing.T) { t.Parallel() diff --git a/builtin/logical/pki/path_intermediate.go b/builtin/logical/pki/path_intermediate.go index cdc10578da..555fc2553a 100644 --- a/builtin/logical/pki/path_intermediate.go +++ b/builtin/logical/pki/path_intermediate.go @@ -121,19 +121,25 @@ func (b *backend) pathGenerateIntermediate(ctx context.Context, req *logical.Req } data.Raw["use_pss"] = false + resp := &logical.Response{ + Data: map[string]interface{}{}, + } + sc := b.makeStorageContext(ctx, req.Storage) - exported, format, role, errorResp := getGenerationParams(sc, data, false) + genParams, warnings, errorResp := getGenerationParams(sc, data, false) if errorResp != nil { return errorResp, nil } + if len(warnings) > 0 { + resp.Warnings = append(resp.Warnings, warnings...) + } keyName, err := getKeyName(sc, data) if err != nil { return logical.ErrorResponse(err.Error()), nil } - var resp *logical.Response input := &inputBundle{ - role: role, + role: genParams.role, req: req, apiData: data, } @@ -153,10 +159,6 @@ func (b *backend) pathGenerateIntermediate(ctx context.Context, req *logical.Req return nil, fmt.Errorf("error converting raw CSR bundle to CSR bundle: %w", err) } - resp = &logical.Response{ - Data: map[string]interface{}{}, - } - entries, err := getGlobalAIAURLs(ctx, req.Storage) if err == nil && len(entries.OCSPServers) == 0 && len(entries.IssuingCertificates) == 0 && len(entries.CRLDistributionPoints) == 0 && len(entries.DeltaCRLDistributionPoints) == 0 { @@ -170,17 +172,17 @@ func (b *backend) pathGenerateIntermediate(ctx context.Context, req *logical.Req resp.AddWarning("This mount has configured delta crl distribution points but no base crl distribution points were set.") } - switch format { + switch genParams.format { case "pem": resp.Data["csr"] = csrb.CSR - if exported { + if genParams.exported { resp.Data["private_key"] = csrb.PrivateKey resp.Data["private_key_type"] = csrb.PrivateKeyType } case "pem_bundle": resp.Data["csr"] = csrb.CSR - if exported { + if genParams.exported { resp.Data["csr"] = fmt.Sprintf("%s\n%s", csrb.PrivateKey, csrb.CSR) resp.Data["private_key"] = csrb.PrivateKey resp.Data["private_key_type"] = csrb.PrivateKeyType @@ -188,12 +190,12 @@ func (b *backend) pathGenerateIntermediate(ctx context.Context, req *logical.Req case "der": resp.Data["csr"] = base64.StdEncoding.EncodeToString(parsedBundle.CSRBytes) - if exported { + if genParams.exported { resp.Data["private_key"] = base64.StdEncoding.EncodeToString(parsedBundle.PrivateKeyBytes) resp.Data["private_key_type"] = csrb.PrivateKeyType } default: - return nil, fmt.Errorf("unsupported format argument: %s", format) + return nil, fmt.Errorf("unsupported format argument: %s", genParams.format) } if data.Get("private_key_format").(string) == "pkcs8" { @@ -215,9 +217,9 @@ func (b *backend) pathGenerateIntermediate(ctx context.Context, req *logical.Req observe.NewAdditionalPKIMetadata("key_id", myKey.ID), observe.NewAdditionalPKIMetadata("key_name", myKey.Name), observe.NewAdditionalPKIMetadata("key_type", myKey.PrivateKeyType), - observe.NewAdditionalPKIMetadata("role_name", role.Name), - observe.NewAdditionalPKIMetadata("exported", exported), - observe.NewAdditionalPKIMetadata("type", format)) + observe.NewAdditionalPKIMetadata("role_name", genParams.role.Name), + observe.NewAdditionalPKIMetadata("exported", genParams.exported), + observe.NewAdditionalPKIMetadata("type", genParams.format)) return resp, nil } diff --git a/builtin/logical/pki/path_roles.go b/builtin/logical/pki/path_roles.go index 51e42ee8ba..4a2638e47a 100644 --- a/builtin/logical/pki/path_roles.go +++ b/builtin/logical/pki/path_roles.go @@ -1045,27 +1045,28 @@ func (b *backend) pathRoleCreate(ctx context.Context, req *logical.Request, data } *entry.AllowWildcardCertificates = allowWildcardCertificates.(bool) - warning := "" + var warnings []string // no_store implies generate_lease := false if entry.NoStore { *entry.GenerateLease = false if data.Get("generate_lease").(bool) { - warning = "mutually exclusive values no_store=true and generate_lease=true were both specified; no_store=true takes priority" + warnings = append(warnings, "mutually exclusive values no_store=true and generate_lease=true were both specified; no_store=true takes priority") } } else { *entry.GenerateLease = data.Get("generate_lease").(bool) if *entry.GenerateLease { - warning = "it is encouraged to disable generate_lease and rely on PKI's native capabilities when possible; this option can cause Vault-wide issues with large numbers of issued certificates" + warnings = append(warnings, "it is encouraged to disable generate_lease and rely on PKI's native capabilities when possible; this option can cause Vault-wide issues with large numbers of issued certificates") } } - userError, warnings, err := validateRole(b, entry, ctx, req.Storage) + userError, vrWarns, err := validateRole(b, entry, ctx, req.Storage) if err != nil { return nil, err } - if warning != "" { - warnings = append(warnings, warning) + if len(vrWarns) > 0 { + warnings = append(warnings, vrWarns...) } + if userError != "" { return logical.ErrorResponse(userError), nil } @@ -1112,6 +1113,11 @@ func validateRole(b *backend, entry *issuing.RoleEntry, ctx context.Context, s l return fmt.Sprintf("error setting keyBits %v on role for keyType %v: %v", entry.KeyBits, entry.KeyType, err.Error()), nil, nil } + var warnings []string + if err := validateCountry(entry.Country); err != nil { + warnings = append(warnings, err.Error()) + } + if entry.SerialNumberSource != "" && entry.SerialNumberSource != "json-csr" && entry.SerialNumberSource != "json" { @@ -1142,7 +1148,6 @@ func validateRole(b *backend, entry *issuing.RoleEntry, ctx context.Context, s l issuer = defaultRef } - var warnings []string // Check that the issuers reference set resolves to something if !b.UseLegacyBundleCaStorage() { sc := b.makeStorageContext(ctx, s) diff --git a/builtin/logical/pki/path_roles_test.go b/builtin/logical/pki/path_roles_test.go index 1024d390b8..47ab0c0c17 100644 --- a/builtin/logical/pki/path_roles_test.go +++ b/builtin/logical/pki/path_roles_test.go @@ -219,6 +219,37 @@ func TestPki_RoleKeyUsage(t *testing.T) { } } +// TestPki_RoleBadCountry validates that ISO 3166 is followed for the Country field +func TestPki_RoleBadCountry(t *testing.T) { + t.Parallel() + b, storage := CreateBackendWithStorage(t) + + roleData := map[string]interface{}{ + "allowed_domains": "myvault.com", + "ttl": "5h", + "country": "Japan", + } + + roleReq := &logical.Request{ + Operation: logical.UpdateOperation, + Path: "roles/testrole", + Storage: storage, + Data: roleData, + } + + resp, err := b.HandleRequest(context.Background(), roleReq) + require.NoError(t, err) + require.False(t, resp.IsError()) + require.True(t, stringSliceContainsAny(resp.Warnings, "3166")) + + roleData["country"] = "3p" + + resp, err = b.HandleRequest(context.Background(), roleReq) + require.NoError(t, err) + require.False(t, resp.IsError()) + require.True(t, stringSliceContainsAny(resp.Warnings, "country code")) +} + func TestPki_RoleOUOrganizationUpgrade(t *testing.T) { t.Parallel() var resp *logical.Response @@ -897,7 +928,7 @@ func TestPki_RolePatch(t *testing.T) { { Field: "country", Before: []string{"US"}, - Patched: []string{"USA"}, + Patched: []string{"CA"}, }, { Field: "locality", diff --git a/builtin/logical/pki/path_root.go b/builtin/logical/pki/path_root.go index de88690186..ecf2e63947 100644 --- a/builtin/logical/pki/path_root.go +++ b/builtin/logical/pki/path_root.go @@ -141,11 +141,18 @@ func (b *backend) pathCAGenerateRoot(ctx context.Context, req *logical.Request, sc := b.makeStorageContext(ctx, req.Storage) + resp := &logical.Response{ + Data: make(map[string]any), + } + // Tell getGenerationParams we are generating a root, so that we can validate signatureBits against KeyType - exported, format, role, errorResp := getGenerationParams(sc, data, true) + genParams, warnings, errorResp := getGenerationParams(sc, data, true) if errorResp != nil { return errorResp, nil } + if len(warnings) > 0 { + resp.Warnings = append(resp.Warnings, warnings...) + } maxPathLengthIface, ok := data.GetOk("max_path_length") if ok { @@ -153,7 +160,7 @@ func (b *backend) pathCAGenerateRoot(ctx context.Context, req *logical.Request, if maxPathLength < -1 { return logical.ErrorResponse("requested max_path_length %d is invalid: must be a non-negative integer or -1 for no constraint", maxPathLength), nil } - role.MaxPathLength = &maxPathLength + genParams.role.MaxPathLength = &maxPathLength } issuerName, err := getIssuerName(sc, data) @@ -178,7 +185,7 @@ func (b *backend) pathCAGenerateRoot(ctx context.Context, req *logical.Request, input := &inputBundle{ req: req, apiData: data, - role: role, + role: genParams.role, } b.adjustInputBundle(input) parsedBundle, warnings, err := generateCert(sc, input, nil, true, b.Backend.GetRandomReader()) @@ -196,12 +203,8 @@ func (b *backend) pathCAGenerateRoot(ctx context.Context, req *logical.Request, return nil, fmt.Errorf("error converting raw cert bundle to cert bundle: %w", err) } - resp := &logical.Response{ - Data: map[string]interface{}{ - "expiration": int64(parsedBundle.Certificate.NotAfter.Unix()), - "serial_number": cb.SerialNumber, - }, - } + resp.Data["expiration"] = int64(parsedBundle.Certificate.NotAfter.Unix()) + resp.Data["serial_number"] = cb.SerialNumber if keyUsages, ok := data.GetOk("key_usage"); ok { err = validateCaKeyUsages(keyUsages.([]string)) @@ -237,11 +240,12 @@ func (b *backend) pathCAGenerateRoot(ctx context.Context, req *logical.Request, resp.AddWarning("This mount has configured Delta CRL distribution points are set but no CRL Distribution Points.") } + format := genParams.format switch format { case "pem": resp.Data["certificate"] = cb.Certificate resp.Data["issuing_ca"] = cb.Certificate - if exported { + if genParams.exported { resp.Data["private_key"] = cb.PrivateKey resp.Data["private_key_type"] = cb.PrivateKeyType } @@ -249,7 +253,7 @@ func (b *backend) pathCAGenerateRoot(ctx context.Context, req *logical.Request, case "pem_bundle": resp.Data["issuing_ca"] = cb.Certificate - if exported { + if genParams.exported { resp.Data["private_key"] = cb.PrivateKey resp.Data["private_key_type"] = cb.PrivateKeyType resp.Data["certificate"] = fmt.Sprintf("%s\n%s", cb.PrivateKey, cb.Certificate) @@ -260,7 +264,7 @@ func (b *backend) pathCAGenerateRoot(ctx context.Context, req *logical.Request, case "der": resp.Data["certificate"] = base64.StdEncoding.EncodeToString(parsedBundle.CertificateBytes) resp.Data["issuing_ca"] = base64.StdEncoding.EncodeToString(parsedBundle.CertificateBytes) - if exported { + if genParams.exported { resp.Data["private_key"] = base64.StdEncoding.EncodeToString(parsedBundle.PrivateKeyBytes) resp.Data["private_key_type"] = cb.PrivateKeyType } @@ -344,7 +348,7 @@ func (b *backend) pathCAGenerateRoot(ctx context.Context, req *logical.Request, observe.NewAdditionalPKIMetadata("key_id", myKey.ID), observe.NewAdditionalPKIMetadata("key_name", myKey.Name), observe.NewAdditionalPKIMetadata("key_type", myKey.PrivateKeyType), - observe.NewAdditionalPKIMetadata("role_name", role.Name), + observe.NewAdditionalPKIMetadata("role_name", genParams.role.Name), observe.NewAdditionalPKIMetadata("serial_number", parsing.SerialFromCert(parsedBundle.Certificate)), observe.NewAdditionalPKIMetadata("type", format), observe.NewAdditionalPKIMetadata("common_name", parsedBundle.Certificate.Subject.CommonName), diff --git a/builtin/logical/pki/storage_test.go b/builtin/logical/pki/storage_test.go index 47c55b603f..c1b7eeb4a4 100644 --- a/builtin/logical/pki/storage_test.go +++ b/builtin/logical/pki/storage_test.go @@ -263,7 +263,7 @@ func genCertBundle(t *testing.T, b *backend, s logical.Storage) *certutil.CertBu }, } sc := b.makeStorageContext(ctx, s) - _, _, role, respErr := getGenerationParams(sc, apiData, true) + genParams, _, respErr := getGenerationParams(sc, apiData, true) require.Nil(t, respErr) input := &inputBundle{ @@ -273,7 +273,7 @@ func genCertBundle(t *testing.T, b *backend, s logical.Storage) *certutil.CertBu Storage: s, }, apiData: apiData, - role: role, + role: genParams.role, } parsedCertBundle, _, err := generateCert(sc, input, nil, true, b.GetRandomReader()) diff --git a/changelog/_13346.txt b/changelog/_13346.txt new file mode 100644 index 0000000000..e136b914f7 --- /dev/null +++ b/changelog/_13346.txt @@ -0,0 +1,3 @@ +```release-note:bug +secrets/pki: Warn if the Country field on roles and when generating CAs is not ISO 3166 compliant +``` diff --git a/main.go b/main.go index 7ceceeeeb0..8e47ce2632 100644 --- a/main.go +++ b/main.go @@ -1,6 +1,8 @@ // Copyright IBM Corp. 2016, 2025 // SPDX-License-Identifier: BUSL-1.1 +//go:debug cryptocustomrand=1 + package main // import "github.com/hashicorp/vault" import (