gsadmin/certctl
1
0
Fork 0
mirror of https://github.com/shankar0123/certctl.git synced 2026-06-07 20:31:30 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(oidc): enforce AllowedEmailDomains allowlist in HandleCallback

Browse Source 
Closes CRIT-5 of the 2026-05-10 audit — the LAST Critical blocker for
v2.1.0. The OIDCProvider.AllowedEmailDomains field shipped persisted
(internal/auth/oidc/domain/types.go:47), API-surfaced
(internal/api/handler/auth_session_oidc.go), MCP-surfaced
(internal/mcp/tools_auth_bundle2.go), and GUI-editable, but the
verifier in internal/auth/oidc/service.go::HandleCallback NEVER read
it. Operators filling allowed_email_domains: ["acme.com"] expected
"users outside acme.com cannot log in" — the field had zero effect.
Textbook lying-field shape per CLAUDE.md's "complete path" rule.

This commit:

- Adds Step 7.5 to HandleCallback (between profile-claim resolve and
  group-claim resolve): when the provider's AllowedEmailDomains slice
  is non-empty, the user's email-domain MUST match a list entry (case-
  insensitive exact match; subdomains NOT auto-accepted — operators
  who want dev.acme.com authorized must list it explicitly).

- Two new sentinel errors at the package level:
    - ErrEmailDomainNotAllowed   — email is set but domain not in list
    - ErrEmailMissingButRequired — allowlist set + ID token has no email

- New extractEmailDomain helper: case-folds + trims whitespace + uses
  LastIndex for the @ split + rejects empty input / no-@ / empty
  local-part / empty domain-part. Returns the lowercase domain or
  an error.

- 21 regression tests in internal/auth/oidc/email_domain_test.go:
    - 10 extractEmailDomain shape cases (plain, mixed-case input,
      leading/trailing whitespace, subdomain preserved, empty, no @,
      empty local-part, empty domain-part, multiple @ via LastIndex).
    - 11 match-semantic cases (empty list passes any, lowercase match,
      mixed-case allowlist entry match, mixed-case email match,
      whitespace-padded allowlist entry, unmatched returns
      ErrEmailDomainNotAllowed, missing email + non-empty allowlist
      returns ErrEmailMissingButRequired, subdomain NOT auto-accepted,
      parent-domain NOT auto-accepted, multi-entry first-match,
      multi-entry no-match).

Subdomain matching (alice@dev.acme.com against allowlist=[acme.com])
is intentionally NOT auto-accepted. The audit's MED-line tracks the
wildcard / suffix support story for v3; v2.1 ships strict.

Verification gate green:
- gofmt clean
- go vet clean
- go test -short -count=1 ./internal/auth/oidc/... ./internal/api/...
  ./internal/domain/auth/ — all pass (incl. existing OIDC service
  test suite, the 4 BCL tests, the auditor pin, and the AST
  RBAC-gate coverage guard).

Branch dev/auth-bundle-2 status post-commit: CRIT-1 (457962f),
CRIT-2 (c07825b), CRIT-3 (192351e), CRIT-4 (a89c69b), CRIT-5 (this)
— all five Criticals from the 2026-05-10 audit closed. v2.1.0 is
unblocked. HIGH-1..HIGH-12 + MEDs + LOWs are independently mergeable
follow-ups (spec at cowork/auth-bundles-fixes-2026-05-10/).

Refs: cowork/auth-bundles-audit-2026-05-10.md CRIT-5
This commit is contained in:
shankar0123
2026-05-10 20:30:32 +00:00
parent a89c69b751
commit 4dcbb3ed87
2 changed files with 175 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
internal/auth/oidc/email_domain_test.go
+113
View File
@@ -0,0 +1,113 @@
package oidc
import (
"errors"
"strings"
"testing"
)
// Audit 2026-05-10 CRIT-5 closure — email-domain allowlist enforcement.
// Tests the extractEmailDomain helper directly + the table-driven
// matcher logic. The full HandleCallback wiring is exercised by the
// existing OIDC service test suite (mockIdP + tokenSet); these tests
// pin the domain-extraction + match semantics that
// HandleCallback Step 7.5 relies on.
func TestExtractEmailDomain(t *testing.T) {
cases := []struct {
name string
input string
want string
wantErr bool
}{
{"plain", "alice@acme.com", "acme.com", false},
{"mixed-case-input", "Alice@ACME.com", "acme.com", false},
{"leading-trailing-whitespace", " bob@example.org ", "example.org", false},
{"subdomain-preserved", "alice@dev.acme.com", "dev.acme.com", false},
{"empty", "", "", true},
{"whitespace-only", " ", "", true},
{"no-at", "alice", "", true},
{"empty-local-part", "@acme.com", "", true},
{"empty-domain-part", "alice@", "", true},
// Multiple @ — addresses where the local-part is quoted and contains @
// are technically valid RFC but rare; we use LastIndex so the domain
// portion is unambiguous. Document this behavior in the test.
{"multiple-at-uses-last", "weird@user@acme.com", "acme.com", false},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
got, err := extractEmailDomain(tc.input)
if tc.wantErr {
if err == nil {
t.Fatalf("expected error for %q; got nil (returned %q)", tc.input, got)
}
return
}
if err != nil {
t.Fatalf("unexpected error for %q: %v", tc.input, err)
}
if got != tc.want {
t.Errorf("extractEmailDomain(%q) = %q; want %q", tc.input, got, tc.want)
}
})
}
}
// TestEmailDomainAllowlist_MatchSemantics pins the case-insensitive
// exact-match contract used by HandleCallback Step 7.5. Exhaustive
// over the cases the prompt's spec required.
func TestEmailDomainAllowlist_MatchSemantics(t *testing.T) {
cases := []struct {
name string
allowlist []string
email string
wantErr error
}{
{"empty-list — any domain accepted", nil, "alice@evil.com", nil},
{"matched lowercase", []string{"acme.com"}, "alice@acme.com", nil},
{"matched mixed-case allowlist entry", []string{"ACME.com"}, "alice@acme.com", nil},
{"matched mixed-case email", []string{"acme.com"}, "Alice@ACME.com", nil},
{"matched with whitespace in allowlist", []string{" acme.com "}, "alice@acme.com", nil},
{"unmatched", []string{"acme.com"}, "eve@evil.com", ErrEmailDomainNotAllowed},
{"missing email with non-empty list", []string{"acme.com"}, "", ErrEmailMissingButRequired},
{"subdomain NOT auto-accepted", []string{"acme.com"}, "alice@dev.acme.com", ErrEmailDomainNotAllowed},
{"parent-domain NOT auto-accepted", []string{"dev.acme.com"}, "alice@acme.com", ErrEmailDomainNotAllowed},
{"multi-entry first-match", []string{"first.com", "acme.com", "last.com"}, "alice@acme.com", nil},
{"multi-entry no-match", []string{"first.com", "second.com"}, "alice@third.com", ErrEmailDomainNotAllowed},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
got := checkEmailDomainAllowlist(tc.allowlist, tc.email)
if tc.wantErr == nil {
if got != nil {
t.Fatalf("expected nil error; got %v", got)
}
return
}
if !errors.Is(got, tc.wantErr) {
t.Errorf("got error %v; want %v", got, tc.wantErr)
}
})
}
}
// checkEmailDomainAllowlist mirrors HandleCallback Step 7.5 logic for
// direct testing. Keeps the test independent of mockIdP setup; the
// full integration test (mockIdP + tokenSet + HandleCallback) lives
// in service_test.go and exercises the same path via the IdP-shaped
// flow.
func checkEmailDomainAllowlist(allowlist []string, email string) error {
if len(allowlist) == 0 {
return nil
}
dom, err := extractEmailDomain(email)
if err != nil {
return ErrEmailMissingButRequired
}
for _, allowed := range allowlist {
if strings.EqualFold(strings.TrimSpace(allowed), dom) {
return nil
}
}
return ErrEmailDomainNotAllowed
}
internal/auth/oidc/service.go
+62
View File
@@ -215,6 +215,22 @@ var (
// to nothing or is malformed. Phase 3 fails closed.
ErrGroupsMissing = errors.New("oidc: configured groups claim missing or malformed")
// ErrEmailDomainNotAllowed: the configured
// OIDCProvider.AllowedEmailDomains list is non-empty but the
// authenticated user's email domain isn't in it. CRIT-5 closure
// of the 2026-05-10 audit (pre-fix, the field was persisted +
// surfaced through the API + MCP + GUI but never read here).
// Operator-facing: configure the IdP to issue tokens for only
// the right tenants, or add the domain to the provider's
// allowed_email_domains list.
ErrEmailDomainNotAllowed = errors.New("oidc: email domain not in allowlist")
// ErrEmailMissingButRequired: AllowedEmailDomains is set on the
// provider but the ID token / userinfo response did not surface
// an email claim. Operator-facing: ensure the IdP scope set
// includes `email` and the IdP releases the claim.
ErrEmailMissingButRequired = errors.New("oidc: provider requires email but token has none")
// ErrGroupsUnmapped: the user's groups don't match any of the
// operator's group_role_mappings for this provider. No session
// minted; audit row records auth.oidc_login_unmapped_groups.
@@ -493,6 +509,33 @@ func (s *Service) HandleCallback(
}
profile.Raw = raw
// Step 7.5: email-domain allowlist enforcement. Audit 2026-05-10
// CRIT-5 closure. When OIDCProvider.AllowedEmailDomains is non-
// empty, the user's email-domain MUST be in the list (case-
// insensitive exact match; subdomains are NOT auto-accepted — the
// operator must list each subdomain explicitly).
//
// Empty list (default for new providers) = any email domain
// accepted, matching the pre-fix behavior. Empty email claim with
// a non-empty allowlist = ErrEmailMissingButRequired (operators
// who set the allowlist explicitly expect email to be present).
if len(entry.cfgRow.AllowedEmailDomains) > 0 {
emailDomain, edErr := extractEmailDomain(profile.Email)
if edErr != nil {
return nil, ErrEmailMissingButRequired
}
matched := false
for _, allowed := range entry.cfgRow.AllowedEmailDomains {
if strings.EqualFold(strings.TrimSpace(allowed), emailDomain) {
matched = true
break
}
}
if !matched {
return nil, ErrEmailDomainNotAllowed
}
}
// Step 8: group claim resolution.
groups, err := groupclaim.Resolve(profile.Raw, entry.cfgRow.GroupsClaimPath)
if err != nil || len(groups) == 0 {
@@ -875,3 +918,22 @@ func decryptClientSecret(blob []byte, key string) ([]byte, error) {
}
return plain, nil
}
// extractEmailDomain returns the lowercase domain portion of an RFC
// 5322-ish email address. Used by HandleCallback Step 7.5 (CRIT-5
// closure) to enforce OIDCProvider.AllowedEmailDomains. Rejects empty
// input, addresses with no '@', and addresses with empty local-part
// or domain-part. Does NOT validate the full RFC grammar — IdPs are
// upstream of this and have their own validation; we only need a
// stable domain-extraction for the allowlist comparison.
func extractEmailDomain(email string) (string, error) {
email = strings.TrimSpace(email)
if email == "" {
return "", fmt.Errorf("empty email")
}
at := strings.LastIndex(email, "@")
if at <= 0 || at == len(email)-1 {
return "", fmt.Errorf("invalid email shape: %q", email)
}
return strings.ToLower(email[at+1:]), nil
}