feat(access): Gate domain routes with email verification - #225
Conversation
Code Review Summary✨ Adds an opt-in email-verification gate for individual domains and path prefixes. A new 🚀 Key Improvements
|
![sourceant[bot]](https://avatars.githubusercontent.com/in/1161781?s=60&v=4){kind=small}
| c.String(http.StatusServiceUnavailable, "Access service is unavailable") | ||
| return | ||
| } | ||
| email, host, returnPath, err := s.access.VerifyMagicLink(c.Query("token")) |
There was a problem hiding this comment.
s.applicationAccessPolicy calls s.manager.FindDeployments() (a full scan/parse of deployment metadata) on every auth_request. Here it is invoked even when the token fails to verify, wasting the scan and passing empty host/path. Check the error first and only then resolve the policy; this both short-circuits the expensive lookup and avoids resolving a policy from empty values.
| email, host, returnPath, err := s.access.VerifyMagicLink(c.Query("token")) | |
| email, host, returnPath, err := s.access.VerifyMagicLink(c.Query("token")) | |
| if err != nil { | |
| c.String(http.StatusUnauthorized, "This sign-in link is invalid or expired") | |
| return | |
| } | |
| policy, ok := s.applicationAccessPolicy(host, returnPath) | |
| if !ok || !access.Allows(policy, email) || !strings.EqualFold(hostnameOnly(c.Request.Host), hostnameOnly(host)) { | |
| c.String(http.StatusUnauthorized, "This sign-in link is invalid or expired") | |
| return | |
| } |
| } | ||
|
|
||
| func (s *Server) getAccessEmailTargets(c *gin.Context) { | ||
| options := make([]gin.H, 0) |
There was a problem hiding this comment.
validateDomainAccess defensively checks s.notify == nil, but this handler dereferences s.notify unconditionally. A partially-initialized Server (as constructed in tests) would panic here. Return an empty list when the notification service is unavailable.
| options := make([]gin.H, 0) | |
| options := make([]gin.H, 0) | |
| if s.notify == nil { | |
| c.JSON(http.StatusOK, gin.H{"targets": options}) | |
| return | |
| } | |
| for _, target := range s.notify.Load().Targets { |
| if hostname(domain.Domain) == host { | ||
| return true | ||
| } | ||
| for _, alias := range append(domain.Aliases, domain.RouteOnlyAliases...) { |
There was a problem hiding this comment.
append(domain.Aliases, domain.RouteOnlyAliases...) may reuse the backing array of domain.Aliases when it has spare capacity, which can silently overwrite data shared by other slices. Iterate the two slices separately to avoid the aliasing hazard.
| for _, alias := range append(domain.Aliases, domain.RouteOnlyAliases...) { | |
| for _, alias := range domain.Aliases { | |
| if hostname(alias) == host { | |
| return true | |
| } | |
| } | |
| for _, alias := range domain.RouteOnlyAliases { | |
| if hostname(alias) == host { | |
| return true | |
| } | |
| } |
| if policy.Mode == "allowlist" && len(policy.AllowedEmails) == 0 { | ||
| return apiErrf(http.StatusBadRequest, "At least one allowed email is required") | ||
| } | ||
| for _, email := range policy.AllowedEmails { |
There was a problem hiding this comment.
This loop duplicates the email syntax check already provided by access.ValidEmail, which is the function the runtime authorizer access.Allows actually uses. Because the config-time validator and the request-time authorizer are now separate implementations, an address can pass API validation yet be treated differently at request time (or vice-versa). Delegating to the shared helper keeps the two contracts identical. Note: add the github.com/flatrun/agent/internal/access import and drop the now-unused net/mail import.
| for _, email := range policy.AllowedEmails { | |
| for _, email := range policy.AllowedEmails { | |
| if !access.ValidEmail(email) { | |
| return apiErrf(http.StatusBadRequest, "Allowed email %q is invalid", email) | |
| } | |
| } |
| if policy.Mode == "allowlist" && len(policy.AllowedEmails) == 0 { | ||
| return apiErrf(http.StatusBadRequest, "At least one allowed email is required") | ||
| } | ||
| for _, email := range policy.AllowedEmails { |
There was a problem hiding this comment.
This inline loop duplicates access.ValidEmail (internal/access/service.go), which implements the exact same contract: trim, mail.ParseAddress, then strings.EqualFold(address.Address, trimmed). Because the api package already imports internal/access, the canonical validator should be reused so the two contracts cannot diverge. As written, a future tightening of access.ValidEmail (e.g. rejecting display-name forms or unicode) would not apply to domain saves, so an allowed_emails value accepted here could later be silently rejected by the login-time gate (access.Allows → ValidEmail), producing inconsistent behavior for the same field. Reusing the helper also removes the local re-derivation of the validation rule. Add the github.com/flatrun/agent/internal/access import and drop the now-unused net/mail import.
| for _, email := range policy.AllowedEmails { | |
| +` for _, email := range policy.AllowedEmails { | |
| + if !access.ValidEmail(email) { | |
| + return apiErrf(http.StatusBadRequest, "Allowed email %q is invalid", email) | |
| + } | |
| + } | |
| +` |
Operators can require an email allowlist or admit any verified address before visitors reach an application. Sign-in links remain single use across agent restarts, and sessions stay bound to the requested host.
Protected routes remain closed when policy data is unavailable. Deployment editors can select email targets without notification administration access, and sign-in requests are covered through HTTP.
Access requests avoid unnecessary policy scans and handle unavailable email targets safely. Alias matching preserves shared storage, and saved emails use the runtime validation rules.
ac39917 to
f79e1de
Compare
| } | ||
| } | ||
|
|
||
| func (s *Service) AllowEmailRequest(host, email string) bool { |
There was a problem hiding this comment.
lastRequests is keyed by host+email and is only ever written, never evicted. Because AllowEmailRequest is reached for every syntactically valid address in any_verified mode, an attacker (or ordinary traffic to distinct addresses) accumulates one map entry per key forever — an unbounded memory growth path. The rate-limit window is one minute, so entries older than that are dead weight and can be dropped opportunistically.
| func (s *Service) AllowEmailRequest(host, email string) bool { | |
| func (s *Service) AllowEmailRequest(host, email string) bool { | |
| key := hostname(host) + "\x00" + normalizeEmail(email) | |
| now := s.now() | |
| s.mu.Lock() | |
| defer s.mu.Unlock() | |
| // lastRequests is only ever written, so stale entries (older than the | |
| // one-minute window) would accumulate without bound; drop them once the | |
| // map grows past a small threshold. | |
| if len(s.lastRequests) > 1024 { | |
| for entry, seen := range s.lastRequests { | |
| if now.Sub(seen) >= time.Minute { | |
| delete(s.lastRequests, entry) | |
| } | |
| } | |
| } | |
| if last, ok := s.lastRequests[key]; ok && now.Sub(last) < time.Minute { | |
| return false | |
| } | |
| s.lastRequests[key] = now | |
| return true | |
| } |
| return apiErrf(http.StatusBadRequest, "Allowed email %q is invalid", email) | ||
| } | ||
| } | ||
| if policy.SessionHours < 0 || policy.SessionHours > 720 { |
There was a problem hiding this comment.
The accepted range and the error text disagree: 0 is a valid value (it means "use the default" — access.Session falls back to 24 hours), yet the message claims the range is 1–720. A client that sends 0 gets no error, and a client that sends -1 is told the range starts at 1, which is misleading for a field whose zero value is legal.
| if policy.SessionHours < 0 || policy.SessionHours > 720 { | |
| if policy.SessionHours != 0 && (policy.SessionHours < 1 || policy.SessionHours > 720) { | |
| return apiErrf(http.StatusBadRequest, "Session hours must be between 1 and 720, or 0 for the 24-hour default") | |
| } |
| return value | ||
| } | ||
|
|
||
| func hostnameOnly(value string) string { |
There was a problem hiding this comment.
hostnameOnly is a second host-normalizer that diverges from access.hostname, which is the normalizer both access.Resolve (policy lookup) and access.ValidateSession (the check endpoint) use. hostnameOnly only truncates at the first :, so a bracketed IPv6 authority ([::1]:8443 → [) and a trailing-dot host (app.example.com. → unchanged, because only access.hostname strips the trailing dot) are normalized differently. Concretely, verifyApplicationAccess compares hostnameOnly(c.Request.Host) against the magic link's host — which was produced by MagicLink via access.hostname — so a Host header carrying a trailing dot (or an IPv6 literal) is accepted by the check endpoint (ValidateSession → access.hostname) but rejected at verify, i.e. the same session is valid on one path and invalid on the other. Align this helper with access.hostname so both paths normalize identically.
| func hostnameOnly(value string) string { | |
| func hostnameOnly(value string) string { | |
| value = strings.TrimSpace(strings.ToLower(value)) | |
| if strings.HasPrefix(value, "[") { | |
| if end := strings.IndexByte(value, ']'); end > 0 { | |
| return value[1:end] | |
| } | |
| } | |
| if index := strings.IndexByte(value, ':'); index >= 0 { | |
| return strings.TrimSuffix(value[:index], ".") | |
| } | |
| return strings.TrimSuffix(value, ".") | |
| } |
| return apiErrf(http.StatusBadRequest, "Allowed email %q is invalid", email) | ||
| } | ||
| } | ||
| if policy.SessionHours < 0 || policy.SessionHours > 720 { |
There was a problem hiding this comment.
The accepted range and the message disagree. The guard rejects only negative values and values above 720, so session_hours: 0 is accepted and is meaningful (access.Session and verifyApplicationAccess both substitute 24 for any value <= 0). The error text tells clients the floor is 1, so a client that honours the message and refuses to send 0 cannot express the document(ed) default. Make the message match the inputs the handler actually accepts.
| if policy.SessionHours < 0 || policy.SessionHours > 720 { | |
| if policy.SessionHours < 0 || policy.SessionHours > 720 { | |
| return apiErrf(http.StatusBadRequest, "Session hours must be between 0 and 720, where 0 uses the default") | |
| } |
| return access.Resolve(deployments, host, requestPath) | ||
| } | ||
|
|
||
| func safeAccessReturn(value string) string { |
There was a problem hiding this comment.
safeAccessReturn duplicates safeReturn in internal/access/service.go character for character. It is the redirect/sanitisation guard for the protected-application flow: applicationAccessLogin and requestApplicationAccess use this copy, while access.MagicLink applies its own copy to the same value written into the token. Because the two guards are independent, a tightening of the canonical helper (for example also rejecting /%2e%2e or control characters) would leave the API copy permissive, so the login page and the return field could accept a value the token issuer would have normalised. Export the sanitizer from internal/access (rename safeReturn to SafeReturn) and delegate, so the open-redirect guard has one implementation.
| func safeAccessReturn(value string) string { | |
| +func safeAccessReturn(value string) string { | |
| + return access.SafeReturn(value) | |
| +} |
| return value | ||
| } | ||
|
|
||
| func hostnameOnly(value string) string { |
There was a problem hiding this comment.
hostnameOnly is a second implementation of the host normalisation that internal/access already provides as hostname (used by MagicLink, Session and ValidateSession). The two disagree: access.hostname lowercases, splits host:port with net.SplitHostPort, and strips a trailing dot, whereas this copy only truncates at the first : and keeps the trailing dot. In verifyApplicationAccess the token's host was produced by access.hostname, but the request host is normalised by this copy, so a request that arrives as private.example.com. (or with any input net.SplitHostPort would have handled differently) fails the strings.EqualFold binding check even though the link is valid. Delegate to the canonical normaliser (export hostname as Hostname) so the value that binds the token and the value checked at verification use the same rule.
| func hostnameOnly(value string) string { | |
| +func hostnameOnly(value string) string { | |
| + return access.Hostname(value) | |
| +} |
Access requests now share host and redirect normalization across every step. Expired rate limit entries are removed, and session validation text matches accepted values.
1 participant
Add email verification gates for domains and paths. Operators can allow listed addresses or any verified address without changing the protected application.