From 7781b037daa6f8acd0bd179bc53a0557f1df42a0 Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Fri, 28 Apr 2023 15:21:26 -0400 Subject: [PATCH] Validate identifiers against role when creating order (#20410) * Validate identifiers against role when creating order Perform some initial validation against the order's requested identifiers during creation; this gives a client a heads up that their request might be rejected by the server before they have to solve challenges for these, only to find out during CSR submission time that there is no way to request the specified certificate. Co-authored-by: Steven Clark Signed-off-by: Alexander Scheel * Add unit tests and switch errors to ErrRejectedIdentifier - Change the error messages from validating identifiers against the role to ErrRejectedIdentifier errors if they do occur - Add unit tests to validate that we validate against the various roles somewhat okay. * go doc * Fix typo in test godoc --------- Signed-off-by: Alexander Scheel Co-authored-by: Steven Clark --- builtin/logical/pki/path_acme_order.go | 32 ++++- builtin/logical/pki/path_acme_order_test.go | 142 ++++++++++++++++++++ 2 files changed, 173 insertions(+), 1 deletion(-) create mode 100644 builtin/logical/pki/path_acme_order_test.go diff --git a/builtin/logical/pki/path_acme_order.go b/builtin/logical/pki/path_acme_order.go index 693020414..20590a275 100644 --- a/builtin/logical/pki/path_acme_order.go +++ b/builtin/logical/pki/path_acme_order.go @@ -356,6 +356,33 @@ func validateCsrMatchesOrder(csr *x509.CertificateRequest, order *acmeOrder) err return nil } +func (b *backend) validateIdentifiersAgainstRole(role *roleEntry, identifiers []*ACMEIdentifier) error { + for _, identifier := range identifiers { + switch identifier.Type { + case ACMEDNSIdentifier: + data := &inputBundle{ + role: role, + req: &logical.Request{}, + apiData: &framework.FieldData{}, + } + + if validateNames(b, data, []string{identifier.OriginalValue}) != "" { + return fmt.Errorf("%w: role (%s) will not issue certificate for name %v", + ErrRejectedIdentifier, role.Name, identifier.OriginalValue) + } + case ACMEIPIdentifier: + if !role.AllowIPSANs { + return fmt.Errorf("%w: role (%s) does not allow IP sans, so cannot issue certificate for %v", + ErrRejectedIdentifier, role.Name, identifier.OriginalValue) + } + default: + return fmt.Errorf("unknown type of identifier: %v for %v", identifier.Type, identifier.OriginalValue) + } + } + + return nil +} + func getIdentifiersFromCSR(csr *x509.CertificateRequest) ([]string, []net.IP) { dnsIdentifiers := append([]string(nil), csr.DNSNames...) ipIdentifiers := append([]net.IP(nil), csr.IPAddresses...) @@ -568,7 +595,10 @@ func (b *backend) acmeNewOrderHandler(ac *acmeContext, _ *logical.Request, _ *fr return nil, err } - // TODO: Implement checks against role here. + err = b.validateIdentifiersAgainstRole(ac.role, identifiers) + if err != nil { + return nil, err + } // Per RFC 8555 -> 7.1.3. Order Objects // For pending orders, the authorizations that the client needs to complete before the diff --git a/builtin/logical/pki/path_acme_order_test.go b/builtin/logical/pki/path_acme_order_test.go new file mode 100644 index 000000000..5340bbd31 --- /dev/null +++ b/builtin/logical/pki/path_acme_order_test.go @@ -0,0 +1,142 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package pki + +import ( + "net" + "testing" + + "github.com/hashicorp/vault/sdk/framework" + "github.com/hashicorp/vault/sdk/logical" + "github.com/stretchr/testify/require" +) + +// TestACME_ValidateIdentifiersAgainstRole Verify the ACME order creation +// function verifies somewhat the identifiers that were provided have a +// decent chance of being allowed by the selected role. +func TestACME_ValidateIdentifiersAgainstRole(t *testing.T) { + b, _ := CreateBackendWithStorage(t) + + tests := []struct { + name string + role *roleEntry + identifiers []*ACMEIdentifier + expectErr bool + }{ + { + name: "verbatim-role-allows-dns-ip", + role: buildSignVerbatimRoleWithNoData(nil), + identifiers: _buildACMEIdentifiers("test.com", "127.0.0.1"), + expectErr: false, + }, + { + name: "default-role-does-not-allow-dns", + role: buildTestRole(t, nil), + identifiers: _buildACMEIdentifiers("www.test.com"), + expectErr: true, + }, + { + name: "default-role-allows-ip", + role: buildTestRole(t, nil), + identifiers: _buildACMEIdentifiers("192.168.0.1"), + expectErr: false, + }, + { + name: "disable-ip-sans-forbids-ip", + role: buildTestRole(t, map[string]interface{}{"allow_ip_sans": false}), + identifiers: _buildACMEIdentifiers("192.168.0.1"), + expectErr: true, + }, + { + name: "role-no-wildcards-allowed-without", + role: buildTestRole(t, map[string]interface{}{ + "allow_subdomains": true, + "allow_bare_domains": true, + "allowed_domains": []string{"test.com"}, + "allow_wildcard_certificates": false, + }), + identifiers: _buildACMEIdentifiers("www.test.com", "test.com"), + expectErr: false, + }, + { + name: "role-no-wildcards-allowed-with-wildcard", + role: buildTestRole(t, map[string]interface{}{ + "allow_subdomains": true, + "allowed_domains": []string{"test.com"}, + "allow_wildcard_certificates": false, + }), + identifiers: _buildACMEIdentifiers("*.test.com"), + expectErr: true, + }, + { + name: "role-wildcards-allowed-with-wildcard", + role: buildTestRole(t, map[string]interface{}{ + "allow_subdomains": true, + "allowed_domains": []string{"test.com"}, + "allow_wildcard_certificates": true, + }), + identifiers: _buildACMEIdentifiers("*.test.com"), + expectErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := b.validateIdentifiersAgainstRole(tt.role, tt.identifiers) + + if tt.expectErr { + require.Error(t, err, "validateIdentifiersAgainstRole(%v, %v)", tt.role.ToResponseData(), tt.identifiers) + // If we did return an error if should be classified as a ErrRejectedIdentifier + require.ErrorIs(t, err, ErrRejectedIdentifier) + } else { + require.NoError(t, err, "validateIdentifiersAgainstRole(%v, %v)", tt.role.ToResponseData(), tt.identifiers) + } + }) + } +} + +func _buildACMEIdentifiers(values ...string) []*ACMEIdentifier { + var identifiers []*ACMEIdentifier + + for _, value := range values { + identifiers = append(identifiers, _buildACMEIdentifier(value)) + } + + return identifiers +} + +func _buildACMEIdentifier(val string) *ACMEIdentifier { + ip := net.ParseIP(val) + if ip == nil { + identifier := &ACMEIdentifier{Type: "dns", Value: val, OriginalValue: val, IsWildcard: false} + _, _, _ = identifier.MaybeParseWildcard() + return identifier + } + + return &ACMEIdentifier{Type: "ip", Value: val, OriginalValue: val, IsWildcard: false} +} + +// Easily allow tests to create valid roles with proper defaults, since we don't have an easy +// way to generate roles with proper defaults, go through the createRole handler with the handlers +// field data so we pickup all the defaults specified there. +func buildTestRole(t *testing.T, config map[string]interface{}) *roleEntry { + b, s := CreateBackendWithStorage(t) + + path := pathRoles(b) + fields := path.Fields + if config == nil { + config = map[string]interface{}{} + } + + if _, exists := config["name"]; !exists { + config["name"] = genUuid() + } + + _, err := b.pathRoleCreate(ctx, &logical.Request{Storage: s}, &framework.FieldData{Raw: config, Schema: fields}) + require.NoError(t, err, "failed generating role with config %v", config) + + role, err := b.getRole(ctx, s, config["name"].(string)) + require.NoError(t, err, "failed loading stored role") + + return role +}