mirror/opencloud
1
0
Fork 0
mirror of https://github.com/opencloud-eu/opencloud.git synced 2025-12-31 01:10:20 -06:00
Code Issues Packages Projects Releases Wiki Activity

oidc: Remove "aud" claim validation of logout tokens (#6156)

Browse Source 
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
This commit is contained in:
Ralf Haferkamp
2023-04-27 10:34:09 +02:00
committed by GitHub
parent 8d06b293b4
commit b7990875c1
6 changed files with 2 additions and 111 deletions
Download Patch File Download Diff File Expand all files Collapse all files

17
ocis-pkg/oidc/client.go
View File

@@ -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")

72
ocis-pkg/oidc/client_test.go
View File

@@ -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)

19
ocis-pkg/oidc/options.go
View File

@@ -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) {

2
services/proxy/pkg/command/server.go
View File

@@ -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 (

2
services/proxy/pkg/config/config.go
View File

@@ -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 {

1
services/proxy/pkg/config/defaults/defaultconfig.go
View File

@@ -53,7 +53,6 @@ func DefaultConfig() *config.Config {
RefreshTimeout: 10, // seconds
RefreshUnknownKID: true,
},
ClientID: "web",
},
PolicySelector: nil,
RoleAssignment: config.RoleAssignment{