Native GitHub auth for route protection, reshape auth provider add CLI

[phinze]

Route protection landed in v0.8.0 with a clean promise: identity arrives at your app as plain HTTP headers, no OAuth library required. That promise held directly for any provider with an OIDC discovery endpoint (Google, GitLab, Keycloak) but fell off for GitHub because GitHub doesn't expose .well-known/openid-configuration. The escape hatch in our docs was "stand up Dex yourself," which is a credibility gap on a very common identity provider. The rfd app in the next directory over had grown its own ~400 lines of GitHub OAuth handling as proof of the pain.

Rather than write GitHub-specific OAuth code ourselves and inherit the same one-provider-per-effort tax, this PR pulls in dexidp/dex/connector as a Go library. Dex's CallbackConnector is a stable two-method interface (LoginURL and HandleCallback), and there are roughly fourteen production-grade implementations behind it (GitHub, GitLab native, Microsoft, Bitbucket, LDAP, SAML, and so on). The work to wire one of them in equally enables the rest as cheap follow-ons.

The oidc_provider entity evolves to back both shapes rather than introducing a parallel connector_provider kind. An OIDC-style provider has empty (or "oidc") connector_type, populates provider_url + scopes, and runs through the existing pkg/oidc discovery + JWKS flow. A connector-backed provider sets connector_type to a non-empty value (e.g. "github") and stores the type-specific knobs in config_json. The kind name oidc_provider is a legacy keepsake (preserved so we don't have to migrate any existing v0.8.0 OIDC entities) but the entity is now genuinely the "auth provider" table.

httpingress dispatch reads honestly: when authMiddleware resolves a route's auth_provider to an oidc_provider entity, it peeks at connector_type and routes to either oidcMiddleware (existing OIDC discovery path) or connectorMiddleware (new Dex-connector path). The shared oidcSessionManager handles state and session cookies for both. The new connectorHandler mirrors oidcHandler with the small differences a connector needs (no PKCE, opaque connData blob round-tripped through its own cookie, identity comes back from HandleCallback instead of an ID token).

CLI surface arrives in a reshaped form. v0.8.0 left us with three shapes for what is conceptually one operation: auth provider add NAME --provider-url ... for OIDC, auth provider add-password NAME ... for passwords, and adding a connector flag (--connector github) on top would have made the asymmetry worse with every new connector. The second commit replaces all three with per-type subcommands: auth provider add oidc | github | password NAME [flags], where each type has its own help screen with only the flags that apply. Adding github reads as miren auth provider add github my-github --client-id ... --client-secret ... --org mirendev:engineering. --org NAME for any-org-member access, --org NAME:team1,team2 for team-scoped access. Repeatable for multiple orgs.

End-to-end smoke verified against real GitHub OAuth on a dev cluster: log in via the browser, get bounced through GitHub, land back on the protected app with X-User-Email, X-User-Login, X-User-Id, and X-User-Groups: ["org:team"] headers populated by Miren.

One Dex-shape gotcha that bit us during the smoke and now lives in docs/docs/route-protect.md: bare --org NAME authorizes the user but emits no groups claim, because Dex's GitHub connector only surfaces team-prefixed entries (org:team format). For org-level signal without teams, operators rely on the request reaching the app at all, or they configure team filters.

The reshape breaks the v0.8.0 CLI for existing users: prepend oidc to existing OIDC commands, swap add-password for add password (with a space). Pre-1.0 leeway and a narrow audience meant we leaned toward a hard break with a CHANGELOG migration note rather than carrying a soft-deprecation path. Tracked separately as MIR-1161 so the entity-layer connector work and the CLI reshape land as two commits in this PR.

The Dex import drags otel up from 1.38 to 1.43, grpc-contrib from 0.61 to 0.67, cel-go from 0.24.1 to 0.28.1, x/oauth2 from 0.32 to 0.36, plus minor bumps to cbor, go-jose, jwt, sqlite, prometheus, and pflag. All minor-version moves; project still builds and make lint is clean. We bundled these into this PR rather than splitting them off since none are major bumps and the cluster of related changes reads more coherently as one.

Closes MIR-1160. Closes MIR-1161. Adjacent work this builds toward: MIR-889 (pass-through auth, the other half of the rfd dogfood story) and MIR-885 (path-scoped route protection, where multi-connector composition like "/users via oidc, /admin via github with group check" lands).

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

This pull request adds GitHub connector-backed authentication as an alternative to OIDC providers. The OidcProvider schema gains connector_type and config_json fields to support both OIDC and connector modes. A new connectors package wraps Dex GitHub connectors and exposes a standardized Connector interface. The HTTP ingress gains a connectorMiddleware that orchestrates login redirects, state/callback validation, session creation, and claim injection. CLI commands add provider-specific subcommands (oidc|github|password) and helpers to manage connector providers and display connector fields. Documentation and dependency updates (including github.com/dexidp/dex) complete the feature.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

cli/commands/route_show.go (1)

103-114: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add connector_type to JSON route output for connector-protected routes.

route show --format json currently omits connector type, even though text output includes it. This breaks parity and machine-readable introspection for connector routes (Line 103 and Line 143).

Suggested patch

 		type RouteJSON struct {
 			Host            string              `json:"host"`
 			App             string              `json:"app"`
 			Default         bool                `json:"default"`
 			Protected       bool                `json:"protected"`
 			ProtectionType  string              `json:"protection_type"`
 			ProviderName    string              `json:"provider_name,omitempty"`
 			ProviderURL     string              `json:"provider_url,omitempty"`
+			ConnectorType   string              `json:"connector_type,omitempty"`
 			ProviderMissing bool                `json:"provider_missing,omitempty"`
 			ClaimMappings   []map[string]string `json:"claim_mappings,omitempty"`
 			WafLevel        int                 `json:"waf_level"`
 		}
@@
 			case connProvider != nil:
 				r.ProviderName = connProvider.Name
+				r.ConnectorType = connProvider.ConnectorType
 				for _, m := range route.ClaimMappings {
 					r.ClaimMappings = append(r.ClaimMappings, map[string]string{
 						"claim":  m.Claim,
 						"header": m.Header,
 					})
 				}

Also applies to: 143-150

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cli/commands/route_show.go` around lines 103 - 114, The RouteJSON struct is
missing connector type for connector-protected routes; add a new field
ConnectorType string `json:"connector_type,omitempty"` to RouteJSON and ensure
the code that builds the JSON output for routes (the same place that sets
ProviderName/ProviderURL/ProviderMissing and ProtectionType) populates
RouteJSON.ConnectorType from the route's connector/protection info so `route
show --format json` includes the connector_type value for connector-protected
routes; update all places that construct RouteJSON (the JSON path that mirrors
the text output) to set this field.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@go.mod`:
- Around line 35-36: The go.mod pins vulnerable Docker modules; update the
versions for github.com/docker/cli and github.com/docker/docker to a safe
release (preferably v29.5.2, or at minimum v29.3.1) to address the AuthZ plugin
vulnerability, then run module tooling (go get github.com/docker/cli@v29.5.2
github.com/docker/docker@v29.5.2 and go mod tidy) and run the test suite/CI to
validate; ensure the updated module lines for github.com/docker/cli and
github.com/docker/docker reflect the new versions and commit the changed go.mod
and go.sum.

In `@servers/httpingress/connector.go`:
- Around line 164-169: The callback currently treats missing connData cookie as
a hard error; change the GetNamedCookie error handling in the callback so a
missing/empty connData cookie (set intentionally omitted by redirectToProvider /
miren_oidc_conn flows) is treated as optional: when
h.sessionManager.GetNamedCookie(r, connDataCookieName) returns an error
indicating the cookie is not present, log at debug/trace and continue with
nil/empty connData instead of returning HTTP 400; only return the error response
for genuine read errors (non-missing errors). Reference:
h.sessionManager.GetNamedCookie, connDataCookieName, redirectToProvider,
miren_oidc_conn.

---

Outside diff comments:
In `@cli/commands/route_show.go`:
- Around line 103-114: The RouteJSON struct is missing connector type for
connector-protected routes; add a new field ConnectorType string
`json:"connector_type,omitempty"` to RouteJSON and ensure the code that builds
the JSON output for routes (the same place that sets
ProviderName/ProviderURL/ProviderMissing and ProtectionType) populates
RouteJSON.ConnectorType from the route's connector/protection info so `route
show --format json` includes the connector_type value for connector-protected
routes; update all places that construct RouteJSON (the JSON path that mirrors
the text output) to set this field.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: fbe73ff8-0d5f-4063-88f7-83d3d4df355a

📥 Commits

Reviewing files that changed from the base of the PR and between 10dd9b4 and 8987934.

⛔ Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

📒 Files selected for processing (13)

• api/ingress/client.go
• api/ingress/ingress_v1alpha/schema.gen.go
• api/ingress/schema.yml
• cli/commands/auth_provider.go
• cli/commands/route_protect.go
• cli/commands/route_show.go
• docs/docs/command/auth-provider-add.md
• docs/docs/route-protect.md
• go.mod
• pkg/connectors/connectors.go
• pkg/connectors/connectors_test.go
• servers/httpingress/connector.go
• servers/httpingress/httpingress.go

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (1)

servers/httpingress/connector.go (1)

164-169: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Handle missing connData cookie gracefully for connectors that don't populate it on LoginURL.

The GitHub connector returns nil connData from LoginURL (line 56-58 in test confirms this), so redirectToProvider skips setting the cookie (line 129). However, handleCallback fails hard when the cookie is absent. This will break GitHub authentication flows.

Proposed fix

 	connData, err := h.sessionManager.GetNamedCookie(r, connDataCookieName)
 	if err != nil {
-		h.logger.Error("failed to read conn data cookie", "error", err)
-		http.Error(w, "Invalid session", http.StatusBadRequest)
-		return
+		// Some connectors (e.g. GitHub) don't return connData from LoginURL.
+		// Treat missing cookie as empty connData.
+		h.logger.Debug("no conn data cookie present, using empty connData", "error", err)
+		connData = nil
 	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@servers/httpingress/connector.go` around lines 164 - 169, The handler
currently treats a missing connData cookie from h.sessionManager.GetNamedCookie
as a fatal error, which breaks connectors (like GitHub) that intentionally don't
set the cookie in redirectToProvider; update handleCallback to accept a
nil/absent connData instead of returning an error: call GetNamedCookie as before
but if it returns no error and a nil cookie, do not http.Error/return — instead
set connData to a zero-value/default (or nil) and proceed with the rest of
handleCallback logic, ensuring subsequent code that reads connData (use of
connDataCookieName, any parsing or lookups) can handle a nil value; reference
functions/identifiers: handleCallback, redirectToProvider,
h.sessionManager.GetNamedCookie, connDataCookieName.

🧹 Nitpick comments (2)

servers/httpingress/connector.go (1)

178-181: 💤 Low value

Consider making session expiry configurable rather than hardcoded.

The 24-hour session expiry is hardcoded. While reasonable as a default, this differs from OIDC flows where token expiry may vary. Consider extracting this to a constant or making it configurable for consistency.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@servers/httpingress/connector.go` around lines 178 - 181, The session
Expiration is hardcoded when creating the oidc.SessionData (ExpiresAt:
time.Now().Add(24 * time.Hour)); change this to use a configurable value or
named constant instead: add a configurable setting (e.g., SessionExpiry or
SessionExpiryDuration) exposed via the existing config mechanism or a
package-level constant and replace the literal 24*time.Hour with that value when
constructing session in the function that creates oidc.SessionData; ensure the
config has a sensible default (24h) and update any docs or tests that rely on
the former hardcoded behaviour.

cli/commands/route_protect.go (1)

154-169: ⚡ Quick win

Consider extracting claim-header parsing to a shared helper.

The claim-header parsing logic (lines 154-169) is duplicated from the OIDC path (lines 70-85). Extracting to a helper like parseClaimMappings([]string) ([]ingress_v1alpha.ClaimMappings, error) would reduce duplication and ensure consistent validation.

Example helper extraction

// parseClaimMappings converts "claim:header" specs to ClaimMappings.
func parseClaimMappings(specs []string) ([]ingress_v1alpha.ClaimMappings, error) {
	var mappings []ingress_v1alpha.ClaimMappings
	for _, mapping := range specs {
		parts := strings.SplitN(mapping, ":", 2)
		if len(parts) != 2 {
			return nil, fmt.Errorf("invalid claim-header mapping format: %s (expected 'claim:header')", mapping)
		}
		claim := strings.TrimSpace(parts[0])
		header := strings.TrimSpace(parts[1])
		if claim == "" || header == "" {
			return nil, fmt.Errorf("invalid claim-header mapping format: %q (expected non-empty 'claim:header')", mapping)
		}
		mappings = append(mappings, ingress_v1alpha.ClaimMappings{
			Claim:  claim,
			Header: header,
		})
	}
	return mappings, nil
}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cli/commands/route_protect.go` around lines 154 - 169, Extract the duplicated
"claim:header" parsing into a helper function parseClaimMappings(specs []string)
([]ingress_v1alpha.ClaimMappings, error) that performs the same validation and
trimming currently done inline (the logic using strings.SplitN,
strings.TrimSpace and error messages for invalid format or empty parts); replace
the inline loops in both the OIDC path and the route_protect code that iterate
over opts.ClaimHeader and build []ingress_v1alpha.ClaimMappings with a single
call to parseClaimMappings(opts.ClaimHeader) and propagate any error upward,
ensuring callers use the returned []ingress_v1alpha.ClaimMappings instead of
building it themselves.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cli/commands/route_show.go`:
- Around line 209-224: The current conditional `if connProvider != nil &&
len(route.ClaimMappings) > 0 { ... }` hides claim mappings when the connector
provider entity was deleted; change the guard to only check the presence of
mappings (e.g., `if len(route.ClaimMappings) > 0 { ... }`) so the block that
builds `rows` from `route.ClaimMappings`, creates the `headers`/`columns`,
`table := ui.NewTable(...)`, and `ctx.Printf("\n%s\n", table.Render())` still
runs regardless of `connProvider` being nil.

In `@go.mod`:
- Line 34: The go.mod entry for the Dex dependency pins github.com/dexidp/dex to
a pseudo-version that contains critical vulnerabilities; update that module to
v2.45.1 or later (e.g., change the version for github.com/dexidp/dex to
v2.45.1), then run module resolution commands (go get
github.com/dexidp/dex@v2.45.1 and go mod tidy / go mod download) to refresh the
lockfile and ensure builds/tests pass before merging.

---

Duplicate comments:
In `@servers/httpingress/connector.go`:
- Around line 164-169: The handler currently treats a missing connData cookie
from h.sessionManager.GetNamedCookie as a fatal error, which breaks connectors
(like GitHub) that intentionally don't set the cookie in redirectToProvider;
update handleCallback to accept a nil/absent connData instead of returning an
error: call GetNamedCookie as before but if it returns no error and a nil
cookie, do not http.Error/return — instead set connData to a zero-value/default
(or nil) and proceed with the rest of handleCallback logic, ensuring subsequent
code that reads connData (use of connDataCookieName, any parsing or lookups) can
handle a nil value; reference functions/identifiers: handleCallback,
redirectToProvider, h.sessionManager.GetNamedCookie, connDataCookieName.

---

Nitpick comments:
In `@cli/commands/route_protect.go`:
- Around line 154-169: Extract the duplicated "claim:header" parsing into a
helper function parseClaimMappings(specs []string)
([]ingress_v1alpha.ClaimMappings, error) that performs the same validation and
trimming currently done inline (the logic using strings.SplitN,
strings.TrimSpace and error messages for invalid format or empty parts); replace
the inline loops in both the OIDC path and the route_protect code that iterate
over opts.ClaimHeader and build []ingress_v1alpha.ClaimMappings with a single
call to parseClaimMappings(opts.ClaimHeader) and propagate any error upward,
ensuring callers use the returned []ingress_v1alpha.ClaimMappings instead of
building it themselves.

In `@servers/httpingress/connector.go`:
- Around line 178-181: The session Expiration is hardcoded when creating the
oidc.SessionData (ExpiresAt: time.Now().Add(24 * time.Hour)); change this to use
a configurable value or named constant instead: add a configurable setting
(e.g., SessionExpiry or SessionExpiryDuration) exposed via the existing config
mechanism or a package-level constant and replace the literal 24*time.Hour with
that value when constructing session in the function that creates
oidc.SessionData; ensure the config has a sensible default (24h) and update any
docs or tests that rely on the former hardcoded behaviour.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 21686eb8-315d-428f-9b69-68b2b570981f

📥 Commits

Reviewing files that changed from the base of the PR and between 8987934 and 3b517fe.

⛔ Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

📒 Files selected for processing (13)

• api/ingress/client.go
• api/ingress/ingress_v1alpha/schema.gen.go
• api/ingress/schema.yml
• cli/commands/auth_provider.go
• cli/commands/route_protect.go
• cli/commands/route_show.go
• docs/docs/command/auth-provider-add.md
• docs/docs/route-protect.md
• go.mod
• pkg/connectors/connectors.go
• pkg/connectors/connectors_test.go
• servers/httpingress/connector.go
• servers/httpingress/httpingress.go

✅ Files skipped from review due to trivial changes (1)

• api/ingress/ingress_v1alpha/schema.gen.go

[evanphx]

Love it! Some good places to eventually dry up when we add more middleware work, but no need now.

[phinze]

Heads up — pushed a follow-up after you approved that collapses the new connector_provider entity into the existing oidc_provider. The split was the wrong shape: OIDC is itself a connector type (Dex even has connector/oidc), and having two parallel entity kinds was creating duplicate CLI paths, duplicate dispatch, and duplicate name-collision checks for what's conceptually one auth-provider table.

Now oidc_provider carries a connector_type field. Empty or "oidc" keeps the existing OIDC discovery flow; non-empty names a Dex-backed connector. Kind name is a legacy keepsake (skips migrating v0.8.0 OIDC entities) but the entity is now genuinely "auth_provider" in spirit. Single CLI surface, single dispatch with an inner branch on connector_type.

If you've got the cycles I'd love a re-look.

[evanphx]

@phinze no prob, good to reuse that pipeline, make sense!

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@servers/httpingress/connector.go`:
- Line 291: The code mutates shared s.oidcSessionManager by calling
SetSecure(scheme == "https") per request which creates races and incorrect
Secure attributes; instead stop changing the shared manager at request time —
either determine secure := (scheme == "https") per request and pass that flag
into the cookie/session save functions so cookies are created with the right
Secure attribute, or create two preconfigured session managers (e.g.,
oidcSessionManagerSecure and oidcSessionManagerInsecure) and select the
appropriate one based on scheme without mutating them; update any callers that
relied on s.oidcSessionManager.SetSecure to use the per-request secure flag or
the selected manager (references: s.oidcSessionManager, SetSecure, scheme).
- Around line 191-197: The returnPath taken from state.ReturnPath must be
validated/sanitized before use to prevent open redirect; update the logic around
returnPath (the variable reading state.ReturnPath used before h.logger.Info and
http.Redirect) to allow only safe local paths (e.g. require it to start with a
single '/' and reject paths that start with '//' or contain a scheme/host like
'http:' or '//'); if validation fails, fall back to "/" and then log and call
http.Redirect as before. Ensure you check returnPath for leading '//' and URL
scheme/host components and normalize or replace it prior to calling
h.logger.Info and http.Redirect.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a46e47d0-1cc5-44a2-ac6b-db2251161305

📥 Commits

Reviewing files that changed from the base of the PR and between c2e5bb5 and 87fb744.

⛔ Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

📒 Files selected for processing (24)

• api/ingress/ingress_v1alpha/schema.gen.go
• api/ingress/schema.yml
• blackbox/route_password_test.go
• cli/commands/auth_provider.go
• cli/commands/auth_provider_github.go
• cli/commands/auth_provider_helpers.go
• cli/commands/auth_provider_oidc.go
• cli/commands/commands.go
• cli/commands/route_protect.go
• cli/commands/route_show.go
• docs/command-sidebar.json
• docs/docs/changelog.md
• docs/docs/command/auth-provider-add-github.md
• docs/docs/command/auth-provider-add-oidc.md
• docs/docs/command/auth-provider-add-password.md
• docs/docs/command/auth-provider-add.md
• docs/docs/command/auth-provider.md
• docs/docs/commands.md
• docs/docs/route-protect.md
• go.mod
• pkg/connectors/connectors.go
• pkg/connectors/connectors_test.go
• servers/httpingress/connector.go
• servers/httpingress/httpingress.go

💤 Files with no reviewable changes (1)

• docs/docs/command/auth-provider.md

✅ Files skipped from review due to trivial changes (4)

• docs/docs/command/auth-provider-add-oidc.md
• docs/command-sidebar.json
• docs/docs/command/auth-provider-add.md
• api/ingress/ingress_v1alpha/schema.gen.go