Skip to content

Commit 93994bc

Browse files
olamilekan000olamilekan000
authored
implement PKCE for OIDC auth flow (#486)
1 parent 8e4325e commit 93994bc

14 files changed

Lines changed: 701 additions & 409 deletions

File tree

‎backend/auth/handler.go‎

Lines changed: 48 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ package auth
1818

1919
import (
2020
"context"
21+
"crypto/rand"
22+
"crypto/sha256"
2123
"encoding/base64"
2224
"encoding/json"
2325
"errors"
@@ -85,6 +87,8 @@ func (ah *AuthHandler) HandleAuthorize(w http.ResponseWriter, r *http.Request) {
8587
}
8688
}
8789

90+
logger.V(2).Info("AuthorizeRequest prepared", "authReq", authReq, "params", params)
91+
8892
if authReq.RedirectURL == "" || authReq.SessionID == "" {
8993
logger.Error(errors.New("missing required parameters"), "failed to authorize")
9094
ah.respondWithError(w, authReq.ClientType, "missing redirect_url or session_id", http.StatusBadRequest)
@@ -94,20 +98,37 @@ func (ah *AuthHandler) HandleAuthorize(w http.ResponseWriter, r *http.Request) {
9498
scopes := []string{"openid", "profile", "email", "offline_access", "groups"}
9599
dataCode, err := json.Marshal(authReq)
96100
if err != nil {
97-
logger.Info("failed to marshal auth code", "error", err)
101+
logger.Error(err, "failed to marshal auth code")
98102
ah.respondWithError(w, authReq.ClientType, err.Error(), http.StatusInternalServerError)
99103
return
100104
}
101105

102106
provider, err := ah.oidc.GetOIDCProvider(r.Context())
103107
if err != nil {
104-
logger.Info("failed to get OIDC provider", "error", err)
108+
logger.Error(err, "failed to get OIDC provider")
105109
ah.respondWithError(w, authReq.ClientType, err.Error(), http.StatusInternalServerError)
106110
return
107111
}
108112

109113
encoded := base64.URLEncoding.EncodeToString(dataCode)
110-
authURL := provider.OIDCProviderConfig(scopes).AuthCodeURL(encoded)
114+
115+
verifier, challenge, err := generatePKCE()
116+
if err != nil {
117+
logger.Error(err, "failed to generate PKCE")
118+
ah.respondWithError(w, authReq.ClientType, "failed to generate PKCE", http.StatusInternalServerError)
119+
return
120+
}
121+
if err := ah.sessionStore.SavePKCEVerifier(authReq.SessionID, verifier); err != nil {
122+
logger.Error(err, "failed to store PKCE verifier")
123+
ah.respondWithError(w, authReq.ClientType, "failed to store PKCE verifier", http.StatusInternalServerError)
124+
return
125+
}
126+
127+
opts := []oauth2.AuthCodeOption{
128+
oauth2.SetAuthURLParam("code_challenge", challenge),
129+
oauth2.SetAuthURLParam("code_challenge_method", "S256"),
130+
}
131+
authURL := provider.OIDCProviderConfig(scopes).AuthCodeURL(encoded, opts...)
111132

112133
http.Redirect(w, r, authURL, http.StatusFound)
113134
}
@@ -153,10 +174,11 @@ func (ah *AuthHandler) HandleCallback(w http.ResponseWriter, r *http.Request) {
153174
http.Error(w, err.Error(), http.StatusBadRequest)
154175
return
155176
}
177+
logger.V(2).Info("HandleCallback state unmarshaled", "authCode", authCode)
156178

157179
provider, err := ah.oidc.GetOIDCProvider(r.Context())
158180
if err != nil {
159-
logger.Info("failed to get OIDC provider", "error", err)
181+
logger.Error(err, "failed to get OIDC provider")
160182
ah.respondWithError(w, authCode.ClientType, err.Error(), http.StatusInternalServerError)
161183
return
162184
}
@@ -172,7 +194,16 @@ func (ah *AuthHandler) HandleCallback(w http.ResponseWriter, r *http.Request) {
172194
ctx = context.WithValue(ctx, oauth2.HTTPClient, client)
173195
}
174196

175-
token, err := provider.OIDCProviderConfig(nil).Exchange(ctx, code)
197+
verifier, err := ah.sessionStore.LoadAndDeletePKCEVerifier(authCode.SessionID)
198+
if err != nil || verifier == "" {
199+
logger.Error(err, "PKCE verifier not found for session; cannot exchange code", "sessionID", authCode.SessionID)
200+
msg := "PKCE verifier not found. If you run multiple backend instances, use a shared session store (e.g. Redis) so the instance handling the callback can read the verifier stored at authorize time."
201+
ah.respondWithError(w, authCode.ClientType, msg, http.StatusBadRequest)
202+
return
203+
}
204+
205+
exchangeOpts := []oauth2.AuthCodeOption{oauth2.VerifierOption(verifier)}
206+
token, err := provider.OIDCProviderConfig(nil).Exchange(ctx, code, exchangeOpts...)
176207
if err != nil {
177208
logger.Error(err, "failed to exchange token")
178209
http.Error(w, "internal error", http.StatusInternalServerError)
@@ -204,6 +235,7 @@ func (ah *AuthHandler) HandleCallback(w http.ResponseWriter, r *http.Request) {
204235
sessionState.SessionID,
205236
sessionState.ClusterID,
206237
sessionState.RedirectURL,
238+
sessionState.Token.Groups,
207239
24*time.Hour, // 24 hours expiration
208240
)
209241
if err != nil {
@@ -326,3 +358,14 @@ func (ah *AuthHandler) unwrapJWT(p string) ([]byte, error) {
326358
}
327359
return payload, nil
328360
}
361+
362+
func generatePKCE() (string, string, error) {
363+
data := make([]byte, 32)
364+
if _, err := rand.Read(data); err != nil {
365+
return "", "", err
366+
}
367+
verifier := base64.RawURLEncoding.EncodeToString(data)
368+
hash := sha256.Sum256([]byte(verifier))
369+
challenge := base64.RawURLEncoding.EncodeToString(hash[:])
370+
return verifier, challenge, nil
371+
}

‎backend/auth/jwt.go‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -32,11 +32,12 @@ type JWTService struct {
3232
}
3333

3434
type Claims struct {
35-
Subject string `json:"sub"`
36-
Issuer string `json:"iss"`
37-
SessionID string `json:"sid"`
38-
ClusterID string `json:"cid"`
39-
RedirectURL string `json:"red,omitempty"`
35+
Subject string `json:"sub"`
36+
Issuer string `json:"iss"`
37+
SessionID string `json:"sid"`
38+
ClusterID string `json:"cid"`
39+
Groups []string `json:"groups,omitempty"`
40+
RedirectURL string `json:"red,omitempty"`
4041
jwt.RegisteredClaims
4142
}
4243

@@ -53,13 +54,14 @@ func NewJWTService(issuer string) (*JWTService, error) {
5354
}, nil
5455
}
5556

56-
func (js *JWTService) GenerateToken(subject, oidcIssuer, sessionID, clusterID, redirectURL string, expiration time.Duration) (string, error) {
57+
func (js *JWTService) GenerateToken(subject, oidcIssuer, sessionID, clusterID, redirectURL string, groups []string, expiration time.Duration) (string, error) {
5758
now := time.Now()
5859
claims := &Claims{
5960
Subject: subject,
6061
Issuer: oidcIssuer,
6162
SessionID: sessionID,
6263
ClusterID: clusterID,
64+
Groups: groups,
6365
RedirectURL: redirectURL,
6466
RegisteredClaims: jwt.RegisteredClaims{
6567
Subject: subject,

‎backend/auth/middleware.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package auth
1919
import (
2020
"context"
2121
"encoding/json"
22+
"fmt"
2223
"net/http"
2324
"strings"
2425
"time"
@@ -111,6 +112,7 @@ func (am *AuthMiddleware) authenticate(next http.Handler) http.Handler {
111112
Token: session.TokenInfo{
112113
Subject: claims.Subject,
113114
Issuer: claims.Issuer,
115+
Groups: claims.Groups,
114116
},
115117
SessionID: claims.SessionID,
116118
ClusterID: claims.ClusterID,
@@ -219,7 +221,8 @@ func (am *AuthMiddleware) authorizeK8S(next http.Handler) http.Handler {
219221
if err != nil {
220222
logger.V(2).Info("Kubernetes RBAC authorization failed", "error", err)
221223
statusCode, code, details := mapErrorToCode(err)
222-
writeErrorResponse(w, statusCode, code, "Cluster authorization failed. Missing required permissions in the cluster to access bindings.", details)
224+
hint := fmt.Sprintf("%s Start the backend with --oidc-allowed-users=%s or --oidc-allowed-groups=<group> (user must be in one of the allowed groups).", details, authCtx.SessionState.Token.Subject)
225+
writeErrorResponse(w, statusCode, code, "Cluster authorization failed. Missing required permissions in the cluster to access bindings.", hint)
223226
return
224227
}
225228

‎backend/auth/types.go‎

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -121,11 +121,14 @@ func NewOIDCServiceProviderWithTLS(
121121

122122
func (o *OIDCServiceProvider) OIDCProviderConfig(scopes []string) *oauth2.Config {
123123
config := &oauth2.Config{
124-
ClientID: o.clientID,
125-
ClientSecret: o.clientSecret,
126-
Endpoint: o.provider.Endpoint(),
127-
RedirectURL: o.redirectURI,
128-
Scopes: scopes,
124+
ClientID: o.clientID,
125+
Endpoint: o.provider.Endpoint(),
126+
RedirectURL: o.redirectURI,
127+
Scopes: scopes,
128+
}
129+
130+
if o.clientSecret != "" {
131+
config.ClientSecret = o.clientSecret
129132
}
130133

131134
return config
@@ -134,3 +137,15 @@ func (o *OIDCServiceProvider) OIDCProviderConfig(scopes []string) *oauth2.Config
134137
func (o *OIDCServiceProvider) GetTLSConfig() *tls.Config {
135138
return o.tlsConfig
136139
}
140+
141+
func (o *OIDCServiceProvider) ClientID() string {
142+
return o.clientID
143+
}
144+
145+
func (o *OIDCServiceProvider) ClientSecret() string {
146+
return o.clientSecret
147+
}
148+
149+
func (o *OIDCServiceProvider) IssuerURL() string {
150+
return o.issuerURL
151+
}

‎backend/options/oidc.go‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -112,9 +112,6 @@ func (options *OIDC) Validate() error {
112112
if options.IssuerClientID == "" {
113113
return fmt.Errorf("OIDC issuer client ID cannot be empty")
114114
}
115-
if options.IssuerClientSecret == "" && os.Getenv("OIDC_CLIENT_SECRET") == "" {
116-
return fmt.Errorf("OIDC issuer client secret cannot be empty")
117-
}
118115
if os.Getenv("OIDC_CLIENT_SECRET") != "" && options.Type == string(kubebindv1alpha2.OIDCProviderTypeEmbedded) {
119116
return fmt.Errorf("OIDC issuer client secret cannot be provided via environment variable when using embedded OIDC provider")
120117
}

‎backend/server.go‎

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -270,22 +270,33 @@ func NewServer(ctx context.Context, c *Config) (*Server, error) {
270270
func (s *Server) initializeOIDCProvider(ctx context.Context, callback string) (*auth.OIDCServiceProvider, error) {
271271
logger := klog.FromContext(ctx)
272272
logger.Info("Initializing OIDC Service Provider", "issuerURL", s.Config.Options.OIDC.IssuerURL)
273-
if s.Config.Options.OIDC.TLSConfig != nil {
273+
274+
clientID := s.Config.Options.OIDC.IssuerClientID
275+
clientSecret := s.Config.Options.OIDC.IssuerClientSecret
276+
issuerURL := s.Config.Options.OIDC.IssuerURL
277+
tlsConfig := s.Config.Options.OIDC.TLSConfig
278+
279+
if clientSecret != "" {
280+
logger.Info("WARNING: OIDC client secret is configured. For CLI applications, consider using PKCE.")
281+
}
282+
283+
if tlsConfig != nil {
274284
return auth.NewOIDCServiceProviderWithTLS(
275285
ctx,
276-
s.Config.Options.OIDC.IssuerClientID,
277-
s.Config.Options.OIDC.IssuerClientSecret,
286+
clientID,
287+
clientSecret,
278288
callback,
279-
s.Config.Options.OIDC.IssuerURL,
280-
s.Config.Options.OIDC.TLSConfig,
289+
issuerURL,
290+
tlsConfig,
281291
)
282292
}
293+
283294
return auth.NewOIDCServiceProvider(
284295
ctx,
285-
s.Config.Options.OIDC.IssuerClientID,
286-
s.Config.Options.OIDC.IssuerClientSecret,
296+
clientID,
297+
clientSecret,
287298
callback,
288-
s.Config.Options.OIDC.IssuerURL,
299+
issuerURL,
289300
)
290301
}
291302

‎backend/session/store.go‎

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,26 +17,32 @@ limitations under the License.
1717
package session
1818

1919
import (
20+
"errors"
2021
"fmt"
2122
"sync"
2223
)
2324

2425
var ErrSessionNotFound = fmt.Errorf("session not found")
26+
var ErrPKCEVerifierNotFound = fmt.Errorf("pkce verifier not found")
2527

2628
type Store interface {
2729
Save(state *State) error
2830
Load(sessionID string) (*State, error)
2931
Delete(sessionID string) error
32+
SavePKCEVerifier(sessionID, verifier string) error
33+
LoadAndDeletePKCEVerifier(sessionID string) (string, error)
3034
}
3135

3236
type InMemoryStore struct {
33-
lock sync.RWMutex
34-
sessions map[string]*State
37+
lock sync.RWMutex
38+
sessions map[string]*State
39+
pkceVerifiers map[string]string
3540
}
3641

3742
func NewInMemoryStore() *InMemoryStore {
3843
return &InMemoryStore{
39-
sessions: make(map[string]*State),
44+
sessions: make(map[string]*State),
45+
pkceVerifiers: make(map[string]string),
4046
}
4147
}
4248

@@ -63,3 +69,27 @@ func (s *InMemoryStore) Delete(sessionID string) error {
6369
delete(s.sessions, sessionID)
6470
return nil
6571
}
72+
73+
func (s *InMemoryStore) SavePKCEVerifier(sessionID, verifier string) error {
74+
if sessionID == "" || verifier == "" {
75+
return errors.New("sessionID and verifier cannot be empty")
76+
}
77+
s.lock.Lock()
78+
defer s.lock.Unlock()
79+
s.pkceVerifiers[sessionID] = verifier
80+
return nil
81+
}
82+
83+
func (s *InMemoryStore) LoadAndDeletePKCEVerifier(sessionID string) (string, error) {
84+
if sessionID == "" {
85+
return "", ErrPKCEVerifierNotFound
86+
}
87+
s.lock.Lock()
88+
defer s.lock.Unlock()
89+
verifier, ok := s.pkceVerifiers[sessionID]
90+
if !ok {
91+
return "", ErrPKCEVerifierNotFound
92+
}
93+
delete(s.pkceVerifiers, sessionID)
94+
return verifier, nil
95+
}

‎cli/pkg/kubectl/bind/plugin/authenticate_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,8 @@ func TestValidateVersion(t *testing.T) {
3030
{"v0.0.0", "v0.0.0", false},
3131
{"development version", "v0.0.0-master+$Format:%H$", false},
3232
{"old", "v0.2.3", true},
33-
{"minimum", "v0.3.0", false},
34-
{"newer minor", "v0.3.5", false},
33+
{"minimum", "v0.6.0", false},
34+
{"newer minor", "v0.6.1", false},
3535
{"newer major", "v1.2.5", false},
3636
}
3737
for _, tt := range tests {

0 commit comments

Comments
 (0)