From 203110ed5445df99118222b33bd17d2b4152ec0f Mon Sep 17 00:00:00 2001 From: Scott Miller Date: Thu, 18 Apr 2024 13:30:58 -0500 Subject: [PATCH] Use a less strict URL validation for PKI issuing and crl distribution urls (#26477) (#26514) * Use a less strict URL validation for PKI issuing and crl distribution urls * comma handling * limit to ldap * remove comma hack * changelog * Add unit test validating ldap CRL urls --------- Co-authored-by: Steve Clark --- builtin/logical/pki/integration_test.go | 81 +++++++++++++++++++++++++ builtin/logical/pki/path_config_urls.go | 34 ++++++++++- changelog/26477.txt | 3 + 3 files changed, 116 insertions(+), 2 deletions(-) create mode 100644 changelog/26477.txt diff --git a/builtin/logical/pki/integration_test.go b/builtin/logical/pki/integration_test.go index 657352e50a0b..a7d8a530c84f 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() diff --git a/builtin/logical/pki/path_config_urls.go b/builtin/logical/pki/path_config_urls.go index 4b232e02f6a9..91d1d3578c1c 100644 --- a/builtin/logical/pki/path_config_urls.go +++ b/builtin/logical/pki/path_config_urls.go @@ -7,9 +7,10 @@ import ( "context" "fmt" "net/http" + "net/url" "strings" + "unicode/utf8" - "github.com/asaskevich/govalidator" "github.com/hashicorp/vault/sdk/framework" "github.com/hashicorp/vault/sdk/logical" ) @@ -142,7 +143,7 @@ set on all PR Secondary clusters.`, 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 } } @@ -150,6 +151,35 @@ func validateURLs(urls []string) string { 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 +} + func getGlobalAIAURLs(ctx context.Context, storage logical.Storage) (*aiaConfigEntry, error) { entry, err := storage.Get(ctx, "urls") if err != nil { 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