From 841fdc6c533d712a6d3af3b2e4881b0986421f78 Mon Sep 17 00:00:00 2001 From: John Wregglesworth Date: Thu, 14 May 2026 09:57:31 -0700 Subject: [PATCH 1/4] Add vice-users OAuth callback relay to vice-operator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit vice-proxy can no longer send per-app VICE subdomains as the Keycloak redirect_uri — Keycloak only wildcard-matches a trailing *, so https://*.cyverse.run:4343/* never matches a real app. The operator now exposes a fixed /vice-users/callback endpoint that is registered in Keycloak as the single static redirect_uri for the vice-users client. The handler is a stateless relay: it verifies the HMAC-signed state (go-mod/viceauth), recovers the original app URL from it, checks that URL is a single-label subdomain of the VICE base domain (open-redirect guard), and bounces the browser back with the authorization code intact. vice-proxy holds the client secret and does the token exchange itself. New --public-url and --state-hmac-secret flags feed two new cluster-config keys (OPERATOR_CALLBACK_URL, STATE_HMAC_SECRET) consumed by vice-proxy via EnvFrom. The state HMAC secret must be stable across operator restarts. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/vice-operator/app.go | 13 ++- cmd/vice-operator/main.go | 38 ++++++- cmd/vice-operator/viceusersauth.go | 86 ++++++++++++++++ cmd/vice-operator/viceusersauth_test.go | 131 ++++++++++++++++++++++++ go.mod | 1 + go.sum | 2 + 6 files changed, 267 insertions(+), 4 deletions(-) create mode 100644 cmd/vice-operator/viceusersauth.go create mode 100644 cmd/vice-operator/viceusersauth_test.go diff --git a/cmd/vice-operator/app.go b/cmd/vice-operator/app.go index d339f708..6259ce5f 100644 --- a/cmd/vice-operator/app.go +++ b/cmd/vice-operator/app.go @@ -29,8 +29,9 @@ type App struct { // must additionally carry adminRole in realm_access.roles or at least one // value in adminEntitlements in its entitlement claim — the API is admin-only. // swaggerCfg controls the Swagger UI login gate; when disabled, docs are served -// without authentication. -func NewApp(op *operator.Operator, verifier *oidc.IDTokenVerifier, expectedClientID string, swaggerCfg *SwaggerAuthConfig, adminRole string, adminEntitlements []string) *App { +// without authentication. When viceUsersCfg is non-nil, the unauthenticated +// vice-users OAuth callback relay is registered. +func NewApp(op *operator.Operator, verifier *oidc.IDTokenVerifier, expectedClientID string, swaggerCfg *SwaggerAuthConfig, adminRole string, adminEntitlements []string, viceUsersCfg *ViceUsersAuthConfig) *App { e := echo.New() e.Use(middleware.Logger()) e.Use(middleware.Recover()) @@ -59,6 +60,14 @@ func NewApp(op *operator.Operator, verifier *oidc.IDTokenVerifier, expectedClien e.GET("/docs/*", echoSwagger.EchoWrapHandler(echoSwagger.InstanceName("operator"))) } + // vice-users OAuth callback relay — unauthenticated and outside both the + // Bearer auth group and the /docs login gate, since Keycloak redirects the + // end user's browser here directly. Registered only when a state HMAC + // secret is configured. + if viceUsersCfg != nil { + e.GET(viceUsersCallbackPath, handleViceUsersCallback(viceUsersCfg)) + } + // All API routes go through an optional auth group. api := e.Group("") if verifier != nil { diff --git a/cmd/vice-operator/main.go b/cmd/vice-operator/main.go index c7bef7fd..8efa7568 100644 --- a/cmd/vice-operator/main.go +++ b/cmd/vice-operator/main.go @@ -9,10 +9,12 @@ import ( "fmt" "net/url" "os" + "strings" "github.com/cyverse-de/app-exposer/common" "github.com/cyverse-de/app-exposer/constants" "github.com/cyverse-de/app-exposer/operator" + "github.com/cyverse-de/go-mod/viceauth" "github.com/sirupsen/logrus" ) @@ -56,6 +58,8 @@ func main() { swaggerClientID string swaggerClientSecret string swaggerCookieSecret string + publicURL string + stateHMACSecret string apiSubdomain string apiServiceName string serviceCIDR string @@ -104,6 +108,8 @@ func main() { flag.StringVar(&swaggerClientID, "swagger-client-id", "", "OAuth2 client ID for the Swagger UI login flow (must support authorization code flow in Keycloak)") flag.StringVar(&swaggerClientSecret, "swagger-client-secret", "", "OAuth2 client secret for the Swagger UI login flow (or SWAGGER_CLIENT_SECRET env var)") flag.StringVar(&swaggerCookieSecret, "swagger-cookie-secret", "", "Secret for signing session cookies (random string; auto-generated if empty; or SWAGGER_COOKIE_SECRET env var)") + flag.StringVar(&publicURL, "public-url", "", "Public base URL of this operator (e.g. https://vice-operator-qa.cyverse.org); combined with the vice-users callback path to form the static OAuth redirect_uri") + flag.StringVar(&stateHMACSecret, "state-hmac-secret", "", "Shared HMAC secret for signing the vice-proxy OAuth state parameter (or STATE_HMAC_SECRET env var); must be stable across operator restarts") flag.StringVar(&apiSubdomain, "api-subdomain", "vice-api", "Subdomain prefix for the vice-operator API HTTPRoute; combined with --vice-base-url host to form the full hostname") flag.StringVar(&apiServiceName, "api-service-name", "vice-operator", "K8s Service name for the vice-operator API HTTPRoute backend") flag.StringVar(&serviceCIDR, "service-cidr", "", "Cluster service CIDR to block in egress (auto-detected from kubernetes API server if empty)") @@ -127,6 +133,7 @@ func main() { envFallback(&keycloakClientSecret, "KEYCLOAK_CLIENT_SECRET") envFallback(&swaggerClientSecret, "SWAGGER_CLIENT_SECRET") envFallback(&swaggerCookieSecret, "SWAGGER_COOKIE_SECRET") + envFallback(&stateHMACSecret, "STATE_HMAC_SECRET") envFallback(&adminEntitlementsRaw, "ADMIN_ENTITLEMENTS") // --admin-role has a non-empty default, so the envFallback "is empty?" // trick can't distinguish "user passed default" from "user didn't pass @@ -180,6 +187,16 @@ func main() { log.Fatalf("%v", err) } + // operatorCallbackURL is the single static redirect_uri vice-proxy sends to + // Keycloak for the vice-users client. vice-proxy carries the real app URL + // in a signed state blob; this operator relays the authorization code back + // to it (see handleViceUsersCallback). Left empty when --public-url is + // unset so vice-proxy's own startup validation can flag the gap. + var operatorCallbackURL string + if publicURL != "" { + operatorCallbackURL = strings.TrimSuffix(publicURL, "/") + viceUsersCallbackPath + } + // Build the cluster config map from flags. All keys are always written so // that stale values from a previous run are overwritten. The secret update // replaces the entire data map — omitting a key here removes it from the @@ -190,6 +207,10 @@ func main() { "KEYCLOAK_REALM": keycloakRealm, "KEYCLOAK_CLIENT_ID": keycloakClientID, "KEYCLOAK_CLIENT_SECRET": keycloakClientSecret, + // Static OAuth redirect_uri and the shared secret signing the state + // parameter that round-trips through the operator's callback relay. + "OPERATOR_CALLBACK_URL": operatorCallbackURL, + "STATE_HMAC_SECRET": stateHMACSecret, // Propagated to vice-proxy (via EnvFrom) so it can grant admins // access to running analyses regardless of the per-analysis // allowed-users list. @@ -200,11 +221,14 @@ func main() { } else { clusterConfig["DISABLE_AUTH"] = "false" - // Warn early if auth is enabled but Keycloak settings are missing, + // Warn early if auth is enabled but required settings are missing, // since vice-proxy pods will crash-loop with a fatal validation error. if keycloakBaseURL == "" || keycloakRealm == "" || keycloakClientID == "" || keycloakClientSecret == "" { log.Warn("auth is enabled (--disable-vice-proxy-auth not set) but one or more Keycloak flags are empty; vice-proxy pods will fail to start") } + if publicURL == "" || stateHMACSecret == "" { + log.Warn("auth is enabled but --public-url or --state-hmac-secret is empty; vice-proxy pods will fail to start and the OAuth callback relay is disabled") + } } // Ensure cluster config secret so vice-proxy containers can reference it @@ -353,7 +377,17 @@ func main() { log.Fatalf("%v", err) } - app := NewApp(op, verifier, apiAuthClientID, swaggerCfg, adminRole, adminEntitlements) + // The vice-users OAuth callback relay is enabled only when a state HMAC + // secret is configured; baseDomain bounds where the relay may redirect. + var viceUsersCfg *ViceUsersAuthConfig + if stateHMACSecret != "" { + viceUsersCfg = &ViceUsersAuthConfig{ + StateCodec: viceauth.NewCodec([]byte(stateHMACSecret)), + BaseDomain: baseDomain, + } + } + + app := NewApp(op, verifier, apiAuthClientID, swaggerCfg, adminRole, adminEntitlements, viceUsersCfg) loadingApp := NewLoadingApp(op) apiAddr := fmt.Sprintf(":%d", port) diff --git a/cmd/vice-operator/viceusersauth.go b/cmd/vice-operator/viceusersauth.go new file mode 100644 index 00000000..78ca0fc7 --- /dev/null +++ b/cmd/vice-operator/viceusersauth.go @@ -0,0 +1,86 @@ +package main + +import ( + "fmt" + "net/http" + "net/url" + "strings" + + "github.com/cyverse-de/go-mod/viceauth" + "github.com/labstack/echo/v4" +) + +// viceUsersCallbackPath is the fixed path of the OAuth callback relay. It is +// combined with --public-url to form OPERATOR_CALLBACK_URL, the single static +// redirect_uri registered in Keycloak for the vice-users client. +const viceUsersCallbackPath = "/vice-users/callback" + +// ViceUsersAuthConfig configures the vice-users OAuth callback relay. +type ViceUsersAuthConfig struct { + // StateCodec verifies the HMAC-signed OAuth state produced by vice-proxy. + StateCodec *viceauth.Codec + + // BaseDomain is the VICE base domain (e.g. "cyverse.run"). The relay only + // redirects to single-label subdomains of this domain, so a forged or + // tampered state cannot turn the callback into an open redirect. + BaseDomain string +} + +// isAllowedHost reports whether host is a single-label subdomain of the VICE +// base domain. host is expected to already have any port stripped (as +// url.URL.Hostname does). This is the open-redirect guard for the relay. +func (cfg *ViceUsersAuthConfig) isAllowedHost(host string) bool { + suffix := "." + cfg.BaseDomain + label, ok := strings.CutSuffix(host, suffix) + if !ok { + return false + } + return label != "" && !strings.Contains(label, ".") +} + +// handleViceUsersCallback returns the handler for GET /vice-users/callback. +// +// vice-proxy registers this operator's URL as the single static redirect_uri +// for the Keycloak "vice-users" client, because Keycloak cannot wildcard-match +// per-app VICE subdomains. Keycloak delivers the authorization code here; the +// handler recovers the original app URL from the signed state and relays the +// browser back to it with the code intact. It is intentionally stateless and +// does no token exchange — vice-proxy holds the client secret and redeems the +// code itself. +func handleViceUsersCallback(cfg *ViceUsersAuthConfig) echo.HandlerFunc { + return func(c echo.Context) error { + q := c.Request().URL.Query() + + // Surface Keycloak-side errors instead of relaying a useless redirect. + if errParam := q.Get("error"); errParam != "" { + return echo.NewHTTPError(http.StatusUnauthorized, + fmt.Sprintf("Keycloak error: %s — %s", errParam, q.Get("error_description"))) + } + + code := q.Get("code") + state := q.Get("state") + if code == "" || state == "" { + return echo.NewHTTPError(http.StatusBadRequest, "missing code or state") + } + + // A decode error means a forged or corrupted state — refuse to relay. + claims, err := cfg.StateCodec.Decode(state) + if err != nil { + return echo.NewHTTPError(http.StatusBadRequest, "invalid state") + } + + origin, err := url.Parse(claims.Origin) + if err != nil || origin.Scheme != "https" || !cfg.isAllowedHost(origin.Hostname()) { + return echo.NewHTTPError(http.StatusBadRequest, "invalid redirect target") + } + + // Re-attach code + state so vice-proxy can validate state against its + // cookie and exchange the code. Preserve any path/query already present. + oq := origin.Query() + oq.Set("code", code) + oq.Set("state", state) + origin.RawQuery = oq.Encode() + + return c.Redirect(http.StatusTemporaryRedirect, origin.String()) + } +} diff --git a/cmd/vice-operator/viceusersauth_test.go b/cmd/vice-operator/viceusersauth_test.go new file mode 100644 index 00000000..b2a93a59 --- /dev/null +++ b/cmd/vice-operator/viceusersauth_test.go @@ -0,0 +1,131 @@ +package main + +import ( + "net/http" + "net/http/httptest" + "net/url" + "testing" + + "github.com/cyverse-de/go-mod/viceauth" + "github.com/labstack/echo/v4" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestIsAllowedHost(t *testing.T) { + cfg := &ViceUsersAuthConfig{BaseDomain: "cyverse.run"} + tests := []struct { + host string + want bool + }{ + {"a1234abcd.cyverse.run", true}, + {"cyverse.run", false}, // base domain itself, no subdomain + {"a.b.cyverse.run", false}, // nested subdomain + {"a1234.cyverse.run.evil.com", false}, + {"xcyverse.run", false}, // suffix without the dot boundary + {"evil.com", false}, + {"", false}, + } + for _, tt := range tests { + t.Run(tt.host, func(t *testing.T) { + assert.Equal(t, tt.want, cfg.isAllowedHost(tt.host)) + }) + } +} + +// runCallback invokes the callback handler against a synthetic request and +// returns the recorder. echo.HTTPError results are run through the default +// error handler so the recorder reflects the status the client would see. +func runCallback(t *testing.T, cfg *ViceUsersAuthConfig, rawQuery string) *httptest.ResponseRecorder { + t.Helper() + e := echo.New() + req := httptest.NewRequest(http.MethodGet, "/vice-users/callback?"+rawQuery, nil) + rec := httptest.NewRecorder() + c := e.NewContext(req, rec) + if err := handleViceUsersCallback(cfg)(c); err != nil { + e.HTTPErrorHandler(err, c) + } + return rec +} + +func TestHandleViceUsersCallback(t *testing.T) { + codec := viceauth.NewCodec([]byte("test-secret")) + cfg := &ViceUsersAuthConfig{StateCodec: codec, BaseDomain: "cyverse.run"} + + state := func(t *testing.T, c *viceauth.Codec, origin string) string { + t.Helper() + s, err := c.Encode(viceauth.StateClaims{StateID: "sid", Origin: origin}) + require.NoError(t, err) + return s + } + + validState := state(t, codec, "https://a1234abcd.cyverse.run:4343/foo?x=1") + + tests := []struct { + name string + query url.Values + wantCode int + }{ + { + name: "valid bounce", + query: url.Values{"code": {"abc"}, "state": {validState}}, + wantCode: http.StatusTemporaryRedirect, + }, + { + name: "keycloak error param", + query: url.Values{"error": {"access_denied"}, "error_description": {"nope"}}, + wantCode: http.StatusUnauthorized, + }, + { + name: "missing code", + query: url.Values{"state": {validState}}, + wantCode: http.StatusBadRequest, + }, + { + name: "missing state", + query: url.Values{"code": {"abc"}}, + wantCode: http.StatusBadRequest, + }, + { + name: "bad signature", + query: url.Values{"code": {"abc"}, "state": {state(t, viceauth.NewCodec([]byte("other-secret")), "https://a1234abcd.cyverse.run/")}}, + wantCode: http.StatusBadRequest, + }, + { + name: "off-domain origin", + query: url.Values{"code": {"abc"}, "state": {state(t, codec, "https://evil.com/")}}, + wantCode: http.StatusBadRequest, + }, + { + name: "nested subdomain origin", + query: url.Values{"code": {"abc"}, "state": {state(t, codec, "https://a.b.cyverse.run/")}}, + wantCode: http.StatusBadRequest, + }, + { + name: "non-https origin", + query: url.Values{"code": {"abc"}, "state": {state(t, codec, "http://a1234abcd.cyverse.run/")}}, + wantCode: http.StatusBadRequest, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rec := runCallback(t, cfg, tt.query.Encode()) + require.Equal(t, tt.wantCode, rec.Code) + + if tt.wantCode != http.StatusTemporaryRedirect { + return + } + // On a successful bounce the browser is sent back to the app's + // own URL with code + state re-attached and the original query + // preserved. + loc, err := url.Parse(rec.Header().Get("Location")) + require.NoError(t, err) + assert.Equal(t, "https", loc.Scheme) + assert.Equal(t, "a1234abcd.cyverse.run:4343", loc.Host) + assert.Equal(t, "/foo", loc.Path) + assert.Equal(t, "abc", loc.Query().Get("code")) + assert.Equal(t, validState, loc.Query().Get("state")) + assert.Equal(t, "1", loc.Query().Get("x")) + }) + } +} diff --git a/go.mod b/go.mod index 1eb6c38d..a6ffe0a1 100644 --- a/go.mod +++ b/go.mod @@ -26,6 +26,7 @@ require ( github.com/cyverse-de/go-mod/otelutils v0.0.3 github.com/cyverse-de/go-mod/pbinit v0.1.11 github.com/cyverse-de/go-mod/protobufjson v0.0.3 + github.com/cyverse-de/go-mod/viceauth v0.0.0-20260514164934-ce1628b2909f github.com/cyverse-de/messaging/v12 v12.0.0 github.com/cyverse-de/model/v10 v10.0.0 github.com/cyverse-de/p/go/qms v0.1.13 diff --git a/go.sum b/go.sum index b8c216f2..38e36134 100644 --- a/go.sum +++ b/go.sum @@ -129,6 +129,8 @@ github.com/cyverse-de/go-mod/pbinit v0.1.11 h1:HUGZ5Q0yOwkp8vnGjJzmKX7FweX96f68I github.com/cyverse-de/go-mod/pbinit v0.1.11/go.mod h1:eC6iC5kAxbWvJgZcqK04qeQbioX6KYwBoBw/SzsuPak= github.com/cyverse-de/go-mod/protobufjson v0.0.3 h1:XTIZejY7EUbpF6ZdhRhiFX9PGYDkGGmPP760cDswGWM= github.com/cyverse-de/go-mod/protobufjson v0.0.3/go.mod h1:p/ASemjpl2GEFYb3Tt7N8RozViwonsK0fOm0LxYeCt8= +github.com/cyverse-de/go-mod/viceauth v0.0.0-20260514164934-ce1628b2909f h1:vB6QdLFXYouqfvYO1U64NDBgrzbonlt5+wH4HLrNYWQ= +github.com/cyverse-de/go-mod/viceauth v0.0.0-20260514164934-ce1628b2909f/go.mod h1:58UZ7WOFEAA3gy4Allc9NzVviiK10c4/2fwZDuQVXVo= github.com/cyverse-de/messaging/v12 v12.0.0 h1:fS+QMiFid0tSDFR5jDqSJLPqU9UlptPRs4kbjw5X4hM= github.com/cyverse-de/messaging/v12 v12.0.0/go.mod h1:psHZ100y28PAE06Aa/O8rLTf8ZLRba7zDoC9Q7efsiw= github.com/cyverse-de/model/v10 v10.0.0 h1:ukO7FMD4rtDt92THkOqeagEXlGQkI453BjqGIs0ja/U= From e1ffa0f52db75ccc3778ec20d7ec563c18098114 Mon Sep 17 00:00:00 2001 From: John Wregglesworth Date: Thu, 14 May 2026 10:30:19 -0700 Subject: [PATCH 2/4] Address code review feedback on the vice-users relay MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Build operatorCallbackURL with url.URL.JoinPath() instead of string concatenation, per the project URL-construction guideline; fail fast on an unparseable --public-url. Drops the now-unused strings import. - Fix the startup warning: the relay is not "disabled" when only --public-url is empty (it is gated on the state HMAC secret alone) — the accurate consequence is that vice-proxy pods fail to start without both values. - Harden the relay redirect target: reject origin URLs carrying userinfo (a redirect-spoofing vector) and strip any fragment before relaying. Adds a userinfo test case. Addresses code review feedback on PR #144. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/vice-operator/main.go | 9 ++++++--- cmd/vice-operator/viceusersauth.go | 9 +++++++-- cmd/vice-operator/viceusersauth_test.go | 5 +++++ 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/cmd/vice-operator/main.go b/cmd/vice-operator/main.go index 8efa7568..a191e8e5 100644 --- a/cmd/vice-operator/main.go +++ b/cmd/vice-operator/main.go @@ -9,7 +9,6 @@ import ( "fmt" "net/url" "os" - "strings" "github.com/cyverse-de/app-exposer/common" "github.com/cyverse-de/app-exposer/constants" @@ -194,7 +193,11 @@ func main() { // unset so vice-proxy's own startup validation can flag the gap. var operatorCallbackURL string if publicURL != "" { - operatorCallbackURL = strings.TrimSuffix(publicURL, "/") + viceUsersCallbackPath + parsedPublicURL, parseErr := url.Parse(publicURL) + if parseErr != nil { + log.Fatalf("--public-url must be a valid URL, got %q: %v", publicURL, parseErr) + } + operatorCallbackURL = parsedPublicURL.JoinPath(viceUsersCallbackPath).String() } // Build the cluster config map from flags. All keys are always written so @@ -227,7 +230,7 @@ func main() { log.Warn("auth is enabled (--disable-vice-proxy-auth not set) but one or more Keycloak flags are empty; vice-proxy pods will fail to start") } if publicURL == "" || stateHMACSecret == "" { - log.Warn("auth is enabled but --public-url or --state-hmac-secret is empty; vice-proxy pods will fail to start and the OAuth callback relay is disabled") + log.Warn("auth is enabled but --public-url or --state-hmac-secret is empty; vice-proxy pods will fail to start without both") } } diff --git a/cmd/vice-operator/viceusersauth.go b/cmd/vice-operator/viceusersauth.go index 78ca0fc7..821ec3d3 100644 --- a/cmd/vice-operator/viceusersauth.go +++ b/cmd/vice-operator/viceusersauth.go @@ -69,17 +69,22 @@ func handleViceUsersCallback(cfg *ViceUsersAuthConfig) echo.HandlerFunc { return echo.NewHTTPError(http.StatusBadRequest, "invalid state") } + // Reject anything that isn't a plain https URL on an allowed VICE + // subdomain. origin.User is refused outright — userinfo has no place in + // a relay target and is a classic redirect-spoofing vector. origin, err := url.Parse(claims.Origin) - if err != nil || origin.Scheme != "https" || !cfg.isAllowedHost(origin.Hostname()) { + if err != nil || origin.Scheme != "https" || origin.User != nil || !cfg.isAllowedHost(origin.Hostname()) { return echo.NewHTTPError(http.StatusBadRequest, "invalid redirect target") } // Re-attach code + state so vice-proxy can validate state against its - // cookie and exchange the code. Preserve any path/query already present. + // cookie and exchange the code. Preserve any path/query already present; + // drop any fragment, which has no business in a server-side redirect. oq := origin.Query() oq.Set("code", code) oq.Set("state", state) origin.RawQuery = oq.Encode() + origin.Fragment = "" return c.Redirect(http.StatusTemporaryRedirect, origin.String()) } diff --git a/cmd/vice-operator/viceusersauth_test.go b/cmd/vice-operator/viceusersauth_test.go index b2a93a59..d36867cf 100644 --- a/cmd/vice-operator/viceusersauth_test.go +++ b/cmd/vice-operator/viceusersauth_test.go @@ -106,6 +106,11 @@ func TestHandleViceUsersCallback(t *testing.T) { query: url.Values{"code": {"abc"}, "state": {state(t, codec, "http://a1234abcd.cyverse.run/")}}, wantCode: http.StatusBadRequest, }, + { + name: "userinfo in origin", + query: url.Values{"code": {"abc"}, "state": {state(t, codec, "https://attacker@a1234abcd.cyverse.run/")}}, + wantCode: http.StatusBadRequest, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From f26be6e8f3f8855c9a7d04c50c842231c81ec6ce Mon Sep 17 00:00:00 2001 From: John Wregglesworth Date: Thu, 14 May 2026 10:41:04 -0700 Subject: [PATCH 3/4] Pin viceauth to v1.0.0 go-mod#11 merged and viceauth/v1.0.0 is tagged; move off the auth-relay pseudo-version to the released tag. Co-Authored-By: Claude Opus 4.7 (1M context) --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index a6ffe0a1..31c4b76c 100644 --- a/go.mod +++ b/go.mod @@ -26,7 +26,7 @@ require ( github.com/cyverse-de/go-mod/otelutils v0.0.3 github.com/cyverse-de/go-mod/pbinit v0.1.11 github.com/cyverse-de/go-mod/protobufjson v0.0.3 - github.com/cyverse-de/go-mod/viceauth v0.0.0-20260514164934-ce1628b2909f + github.com/cyverse-de/go-mod/viceauth v1.0.0 github.com/cyverse-de/messaging/v12 v12.0.0 github.com/cyverse-de/model/v10 v10.0.0 github.com/cyverse-de/p/go/qms v0.1.13 diff --git a/go.sum b/go.sum index 38e36134..a110b2e2 100644 --- a/go.sum +++ b/go.sum @@ -129,8 +129,8 @@ github.com/cyverse-de/go-mod/pbinit v0.1.11 h1:HUGZ5Q0yOwkp8vnGjJzmKX7FweX96f68I github.com/cyverse-de/go-mod/pbinit v0.1.11/go.mod h1:eC6iC5kAxbWvJgZcqK04qeQbioX6KYwBoBw/SzsuPak= github.com/cyverse-de/go-mod/protobufjson v0.0.3 h1:XTIZejY7EUbpF6ZdhRhiFX9PGYDkGGmPP760cDswGWM= github.com/cyverse-de/go-mod/protobufjson v0.0.3/go.mod h1:p/ASemjpl2GEFYb3Tt7N8RozViwonsK0fOm0LxYeCt8= -github.com/cyverse-de/go-mod/viceauth v0.0.0-20260514164934-ce1628b2909f h1:vB6QdLFXYouqfvYO1U64NDBgrzbonlt5+wH4HLrNYWQ= -github.com/cyverse-de/go-mod/viceauth v0.0.0-20260514164934-ce1628b2909f/go.mod h1:58UZ7WOFEAA3gy4Allc9NzVviiK10c4/2fwZDuQVXVo= +github.com/cyverse-de/go-mod/viceauth v1.0.0 h1:k5lCiBi7Lfa4eHZfkD1Msnez5kcAgoyUELPVSSr61H4= +github.com/cyverse-de/go-mod/viceauth v1.0.0/go.mod h1:58UZ7WOFEAA3gy4Allc9NzVviiK10c4/2fwZDuQVXVo= github.com/cyverse-de/messaging/v12 v12.0.0 h1:fS+QMiFid0tSDFR5jDqSJLPqU9UlptPRs4kbjw5X4hM= github.com/cyverse-de/messaging/v12 v12.0.0/go.mod h1:psHZ100y28PAE06Aa/O8rLTf8ZLRba7zDoC9Q7efsiw= github.com/cyverse-de/model/v10 v10.0.0 h1:ukO7FMD4rtDt92THkOqeagEXlGQkI453BjqGIs0ja/U= From d448257688b3f53cd879451a32ed1cb6dc6305c9 Mon Sep 17 00:00:00 2001 From: John Wregglesworth Date: Thu, 14 May 2026 11:16:55 -0700 Subject: [PATCH 4/4] Address PR #144 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - NewApp now takes an AppConfig struct instead of a 7-parameter positional list. - Rename the OAuth callback path from /vice-users/callback to /auth/callback. - Reword the relay comments so they no longer state the Keycloak client name as fixed — the client is configurable and could be renamed or recreated. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/vice-operator/app.go | 36 +++++++++++++++++++++++------- cmd/vice-operator/main.go | 10 ++++++++- cmd/vice-operator/viceusersauth.go | 15 +++++++------ 3 files changed, 45 insertions(+), 16 deletions(-) diff --git a/cmd/vice-operator/app.go b/cmd/vice-operator/app.go index 6259ce5f..104b4dc7 100644 --- a/cmd/vice-operator/app.go +++ b/cmd/vice-operator/app.go @@ -23,15 +23,35 @@ type App struct { // @description The vice-operator API for managing VICE analyses on remote clusters. // @BasePath / +// AppConfig holds the dependencies for NewApp, grouped into a struct rather +// than a long positional parameter list. +type AppConfig struct { + Operator *operator.Operator + Verifier *oidc.IDTokenVerifier + ExpectedClientID string + SwaggerCfg *SwaggerAuthConfig + AdminRole string + AdminEntitlements []string + ViceUsersCfg *ViceUsersAuthConfig +} + // NewApp creates a new App with all operator routes registered. -// When verifier is non-nil, all API routes require a valid Keycloak JWT Bearer -// token (or a valid session cookie set by the Swagger login flow). The token -// must additionally carry adminRole in realm_access.roles or at least one -// value in adminEntitlements in its entitlement claim — the API is admin-only. -// swaggerCfg controls the Swagger UI login gate; when disabled, docs are served -// without authentication. When viceUsersCfg is non-nil, the unauthenticated -// vice-users OAuth callback relay is registered. -func NewApp(op *operator.Operator, verifier *oidc.IDTokenVerifier, expectedClientID string, swaggerCfg *SwaggerAuthConfig, adminRole string, adminEntitlements []string, viceUsersCfg *ViceUsersAuthConfig) *App { +// When cfg.Verifier is non-nil, all API routes require a valid Keycloak JWT +// Bearer token (or a valid session cookie set by the Swagger login flow). The +// token must additionally carry cfg.AdminRole in realm_access.roles or at least +// one value in cfg.AdminEntitlements in its entitlement claim — the API is +// admin-only. cfg.SwaggerCfg controls the Swagger UI login gate; when disabled, +// docs are served without authentication. When cfg.ViceUsersCfg is non-nil, the +// unauthenticated OAuth callback relay is registered. +func NewApp(cfg AppConfig) *App { + op := cfg.Operator + verifier := cfg.Verifier + expectedClientID := cfg.ExpectedClientID + swaggerCfg := cfg.SwaggerCfg + adminRole := cfg.AdminRole + adminEntitlements := cfg.AdminEntitlements + viceUsersCfg := cfg.ViceUsersCfg + e := echo.New() e.Use(middleware.Logger()) e.Use(middleware.Recover()) diff --git a/cmd/vice-operator/main.go b/cmd/vice-operator/main.go index a191e8e5..bdcad400 100644 --- a/cmd/vice-operator/main.go +++ b/cmd/vice-operator/main.go @@ -390,7 +390,15 @@ func main() { } } - app := NewApp(op, verifier, apiAuthClientID, swaggerCfg, adminRole, adminEntitlements, viceUsersCfg) + app := NewApp(AppConfig{ + Operator: op, + Verifier: verifier, + ExpectedClientID: apiAuthClientID, + SwaggerCfg: swaggerCfg, + AdminRole: adminRole, + AdminEntitlements: adminEntitlements, + ViceUsersCfg: viceUsersCfg, + }) loadingApp := NewLoadingApp(op) apiAddr := fmt.Sprintf(":%d", port) diff --git a/cmd/vice-operator/viceusersauth.go b/cmd/vice-operator/viceusersauth.go index 821ec3d3..9bb12f7c 100644 --- a/cmd/vice-operator/viceusersauth.go +++ b/cmd/vice-operator/viceusersauth.go @@ -11,9 +11,9 @@ import ( ) // viceUsersCallbackPath is the fixed path of the OAuth callback relay. It is -// combined with --public-url to form OPERATOR_CALLBACK_URL, the single static -// redirect_uri registered in Keycloak for the vice-users client. -const viceUsersCallbackPath = "/vice-users/callback" +// combined with --public-url to form OPERATOR_CALLBACK_URL, the static +// redirect_uri that must be registered in Keycloak. +const viceUsersCallbackPath = "/auth/callback" // ViceUsersAuthConfig configures the vice-users OAuth callback relay. type ViceUsersAuthConfig struct { @@ -38,11 +38,12 @@ func (cfg *ViceUsersAuthConfig) isAllowedHost(host string) bool { return label != "" && !strings.Contains(label, ".") } -// handleViceUsersCallback returns the handler for GET /vice-users/callback. +// handleViceUsersCallback returns the OAuth callback relay handler. // -// vice-proxy registers this operator's URL as the single static redirect_uri -// for the Keycloak "vice-users" client, because Keycloak cannot wildcard-match -// per-app VICE subdomains. Keycloak delivers the authorization code here; the +// The operator's callback URL is registered in Keycloak as the static +// redirect_uri for the OAuth client vice-proxy authenticates against — +// Keycloak cannot wildcard-match per-app VICE subdomains, so a single fixed +// callback is needed. Keycloak delivers the authorization code here; the // handler recovers the original app URL from the signed state and relays the // browser back to it with the code intact. It is intentionally stateless and // does no token exchange — vice-proxy holds the client secret and redeems the