Repository navigation
v0.28.0: rate limits, security events and remaining review findings - #72
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Programmatic clients authenticate with DPoP-bound SystemAuth access tokens: command-line tools sign people in with identity/publicclient and automation uses service accounts (ADR-016). An opaque Bearer value is now an invalid token and the X-API-Key header is not a credential. The identity/apikey package and the session/middleware API key middleware are unchanged. BREAKING CHANGE: appauth.Options.APIKeys, relyingparty.BearerConfig.APIKeys, relyingparty.APIKeyValidator, relyingparty.AuthMethodAPIKey and relyingparty.AuthenticatedPrincipal.APIKeyID are removed. Remove the APIKeys option and move callers to publicclient sign-in or service accounts. Refs: RMI-SYSTEMFORGE-217 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add an upgrade section to the application authentication guide and drop API keys from the relying-party, AAuth and README descriptions. Refs: RMI-SYSTEMFORGE-217 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Describe the scope and assumptions, assets, actors and trust boundaries, a STRIDE table of threats per boundary with the mitigation and the package that holds it, the residual risks and accepted trade-offs, and how the model is kept current. Link it from the security policy, the production checklist and the Security section of the site navigation. Refs: RMI-SYSTEMFORGE-217 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… rate limits Add session/clientaddr: a Resolver configured with trusted proxy networks and the header they write (X-Forwarded-For, read from the right, or a single-value header such as CF-Connecting-IP), falling back to the direct peer. Headers from peers outside the trusted networks are never believed. SystemAuth reads it from the new client_address configuration (or WithClientAddressResolver) and uses it for login-token, waitlist and sign-up limits, recorded key-use addresses and SCIM audit events. TrustedProxyNetworks, ratelimit.PeerIP and ForwardedClientIP, the BFF client IP extractor and the API key middleware are built on it. ForwardedClientIP, ForwardedIPKey, WithLoginTokenClientIP, CloudflareClientIPConfig and the legacy ClientIPConfig fields are deprecated in favour of a Resolver; ratelimit.AddressKey keys limits on it. Refs: RMI-SYSTEMFORGE-213 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs: RMI-SYSTEMFORGE-213 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add ratelimit.Policy, plain net/http middleware applying a default limit per client address on every route and a route table: the token endpoint per address and per OAuth client, login, sign-up and BFF authentication paths per address, and exemptions for health checks, static assets and the payment webhook. Route entries are replaced by name with MergeRoutes; back-channel networks can be exempted. NewSignedInLimiter limits signed-in principals and service accounts after authentication. Check, Exhausted and Record give handlers limits on the same storage, and MemoryStorage and RedisStorage can be peeked without counting. Responses carry the IETF RateLimit-Policy and RateLimit headers beside X-RateLimit-*, and Retry-After is rounded up to at least one second. The default layer fails open and logs on a storage outage while route limits fail closed; a full MemoryStorage is always a 429. Refs: RMI-SYSTEMFORGE-214 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SystemAuth installs the default layer and route policies outermost, configured under rate_limit (default, signed_in, routes, exempt_networks, disabled, redis_url, single_instance). The login-token, waitlist and sign-up page limits and the managed-principal code limits move onto the shared storage. New handler limits cover failed client authentication at the token endpoint, tokens issued per service account by the JWT-bearer grant, and social login start and callback. The managed-principal API gets the signed-in limit after authentication. systemauthsvc connects rate_limit.redis_url or takes Options.RateLimitStorage, and warns in production when counters are per instance without a declared single instance. appauth accepts a client-address resolver and a signed-in limiter for its middleware. Refs: RMI-SYSTEMFORGE-214 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs: RMI-SYSTEMFORGE-214 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add a guide to the client-address rule and the rate limit layers, and update the deployment guide, the production checklist, the BFF and rate limiting pages for client_address and rate_limit. Refs: RMI-SYSTEMFORGE-213 Refs: RMI-SYSTEMFORGE-214 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CookieConfig.Validate, and so NewHandler, now rejects SameSite=None: the origin check that protects state-changing requests relies on the browser withholding the session cookie from cross-site subrequests. An unset or http.SameSiteDefaultMode SameSite still defaults to Strict. The other session cookie writers in systemauth, relyingparty and oauthclient already hard-code or require Lax. Refs: RMI-SYSTEMFORGE-218 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Revoke passed the raw token where storage expects a Fosite request ID, so it reported success without revoking anything. It now looks the access or refresh token up through Fosite's introspection, revokes the access and refresh tokens of its family, and returns an error wrapping ErrInvalidToken when the token is unknown, expired or already revoked. Refs: RMI-SYSTEMFORGE-219 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… builds The multiapp server now bounds the time a client may take to send its request headers, and the publicclient loopback callback server sets read, write and idle timeouts alongside its header timeout. cmd/systemauth already set all four. Refs: RMI-SYSTEMFORGE-220 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…their text Internal logs an unexpected error and returns a Huma 500 with a generic message; Log does the same for net/http handlers. Huma copies an error passed to its constructors into the response body, so handlers must not pass internal errors to it. Refs: RMI-SYSTEMFORGE-216 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Known service errors map to fixed messages and statuses; anything else is logged and answered with a generic 500. The ad-hoc validation errors of the service now wrap new sentinels (ErrOwnerRequired, ErrAlreadyMember, ErrNewOwnerNotMember, ErrInvalidOwnerRole) or existing ones (ErrInvalidSlug, ErrNotOwner, ErrOrganizationNotFound, ErrMembershipNotFound), so their messages stay available to clients. Refs: RMI-SYSTEMFORGE-216 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Listing service errors a client can act on map to fixed messages; anything else is logged and answered with a generic 500. Ent validation failures on create and update now wrap the new ErrInvalidListing, and deleting a published listing wraps the new ErrListingNotDeletable. Refs: RMI-SYSTEMFORGE-216 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Identity and policy operations log provider failures and answer a generic 500 instead of passing the error to Huma. The token middleware answers "invalid token" and logs the reason, and ToContractError no longer copies a non-contract error's text into its message. Refs: RMI-SYSTEMFORGE-216 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Known invite, organization, authorization and application role errors map to fixed messages; anything else is logged and answered with a generic 500. The service's ad-hoc validation errors now wrap new sentinels (ErrInviteNotFound, ErrPendingInviteExists, ErrAlreadyMember, ErrAppRolesNotConfigured, ErrCollaboratorAppRoles). Refs: RMI-SYSTEMFORGE-216 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…onse Known waitlist failures keep their message; any other failure is logged and reported as "internal error". Refs: RMI-SYSTEMFORGE-216 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Credential, scope, rate-limit, transient and conflict errors answer fixed messages and log the detail; a transient error could otherwise carry the provider's network error text. Refs: RMI-SYSTEMFORGE-216 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The bearer and DPoP middlewares send the matching sentinel's text instead of the full error chain, so parser, key and replay-cache details stay out of 401 bodies; an unexpected error is logged and answered "authentication failed". Refs: RMI-SYSTEMFORGE-216 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…onses Refs: RMI-SYSTEMFORGE-216 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Known sync failures keep their message; storage errors are logged and answered generically. Refs: RMI-SYSTEMFORGE-216 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ixture Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Token, authorize and access-token log records now carry the RFC 6749 error code, description, hint and status of a refused request instead of the error value, which slog's text handler formats with %+v. The wrapped cause and Fosite's debug text are added only when the logger is enabled at debug level, or for server errors, and always by message. Refs: RMI-SYSTEMFORGE-215 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
audit.Context gains RequestID, filled from chi's request-ID middleware by audit.RequestID. NopEmitter is the default where an emitter is optional, and LogEmitter writes each event as one structured log record, leaving out the session ID. requestprincipal.AuditActor names the request's principal as an event's actor. Refs: RMI-SYSTEMFORGE-215 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
WithAuditEmitter sets where SystemAuth records sign-ins and their refusals, sign-outs, refused token requests, refresh-token reuse, authorization-code replay, revocations of tokens and token families, and the signing key in use at startup. Each event carries the request ID, the principal or client ID and the client address, taken from one resolver, and never a token, code or secret. Events are discarded by default. Refs: RMI-SYSTEMFORGE-215 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… changes organization.WithAuditEmitter records memberships added, updated, removed and ownership transfers; principal.WithAuditEmitter records platform administration granted and revoked; serviceaccount.Config.Audit records account, key and role changes, by key ID only. The actor is the request's principal, or the operator the call names. Refs: RMI-SYSTEMFORGE-215 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…waivers Options.AuditEmitter now reaches SystemAuth and the organization and principal services the package builds, and every production check waived at startup is recorded as a systemauth.production.waiver event. The systemauth command writes its security events to its log. Refs: RMI-SYSTEMFORGE-215 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Real flows run through an embedded SystemAuth logging at debug level to text and JSON handlers: social login, the authorization code flow with PKCE, refresh, code and refresh-token replay, client credentials with a right and a wrong secret, JWT bearer, revocation, logout, and BFF sign-in and sign-out. No token, code, verifier, client or HMAC secret, DPoP proof, assertion, key or session cookie may appear in any log attribute or event. Further tests cover each security event. Refs: RMI-SYSTEMFORGE-215 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs: RMI-SYSTEMFORGE-215 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nd a proxy Security events took the client address from the request's direct peer, so behind a reverse proxy every event recorded the proxy. They now use the server's client-address rule, the same one rate limits use. Refs: RMI-SYSTEMFORGE-215 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs: RMI-SYSTEMFORGE-217 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…address rule Refs: RMI-SYSTEMFORGE-215 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs: RMI-SYSTEMFORGE-221 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedToo many files! This PR contains 136 files, which is 36 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry.
 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| case errors.Is(err, cf.ErrInvalidCredential): | ||
| // A provider's error can name hosts or carry its own text: the | ||
| // client gets a fixed message and the detail is logged. | ||
| h.log.InfoContext(c.r.Context(), "connections: provider rejected the credential", "error", err) |
There was a problem hiding this comment.
Not exploitable as log injection: this is a log/slog call, and the value is one structured attribute. slog's text handler quotes any value containing spaces, '=', quotes or control characters (including newlines), and the JSON handler escapes them, so a value cannot start a forged log line. The attribute is kept on purpose: the error text or client and grant identifiers are what an operator needs to investigate a refused request. No change.
| h.log.InfoContext(c.r.Context(), "connections: provider rejected the credential", "error", err) | ||
| writeError(c.w, http.StatusUnprocessableEntity, "the provider rejected the credential") | ||
| case errors.Is(err, cf.ErrInsufficientScope): | ||
| h.log.InfoContext(c.r.Context(), "connections: credential lacks scope", "error", err) |
There was a problem hiding this comment.
Not exploitable as log injection: this is a log/slog call, and the value is one structured attribute. slog's text handler quotes any value containing spaces, '=', quotes or control characters (including newlines), and the JSON handler escapes them, so a value cannot start a forged log line. The attribute is kept on purpose: the error text or client and grant identifiers are what an operator needs to investigate a refused request. No change.
| h.log.InfoContext(c.r.Context(), "connections: credential lacks scope", "error", err) | ||
| writeError(c.w, http.StatusUnprocessableEntity, "the credential lacks a required scope") | ||
| case errors.Is(err, cf.ErrRateLimited): | ||
| h.log.WarnContext(c.r.Context(), "connections: provider rate limited", "error", err) |
There was a problem hiding this comment.
Not exploitable as log injection: this is a log/slog call, and the value is one structured attribute. slog's text handler quotes any value containing spaces, '=', quotes or control characters (including newlines), and the JSON handler escapes them, so a value cannot start a forged log line. The attribute is kept on purpose: the error text or client and grant identifiers are what an operator needs to investigate a refused request. No change.
| writeError(c.w, http.StatusBadGateway, "the provider is limiting requests; try again later") | ||
| case errors.Is(err, cf.ErrTransient): | ||
| // A transient error wraps the network or provider failure. | ||
| h.log.WarnContext(c.r.Context(), "connections: provider unavailable", "error", err) |
There was a problem hiding this comment.
Not exploitable as log injection: this is a log/slog call, and the value is one structured attribute. slog's text handler quotes any value containing spaces, '=', quotes or control characters (including newlines), and the JSON handler escapes them, so a value cannot start a forged log line. The attribute is kept on purpose: the error text or client and grant identifiers are what an operator needs to investigate a refused request. No change.
| if len(ev.Details) > 0 { | ||
| attrs = append(attrs, slog.Any("details", ev.Details)) | ||
| } | ||
| e.logger.LogAttrs(ctx, level, "security event", attrs...) |
There was a problem hiding this comment.
Not exploitable as log injection: this is a log/slog call, and the value is one structured attribute. slog's text handler quotes any value containing spaces, '=', quotes or control characters (including newlines), and the JSON handler escapes them, so a value cannot start a forged log line. The attribute is kept on purpose: the error text or client and grant identifiers are what an operator needs to investigate a refused request. No change.
| // already bound to a key needs a proof by it. | ||
| if err := s.bindDPoP(ar, proof); err != nil { | ||
| logger.Info("token request rejected: DPoP", "grant_type", grantType, "client_id", clientID, "error", err) | ||
| logger.Info("token request rejected: DPoP", "grant_type", grantType, "client_id", clientID, oauthcore.ErrorAttr(ctx, logger, err)) |
There was a problem hiding this comment.
Not exploitable as log injection: this is a log/slog call, and the value is one structured attribute. slog's text handler quotes any value containing spaces, '=', quotes or control characters (including newlines), and the JSON handler escapes them, so a value cannot start a forged log line. The attribute is kept on purpose: the error text or client and grant identifiers are what an operator needs to investigate a refused request. No change.
| // active, and its token family is revoked. | ||
| if err := s.requireActiveSubject(ctx, ar); err != nil { | ||
| logger.Info("token request rejected", "grant_type", grantType, "client_id", clientID, "error", err) | ||
| logger.Info("token request rejected", "grant_type", grantType, "client_id", clientID, oauthcore.ErrorAttr(ctx, logger, err)) |
There was a problem hiding this comment.
Not exploitable as log injection: this is a log/slog call, and the value is one structured attribute. slog's text handler quotes any value containing spaces, '=', quotes or control characters (including newlines), and the JSON handler escapes them, so a value cannot start a forged log line. The attribute is kept on purpose: the error text or client and grant identifiers are what an operator needs to investigate a refused request. No change.
| // Bound refresh-token rotation by the family's absolute lifetime. | ||
| if err := s.applyTokenFamilyLifetime(ctx, ar); err != nil { | ||
| logger.Info("token request rejected", "grant_type", grantType, "client_id", clientID, "error", err) | ||
| logger.Info("token request rejected", "grant_type", grantType, "client_id", clientID, oauthcore.ErrorAttr(ctx, logger, err)) |
There was a problem hiding this comment.
Not exploitable as log injection: this is a log/slog call, and the value is one structured attribute. slog's text handler quotes any value containing spaces, '=', quotes or control characters (including newlines), and the JSON handler escapes them, so a value cannot start a forged log line. The attribute is kept on purpose: the error text or client and grant identifiers are what an operator needs to investigate a refused request. No change.
| if logger == nil { | ||
| logger = slog.Default() | ||
| } | ||
| logger.ErrorContext(ctx, op, "error", err) |
There was a problem hiding this comment.
Not exploitable as log injection: this is a log/slog call, and the value is one structured attribute. slog's text handler quotes any value containing spaces, '=', quotes or control characters (including newlines), and the JSON handler escapes them, so a value cannot start a forged log line. The attribute is kept on purpose: the error text or client and grant identifiers are what an operator needs to investigate a refused request. No change.
The access-token middleware and the managed-principal handler logged r.URL.Path, which the client chooses; they now log the route the request matched. Refs: RMI-SYSTEMFORGE-216 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| writeError(w, http.StatusConflict, msg) | ||
| return | ||
| } | ||
| h.svc.logger.ErrorContext(r.Context(), "managedprincipal: request failed", "route", chi.RouteContext(r.Context()).RoutePattern(), "error", err) |
There was a problem hiding this comment.
The request path on this line was replaced by the matched route in 22bf9e4; this alert is the remaining source, the error value. Not exploitable as log injection: it is one log/slog structured attribute, which slog's text handler quotes (including any newline) and the JSON handler escapes, so it cannot start a forged log line. The error is kept because it is what an operator needs to investigate a failed request. No change.
The rest of the production-hardening initiative: Phase 4 and Phase 5 of
docs/specs/initiatives/INIT-SYSTEMFORGE-015/ROADMAP.md. Release notes and upgrade guide:docs/releases/v0.28.0.md.Security
One client-address rule (
session/clientaddr): trusted proxy networks and the header to read, used by SystemAuth, the BFF,appauth, rate limits and security events. Headers from untrusted peers are ignored.Layered rate limits, on by default in SystemAuth:
429 responses carry
Retry-After(never 0) andRateLimit-Policy/RateLimit. When storage fails, the default layer lets requests through and credential limits refuse.Logs fit for incidents:
contract/audit, with request IDs and the visitor's address (docs/security/security-events.md).No internal error text in API responses: about 60 sites across organization, marketplace, contract, invite, connections, session middleware, managed principals, federation and the sign-up API.
Static API key authentication removed from relying parties. Use service accounts or
identity/publicclient.Smaller findings:
SameSite=None;EmbeddedOAuthProvider.Revokenow revokes;ReadHeaderTimeout.Threat model:
docs/security/threat-model.md.Breaking
rate_limit.disabledto opt out).contract.ToContractErrorno longer includes error text.SameSite=None.See the upgrade guide. Behind a reverse proxy, set
client_address, or every visitor shares the proxy's address.Tests, race tests on SystemAuth and sessions, and lint pass locally. The publication scan is clean.
🤖 Generated with Claude Code