From 248f1d49d45c2fb1e9cf5727addca44aa17c5376 Mon Sep 17 00:00:00 2001 From: James Elliott Date: Thu, 20 Oct 2022 14:21:45 +1100 Subject: [PATCH] feat(oidc): hashed client secrets (#4026) Allow use of hashed OpenID Connect client secrets. --- ...contributing-development-commitmsg.md.tmpl | 1 + .../identity-providers/open-id-connect.md | 6 +- .../contributing/guidelines/commit-message.md | 1 + .../contributing/guidelines/pull-request.md | 2 +- .../openid-connect/apache-guacamole/index.md | 2 +- .../openid-connect/argocd/index.md | 2 +- .../openid-connect/bookstack/index.md | 2 +- .../cloudflare-zerotrust/index.md | 2 +- .../integration/openid-connect/gitea/index.md | 2 +- .../openid-connect/gitlab/index.md | 2 +- .../openid-connect/grafana/index.md | 2 +- .../openid-connect/harbor/index.md | 2 +- .../openid-connect/hashicorp-vault/index.md | 2 +- .../integration/openid-connect/komga/index.md | 2 +- .../openid-connect/nextcloud/index.md | 2 +- .../openid-connect/outline/index.md | 2 +- .../openid-connect/portainer/index.md | 2 +- .../openid-connect/proxmox/index.md | 2 +- .../openid-connect/seafile/index.md | 2 +- .../openid-connect/synapse/index.md | 2 +- .../en/reference/guides/rule-operators.md | 2 +- internal/configuration/decode_hooks.go | 52 +++++++++++++++++ internal/configuration/provider.go | 1 + .../schema/identity_providers.go | 10 ++-- internal/configuration/schema/types.go | 25 ++++++++ .../test_resources/config_oidc.yml | 2 +- .../validator/identity_providers.go | 4 +- .../validator/identity_providers_test.go | 58 +++++++++++-------- internal/oidc/client.go | 8 ++- internal/oidc/client_test.go | 47 ++++++++++----- internal/oidc/hasher.go | 19 ++++-- internal/oidc/hasher_test.go | 10 ++-- internal/oidc/provider.go | 2 +- internal/oidc/provider_test.go | 12 ++-- internal/oidc/store_test.go | 14 ++--- internal/oidc/types.go | 7 ++- 36 files changed, 218 insertions(+), 97 deletions(-) diff --git a/cmd/authelia-gen/templates/docs-contributing-development-commitmsg.md.tmpl b/cmd/authelia-gen/templates/docs-contributing-development-commitmsg.md.tmpl index 7384db785..28f51882c 100644 --- a/cmd/authelia-gen/templates/docs-contributing-development-commitmsg.md.tmpl +++ b/cmd/authelia-gen/templates/docs-contributing-development-commitmsg.md.tmpl @@ -12,6 +12,7 @@ weight: 320 toc: true aliases: - /docs/contributing/commitmsg-guidelines.html + - /contributing/development/guidelines-commit-message/ --- The reasons for these conventions are as follows: diff --git a/docs/content/en/configuration/identity-providers/open-id-connect.md b/docs/content/en/configuration/identity-providers/open-id-connect.md index 57e31742d..e1018e764 100644 --- a/docs/content/en/configuration/identity-providers/open-id-connect.md +++ b/docs/content/en/configuration/identity-providers/open-id-connect.md @@ -118,7 +118,7 @@ identity_providers: clients: - id: myapp description: My Application - secret: this_is_a_secret + secret: '$plaintext$this_is_a_secret' sector_identifier: '' public: false authorization_policy: two_factor @@ -352,7 +352,9 @@ A friendly description for this client shown in the UI. This defaults to the sam {{< confkey type="string" required="situational" >}} The shared secret between Authelia and the application consuming this client. This secret must match the secret -configured in the application. Currently this is stored in plain text. +configured in the application. This can either be stored in plain text (by prefixing the plain text secret with +`$plaintext$` or can be a hashed password generated with +[authelia crypto hash](../../reference/cli/authelia/authelia_hash-password.md). This secret must be generated by the administrator and can be done by following the [Generating a Random Alphanumeric String](../miscellaneous/guides.md#generating-a-random-alphanumeric-string) guide. diff --git a/docs/content/en/contributing/guidelines/commit-message.md b/docs/content/en/contributing/guidelines/commit-message.md index 1260b5108..8683eb194 100644 --- a/docs/content/en/contributing/guidelines/commit-message.md +++ b/docs/content/en/contributing/guidelines/commit-message.md @@ -12,6 +12,7 @@ weight: 320 toc: true aliases: - /docs/contributing/commitmsg-guidelines.html + - /contributing/development/guidelines-commit-message/ --- The reasons for these conventions are as follows: diff --git a/docs/content/en/contributing/guidelines/pull-request.md b/docs/content/en/contributing/guidelines/pull-request.md index b7aa4ffa5..d3f2830ab 100644 --- a/docs/content/en/contributing/guidelines/pull-request.md +++ b/docs/content/en/contributing/guidelines/pull-request.md @@ -11,7 +11,7 @@ menu: weight: 320 toc: true aliases: - - /contributing/development/pull-request/ + - /contributing/development/guidelines-pull-request/ --- [Pull Request] guidelines are in place in order to maintain consistency and clearly communicate our process for diff --git a/docs/content/en/integration/openid-connect/apache-guacamole/index.md b/docs/content/en/integration/openid-connect/apache-guacamole/index.md index 26bd069d0..c609109c5 100644 --- a/docs/content/en/integration/openid-connect/apache-guacamole/index.md +++ b/docs/content/en/integration/openid-connect/apache-guacamole/index.md @@ -59,7 +59,7 @@ The following YAML configuration is an example __Authelia__ ```yaml - id: guacamole description: Apache Guacamole - secret: guacamole_client_secret + secret: '$plaintext$guacamole_client_secret' public: false authorization_policy: two_factor redirect_uris: diff --git a/docs/content/en/integration/openid-connect/argocd/index.md b/docs/content/en/integration/openid-connect/argocd/index.md index 64a3b7bc2..625b97e3b 100644 --- a/docs/content/en/integration/openid-connect/argocd/index.md +++ b/docs/content/en/integration/openid-connect/argocd/index.md @@ -62,7 +62,7 @@ which will operate with the above example: ```yaml - id: argocd description: Argo CD - secret: argocd_client_secret + secret: '$plaintext$argocd_client_secret' public: false authorization_policy: two_factor redirect_uris: diff --git a/docs/content/en/integration/openid-connect/bookstack/index.md b/docs/content/en/integration/openid-connect/bookstack/index.md index a23399516..f1a10653a 100644 --- a/docs/content/en/integration/openid-connect/bookstack/index.md +++ b/docs/content/en/integration/openid-connect/bookstack/index.md @@ -64,7 +64,7 @@ which will operate with the above example: ```yaml - id: bookstack description: BookStack - secret: bookstack_client_secret + secret: '$plaintext$bookstack_client_secret' public: false authorization_policy: two_factor redirect_uris: diff --git a/docs/content/en/integration/openid-connect/cloudflare-zerotrust/index.md b/docs/content/en/integration/openid-connect/cloudflare-zerotrust/index.md index 0ff85a013..97fc8e3d7 100644 --- a/docs/content/en/integration/openid-connect/cloudflare-zerotrust/index.md +++ b/docs/content/en/integration/openid-connect/cloudflare-zerotrust/index.md @@ -72,7 +72,7 @@ which will operate with the above example: ```yaml - id: cloudflare description: Cloudflare ZeroTrust - secret: cloudflare_client_secret + secret: '$plaintext$cloudflare_client_secret' public: false authorization_policy: two_factor redirect_uris: diff --git a/docs/content/en/integration/openid-connect/gitea/index.md b/docs/content/en/integration/openid-connect/gitea/index.md index 3c395c414..3669ae1c3 100644 --- a/docs/content/en/integration/openid-connect/gitea/index.md +++ b/docs/content/en/integration/openid-connect/gitea/index.md @@ -79,7 +79,7 @@ will operate with the above example: ```yaml - id: gitea description: Gitea - secret: gitea_client_secret + secret: '$plaintext$gitea_client_secret' public: false authorization_policy: two_factor redirect_uris: diff --git a/docs/content/en/integration/openid-connect/gitlab/index.md b/docs/content/en/integration/openid-connect/gitlab/index.md index 3c3beaf2a..342da49eb 100644 --- a/docs/content/en/integration/openid-connect/gitlab/index.md +++ b/docs/content/en/integration/openid-connect/gitlab/index.md @@ -75,7 +75,7 @@ which will operate with the above example: ```yaml - id: gitlab description: GitLab - secret: gitlab_client_secret + secret: '$plaintext$gitlab_client_secret' public: false authorization_policy: two_factor redirect_uris: diff --git a/docs/content/en/integration/openid-connect/grafana/index.md b/docs/content/en/integration/openid-connect/grafana/index.md index a0fe50571..002fc63e2 100644 --- a/docs/content/en/integration/openid-connect/grafana/index.md +++ b/docs/content/en/integration/openid-connect/grafana/index.md @@ -93,7 +93,7 @@ which will operate with the above example: ```yaml - id: grafana description: Grafana - secret: grafana_client_secret + secret: '$plaintext$grafana_client_secret' public: false authorization_policy: two_factor redirect_uris: diff --git a/docs/content/en/integration/openid-connect/harbor/index.md b/docs/content/en/integration/openid-connect/harbor/index.md index 2b32ca0ff..59d36873a 100644 --- a/docs/content/en/integration/openid-connect/harbor/index.md +++ b/docs/content/en/integration/openid-connect/harbor/index.md @@ -66,7 +66,7 @@ which will operate with the above example: ```yaml - id: harbor description: Harbor - secret: harbor_client_secret + secret: '$plaintext$harbor_client_secret' public: false authorization_policy: two_factor redirect_uris: diff --git a/docs/content/en/integration/openid-connect/hashicorp-vault/index.md b/docs/content/en/integration/openid-connect/hashicorp-vault/index.md index dd0a53c3b..e8dd07aa6 100644 --- a/docs/content/en/integration/openid-connect/hashicorp-vault/index.md +++ b/docs/content/en/integration/openid-connect/hashicorp-vault/index.md @@ -49,7 +49,7 @@ which will operate with the above example: ```yaml - id: vault description: HashiCorp Vault - secret: vault_client_secret + secret: '$plaintext$vault_client_secret' public: false authorization_policy: two_factor redirect_uris: diff --git a/docs/content/en/integration/openid-connect/komga/index.md b/docs/content/en/integration/openid-connect/komga/index.md index cf4cf2370..1264b2c6b 100644 --- a/docs/content/en/integration/openid-connect/komga/index.md +++ b/docs/content/en/integration/openid-connect/komga/index.md @@ -71,7 +71,7 @@ which will operate with the above example: ```yaml - id: komga description: Komga - secret: komga_client_secret + secret: '$plaintext$komga_client_secret' public: false authorization_policy: two_factor redirect_uris: diff --git a/docs/content/en/integration/openid-connect/nextcloud/index.md b/docs/content/en/integration/openid-connect/nextcloud/index.md index dbbf81710..b5b933702 100644 --- a/docs/content/en/integration/openid-connect/nextcloud/index.md +++ b/docs/content/en/integration/openid-connect/nextcloud/index.md @@ -87,7 +87,7 @@ which will operate with the above example: ```yaml - id: nextcloud description: NextCloud - secret: nextcloud_client_secret + secret: '$plaintext$nextcloud_client_secret' public: false authorization_policy: two_factor redirect_uris: diff --git a/docs/content/en/integration/openid-connect/outline/index.md b/docs/content/en/integration/openid-connect/outline/index.md index be37bde99..c49beb489 100644 --- a/docs/content/en/integration/openid-connect/outline/index.md +++ b/docs/content/en/integration/openid-connect/outline/index.md @@ -66,7 +66,7 @@ which will operate with the above example: ```yaml - id: outline description: Outline - secret: outline_client_secret + secret: '$plaintext$outline_client_secret' public: false authorization_policy: two_factor redirect_uris: diff --git a/docs/content/en/integration/openid-connect/portainer/index.md b/docs/content/en/integration/openid-connect/portainer/index.md index 7e9c6afd4..a90a98443 100644 --- a/docs/content/en/integration/openid-connect/portainer/index.md +++ b/docs/content/en/integration/openid-connect/portainer/index.md @@ -67,7 +67,7 @@ which will operate with the above example: ```yaml - id: portainer description: Portainer - secret: portainer_client_secret + secret: '$plaintext$portainer_client_secret' public: false authorization_policy: two_factor redirect_uris: diff --git a/docs/content/en/integration/openid-connect/proxmox/index.md b/docs/content/en/integration/openid-connect/proxmox/index.md index 964eb8496..70c6f70cc 100644 --- a/docs/content/en/integration/openid-connect/proxmox/index.md +++ b/docs/content/en/integration/openid-connect/proxmox/index.md @@ -69,7 +69,7 @@ which will operate with the above example: ```yaml - id: proxmox description: Proxmox - secret: proxmox_client_secret + secret: '$plaintext$proxmox_client_secret' public: false authorization_policy: two_factor redirect_uris: diff --git a/docs/content/en/integration/openid-connect/seafile/index.md b/docs/content/en/integration/openid-connect/seafile/index.md index eb722175e..740af8d1d 100644 --- a/docs/content/en/integration/openid-connect/seafile/index.md +++ b/docs/content/en/integration/openid-connect/seafile/index.md @@ -75,7 +75,7 @@ which will operate with the above example: ```yaml - id: seafile description: Seafile - secret: seafile_client_secret + secret: '$plaintext$seafile_client_secret' public: false authorization_policy: two_factor redirect_uris: diff --git a/docs/content/en/integration/openid-connect/synapse/index.md b/docs/content/en/integration/openid-connect/synapse/index.md index bbdac2740..8dc751d49 100644 --- a/docs/content/en/integration/openid-connect/synapse/index.md +++ b/docs/content/en/integration/openid-connect/synapse/index.md @@ -69,7 +69,7 @@ which will operate with the above example: ```yaml - id: synapse description: Synapse - secret: synapse_client_secret + secret: '$plaintext$synapse_client_secret' public: false authorization_policy: two_factor redirect_uris: diff --git a/docs/content/en/reference/guides/rule-operators.md b/docs/content/en/reference/guides/rule-operators.md index 4c0a68086..7bd4bff85 100644 --- a/docs/content/en/reference/guides/rule-operators.md +++ b/docs/content/en/reference/guides/rule-operators.md @@ -2,7 +2,7 @@ title: "Access Control Rule Guide" description: "A reference guide on access control rule operators" lead: "This section contains a reference guide on access control rule operators." -date: 2022-09-09T15:44:23+10:00 +date: 2022-10-19T14:09:22+11:00 draft: false images: [] menu: diff --git a/internal/configuration/decode_hooks.go b/internal/configuration/decode_hooks.go index 3dca0409f..3fa27396e 100644 --- a/internal/configuration/decode_hooks.go +++ b/internal/configuration/decode_hooks.go @@ -9,8 +9,10 @@ import ( "net/url" "reflect" "regexp" + "strings" "time" + "github.com/go-crypt/crypt" "github.com/mitchellh/mapstructure" "github.com/authelia/authelia/v4/internal/configuration/schema" @@ -412,3 +414,53 @@ func StringToPrivateKeyHookFunc() mapstructure.DecodeHookFuncType { } } } + +// StringToPasswordDigestHookFunc decodes a string into a crypt.Digest. +func StringToPasswordDigestHookFunc(plaintext bool) mapstructure.DecodeHookFuncType { + return func(f reflect.Type, t reflect.Type, data interface{}) (value interface{}, err error) { + var ptr bool + + if f.Kind() != reflect.String { + return data, nil + } + + prefixType := "" + + if t.Kind() == reflect.Ptr { + ptr = true + prefixType = "*" + } + + expectedType := reflect.TypeOf(schema.PasswordDigest{}) + + if ptr && t.Elem() != expectedType { + return data, nil + } else if !ptr && t != expectedType { + return data, nil + } + + dataStr := data.(string) + + var result *schema.PasswordDigest + + if !strings.HasPrefix(dataStr, "$") { + dataStr = fmt.Sprintf(crypt.StorageFormatSimple, crypt.AlgorithmPrefixPlainText, dataStr) + } + + if dataStr != "" { + if result, err = schema.NewPasswordDigest(dataStr, plaintext); err != nil { + return nil, fmt.Errorf(errFmtDecodeHookCouldNotParse, dataStr, prefixType, expectedType.String(), err) + } + } + + if ptr { + return result, nil + } + + if result == nil { + return nil, fmt.Errorf(errFmtDecodeHookCouldNotParseEmptyValue, prefixType, expectedType.String(), errDecodeNonPtrMustHaveValue) + } + + return *result, nil + } +} diff --git a/internal/configuration/provider.go b/internal/configuration/provider.go index 679589c12..91476ff4b 100644 --- a/internal/configuration/provider.go +++ b/internal/configuration/provider.go @@ -64,6 +64,7 @@ func unmarshal(ko *koanf.Koanf, val *schema.StructValidator, path string, o any) StringToX509CertificateHookFunc(), StringToX509CertificateChainHookFunc(), StringToPrivateKeyHookFunc(), + StringToPasswordDigestHookFunc(true), ToTimeDurationHookFunc(), ), Metadata: nil, diff --git a/internal/configuration/schema/identity_providers.go b/internal/configuration/schema/identity_providers.go index 916dbd6b7..04c6f35f9 100644 --- a/internal/configuration/schema/identity_providers.go +++ b/internal/configuration/schema/identity_providers.go @@ -43,11 +43,11 @@ type OpenIDConnectCORSConfiguration struct { // OpenIDConnectClientConfiguration configuration for an OpenID Connect client. type OpenIDConnectClientConfiguration struct { - ID string `koanf:"id"` - Description string `koanf:"description"` - Secret string `koanf:"secret"` - SectorIdentifier url.URL `koanf:"sector_identifier"` - Public bool `koanf:"public"` + ID string `koanf:"id"` + Description string `koanf:"description"` + Secret *PasswordDigest `koanf:"secret"` + SectorIdentifier url.URL `koanf:"sector_identifier"` + Public bool `koanf:"public"` RedirectURIs []string `koanf:"redirect_uris"` diff --git a/internal/configuration/schema/types.go b/internal/configuration/schema/types.go index 6ac00a7a5..e421d7eb8 100644 --- a/internal/configuration/schema/types.go +++ b/internal/configuration/schema/types.go @@ -13,6 +13,8 @@ import ( "strconv" "strings" "time" + + "github.com/go-crypt/crypt" ) // NewAddressFromString returns an *Address and error depending on the ability to parse the string as an Address. @@ -107,6 +109,29 @@ func (a Address) Listener() (net.Listener, error) { return net.Listen(a.Scheme, a.HostPort()) } +// NewPasswordDigest returns a new PasswordDigest. +func NewPasswordDigest(value string, plaintext bool) (digest *PasswordDigest, err error) { + var d crypt.Digest + + switch { + case plaintext: + d, err = crypt.DecodeWithPlainText(value) + default: + d, err = crypt.Decode(value) + } + + if err != nil { + return nil, err + } + + return &PasswordDigest{d}, err +} + +// PasswordDigest is a configuration type for the crypt.Digest. +type PasswordDigest struct { + crypt.Digest +} + // 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/test_resources/config_oidc.yml b/internal/configuration/test_resources/config_oidc.yml index 840894586..94a76e0bb 100644 --- a/internal/configuration/test_resources/config_oidc.yml +++ b/internal/configuration/test_resources/config_oidc.yml @@ -132,6 +132,6 @@ identity_providers: - https://example.com clients: - id: abc - secret: 123 + secret: '123' consent_mode: explicit ... diff --git a/internal/configuration/validator/identity_providers.go b/internal/configuration/validator/identity_providers.go index bd1fcacaa..a2ee5a549 100644 --- a/internal/configuration/validator/identity_providers.go +++ b/internal/configuration/validator/identity_providers.go @@ -161,11 +161,11 @@ func validateOIDCClients(config *schema.OpenIDConnectConfiguration, validator *s } if client.Public { - if client.Secret != "" { + if client.Secret != nil { validator.Push(fmt.Errorf(errFmtOIDCClientPublicInvalidSecret, client.ID)) } } else { - if client.Secret == "" { + if client.Secret == nil { validator.Push(fmt.Errorf(errFmtOIDCClientInvalidSecret, client.ID)) } } diff --git a/internal/configuration/validator/identity_providers_test.go b/internal/configuration/validator/identity_providers_test.go index eb913a279..23eca844a 100644 --- a/internal/configuration/validator/identity_providers_test.go +++ b/internal/configuration/validator/identity_providers_test.go @@ -46,7 +46,7 @@ func TestShouldNotRaiseErrorWhenCORSEndpointsValid(t *testing.T) { Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "example", - Secret: "example", + Secret: MustDecodeSecret("$plaintext$example"), }, }, }, @@ -69,7 +69,7 @@ func TestShouldRaiseErrorWhenCORSEndpointsNotValid(t *testing.T) { Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "example", - Secret: "example", + Secret: MustDecodeSecret("$plaintext$example"), }, }, }, @@ -114,7 +114,7 @@ func TestShouldRaiseErrorWhenOIDCCORSOriginsHasInvalidValues(t *testing.T) { Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "myclient", - Secret: "jk12nb3klqwmnelqkwenm", + Secret: MustDecodeSecret("$plaintext$jk12nb3klqwmnelqkwenm"), Policy: "two_factor", RedirectURIs: []string{"https://example.com/oauth2_callback", "https://localhost:566/callback", "http://an.example.com/callback", "file://a/file"}, }, @@ -173,7 +173,7 @@ func TestShouldRaiseErrorWhenOIDCServerClientBadValues(t *testing.T) { Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "", - Secret: "", + Secret: nil, Policy: "", RedirectURIs: []string{}, }, @@ -188,7 +188,7 @@ func TestShouldRaiseErrorWhenOIDCServerClientBadValues(t *testing.T) { Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "client-1", - Secret: "a-secret", + Secret: MustDecodeSecret("$plaintext$a-secret"), Policy: "a-policy", RedirectURIs: []string{ "https://google.com", @@ -202,13 +202,13 @@ func TestShouldRaiseErrorWhenOIDCServerClientBadValues(t *testing.T) { Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "client-x", - Secret: "a-secret", + Secret: MustDecodeSecret("$plaintext$a-secret"), Policy: policyTwoFactor, RedirectURIs: []string{}, }, { ID: "client-x", - Secret: "a-secret", + Secret: MustDecodeSecret("$plaintext$a-secret"), Policy: policyTwoFactor, RedirectURIs: []string{}, }, @@ -220,7 +220,7 @@ func TestShouldRaiseErrorWhenOIDCServerClientBadValues(t *testing.T) { Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "client-check-uri-parse", - Secret: "a-secret", + Secret: MustDecodeSecret("$plaintext$a-secret"), Policy: policyTwoFactor, RedirectURIs: []string{ "http://abc@%two", @@ -236,7 +236,7 @@ func TestShouldRaiseErrorWhenOIDCServerClientBadValues(t *testing.T) { Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "client-check-uri-abs", - Secret: "a-secret", + Secret: MustDecodeSecret("$plaintext$a-secret"), Policy: policyTwoFactor, RedirectURIs: []string{ "google.com", @@ -252,7 +252,7 @@ func TestShouldRaiseErrorWhenOIDCServerClientBadValues(t *testing.T) { Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "client-valid-sector", - Secret: "a-secret", + Secret: MustDecodeSecret("$plaintext$a-secret"), Policy: policyTwoFactor, RedirectURIs: []string{ "https://google.com", @@ -266,7 +266,7 @@ func TestShouldRaiseErrorWhenOIDCServerClientBadValues(t *testing.T) { Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "client-valid-sector", - Secret: "a-secret", + Secret: MustDecodeSecret("$plaintext$a-secret"), Policy: policyTwoFactor, RedirectURIs: []string{ "https://google.com", @@ -280,7 +280,7 @@ func TestShouldRaiseErrorWhenOIDCServerClientBadValues(t *testing.T) { Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "client-invalid-sector", - Secret: "a-secret", + Secret: MustDecodeSecret("$plaintext$a-secret"), Policy: policyTwoFactor, RedirectURIs: []string{ "https://google.com", @@ -302,7 +302,7 @@ func TestShouldRaiseErrorWhenOIDCServerClientBadValues(t *testing.T) { Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "client-invalid-sector", - Secret: "a-secret", + Secret: MustDecodeSecret("$plaintext$a-secret"), Policy: policyTwoFactor, RedirectURIs: []string{ "https://google.com", @@ -350,7 +350,7 @@ func TestShouldRaiseErrorWhenOIDCClientConfiguredWithBadScopes(t *testing.T) { Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "good_id", - Secret: "good_secret", + Secret: MustDecodeSecret("$plaintext$good_secret"), Policy: "two_factor", Scopes: []string{"openid", "bad_scope"}, RedirectURIs: []string{ @@ -376,7 +376,7 @@ func TestShouldRaiseErrorWhenOIDCClientConfiguredWithBadGrantTypes(t *testing.T) Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "good_id", - Secret: "good_secret", + Secret: MustDecodeSecret("$plaintext$good_secret"), Policy: "two_factor", GrantTypes: []string{"bad_grant_type"}, RedirectURIs: []string{ @@ -403,7 +403,7 @@ func TestShouldNotErrorOnCertificateValid(t *testing.T) { Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "good_id", - Secret: "good_secret", + Secret: MustDecodeSecret("$plaintext$good_secret"), Policy: "two_factor", RedirectURIs: []string{ "https://google.com/callback", @@ -429,7 +429,7 @@ func TestShouldRaiseErrorOnCertificateNotValid(t *testing.T) { Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "good_id", - Secret: "good_secret", + Secret: MustDecodeSecret("$plaintext$good_secret"), Policy: "two_factor", RedirectURIs: []string{ "https://google.com/callback", @@ -456,7 +456,7 @@ func TestShouldRaiseErrorOnKeySizeTooSmall(t *testing.T) { Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "good_id", - Secret: "good_secret", + Secret: MustDecodeSecret("$plaintext$good_secret"), Policy: "two_factor", RedirectURIs: []string{ "https://google.com/callback", @@ -483,7 +483,7 @@ func TestShouldRaiseErrorWhenOIDCClientConfiguredWithBadResponseModes(t *testing Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "good_id", - Secret: "good_secret", + Secret: MustDecodeSecret("$plaintext$good_secret"), Policy: "two_factor", ResponseModes: []string{"bad_responsemode"}, RedirectURIs: []string{ @@ -509,7 +509,7 @@ func TestShouldRaiseErrorWhenOIDCClientConfiguredWithBadUserinfoAlg(t *testing.T Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "good_id", - Secret: "good_secret", + Secret: MustDecodeSecret("$plaintext$good_secret"), Policy: "two_factor", UserinfoSigningAlgorithm: "rs256", RedirectURIs: []string{ @@ -536,7 +536,7 @@ func TestValidateIdentityProvidersShouldRaiseWarningOnSecurityIssue(t *testing.T Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "good_id", - Secret: "good_secret", + Secret: MustDecodeSecret("$plaintext$good_secret"), Policy: "two_factor", RedirectURIs: []string{ "https://google.com/callback", @@ -563,7 +563,7 @@ func TestValidateIdentityProvidersShouldRaiseErrorsOnInvalidClientTypes(t *testi Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "client-with-invalid-secret", - Secret: "a-secret", + Secret: MustDecodeSecret("$plaintext$a-secret"), Public: true, Policy: "two_factor", RedirectURIs: []string{ @@ -572,7 +572,7 @@ func TestValidateIdentityProvidersShouldRaiseErrorsOnInvalidClientTypes(t *testi }, { ID: "client-with-bad-redirect-uri", - Secret: "a-secret", + Secret: MustDecodeSecret("$plaintext$a-secret"), Public: false, Policy: "two_factor", RedirectURIs: []string{ @@ -642,7 +642,7 @@ func TestValidateIdentityProvidersShouldSetDefaultValues(t *testing.T) { Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "a-client", - Secret: "a-client-secret", + Secret: MustDecodeSecret("$plaintext$a-client-secret"), RedirectURIs: []string{ "https://google.com", }, @@ -650,7 +650,7 @@ func TestValidateIdentityProvidersShouldSetDefaultValues(t *testing.T) { { ID: "b-client", Description: "Normal Description", - Secret: "b-client-secret", + Secret: MustDecodeSecret("$plaintext$b-client-secret"), Policy: policyOneFactor, UserinfoSigningAlgorithm: "RS256", RedirectURIs: []string{ @@ -775,6 +775,14 @@ func TestValidateOIDCClientRedirectURIsSupportingPrivateUseURISchemes(t *testing }) } +func MustDecodeSecret(value string) *schema.PasswordDigest { + if secret, err := schema.NewPasswordDigest(value, true); err != nil { + panic(err) + } else { + return secret + } +} + func MustParseRSAPrivateKey(data string) *rsa.PrivateKey { block, _ := pem.Decode([]byte(data)) if block == nil || block.Bytes == nil || len(block.Bytes) == 0 { diff --git a/internal/oidc/client.go b/internal/oidc/client.go index 7bdbc8a36..e256a9380 100644 --- a/internal/oidc/client.go +++ b/internal/oidc/client.go @@ -14,7 +14,7 @@ func NewClient(config schema.OpenIDConnectClientConfiguration) (client *Client) client = &Client{ ID: config.ID, Description: config.Description, - Secret: []byte(config.Secret), + Secret: config.Secret, SectorIdentifier: config.SectorIdentifier.String(), Public: config.Public, @@ -76,7 +76,11 @@ func (c *Client) GetConsentResponseBody(consent *model.OAuth2ConsentSession) Con // GetHashedSecret returns the Secret. func (c *Client) GetHashedSecret() []byte { - return c.Secret + if c.Secret == nil { + return []byte(nil) + } + + return []byte(c.Secret.Encode()) } // GetRedirectURIs returns the RedirectURIs. diff --git a/internal/oidc/client_test.go b/internal/oidc/client_test.go index 8483efa5c..01722d21a 100644 --- a/internal/oidc/client_test.go +++ b/internal/oidc/client_test.go @@ -26,7 +26,7 @@ func TestNewClient(t *testing.T) { ID: "myapp", Description: "My App", Policy: "two_factor", - Secret: "abcdef", + Secret: MustDecodeSecret("$plaintext$abcdef"), RedirectURIs: []string{"https://google.com/callback"}, Scopes: schema.DefaultOpenIDConnectClientConfiguration.Scopes, ResponseTypes: schema.DefaultOpenIDConnectClientConfiguration.ResponseTypes, @@ -68,7 +68,7 @@ func TestIsAuthenticationLevelSufficient(t *testing.T) { assert.False(t, c.IsAuthenticationLevelSufficient(authentication.TwoFactor)) } -func TestInternalClient_GetConsentResponseBody(t *testing.T) { +func TestClient_GetConsentResponseBody(t *testing.T) { c := Client{} consentRequestBody := c.GetConsentResponseBody(nil) @@ -95,7 +95,7 @@ func TestInternalClient_GetConsentResponseBody(t *testing.T) { assert.Equal(t, expectedAudiences, consentRequestBody.Audience) } -func TestInternalClient_GetAudience(t *testing.T) { +func TestClient_GetAudience(t *testing.T) { c := Client{} audience := c.GetAudience() @@ -108,7 +108,7 @@ func TestInternalClient_GetAudience(t *testing.T) { assert.Equal(t, "https://example.com", audience[0]) } -func TestInternalClient_GetScopes(t *testing.T) { +func TestClient_GetScopes(t *testing.T) { c := Client{} scopes := c.GetScopes() @@ -121,7 +121,7 @@ func TestInternalClient_GetScopes(t *testing.T) { assert.Equal(t, "openid", scopes[0]) } -func TestInternalClient_GetGrantTypes(t *testing.T) { +func TestClient_GetGrantTypes(t *testing.T) { c := Client{} grantTypes := c.GetGrantTypes() @@ -135,19 +135,30 @@ func TestInternalClient_GetGrantTypes(t *testing.T) { assert.Equal(t, "device_code", grantTypes[0]) } -func TestInternalClient_GetHashedSecret(t *testing.T) { +func TestClient_Hashing(t *testing.T) { c := Client{} hashedSecret := c.GetHashedSecret() assert.Equal(t, []byte(nil), hashedSecret) - c.Secret = []byte("a_bad_secret") + c.Secret = MustDecodeSecret("$plaintext$a_bad_secret") - hashedSecret = c.GetHashedSecret() - assert.Equal(t, []byte("a_bad_secret"), hashedSecret) + assert.True(t, c.Secret.MatchBytes([]byte("a_bad_secret"))) } -func TestInternalClient_GetID(t *testing.T) { +func TestClient_GetHashedSecret(t *testing.T) { + c := Client{} + + hashedSecret := c.GetHashedSecret() + assert.Equal(t, []byte(nil), hashedSecret) + + c.Secret = MustDecodeSecret("$plaintext$a_bad_secret") + + hashedSecret = c.GetHashedSecret() + assert.Equal(t, []byte("$plaintext$a_bad_secret"), hashedSecret) +} + +func TestClient_GetID(t *testing.T) { c := Client{} id := c.GetID() @@ -159,7 +170,7 @@ func TestInternalClient_GetID(t *testing.T) { assert.Equal(t, "myid", id) } -func TestInternalClient_GetRedirectURIs(t *testing.T) { +func TestClient_GetRedirectURIs(t *testing.T) { c := Client{} redirectURIs := c.GetRedirectURIs() @@ -172,7 +183,7 @@ func TestInternalClient_GetRedirectURIs(t *testing.T) { assert.Equal(t, "https://example.com/oauth2/callback", redirectURIs[0]) } -func TestInternalClient_GetResponseModes(t *testing.T) { +func TestClient_GetResponseModes(t *testing.T) { c := Client{} responseModes := c.GetResponseModes() @@ -191,7 +202,7 @@ func TestInternalClient_GetResponseModes(t *testing.T) { assert.Equal(t, fosite.ResponseModeFragment, responseModes[3]) } -func TestInternalClient_GetResponseTypes(t *testing.T) { +func TestClient_GetResponseTypes(t *testing.T) { c := Client{} responseTypes := c.GetResponseTypes() @@ -206,7 +217,7 @@ func TestInternalClient_GetResponseTypes(t *testing.T) { assert.Equal(t, "id_token", responseTypes[1]) } -func TestInternalClient_IsPublic(t *testing.T) { +func TestClient_IsPublic(t *testing.T) { c := Client{} assert.False(t, c.IsPublic()) @@ -214,3 +225,11 @@ func TestInternalClient_IsPublic(t *testing.T) { c.Public = true assert.True(t, c.IsPublic()) } + +func MustDecodeSecret(value string) *schema.PasswordDigest { + if secret, err := schema.NewPasswordDigest(value, true); err != nil { + panic(err) + } else { + return secret + } +} diff --git a/internal/oidc/hasher.go b/internal/oidc/hasher.go index 0bacbe98f..7aad32b25 100644 --- a/internal/oidc/hasher.go +++ b/internal/oidc/hasher.go @@ -2,19 +2,26 @@ package oidc import ( "context" - "crypto/subtle" + + "github.com/go-crypt/crypt" ) // Compare compares the hash with the data and returns an error if they don't match. -func (h PlainTextHasher) Compare(_ context.Context, hash, data []byte) (err error) { - if subtle.ConstantTimeCompare(hash, data) == 0 { - return errPasswordsDoNotMatch +func (h AdaptiveHasher) Compare(_ context.Context, hash, data []byte) (err error) { + var digest crypt.Digest + + if digest, err = crypt.DecodeWithPlainText(string(hash)); err != nil { + return err } - return nil + if digest.MatchBytes(data) { + return nil + } + + return errPasswordsDoNotMatch } // Hash creates a new hash from data. -func (h PlainTextHasher) Hash(_ context.Context, data []byte) (hash []byte, err error) { +func (h AdaptiveHasher) Hash(_ context.Context, data []byte) (hash []byte, err error) { return data, nil } diff --git a/internal/oidc/hasher_test.go b/internal/oidc/hasher_test.go index a86123bf4..bc3dfac1b 100644 --- a/internal/oidc/hasher_test.go +++ b/internal/oidc/hasher_test.go @@ -8,9 +8,9 @@ import ( ) func TestShouldNotRaiseErrorOnEqualPasswordsPlainText(t *testing.T) { - hasher := PlainTextHasher{} + hasher := AdaptiveHasher{} - a := []byte("abc") + a := []byte("$plaintext$abc") b := []byte("abc") ctx := context.Background() @@ -21,9 +21,9 @@ func TestShouldNotRaiseErrorOnEqualPasswordsPlainText(t *testing.T) { } func TestShouldRaiseErrorOnNonEqualPasswordsPlainText(t *testing.T) { - hasher := PlainTextHasher{} + hasher := AdaptiveHasher{} - a := []byte("abc") + a := []byte("$plaintext$abc") b := []byte("abcd") ctx := context.Background() @@ -34,7 +34,7 @@ func TestShouldRaiseErrorOnNonEqualPasswordsPlainText(t *testing.T) { } func TestShouldHashPassword(t *testing.T) { - hasher := PlainTextHasher{} + hasher := AdaptiveHasher{} data := []byte("abc") diff --git a/internal/oidc/provider.go b/internal/oidc/provider.go index 8843a6807..9273df318 100644 --- a/internal/oidc/provider.go +++ b/internal/oidc/provider.go @@ -69,7 +69,7 @@ func NewOpenIDConnectProvider(config *schema.OpenIDConnectConfiguration, store s cconfig, provider.Store, strategy, - PlainTextHasher{}, + AdaptiveHasher{}, /* These are the OAuth2 and OpenIDConnect factories. Order is important (the OAuth2 factories at the top must diff --git a/internal/oidc/provider_test.go b/internal/oidc/provider_test.go index 6d0c3ecef..e4a486eb5 100644 --- a/internal/oidc/provider_test.go +++ b/internal/oidc/provider_test.go @@ -31,7 +31,7 @@ func TestNewOpenIDConnectProvider_ShouldEnableOptionalDiscoveryValues(t *testing Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "a-client", - Secret: "a-client-secret", + Secret: MustDecodeSecret("$plaintext$a-client-secret"), SectorIdentifier: url.URL{Host: "google.com"}, Policy: "one_factor", RedirectURIs: []string{ @@ -62,7 +62,7 @@ func TestOpenIDConnectProvider_NewOpenIDConnectProvider_GoodConfiguration(t *tes Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "a-client", - Secret: "a-client-secret", + Secret: MustDecodeSecret("$plaintext$a-client-secret"), Policy: "one_factor", RedirectURIs: []string{ "https://google.com", @@ -71,7 +71,7 @@ func TestOpenIDConnectProvider_NewOpenIDConnectProvider_GoodConfiguration(t *tes { ID: "b-client", Description: "Normal Description", - Secret: "b-client-secret", + Secret: MustDecodeSecret("$plaintext$b-client-secret"), Policy: "two_factor", RedirectURIs: []string{ "https://google.com", @@ -102,7 +102,7 @@ func TestOpenIDConnectProvider_NewOpenIDConnectProvider_GetOpenIDConnectWellKnow Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "a-client", - Secret: "a-client-secret", + Secret: MustDecodeSecret("$plaintext$a-client-secret"), Policy: "one_factor", RedirectURIs: []string{ "https://google.com", @@ -192,7 +192,7 @@ func TestOpenIDConnectProvider_NewOpenIDConnectProvider_GetOAuth2WellKnownConfig Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "a-client", - Secret: "a-client-secret", + Secret: MustDecodeSecret("$plaintext$a-client-secret"), Policy: "one_factor", RedirectURIs: []string{ "https://google.com", @@ -271,7 +271,7 @@ func TestOpenIDConnectProvider_NewOpenIDConnectProvider_GetOpenIDConnectWellKnow Clients: []schema.OpenIDConnectClientConfiguration{ { ID: "a-client", - Secret: "a-client-secret", + Secret: MustDecodeSecret("$plaintext$a-client-secret"), Policy: "one_factor", RedirectURIs: []string{ "https://google.com", diff --git a/internal/oidc/store_test.go b/internal/oidc/store_test.go index 08cd7ae25..adba95453 100644 --- a/internal/oidc/store_test.go +++ b/internal/oidc/store_test.go @@ -21,14 +21,14 @@ func TestOpenIDConnectStore_GetClientPolicy(t *testing.T) { Description: "myclient desc", Policy: "one_factor", Scopes: []string{ScopeOpenID, ScopeProfile}, - Secret: "mysecret", + Secret: MustDecodeSecret("$plaintext$mysecret"), }, { ID: "myotherclient", Description: "myclient desc", Policy: "two_factor", Scopes: []string{ScopeOpenID, ScopeProfile}, - Secret: "mysecret", + Secret: MustDecodeSecret("$plaintext$mysecret"), }, }, }, nil) @@ -53,7 +53,7 @@ func TestOpenIDConnectStore_GetInternalClient(t *testing.T) { Description: "myclient desc", Policy: "one_factor", Scopes: []string{ScopeOpenID, ScopeProfile}, - Secret: "mysecret", + Secret: MustDecodeSecret("$plaintext$mysecret"), }, }, }, nil) @@ -74,7 +74,7 @@ func TestOpenIDConnectStore_GetInternalClient_ValidClient(t *testing.T) { Description: "myclient desc", Policy: "one_factor", Scopes: []string{ScopeOpenID, ScopeProfile}, - Secret: "mysecret", + Secret: MustDecodeSecret("$plaintext$mysecret"), } s := NewOpenIDConnectStore(&schema.OpenIDConnectConfiguration{ @@ -93,7 +93,7 @@ func TestOpenIDConnectStore_GetInternalClient_ValidClient(t *testing.T) { assert.Equal(t, client.ResponseTypes, c1.ResponseTypes) assert.Equal(t, client.RedirectURIs, c1.RedirectURIs) assert.Equal(t, client.Policy, authorization.OneFactor) - assert.Equal(t, client.Secret, []byte(c1.Secret)) + assert.Equal(t, client.Secret.Encode(), "$plaintext$mysecret") } func TestOpenIDConnectStore_GetInternalClient_InvalidClient(t *testing.T) { @@ -102,7 +102,7 @@ func TestOpenIDConnectStore_GetInternalClient_InvalidClient(t *testing.T) { Description: "myclient desc", Policy: "one_factor", Scopes: []string{ScopeOpenID, ScopeProfile}, - Secret: "mysecret", + Secret: MustDecodeSecret("$plaintext$mysecret"), } s := NewOpenIDConnectStore(&schema.OpenIDConnectConfiguration{ @@ -126,7 +126,7 @@ func TestOpenIDConnectStore_IsValidClientID(t *testing.T) { Description: "myclient desc", Policy: "one_factor", Scopes: []string{ScopeOpenID, ScopeProfile}, - Secret: "mysecret", + Secret: MustDecodeSecret("$plaintext$mysecret"), }, }, }, nil) diff --git a/internal/oidc/types.go b/internal/oidc/types.go index d48b023bf..97d84542a 100644 --- a/internal/oidc/types.go +++ b/internal/oidc/types.go @@ -4,6 +4,7 @@ import ( "net/url" "time" + "github.com/go-crypt/crypt" "github.com/ory/fosite" "github.com/ory/fosite/handler/openid" "github.com/ory/fosite/token/jwt" @@ -101,7 +102,7 @@ type Store struct { type Client struct { ID string Description string - Secret []byte + Secret crypt.Digest SectorIdentifier string Public bool @@ -182,8 +183,8 @@ type KeyManager struct { jwks *jose.JSONWebKeySet } -// PlainTextHasher implements the fosite.Hasher interface without an actual hashing algo. -type PlainTextHasher struct{} +// AdaptiveHasher implements the fosite.Hasher interface without an actual hashing algo. +type AdaptiveHasher struct{} // ConsentGetResponseBody schema of the response body of the consent GET endpoint. type ConsentGetResponseBody struct {