Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for PKCE #265

Merged
merged 5 commits into from
Oct 10, 2024
Merged

Add support for PKCE #265

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ce9584d
Added support for PKCE
gdasson Oct 9, 2024
bdf3f0c
Added support for PKCE - Updated unit tests and fixed formatting
gdasson Oct 9, 2024
232093f
Added support for PKCE - Updated keycloak setup to enable PKCE for e2…
gdasson Oct 10, 2024
ef147fd
Added support for PKCE - Updated go.mod with dependencies
gdasson Oct 10, 2024
c627e8d
Added support for PKCE - Fixed linting issues
gdasson Oct 10, 2024
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
1 change: 1 addition & 0 deletions e2e/keycloak/setup-keycloak.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ set -ex
-s clientId="${CLIENT_ID}" \
-s secret="${CLIENT_SECRET}" \
-s "redirectUris=[\"${REDIRECT_URL}\"]" \
-s "attributes={\"pkce.code.challenge.method\":\"S256\"}" \
-s consentRequired=false \
--server "${KEYCLOAK_SERVER}" \
--realm "${REALM}" \
Expand Down
2 changes: 2 additions & 0 deletions e2e/redis/store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ func TestRedisAuthorizationState(t *testing.T) {
State: "state",
Nonce: "nonce",
RequestedURL: "https://example.com",
CodeVerifier: "code_verifier",
}
require.NoError(t, store.SetAuthorizationState(ctx, "s1", as))

Expand Down Expand Up @@ -126,6 +127,7 @@ func TestSessionExpiration(t *testing.T) {
State: "state",
Nonce: "nonce",
RequestedURL: "https://example.com",
CodeVerifier: "code_verifier",
}
require.NoError(t, store.SetAuthorizationState(ctx, "s1", as))
require.Eventually(t, func() bool {
Expand Down
30 changes: 18 additions & 12 deletions internal/authz/oidc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
typev3 "github.com/envoyproxy/go-control-plane/envoy/type/v3"
"github.com/lestrrat-go/jwx/v2/jws"
"github.com/tetratelabs/telemetry"
"golang.org/x/oauth2"
"google.golang.org/genproto/googleapis/rpc/status"
"google.golang.org/grpc/codes"

Expand Down Expand Up @@ -237,9 +238,10 @@ func (o *oidcHandler) redirectToIDP(ctx context.Context, log telemetry.Logger,
}

var (
sessionID = o.sessionGen.GenerateSessionID()
nonce = o.sessionGen.GenerateNonce()
state = o.sessionGen.GenerateState()
sessionID = o.sessionGen.GenerateSessionID()
nonce = o.sessionGen.GenerateNonce()
state = o.sessionGen.GenerateState()
codeVerifier = o.sessionGen.GenerateCodeVerifier()
)

// Store the authorization state
Expand All @@ -251,6 +253,7 @@ func (o *oidcHandler) redirectToIDP(ctx context.Context, log telemetry.Logger,
State: state,
Nonce: nonce,
RequestedURL: requestedURL,
CodeVerifier: codeVerifier,
}); err != nil {
log.Error("error storing the new authorization state", err)
setDenyResponse(resp, newSessionErrorResponse(), codes.Unauthenticated)
Expand All @@ -259,12 +262,14 @@ func (o *oidcHandler) redirectToIDP(ctx context.Context, log telemetry.Logger,

// Generate the redirect URL
query := url.Values{
"response_type": []string{"code"},
"client_id": []string{o.config.GetClientId()},
"redirect_uri": []string{o.config.GetCallbackUri()},
"scope": []string{strings.Join(o.config.GetScopes(), " ")},
"state": []string{state},
"nonce": []string{nonce},
"response_type": []string{"code"},
"client_id": []string{o.config.GetClientId()},
"redirect_uri": []string{o.config.GetCallbackUri()},
"scope": []string{strings.Join(o.config.GetScopes(), " ")},
"state": []string{state},
"nonce": []string{nonce},
"code_challenge": []string{oauth2.S256ChallengeFromVerifier(codeVerifier)},
"code_challenge_method": []string{"S256"},
}
redirectURL := o.config.GetAuthorizationUri() + "?" + query.Encode()

Expand Down Expand Up @@ -328,9 +333,10 @@ func (o *oidcHandler) retrieveTokens(ctx context.Context, log telemetry.Logger,

// build body
form := url.Values{
"grant_type": []string{"authorization_code"},
"code": []string{codeFromReq},
"redirect_uri": []string{o.config.GetCallbackUri()},
"grant_type": []string{"authorization_code"},
"code": []string{codeFromReq},
"redirect_uri": []string{o.config.GetCallbackUri()},
"code_verifier": []string{stateFromStore.CodeVerifier},
}

// build headers
Expand Down
34 changes: 19 additions & 15 deletions internal/authz/oidc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
"github.com/lestrrat-go/jwx/v2/jwt"
"github.com/stretchr/testify/require"
"github.com/tetratelabs/telemetry"
"golang.org/x/oauth2"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/test/bufconn"
"google.golang.org/protobuf/proto"
Expand Down Expand Up @@ -125,10 +126,11 @@ var (
yesterday = time.Now().Add(-24 * time.Hour)
tomorrow = time.Now().Add(24 * time.Hour)

sessionID = "test-session-id"
newSessionID = "new-session-id"
newNonce = "new-nonce"
newState = "new-state"
sessionID = "test-session-id"
newSessionID = "new-session-id"
newNonce = "new-nonce"
newState = "new-state"
newCodeVerifier = "new-code-verifier"

basicOIDCConfig = &oidcv1.OIDCConfig{
IdToken: &oidcv1.TokenConfig{
Expand Down Expand Up @@ -190,12 +192,14 @@ var (
}`

wantRedirectParams = url.Values{
"response_type": {"code"},
"client_id": {"test-client-id"},
"redirect_uri": {"https://localhost:443/callback"},
"scope": {"openid email"},
"state": {newState},
"nonce": {newNonce},
"response_type": {"code"},
"client_id": {"test-client-id"},
"redirect_uri": {"https://localhost:443/callback"},
"scope": {"openid email"},
"state": {newState},
"nonce": {newNonce},
"code_challenge": {oauth2.S256ChallengeFromVerifier(newCodeVerifier)},
"code_challenge_method": {"S256"},
}

wantRedirectBaseURI = "http://idp-test-server/auth"
Expand Down Expand Up @@ -228,7 +232,7 @@ func TestOIDCProcess(t *testing.T) {
tlsPool := internal.NewTLSConfigPool(context.Background())
h, err := NewOIDCHandler(basicOIDCConfig, tlsPool,
oidc.NewJWKSProvider(newConfigFor(basicOIDCConfig), tlsPool), sessions, clock,
oidc.NewStaticGenerator(newSessionID, newNonce, newState))
oidc.NewStaticGenerator(newSessionID, newNonce, newState, newCodeVerifier))
require.NoError(t, err)

ctx := context.Background()
Expand Down Expand Up @@ -949,7 +953,7 @@ func TestOIDCProcessWithFailingSessionStore(t *testing.T) {
}

h, err := NewOIDCHandler(basicOIDCConfig, tlsPool, oidc.NewJWKSProvider(newConfigFor(basicOIDCConfig), tlsPool),
sessions, oidc.Clock{}, oidc.NewStaticGenerator(newSessionID, newNonce, newState))
sessions, oidc.Clock{}, oidc.NewStaticGenerator(newSessionID, newNonce, newState, newCodeVerifier))
require.NoError(t, err)

ctx := context.Background()
Expand Down Expand Up @@ -1094,7 +1098,7 @@ func TestOIDCProcessWithFailingJWKSProvider(t *testing.T) {
sessions := &mockSessionStoreFactory{store: oidc.NewMemoryStore(&clock, time.Hour, time.Hour)}
store := sessions.Get(basicOIDCConfig)
tlsPool := internal.NewTLSConfigPool(context.Background())
h, err := NewOIDCHandler(basicOIDCConfig, tlsPool, funcJWKSProvider, sessions, clock, oidc.NewStaticGenerator(newSessionID, newNonce, newState))
h, err := NewOIDCHandler(basicOIDCConfig, tlsPool, funcJWKSProvider, sessions, clock, oidc.NewStaticGenerator(newSessionID, newNonce, newState, newCodeVerifier))
require.NoError(t, err)

idpServer := newServer(wellKnownURIs)
Expand Down Expand Up @@ -1425,7 +1429,7 @@ func TestLoadWellKnownConfigError(t *testing.T) {
cfg.ConfigurationUri = "http://stopped-server/.well-known/openid-configuration"
sessions := &mockSessionStoreFactory{store: oidc.NewMemoryStore(&clock, time.Hour, time.Hour)}
_, err := NewOIDCHandler(cfg, tlsPool, oidc.NewJWKSProvider(newConfigFor(basicOIDCConfig), tlsPool),
sessions, clock, oidc.NewStaticGenerator(newSessionID, newNonce, newState))
sessions, clock, oidc.NewStaticGenerator(newSessionID, newNonce, newState, newCodeVerifier))
require.Error(t, err) // Fail to retrieve the dynamic config since the test server is not running
}

Expand All @@ -1447,7 +1451,7 @@ func TestNewOIDCHandler(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {

_, err := NewOIDCHandler(tt.config, tlsPool, oidc.NewJWKSProvider(newConfigFor(basicOIDCConfig), tlsPool),
sessions, clock, oidc.NewStaticGenerator(newSessionID, newNonce, newState))
sessions, clock, oidc.NewStaticGenerator(newSessionID, newNonce, newState, newCodeVerifier))
if tt.wantErr {
require.Error(t, err)
} else {
Expand Down
8 changes: 6 additions & 2 deletions internal/oidc/redis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,13 @@ const (
keyState = "state"
keyNonce = "nonce"
keyRequestedURL = "requested_url"
keyCodeVerifier = "code_verifier"
keyTimeAdded = "time_added"
)

var (
tokenResponseKeys = []string{keyIDToken, keyAccessToken, keyRefreshToken, keyAccessTokenExpiry, keyTimeAdded}
authorizationStateKeys = []string{keyState, keyNonce, keyRequestedURL, keyTimeAdded}
authorizationStateKeys = []string{keyState, keyNonce, keyRequestedURL, keyTimeAdded, keyCodeVerifier}
)

// redisStore is an in-memory implementation of the SessionStore interface that stores
Expand Down Expand Up @@ -165,6 +166,7 @@ func (r *redisStore) SetAuthorizationState(ctx context.Context, sessionID string
keyState: authorizationState.State,
keyNonce: authorizationState.Nonce,
keyRequestedURL: authorizationState.RequestedURL,
keyCodeVerifier: authorizationState.CodeVerifier,
}

if err := r.client.HMSet(ctx, sessionID, state).Err(); err != nil {
Expand Down Expand Up @@ -193,7 +195,7 @@ func (r *redisStore) GetAuthorizationState(ctx context.Context, sessionID string
return nil, err
}

if state.State == "" || state.Nonce == "" || state.RequestedURL == "" {
if state.State == "" || state.Nonce == "" || state.RequestedURL == "" || state.CodeVerifier == "" {
return nil, nil
}

Expand Down Expand Up @@ -286,6 +288,7 @@ type (
State string `redis:"state"`
Nonce string `redis:"nonce"`
RequestedURL string `redis:"requested_url"`
CodeVerifier string `redis:"code_verifier"`
TimeAdded time.Time `redis:"time_added"`
}
)
Expand All @@ -304,5 +307,6 @@ func (r redisAuthState) AuthorizationState() *AuthorizationState {
State: r.State,
Nonce: r.Nonce,
RequestedURL: r.RequestedURL,
CodeVerifier: r.CodeVerifier,
}
}
1 change: 1 addition & 0 deletions internal/oidc/redis_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ func TestRedisAuthorizationState(t *testing.T) {
State: "state",
Nonce: "nonce",
RequestedURL: "requested_url",
CodeVerifier: "code_verifier",
}
require.NoError(t, store.SetAuthorizationState(ctx, "s1", as))

Expand Down
26 changes: 19 additions & 7 deletions internal/oidc/session.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"github.com/redis/go-redis/v9"
"github.com/tetratelabs/run"
"github.com/tetratelabs/telemetry"
"golang.org/x/oauth2"

configv1 "github.com/istio-ecosystem/authservice/config/gen/go/v1"
oidcv1 "github.com/istio-ecosystem/authservice/config/gen/go/v1/oidc"
Expand Down Expand Up @@ -136,6 +137,7 @@ type SessionGenerator interface {
GenerateSessionID() string
GenerateNonce() string
GenerateState() string
GenerateCodeVerifier() string
}

var (
Expand All @@ -151,9 +153,10 @@ type (

// staticGenerator is a session generator that uses static strings.
staticGenerator struct {
sessionID string
nonce string
state string
sessionID string
nonce string
state string
codeVerifier string
}
)

Expand All @@ -176,6 +179,10 @@ func (r randomGenerator) GenerateState() string {
return r.generate(32)
}

func (r randomGenerator) GenerateCodeVerifier() string {
return oauth2.GenerateVerifier()
}

func (r *randomGenerator) generate(n int) string {
const charset = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
b := make([]byte, n)
Expand All @@ -186,11 +193,12 @@ func (r *randomGenerator) generate(n int) string {
}

// NewStaticGenerator creates a new static session generator.
func NewStaticGenerator(sessionID, nonce, state string) SessionGenerator {
func NewStaticGenerator(sessionID, nonce, state, codeVerifier string) SessionGenerator {
return &staticGenerator{
sessionID: sessionID,
nonce: nonce,
state: state,
sessionID: sessionID,
nonce: nonce,
state: state,
codeVerifier: codeVerifier,
}
}

Expand All @@ -205,3 +213,7 @@ func (s staticGenerator) GenerateNonce() string {
func (s staticGenerator) GenerateState() string {
return s.state
}

func (s staticGenerator) GenerateCodeVerifier() string {
return s.codeVerifier
}
5 changes: 4 additions & 1 deletion internal/oidc/session_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,14 +131,17 @@ func TestSessionGenerator(t *testing.T) {
require.NotEqual(t, sg.GenerateSessionID(), sg.GenerateSessionID())
require.NotEqual(t, sg.GenerateState(), sg.GenerateState())
require.NotEqual(t, sg.GenerateNonce(), sg.GenerateNonce())
require.NotEqual(t, sg.GenerateCodeVerifier(), sg.GenerateCodeVerifier())
})
t.Run("static", func(t *testing.T) {
sg := NewStaticGenerator("sessionid", "nonce", "state")
sg := NewStaticGenerator("sessionid", "nonce", "state", "codeverifier")
require.Equal(t, sg.GenerateSessionID(), sg.GenerateSessionID())
require.Equal(t, sg.GenerateState(), sg.GenerateState())
require.Equal(t, sg.GenerateNonce(), sg.GenerateNonce())
Copy link
Collaborator

Choose a reason for hiding this comment

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

[nit] Update this test and the random one to also include a check for the code verifier.

Copy link
Contributor Author

@gdasson gdasson Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@nacx : Thanks for the review. Yes, there is a way to configure keycloak to enforce PKCE use by client. Infact, I also used keycloak for my local testing. In the UI, It's available under Client > Advanced > Proof Key for Code Exchange Code Challenge Method > Set it to S256. (However, I am not sure if the same setting is available through the Keycloak admin API)

Here is a screenshot from the UI:
image

Copy link
Collaborator

@nacx nacx Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The client is created and configured here: https://github.com/istio-ecosystem/authservice/blob/main/e2e/keycloak/setup-keycloak.sh#L50-L58
Let's see if PKCE can be configured in that script.

Actually, there is probably no need to create another suite. The istio suite also uses Keycloak, so we can just change the keycloak suite and we should have all cases covered.

Testing ti locally should be easy:

make docker   # the e2e tests need the update Docker image
make e2e/keycloak

# Or, if you want to see all requests/responses for the e2e tests logged:
E2E_TEST_OPTS="-v -count=1" make e2e/keycloak

Or you can start the infra and run the tests from your IDE: https://github.com/istio-ecosystem/authservice/blob/main/DEVELOPMENT.md#running-tests-from-your-ide
Keycloak will be accessible on https://localhost:9443 (admin / admin)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding this to the keycloak setup script sould do the trick, to enforce PKCE server-side:

diff --git a/e2e/keycloak/setup-keycloak.sh b/e2e/keycloak/setup-keycloak.sh
index 7fc46af..ee3d340 100755
--- a/e2e/keycloak/setup-keycloak.sh
+++ b/e2e/keycloak/setup-keycloak.sh
@@ -51,6 +51,7 @@ set -ex
     -s clientId="${CLIENT_ID}" \
     -s secret="${CLIENT_SECRET}" \
     -s "redirectUris=[\"${REDIRECT_URL}\"]" \
+    -s "attributes={\"pkce.code.challenge.method\":\"S256\"}" \
     -s consentRequired=false \
     --server "${KEYCLOAK_SERVER}" \
     --realm "${REALM}" \

Copy link
Contributor Author

@gdasson gdasson Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Added and ran e2e tests, they run fine. Also physically checked the setting in the UI before destroying infra.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice! I run the e2e with this setting but without the changes in the PR and they were failing, complaining about the missing query params, so all good!

require.Equal(t, sg.GenerateCodeVerifier(), sg.GenerateCodeVerifier())
require.Equal(t, "sessionid", sg.GenerateSessionID())
require.Equal(t, "state", sg.GenerateState())
require.Equal(t, "nonce", sg.GenerateNonce())
require.Equal(t, "codeverifier", sg.GenerateCodeVerifier())
})
}
1 change: 1 addition & 0 deletions internal/oidc/state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,5 @@ type AuthorizationState struct {
State string
Nonce string
RequestedURL string
CodeVerifier string
}
Loading