diff --git a/sdk/auth/oauth/oauth.go b/sdk/auth/oauth/oauth.go index 9f3bd3a46d..56aacf5471 100644 --- a/sdk/auth/oauth/oauth.go +++ b/sdk/auth/oauth/oauth.go @@ -209,6 +209,12 @@ func getDPoPAssertion(dpopJWK jwk.Key, method string, endpoint string, nonce str panic(err) } + slog.Debug( + "building dpop assertion", + slog.String("htm", method), + slog.String("htu", endpoint), + slog.Bool("with_nonce", nonce != ""), + ) tokenBuilder := jwt.NewBuilder(). Claim("jti", uuid.NewString()). Claim("htm", method). diff --git a/sdk/auth/token_adding_interceptor.go b/sdk/auth/token_adding_interceptor.go index afb9a4749c..34d9166a03 100644 --- a/sdk/auth/token_adding_interceptor.go +++ b/sdk/auth/token_adding_interceptor.go @@ -64,6 +64,11 @@ func (i TokenAddingInterceptor) AddCredentials( return status.Error(codes.Unauthenticated, err.Error()) } + slog.DebugContext( + ctx, "preparing dpop for grpc request", + slog.String("grpc_method", method), + slog.String("dpop_htm", http.MethodPost), + ) dpopTok, err := i.GetDPoPToken(method, http.MethodPost, string(accessToken)) if err == nil { newMetadata = append(newMetadata, "DPoP", dpopTok) @@ -99,6 +104,12 @@ func (i TokenAddingInterceptor) AddCredentialsConnect() connect.UnaryInterceptor req.Header().Set("Authorization", fmt.Sprintf("DPoP %s", accessToken)) // Add DPoP header if possible + slog.DebugContext( + ctx, "preparing dpop for connect request", + slog.String("procedure", req.Spec().Procedure), + slog.String("dpop_htm", http.MethodPost), + slog.Any("stream_type", req.Spec().StreamType), + ) dpopTok, err := i.GetDPoPToken(req.Spec().Procedure, http.MethodPost, string(accessToken)) if err == nil { req.Header().Set("DPoP", dpopTok) @@ -146,6 +157,11 @@ func (i TokenAddingInterceptor) GetDPoPToken(path, method, accessToken string) ( h.Write([]byte(accessToken)) ath := h.Sum(nil) + slog.Debug( + "building dpop token", + slog.String("htm", method), + slog.String("htu", path), + ) dpopTok, err := jwt.NewBuilder(). Claim("htu", path). Claim("htm", method). diff --git a/service/cmd/keycloak_data.yaml b/service/cmd/keycloak_data.yaml index ef54501c48..b1e5d76f38 100644 --- a/service/cmd/keycloak_data.yaml +++ b/service/cmd/keycloak_data.yaml @@ -36,6 +36,19 @@ realms: sa_realm_roles: - opentdf-admin copies: 10 + - client: + clientID: opentdf-dpop + enabled: true + name: opentdf-dpop + serviceAccountsEnabled: true + clientAuthenticatorType: client-secret + secret: secret + attributes: + dpop.bound.access.tokens: "true" + protocolMappers: + - *customAudMapper + sa_realm_roles: + - opentdf-admin - client: clientID: opentdf-sdk enabled: true diff --git a/service/internal/auth/authn.go b/service/internal/auth/authn.go index 62b3ec3c88..5bd9cf117b 100644 --- a/service/internal/auth/authn.go +++ b/service/internal/auth/authn.go @@ -3,8 +3,10 @@ package auth import ( "context" "crypto" + "crypto/rand" "crypto/sha256" "encoding/base64" + "encoding/hex" "encoding/json" "errors" "fmt" @@ -13,7 +15,10 @@ import ( "net/url" "path" "slices" + "sort" "strings" + "sync" + "sync/atomic" "time" "connectrpc.com/connect" @@ -73,6 +78,7 @@ var ( const ( refreshInterval = 15 * time.Minute dpopJWTType = "dpop+jwt" + dpopNonceBytes = 16 ActionRead = "read" ActionWrite = "write" ActionDelete = "delete" @@ -82,7 +88,8 @@ const ( // Authentication holds a jwks cache and information about the openid configuration type Authentication struct { - enforceDPoP bool + enforceDPoP bool + strictDPoPHTU bool // tokenVerifier validates access tokens against the configured IdP. tokenVerifier *TokenVerifier // openidConfigurations holds the openid configuration for the issuer @@ -95,16 +102,102 @@ type Authentication struct { ipcReauthRoutes []string // Custom Logger logger *logger.Logger + // DPoP nonce management + dpopNonceManager *dpopNonceManager + // dpopReplayCache rejects replayed DPoP proofs by `jti` + dpopReplayCache *dpopReplayCache // Used for testing _testCheckTokenFunc func(ctx context.Context, authHeader []string, dpopInfo receiverInfo, dpopHeader []string) (jwt.Token, context.Context, error) } +// nonceState is an immutable snapshot of the active and previous nonces, +// swapped atomically on rotation. +type nonceState struct { + currentNonce string + previousNonce string + rotatedAt time.Time +} + +// dpopNonceManager manages server-issued DPoP nonces per RFC 9449 §8 +type dpopNonceManager struct { + state atomic.Pointer[nonceState] + mu sync.Mutex // serializes rotation only + expiration time.Duration + requireNonce bool +} + +func newDPoPNonceManager(requireNonce bool, expiration time.Duration) *dpopNonceManager { + nm := &dpopNonceManager{ + requireNonce: requireNonce, + expiration: expiration, + } + nm.state.Store(&nonceState{}) + if requireNonce { + nm.rotate() + } + return nm +} + +func (nm *dpopNonceManager) storeRotated(s *nonceState) { + nonce := make([]byte, dpopNonceBytes) + if _, err := rand.Read(nonce); err != nil { + panic(fmt.Sprintf("failed to generate nonce: %v", err)) + } + nm.state.Store(&nonceState{ + previousNonce: s.currentNonce, + currentNonce: hex.EncodeToString(nonce), + rotatedAt: time.Now(), + }) +} + +func (nm *dpopNonceManager) rotate() { + nm.mu.Lock() + defer nm.mu.Unlock() + nm.storeRotated(nm.state.Load()) +} + +func (nm *dpopNonceManager) getCurrentNonce() string { + s := nm.state.Load() + if time.Since(s.rotatedAt) <= nm.expiration { + return s.currentNonce + } + + nm.mu.Lock() + defer nm.mu.Unlock() + // Double-check after acquiring the lock to prevent concurrent double-rotation. + s = nm.state.Load() + if time.Since(s.rotatedAt) > nm.expiration { + nm.storeRotated(s) + s = nm.state.Load() + } + return s.currentNonce +} + +func (nm *dpopNonceManager) validateNonce(nonce string) bool { + if !nm.requireNonce { + return true + } + s := nm.state.Load() + return nonce != "" && (nonce == s.currentNonce || nonce == s.previousNonce) +} + // Creates new authN which is used to verify tokens for a set of given issuers func NewAuthenticator(ctx context.Context, cfg Config, logger *logger.Logger, wellknownRegistration func(namespace string, config any) error) (*Authentication, error) { + // Remember DPoP `jti` values for the acceptance window so a captured proof + // cannot be replayed; a proof older than DPoPSkew is rejected by the `iat` + // check, so the cache TTL tracks that window. + replayTTL := cfg.DPoPSkew + if replayTTL <= 0 { + replayTTL = time.Hour + } + a := &Authentication{ - enforceDPoP: cfg.EnforceDPoP, - logger: logger, + enforceDPoP: cfg.EnforceDPoP, + strictDPoPHTU: cfg.DPoP.StrictHTU, + logger: logger, + dpopNonceManager: newDPoPNonceManager(cfg.DPoP.RequireNonce, cfg.DPoP.NonceExpiration), + dpopReplayCache: newDPoPReplayCache(replayTTL), } tokenVerifier, oidcConfig, err := newTokenVerifier(ctx, cfg.AuthNConfig, a.logger) @@ -156,6 +249,31 @@ func NewAuthenticator(ctx context.Context, cfg Config, logger *logger.Logger, we logger.Warn("failed to register platform idp information", slog.Any("error", err)) } + // Register DPoP support feature + if err := wellknownRegistration("supports_dpop", true); err != nil { + logger.Warn("failed to register dpop support", slog.Any("error", err)) + } + + // Register supported DPoP JWT signing algorithms (RFC 9449 §5.1 dpop_signing_alg_values_supported) + // structpb.NewStruct rejects []string — convert to []any so each element goes through + // structpb.NewValue's accepted-type list. + supportedAlgsStr := make([]string, 0, len(allowedSignatureAlgorithms)) + for alg := range allowedSignatureAlgorithms { + supportedAlgsStr = append(supportedAlgsStr, alg.String()) + } + sort.Strings(supportedAlgsStr) + supportedAlgs := make([]any, len(supportedAlgsStr)) + for i, s := range supportedAlgsStr { + supportedAlgs[i] = s + } + if err := wellknownRegistration("dpop_signing_alg_values_supported", supportedAlgs); err != nil { + logger.Warn("failed to register dpop supported alg values", slog.Any("error", err)) + } + + if err := wellknownRegistration("dpop_nonce_required", cfg.DPoP.RequireNonce); err != nil { + logger.Warn("failed to register dpop nonce required", slog.Any("error", err)) + } + return a, nil } @@ -166,6 +284,25 @@ type receiverInfo struct { m []string } +// DPoPNonceError indicates a missing or expired nonce that the client should retry with a fresh one. +type DPoPNonceError struct { + Message string +} + +func (e *DPoPNonceError) Error() string { + return e.Message +} + +// DPoPNonceMalformedError indicates the nonce claim was present but had an invalid type or format. +// Unlike DPoPNonceError, this is not retryable — the client sent a malformed proof. +type DPoPNonceMalformedError struct { + Message string +} + +func (e *DPoPNonceMalformedError) Error() string { + return e.Message +} + func normalizeURL(o string, u *url.URL) string { // Currently this does not do a full normatlization ou, err := url.Parse(o) @@ -176,11 +313,46 @@ func normalizeURL(o string, u *url.URL) string { return ou.String() } +// matchHTU reports whether the htu claim from a DPoP JWT is acceptable. +// +// In strict mode the origin (scheme+host) must be present; a path-only htu is +// rejected outright. When the origin is present it must match the origin of at +// least one URI in acceptable (both http and https are tried). +// +// In loose mode a path-only htu (no scheme, no host) is accepted when its path +// matches the path component of any URI in acceptable. When the origin is +// present the same origin+path matching applies as in strict mode. +func matchHTU(received string, acceptable []string, strict bool) bool { + u, err := url.Parse(received) + if err != nil { + return false + } + if u.Scheme == "" && u.Host == "" { + // Path-only htu. + if strict { + return false + } + for _, a := range acceptable { + au, err := url.Parse(a) + if err != nil { + continue + } + if au.Path == u.Path { + return true + } + } + return false + } + // Full URL: must match one of the acceptable URIs exactly. + return slices.Contains(acceptable, received) +} + // verifyTokenHandler is a http handler that verifies the token func (a Authentication) MuxHandler(handler http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { publicRoute := slices.ContainsFunc(a.publicRoutes, a.isPublicRoute(r.URL.Path)) //nolint:contextcheck // There is no way to pass a context here - r = r.WithContext(ctxAuth.ContextWithPublicRoute(r.Context(), publicRoute)) + ctxWithRoute := ctxAuth.ContextWithPublicRoute(r.Context(), publicRoute) + r = r.WithContext(ctxWithRoute) if publicRoute { handler.ServeHTTP(w, r) return @@ -204,12 +376,29 @@ func (a Authentication) MuxHandler(handler http.Handler) http.Handler { origin = "http://" + strings.TrimSuffix(origin, ":80") } } - accessTok, ctx, err := a.checkToken(r.Context(), header, receiverInfo{ + ri := receiverInfo{ u: []string{normalizeURL(origin, r.URL)}, m: []string{r.Method}, - }, dp) + } + log.DebugContext( + ctxWithRoute, "dpop receiverInfo set (http handler)", + slog.Any("expected_htm", ri.m), + slog.Any("expected_htu", ri.u), + slog.String("request_method", r.Method), + ) + accessTok, ctxWithAuthX, err := a.checkToken(ctxWithRoute, header, ri, dp) if err != nil { - log.WarnContext(ctx, + // Check if this is a nonce error requiring a challenge + var nonceErr *DPoPNonceError + if errors.As(err, &nonceErr) { + nonce := a.dpopNonceManager.getCurrentNonce() + w.Header().Set("DPoP-Nonce", nonce) + w.Header().Set("WWW-Authenticate", `DPoP error="use_dpop_nonce"`) + http.Error(w, "unauthenticated", http.StatusUnauthorized) + return + } + log.WarnContext( + ctxWithAuthX, "unauthenticated", slog.Any("error", err), slog.Any("dpop", dp), @@ -218,10 +407,15 @@ func (a Authentication) MuxHandler(handler http.Handler) http.Handler { return } - clientID, err := a.getClientIDFromToken(ctx, accessTok) + // Always send fresh nonce in successful responses when nonces are enabled + if a.dpopNonceManager.requireNonce { + w.Header().Set("DPoP-Nonce", a.dpopNonceManager.getCurrentNonce()) + } + + clientID, err := a.getClientIDFromToken(ctxWithAuthX, accessTok) if err != nil { log.WarnContext( - ctx, + ctxWithAuthX, "could not determine client ID from token", slog.Any("err", err), ) @@ -229,8 +423,8 @@ func (a Authentication) MuxHandler(handler http.Handler) http.Handler { log = log. With("client_id", clientID). With("configured_client_id_claim_name", a.oidcConfiguration.Policy.ClientIDClaim) - ctx = ctxAuth.EnrichIncomingContextMetadataWithAuthn(ctx, log, clientID) - ctx = authz.ContextWithClientID(ctx, clientID) + ctxWithAuthX = ctxAuth.EnrichIncomingContextMetadataWithAuthn(ctxWithAuthX, log, clientID) + ctxWithAuthX = authz.ContextWithClientID(ctxWithAuthX, clientID) } // Check if the token is allowed to access the resource @@ -250,9 +444,9 @@ func (a Authentication) MuxHandler(handler http.Handler) http.Handler { Resource: r.URL.Path, Action: action, } - ctx, err = a.enforcer.ContextWithClaims(ctx, accessTok, roleReq) + ctxWithClaims, err := a.enforcer.ContextWithClaims(ctxWithAuthX, accessTok, roleReq) if err != nil { - log.WarnContext(r.Context(), "role provider error", slog.Any("error", err)) //nolint:contextcheck // request context is already derived with public route state above. + log.WarnContext(ctxWithClaims, "role provider error", slog.Any("error", err)) if errors.Is(err, ErrPermissionDenied) { http.Error(w, "permission denied", http.StatusForbidden) return @@ -260,10 +454,10 @@ func (a Authentication) MuxHandler(handler http.Handler) http.Handler { http.Error(w, "internal server error", http.StatusInternalServerError) return } - if result, err := a.enforcer.Enforce(ctx, accessTok, roleReq); err != nil { + if result, err := a.enforcer.Enforce(ctxWithClaims, accessTok, roleReq); err != nil { if errors.Is(err, ErrPermissionDenied) { log.WarnContext( - ctx, + ctxWithClaims, "permission denied", permissionDeniedLogAttrs(accessTok, result.CasbinAuthz, err)..., ) @@ -274,7 +468,7 @@ func (a Authentication) MuxHandler(handler http.Handler) http.Handler { return } else if !result.Allowed { log.WarnContext( - ctx, + ctxWithClaims, "permission denied", permissionDeniedLogAttrs(accessTok, result.CasbinAuthz, nil)..., ) @@ -282,7 +476,7 @@ func (a Authentication) MuxHandler(handler http.Handler) http.Handler { return } - r = r.WithContext(ctx) + r = r.WithContext(ctxWithClaims) handler.ServeHTTP(w, r) }) } @@ -303,10 +497,22 @@ func (a Authentication) ConnectAuthNInterceptor() connect.UnaryInterceptorFunc { log := a.logger + procedure := req.Spec().Procedure + host := req.Header().Get("Host") ri := receiverInfo{ - u: []string{req.Spec().Procedure}, - m: []string{http.MethodPost}, + u: []string{ + "http://" + host + procedure, + "https://" + host + procedure, + }, + m: []string{req.HTTPMethod()}, } + log.DebugContext( + ctx, "dpop receiverInfo set (connect interceptor)", + slog.Any("expected_htm", ri.m), + slog.Any("expected_htu", ri.u), + slog.String("procedure", procedure), + slog.String("connect_http_method", req.HTTPMethod()), + ) header := req.Header()["Authorization"] if len(header) < 1 { @@ -320,9 +526,36 @@ func (a Authentication) ConnectAuthNInterceptor() connect.UnaryInterceptorFunc { req.Header()["Dpop"], ) if err != nil { + // Check if this is a nonce error requiring a challenge + var nonceErr *DPoPNonceError + if errors.As(err, &nonceErr) { + nonce := a.dpopNonceManager.getCurrentNonce() + connectErr := connect.NewError(connect.CodeUnauthenticated, errors.New("unauthenticated")) + connectErr.Meta().Set("DPoP-Nonce", nonce) + connectErr.Meta().Set("WWW-Authenticate", `DPoP error="use_dpop_nonce"`) + return nil, connectErr + } return nil, connect.NewError(connect.CodeUnauthenticated, errors.New("unauthenticated")) } + // Always send fresh nonce in successful responses when nonces are enabled. + // Wrap next so the DPoP-Nonce header is set on the response, not the outgoing client context. + if a.dpopNonceManager.requireNonce { + originalNext := next + next = func(ctx context.Context, req connect.AnyRequest) (connect.AnyResponse, error) { + res, err := originalNext(ctx, req) + if err != nil { + var connectErr *connect.Error + if errors.As(err, &connectErr) { + connectErr.Meta().Set("DPoP-Nonce", a.dpopNonceManager.getCurrentNonce()) + } + return nil, err + } + res.Header().Set("DPoP-Nonce", a.dpopNonceManager.getCurrentNonce()) + return res, nil + } + } + clientID, err := a.getClientIDFromToken(ctxWithJWK, token) if err != nil { log.WarnContext( @@ -404,7 +637,8 @@ func permissionDeniedLogAttrs(token jwt.Token, casbinAuthz CasbinAuthzLog, err e attrs := []any{slog.String("azp", token.Subject())} if casbinAuthz.ConfiguredGroupsClaim != "" || casbinAuthz.SubjectGroups != nil { - attrs = append(attrs, slog.Group("casbin_authz", + attrs = append(attrs, slog.Group( + "casbin_authz", slog.String("configured_groups_claim", casbinAuthz.ConfiguredGroupsClaim), slog.Any("subject_groups", casbinAuthz.SubjectGroups), )) @@ -526,14 +760,19 @@ func (a *Authentication) checkToken(ctx context.Context, authHeader []string, dp return a._testCheckTokenFunc(ctx, authHeader, dpopInfo, dpopHeader) } - var tokenRaw string + var ( + tokenRaw string + authScheme string + ) // If we don't get a DPoP/Bearer token type, we can't proceed switch { case strings.HasPrefix(authHeader[0], "DPoP "): tokenRaw = strings.TrimPrefix(authHeader[0], "DPoP ") + authScheme = "DPoP" case strings.HasPrefix(authHeader[0], "Bearer "): tokenRaw = strings.TrimPrefix(authHeader[0], "Bearer ") + authScheme = "Bearer" default: a.logger.WarnContext(ctx, "failed to validate authentication header: not of type bearer or dpop", slog.String("header", authHeader[0])) return nil, nil, errors.New("not of type bearer or dpop") @@ -557,6 +796,12 @@ func (a *Authentication) checkToken(ctx context.Context, authHeader []string, dp } _, tokenHasCNF := accessToken.Get("cnf") + if tokenHasCNF && authScheme == "Bearer" { + // RFC 9449 §7.1: DPoP-bound access tokens MUST be presented under the "DPoP" + // Authorization scheme. Warn for now to surface non-compliant SDKs; will be + // promoted to a hard reject in a future release once all SDKs are compliant. + a.logger.WarnContext(ctx, "DPoP-bound access token presented under Bearer Authorization scheme; per RFC 9449 §7.1 the DPoP scheme is required") + } if !tokenHasCNF && !a.enforceDPoP { // this condition is not quite tight because it's possible that the `cnf` claim may // come from token introspection @@ -565,7 +810,12 @@ func (a *Authentication) checkToken(ctx context.Context, authHeader []string, dp } dpopKey, err := a.validateDPoP(accessToken, tokenRaw, dpopInfo, dpopHeader) if err != nil { - a.logger.Warn("failed to validate dpop", slog.Any("err", err)) + var nonceErr *DPoPNonceError + if errors.As(err, &nonceErr) { + a.logger.DebugContext(ctx, "dpop nonce challenge issued", slog.String("reason", nonceErr.Message)) + } else { + a.logger.WarnContext(ctx, "failed to validate dpop", slog.Any("err", err)) + } return nil, nil, err } ctx = ctxAuth.ContextWithAuthNInfo(ctx, dpopKey, accessToken, tokenRaw) @@ -583,7 +833,7 @@ func (a Authentication) validateDPoP(accessToken jwt.Token, acessTokenRaw string return nil, errors.New("missing `cnf` claim in access token") } - cnfDict, ok := cnf.(map[string]interface{}) + cnfDict, ok := cnf.(map[string]any) if !ok { return nil, errors.New("got `cnf` in an invalid format") } @@ -664,6 +914,12 @@ func (a Authentication) validateDPoP(accessToken jwt.Token, acessTokenRaw string return nil, errors.New("`htm` claim invalid format in DPoP JWT") } + a.logger.Debug( + "dpop htm validation", + slog.String("received_htm", htm), + slog.Any("expected_htm", dpopInfo.m), + slog.Bool("match", slices.Contains(dpopInfo.m, htm)), + ) if !slices.Contains(dpopInfo.m, htm) { return nil, fmt.Errorf("incorrect `htm` claim in DPoP JWT; received [%v], but should match [%v]", htm, dpopInfo.m) } @@ -677,7 +933,7 @@ func (a Authentication) validateDPoP(accessToken jwt.Token, acessTokenRaw string return nil, errors.New("`htu` claim invalid format in DPoP JWT") } - if !slices.Contains(dpopInfo.u, htu) { + if !matchHTU(htu, dpopInfo.u, a.strictDPoPHTU) { return nil, fmt.Errorf("incorrect `htu` claim in DPoP JWT; received [%v], but should match [%v]", htu, dpopInfo.u) } @@ -691,6 +947,37 @@ func (a Authentication) validateDPoP(accessToken jwt.Token, acessTokenRaw string if ath != base64.URLEncoding.WithPadding(base64.NoPadding).EncodeToString(h.Sum(nil)) { return nil, errors.New("incorrect `ath` claim in DPoP JWT") } + + // Validate nonce if required + if a.dpopNonceManager.requireNonce { + nonceI, hasNonce := dpopToken.Get("nonce") + if !hasNonce { + return nil, &DPoPNonceError{Message: "missing `nonce` claim in DPoP JWT"} + } + nonce, nonceOk := nonceI.(string) + if !nonceOk { + return nil, &DPoPNonceMalformedError{Message: "`nonce` claim invalid format in DPoP JWT"} + } + if !a.dpopNonceManager.validateNonce(nonce) { + return nil, &DPoPNonceError{Message: "invalid or expired `nonce` claim in DPoP JWT"} + } + } + + // Replay protection (RFC 9449 §11.1): reject a proof whose `jti` has already + // been seen within the acceptance window. Checked last so only otherwise-valid + // proofs populate the cache, preventing an attacker from poisoning it. + jtiI, ok := dpopToken.Get("jti") + if !ok { + return nil, errors.New("missing `jti` claim in DPoP JWT") + } + jti, ok := jtiI.(string) + if !ok { + return nil, errors.New("`jti` claim invalid format in DPoP JWT") + } + if a.dpopReplayCache.observe(jti, time.Now()) { + return nil, errors.New("DPoP proof replay detected; `jti` already used") + } + return dpopKey, nil } @@ -698,14 +985,16 @@ func (a Authentication) isPublicRoute(path string) func(string) bool { return func(route string) bool { matched, err := doublestar.Match(route, path) if err != nil { - a.logger.Warn("error matching route", + a.logger.Warn( + "error matching route", slog.String("route", route), slog.String("path", path), slog.Any("error", err), ) return false } - a.logger.Trace("matching route", + a.logger.Trace( + "matching route", slog.String("route", route), slog.String("path", path), slog.Bool("matched", matched), @@ -725,10 +1014,21 @@ func (a Authentication) ipcReauthCheck(ctx context.Context, path string, header } // Validate the token and create a JWT token - token, ctxWithJWK, err := a.checkToken(ctx, authHeader, receiverInfo{ - u: []string{path}, + ipcHost := header.Get("Host") + ri := receiverInfo{ + u: []string{ + "http://" + ipcHost + path, + "https://" + ipcHost + path, + }, m: []string{http.MethodPost}, - }, header["Dpop"]) + } + a.logger.DebugContext( + ctx, "dpop receiverInfo set (ipc reauth)", + slog.Any("expected_htm", ri.m), + slog.Any("expected_htu", ri.u), + slog.String("path", path), + ) + token, ctxWithJWK, err := a.checkToken(ctx, authHeader, ri, header["Dpop"]) if err != nil { return nil, connect.NewError(connect.CodeUnauthenticated, errors.New("unauthenticated")) } diff --git a/service/internal/auth/authn_test.go b/service/internal/auth/authn_test.go index 4052b7e97b..6194103750 100644 --- a/service/internal/auth/authn_test.go +++ b/service/internal/auth/authn_test.go @@ -459,7 +459,8 @@ func (s *AuthSuite) Test_ConnectAuthNInterceptor_RoleProviderErrorReturnsInterna type authnTestRequest struct { *connect.Request[kas.RewrapRequest] - procedure string + procedure string + httpMethod string } func (r *authnTestRequest) Spec() connect.Spec { @@ -474,6 +475,13 @@ func (r *authnTestRequest) Any() any { return r.Msg } +func (r *authnTestRequest) HTTPMethod() string { + if r.httpMethod == "" { + return http.MethodPost + } + return r.httpMethod +} + func (s *AuthSuite) Test_ConnectAuthNInterceptor_SetsPublicRouteContextForChainedMiddleware() { var ( middlewarePublicRoute bool @@ -718,6 +726,7 @@ func (s *AuthSuite) TestInvalid_DPoP_Cases() { {dpopPublic, otherKey, signedTok, jwa.RS256, "dpop+jwt", http.MethodPost, "/a/path", "", time.Now(), "failed to verify signature on DPoP JWT"}, {dpopPublic, dpopKey, signedTok, jwa.RS256, "dpop+jwt", http.MethodPost, "/a/different/path", "", time.Now(), "incorrect `htu` claim in DPoP JWT"}, {dpopPublic, dpopKey, signedTok, jwa.RS256, "dpop+jwt", "POSTERS", "/a/path", "", time.Now(), "incorrect `htm` claim in DPoP JWT"}, + {dpopPublic, dpopKey, signedTok, jwa.RS256, "dpop+jwt", http.MethodGet, "/a/path", "", time.Now(), "incorrect `htm` claim in DPoP JWT"}, {dpopPublic, dpopKey, signedTok, jwa.RS256, "dpop+jwt", http.MethodPost, "/a/path", "bad ath", time.Now(), "incorrect `ath` claim in DPoP JWT"}, {dpopPublic, dpopKey, signedTok, jwa.RS256, "dpop+jwt", http.MethodPost, "/a/path", "bad iat", time.Now().Add(2 * time.Hour), "\"iat\" not satisfied"}, { @@ -749,6 +758,190 @@ func (s *AuthSuite) TestInvalid_DPoP_Cases() { } } +// Test_CheckToken_AcceptsDPoP_GET covers the ConnectRPC "Get requests" / +// idempotent-RPC scenario (used by the Java connect-go client's Hasan +// extension): the DPoP proof carries htm:"GET" and the server side must accept +// it when the actual request method is GET. +func (s *AuthSuite) Test_CheckToken_AcceptsDPoP_GET() { + dpopRaw, err := rsa.GenerateKey(rand.Reader, 2048) + s.Require().NoError(err) + dpopKey, err := jwk.FromRaw(dpopRaw) + s.Require().NoError(err) + s.Require().NoError(dpopKey.Set(jwk.AlgorithmKey, jwa.RS256)) + dpopPublic, err := dpopKey.PublicKey() + s.Require().NoError(err) + + tok := jwt.New() + s.Require().NoError(tok.Set(jwt.ExpirationKey, time.Now().Add(time.Hour))) + s.Require().NoError(tok.Set("iss", s.server.URL)) + s.Require().NoError(tok.Set("aud", "test")) + s.Require().NoError(tok.Set("cid", "client-get")) + thumbprint, err := dpopKey.Thumbprint(crypto.SHA256) + s.Require().NoError(err) + cnf := map[string]string{"jkt": base64.URLEncoding.WithPadding(base64.NoPadding).EncodeToString(thumbprint)} + s.Require().NoError(tok.Set("cnf", cnf)) + signedTok, err := jwt.Sign(tok, jwt.WithKey(jwa.RS256, s.key)) + s.Require().NoError(err) + + dpopToken := makeDPoPToken(s.T(), dpopTestCase{ + key: dpopPublic, + actualSigningKey: dpopKey, + accessToken: signedTok, + alg: jwa.RS256, + typ: "dpop+jwt", + htm: http.MethodGet, + htu: "https://localhost:8080/kas.AccessService/PublicKey", + iat: time.Now(), + }) + + _, _, err = s.auth.checkToken( + context.Background(), + []string{"DPoP " + string(signedTok)}, + receiverInfo{ + u: []string{ + "http://localhost:8080/kas.AccessService/PublicKey", + "https://localhost:8080/kas.AccessService/PublicKey", + }, + m: []string{http.MethodGet}, + }, + []string{dpopToken}, + ) + s.Require().NoError(err) +} + +// Test_CheckToken_RejectsReplayedDPoP verifies that reusing the same DPoP proof +// (identical `jti`) within the acceptance window is rejected per RFC 9449 §11.1, +// even though the first presentation succeeds. +func (s *AuthSuite) Test_CheckToken_RejectsReplayedDPoP() { + dpopRaw, err := rsa.GenerateKey(rand.Reader, 2048) + s.Require().NoError(err) + dpopKey, err := jwk.FromRaw(dpopRaw) + s.Require().NoError(err) + s.Require().NoError(dpopKey.Set(jwk.AlgorithmKey, jwa.RS256)) + dpopPublic, err := dpopKey.PublicKey() + s.Require().NoError(err) + + tok := jwt.New() + s.Require().NoError(tok.Set(jwt.ExpirationKey, time.Now().Add(time.Hour))) + s.Require().NoError(tok.Set("iss", s.server.URL)) + s.Require().NoError(tok.Set("aud", "test")) + s.Require().NoError(tok.Set("cid", "client-replay")) + thumbprint, err := dpopKey.Thumbprint(crypto.SHA256) + s.Require().NoError(err) + cnf := map[string]string{"jkt": base64.URLEncoding.WithPadding(base64.NoPadding).EncodeToString(thumbprint)} + s.Require().NoError(tok.Set("cnf", cnf)) + signedTok, err := jwt.Sign(tok, jwt.WithKey(jwa.RS256, s.key)) + s.Require().NoError(err) + + dpopToken := makeDPoPToken(s.T(), dpopTestCase{ + key: dpopPublic, + actualSigningKey: dpopKey, + accessToken: signedTok, + alg: jwa.RS256, + typ: "dpop+jwt", + htm: http.MethodPost, + htu: "/a/path", + iat: time.Now(), + }) + + ri := receiverInfo{u: []string{"/a/path"}, m: []string{http.MethodPost}} + + _, _, err = s.auth.checkToken(context.Background(), []string{"DPoP " + string(signedTok)}, ri, []string{dpopToken}) + s.Require().NoError(err, "first presentation of the proof should succeed") + + _, _, err = s.auth.checkToken(context.Background(), []string{"DPoP " + string(signedTok)}, ri, []string{dpopToken}) + s.Require().Error(err, "replaying the same proof should be rejected") + s.Contains(err.Error(), "replay") +} + +// Test_ConnectAuthNInterceptor_PropagatesHTTPMethod verifies that the Connect +// interceptor forwards the actual HTTP method (from req.HTTPMethod()) into +// receiverInfo.m, instead of hardcoding POST. This is what makes DPoP +// validation succeed for idempotent RPCs invoked via HTTP GET (ConnectRPC's +// "Get requests" feature used by the Java client's Hasan extension). +func (s *AuthSuite) Test_ConnectAuthNInterceptor_PropagatesHTTPMethod() { + tok := jwt.New() + s.Require().NoError(tok.Set(jwt.ExpirationKey, time.Now().Add(time.Hour))) + s.Require().NoError(tok.Set("iss", s.server.URL)) + s.Require().NoError(tok.Set("aud", "test")) + s.Require().NoError(tok.Set("azp", "client-get")) + s.Require().NoError(tok.Set("realm_access", map[string][]string{"roles": {"opentdf-standard"}})) + signedTok, err := jwt.Sign(tok, jwt.WithKey(jwa.RS256, s.key)) + s.Require().NoError(err) + + for _, method := range []string{http.MethodGet, http.MethodPost} { + s.Run(method, func() { + var capturedMethods []string + s.auth._testCheckTokenFunc = func(ctx context.Context, _ []string, ri receiverInfo, _ []string) (jwt.Token, context.Context, error) { + capturedMethods = ri.m + return tok, ctx, nil + } + s.T().Cleanup(func() { s.auth._testCheckTokenFunc = nil }) + + interceptor := s.auth.ConnectAuthNInterceptor() + called := false + next := func(_ context.Context, _ connect.AnyRequest) (connect.AnyResponse, error) { + called = true + return connect.NewResponse(&kas.RewrapResponse{}), nil + } + req := &authnTestRequest{ + Request: connect.NewRequest(&kas.RewrapRequest{}), + procedure: "/kas.AccessService/Rewrap", + httpMethod: method, + } + req.Header().Set("Authorization", "DPoP "+string(signedTok)) + + _, err := interceptor(next)(s.T().Context(), req) + s.Require().NoError(err) + s.True(called) + s.Equal([]string{method}, capturedMethods) + }) + } +} + +func TestMatchHTU(t *testing.T) { + full := []string{ + "http://localhost:8080/svc/Method", + "https://localhost:8080/svc/Method", + } + pathOnly := []string{"/svc/Method"} + + tests := []struct { + name string + received string + acceptable []string + strict bool + want bool + }{ + // Loose mode: path-only htu accepted when path matches + {"loose/path-only match", "/svc/Method", full, false, true}, + {"loose/path-only match against path-only acceptable", "/svc/Method", pathOnly, false, true}, + {"loose/path-only mismatch", "/svc/Other", full, false, false}, + // Loose mode: full URL accepted when it matches exactly + {"loose/full http match", "http://localhost:8080/svc/Method", full, false, true}, + {"loose/full https match", "https://localhost:8080/svc/Method", full, false, true}, + {"loose/full wrong path", "http://localhost:8080/svc/Other", full, false, false}, + {"loose/full wrong host", "http://other:8080/svc/Method", full, false, false}, + // Strict mode: path-only htu always rejected + {"strict/path-only rejected", "/svc/Method", full, true, false}, + {"strict/path-only rejected against path-only acceptable", "/svc/Method", pathOnly, true, false}, + // Strict mode: full URL accepted when it matches exactly + {"strict/full http match", "http://localhost:8080/svc/Method", full, true, true}, + {"strict/full https match", "https://localhost:8080/svc/Method", full, true, true}, + {"strict/full wrong path", "http://localhost:8080/svc/Other", full, true, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := matchHTU(tt.received, tt.acceptable, tt.strict) + if got != tt.want { + t.Errorf("matchHTU(%q, %v, strict=%v) = %v, want %v", + tt.received, tt.acceptable, tt.strict, got, tt.want) + } + }) + } +} + func (s *AuthSuite) TestDPoPEndToEnd_GRPC() { dpopKeyRaw, err := rsa.GenerateKey(rand.Reader, 2048) s.Require().NoError(err) @@ -879,9 +1072,10 @@ func (s *AuthSuite) Test_Allowing_Auth_With_No_DPoP() { } config := Config{} config.AuthNConfig = authnConfig - auth, err := NewAuthenticator(context.Background(), config, &logger.Logger{ - Logger: slog.New(slog.Default().Handler()), - }, + auth, err := NewAuthenticator( + context.Background(), config, &logger.Logger{ + Logger: slog.New(slog.Default().Handler()), + }, func(_ string, _ any) error { return nil }, ) diff --git a/service/internal/auth/config.go b/service/internal/auth/config.go index 3a449e30d9..06f04390c4 100644 --- a/service/internal/auth/config.go +++ b/service/internal/auth/config.go @@ -31,6 +31,23 @@ type AuthNConfig struct { //nolint:revive // AuthNConfig is a valid name CacheRefresh string `mapstructure:"cache_refresh_interval" json:"cache_refresh_interval"` DPoPSkew time.Duration `mapstructure:"dpopskew" json:"dpopskew" default:"1h"` TokenSkew time.Duration `mapstructure:"skew" json:"skew" default:"1m"` + DPoP DPoPConfig `mapstructure:"dpop" json:"dpop"` +} + +type DPoPConfig struct { + RequireNonce bool `mapstructure:"require_nonce" json:"require_nonce" default:"false"` + NonceExpiration time.Duration `mapstructure:"nonce_expiration" json:"nonce_expiration" default:"5m"` + // StrictHTU requires the htu claim in DPoP JWTs to include the origin + // (scheme + host). When false (default), a path-only htu is accepted as + // long as the path matches, easing SDK skew during rollout. + StrictHTU bool `mapstructure:"strict_htu" json:"strict_htu" default:"false"` +} + +func (c DPoPConfig) Validate() error { + if c.RequireNonce && c.NonceExpiration <= 0 { + return errors.New("auth.dpop.nonce_expiration must be positive when require_nonce is true") + } + return nil } type PolicyConfig struct { @@ -74,5 +91,9 @@ func (c AuthNConfig) validateAuthNConfig(logger *logger.Logger) error { logger.Warn("config Auth.EnforceDPoP is false. DPoP will not be enforced.") } + if err := c.DPoP.Validate(); err != nil { + return err + } + return nil } diff --git a/service/internal/auth/dpop_nonce_test.go b/service/internal/auth/dpop_nonce_test.go new file mode 100644 index 0000000000..97f61d8ae1 --- /dev/null +++ b/service/internal/auth/dpop_nonce_test.go @@ -0,0 +1,308 @@ +package auth + +import ( + "context" + "crypto" + "crypto/rand" + "crypto/rsa" + "crypto/sha256" + "encoding/base64" + "net/http" + "testing" + "time" + + "github.com/lestrrat-go/jwx/v2/jwa" + "github.com/lestrrat-go/jwx/v2/jwk" + "github.com/lestrrat-go/jwx/v2/jws" + "github.com/lestrrat-go/jwx/v2/jwt" + sdkauth "github.com/opentdf/platform/sdk/auth" + "github.com/opentdf/platform/service/logger" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestDPoPNonceManager(t *testing.T) { + t.Run("nonce generation", func(t *testing.T) { + nm := newDPoPNonceManager(true, 5*time.Minute) + nonce1 := nm.getCurrentNonce() + assert.NotEmpty(t, nonce1) + assert.Len(t, nonce1, 32) // 16 bytes hex encoded = 32 chars + }) + + t.Run("nonce rotation", func(t *testing.T) { + nm := newDPoPNonceManager(true, 100*time.Millisecond) + nonce1 := nm.getCurrentNonce() + + time.Sleep(150 * time.Millisecond) + + nonce2 := nm.getCurrentNonce() + assert.NotEqual(t, nonce1, nonce2, "nonce should rotate after expiration") + }) + + t.Run("nonce validation window", func(t *testing.T) { + nm := newDPoPNonceManager(true, 5*time.Minute) + currentNonce := nm.getCurrentNonce() + + assert.True(t, nm.validateNonce(currentNonce)) + + // Rotate to create a previous nonce; both current and previous must be accepted + // to tolerate in-flight requests that received the old nonce just before rotation. + nm.rotate() + newNonce := nm.getCurrentNonce() + + assert.True(t, nm.validateNonce(newNonce), "current nonce should validate") + assert.True(t, nm.validateNonce(currentNonce), "previous nonce should validate") + + // After a second rotation the original nonce is evicted. + nm.rotate() + assert.False(t, nm.validateNonce(currentNonce), "nonce older than previous should not validate") + }) + + t.Run("empty nonce rejected when required", func(t *testing.T) { + nm := newDPoPNonceManager(true, 5*time.Minute) + assert.False(t, nm.validateNonce(""), "empty nonce must not match initial empty previousNonce") + }) + + t.Run("disabled nonces", func(t *testing.T) { + nm := newDPoPNonceManager(false, 5*time.Minute) + assert.True(t, nm.validateNonce("any-random-nonce")) + assert.True(t, nm.validateNonce("")) + }) +} + +func TestDPoPNonceError(t *testing.T) { + t.Run("nonce error message", func(t *testing.T) { + err := &DPoPNonceError{Message: "test error"} + assert.Equal(t, "test error", err.Error()) + }) + + t.Run("nonce error detection via errors.As", func(t *testing.T) { + var err error = &DPoPNonceError{Message: "test"} + var nonceErr *DPoPNonceError + require.ErrorAs(t, err, &nonceErr) + assert.Equal(t, "test", nonceErr.Message) + }) + + t.Run("malformed error message", func(t *testing.T) { + err := &DPoPNonceMalformedError{Message: "bad nonce"} + assert.Equal(t, "bad nonce", err.Error()) + }) + + t.Run("malformed error detection via errors.As", func(t *testing.T) { + var err error = &DPoPNonceMalformedError{Message: "bad nonce"} + var malformedErr *DPoPNonceMalformedError + require.ErrorAs(t, err, &malformedErr) + }) + + t.Run("malformed error does not match DPoPNonceError", func(t *testing.T) { + // Critical: DPoPNonceMalformedError must NOT match DPoPNonceError so handlers + // treat it as a hard rejection rather than issuing a nonce challenge. + var err error = &DPoPNonceMalformedError{Message: "bad nonce"} + var nonceErr *DPoPNonceError + assert.NotErrorAs(t, err, &nonceErr) + }) +} + +func TestDPoPAlgorithmRestrictions(t *testing.T) { + testCases := []struct { + alg jwa.SignatureAlgorithm + allowed bool + }{ + {jwa.RS256, true}, + {jwa.RS384, true}, + {jwa.RS512, true}, + {jwa.ES256, true}, + {jwa.ES384, true}, + {jwa.ES512, true}, + {jwa.PS256, true}, + {jwa.PS384, true}, + {jwa.PS512, true}, + {jwa.HS256, false}, + {jwa.NoSignature, false}, + } + + for _, tc := range testCases { + t.Run(tc.alg.String(), func(t *testing.T) { + _, exists := allowedSignatureAlgorithms[tc.alg] + assert.Equal(t, tc.allowed, exists) + }) + } +} + +// newAuthWithNonce creates an Authentication using the suite's OIDC server with RequireNonce=true. +func (s *AuthSuite) newAuthWithNonce() *Authentication { + auth, err := NewAuthenticator( + context.Background(), + Config{ + AuthNConfig: AuthNConfig{ + EnforceDPoP: true, + Issuer: s.server.URL, + Audience: "test", + DPoPSkew: time.Hour, + TokenSkew: time.Minute, + DPoP: DPoPConfig{ + RequireNonce: true, + NonceExpiration: 5 * time.Minute, + StrictHTU: false, + }, + }, + }, + logger.CreateTestLogger(), + func(_ string, _ any) error { return nil }, + ) + s.Require().NoError(err) + return auth +} + +// makeDPoPBoundAccessToken creates an access token with cnf.jkt bound to dpopKey, +// signed by the suite's OIDC key. +func (s *AuthSuite) makeDPoPBoundAccessToken(dpopKey jwk.Key) []byte { + thumbprint, err := dpopKey.Thumbprint(crypto.SHA256) + s.Require().NoError(err) + jkt := base64.URLEncoding.WithPadding(base64.NoPadding).EncodeToString(thumbprint) + + tok := jwt.New() + s.Require().NoError(tok.Set(jwt.ExpirationKey, time.Now().Add(time.Hour))) + s.Require().NoError(tok.Set("iss", s.server.URL)) + s.Require().NoError(tok.Set("aud", "test")) + s.Require().NoError(tok.Set("cid", "client-nonce-test")) + s.Require().NoError(tok.Set("cnf", map[string]string{"jkt": jkt})) + + signedTok, err := jwt.Sign(tok, jwt.WithKey(jwa.RS256, s.key)) + s.Require().NoError(err) + return signedTok +} + +// makeDPoPProof builds a signed DPoP proof. nonceVal may be nil (omit nonce), a string, +// or any other type (to produce a malformed nonce claim for testing). +func makeDPoPProof(t *testing.T, tc dpopTestCase, nonceVal any) string { + t.Helper() + jtiBytes := make([]byte, sdkauth.JTILength) + _, err := rand.Read(jtiBytes) + require.NoError(t, err) + + headers := jws.NewHeaders() + require.NoError(t, headers.Set(jws.JWKKey, tc.key)) + require.NoError(t, headers.Set(jws.TypeKey, tc.typ)) + require.NoError(t, headers.Set(jws.AlgorithmKey, tc.alg)) + + h := sha256.New() + h.Write(tc.accessToken) + ath := base64.URLEncoding.WithPadding(base64.NoPadding).EncodeToString(h.Sum(nil)) + + b := jwt.NewBuilder(). + Claim("htu", tc.htu). + Claim("htm", tc.htm). + Claim("ath", ath). + Claim("jti", base64.StdEncoding.EncodeToString(jtiBytes)). + IssuedAt(time.Now()) + + if nonceVal != nil { + b = b.Claim("nonce", nonceVal) + } + + dpopTok, err := b.Build() + require.NoError(t, err) + + signedToken, err := jwt.Sign(dpopTok, jwt.WithKey(tc.actualSigningKey.Algorithm(), tc.actualSigningKey, jws.WithProtectedHeaders(headers))) + require.NoError(t, err) + return string(signedToken) +} + +func (s *AuthSuite) newDPoPKeyAndAccessToken() (jwk.Key, jwk.Key, []byte) { + dpopKeyRaw, err := rsa.GenerateKey(rand.Reader, 2048) + s.Require().NoError(err) + dpopKey, err := jwk.FromRaw(dpopKeyRaw) + s.Require().NoError(err) + s.Require().NoError(dpopKey.Set(jwk.AlgorithmKey, jwa.RS256)) + dpopPublic, err := dpopKey.PublicKey() + s.Require().NoError(err) + signedTok := s.makeDPoPBoundAccessToken(dpopKey) + return dpopKey, dpopPublic, signedTok +} + +func (s *AuthSuite) TestDPoP_MissingNonce_Returns_DPoPNonceError() { + auth := s.newAuthWithNonce() + dpopKey, dpopPublic, signedTok := s.newDPoPKeyAndAccessToken() + + dpopToken := makeDPoPProof(s.T(), dpopTestCase{ + key: dpopPublic, actualSigningKey: dpopKey, accessToken: signedTok, + alg: jwa.RS256, typ: dpopJWTType, htm: http.MethodPost, htu: "/a/path", + }, nil) // no nonce claim + + _, _, err := auth.checkToken( + context.Background(), + []string{"DPoP " + string(signedTok)}, + receiverInfo{u: []string{"/a/path"}, m: []string{http.MethodPost}}, + []string{dpopToken}, + ) + + var nonceErr *DPoPNonceError + s.Require().ErrorAs(err, &nonceErr, "missing nonce must return DPoPNonceError so handlers issue a challenge") +} + +func (s *AuthSuite) TestDPoP_ValidNonce_Succeeds() { + auth := s.newAuthWithNonce() + dpopKey, dpopPublic, signedTok := s.newDPoPKeyAndAccessToken() + + nonce := auth.dpopNonceManager.getCurrentNonce() + + dpopToken := makeDPoPProof(s.T(), dpopTestCase{ + key: dpopPublic, actualSigningKey: dpopKey, accessToken: signedTok, + alg: jwa.RS256, typ: dpopJWTType, htm: http.MethodPost, htu: "/a/path", + }, nonce) + + _, _, err := auth.checkToken( + context.Background(), + []string{"DPoP " + string(signedTok)}, + receiverInfo{u: []string{"/a/path"}, m: []string{http.MethodPost}}, + []string{dpopToken}, + ) + + s.Require().NoError(err, "correct nonce should pass validation") +} + +func (s *AuthSuite) TestDPoP_MalformedNonce_Returns_DPoPNonceMalformedError() { + auth := s.newAuthWithNonce() + dpopKey, dpopPublic, signedTok := s.newDPoPKeyAndAccessToken() + + // Integer nonce triggers the type-assertion failure in validateDPoP. + dpopToken := makeDPoPProof(s.T(), dpopTestCase{ + key: dpopPublic, actualSigningKey: dpopKey, accessToken: signedTok, + alg: jwa.RS256, typ: dpopJWTType, htm: http.MethodPost, htu: "/a/path", + }, 42) + + _, _, err := auth.checkToken( + context.Background(), + []string{"DPoP " + string(signedTok)}, + receiverInfo{u: []string{"/a/path"}, m: []string{http.MethodPost}}, + []string{dpopToken}, + ) + + var malformedErr *DPoPNonceMalformedError + s.Require().ErrorAs(err, &malformedErr, "non-string nonce must return DPoPNonceMalformedError, not a retryable DPoPNonceError") + + // Confirm it does NOT match DPoPNonceError, so handlers hard-reject rather than issue a challenge. + var nonceErr *DPoPNonceError + s.Require().NotErrorAs(err, &nonceErr) +} + +func (s *AuthSuite) TestDPoP_WrongNonce_Returns_DPoPNonceError() { + auth := s.newAuthWithNonce() + dpopKey, dpopPublic, signedTok := s.newDPoPKeyAndAccessToken() + + dpopToken := makeDPoPProof(s.T(), dpopTestCase{ + key: dpopPublic, actualSigningKey: dpopKey, accessToken: signedTok, + alg: jwa.RS256, typ: dpopJWTType, htm: http.MethodPost, htu: "/a/path", + }, "not-the-right-nonce") + + _, _, err := auth.checkToken( + context.Background(), + []string{"DPoP " + string(signedTok)}, + receiverInfo{u: []string{"/a/path"}, m: []string{http.MethodPost}}, + []string{dpopToken}, + ) + + var nonceErr *DPoPNonceError + s.Require().ErrorAs(err, &nonceErr, "wrong nonce must return DPoPNonceError so handlers issue a fresh challenge") +} diff --git a/service/internal/auth/dpop_replay.go b/service/internal/auth/dpop_replay.go new file mode 100644 index 0000000000..251c0a37d0 --- /dev/null +++ b/service/internal/auth/dpop_replay.go @@ -0,0 +1,55 @@ +package auth + +import ( + "sync" + "time" +) + +// dpopReplayCache rejects replayed DPoP proofs by their `jti` value within the +// acceptance window, per RFC 9449 §11.1. Entries expire after ttl (set to the +// DPoP `iat` skew): a proof older than that window is already rejected by the +// `iat` freshness check, so the `jti` need not be remembered any longer. +type dpopReplayCache struct { + mu sync.Mutex + seen map[string]time.Time // jti -> expiry + ttl time.Duration + lastGC time.Time +} + +func newDPoPReplayCache(ttl time.Duration) *dpopReplayCache { + return &dpopReplayCache{ + seen: make(map[string]time.Time), + ttl: ttl, + } +} + +// observe records jti and reports whether it is a replay (already seen and not +// yet expired). The first time a jti is seen it returns false and is remembered +// until now+ttl; any subsequent call within that window returns true. +func (c *dpopReplayCache) observe(jti string, now time.Time) bool { + c.mu.Lock() + defer c.mu.Unlock() + + c.gc(now) + + if expiry, ok := c.seen[jti]; ok && expiry.After(now) { + return true + } + c.seen[jti] = now.Add(c.ttl) + return false +} + +// gc prunes expired entries to keep the map bounded. It is throttled to run at +// most once per ttl so the common (cache-hot) path stays O(1). Callers must hold +// c.mu. +func (c *dpopReplayCache) gc(now time.Time) { + if c.ttl <= 0 || now.Sub(c.lastGC) < c.ttl { + return + } + for jti, expiry := range c.seen { + if !expiry.After(now) { + delete(c.seen, jti) + } + } + c.lastGC = now +} diff --git a/service/internal/auth/dpop_replay_test.go b/service/internal/auth/dpop_replay_test.go new file mode 100644 index 0000000000..a4b5f4de2e --- /dev/null +++ b/service/internal/auth/dpop_replay_test.go @@ -0,0 +1,53 @@ +package auth + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestDPoPReplayCache_FirstUseThenReplay(t *testing.T) { + c := newDPoPReplayCache(time.Hour) + now := time.Now() + + require.False(t, c.observe("jti-1", now), "first use of a jti is not a replay") + require.True(t, c.observe("jti-1", now), "second use of the same jti within the window is a replay") + require.False(t, c.observe("jti-2", now), "a different jti is not a replay") +} + +func TestDPoPReplayCache_ExpiredEntryIsReusable(t *testing.T) { + ttl := time.Minute + c := newDPoPReplayCache(ttl) + now := time.Now() + + require.False(t, c.observe("jti", now)) + // After the TTL elapses the proof would already be rejected by the iat check, + // so the jti is allowed to be re-recorded rather than reported as a replay. + require.False(t, c.observe("jti", now.Add(ttl+time.Second)), "entry past its TTL should not count as a replay") +} + +func TestDPoPReplayCache_GCPrunesExpiredEntries(t *testing.T) { + ttl := time.Minute + c := newDPoPReplayCache(ttl) + now := time.Now() + + for _, jti := range []string{"a", "b", "c"} { + require.False(t, c.observe(jti, now)) + } + assert.Len(t, c.seen, 3) + + // A later observe past the GC interval prunes the expired entries, leaving + // only the freshly recorded one. + require.False(t, c.observe("d", now.Add(ttl+time.Second))) + assert.Len(t, c.seen, 1, "expired entries should be pruned, leaving only the new jti") +} + +func TestDPoPReplayCache_ZeroTTLDoesNotPanic(t *testing.T) { + c := newDPoPReplayCache(0) + now := time.Now() + // With a non-positive TTL entries expire immediately, so nothing is ever a replay. + require.False(t, c.observe("jti", now)) + require.False(t, c.observe("jti", now)) +} diff --git a/service/kas/access/accessPdp.go b/service/kas/access/accessPdp.go index 1253bfe765..dffe5c8e73 100644 --- a/service/kas/access/accessPdp.go +++ b/service/kas/access/accessPdp.go @@ -3,8 +3,10 @@ package access import ( "context" "errors" + "log/slog" "strconv" + "connectrpc.com/connect" authzV2 "github.com/opentdf/platform/protocol/go/authorization/v2" "github.com/opentdf/platform/protocol/go/entity" "github.com/opentdf/platform/protocol/go/policy" @@ -18,6 +20,39 @@ const ( ErrDecisionCountUnexpected = Error("authorization decision count unexpected") ) +// Sanitized cause-class vocabulary surfaced in client-facing rewrap error +// messages and in InfoContext log lines. Stable strings; greppable; safe to +// emit at info level (no resource attributes, no client identifiers, no FQNs). +const ( + // 4xx: routine denials. Default for any unknown access-evaluation error. + AccessErrCategoryPDPDenied = "pdp-denied" + // 5xx: the authorization service did not answer (transport / availability). + AccessErrCategoryAuthServiceUnavailable = "auth-service-unavailable" + // 5xx: our context was cancelled or timed out before we got an answer. + AccessErrCategoryContextCancelled = "context-cancelled" +) + +// classifyAccessError maps an error from canAccess / GetDecision into a +// sanitized category tag and a flag indicating whether it represents an +// internal/infrastructure failure (5xx) rather than a routine denial (4xx). +// +// Default to 403: misclassifying a denial as 500 pages oncall on every +// failed rewrap. Only mark internal for clear transport-level failures. +func classifyAccessError(ctx context.Context, err error) (string, bool) { + if ctxErr := ctx.Err(); ctxErr != nil && errors.Is(err, ctxErr) { + return AccessErrCategoryContextCancelled, true + } + switch connect.CodeOf(err) { //nolint:exhaustive // transport-level codes only; everything else defaults to a denial + case connect.CodeUnavailable, + connect.CodeDeadlineExceeded, + connect.CodeCanceled, + connect.CodeResourceExhausted, + connect.CodeAborted: + return AccessErrCategoryAuthServiceUnavailable, true + } + return AccessErrCategoryPDPDenied, false +} + var decryptAction = &policy.Action{ Name: actions.ActionNameRead, } @@ -101,7 +136,22 @@ func (p *Provider) checkAttributes(ctx context.Context, resources []*authzV2.Res } dr, err := p.SDK.AuthorizationV2.GetDecision(ctx, req) if err != nil { - p.Logger.ErrorContext(ctx, "error received from GetDecision") + category, _ := classifyAccessError(ctx, err) + p.Logger.InfoContext( + ctx, + "get decision failed", + slog.String("category", category), + slog.String("code", connect.CodeOf(err).String()), + slog.Int("resources", 1), + ) + p.Logger.DebugContext( + ctx, + "get decision failed: details", + slog.String("category", category), + slog.Any("error", err), + slog.Any("fulfillable_obligation_fqns", fulfillableObligationFQNs), + slog.Any("resource", resources[0]), + ) return nil, errors.Join(ErrDecisionUnexpected, err) } return []*authzV2.ResourceDecision{dr.GetDecision()}, nil @@ -119,7 +169,22 @@ func (p *Provider) checkAttributes(ctx context.Context, resources []*authzV2.Res dr, err := p.SDK.AuthorizationV2.GetDecisionMultiResource(ctx, req) if err != nil { - p.Logger.ErrorContext(ctx, "error received from GetDecisionMultiResource") + category, _ := classifyAccessError(ctx, err) + p.Logger.InfoContext( + ctx, + "get multi-resource decision failed", + slog.String("category", category), + slog.String("code", connect.CodeOf(err).String()), + slog.Int("resources", len(resources)), + ) + p.Logger.DebugContext( + ctx, + "get multi-resource decision failed: details", + slog.String("category", category), + slog.Any("error", err), + slog.Any("fulfillable_obligation_fqns", fulfillableObligationFQNs), + slog.Any("resources", resources), + ) return nil, errors.Join(ErrDecisionUnexpected, err) } return dr.GetResourceDecisions(), nil diff --git a/service/kas/access/rewrap.go b/service/kas/access/rewrap.go index aba7ffa84e..67a0e33690 100644 --- a/service/kas/access/rewrap.go +++ b/service/kas/access/rewrap.go @@ -160,7 +160,8 @@ func (p *Provider) parseSRT(ctx context.Context, srt string) (jwt.Token, string, "unable to validate or parse token", slog.Any("error", err), slog.Int("srt_length", len(srt)), - jwkThumbprintAttr(ctxAuth.GetJWKFromContext(ctx, p.Logger))) + jwkThumbprintAttr(ctxAuth.GetJWKFromContext(ctx, p.Logger)), + ) return nil, "", err401("could not parse token") } @@ -187,7 +188,8 @@ func (p *Provider) logSRTValidationFailure(ctx context.Context, token jwt.Token, issuedAt := token.IssuedAt() if !issuedAt.IsZero() { - fields = append(fields, + fields = append( + fields, slog.Time("iat", issuedAt), slog.Duration("iat_delta", issuedAt.Sub(now)), ) @@ -198,7 +200,8 @@ func (p *Provider) logSRTValidationFailure(ctx context.Context, token jwt.Token, expires := token.Expiration() if !expires.IsZero() { - fields = append(fields, + fields = append( + fields, slog.Time("exp", expires), slog.Duration("exp_delta", now.Sub(expires)), ) @@ -209,7 +212,8 @@ func (p *Provider) logSRTValidationFailure(ctx context.Context, token jwt.Token, notBefore := token.NotBefore() if !notBefore.IsZero() { - fields = append(fields, + fields = append( + fields, slog.Time("nbf", notBefore), slog.Duration("nbf_delta", notBefore.Sub(now)), ) @@ -261,7 +265,8 @@ func (p *Provider) verifySRTSignature(ctx context.Context, srt string, dpopJWK j ) if err != nil { if p.Logger != nil { - p.Logger.WarnContext(ctx, + p.Logger.WarnContext( + ctx, "unable to verify request token", slog.Int("srt_length", len(srt)), jwkThumbprintAttr(dpopJWK), @@ -370,7 +375,8 @@ func (p *Provider) extractSRTBody(ctx context.Context, headers http.Header, in * err := protojson.UnmarshalOptions{DiscardUnknown: true}.Unmarshal([]byte(rbString), &requestBody) // if there are no requests then it could be a v1 request if err != nil { - p.Logger.WarnContext(ctx, + p.Logger.WarnContext( + ctx, "invalid SRT", slog.Any("err_v2", err), slog.Int("rb_string_length", len(rbString)), @@ -382,7 +388,8 @@ func (p *Provider) extractSRTBody(ctx context.Context, headers http.Header, in * var errv1 error if requestBody, errv1 = extractAndConvertV1SRTBody([]byte(rbString)); errv1 != nil { - p.Logger.WarnContext(ctx, + p.Logger.WarnContext( + ctx, "invalid SRT", slog.Any("err_v1", errv1), slog.Int("rb_string_length", len(rbString)), @@ -393,7 +400,8 @@ func (p *Provider) extractSRTBody(ctx context.Context, headers http.Header, in * isV1 = true } // TODO: this log is too big and should be reconsidered or removed - p.Logger.DebugContext(ctx, + p.Logger.DebugContext( + ctx, "extracted request body", slog.String("rewrap_body", requestBody.String()), slog.String("rewrap_srt", rbString), @@ -594,7 +602,8 @@ func (p *Provider) Rewrap(ctx context.Context, req *connect.Request[kaspb.Rewrap } kaoResults := *getMapValue(results) if len(kaoResults) != 1 { - p.Logger.WarnContext(ctx, + p.Logger.WarnContext( + ctx, "status 400 due to wrong result set size", slog.Any("kao_results", kaoResults), slog.Any("results", results), @@ -681,7 +690,8 @@ func (p *Provider) verifyRewrapRequests(ctx context.Context, req *kaspb.Unsigned // Get EC key size and convert to mode keySize, err := ocrypto.GetECKeySize([]byte(ephemeralPubKeyPEM)) if err != nil { - p.Logger.WarnContext(ctx, + p.Logger.WarnContext( + ctx, "failed to get EC key size", slog.Any("kao", kao), slog.Any("error", err), @@ -692,7 +702,8 @@ func (p *Provider) verifyRewrapRequests(ctx context.Context, req *kaspb.Unsigned mode, err := ocrypto.ECSizeToMode(keySize) if err != nil { - p.Logger.WarnContext(ctx, + p.Logger.WarnContext( + ctx, "failed to convert key size to mode", slog.Any("kao", kao), slog.Any("error", err), @@ -704,7 +715,8 @@ func (p *Provider) verifyRewrapRequests(ctx context.Context, req *kaspb.Unsigned // Parse the PEM public key block, _ := pem.Decode([]byte(ephemeralPubKeyPEM)) if block == nil { - p.Logger.WarnContext(ctx, + p.Logger.WarnContext( + ctx, "failed to decode PEM block", slog.Any("kao", kao), slog.Any("error", err), @@ -715,7 +727,8 @@ func (p *Provider) verifyRewrapRequests(ctx context.Context, req *kaspb.Unsigned pub, err := x509.ParsePKIXPublicKey(block.Bytes) if err != nil { - p.Logger.WarnContext(ctx, + p.Logger.WarnContext( + ctx, "failed to parse public key", slog.Any("kao", kao), slog.Any("error", err), @@ -898,7 +911,8 @@ func (p *Provider) tdf3Rewrap(ctx context.Context, requests []*kaspb.UnsignedRew // Store per-KAO results even on error so tamper signals (e.g. corrupted // policy body → generic "bad request") reach the SDK rather than being // replaced by a top-level "invalid request". - p.Logger.WarnContext(ctx, + p.Logger.WarnContext( + ctx, "rewrap: verifyRewrapRequests failed", slog.String("policy_id", policyID), slog.Any("error", err), @@ -916,12 +930,31 @@ func (p *Provider) tdf3Rewrap(ctx context.Context, requests []*kaspb.UnsignedRew pdpAccessResults, accessErr := p.canAccess(ctx, tok, policies, additionalRewrapContext.Obligations.FulfillableFQNs) if accessErr != nil { - p.Logger.DebugContext(ctx, - "tdf3rewrap: cannot access policy", - slog.Any("policies", policies), + category, isInternal := classifyAccessError(ctx, accessErr) + // Terse, sensitive-payload-free line for the routine-denial flood case. + // Floods read as floods, not as a forest of stack traces. + p.Logger.InfoContext( + ctx, + "tdf3rewrap: access evaluation failed", + slog.String("category", category), + slog.Bool("internal", isInternal), + slog.Int("policies", len(policies)), + slog.Int("requests", len(requests)), + ) + // Verbose / sensitive: only read when investigating one specific request. + p.Logger.DebugContext( + ctx, + "tdf3rewrap: access evaluation failed: details", + slog.String("category", category), slog.Any("error", accessErr), + slog.Any("policies", policies), + slog.Any("fulfillable_obligation_fqns", additionalRewrapContext.Obligations.FulfillableFQNs), ) - failAllKaos(requests, results, err500("could not perform access")) + if isInternal { + failAllKaos(requests, results, err500("internal: "+category)) + } else { + failAllKaos(requests, results, err403("forbidden: "+category)) + } return "", results, nil } diff --git a/test/start-up-with-containers/action.yaml b/test/start-up-with-containers/action.yaml index 20ba6100d6..33ccd5cec7 100644 --- a/test/start-up-with-containers/action.yaml +++ b/test/start-up-with-containers/action.yaml @@ -31,6 +31,10 @@ inputs: default: "true" description: 'Whether to provision fixture policy data after bootstrapping the platform' required: false + dpop-challenge-enabled: + default: "false" + description: 'Whether to enable the DPoP nonce challenge flow (sets server.auth.dpop.require_nonce: true)' + required: false outputs: platform-working-dir: @@ -53,6 +57,7 @@ runs: LOG_LEVEL: ${{ inputs.log-level }} LOG_TYPE: ${{ inputs.log-type }} PROVISION_POLICY_FIXTURES: ${{ inputs.provision-policy-fixtures }} + DPOP_CHALLENGE_ENABLED: ${{ inputs.dpop-challenge-enabled }} run: | # Validate platform-ref (must contain only safe characters for a git ref) if [[ ! "${PLATFORM_REF}" =~ ^[a-zA-Z0-9._/-]+$ ]]; then @@ -115,6 +120,16 @@ runs: exit 1 ;; esac + + # Validate dpop-challenge-enabled (must be true or false) + case "${DPOP_CHALLENGE_ENABLED}" in + true|false) + ;; + *) + echo "Error: dpop-challenge-enabled must be 'true' or 'false'." + exit 1 + ;; + esac - name: Check out platform uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: @@ -260,6 +275,24 @@ runs: | (.server.cryptoProvider.standard.keys += [{"kid":"x1","alg":"hpqt:xwing","private":"kas-xwing-private.pem","cert":"kas-xwing-public.pem"},{"kid":"h1","alg":"hpqt:secp256r1-mlkem768","private":"kas-p256mlkem768-private.pem","cert":"kas-p256mlkem768-public.pem"},{"kid":"h2","alg":"hpqt:secp384r1-mlkem1024","private":"kas-p384mlkem1024-private.pem","cert":"kas-p384mlkem1024-public.pem"}]) ' -i opentdf.yaml working-directory: otdf-test-platform + - name: Enable DPoP nonce challenge + shell: bash + if: ${{ inputs.dpop-challenge-enabled == 'true' }} + run: | + yq e '.server.auth.dpop.require_nonce = true' -i opentdf.yaml + working-directory: otdf-test-platform + - name: Overlay DPoP-capable Keycloak (26.2) + # The default docker-compose pins Keycloak 25 so downstream consumers stay on + # it; DPoP testing needs Keycloak 26.2 plus the admin-fine-grained-authz:v1 + # feature flag, overlaid only when the nonce challenge flow is enabled. + shell: bash + if: ${{ inputs.dpop-challenge-enabled == 'true' }} + run: | + yq e ' + (.services.keycloak.image = "keycloak/keycloak:26.2") + | (.services.keycloak.environment.KC_FEATURES = "preview,token-exchange,admin-fine-grained-authz:v1") + ' -i docker-compose.yaml + working-directory: otdf-test-platform - name: Configure logging shell: bash env: