Reduce ID token verification errors to a fixed category
go-oidc's RemoteKeySet puts the JWKS endpoint's response body in its error for non-200 and malformed 200 responses; Verify wrapped it and the callback logged it. verifyError now keeps only a fixed category, plus the expiry for an expired token. A test with a sentinel JWKS body checks it reaches neither the returned error, the response nor the logs. Addresses feature-review finding F03-03 on PR #24. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
+12
-1
@@ -353,7 +353,7 @@ func (a *Auth) verify(ctx context.Context, code string, state loginState) (Claim
|
||||
}
|
||||
idToken, err := a.verifier.Verify(ctx, raw)
|
||||
if err != nil {
|
||||
return Claims{}, fmt.Errorf("verify ID token: %w", err)
|
||||
return Claims{}, verifyError(err)
|
||||
}
|
||||
if subtle.ConstantTimeCompare([]byte(idToken.Nonce), []byte(state.Nonce)) != 1 {
|
||||
return Claims{}, errors.New("ID token nonce mismatch")
|
||||
@@ -404,6 +404,17 @@ func userinfoError(err error) error {
|
||||
return errors.New("userinfo: endpoint returned an unusable response")
|
||||
}
|
||||
|
||||
// verifyError reduces a failed ID token verification to a fixed category.
|
||||
// go-oidc's error text can carry the JWKS endpoint's response body or
|
||||
// token-supplied values, so only the expiry of an expired token is kept.
|
||||
func verifyError(err error) error {
|
||||
var expired *oidc.TokenExpiredError
|
||||
if errors.As(err, &expired) {
|
||||
return fmt.Errorf("verify ID token: token expired at %s", expired.Expiry.UTC().Format(time.RFC3339))
|
||||
}
|
||||
return errors.New("verify ID token: token or provider key set rejected")
|
||||
}
|
||||
|
||||
// member reports whether groups contains an allowed group.
|
||||
func (a *Auth) member(groups []string) bool {
|
||||
return slices.ContainsFunc(groups, func(g string) bool { return slices.Contains(a.allowed, g) })
|
||||
|
||||
@@ -60,6 +60,8 @@ type fakeProvider struct {
|
||||
userinfoHits int
|
||||
// userinfoReply, when set, is returned by the userinfo endpoint instead.
|
||||
userinfoReply *tokenReply
|
||||
// jwksReply, when set, is returned by the JWKS endpoint instead.
|
||||
jwksReply *tokenReply
|
||||
}
|
||||
|
||||
type tokenReply struct {
|
||||
@@ -110,6 +112,15 @@ func (p *fakeProvider) discovery(w http.ResponseWriter, _ *http.Request) {
|
||||
}
|
||||
|
||||
func (p *fakeProvider) jwks(w http.ResponseWriter, _ *http.Request) {
|
||||
p.mu.Lock()
|
||||
reply := p.jwksReply
|
||||
p.mu.Unlock()
|
||||
if reply != nil {
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
w.WriteHeader(reply.status)
|
||||
io.WriteString(w, reply.body)
|
||||
return
|
||||
}
|
||||
writeJSON(w, map[string]any{"keys": []map[string]string{{
|
||||
"kty": "RSA",
|
||||
"kid": testKID,
|
||||
@@ -308,6 +319,7 @@ func (m *memorySessions) started() []Claims {
|
||||
}
|
||||
|
||||
type harness struct {
|
||||
auth *Auth
|
||||
provider *fakeProvider
|
||||
sessions *memorySessions
|
||||
router http.Handler
|
||||
@@ -350,7 +362,7 @@ func newHarness(t *testing.T) *harness {
|
||||
}
|
||||
r := chi.NewRouter()
|
||||
a.Mount(r)
|
||||
return &harness{provider: p, sessions: sessions, router: r, logs: logs}
|
||||
return &harness{auth: a, provider: p, sessions: sessions, router: r, logs: logs}
|
||||
}
|
||||
|
||||
func (h *harness) get(target string, cookies ...*http.Cookie) *http.Response {
|
||||
@@ -728,6 +740,66 @@ func TestCallbackExchangeFailureDoesNotLogProviderResponse(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestCallbackKeySetFailureDoesNotLogProviderResponse(t *testing.T) {
|
||||
t.Parallel()
|
||||
const sentinel = "sentinel-jwks-body-5c1e"
|
||||
tests := []struct {
|
||||
name string
|
||||
reply tokenReply
|
||||
}{
|
||||
{"error response", tokenReply{http.StatusInternalServerError, sentinel}},
|
||||
{"malformed success", tokenReply{http.StatusOK, `{"keys":"` + sentinel + `"`}},
|
||||
}
|
||||
for _, tc := range tests {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
h := newHarness(t)
|
||||
h.provider.mu.Lock()
|
||||
h.provider.jwksReply = &tc.reply
|
||||
h.provider.mu.Unlock()
|
||||
|
||||
// The returned error, checked through verify directly.
|
||||
l := h.login(t, LoginPath)
|
||||
req := httptest.NewRequest(http.MethodGet, CallbackPath, nil)
|
||||
req.AddCookie(l.cookie)
|
||||
state, err := h.auth.cookie.read(req, time.Now())
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
code := h.provider.authorize(l.query.Get("code_challenge"), h.provider.signRS256(h.provider.claims(l.query.Get("nonce"))))
|
||||
_, err = h.auth.verify(context.Background(), code, state)
|
||||
if err == nil {
|
||||
t.Fatal("verify succeeded with an unusable key set")
|
||||
}
|
||||
if strings.Contains(err.Error(), sentinel) {
|
||||
t.Errorf("verify error contains the JWKS response: %v", err)
|
||||
}
|
||||
|
||||
// The logs and response, through the callback.
|
||||
l = h.login(t, LoginPath)
|
||||
resp := h.callback(l, l.query.Get("state"), h.provider.signRS256(h.provider.claims(l.query.Get("nonce"))))
|
||||
|
||||
if resp.StatusCode != http.StatusUnauthorized {
|
||||
t.Errorf("callback status = %d, want 401", resp.StatusCode)
|
||||
}
|
||||
body, _ := io.ReadAll(resp.Body)
|
||||
if strings.Contains(string(body), sentinel) {
|
||||
t.Errorf("response contains the JWKS response: %s", body)
|
||||
}
|
||||
logs := h.logs.String()
|
||||
if strings.Contains(logs, sentinel) {
|
||||
t.Errorf("logs contain the JWKS response: %s", logs)
|
||||
}
|
||||
if want := "token or provider key set rejected"; !strings.Contains(logs, want) {
|
||||
t.Errorf("logs = %q, want %q", logs, want)
|
||||
}
|
||||
if len(h.sessions.started()) != 0 {
|
||||
t.Error("a session started")
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestLoginCookieExpires(t *testing.T) {
|
||||
t.Parallel()
|
||||
c := newLoginCookie([]byte("server-secret-key"))
|
||||
|
||||
Reference in New Issue
Block a user