diff --git a/cmd/vice-operator/app.go b/cmd/vice-operator/app.go index d339f708..104b4dc7 100644 --- a/cmd/vice-operator/app.go +++ b/cmd/vice-operator/app.go @@ -23,14 +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. -func NewApp(op *operator.Operator, verifier *oidc.IDTokenVerifier, expectedClientID string, swaggerCfg *SwaggerAuthConfig, adminRole string, adminEntitlements []string) *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()) @@ -59,6 +80,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..bdcad400 100644 --- a/cmd/vice-operator/main.go +++ b/cmd/vice-operator/main.go @@ -13,6 +13,7 @@ import ( "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 +57,8 @@ func main() { swaggerClientID string swaggerClientSecret string swaggerCookieSecret string + publicURL string + stateHMACSecret string apiSubdomain string apiServiceName string serviceCIDR string @@ -104,6 +107,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 +132,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 +186,20 @@ 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 != "" { + 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 // 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 +210,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 +224,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 without both") + } } // Ensure cluster config secret so vice-proxy containers can reference it @@ -353,7 +380,25 @@ 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(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 new file mode 100644 index 00000000..9bb12f7c --- /dev/null +++ b/cmd/vice-operator/viceusersauth.go @@ -0,0 +1,92 @@ +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 static +// redirect_uri that must be registered in Keycloak. +const viceUsersCallbackPath = "/auth/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 OAuth callback relay handler. +// +// 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 +// 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") + } + + // 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" || 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; + // 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 new file mode 100644 index 00000000..d36867cf --- /dev/null +++ b/cmd/vice-operator/viceusersauth_test.go @@ -0,0 +1,136 @@ +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, + }, + { + 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) { + 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..31c4b76c 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 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 b8c216f2..a110b2e2 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 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=