Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
200 changes: 200 additions & 0 deletions pkg/oidc/access_token_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,200 @@
package oidc_test

import (
"context"
"testing"
"time"

"github.com/golang-jwt/jwt/v5"
"github.com/opencloud-eu/opencloud/pkg/log"
"github.com/opencloud-eu/opencloud/pkg/oidc"
"github.com/opencloud-eu/opencloud/services/proxy/pkg/config"
"github.com/stretchr/testify/require"
)

func TestAccessTokenAudiences(t *testing.T) {
key := newRSAKey(t)
tests := []struct {
name string
audiences []string
aud any
missing bool
wantErr error
}{
{name: "disabled missing", missing: true},
{name: "disabled foreign", aud: "immich"},
{name: "disabled null", aud: nil},
{name: "disabled empty array", aud: []string{}},
{name: "explicitly empty configuration", audiences: []string{}, aud: "immich"},
{name: "single audience", audiences: []string{"opencloud"}, aud: "opencloud"},
{name: "array first match", audiences: []string{"opencloud"}, aud: []string{"opencloud", "immich"}},
{name: "array last match", audiences: []string{"opencloud"}, aud: []string{"immich", "opencloud"}},
{name: "any allowed audience", audiences: []string{"opencloud", "opencloud-api"}, aud: "opencloud-api"},
{name: "any allowed audience in array", audiences: []string{"opencloud", "opencloud-api"}, aud: []string{"immich", "opencloud-api"}},
{name: "duplicate audiences", audiences: []string{"opencloud", "opencloud"}, aud: []string{"opencloud", "opencloud"}},
{name: "URI audience", audiences: []string{"https://cloud.example/api"}, aud: "https://cloud.example/api"},
{name: "foreign", audiences: []string{"opencloud"}, aud: "immich", wantErr: jwt.ErrTokenInvalidAudience},
{name: "foreign array", audiences: []string{"opencloud"}, aud: []string{"immich", "account"}, wantErr: jwt.ErrTokenInvalidAudience},
{name: "case sensitive", audiences: []string{"opencloud"}, aud: "OpenCloud", wantErr: jwt.ErrTokenInvalidAudience},
{name: "exact match", audiences: []string{"opencloud"}, aud: "opencloud-api", wantErr: jwt.ErrTokenInvalidAudience},
{name: "no token normalization", audiences: []string{"opencloud"}, aud: " opencloud ", wantErr: jwt.ErrTokenInvalidAudience},
{name: "no wildcard matching", audiences: []string{"*"}, aud: "opencloud", wantErr: jwt.ErrTokenInvalidAudience},
{name: "missing", audiences: []string{"opencloud"}, missing: true, wantErr: jwt.ErrTokenRequiredClaimMissing},
{name: "null", audiences: []string{"opencloud"}, aud: nil, wantErr: jwt.ErrTokenRequiredClaimMissing},
{name: "empty string", audiences: []string{"opencloud"}, aud: "", wantErr: jwt.ErrTokenRequiredClaimMissing},
{name: "empty array", audiences: []string{"opencloud"}, aud: []string{}, wantErr: jwt.ErrTokenRequiredClaimMissing},
{name: "array empty string", audiences: []string{"opencloud"}, aud: []string{""}, wantErr: jwt.ErrTokenRequiredClaimMissing},
{name: "number", audiences: []string{"opencloud"}, aud: 123, wantErr: jwt.ErrTokenMalformed},
{name: "object", audiences: []string{"opencloud"}, aud: map[string]string{"aud": "opencloud"}, wantErr: jwt.ErrTokenMalformed},
{name: "mixed array", audiences: []string{"opencloud"}, aud: []any{"opencloud", 123}, wantErr: jwt.ErrTokenMalformed},
{name: "null array entry", audiences: []string{"opencloud"}, aud: []any{"opencloud", nil}, wantErr: jwt.ErrTokenMalformed},
{name: "disabled still rejects invalid type", aud: 123, wantErr: jwt.ErrTokenMalformed},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
claims := jwt.MapClaims{
"iss": "https://issuer.example",
"sub": "alice",
"sid": "session",
"exp": time.Now().Add(time.Hour).Unix(),
}
if !tt.missing {
claims["aud"] = tt.aud
}
client := newAccessTokenTestClient(t, key, tt.audiences, &oidc.ProviderMetadata{})
registered, all, err := client.VerifyAccessToken(context.Background(), signAccessToken(t, key, claims))
if tt.wantErr != nil {
require.ErrorIs(t, err, tt.wantErr)
require.Empty(t, all, "unverified claims must not be returned")
return
}
require.NoError(t, err)
require.Equal(t, "alice", registered.Subject)
require.Equal(t, "session", registered.SessionID)
require.Equal(t, "alice", all["sub"])
})
}
}

func TestAccessTokenValidationWithAudiences(t *testing.T) {
key, otherKey := newRSAKey(t), newRSAKey(t)
tests := []struct {
name string
issuer string
provider *oidc.ProviderMetadata
signingKey *signingKey
exp time.Time
nbf time.Time
wantErr error
}{
{name: "invalid signature", signingKey: otherKey, wantErr: jwt.ErrTokenSignatureInvalid},
{name: "invalid issuer", issuer: "https://other.example", wantErr: jwt.ErrTokenInvalidIssuer},
{name: "expired", exp: time.Now().Add(-time.Hour), wantErr: jwt.ErrTokenExpired},
{name: "not yet valid", nbf: time.Now().Add(time.Hour), wantErr: jwt.ErrTokenNotValidYet},
{name: "AD FS access token issuer", issuer: "https://adfs.example", provider: &oidc.ProviderMetadata{AccessTokenIssuer: "https://adfs.example"}},
{name: "AD FS rejects discovery issuer", provider: &oidc.ProviderMetadata{AccessTokenIssuer: "https://adfs.example"}, wantErr: jwt.ErrTokenInvalidIssuer},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if tt.issuer == "" {
tt.issuer = "https://issuer.example"
}
if tt.provider == nil {
tt.provider = &oidc.ProviderMetadata{}
}
if tt.signingKey == nil {
tt.signingKey = key
}
if tt.exp.IsZero() {
tt.exp = time.Now().Add(time.Hour)
}
claims := jwt.MapClaims{"iss": tt.issuer, "sub": "alice", "aud": "opencloud", "exp": tt.exp.Unix()}
if !tt.nbf.IsZero() {
claims["nbf"] = tt.nbf.Unix()
}
client := newAccessTokenTestClient(t, key, []string{"opencloud"}, tt.provider)
_, _, err := client.VerifyAccessToken(context.Background(), signAccessToken(t, tt.signingKey, claims))
require.ErrorIs(t, err, tt.wantErr)
})
}
}

func TestAccessTokenAudienceConfiguration(t *testing.T) {
for _, method := range []string{config.AccessTokenVerificationNone, ""} {
t.Run("incompatible method "+method, func(t *testing.T) {
// No HTTP client is supplied: invalid configuration must fail before discovery.
client, err := oidc.NewOIDCClient(
oidc.WithAccessTokenVerifyMethod(method),
oidc.WithAccessTokenAudiences([]string{"opencloud"}),
)
require.ErrorContains(t, err, "requires the jwt verification method")
require.Nil(t, client)
})
}
for _, audiences := range [][]string{{""}, {" \t"}, {"opencloud", ""}} {
client, err := oidc.NewOIDCClient(
oidc.WithAccessTokenVerifyMethod(config.AccessTokenVerificationJWT),
oidc.WithAccessTokenAudiences(audiences),
)
require.ErrorContains(t, err, "empty or whitespace-only")
require.Nil(t, client)
}
t.Run("none remains compatible when disabled", func(t *testing.T) {
client, err := oidc.NewOIDCClient(
oidc.WithLogger(log.NopLogger()),
oidc.WithAccessTokenVerifyMethod(config.AccessTokenVerificationNone),
oidc.WithProviderMetadata(&oidc.ProviderMetadata{}),
)
require.NoError(t, err)
_, _, err = client.VerifyAccessToken(context.Background(), "opaque-token")
require.NoError(t, err)
})
t.Run("caller cannot mutate the policy", func(t *testing.T) {
key := newRSAKey(t)
audiences := []string{"opencloud"}
client := newAccessTokenTestClient(t, key, audiences, &oidc.ProviderMetadata{})
audiences[0] = "immich"
_, _, err := client.VerifyAccessToken(context.Background(), signAccessToken(t, key,
jwt.MapClaims{"iss": "https://issuer.example", "aud": "immich"}))
require.ErrorIs(t, err, jwt.ErrTokenInvalidAudience)
})
}

func TestAccessTokenAudiencesDoNotApplyToLogoutTokens(t *testing.T) {
key := newRSAKey(t)
client := newAccessTokenTestClient(t, key, []string{"opencloud"}, &oidc.ProviderMetadata{})
token := signAccessToken(t, key, jwt.MapClaims{
"iss": "https://issuer.example",
"sub": "alice",
"aud": "web-client",
"events": map[string]any{
"http://schemas.openid.net/event/backchannel-logout": map[string]any{},
},
})
_, err := client.VerifyLogoutToken(context.Background(), token)
require.NoError(t, err)
}

func newAccessTokenTestClient(t *testing.T, key *signingKey, audiences []string, provider *oidc.ProviderMetadata) oidc.OIDCClient {
t.Helper()
client, err := oidc.NewOIDCClient(
oidc.WithLogger(log.NopLogger()),
oidc.WithOidcIssuer("https://issuer.example"),
oidc.WithAccessTokenVerifyMethod(config.AccessTokenVerificationJWT),
oidc.WithAccessTokenAudiences(audiences),
oidc.WithJWKS(key.jwks),
oidc.WithProviderMetadata(provider),
)
require.NoError(t, err)
return client
}

func signAccessToken(t *testing.T, key *signingKey, claims jwt.MapClaims) string {
t.Helper()
token := jwt.NewWithClaims(jwt.SigningMethodRS256, claims)
token.Header["kid"] = "1"
signed, err := token.SignedString(key.priv)
require.NoError(t, err)
return signed
}
19 changes: 15 additions & 4 deletions pkg/oidc/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ type oidcClient struct {
providerLock *sync.Mutex
skipIssuerValidation bool
accessTokenVerifyMethod string
accessTokenAudiences []string
remoteKeySet KeySet
algorithms []string

Expand All @@ -82,22 +83,32 @@ var _supportedAlgorithms = map[string]bool{
PS512: true,
}

// NewOIDCClient returns an OIDClient instance for the given issuer
func NewOIDCClient(opts ...Option) OIDCClient {
// NewOIDCClient returns an OIDCClient instance for the given issuer.
// Invalid access token audience configuration is rejected before creating the client.
func NewOIDCClient(opts ...Option) (OIDCClient, error) {
options := newOptions(opts...)
if len(options.AccessTokenAudiences) > 0 && options.AccessTokenVerifyMethod != config.AccessTokenVerificationJWT {
return nil, errors.New("access token audience validation requires the jwt verification method")
}
for _, audience := range options.AccessTokenAudiences {
if strings.TrimSpace(audience) == "" {
return nil, errors.New("access token audiences must not contain empty or whitespace-only entries")
}
}

return &oidcClient{
Logger: options.Logger,
issuer: options.OIDCIssuer,
httpClient: options.HTTPClient,
accessTokenVerifyMethod: options.AccessTokenVerifyMethod,
accessTokenAudiences: options.AccessTokenAudiences,
JWKSOptions: options.JWKSOptions, // TODO I don't like that we pass down config options ...
JWKS: options.JWKS,
providerLock: &sync.Mutex{},
jwksLock: &sync.Mutex{},
remoteKeySet: options.KeySet,
provider: options.ProviderMetadata,
}
}, nil
}

func (c *oidcClient) lookupWellKnownOpenidConfiguration(ctx context.Context) error {
Expand Down Expand Up @@ -301,7 +312,7 @@ func (c *oidcClient) verifyAccessTokenJWT(token string) (RegClaimsWithSID, jwt.M
issuer = c.provider.AccessTokenIssuer
}

_, err := jwt.ParseWithClaims(token, &claims, jwks.Keyfunc, jwt.WithIssuer(issuer))
_, err := jwt.ParseWithClaims(token, &claims, jwks.Keyfunc, jwt.WithIssuer(issuer), jwt.WithAudience(c.accessTokenAudiences...))
if err != nil {
return claims, mapClaims, err
}
Expand Down
5 changes: 4 additions & 1 deletion pkg/oidc/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,11 +167,14 @@ func (v logoutVerificationTest) runGetToken(t *testing.T) (*oidc.LogoutToken, er
}

pm := oidc.ProviderMetadata{}
verifier := oidc.NewOIDCClient(
verifier, err := oidc.NewOIDCClient(
oidc.WithOidcIssuer(issuer),
oidc.WithJWKS(jwks),
oidc.WithProviderMetadata(&pm),
)
if err != nil {
t.Fatal(err)
}

return verifier.VerifyLogoutToken(ctx, token)
}
Expand Down
11 changes: 11 additions & 0 deletions pkg/oidc/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ type Options struct {
// AccessTokenVerifyMethod to use when verifying access tokens
// TODO pass a function or interface to verify? an AccessTokenVerifier?
AccessTokenVerifyMethod string
// AccessTokenAudiences requires at least one matching audience in access tokens.
// An empty list disables audience validation.
AccessTokenAudiences []string
// Config to use
Config *goidc.Config

Expand Down Expand Up @@ -74,6 +77,14 @@ func WithAccessTokenVerifyMethod(val string) Option {
}
}

// WithAccessTokenAudiences sets the allowed audiences for access tokens only.
// An empty list disables audience validation.
func WithAccessTokenAudiences(val []string) Option {
return func(o *Options) {
o.AccessTokenAudiences = append([]string(nil), val...)
}
}

// WithHTTPClient provides a function to set the httpClient option.
func WithHTTPClient(val *http.Client) Option {
return func(o *Options) {
Expand Down
69 changes: 69 additions & 0 deletions services/proxy/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,72 @@ The following request authentication schemes are implemented:
- Signed URL
- Public Share Token

### OIDC Access Token Audiences

For production deployments, **enable audience validation** so that OpenCloud only
accepts access tokens intended for it. This is especially relevant when the same
identity provider serves several applications: without this check, an otherwise
valid token issued for another application can also be accepted by OpenCloud.

Set the allowed audiences as a comma-separated environment variable:

```console
PROXY_OIDC_AUDIENCES=opencloud,opencloud-api
PROXY_OIDC_ACCESS_TOKEN_VERIFY_METHOD=jwt
```

Alternatively, configure the list in `proxy.yaml`:

```yaml
oidc:
audiences:
- opencloud
- opencloud-api
access_token_verify_method: jwt
```

These audience values are examples. Configure your identity provider to include
the intended OpenCloud resource audience in the **access tokens** issued to all
relevant clients, including web, desktop and mobile clients. Adding an audience
only to an ID token or a Userinfo response does not satisfy this check.

The built-in IDP sets the access token's `aud` to the client ID of the
authenticated client. It does not support configuring a separate resource
audience. When using this IDP, list the client IDs of all OpenCloud clients you
use in `PROXY_OIDC_AUDIENCES`, including web, desktop and mobile clients. Setting
this proxy option does not change the tokens issued by the IDP.

For other IDPs, please refer to their documentation for proper support for the `aud` claim. For Keycloak see e.g.: [Keycloak's audience support documentation](https://www.keycloak.org/docs/latest/server_admin/#audience-support)

An access token must contain at least one exactly matching, case-sensitive value
in its `aud` claim. Both strings, such as `"aud": "opencloud"`, and arrays, such as
`"aud": ["another-api", "opencloud"]`, are supported. Tokens with missing, empty,
malformed or exclusively nonmatching audiences receive HTTP 401 on protected
routes. Configured audiences require JWT verification; combining a nonempty list
with `access_token_verify_method: none` prevents startup. List entries must not be
empty or consist only of whitespace.

The default list is empty, which disables audience validation to preserve
compatibility with existing identity provider configurations. An explicitly empty
`PROXY_OIDC_AUDIENCES` overrides any YAML list and disables the check; `audiences: []`
does the same in YAML. When OIDC is active, JWT verification is enabled and the
audience check is disabled, the proxy logs one startup warning, subject to the
configured log level. No audience warning is logged when
`access_token_verify_method` is `none`.

Restart the proxy after changing the configuration and apply the same policy to
all proxy instances. The signed access token, including its audience when
configured, is verified on a Userinfo cache miss. Cache hits reuse the cached
claims without verifying the token again or requesting Userinfo. Existing entries
in a shared or persistent cache can remain valid under the previous audience
configuration until they expire. Clear the Userinfo cache after updating all
proxy instances if the new policy must take effect immediately.

The disabled default is a compatibility decision. It does not relax the
[audience validation requirement in RFC 9068, Section 4](https://www.rfc-editor.org/rfc/rfc9068.html#name-validating-jwt-access-token):
a resource server following that JWT access token profile must reject tokens
whose audience does not identify the resource server.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add a few words/links about:

## Configuring Routes

The proxy handles routing to all endpoints that OpenCloud offers. The currently availabe default routes can be found [in the code](https://github.com/opencloud-eu/opencloud/blob/main/services/proxy/pkg/config/defaults/defaultconfig.go). Changing or adding routes can be necessary when writing own OpenCloud extensions.
Expand Down Expand Up @@ -231,6 +297,9 @@ The default `role_claim` (or `PROXY_ROLE_ASSIGNMENT_OIDC_CLAIM`) is `roles`. The

In a production deployment, you want to have basic authentication (`PROXY_ENABLE_BASIC_AUTH`) disabled which is the default state. You also want to setup a firewall to only allow requests to the proxy service or the reverse proxy if you have one. Requests to the other services should be blocked by the firewall.

Configure `PROXY_OIDC_AUDIENCES` as described in [OIDC Access Token Audiences](#oidc-access-token-audiences).
Enabling this check is strongly recommended for production deployments.

### Content Security Policy

For OpenCloud, external resources like an IDP (e.g. Keycloak) or when using web office documents or web apps, require defining a CSP. If not defined, the referenced services will not work.
Expand Down
Loading