fix(configuration): incorrect sector_identifier_uri validation (#7037)

This fixes a number of validation errors with the sector_identifier_uri for clients. For starters empty strings should be ignored, secondly a sector_identifier_uri must point to a JSON document on a secure protocol i.e. HTTPS but this was not reflected in the tests. We still need to add a check to ensure the JSON document is valid for the client (i.e. includes all of the registered redirect_uris).

Signed-off-by: James Elliott <james-d-elliott@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

config.template.yml

@@ -1368,9 +1368,7 @@ notifier:
         # client_secret: '$pbkdf2-sha512$310000$c8p78n7pUMln0jzvd4aK4Q$JNRBzwAo0ek5qKn50cFzzvE9RXV88h1wJn5KGiHrD0YKtZaR/nCb2CJPOsKaPK0hjf.9yHxzQGZziziccp6Yng'  # The digest of 'insecure_secret'.
 
         ## Sector Identifiers are occasionally used to generate pairwise subject identifiers. In most cases this is not
-        ## necessary. Read the documentation for more information.
-        ## The subject identifier must be the host component of a URL, which is a domain name with an optional port. It
-        ## must return a JSON document which contains an array of every 'redirect_uris' value of each client using it.
+        ## necessary. It is critical to read the documentation for more information.
         # sector_identifier_uri: 'https://example.com/sector.json'
 
         ## Sets the client to public. This should typically not be set, please see the documentation for usage.

docs/content/configuration/identity-providers/openid-connect/clients.md

@@ -155,8 +155,18 @@ the specified client, changing this should cause the relying party to detect all
 users.*
 
 *__Important Note:__ This **must** either not be configured at all i.e. commented or completely absent from the
-configuration, or it must be an absolute HTTPS URL which contains a valid sector identifier JSON document. An empty
-string is not a valid configuration.*
+configuration, or it must be an absolute HTTPS URL which contains a valid sector identifier JSON document. Configuration
+of this option with the `https://` scheme per the requirements will cause Authelia to validate this JSON document.*
+
+A valid `sector_identifier_uri` will:
+  1. Have the scheme `https://`.
+  2. Be the absolute URI of a JSON document which:
+     1. Is a JSON array of strings (URIs).
+     2. Has every URI registered with this clients [redirect_uris](#redirect_uris) when compared using an exact string
+        match as defined in [OAuth 2.0 Security Best Current Practice Section 2.1].
+     3. May or may not have additional [redirect_uris](#redirect_uris) from other clients.
+
+[OAuth 2.0 Security Best Current Practice Section 2.1]: https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics#section-2.1
 
 Authelia utilizes UUID version 4 subject identifiers. By default the public [Subject Identifier Type] is utilized for
 all clients. This means the subject identifiers will be the same for all clients. This configuration option enables
@@ -166,10 +176,10 @@ the lookup of the subject identifier.
 1. All clients who do not have this configured will generate the same subject identifier for a particular user
    regardless of which client obtains the ID token.
 2. All clients which have the same sector identifier will:
-   1. have the same subject identifier for a particular user when compared to clients with the same sector identifier.
-   2. have a completely different subject identifier for a particular user whe compared to:
-      1. any client with the public subject identifier type.
-      2. any client with a differing sector identifier.
+   1. Have the same subject identifier for a particular user when compared to clients with the same sector identifier.
+   2. Have a completely different subject identifier for a particular user when compared to:
+      1. Any client with the public subject identifier type.
+      2. Any client with a differing `sector_identifier_uri`.
 
 In specific but limited scenarios this option is beneficial for privacy reasons. In particular this is useful when the
 party utilizing the *Authelia* [OpenID Connect 1.0] Authorization Server is foreign and not controlled by the user. It would

go.mod

@@ -3,7 +3,7 @@ module github.com/authelia/authelia/v4
 go 1.21
 
 require (
-	authelia.com/provider/oauth2 v0.1.3
+	authelia.com/provider/oauth2 v0.1.4-0.20240331003803-77b086f7aae4
 	github.com/Gurpartap/logrus-stack v0.0.0-20170710170904-89c00d8a28f4
 	github.com/asaskevich/govalidator v0.0.0-20230301143203-a9d515a09cc2
 	github.com/authelia/jsonschema v0.1.7

go.sum

@@ -1,5 +1,5 @@
-authelia.com/provider/oauth2 v0.1.3 h1:xf+UqyismJGmCXWEtWI8o/XO/yXYKzithLHGGm9EvcA=
-authelia.com/provider/oauth2 v0.1.3/go.mod h1:NDritA+Ls5jTa5+t3CSQy58f/QPDofR53IKP8rRHJ8M=
+authelia.com/provider/oauth2 v0.1.4-0.20240331003803-77b086f7aae4 h1:KPTnfrYzKcBo5nZkLBH6wye9diF48ALFtVeMzu76SjI=
+authelia.com/provider/oauth2 v0.1.4-0.20240331003803-77b086f7aae4/go.mod h1:NDritA+Ls5jTa5+t3CSQy58f/QPDofR53IKP8rRHJ8M=
 cloud.google.com/go v0.26.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMTw=
 cloud.google.com/go v0.34.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMTw=
 cloud.google.com/go v0.38.0/go.mod h1:990N+gfupTy94rShfmMCWGDn0LpTmnzTp2qbd1dvSRU=

internal/commands/context.go

@@ -2,6 +2,7 @@ package commands
 
 import (
 	"context"
+	"crypto/tls"
 	"crypto/x509"
 	"fmt"
 	"os"
@@ -228,7 +229,13 @@ func (ctx *CmdCtx) HelperConfigValidateKeysRunE(_ *cobra.Command, _ []string) (e
 
 // HelperConfigValidateRunE validates the configuration (structure).
 func (ctx *CmdCtx) HelperConfigValidateRunE(_ *cobra.Command, _ []string) (err error) {
-	validator.ValidateConfiguration(ctx.config, ctx.cconfig.validator)
+	tc := &tls.Config{
+		RootCAs:    ctx.trusted,
+		MinVersion: tls.VersionTLS12,
+		MaxVersion: tls.VersionTLS13,
+	}
+
+	validator.ValidateConfiguration(ctx.config, ctx.cconfig.validator, validator.WithTLSConfig(tc))
 
 	return nil
 }

internal/configuration/config.template.yml

@@ -1368,9 +1368,7 @@ notifier:
         # client_secret: '$pbkdf2-sha512$310000$c8p78n7pUMln0jzvd4aK4Q$JNRBzwAo0ek5qKn50cFzzvE9RXV88h1wJn5KGiHrD0YKtZaR/nCb2CJPOsKaPK0hjf.9yHxzQGZziziccp6Yng'  # The digest of 'insecure_secret'.
 
         ## Sector Identifiers are occasionally used to generate pairwise subject identifiers. In most cases this is not
-        ## necessary. Read the documentation for more information.
-        ## The subject identifier must be the host component of a URL, which is a domain name with an optional port. It
-        ## must return a JSON document which contains an array of every 'redirect_uris' value of each client using it.
+        ## necessary. It is critical to read the documentation for more information.
         # sector_identifier_uri: 'https://example.com/sector.json'
 
         ## Sets the client to public. This should typically not be set, please see the documentation for usage.

internal/configuration/provider_test.go

@@ -534,7 +534,7 @@ func TestShouldDisableOIDCEntropy(t *testing.T) {
 
 	assert.Equal(t, -1, config.IdentityProviders.OIDC.MinimumParameterEntropy)
 
-	validator.ValidateIdentityProviders(&config.IdentityProviders, val)
+	validator.ValidateIdentityProviders(validator.NewValidateCtx(), &config.IdentityProviders, val)
 
 	assert.Len(t, val.Errors(), 1)
 	require.Len(t, val.Warnings(), 2)

internal/configuration/validator/access_control.go

@@ -59,7 +59,7 @@ func ValidateAccessControl(config *schema.Configuration, validator *schema.Struc
 	}
 
 	if !IsPolicyValid(config.AccessControl.DefaultPolicy) {
-		validator.Push(fmt.Errorf(errFmtAccessControlDefaultPolicyValue, strJoinOr(validACLRulePolicies), config.AccessControl.DefaultPolicy))
+		validator.Push(fmt.Errorf(errFmtAccessControlDefaultPolicyValue, utils.StringJoinOr(validACLRulePolicies), config.AccessControl.DefaultPolicy))
 	}
 
 	for _, n := range config.AccessControl.Networks {
@@ -95,7 +95,7 @@ func ValidateRules(config *schema.Configuration, validator *schema.StructValidat
 			validator.Push(fmt.Errorf(errFmtAccessControlRuleNoPolicy, ruleDescriptor(rulePosition, rule)))
 		default:
 			if !IsPolicyValid(rule.Policy) {
-				validator.Push(fmt.Errorf(errFmtAccessControlRuleInvalidPolicy, ruleDescriptor(rulePosition, rule), strJoinOr(validACLRulePolicies), rule.Policy))
+				validator.Push(fmt.Errorf(errFmtAccessControlRuleInvalidPolicy, ruleDescriptor(rulePosition, rule), utils.StringJoinOr(validACLRulePolicies), rule.Policy))
 			}
 		}
 
@@ -162,11 +162,11 @@ func validateMethods(rulePosition int, rule schema.AccessControlRule, validator
 	invalid, duplicates := validateList(rule.Methods, validACLHTTPMethodVerbs, true)
 
 	if len(invalid) != 0 {
-		validator.Push(fmt.Errorf(errFmtAccessControlRuleInvalidEntries, ruleDescriptor(rulePosition, rule), "methods", strJoinOr(validACLHTTPMethodVerbs), strJoinAnd(invalid)))
+		validator.Push(fmt.Errorf(errFmtAccessControlRuleInvalidEntries, ruleDescriptor(rulePosition, rule), "methods", utils.StringJoinOr(validACLHTTPMethodVerbs), utils.StringJoinAnd(invalid)))
 	}
 
 	if len(duplicates) != 0 {
-		validator.Push(fmt.Errorf(errFmtAccessControlRuleInvalidDuplicates, ruleDescriptor(rulePosition, rule), "methods", strJoinAnd(duplicates)))
+		validator.Push(fmt.Errorf(errFmtAccessControlRuleInvalidDuplicates, ruleDescriptor(rulePosition, rule), "methods", utils.StringJoinAnd(duplicates)))
 	}
 }
 
@@ -184,7 +184,7 @@ func validateQuery(i int, rule schema.AccessControlRule, config *schema.Configur
 					}
 				}
 			} else if !utils.IsStringInSliceFold(config.AccessControl.Rules[i].Query[j][k].Operator, validACLRuleOperators) {
-				validator.Push(fmt.Errorf(errFmtAccessControlRuleQueryInvalid, ruleDescriptor(i+1, rule), strJoinOr(validACLRuleOperators), config.AccessControl.Rules[i].Query[j][k].Operator))
+				validator.Push(fmt.Errorf(errFmtAccessControlRuleQueryInvalid, ruleDescriptor(i+1, rule), utils.StringJoinOr(validACLRuleOperators), config.AccessControl.Rules[i].Query[j][k].Operator))
 			}
 
 			if config.AccessControl.Rules[i].Query[j][k].Key == "" {

internal/configuration/validator/authentication.go

@@ -69,7 +69,7 @@ func ValidatePasswordConfiguration(config *schema.AuthenticationBackendFilePassw
 	case utils.IsStringInSlice(config.Algorithm, validHashAlgorithms):
 		break
 	default:
-		validator.Push(fmt.Errorf(errFmtFileAuthBackendPasswordUnknownAlg, strJoinOr(validHashAlgorithms), config.Algorithm))
+		validator.Push(fmt.Errorf(errFmtFileAuthBackendPasswordUnknownAlg, utils.StringJoinOr(validHashAlgorithms), config.Algorithm))
 	}
 
 	validateFileAuthenticationBackendPasswordConfigArgon2(config, validator)
@@ -87,7 +87,7 @@ func validateFileAuthenticationBackendPasswordConfigArgon2(config *schema.Authen
 	case utils.IsStringInSlice(config.Argon2.Variant, validArgon2Variants):
 		break
 	default:
-		validator.Push(fmt.Errorf(errFmtFileAuthBackendPasswordInvalidVariant, hashArgon2, strJoinOr(validArgon2Variants), config.Argon2.Variant))
+		validator.Push(fmt.Errorf(errFmtFileAuthBackendPasswordInvalidVariant, hashArgon2, utils.StringJoinOr(validArgon2Variants), config.Argon2.Variant))
 	}
 
 	switch {
@@ -145,7 +145,7 @@ func validateFileAuthenticationBackendPasswordConfigSHA2Crypt(config *schema.Aut
 	case utils.IsStringInSlice(config.SHA2Crypt.Variant, validSHA2CryptVariants):
 		break
 	default:
-		validator.Push(fmt.Errorf(errFmtFileAuthBackendPasswordInvalidVariant, hashSHA2Crypt, strJoinOr(validSHA2CryptVariants), config.SHA2Crypt.Variant))
+		validator.Push(fmt.Errorf(errFmtFileAuthBackendPasswordInvalidVariant, hashSHA2Crypt, utils.StringJoinOr(validSHA2CryptVariants), config.SHA2Crypt.Variant))
 	}
 
 	switch {
@@ -174,7 +174,7 @@ func validateFileAuthenticationBackendPasswordConfigPBKDF2(config *schema.Authen
 	case utils.IsStringInSlice(config.PBKDF2.Variant, validPBKDF2Variants):
 		break
 	default:
-		validator.Push(fmt.Errorf(errFmtFileAuthBackendPasswordInvalidVariant, hashPBKDF2, strJoinOr(validPBKDF2Variants), config.PBKDF2.Variant))
+		validator.Push(fmt.Errorf(errFmtFileAuthBackendPasswordInvalidVariant, hashPBKDF2, utils.StringJoinOr(validPBKDF2Variants), config.PBKDF2.Variant))
 	}
 
 	switch {
@@ -203,7 +203,7 @@ func validateFileAuthenticationBackendPasswordConfigBCrypt(config *schema.Authen
 	case utils.IsStringInSlice(config.BCrypt.Variant, validBCryptVariants):
 		break
 	default:
-		validator.Push(fmt.Errorf(errFmtFileAuthBackendPasswordInvalidVariant, hashBCrypt, strJoinOr(validBCryptVariants), config.BCrypt.Variant))
+		validator.Push(fmt.Errorf(errFmtFileAuthBackendPasswordInvalidVariant, hashBCrypt, utils.StringJoinOr(validBCryptVariants), config.BCrypt.Variant))
 	}
 
 	switch {
@@ -363,7 +363,7 @@ func validateLDAPAuthenticationBackendImplementation(config *schema.Authenticati
 	case schema.LDAPImplementationGLAuth:
 		implementation = &schema.DefaultLDAPAuthenticationBackendConfigurationImplementationGLAuth
 	default:
-		validator.Push(fmt.Errorf(errFmtLDAPAuthBackendOptionMustBeOneOf, "implementation", strJoinOr(validLDAPImplementations), config.LDAP.Implementation))
+		validator.Push(fmt.Errorf(errFmtLDAPAuthBackendOptionMustBeOneOf, "implementation", utils.StringJoinOr(validLDAPImplementations), config.LDAP.Implementation))
 	}
 
 	tlsconfig := &schema.TLS{}
@@ -507,22 +507,22 @@ func validateLDAPGroupFilter(config *schema.AuthenticationBackend, validator *sc
 	}
 
 	if !utils.IsStringInSlice(config.LDAP.GroupSearchMode, validLDAPGroupSearchModes) {
-		validator.Push(fmt.Errorf(errFmtLDAPAuthBackendOptionMustBeOneOf, "group_search_mode", strJoinOr(validLDAPGroupSearchModes), config.LDAP.GroupSearchMode))
+		validator.Push(fmt.Errorf(errFmtLDAPAuthBackendOptionMustBeOneOf, "group_search_mode", utils.StringJoinOr(validLDAPGroupSearchModes), config.LDAP.GroupSearchMode))
 	}
 
 	pMemberOfDN, pMemberOfRDN := strings.Contains(config.LDAP.GroupsFilter, "{memberof:dn}"), strings.Contains(config.LDAP.GroupsFilter, "{memberof:rdn}")
 
 	if config.LDAP.GroupSearchMode == schema.LDAPGroupSearchModeMemberOf {
 		if !pMemberOfDN && !pMemberOfRDN {
-			validator.Push(fmt.Errorf(errFmtLDAPAuthBackendFilterMissingPlaceholderGroupSearchMode, "groups_filter", strJoinOr([]string{"{memberof:rdn}", "{memberof:dn}"}), config.LDAP.GroupSearchMode))
+			validator.Push(fmt.Errorf(errFmtLDAPAuthBackendFilterMissingPlaceholderGroupSearchMode, "groups_filter", utils.StringJoinOr([]string{"{memberof:rdn}", "{memberof:dn}"}), config.LDAP.GroupSearchMode))
 		}
 	}
 
 	if pMemberOfDN && config.LDAP.Attributes.DistinguishedName == "" {
-		validator.Push(fmt.Errorf(errFmtLDAPAuthBackendFilterMissingAttribute, "distinguished_name", strJoinOr([]string{"{memberof:dn}"})))
+		validator.Push(fmt.Errorf(errFmtLDAPAuthBackendFilterMissingAttribute, "distinguished_name", utils.StringJoinOr([]string{"{memberof:dn}"})))
 	}
 
 	if (pMemberOfDN || pMemberOfRDN) && config.LDAP.Attributes.MemberOf == "" {
-		validator.Push(fmt.Errorf(errFmtLDAPAuthBackendFilterMissingAttribute, "member_of", strJoinOr([]string{"{memberof:rdn}", "{memberof:dn}"})))
+		validator.Push(fmt.Errorf(errFmtLDAPAuthBackendFilterMissingAttribute, "member_of", utils.StringJoinOr([]string{"{memberof:rdn}", "{memberof:dn}"})))
 	}
 }

internal/configuration/validator/configuration.go

@@ -1,17 +1,28 @@
 package validator
 
 import (
+	"context"
+	"crypto/tls"
 	"fmt"
+	"net"
+	"net/http"
 	"os"
+	"time"
 
 	"github.com/authelia/authelia/v4/internal/configuration/schema"
 	"github.com/authelia/authelia/v4/internal/utils"
 )
 
 // ValidateConfiguration and adapt the configuration read from file.
-func ValidateConfiguration(config *schema.Configuration, validator *schema.StructValidator) {
+func ValidateConfiguration(config *schema.Configuration, validator *schema.StructValidator, opts ...func(ctx *ValidateCtx)) {
 	var err error
 
+	ctx := NewValidateCtx()
+
+	for _, opt := range opts {
+		opt(ctx)
+	}
+
 	if config.CertificatesDirectory != "" {
 		var info os.FileInfo
 
@@ -52,7 +63,7 @@ func ValidateConfiguration(config *schema.Configuration, validator *schema.Struc
 
 	ValidateNotifier(&config.Notifier, validator)
 
-	ValidateIdentityProviders(&config.IdentityProviders, validator)
+	ValidateIdentityProviders(ctx, &config.IdentityProviders, validator)
 
 	ValidateIdentityValidation(config, validator)
 
@@ -69,7 +80,7 @@ func validateDefault2FAMethod(config *schema.Configuration, validator *schema.St
 	}
 
 	if !utils.IsStringInSlice(config.Default2FAMethod, validDefault2FAMethods) {
-		validator.Push(fmt.Errorf(errFmtInvalidDefault2FAMethod, strJoinOr(validDefault2FAMethods), config.Default2FAMethod))
+		validator.Push(fmt.Errorf(errFmtInvalidDefault2FAMethod, utils.StringJoinOr(validDefault2FAMethods), config.Default2FAMethod))
 
 		return
 	}
@@ -89,6 +100,52 @@ func validateDefault2FAMethod(config *schema.Configuration, validator *schema.St
 	}
 
 	if !utils.IsStringInSlice(config.Default2FAMethod, enabledMethods) {
-		validator.Push(fmt.Errorf(errFmtInvalidDefault2FAMethodDisabled, strJoinOr(enabledMethods), config.Default2FAMethod))
+		validator.Push(fmt.Errorf(errFmtInvalidDefault2FAMethodDisabled, utils.StringJoinOr(enabledMethods), config.Default2FAMethod))
+	}
+}
+
+func NewValidateCtx() *ValidateCtx {
+	return &ValidateCtx{
+		Context: context.Background(),
+	}
+}
+
+type ValidateCtx struct {
+	client *http.Client
+
+	tlsconfig *tls.Config
+
+	cacheSectorIdentifierURIs map[string][]string
+
+	context.Context
+}
+
+func (ctx *ValidateCtx) GetHTTPClient() (client *http.Client) {
+	if ctx.client == nil {
+		dialer := &net.Dialer{
+			Timeout:   5 * time.Second,
+			KeepAlive: 30 * time.Second,
+		}
+
+		transport := &http.Transport{
+			Proxy:                 http.ProxyFromEnvironment,
+			DialContext:           dialer.DialContext,
+			ForceAttemptHTTP2:     true,
+			MaxIdleConns:          100,
+			IdleConnTimeout:       90 * time.Second,
+			TLSHandshakeTimeout:   10 * time.Second,
+			ExpectContinueTimeout: 1 * time.Second,
+			TLSClientConfig:       ctx.tlsconfig,
+		}
+
+		ctx.client = &http.Client{Transport: transport}
+	}
+
+	return ctx.client
+}
+
+func WithTLSConfig(config *tls.Config) func(ctx *ValidateCtx) {
+	return func(ctx *ValidateCtx) {
+		ctx.tlsconfig, ctx.client = config, nil
 	}
 }

internal/configuration/validator/configuration_test.go

@@ -1,7 +1,9 @@
 package validator
 
 import (
+	"crypto/tls"
 	"fmt"
+	"net/http"
 	"net/url"
 	"runtime"
 	"testing"
@@ -305,3 +307,50 @@ func TestValidateDefault2FAMethod(t *testing.T) {
 		})
 	}
 }
+
+func TestNewValidateCtx(t *testing.T) {
+	ctx := NewValidateCtx()
+	require.NotNil(t, ctx)
+	assert.NotNil(t, ctx.Context)
+}
+
+func TestValidateCtx_GetHTTPClient_DefaultTLSConfig(t *testing.T) {
+	ctx := NewValidateCtx()
+	client := ctx.GetHTTPClient()
+
+	require.NotNil(t, client)
+	assert.IsType(t, &http.Client{}, client)
+	assert.NotNil(t, client.Transport)
+	transport, ok := client.Transport.(*http.Transport)
+	require.True(t, ok)
+	assert.Nil(t, transport.TLSClientConfig)
+}
+
+func TestValidateCtx_GetHTTPClient_CustomTLSConfig(t *testing.T) {
+	customTLSConfig := &tls.Config{
+		InsecureSkipVerify: true, //nolint:gosec
+		MinVersion:         tls.VersionTLS12,
+		MaxVersion:         tls.VersionTLS13,
+	}
+
+	ctx := NewValidateCtx()
+	WithTLSConfig(customTLSConfig)(ctx)
+	client := ctx.GetHTTPClient()
+
+	require.NotNil(t, client)
+	transport, ok := client.Transport.(*http.Transport)
+	require.True(t, ok)
+	assert.Equal(t, true, transport.TLSClientConfig.InsecureSkipVerify)
+}
+
+func TestValidateCtx_CacheSectorIdentifierURIs(t *testing.T) {
+	ctx := NewValidateCtx()
+	ctx.cacheSectorIdentifierURIs = make(map[string][]string)
+
+	uri := "https://example.com"
+	ctx.cacheSectorIdentifierURIs[uri] = []string{"redirect_uri1", "redirect_uri2"}
+
+	cachedURIs, exists := ctx.cacheSectorIdentifierURIs[uri]
+	require.True(t, exists)
+	assert.Equal(t, []string{"redirect_uri1", "redirect_uri2"}, cachedURIs)
+}