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 <[email protected]>
hashicorp · Apr 18, 2024 · 203110e · 203110e
1 parent 334c185
commit 203110e
Show file tree

Hide file tree

Showing 3 changed files with 116 additions and 2 deletions.
diff --git 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
@@ -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,14 +143,43 @@ 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
 		}
 	}
 
 	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
@@ -0,0 +1,3 @@
+```release-note:bug
+secrets/pki: fixed validation bug which rejected ldap schemed URLs in crl_distribution_points.
+```