From b7990875c124d7f8914162f7aaa3003dc05ef095 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Thu, 27 Apr 2023 10:34:09 +0200 Subject: [PATCH] oidc: Remove "aud" claim validation of logout tokens (#6156) The "aud" claim of the logout token is supposed to contain the client-id of the client for which the token was issued. Our current implementation of validating that claim is somewhat broken. We only allow to configure a single value for the allowed client id. But we have different client-ids accessing oCIS. This completely removes the current validation of the `aud` claim until we come up with a working solution. As we currently require a session id to be present in the logout token the risk not validating the `aud` claim is pretty low. Related: #6149 --- ocis-pkg/oidc/client.go | 17 ----- ocis-pkg/oidc/client_test.go | 72 +------------------ ocis-pkg/oidc/options.go | 19 ----- services/proxy/pkg/command/server.go | 2 - services/proxy/pkg/config/config.go | 2 - .../pkg/config/defaults/defaultconfig.go | 1 - 6 files changed, 2 insertions(+), 111 deletions(-) diff --git a/ocis-pkg/oidc/client.go b/ocis-pkg/oidc/client.go index 7ecdebb36..f4fab46ee 100644 --- a/ocis-pkg/oidc/client.go +++ b/ocis-pkg/oidc/client.go @@ -55,8 +55,6 @@ type oidcClient struct { // Logger to use for logging, must be set Logger log.Logger - clientID string - skipClientIDCheck bool issuer string provider *ProviderMetadata providerLock *sync.Mutex @@ -99,8 +97,6 @@ func NewOIDCClient(opts ...Option) OIDCClient { JWKSOptions: options.JWKSOptions, // TODO I don't like that we pass down config options ... providerLock: &sync.Mutex{}, jwksLock: &sync.Mutex{}, - clientID: options.ClientID, - skipClientIDCheck: options.SkipClientIDCheck, remoteKeySet: options.KeySet, provider: options.ProviderMetadata, } @@ -363,19 +359,6 @@ func (c *oidcClient) VerifyLogoutToken(ctx context.Context, rawToken string) (*L return nil, fmt.Errorf("oidc: logout token issued by a different provider, expected %q got %q", c.issuer, token.Issuer) } - // If a client ID has been provided, make sure it's part of the audience. SkipClientIDCheck must be true if ClientID is empty. - // - // This check DOES NOT ensure that the ClientID is the party to which the ID Token was issued (i.e. Authorized party). - if !c.skipClientIDCheck { - if c.clientID != "" { - if !contains(token.Audience, c.clientID) { - return nil, fmt.Errorf("oidc: expected audience %q got %q", c.clientID, token.Audience) - } - } else { - return nil, fmt.Errorf("oidc: invalid configuration, clientID must be provided or SkipClientIDCheck must be set") - } - } - switch len(jws.Signatures) { case 0: return nil, fmt.Errorf("oidc: logout token not signed") diff --git a/ocis-pkg/oidc/client_test.go b/ocis-pkg/oidc/client_test.go index e33dbf0d7..93a2fb468 100644 --- a/ocis-pkg/oidc/client_test.go +++ b/ocis-pkg/oidc/client_test.go @@ -60,9 +60,6 @@ func TestLogoutVerify(t *testing.T) { "http://schemas.openid.net/event/backchannel-logout": {} } }`, - config: goidc.Config{ - SkipClientIDCheck: true, - }, signKey: newRSAKey(t), }, { @@ -70,8 +67,7 @@ func TestLogoutVerify(t *testing.T) { issuer: "https://bar", logoutToken: `{"iss":"https://foo"}`, config: goidc.Config{ - SkipClientIDCheck: true, - SkipExpiryCheck: true, + SkipExpiryCheck: true, }, signKey: newRSAKey(t), wantErr: true, @@ -90,8 +86,7 @@ func TestLogoutVerify(t *testing.T) { } }`, config: goidc.Config{ - SkipClientIDCheck: true, - SkipExpiryCheck: true, + SkipExpiryCheck: true, }, signKey: newRSAKey(t), verificationKey: newRSAKey(t), @@ -108,9 +103,6 @@ func TestLogoutVerify(t *testing.T) { "http://schemas.openid.net/event/backchannel-logout": {} } }`, - config: goidc.Config{ - SkipClientIDCheck: true, - }, signKey: newRSAKey(t), wantErr: true, }, @@ -127,9 +119,6 @@ func TestLogoutVerify(t *testing.T) { "http://schemas.openid.net/event/backchannel-logout": {} } }`, - config: goidc.Config{ - SkipClientIDCheck: true, - }, signKey: newRSAKey(t), wantErr: true, }, @@ -146,9 +135,6 @@ func TestLogoutVerify(t *testing.T) { "not a logout event": {} } }`, - config: goidc.Config{ - SkipClientIDCheck: true, - }, signKey: newRSAKey(t), wantErr: true, }, @@ -162,9 +148,6 @@ func TestLogoutVerify(t *testing.T) { "jti": "bWJq", "sid": "08a5019c-17e1-4977-8f42-65a12843ea02", }`, - config: goidc.Config{ - SkipClientIDCheck: true, - }, signKey: newRSAKey(t), wantErr: true, }, @@ -174,49 +157,6 @@ func TestLogoutVerify(t *testing.T) { } } -func TestVerifyAudienceLogout(t *testing.T) { - tests := []logoutVerificationTest{ - { - name: "good audience", - logoutToken: `{"iss":"https://foo","aud":"client1","sub":"subject","events": { - "http://schemas.openid.net/event/backchannel-logout": {} - } - }`, - config: goidc.Config{ - ClientID: "client1", - SkipExpiryCheck: true, - }, - signKey: newRSAKey(t), - }, - { - name: "mismatched audience", - logoutToken: `{"iss":"https://foo","aud":"client2","sub":"subject","events": { - "http://schemas.openid.net/event/backchannel-logout": {} - }}`, - config: goidc.Config{ - ClientID: "client1", - SkipExpiryCheck: true, - }, - signKey: newRSAKey(t), - wantErr: true, - }, - { - name: "multiple audiences, one matches", - logoutToken: `{"iss":"https://foo","aud":["client1","client2"],"sub":"subject","events": { - "http://schemas.openid.net/event/backchannel-logout": {} - }}`, - config: goidc.Config{ - ClientID: "client2", - SkipExpiryCheck: true, - }, - signKey: newRSAKey(t), - }, - } - for _, test := range tests { - t.Run(test.name, test.run) - } -} - type logoutVerificationTest struct { // Name of the subtest. name string @@ -263,19 +203,11 @@ func (v logoutVerificationTest) runGetToken(t *testing.T) (*oidc.LogoutToken, er } pm := oidc.ProviderMetadata{} - var clientID string - switch t.Name() { - case "TestLogoutVerify/good_token": - clientID = "s6BhdRkqt3" - default: - clientID = "client1" - } verifier := oidc.NewOIDCClient( oidc.WithOidcIssuer(issuer), oidc.WithKeySet(ks), oidc.WithConfig(&v.config), oidc.WithProviderMetadata(&pm), - oidc.WithClientID(clientID), ) return verifier.VerifyLogoutToken(ctx, token) diff --git a/ocis-pkg/oidc/options.go b/ocis-pkg/oidc/options.go index 86439532a..149433eba 100644 --- a/ocis-pkg/oidc/options.go +++ b/ocis-pkg/oidc/options.go @@ -27,11 +27,6 @@ type Options struct { // AccessTokenVerifyMethod to use when verifying access tokens // TODO pass a function or interface to verify? an AccessTokenVerifier? AccessTokenVerifyMethod string - // ClientID the client id to expect in tokens. If not set SkipClientIDCheck must be true - // TODO also check in access token - ClientID string - // SkipClientIDCheck must be true if ClientID is empty - SkipClientIDCheck bool // Config to use Config *goidc.Config @@ -92,20 +87,6 @@ func WithKeySet(val KeySet) Option { } } -// WithClientID provides a function to set the clientID option. -func WithClientID(val string) Option { - return func(o *Options) { - o.ClientID = val - } -} - -// WithSkipClientIDCheck provides a function to set the skipClientIDCheck option. -func WithSkipClientIDCheck(val bool) Option { - return func(o *Options) { - o.SkipClientIDCheck = val - } -} - // WithConfig provides a function to set the Config option. func WithConfig(val *goidc.Config) Option { return func(o *Options) { diff --git a/services/proxy/pkg/command/server.go b/services/proxy/pkg/command/server.go index 3ed00b105..8e4cbbef3 100644 --- a/services/proxy/pkg/command/server.go +++ b/services/proxy/pkg/command/server.go @@ -87,8 +87,6 @@ func Server(cfg *config.Config) *cli.Command { oidc.WithHTTPClient(oidcHTTPClient), oidc.WithOidcIssuer(cfg.OIDC.Issuer), oidc.WithJWKSOptions(cfg.OIDC.JWKS), - oidc.WithClientID(cfg.OIDC.ClientID), - oidc.WithSkipClientIDCheck(cfg.OIDC.SkipClientIDCheck), ) var ( diff --git a/services/proxy/pkg/config/config.go b/services/proxy/pkg/config/config.go index 4ed7220e5..9bf50d2ee 100644 --- a/services/proxy/pkg/config/config.go +++ b/services/proxy/pkg/config/config.go @@ -108,8 +108,6 @@ type OIDC struct { UserinfoCache *Cache `yaml:"user_info_cache"` JWKS JWKS `yaml:"jwks"` RewriteWellKnown bool `yaml:"rewrite_well_known" env:"PROXY_OIDC_REWRITE_WELLKNOWN" desc:"Enables rewriting the /.well-known/openid-configuration to the configured OIDC issuer. Needed by the Desktop Client, Android Client and iOS Client to discover the OIDC provider."` - ClientID string `yaml:"client_id" env:"OCIS_OIDC_CLIENT_ID;PROXY_OIDC_CLIENT_ID" desc:"OIDC client ID, which ownCloud Web uses. This client needs to be set up in your external IDP (has no effect when using the builtin IDP)."` - SkipClientIDCheck bool `yaml:"skip_client_id_check" env:"PROXY_OIDC_SKIP_CLIENT_ID_CHECK" desc:"If true will skip checking the configured client ID is present in audience claims. See following chapter for more details: https://openid.net/specs/openid-connect-core-1_0.html#IDToken"` } type JWKS struct { diff --git a/services/proxy/pkg/config/defaults/defaultconfig.go b/services/proxy/pkg/config/defaults/defaultconfig.go index c8ac0e431..16ee92d71 100644 --- a/services/proxy/pkg/config/defaults/defaultconfig.go +++ b/services/proxy/pkg/config/defaults/defaultconfig.go @@ -53,7 +53,6 @@ func DefaultConfig() *config.Config { RefreshTimeout: 10, // seconds RefreshUnknownKID: true, }, - ClientID: "web", }, PolicySelector: nil, RoleAssignment: config.RoleAssignment{