From 2e8e46d02b77457dbc25bfd52b7a31668b580007 Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Wed, 17 Apr 2024 15:18:37 -0500 Subject: [PATCH 1/6] Use a less strict URL validation for PKI issuing and crl distribution urls --- builtin/logical/pki/issuing/aia.go | 34 ++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/builtin/logical/pki/issuing/aia.go b/builtin/logical/pki/issuing/aia.go index 4c05c4231d77..0f2e76b99f4a 100644 --- a/builtin/logical/pki/issuing/aia.go +++ b/builtin/logical/pki/issuing/aia.go @@ -6,9 +6,10 @@ package issuing import ( "context" "fmt" + "net/url" "strings" + "unicode/utf8" - "github.com/asaskevich/govalidator" "github.com/hashicorp/vault/sdk/helper/certutil" "github.com/hashicorp/vault/sdk/logical" ) @@ -145,10 +146,39 @@ func GetClusterConfig(ctx context.Context, s logical.Storage) (*ClusterConfigEnt func ValidateURLs(urls []string) string { for _, curr := range urls { - if !govalidator.IsURL(curr) || strings.Contains(curr, "{{issuer_id}}") || strings.Contains(curr, "{{cluster_path}}") || strings.Contains(curr, "{{cluster_aia_path}}") { + if !isURL(curr) || strings.Contains(curr, "{{issuer_id}}") || strings.Contains(curr, "{{cluster_path}}") || strings.Contains(curr, "{{cluster_aia_path}}") { return curr } } return "" } + +const ( + maxURLRuneCount = 2083 + minURLRuneCount = 3 +) + +// IsURL checks if the string is an URL. +func isURL(str string) bool { + if str == "" || utf8.RuneCountInString(str) >= maxURLRuneCount || len(str) <= minURLRuneCount || strings.HasPrefix(str, ".") { + return false + } + strTemp := str + if strings.Contains(str, ":") && !strings.Contains(str, "://") { + // support no indicated urlscheme but with colon for port number + // http:// is appended so url.Parse will succeed, strTemp used so it does not impact rxURL.MatchString + strTemp = "http://" + str + } + u, err := url.ParseRequestURI(strTemp) + if err != nil { + return false + } + if strings.HasPrefix(u.Host, ".") { + return false + } + if u.Host == "" && (u.Path != "" && !strings.Contains(u.Path, ".")) { + return false + } + return true +} From 61130c6b08350af9864d32f9df851f2cbe8a606a Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Wed, 17 Apr 2024 15:27:34 -0500 Subject: [PATCH 2/6] comma handling --- builtin/logical/pki/path_config_urls.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/builtin/logical/pki/path_config_urls.go b/builtin/logical/pki/path_config_urls.go index c79102b0350a..563071e1ad33 100644 --- a/builtin/logical/pki/path_config_urls.go +++ b/builtin/logical/pki/path_config_urls.go @@ -7,6 +7,7 @@ import ( "context" "fmt" "net/http" + "strings" "github.com/hashicorp/vault/builtin/logical/pki/issuing" "github.com/hashicorp/vault/sdk/framework" @@ -217,6 +218,12 @@ func (b *backend) pathWriteURL(ctx context.Context, req *logical.Request, data * entries.OCSPServers = urlsInt.([]string) } + for i, e := range entries.CRLDistributionPoints { + if strings.Contains(e, "%2C") || strings.Contains(e, "%2c") { + // May be URL encoded to avoid problems with commas + entries.CRLDistributionPoints[i] = strings.ReplaceAll(strings.ReplaceAll(e, "%2c", ","), "%2C", ",") + } + } resp := &logical.Response{ Data: map[string]interface{}{ "issuing_certificates": entries.IssuingCertificates, From a041bc1cb5384bf3add839fe667db871a233f5be Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Wed, 17 Apr 2024 16:14:16 -0500 Subject: [PATCH 3/6] limit to ldap --- builtin/logical/pki/path_config_urls.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/logical/pki/path_config_urls.go b/builtin/logical/pki/path_config_urls.go index 563071e1ad33..27bb617dcf07 100644 --- a/builtin/logical/pki/path_config_urls.go +++ b/builtin/logical/pki/path_config_urls.go @@ -219,8 +219,8 @@ func (b *backend) pathWriteURL(ctx context.Context, req *logical.Request, data * } for i, e := range entries.CRLDistributionPoints { - if strings.Contains(e, "%2C") || strings.Contains(e, "%2c") { - // May be URL encoded to avoid problems with commas + if strings.HasPrefix(e, "ldap") && strings.Contains(e, "%2C") || strings.Contains(e, "%2c") { + // May be an LDAP(S) URL encoded to avoid problems with commas entries.CRLDistributionPoints[i] = strings.ReplaceAll(strings.ReplaceAll(e, "%2c", ","), "%2C", ",") } } From 43a6721fb3f68e4ad95677b62e53d6db297020ff Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Thu, 18 Apr 2024 10:39:18 -0500 Subject: [PATCH 4/6] remove comma hack --- builtin/logical/pki/path_config_urls.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/builtin/logical/pki/path_config_urls.go b/builtin/logical/pki/path_config_urls.go index 27bb617dcf07..c79102b0350a 100644 --- a/builtin/logical/pki/path_config_urls.go +++ b/builtin/logical/pki/path_config_urls.go @@ -7,7 +7,6 @@ import ( "context" "fmt" "net/http" - "strings" "github.com/hashicorp/vault/builtin/logical/pki/issuing" "github.com/hashicorp/vault/sdk/framework" @@ -218,12 +217,6 @@ func (b *backend) pathWriteURL(ctx context.Context, req *logical.Request, data * entries.OCSPServers = urlsInt.([]string) } - for i, e := range entries.CRLDistributionPoints { - if strings.HasPrefix(e, "ldap") && strings.Contains(e, "%2C") || strings.Contains(e, "%2c") { - // May be an LDAP(S) URL encoded to avoid problems with commas - entries.CRLDistributionPoints[i] = strings.ReplaceAll(strings.ReplaceAll(e, "%2c", ","), "%2C", ",") - } - } resp := &logical.Response{ Data: map[string]interface{}{ "issuing_certificates": entries.IssuingCertificates, From ed3e9d0946f190644ce106943ce549e8cc839820 Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Thu, 18 Apr 2024 11:04:51 -0500 Subject: [PATCH 5/6] changelog --- changelog/26477.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/26477.txt diff --git a/changelog/26477.txt b/changelog/26477.txt new file mode 100644 index 000000000000..f24fca805cd2 --- /dev/null +++ b/changelog/26477.txt @@ -0,0 +1,3 @@ +```release-note:bug +secrets/pki: fixed validation bug which rejected ldap schemed URLs in crl_distribution_points. +``` \ No newline at end of file From 3bfe7a02b9a87dc50dc712c132fbb1071c76d089 Mon Sep 17 00:00:00 2001 From: Steve Clark Date: Thu, 18 Apr 2024 12:06:37 -0400 Subject: [PATCH 6/6] Add unit test validating ldap CRL urls --- builtin/logical/pki/integration_test.go | 81 +++++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/builtin/logical/pki/integration_test.go b/builtin/logical/pki/integration_test.go index a0912dab1c4b..73d7d113ad88 100644 --- a/builtin/logical/pki/integration_test.go +++ b/builtin/logical/pki/integration_test.go @@ -480,6 +480,87 @@ func TestIntegration_AutoIssuer(t *testing.T) { require.Equal(t, issuerIdOneReimported, resp.Data["default"]) } +// TestLDAPAiaCrlUrls validates we can properly handle CRL urls that are ldap based. +func TestLDAPAiaCrlUrls(t *testing.T) { + t.Parallel() + + coreConfig := &vault.CoreConfig{ + LogicalBackends: map[string]logical.Factory{ + "pki": Factory, + }, + } + cluster := vault.NewTestCluster(t, coreConfig, &vault.TestClusterOptions{ + NumCores: 1, + HandlerFunc: vaulthttp.Handler, + }) + cluster.Start() + defer cluster.Cleanup() + singleCore := cluster.Cores[0] + vault.TestWaitActive(t, singleCore.Core) + client := singleCore.Client + + mountPKIEndpoint(t, client, "pki") + + // Attempt multiple urls + crls := []string{ + "ldap://ldap.example.com/cn=example%20CA,dc=example,dc=com?certificateRevocationList;binary", + "ldap://ldap.example.com/cn=CA,dc=example,dc=com?authorityRevocationList;binary", + } + + _, err := client.Logical().Write("pki/config/urls", map[string]interface{}{ + "crl_distribution_points": crls, + }) + require.NoError(t, err) + + resp, err := client.Logical().Read("pki/config/urls") + require.NoError(t, err, "failed reading config/urls") + require.NotNil(t, resp, "resp was nil") + require.NotNil(t, resp.Data, "data within response was nil") + require.NotEmpty(t, resp.Data["crl_distribution_points"], "crl_distribution_points was nil within data") + require.Len(t, resp.Data["crl_distribution_points"], len(crls)) + + for _, crlVal := range crls { + require.Contains(t, resp.Data["crl_distribution_points"], crlVal) + } + + resp, err = client.Logical().Write("pki/root/generate/internal", map[string]interface{}{ + "ttl": "40h", + "common_name": "Root R1", + "key_type": "ec", + }) + require.NoError(t, err) + require.NotNil(t, resp) + require.NotNil(t, resp.Data) + require.NotEmpty(t, resp.Data["issuer_id"]) + rootIssuerId := resp.Data["issuer_id"].(string) + + _, err = client.Logical().Write("pki/roles/example-root", map[string]interface{}{ + "allowed_domains": "example.com", + "allow_subdomains": "true", + "max_ttl": "1h", + "key_type": "ec", + "issuer_ref": rootIssuerId, + }) + require.NoError(t, err) + + resp, err = client.Logical().Write("pki/issue/example-root", map[string]interface{}{ + "common_name": "test.example.com", + "ttl": "5m", + }) + require.NoError(t, err) + require.NotNil(t, resp) + require.NotNil(t, resp.Data) + require.NotEmpty(t, resp.Data["certificate"]) + + certPEM := resp.Data["certificate"].(string) + certBlock, _ := pem.Decode([]byte(certPEM)) + require.NotNil(t, certBlock) + cert, err := x509.ParseCertificate(certBlock.Bytes) + require.NoError(t, err) + + require.EqualValues(t, crls, cert.CRLDistributionPoints) +} + func TestIntegrationOCSPClientWithPKI(t *testing.T) { t.Parallel()