Skip to content

Commit

Permalink
Use the OIDC Discovery end session endpoint if present
Browse files Browse the repository at this point in the history
Rate limit · GitHub

Access has been restricted

You have triggered a rate limit.

Please wait a few minutes before you try again;
in some cases this may take up to an hour.

nacx committed Apr 16, 2024
1 parent 11a2002 commit 0f4df22
Showing 6 changed files with 177 additions and 151 deletions.
252 changes: 126 additions & 126 deletions config/gen/go/v1/oidc/config.pb.go

Large diffs are not rendered by default.

11 changes: 1 addition & 10 deletions config/gen/go/v1/oidc/config.pb.validate.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions config/v1/oidc/config.proto
Original file line number Diff line number Diff line change
@@ -66,8 +66,9 @@ message LogoutConfig {
// of the service application, or to the
// [logout endpoint of the OIDC Provider](https://openid.net/specs/openid-connect-session-1_0.html#RPLogout).
// As with all redirects, the user's browser will perform a GET to this URI.
// Required.
string redirect_uri = 2 [(validate.rules).string.min_len = 1];
// Required when the OIDC discovery is not used or when the OIDC discovery does not provide the
// `end_session_endpoint`.
string redirect_uri = 2;
}

// The configuration of an OpenID Connect filter that can be used to retrieve identity and access tokens
3 changes: 1 addition & 2 deletions e2e/keycloak/authz-config.json
Original file line number Diff line number Diff line change
@@ -22,8 +22,7 @@
"header": "x-access-token"
},
"logout": {
"path": "/logout",
"redirect_uri": "https://host.docker.internal:9443/realms/master/protocol/openid-connect/logout"
"path": "/logout"
},
"redis_session_store_config": {
"server_uri": "redis://redis:6379"
12 changes: 12 additions & 0 deletions internal/authz/oidc.go
Original file line number Diff line number Diff line change
@@ -17,6 +17,7 @@ package authz
import (
"context"
"encoding/json"
"errors"
"fmt"
"io"
"net/http"
@@ -46,6 +47,10 @@ var (
{Header: &corev3.HeaderValue{Key: inthttp.HeaderCacheControl, Value: inthttp.HeaderCacheControlNoCache}},
{Header: &corev3.HeaderValue{Key: inthttp.HeaderPragma, Value: inthttp.HeaderPragmaNoCache}},
}

// ErrMissingLogoutRedirectURI is returned when the logout redirect uri is missing because it was not explicitly
// configured or the OIDC Discovery did not return it.
ErrMissingLogoutRedirectURI = errors.New("missing logout redirect uri")
)

// oidc handler is an implementation of the Handler interface that implements
@@ -839,5 +844,12 @@ func loadWellKnownConfig(client *http.Client, cfg *oidcv1.OIDCConfig) error {
}
cfg.GetJwksFetcher().JwksUri = wellKnownConfig.JWKSURL

if cfg.GetLogout() != nil && cfg.GetLogout().GetRedirectUri() == "" {
if wellKnownConfig.EndSessionEndpoint == "" {
return ErrMissingLogoutRedirectURI
}
cfg.GetLogout().RedirectUri = wellKnownConfig.EndSessionEndpoint
}

return nil
}
45 changes: 34 additions & 11 deletions internal/authz/oidc_test.go
Original file line number Diff line number Diff line change
@@ -36,6 +36,7 @@ import (
"github.com/tetratelabs/telemetry"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/test/bufconn"
"google.golang.org/protobuf/proto"

configv1 "github.com/istio-ecosystem/authservice/config/gen/go/v1"
oidcv1 "github.com/istio-ecosystem/authservice/config/gen/go/v1/oidc"
@@ -168,9 +169,19 @@ var (
ClientSecret: "test-client-secret",
},
Scopes: []string{"openid", "email"},
Logout: &oidcv1.LogoutConfig{Path: "/logout"},
}

wellKnownURIs = `
{
"issuer": "http://idp-test-server",
"authorization_endpoint": "http://idp-test-server/authorize",
"end_session_endpoint": "http://idp-test-server/endsession",
"token_endpoint": "http://idp-test-server/token",
"jwks_uri": "http://idp-test-server/jwks"
}`

wellKnownURIsNoEndSessionEndpoint = `
{
"issuer": "http://idp-test-server",
"authorization_endpoint": "http://idp-test-server/authorize",
@@ -343,7 +354,7 @@ func TestOIDCProcess(t *testing.T) {

// The following subset of tests is testing the callback requests, so there's expected communication with the IDP server.

idpServer := newServer()
idpServer := newServer(wellKnownURIs)
h.(*oidcHandler).httpClient = idpServer.newHTTPClient()

callbackTests := []struct {
@@ -964,7 +975,7 @@ func TestOIDCProcessWithFailingSessionStore(t *testing.T) {
})
}

idpServer := newServer()
idpServer := newServer(wellKnownURIs)
idpServer.statusCode = http.StatusOK
idpServer.tokensResponse = &idpTokensResponse{
IDToken: newJWT(t, jwkPriv, jwt.NewBuilder().Audience([]string{"test-client-id"}).Claim("nonce", newNonce)),
@@ -1068,7 +1079,7 @@ func TestOIDCProcessWithFailingJWKSProvider(t *testing.T) {
h, err := NewOIDCHandler(basicOIDCConfig, tlsPool, funcJWKSProvider, sessions, clock, oidc.NewStaticGenerator(newSessionID, newNonce, newState))
require.NoError(t, err)

idpServer := newServer()
idpServer := newServer(wellKnownURIs)
h.(*oidcHandler).httpClient = idpServer.newHTTPClient()

ctx := context.Background()
@@ -1367,21 +1378,33 @@ func TestAreTokensExpired(t *testing.T) {
}

func TestLoadWellKnownConfig(t *testing.T) {
idpServer := newServer()
idpServer := newServer(wellKnownURIs)
idpServer.Start()
t.Cleanup(idpServer.Stop)

cfg := proto.Clone(dynamicOIDCConfig).(*oidcv1.OIDCConfig)
require.NoError(t, loadWellKnownConfig(idpServer.newHTTPClient(), cfg))
require.Equal(t, cfg.AuthorizationUri, "http://idp-test-server/authorize")
require.Equal(t, cfg.TokenUri, "http://idp-test-server/token")
require.Equal(t, cfg.GetJwksFetcher().GetJwksUri(), "http://idp-test-server/jwks")
require.Equal(t, cfg.GetLogout().GetRedirectUri(), "http://idp-test-server/endsession")
}

func TestLoadWellKnownConfigMissingLogoutRedirectURI(t *testing.T) {
idpServer := newServer(wellKnownURIsNoEndSessionEndpoint)
idpServer.Start()
t.Cleanup(idpServer.Stop)

require.NoError(t, loadWellKnownConfig(idpServer.newHTTPClient(), dynamicOIDCConfig))
require.Equal(t, dynamicOIDCConfig.AuthorizationUri, "http://idp-test-server/authorize")
require.Equal(t, dynamicOIDCConfig.TokenUri, "http://idp-test-server/token")
require.Equal(t, dynamicOIDCConfig.GetJwksFetcher().GetJwksUri(), "http://idp-test-server/jwks")
cfg := proto.Clone(dynamicOIDCConfig).(*oidcv1.OIDCConfig)
require.ErrorIs(t, loadWellKnownConfig(idpServer.newHTTPClient(), cfg), ErrMissingLogoutRedirectURI)
}

func TestLoadWellKnownConfigError(t *testing.T) {
clock := oidc.Clock{}
tlsPool := internal.NewTLSConfigPool(context.Background())
cfg := proto.Clone(dynamicOIDCConfig).(*oidcv1.OIDCConfig)
sessions := &mockSessionStoreFactory{store: oidc.NewMemoryStore(&clock, time.Hour, time.Hour)}
_, err := NewOIDCHandler(dynamicOIDCConfig, tlsPool, oidc.NewJWKSProvider(newConfigFor(basicOIDCConfig), tlsPool),
_, err := NewOIDCHandler(cfg, tlsPool, oidc.NewJWKSProvider(newConfigFor(basicOIDCConfig), tlsPool),
sessions, clock, oidc.NewStaticGenerator(newSessionID, newNonce, newState))
require.Error(t, err) // Fail to retrieve the dynamic config since the test server is not running
}
@@ -1597,7 +1620,7 @@ type idpServer struct {
statusCode int
}

func newServer() *idpServer {
func newServer(wellKnownPayload string) *idpServer {
s := &http.Server{}
idpServer := &idpServer{server: s, listener: bufconn.Listen(1024)}

@@ -1616,7 +1639,7 @@ func newServer() *idpServer {
handler.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusOK)
_, _ = w.Write([]byte(wellKnownURIs))
_, _ = w.Write([]byte(wellKnownPayload))
})
s.Handler = handler
return idpServer

0 comments on commit 0f4df22

Please sign in to comment.