From b490396c606fc59bd267a9bb076fca652c0da853 Mon Sep 17 00:00:00 2001 From: James Elliott Date: Thu, 9 Mar 2023 18:26:52 +1100 Subject: [PATCH] refactor: log warnings on startup about oidc secrets (#5047) --- internal/configuration/schema/types.go | 14 +++++ internal/configuration/validator/const.go | 5 +- .../validator/identity_providers.go | 2 + .../validator/identity_providers_test.go | 57 ++++++++++++++----- 4 files changed, 62 insertions(+), 16 deletions(-) diff --git a/internal/configuration/schema/types.go b/internal/configuration/schema/types.go index 342a81fc7..01bbe26a0 100644 --- a/internal/configuration/schema/types.go +++ b/internal/configuration/schema/types.go @@ -140,6 +140,20 @@ type PasswordDigest struct { algorithm.Digest } +// IsPlainText returns true if the underlying algorithm.Digest is a *plaintext.Digest. +func (d *PasswordDigest) IsPlainText() bool { + if d == nil || d.Digest == nil { + return false + } + + switch d.Digest.(type) { + case *plaintext.Digest: + return true + default: + return false + } +} + // NewX509CertificateChain creates a new *X509CertificateChain from a given string, parsing each PEM block one by one. func NewX509CertificateChain(in string) (chain *X509CertificateChain, err error) { if in == "" { diff --git a/internal/configuration/validator/const.go b/internal/configuration/validator/const.go index 0f4b26a0c..0a91a138f 100644 --- a/internal/configuration/validator/const.go +++ b/internal/configuration/validator/const.go @@ -159,8 +159,9 @@ const ( errFmtOIDCClientsWithEmptyID = "identity_providers: oidc: one or more clients have been configured with " + "an empty id" - errFmtOIDCClientInvalidSecret = "identity_providers: oidc: client '%s': option 'secret' is required" - errFmtOIDCClientPublicInvalidSecret = "identity_providers: oidc: client '%s': option 'secret' is " + + errFmtOIDCClientInvalidSecret = "identity_providers: oidc: client '%s': option 'secret' is required" + errFmtOIDCClientInvalidSecretPlainText = "identity_providers: oidc: client '%s': option 'secret' is plaintext but it should be a hashed value as plaintext values are deprecated and will be removed when oidc becomes stable" + errFmtOIDCClientPublicInvalidSecret = "identity_providers: oidc: client '%s': option 'secret' is " + "required to be empty when option 'public' is true" errFmtOIDCClientRedirectURICantBeParsed = "identity_providers: oidc: client '%s': option 'redirect_uris' has an " + "invalid value: redirect uri '%s' could not be parsed: %v" diff --git a/internal/configuration/validator/identity_providers.go b/internal/configuration/validator/identity_providers.go index d63f2fb04..95f7c2707 100644 --- a/internal/configuration/validator/identity_providers.go +++ b/internal/configuration/validator/identity_providers.go @@ -166,6 +166,8 @@ func validateOIDCClients(config *schema.OpenIDConnectConfiguration, val *schema. } else { if client.Secret == nil { val.Push(fmt.Errorf(errFmtOIDCClientInvalidSecret, client.ID)) + } else if client.Secret.IsPlainText() { + val.PushWarning(fmt.Errorf(errFmtOIDCClientInvalidSecretPlainText, client.ID)) } } diff --git a/internal/configuration/validator/identity_providers_test.go b/internal/configuration/validator/identity_providers_test.go index 8113e857e..d77844046 100644 --- a/internal/configuration/validator/identity_providers_test.go +++ b/internal/configuration/validator/identity_providers_test.go @@ -179,8 +179,8 @@ func TestShouldRaiseErrorWhenOIDCServerClientBadValues(t *testing.T) { }, }, Errors: []string{ - fmt.Sprintf(errFmtOIDCClientInvalidSecret, ""), - errFmtOIDCClientsWithEmptyID, + "identity_providers: oidc: client '': option 'secret' is required", + "identity_providers: oidc: one or more clients have been configured with an empty id", }, }, { @@ -195,7 +195,7 @@ func TestShouldRaiseErrorWhenOIDCServerClientBadValues(t *testing.T) { }, }, }, - Errors: []string{fmt.Sprintf(errFmtOIDCClientInvalidPolicy, "client-1", "a-policy")}, + Errors: []string{"identity_providers: oidc: client 'client-1': option 'policy' must be 'one_factor' or 'two_factor' but it is configured as 'a-policy'"}, }, { Name: "ClientIDDuplicated", @@ -427,7 +427,7 @@ func TestShouldRaiseErrorWhenOIDCClientConfiguredWithBadGrantTypes(t *testing.T) Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "good_id", - Secret: MustDecodeSecret("$plaintext$good_secret"), + Secret: MustDecodeSecret(goodOpenIDConnectClientSecret), Policy: "two_factor", GrantTypes: []string{"bad_grant_type"}, RedirectURIs: []string{ @@ -454,7 +454,7 @@ func TestShouldNotErrorOnCertificateValid(t *testing.T) { Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "good_id", - Secret: MustDecodeSecret("$plaintext$good_secret"), + Secret: MustDecodeSecret(goodOpenIDConnectClientSecret), Policy: "two_factor", RedirectURIs: []string{ "https://google.com/callback", @@ -480,7 +480,7 @@ func TestShouldRaiseErrorOnCertificateNotValid(t *testing.T) { Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "good_id", - Secret: MustDecodeSecret("$plaintext$good_secret"), + Secret: MustDecodeSecret(goodOpenIDConnectClientSecret), Policy: "two_factor", RedirectURIs: []string{ "https://google.com/callback", @@ -507,7 +507,7 @@ func TestShouldRaiseErrorOnKeySizeTooSmall(t *testing.T) { Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "good_id", - Secret: MustDecodeSecret("$plaintext$good_secret"), + Secret: MustDecodeSecret(goodOpenIDConnectClientSecret), Policy: "two_factor", RedirectURIs: []string{ "https://google.com/callback", @@ -587,7 +587,7 @@ func TestValidateIdentityProvidersShouldRaiseWarningOnSecurityIssue(t *testing.T Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "good_id", - Secret: MustDecodeSecret("$plaintext$good_secret"), + Secret: MustDecodeSecret(goodOpenIDConnectClientSecret), Policy: "two_factor", RedirectURIs: []string{ "https://google.com/callback", @@ -623,7 +623,7 @@ func TestValidateIdentityProvidersShouldRaiseErrorsOnInvalidClientTypes(t *testi }, { ID: "client-with-bad-redirect-uri", - Secret: MustDecodeSecret("$plaintext$a-secret"), + Secret: MustDecodeSecret(goodOpenIDConnectClientSecret), Public: false, Policy: "two_factor", RedirectURIs: []string{ @@ -702,6 +702,33 @@ func TestValidateIdentityProvidersShouldNotRaiseErrorsOnValidClientOptions(t *te assert.Len(t, validator.Warnings(), 0) } +func TestValidateIdentityProvidersShouldRaiseWarningOnPlainTextClients(t *testing.T) { + validator := schema.NewStructValidator() + config := &schema.IdentityProvidersConfiguration{ + OIDC: &schema.OpenIDConnectConfiguration{ + HMACSecret: "hmac1", + IssuerPrivateKey: MustParseRSAPrivateKey(testKey1), + Clients: []schema.OpenIDConnectClientConfiguration{ + { + ID: "client-with-invalid-secret_standard", + Secret: MustDecodeSecret("$plaintext$a-secret"), + Policy: "two_factor", + RedirectURIs: []string{ + "https://localhost", + }, + }, + }, + }, + } + + ValidateIdentityProviders(config, validator) + + assert.Len(t, validator.Errors(), 0) + require.Len(t, validator.Warnings(), 1) + + assert.EqualError(t, validator.Warnings()[0], "identity_providers: oidc: client 'client-with-invalid-secret_standard': option 'secret' is plaintext but it should be a hashed value as plaintext values are deprecated and will be removed when oidc becomes stable") +} + func TestValidateIdentityProvidersShouldSetDefaultValues(t *testing.T) { timeDay := time.Hour * 24 @@ -713,7 +740,7 @@ func TestValidateIdentityProvidersShouldSetDefaultValues(t *testing.T) { Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "a-client", - Secret: MustDecodeSecret("$plaintext$a-client-secret"), + Secret: MustDecodeSecret(goodOpenIDConnectClientSecret), RedirectURIs: []string{ "https://google.com", }, @@ -722,7 +749,7 @@ func TestValidateIdentityProvidersShouldSetDefaultValues(t *testing.T) { { ID: "b-client", Description: "Normal Description", - Secret: MustDecodeSecret("$plaintext$b-client-secret"), + Secret: MustDecodeSecret(goodOpenIDConnectClientSecret), Policy: policyOneFactor, UserinfoSigningAlgorithm: "RS256", RedirectURIs: []string{ @@ -745,7 +772,7 @@ func TestValidateIdentityProvidersShouldSetDefaultValues(t *testing.T) { }, { ID: "c-client", - Secret: MustDecodeSecret("$plaintext$a-client-secret"), + Secret: MustDecodeSecret(goodOpenIDConnectClientSecret), RedirectURIs: []string{ "https://google.com", }, @@ -753,7 +780,7 @@ func TestValidateIdentityProvidersShouldSetDefaultValues(t *testing.T) { }, { ID: "d-client", - Secret: MustDecodeSecret("$plaintext$a-client-secret"), + Secret: MustDecodeSecret(goodOpenIDConnectClientSecret), RedirectURIs: []string{ "https://google.com", }, @@ -761,7 +788,7 @@ func TestValidateIdentityProvidersShouldSetDefaultValues(t *testing.T) { }, { ID: "e-client", - Secret: MustDecodeSecret("$plaintext$a-client-secret"), + Secret: MustDecodeSecret(goodOpenIDConnectClientSecret), RedirectURIs: []string{ "https://google.com", }, @@ -1019,4 +1046,6 @@ AQmB98tdGLggbyXiODV2h+Rd37aFGb0QHzerIIsVNtMwlPCcp733D4kWJqTUYWZ+ KBL3XEahgs6Os5EYZ4aBAkEAjKE+2/nBYUdHVusjMXeNsE5rqwJND5zvYzmToG7+ xhv4RUAe4dHL4IDQoQRjhr3Nw+JYvtzBx0Iq/178xMnGKg== -----END RSA PRIVATE KEY-----` + + goodOpenIDConnectClientSecret = "$pbkdf2-sha512$310000$c8p78n7pUMln0jzvd4aK4Q$JNRBzwAo0ek5qKn50cFzzvE9RXV88h1wJn5KGiHrD0YKtZaR/nCb2CJPOsKaPK0hjf.9yHxzQGZziziccp6Yng" //nolint:gosec )