diff --git a/admin/server/magic_tokens.go b/admin/server/magic_tokens.go index e87df6604048..5e3af9e2fbb9 100644 --- a/admin/server/magic_tokens.go +++ b/admin/server/magic_tokens.go @@ -47,10 +47,16 @@ func (s *Server) IssueMagicAuthToken(ctx context.Context, req *adminv1.IssueMagi return nil, status.Error(codes.PermissionDenied, "not allowed to create a magic auth token") } + // Reject invalid user input before constructing the persistent token options. In + // particular, a negative TTL must not create an already-expired database row. + if req.TtlMinutes < 0 { + return nil, status.Error(codes.InvalidArgument, "ttl_minutes cannot be negative") + } + if req.ResourceName != "" && req.ResourceType != "" { // nolint:staticcheck // for backwards compatibility addResource := true for _, r := range req.Resources { - if r.Type == req.ResourceType && r.Name == req.ResourceName { // nolint:staticcheck // for backwards compatibility + if r != nil && r.Type == req.ResourceType && r.Name == req.ResourceName { // nolint:staticcheck // for backwards compatibility addResource = false break } @@ -65,6 +71,9 @@ func (s *Server) IssueMagicAuthToken(ctx context.Context, req *adminv1.IssueMagi resources := make([]database.ResourceName, len(req.Resources)) for i, r := range req.Resources { + if r == nil || r.Type == "" || r.Name == "" { + return nil, status.Errorf(codes.InvalidArgument, "resource at index %d must have a type and name", i) + } resources[i] = database.ResourceName{ Type: r.Type, Name: r.Name, @@ -111,7 +120,9 @@ func (s *Server) IssueMagicAuthToken(ctx context.Context, req *adminv1.IssueMagi filterSize += len(val) if filterSize > magicAuthTokenFilterMaxSize { - return nil, status.Errorf(codes.InvalidArgument, "filter size exceeds limit (got %d bytes, but the limit is %d bytes)", len(val), magicAuthTokenFilterMaxSize) + // The limit is cumulative across all filters; rejecting here guarantees an + // oversized request never reaches the database write below. + return nil, status.Errorf(codes.InvalidArgument, "filter size exceeds limit (got %d bytes, but the limit is %d bytes)", filterSize, magicAuthTokenFilterMaxSize) } filterJSON := string(val) @@ -253,6 +264,9 @@ func (s *Server) RevokeMagicAuthToken(ctx context.Context, req *adminv1.RevokeMa if err != nil { return nil, err } + // Validation caches successful token lookups briefly. Purge them so revocation + // takes effect on the very next request instead of after the cache TTL. + s.admin.PurgeAuthTokenCache() return &adminv1.RevokeMagicAuthTokenResponse{}, nil } diff --git a/admin/server/magic_tokens_test.go b/admin/server/magic_tokens_test.go new file mode 100644 index 000000000000..67b332383597 --- /dev/null +++ b/admin/server/magic_tokens_test.go @@ -0,0 +1,321 @@ +package server_test + +import ( + "database/sql" + "errors" + "strings" + "testing" + "time" + + "github.com/rilldata/rill/admin" + "github.com/rilldata/rill/admin/database" + "github.com/rilldata/rill/admin/pkg/authtoken" + "github.com/rilldata/rill/admin/testadmin" + adminv1 "github.com/rilldata/rill/proto/gen/rill/admin/v1" + runtimev1 "github.com/rilldata/rill/proto/gen/rill/runtime/v1" + "github.com/rilldata/rill/runtime" + runtimeclient "github.com/rilldata/rill/runtime/client" + "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + "google.golang.org/protobuf/encoding/protojson" +) + +func TestMagicAuthTokenAuthorizationBoundary(t *testing.T) { + // This fixture keeps the complete boundary under test: Postgres stores the + // magic token, admin signs a real JWT, and the embedded runtime enforces it. + fix := testadmin.NewWithOptionalRuntime(t, true) + ctx := t.Context() + + _, _ = fix.NewUser(t) // The first fixture user is a superuser; keep the project owner ordinary. + manager, managerClient := fix.NewUser(t) + creator, creatorClient := fix.NewUser(t) + + orgResp, err := managerClient.CreateOrganization(ctx, &adminv1.CreateOrganizationRequest{Name: "magic-boundary"}) + require.NoError(t, err) + orgName := orgResp.Organization.Name + + projectResp, err := managerClient.CreateProject(ctx, &adminv1.CreateProjectRequest{ + Org: orgName, + Project: "project-a", + ProdSlots: 1, + SkipDeploy: true, + }) + require.NoError(t, err) + projectName := projectResp.Project.Name + projectID := projectResp.Project.Id + + instanceID := fix.NewRuntimeInstance(t) + deploymentResp, err := managerClient.CreateDeployment(ctx, &adminv1.CreateDeploymentRequest{ + Org: orgName, + Project: projectName, + Environment: "prod", + }) + require.NoError(t, err) + _, err = fix.Admin.DB.UpdateDeploymentUnsafe(ctx, deploymentResp.Deployment.Id, &database.UpdateDeploymentUnsafeOptions{ + RuntimeHost: fix.RuntimeURL(), + RuntimeInstanceID: instanceID, + RuntimeAudience: fix.RuntimeURL(), + Status: database.DeploymentStatusRunning, + StatusMessage: "Running", + }) + require.NoError(t, err) + + ctrl, err := fix.Runtime.Controller(ctx, instanceID) + require.NoError(t, err) + createMagicBoundaryResources(t, ctrl) + + db := openFixturePostgres(t, fix.DatabaseURL) + // The editor can create tokens but cannot manage other creators' tokens. The + // project owner remains the manager through their independent admin role. + res, err := db.ExecContext(ctx, "UPDATE project_roles SET create_magic_auth_tokens=true, manage_magic_auth_tokens=false WHERE name=$1", database.ProjectRoleNameEditor) + require.NoError(t, err) + rows, err := res.RowsAffected() + require.NoError(t, err) + require.Equal(t, int64(1), rows) + _, err = managerClient.AddProjectMemberUser(ctx, &adminv1.AddProjectMemberUserRequest{ + Org: orgName, + Project: projectName, + Email: creator.Email, + Role: database.ProjectRoleNameEditor, + }) + require.NoError(t, err) + + restrictedResp, err := managerClient.IssueMagicAuthToken(ctx, &adminv1.IssueMagicAuthTokenRequest{ + Org: orgName, + Project: projectName, + DisplayName: "restricted-admin", + Resources: []*adminv1.ResourceName{{ + Type: runtime.ResourceKindAPI, + Name: "resource-a", + }}, + }) + require.NoError(t, err) + restrictedID := magicTokenID(t, restrictedResp.Token) + restrictedClient := fix.NewClient(t, restrictedResp.Token) + + t.Run("project and runtime resource boundary", func(t *testing.T) { + // A token belongs to exactly one project, and its captured project-admin + // attribute may shape data policy but cannot enlarge resource A's allowlist. + managerProject, err := managerClient.GetProject(ctx, &adminv1.GetProjectRequest{Org: orgName, Project: projectName}) + require.NoError(t, err) + managerClaims, err := fix.Audience.ParseAndValidate(managerProject.Jwt) + require.NoError(t, err) + require.False(t, managerClaims.Claims(instanceID).EnforceResourceAllowlist, "ordinary user JWTs must retain existing built-in access behavior") + + model, err := fix.Admin.DB.FindMagicAuthToken(ctx, restrictedID, false) + require.NoError(t, err) + require.Equal(t, projectID, model.ProjectID) + require.Equal(t, true, model.Attributes["admin"], "the creator's attribute should remain captured in storage") + + project, err := restrictedClient.GetProject(ctx, &adminv1.GetProjectRequest{Org: orgName, Project: projectName}) + require.NoError(t, err) + parsedClaims, err := fix.Audience.ParseAndValidate(project.Jwt) + require.NoError(t, err) + runtimeClaims := parsedClaims.Claims(instanceID) + require.Equal(t, true, runtimeClaims.UserAttributes["admin"]) + require.True(t, runtimeClaims.EnforceResourceAllowlist) + + rtClient, err := runtimeclient.New(fix.RuntimeURL(), project.Jwt) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, rtClient.Close()) }) + + _, err = rtClient.GetResource(ctx, &runtimev1.GetResourceRequest{ + InstanceId: instanceID, + Name: &runtimev1.ResourceName{Kind: runtime.ResourceKindAPI, Name: "resource-a"}, + }) + require.NoError(t, err, "the explicitly allowed resource should be readable") + + // Resource B, reports, and alerts all exist, so PermissionDenied proves a + // policy boundary rather than a missing-resource accident. + for _, name := range []*runtimev1.ResourceName{ + {Kind: runtime.ResourceKindAPI, Name: "resource-b"}, + {Kind: runtime.ResourceKindReport, Name: "unlisted-report"}, + {Kind: runtime.ResourceKindAlert, Name: "unlisted-alert"}, + } { + _, err = rtClient.GetResource(ctx, &runtimev1.GetResourceRequest{InstanceId: instanceID, Name: name}) + require.Equal(t, codes.PermissionDenied, status.Code(err), "resource %s/%s must remain outside the allowlist", name.Kind, name.Name) + } + + _, err = managerClient.CreateProject(ctx, &adminv1.CreateProjectRequest{ + Org: orgName, + Project: "project-b", + ProdSlots: 1, + SkipDeploy: true, + }) + require.NoError(t, err) + _, err = restrictedClient.GetProject(ctx, &adminv1.GetProjectRequest{Org: orgName, Project: "project-b"}) + require.Equal(t, codes.PermissionDenied, status.Code(err), "the token must not cross its stored project boundary") + }) + + t.Run("creator and manager ownership", func(t *testing.T) { + // Creators list and revoke only their own tokens; project token managers can + // see and revoke every creator's token without becoming the token owner. + creatorSelf, err := creatorClient.IssueMagicAuthToken(ctx, &adminv1.IssueMagicAuthTokenRequest{ + Org: orgName, Project: projectName, DisplayName: "creator-self", + Resources: []*adminv1.ResourceName{{Type: runtime.ResourceKindAPI, Name: "resource-a"}}, + }) + require.NoError(t, err) + creatorManaged, err := creatorClient.IssueMagicAuthToken(ctx, &adminv1.IssueMagicAuthTokenRequest{ + Org: orgName, Project: projectName, DisplayName: "creator-managed", + Resources: []*adminv1.ResourceName{{Type: runtime.ResourceKindAPI, Name: "resource-a"}}, + }) + require.NoError(t, err) + + creatorList, err := creatorClient.ListMagicAuthTokens(ctx, &adminv1.ListMagicAuthTokensRequest{Org: orgName, Project: projectName, PageSize: 100}) + require.NoError(t, err) + require.ElementsMatch(t, []string{"creator-self", "creator-managed"}, magicTokenDisplayNames(creatorList.Tokens)) + + managerList, err := managerClient.ListMagicAuthTokens(ctx, &adminv1.ListMagicAuthTokensRequest{Org: orgName, Project: projectName, PageSize: 100}) + require.NoError(t, err) + require.Contains(t, magicTokenDisplayNames(managerList.Tokens), "restricted-admin") + require.Contains(t, magicTokenDisplayNames(managerList.Tokens), "creator-self") + require.Contains(t, magicTokenDisplayNames(managerList.Tokens), "creator-managed") + + _, err = creatorClient.RevokeMagicAuthToken(ctx, &adminv1.RevokeMagicAuthTokenRequest{TokenId: restrictedID}) + require.Equal(t, codes.PermissionDenied, status.Code(err), "a creator must not revoke another owner's token") + _, err = creatorClient.RevokeMagicAuthToken(ctx, &adminv1.RevokeMagicAuthTokenRequest{TokenId: magicTokenID(t, creatorSelf.Token)}) + require.NoError(t, err, "a creator should revoke their own token") + _, err = managerClient.RevokeMagicAuthToken(ctx, &adminv1.RevokeMagicAuthTokenRequest{TokenId: magicTokenID(t, creatorManaged.Token)}) + require.NoError(t, err, "a manager should revoke another creator's token") + }) + + t.Run("invalid writes and filter size boundary", func(t *testing.T) { + // Invalid TTLs, resource names, and cumulative filters are rejected before + // persistence; an exactly 1024-byte filter set remains a valid boundary case. + before := countMagicTokenRows(t, db) + _, err := managerClient.IssueMagicAuthToken(ctx, &adminv1.IssueMagicAuthTokenRequest{ + Org: orgName, Project: projectName, TtlMinutes: -1, DisplayName: "negative-ttl", + }) + require.Equal(t, codes.InvalidArgument, status.Code(err)) + require.Equal(t, before, countMagicTokenRows(t, db), "negative TTL must not leave a row") + + _, err = managerClient.IssueMagicAuthToken(ctx, &adminv1.IssueMagicAuthTokenRequest{ + Org: orgName, Project: projectName, DisplayName: "empty-resource", + Resources: []*adminv1.ResourceName{{Type: runtime.ResourceKindAPI}}, + }) + require.Equal(t, codes.InvalidArgument, status.Code(err)) + require.Equal(t, before, countMagicTokenRows(t, db), "invalid resource must not leave a row") + + _, err = managerClient.IssueMagicAuthToken(ctx, &adminv1.IssueMagicAuthTokenRequest{ + Org: orgName, Project: projectName, DisplayName: "filter-at-limit", + MetricsViewFilters: map[string]*runtimev1.Expression{ + "one": expressionWithJSONSize(t, 512), + "two": expressionWithJSONSize(t, 512), + }, + }) + require.NoError(t, err) + require.Equal(t, before+1, countMagicTokenRows(t, db)) + + _, err = managerClient.IssueMagicAuthToken(ctx, &adminv1.IssueMagicAuthTokenRequest{ + Org: orgName, Project: projectName, DisplayName: "filter-over-limit", + MetricsViewFilters: map[string]*runtimev1.Expression{ + "one": expressionWithJSONSize(t, 512), + "two": expressionWithJSONSize(t, 513), + }, + }) + require.Equal(t, codes.InvalidArgument, status.Code(err)) + require.Contains(t, err.Error(), "got 1025 bytes") + require.Equal(t, before+1, countMagicTokenRows(t, db), "oversized cumulative filters must not leave a row") + }) + + t.Run("expiry and immediate revocation", func(t *testing.T) { + // The service-level fixture creates an already-expired row without sleeping; + // the public handler separately proved that users cannot request negative TTLs. + expiredTTL := -time.Nanosecond + expired, err := fix.Admin.IssueMagicAuthToken(ctx, &admin.IssueMagicAuthTokenOptions{ + ProjectID: projectID, + TTL: &expiredTTL, + CreatedByUserID: &manager.ID, + Attributes: map[string]any{"admin": true, "email": manager.Email}, + Resources: []database.ResourceName{{Type: runtime.ResourceKindAPI, Name: "resource-a"}}, + DisplayName: "expired", + }) + require.NoError(t, err) + expiredClient := fix.NewClient(t, expired.Token().String()) + _, err = expiredClient.GetProject(ctx, &adminv1.GetProjectRequest{Org: orgName, Project: projectName}) + require.Equal(t, codes.Unauthenticated, status.Code(err), "expired tokens must fail before authorization") + + // The restricted token was authenticated above and therefore cached. A + // successful revoke must invalidate that cached credential immediately. + _, err = managerClient.RevokeMagicAuthToken(ctx, &adminv1.RevokeMagicAuthTokenRequest{TokenId: restrictedID}) + require.NoError(t, err) + _, err = restrictedClient.GetProject(ctx, &adminv1.GetProjectRequest{Org: orgName, Project: projectName}) + require.Equal(t, codes.Unauthenticated, status.Code(err), "revoked tokens must fail on the next request") + _, err = fix.Admin.DB.FindMagicAuthToken(ctx, restrictedID, false) + require.True(t, errors.Is(err, database.ErrNotFound)) + }) +} + +func createMagicBoundaryResources(t *testing.T, ctrl *runtime.Controller) { + t.Helper() + resources := []struct { + name *runtimev1.ResourceName + res *runtimev1.Resource + }{ + { + name: &runtimev1.ResourceName{Kind: runtime.ResourceKindAPI, Name: "resource-a"}, + res: &runtimev1.Resource{Resource: &runtimev1.Resource_Api{Api: &runtimev1.API{Spec: &runtimev1.APISpec{}}}}, + }, + { + name: &runtimev1.ResourceName{Kind: runtime.ResourceKindAPI, Name: "resource-b"}, + res: &runtimev1.Resource{Resource: &runtimev1.Resource_Api{Api: &runtimev1.API{Spec: &runtimev1.APISpec{}}}}, + }, + { + name: &runtimev1.ResourceName{Kind: runtime.ResourceKindReport, Name: "unlisted-report"}, + res: &runtimev1.Resource{Resource: &runtimev1.Resource_Report{Report: &runtimev1.Report{Spec: &runtimev1.ReportSpec{}}}}, + }, + { + name: &runtimev1.ResourceName{Kind: runtime.ResourceKindAlert, Name: "unlisted-alert"}, + res: &runtimev1.Resource{Resource: &runtimev1.Resource_Alert{Alert: &runtimev1.Alert{Spec: &runtimev1.AlertSpec{}}}}, + }, + } + for _, r := range resources { + require.NoError(t, ctrl.Create(t.Context(), r.name, nil, nil, nil, nil, false, r.res)) + } +} + +func openFixturePostgres(t *testing.T, dsn string) *sql.DB { + t.Helper() + db, err := sql.Open("pgx", dsn) + require.NoError(t, err) + require.NoError(t, db.PingContext(t.Context())) + t.Cleanup(func() { require.NoError(t, db.Close()) }) + return db +} + +func countMagicTokenRows(t *testing.T, db *sql.DB) int { + t.Helper() + var count int + require.NoError(t, db.QueryRowContext(t.Context(), "SELECT COUNT(*) FROM magic_auth_tokens").Scan(&count)) + return count +} + +func magicTokenID(t *testing.T, value string) string { + t.Helper() + token, err := authtoken.FromString(value) + require.NoError(t, err) + return token.ID.String() +} + +func magicTokenDisplayNames(tokens []*adminv1.MagicAuthToken) []string { + names := make([]string, len(tokens)) + for i, token := range tokens { + names[i] = token.DisplayName + } + return names +} + +func expressionWithJSONSize(t *testing.T, size int) *runtimev1.Expression { + t.Helper() + base := &runtimev1.Expression{Expression: &runtimev1.Expression_Ident{Ident: ""}} + data, err := protojson.Marshal(base) + require.NoError(t, err) + require.GreaterOrEqual(t, size, len(data)) + + expr := &runtimev1.Expression{Expression: &runtimev1.Expression_Ident{Ident: strings.Repeat("x", size-len(data))}} + data, err = protojson.Marshal(expr) + require.NoError(t, err) + require.Len(t, data, size) + return expr +} diff --git a/admin/server/runtime_jwt.go b/admin/server/runtime_jwt.go index 0c827262adab..64510410d511 100644 --- a/admin/server/runtime_jwt.go +++ b/admin/server/runtime_jwt.go @@ -85,6 +85,7 @@ func (s *Server) issueRuntimeToken(ctx context.Context, opts *issueRuntimeTokenO var subject string var attr map[string]any var rules []*runtimev1.SecurityRule + var enforceResourceAllowlist bool switch { case opts.forOwner: if opts.externalUserID != "" { @@ -116,7 +117,11 @@ func (s *Server) issueRuntimeToken(ctx context.Context, opts *issueRuntimeTokenO if !ok { return "", status.Errorf(codes.Internal, "unexpected type %T for magic auth token model", claims.AuthTokenModel()) } + // Preserve the attributes captured when the token was created for data + // policies, but make its project resource list authoritative. Captured + // admin/owner/recipient attributes must never add unlisted resources. attr = mdl.Attributes + enforceResourceAllowlist = true magicRules, err := securityRulesFromMagicAuthToken(mdl) if err != nil { return "", err @@ -244,6 +249,9 @@ func (s *Server) issueRuntimeToken(ctx context.Context, opts *issueRuntimeTokenO }, Attributes: attr, SecurityRules: rules, + // This marker is deliberately set only for magic-token owners. Other JWT + // flows retain their existing built-in report and alert behavior. + EnforceResourceAllowlist: enforceResourceAllowlist, }) if err != nil { return "", status.Errorf(codes.Internal, "could not issue jwt: %s", err.Error()) diff --git a/admin/testadmin/testadmin.go b/admin/testadmin/testadmin.go index a0d0f6429e81..dcb50012c938 100644 --- a/admin/testadmin/testadmin.go +++ b/admin/testadmin/testadmin.go @@ -26,6 +26,7 @@ import ( "github.com/rilldata/rill/admin/pkg/pgtestcontainer" "github.com/rilldata/rill/admin/server" "github.com/rilldata/rill/cli/pkg/version" + runtimev1 "github.com/rilldata/rill/proto/gen/rill/runtime/v1" "github.com/rilldata/rill/runtime" "github.com/rilldata/rill/runtime/drivers" "github.com/rilldata/rill/runtime/pkg/activity" @@ -40,6 +41,7 @@ import ( "github.com/stretchr/testify/require" "go.uber.org/zap" "golang.org/x/sync/errgroup" + "google.golang.org/protobuf/types/known/structpb" // Register drivers _ "github.com/rilldata/rill/admin/database/postgres" @@ -65,6 +67,9 @@ type Fixture struct { Server *server.Server ServerOpts *server.Options Audience *runtimeauth.Audience + // DatabaseURL is exposed for narrow integration-test assertions that cannot + // be expressed through the production database interface. + DatabaseURL string Runtime *runtime.Runtime RuntimeServer *runtimeserver.Server @@ -204,10 +209,11 @@ func NewWithOptionalRuntime(t *testing.T, startRt bool) *Fixture { if !startRt { return &Fixture{ - Admin: adm, - Server: srv, - ServerOpts: srvOpts, - Audience: aud, + Admin: adm, + Server: srv, + ServerOpts: srvOpts, + Audience: aud, + DatabaseURL: pg.DatabaseURL, } } @@ -219,6 +225,7 @@ func NewWithOptionalRuntime(t *testing.T, startRt bool) *Fixture { Server: srv, ServerOpts: srvOpts, Audience: aud, + DatabaseURL: pg.DatabaseURL, Runtime: rtFixture.Runtime, RuntimeServer: rtFixture.RuntimeServer, RuntimeServerOpts: rtFixture.RuntimeServerOpts, @@ -280,6 +287,54 @@ func (f *Fixture) RuntimeURL() string { return fmt.Sprintf("http://localhost:%d", f.RuntimeServerOpts.GRPCPort) } +// NewRuntimeInstance creates a minimal instance in the fixture's embedded +// runtime. It lets admin/runtime authorization tests use a real JWT handoff +// without provisioning a deployment or contacting an external service. +func (f *Fixture) NewRuntimeInstance(t *testing.T) string { + t.Helper() + require.NotNil(t, f.Runtime, "fixture was not started with an embedded runtime") + + repoDir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(repoDir, "rill.yaml"), []byte("olap_connector: duckdb\n"), 0o644)) + + instance := &drivers.Instance{ + Environment: "prod", + OLAPConnector: "duckdb", + RepoConnector: "repo", + CatalogConnector: "catalog", + AdminConnector: "noop_admin", + Connectors: []*runtimev1.Connector{ + { + Type: "file", + Name: "repo", + Config: must(structpb.NewStruct(map[string]any{"dsn": repoDir})), + }, + { + Type: "duckdb", + Name: "duckdb", + Config: must(structpb.NewStruct(map[string]any{"dsn": ":memory:", "mode": "readwrite"})), + }, + { + Type: "sqlite", + Name: "catalog", + Config: must(structpb.NewStruct(map[string]any{ + "dsn": fmt.Sprintf("file:testadmin-%x?mode=memory&cache=shared", randomBytes()), + })), + }, + }, + Variables: map[string]string{"rill.watch_repo": "false"}, + } + require.NoError(t, f.Runtime.CreateInstance(t.Context(), instance)) + require.NotEmpty(t, instance.ID) + + ctrl, err := f.Runtime.Controller(t.Context(), instance.ID) + require.NoError(t, err) + _, err = ctrl.Get(t.Context(), runtime.GlobalProjectParserName, false) + require.NoError(t, err) + require.NoError(t, ctrl.WaitUntilIdle(t.Context(), false)) + return instance.ID +} + func (f *Fixture) TriggerDeployment(t *testing.T, org, project string) *database.Deployment { proj, err := f.Admin.DB.FindProjectByName(t.Context(), org, project) require.NoError(t, err) diff --git a/runtime/security.go b/runtime/security.go index 7cda3ccd74d5..f878c81da857 100644 --- a/runtime/security.go +++ b/runtime/security.go @@ -21,6 +21,7 @@ import ( "github.com/rilldata/rill/runtime/pkg/pbutil" "go.uber.org/zap" "google.golang.org/protobuf/encoding/protojson" + "google.golang.org/protobuf/proto" ) // ErrForbidden is returned when an action is not allowed. @@ -77,6 +78,9 @@ type SecurityClaims struct { // AdditionalRules are optional security rules to apply *in addition* to the built-in rules and the rules defined on the requested resource. // These are currently leveraged by the admin service to enforce restrictions for magic auth tokens. AdditionalRules []*runtimev1.SecurityRule + // EnforceResourceAllowlist prevents built-in identity rules from extending an + // explicit resource allowlist. The admin service sets this only for magic tokens. + EnforceResourceAllowlist bool // SkipChecks enables completely skipping all security checks. Used in local development. SkipChecks bool } @@ -102,11 +106,12 @@ func (c *SecurityClaims) Can(p Permission) bool { // It serializes the AdditionalRules using protojson. func (c *SecurityClaims) MarshalJSON() ([]byte, error) { tmp := securityClaimsJSON{ - UserID: c.UserID, - UserAttributes: c.UserAttributes, - Permissions: c.Permissions, - AdditionalRules: make([]json.RawMessage, len(c.AdditionalRules)), - SkipChecks: c.SkipChecks, + UserID: c.UserID, + UserAttributes: c.UserAttributes, + Permissions: c.Permissions, + AdditionalRules: make([]json.RawMessage, len(c.AdditionalRules)), + EnforceResourceAllowlist: c.EnforceResourceAllowlist, + SkipChecks: c.SkipChecks, } for i, rule := range c.AdditionalRules { @@ -131,6 +136,7 @@ func (c *SecurityClaims) UnmarshalJSON(data []byte) error { c.UserID = tmp.UserID c.UserAttributes = tmp.UserAttributes c.Permissions = tmp.Permissions + c.EnforceResourceAllowlist = tmp.EnforceResourceAllowlist c.AdditionalRules = make([]*runtimev1.SecurityRule, len(tmp.AdditionalRules)) for i, data := range tmp.AdditionalRules { rule := &runtimev1.SecurityRule{} @@ -147,11 +153,12 @@ func (c *SecurityClaims) UnmarshalJSON(data []byte) error { // securityClaimsJSON is a JSON-serializable representation of SecurityClaims. // SecurityClaims can't be directly serialized to JSON because the SecurityRule proto is not directly JSON serializable. type securityClaimsJSON struct { - UserID string `json:"uid"` - UserAttributes map[string]any `json:"attrs"` - Permissions []Permission `json:"perms"` - AdditionalRules []json.RawMessage `json:"rules"` - SkipChecks bool `json:"skip"` + UserID string `json:"uid"` + UserAttributes map[string]any `json:"attrs"` + Permissions []Permission `json:"perms"` + AdditionalRules []json.RawMessage `json:"rules"` + EnforceResourceAllowlist bool `json:"enforce_resource_allowlist,omitempty"` + SkipChecks bool `json:"skip"` } // ResolvedSecurity represents the resolved security rules for a given claims against a specific resource. @@ -256,6 +263,9 @@ func (p *securityEngine) resolveSecurity(ctx context.Context, instanceID, enviro if err != nil { return nil, fmt.Errorf("failed to expand security rules: %w", err) } + // Built-in report and alert policies may extend exclusive access rules. Clone + // them first so resolving one request never mutates claims shared by another. + expandedRules = cloneSecurityRules(expandedRules) // Combine rules with any contained in the resource itself rules := p.resolveRules(claims, expandedRules, r) @@ -272,7 +282,7 @@ func (p *securityEngine) resolveSecurity(ctx context.Context, instanceID, enviro return ResolvedSecurityClosed, nil } - cacheKey, err := computeCacheKey(instanceID, environment, claims, r) + cacheKey, err := computeCacheKey(instanceID, environment, vars, claims, r, rules) if err != nil { return nil, fmt.Errorf("failed to compute cache key: %w", err) } @@ -348,7 +358,11 @@ func (p *securityEngine) resolveRules(claims *SecurityClaims, rules []*runtimev1 switch r.Meta.Name.Kind { // Admins and creators/recipients can access an alert. case ResourceKindAlert: - spec := r.GetAlert().Spec + alert := r.GetAlert() + if alert == nil || alert.Spec == nil { + return rules + } + spec := alert.Spec rule := p.builtInAlertSecurityRule(r.Meta.Name, spec, claims, rules) if rule != nil { // Prepend instead of append since the rule is likely to lead to a quick deny access @@ -356,7 +370,11 @@ func (p *securityEngine) resolveRules(claims *SecurityClaims, rules []*runtimev1 } // Everyone can access an API. case ResourceKindAPI: - spec := r.GetApi().Spec + api := r.GetApi() + if api == nil || api.Spec == nil { + return rules + } + spec := api.Spec if len(spec.SecurityRules) == 0 { rules = append(rules, allowAccessRule) } else { @@ -368,9 +386,19 @@ func (p *securityEngine) resolveRules(claims *SecurityClaims, rules []*runtimev1 // Determine access using the canvas' security rules. If there are none, then everyone can access it, // unless the canvas is admin-managed (e.g. a personal canvas) in which case only the owner and admins can access it. case ResourceKindCanvas: - spec := r.GetCanvas().State.ValidSpec + canvas := r.GetCanvas() + if canvas == nil { + return rules + } + var spec *runtimev1.CanvasSpec + if canvas.State != nil { + spec = canvas.State.ValidSpec + } + if spec == nil { + spec = canvas.Spec // Not ideal, but better than giving access to the full resource + } if spec == nil { - spec = r.GetCanvas().Spec // Not ideal, but better than giving access to the full resource + return rules } if isAdminManagedAnnotations(spec.Annotations) { // Personal / admin-managed canvas: only owner and admins can access. No default-allow fallthrough. @@ -385,9 +413,19 @@ func (p *securityEngine) resolveRules(claims *SecurityClaims, rules []*runtimev1 } // Determine access using the metrics view's security rules. If there are none, then everyone can access it. case ResourceKindMetricsView: - spec := r.GetMetricsView().State.ValidSpec + mv := r.GetMetricsView() + if mv == nil { + return rules + } + var spec *runtimev1.MetricsViewSpec + if mv.State != nil { + spec = mv.State.ValidSpec + } if spec == nil { - spec = r.GetMetricsView().Spec // Not ideal, but better than giving access to the full resource + spec = mv.Spec // Not ideal, but better than giving access to the full resource + } + if spec == nil { + return rules } if len(spec.SecurityRules) == 0 { rules = append(rules, allowAccessRule) @@ -396,7 +434,14 @@ func (p *securityEngine) resolveRules(claims *SecurityClaims, rules []*runtimev1 } // Determine access using the explore's security rules. If there are none, then everyone can access it. case ResourceKindExplore: - spec := r.GetExplore().State.ValidSpec + explore := r.GetExplore() + if explore == nil { + return rules + } + var spec *runtimev1.ExploreSpec + if explore.State != nil { + spec = explore.State.ValidSpec + } if spec == nil { // Tricky, since security rules on an explore are usually derived from its metrics view and added to ValidSpec during reconciliation. // So we don't want to just fallback to r.GetExplore().Spec here. @@ -411,7 +456,11 @@ func (p *securityEngine) resolveRules(claims *SecurityClaims, rules []*runtimev1 } // Admins and creators/recipients can access a report. case ResourceKindReport: - spec := r.GetReport().Spec + report := r.GetReport() + if report == nil || report.Spec == nil { + return rules + } + spec := report.Spec rule := p.builtInReportSecurityRule(r.Meta.Name, spec, claims, rules) if rule != nil { // Prepend instead of append since the rule is likely to lead to a quick deny access @@ -434,6 +483,14 @@ func (p *securityEngine) resolveRules(claims *SecurityClaims, rules []*runtimev1 // TODO: This implementation is hard-coded specifically to properties currently set by the admin server. // Should we refactor to a generic implementation where the admin server provides a conditional rule in the JWT instead? func (p *securityEngine) builtInAlertSecurityRule(alertRes *runtimev1.ResourceName, spec *runtimev1.AlertSpec, claims *SecurityClaims, rules []*runtimev1.SecurityRule) *runtimev1.SecurityRule { + if spec == nil { + return nil + } + // A magic token's resource list is a hard boundary. Its captured admin, + // owner, or recipient attributes may filter data but cannot add this alert. + if claims.EnforceResourceAllowlist { + return nil + } var explicitAllow bool // Allow if the user is an admin if claims.Admin() { @@ -510,6 +567,14 @@ func (p *securityEngine) builtInAlertSecurityRule(alertRes *runtimev1.ResourceNa // TODO: This implementation is hard-coded specifically to properties currently set by the admin server. // Should we refactor to a generic implementation where the admin server provides a conditional rule in the JWT instead? func (p *securityEngine) builtInReportSecurityRule(reportRes *runtimev1.ResourceName, spec *runtimev1.ReportSpec, claims *SecurityClaims, rules []*runtimev1.SecurityRule) *runtimev1.SecurityRule { + if spec == nil { + return nil + } + // A magic token's resource list is a hard boundary. Its captured admin, + // owner, or recipient attributes may filter data but cannot add this report. + if claims.EnforceResourceAllowlist { + return nil + } var explicitAllow bool // Allow if the user is an admin if claims.Admin() { @@ -590,6 +655,9 @@ func isAdminManagedAnnotations(annotations map[string]string) bool { // builtInCanvasSecurityRule returns a built-in security rule to apply to an admin-managed canvas. // Returns nil if the caller is not an admin and not the owner, so the caller of resolveRules treats it as a deny. func (p *securityEngine) builtInCanvasSecurityRule(canvasRes *runtimev1.ResourceName, spec *runtimev1.CanvasSpec, claims *SecurityClaims) *runtimev1.SecurityRule { + if spec == nil { + return nil + } // Allow if the user is an admin if claims.Admin() { return &runtimev1.SecurityRule{ @@ -871,33 +939,69 @@ func (p *securityEngine) expandTransitiveAccessRules(ctx context.Context, instan } // computeCacheKey computes a cache key for a resolved security policy. -func computeCacheKey(instanceID, environment string, claims *SecurityClaims, r *runtimev1.Resource) (string, error) { - hash := md5.New() - _, err := hash.Write([]byte(instanceID)) - if err != nil { - return "", err +func computeCacheKey(instanceID, environment string, vars map[string]string, claims *SecurityClaims, r *runtimev1.Resource, rules []*runtimev1.SecurityRule) (string, error) { + // Serialize a structured value instead of concatenating strings, which could + // otherwise collide at field boundaries (for example "a"+"bc" and "ab"+"c"). + type cacheKeyResource struct { + Kind string `json:"kind"` + Name string `json:"name"` + UpdatedSecs int64 `json:"updated_secs"` + UpdatedNanos int32 `json:"updated_nanos"` + } + type cacheKeyInput struct { + InstanceID string `json:"instance_id"` + Environment string `json:"environment"` + Variables map[string]string `json:"variables"` + Claims json.RawMessage `json:"claims"` + Resource cacheKeyResource `json:"resource"` + Rules [][]byte `json:"rules"` } - _, err = hash.Write([]byte(environment)) + + claimsJSON, err := json.Marshal(claims) if err != nil { return "", err } - _, err = hash.Write([]byte(r.Meta.Name.Name)) - if err != nil { - return "", err + ruleBytes := make([][]byte, len(rules)) + for i, rule := range rules { + if rule == nil { + continue + } + ruleBytes[i], err = proto.MarshalOptions{Deterministic: true}.Marshal(rule) + if err != nil { + return "", err + } } - _, err = hash.Write([]byte(r.Meta.StateUpdatedOn.AsTime().String())) - if err != nil { - return "", err + input := cacheKeyInput{ + InstanceID: instanceID, + Environment: environment, + Variables: vars, + Claims: claimsJSON, + Resource: cacheKeyResource{ + Kind: r.Meta.Name.Kind, + Name: r.Meta.Name.Name, + }, + Rules: ruleBytes, } - claimsJSON, err := json.Marshal(claims) - if err != nil { - return "", err + if r.Meta.StateUpdatedOn != nil { + input.Resource.UpdatedSecs = r.Meta.StateUpdatedOn.Seconds + input.Resource.UpdatedNanos = r.Meta.StateUpdatedOn.Nanos } - _, err = hash.Write(claimsJSON) + data, err := json.Marshal(input) if err != nil { return "", err } - return hex.EncodeToString(hash.Sum(nil)), nil + sum := md5.Sum(data) + return hex.EncodeToString(sum[:]), nil +} + +func cloneSecurityRules(rules []*runtimev1.SecurityRule) []*runtimev1.SecurityRule { + cloned := make([]*runtimev1.SecurityRule, len(rules)) + for i, rule := range rules { + if rule != nil { + cloned[i] = proto.Clone(rule).(*runtimev1.SecurityRule) + } + } + return cloned } func evaluateConditions(r *runtimev1.Resource, expression string, kinds []string, resources []*runtimev1.ResourceName, td parser.TemplateData) (*bool, error) { diff --git a/runtime/security_builtin_test.go b/runtime/security_builtin_test.go new file mode 100644 index 000000000000..45bd90738589 --- /dev/null +++ b/runtime/security_builtin_test.go @@ -0,0 +1,163 @@ +package runtime + +import ( + "testing" + "time" + + runtimev1 "github.com/rilldata/rill/proto/gen/rill/runtime/v1" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + "google.golang.org/protobuf/types/known/structpb" + "google.golang.org/protobuf/types/known/timestamppb" +) + +func TestBuiltInReportAuthorizationMatrix(t *testing.T) { + // Check each identity source that may grant access to an otherwise private scheduled report. + emailProps, err := structpb.NewStruct(map[string]any{"recipients": []any{"recipient@example.com"}}) + require.NoError(t, err) + slackProps, err := structpb.NewStruct(map[string]any{"users": []any{"slack@example.com"}}) + require.NoError(t, err) + + // This matrix documents every identity source that can grant access to an + // otherwise private scheduled report. + tests := []struct { + name string + attrs map[string]any + allowed bool + }{ + {name: "admin", attrs: map[string]any{"admin": true}, allowed: true}, + {name: "owner", attrs: map[string]any{"id": "owner-id"}, allowed: true}, + {name: "email recipient", attrs: map[string]any{"email": "recipient@example.com"}, allowed: true}, + {name: "Slack user", attrs: map[string]any{"email": "slack@example.com"}, allowed: true}, + {name: "unrelated user", attrs: map[string]any{"id": "other", "email": "other@example.com"}, allowed: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + res := newSecurityTestReport("report", map[string]string{"admin_owner_user_id": "owner-id"}, []*runtimev1.Notifier{ + {Connector: "email", Properties: emailProps}, + {Connector: "slack", Properties: slackProps}, + }) + security, err := newSecurityEngine(10, zap.NewNop(), nil).resolveSecurity(t.Context(), "instance", "prod", nil, &SecurityClaims{UserAttributes: tt.attrs}, res) + require.NoError(t, err) + require.Equal(t, tt.allowed, security.CanAccess()) + }) + } +} + +func TestBuiltInAlertAuthorizationMatrix(t *testing.T) { + // Check owner, recipient, Slack-user, channel, and admin access through the public security result. + emailProps, err := structpb.NewStruct(map[string]any{"recipients": []any{"recipient@example.com"}}) + require.NoError(t, err) + slackProps, err := structpb.NewStruct(map[string]any{ + "users": []any{"slack@example.com"}, + "channels": []any{"alerts"}, + }) + require.NoError(t, err) + + // Channel-only access is intentionally characterized here because the + // current policy grants every project user access when any channel is set. + tests := []struct { + name string + attrs map[string]any + allowed bool + }{ + {name: "admin", attrs: map[string]any{"admin": true}, allowed: true}, + {name: "owner", attrs: map[string]any{"id": "owner-id"}, allowed: true}, + {name: "email recipient", attrs: map[string]any{"email": "recipient@example.com"}, allowed: true}, + {name: "Slack user", attrs: map[string]any{"email": "slack@example.com"}, allowed: true}, + {name: "Slack channel project user", attrs: map[string]any{"email": "unrelated@example.com"}, allowed: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + res := newSecurityTestAlert("alert", map[string]string{"admin_owner_user_id": "owner-id"}, []*runtimev1.Notifier{ + {Connector: "email", Properties: emailProps}, + {Connector: "slack", Properties: slackProps}, + }) + security, err := newSecurityEngine(10, zap.NewNop(), nil).resolveSecurity(t.Context(), "instance", "prod", nil, &SecurityClaims{UserAttributes: tt.attrs}, res) + require.NoError(t, err) + require.Equal(t, tt.allowed, security.CanAccess()) + }) + } +} + +func TestBuiltInCanvasAuthorizationMatrix(t *testing.T) { + // Admin-managed canvases are private unless owned or explicitly shared; + // ordinary canvases remain accessible by default. + tests := []struct { + name string + annotations map[string]string + attrs map[string]any + allowed bool + }{ + {name: "personal owner", annotations: map[string]string{"admin_managed": "true", "admin_owner_user_id": "owner-id"}, attrs: map[string]any{"id": "owner-id"}, allowed: true}, + {name: "personal admin", annotations: map[string]string{"admin_managed": "true"}, attrs: map[string]any{"admin": true}, allowed: true}, + {name: "personal unrelated", annotations: map[string]string{"admin_managed": "true", "admin_owner_user_id": "owner-id"}, attrs: map[string]any{"id": "other"}, allowed: false}, + {name: "shared personal", annotations: map[string]string{"admin_managed": "true", "admin_shared": "true"}, attrs: map[string]any{"id": "other"}, allowed: true}, + {name: "ordinary", annotations: nil, attrs: map[string]any{"id": "other"}, allowed: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + res := newSecurityTestCanvas("canvas", tt.annotations) + security, err := newSecurityEngine(10, zap.NewNop(), nil).resolveSecurity(t.Context(), "instance", "prod", nil, &SecurityClaims{UserAttributes: tt.attrs}, res) + require.NoError(t, err) + require.Equal(t, tt.allowed, security.CanAccess()) + }) + } +} + +func TestBuiltInAuthorizationMalformedResourcesFailClosed(t *testing.T) { + // Partially reconciled resources are observable during failures. Security + // resolution must deny them instead of panicking or defaulting open. + tests := []*runtimev1.Resource{ + {Meta: newSecurityTestMeta(ResourceKindReport, "nil-report"), Resource: &runtimev1.Resource_Report{Report: &runtimev1.Report{}}}, + {Meta: newSecurityTestMeta(ResourceKindAlert, "nil-alert"), Resource: &runtimev1.Resource_Alert{Alert: &runtimev1.Alert{}}}, + {Meta: newSecurityTestMeta(ResourceKindCanvas, "nil-canvas"), Resource: &runtimev1.Resource_Canvas{Canvas: &runtimev1.Canvas{}}}, + {Meta: newSecurityTestMeta(ResourceKindMetricsView, "nil-metrics"), Resource: &runtimev1.Resource_MetricsView{MetricsView: &runtimev1.MetricsView{}}}, + {Meta: newSecurityTestMeta(ResourceKindAPI, "nil-api"), Resource: &runtimev1.Resource_Api{Api: &runtimev1.API{}}}, + } + for _, res := range tests { + t.Run(res.Meta.Name.Name, func(t *testing.T) { + security, err := newSecurityEngine(10, zap.NewNop(), nil).resolveSecurity(t.Context(), "instance", "prod", nil, &SecurityClaims{UserAttributes: map[string]any{"admin": true}}, res) + require.NoError(t, err) + require.False(t, security.CanAccess()) + }) + } +} + +func newSecurityTestReport(name string, annotations map[string]string, notifiers []*runtimev1.Notifier) *runtimev1.Resource { + return &runtimev1.Resource{ + Meta: newSecurityTestMeta(ResourceKindReport, name), + Resource: &runtimev1.Resource_Report{Report: &runtimev1.Report{Spec: &runtimev1.ReportSpec{ + Annotations: annotations, + Notifiers: notifiers, + }}}, + } +} + +func newSecurityTestAlert(name string, annotations map[string]string, notifiers []*runtimev1.Notifier) *runtimev1.Resource { + return &runtimev1.Resource{ + Meta: newSecurityTestMeta(ResourceKindAlert, name), + Resource: &runtimev1.Resource_Alert{Alert: &runtimev1.Alert{Spec: &runtimev1.AlertSpec{ + Annotations: annotations, + Notifiers: notifiers, + }}}, + } +} + +func newSecurityTestCanvas(name string, annotations map[string]string) *runtimev1.Resource { + spec := &runtimev1.CanvasSpec{Annotations: annotations} + return &runtimev1.Resource{ + Meta: newSecurityTestMeta(ResourceKindCanvas, name), + Resource: &runtimev1.Resource_Canvas{Canvas: &runtimev1.Canvas{ + Spec: spec, + State: &runtimev1.CanvasState{ValidSpec: spec}, + }}, + } +} + +func newSecurityTestMeta(kind, name string) *runtimev1.ResourceMeta { + return &runtimev1.ResourceMeta{ + Name: &runtimev1.ResourceName{Kind: kind, Name: name}, + StateUpdatedOn: timestamppb.New(time.Date(2026, time.January, 2, 3, 4, 5, 0, time.UTC)), + } +} diff --git a/runtime/security_cache_test.go b/runtime/security_cache_test.go new file mode 100644 index 000000000000..86b120e3ad97 --- /dev/null +++ b/runtime/security_cache_test.go @@ -0,0 +1,69 @@ +package runtime + +import ( + "testing" + "time" + + runtimev1 "github.com/rilldata/rill/proto/gen/rill/runtime/v1" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + "google.golang.org/protobuf/types/known/timestamppb" +) + +func TestResolveSecurityCacheIncludesTemplateVariables(t *testing.T) { + // Security templates can depend on project variables, so two requests that + // differ only by variables must not share a cached row filter. + res := newSecurityTestMetricsView("orders", []*runtimev1.SecurityRule{ + {Rule: &runtimev1.SecurityRule_Access{Access: &runtimev1.SecurityRuleAccess{Allow: true}}}, + {Rule: &runtimev1.SecurityRule_RowFilter{RowFilter: &runtimev1.SecurityRuleRowFilter{Sql: "region = '{{ .env.region }}'"}}}, + }) + engine := newSecurityEngine(10, zap.NewNop(), nil) + claims := &SecurityClaims{} + + east, err := engine.resolveSecurity(t.Context(), "instance", "prod", map[string]string{"region": "east"}, claims, res) + require.NoError(t, err) + require.Equal(t, "region = 'east'", east.RowFilter()) + + west, err := engine.resolveSecurity(t.Context(), "instance", "prod", map[string]string{"region": "west"}, claims, res) + require.NoError(t, err) + require.Equal(t, "region = 'west'", west.RowFilter()) +} + +func TestResolveSecurityCacheSeparatesResourceKinds(t *testing.T) { + // Resource names are unique only within a kind. An API and a metrics view + // with the same name and timestamp may intentionally have opposite policies. + updated := timestamppb.New(time.Date(2026, time.January, 2, 3, 4, 5, 0, time.UTC)) + api := &runtimev1.Resource{ + Meta: &runtimev1.ResourceMeta{Name: &runtimev1.ResourceName{Kind: ResourceKindAPI, Name: "shared"}, StateUpdatedOn: updated}, + Resource: &runtimev1.Resource_Api{Api: &runtimev1.API{Spec: &runtimev1.APISpec{}}}, + } + metrics := newSecurityTestMetricsView("shared", []*runtimev1.SecurityRule{ + {Rule: &runtimev1.SecurityRule_Access{Access: &runtimev1.SecurityRuleAccess{Allow: false}}}, + }) + metrics.Meta.StateUpdatedOn = updated + + engine := newSecurityEngine(10, zap.NewNop(), nil) + claims := &SecurityClaims{} + + apiSecurity, err := engine.resolveSecurity(t.Context(), "instance", "prod", nil, claims, api) + require.NoError(t, err) + require.True(t, apiSecurity.CanAccess()) + + metricsSecurity, err := engine.resolveSecurity(t.Context(), "instance", "prod", nil, claims, metrics) + require.NoError(t, err) + require.False(t, metricsSecurity.CanAccess()) +} + +func newSecurityTestMetricsView(name string, rules []*runtimev1.SecurityRule) *runtimev1.Resource { + spec := &runtimev1.MetricsViewSpec{SecurityRules: rules} + return &runtimev1.Resource{ + Meta: &runtimev1.ResourceMeta{ + Name: &runtimev1.ResourceName{Kind: ResourceKindMetricsView, Name: name}, + StateUpdatedOn: timestamppb.New(time.Date(2026, time.January, 2, 3, 4, 5, 0, time.UTC)), + }, + Resource: &runtimev1.Resource_MetricsView{MetricsView: &runtimev1.MetricsView{ + Spec: spec, + State: &runtimev1.MetricsViewState{ValidSpec: spec}, + }}, + } +} diff --git a/runtime/security_concurrency_test.go b/runtime/security_concurrency_test.go new file mode 100644 index 000000000000..44f7436c9086 --- /dev/null +++ b/runtime/security_concurrency_test.go @@ -0,0 +1,91 @@ +package runtime + +import ( + "encoding/json" + "fmt" + "sync" + "testing" + "time" + + runtimev1 "github.com/rilldata/rill/proto/gen/rill/runtime/v1" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + "google.golang.org/protobuf/types/known/structpb" + "google.golang.org/protobuf/types/known/timestamppb" +) + +func TestResolveSecurityDoesNotMutateSharedClaims(t *testing.T) { + // Magic-token claims are reused across requests. Built-in recipient access + // must extend a request-local rule rather than mutate that shared token state. + claims := &SecurityClaims{ + UserAttributes: map[string]any{"email": "recipient@example.com"}, + AdditionalRules: []*runtimev1.SecurityRule{{ + Rule: &runtimev1.SecurityRule_Access{Access: &runtimev1.SecurityRuleAccess{ + Allow: true, + Exclusive: true, + ConditionKinds: []string{ResourceKindMetricsView}, + }}, + }}, + } + before, err := json.Marshal(claims) + require.NoError(t, err) + + const resolutions = 64 + engine := newSecurityEngine(resolutions*2, zap.NewNop(), nil) + results := make([]bool, resolutions) + errs := make([]error, resolutions) + var wg sync.WaitGroup + wg.Add(resolutions) + for i := 0; i < resolutions; i++ { + i := i + go func() { + defer wg.Done() + kind := ResourceKindReport + if i%2 == 1 { + kind = ResourceKindAlert + } + res := newSecurityTestNotificationResource(kind, fmt.Sprintf("notification-%d", i), "recipient@example.com") + security, resolveErr := engine.resolveSecurity(t.Context(), "instance", "prod", nil, claims, res) + errs[i] = resolveErr + if resolveErr == nil { + results[i] = security.CanAccess() + } + }() + } + wg.Wait() + + for i := range errs { + require.NoErrorf(t, errs[i], "resolution %d", i) + require.Truef(t, results[i], "recipient should access notification %d", i) + } + after, err := json.Marshal(claims) + require.NoError(t, err) + require.JSONEq(t, string(before), string(after)) + require.Empty(t, claims.AdditionalRules[0].GetAccess().ConditionResources) +} + +func newSecurityTestNotificationResource(kind, name, recipient string) *runtimev1.Resource { + properties, err := structpb.NewStruct(map[string]any{"recipients": []any{recipient}}) + if err != nil { + panic(err) + } + meta := &runtimev1.ResourceMeta{ + Name: &runtimev1.ResourceName{Kind: kind, Name: name}, + StateUpdatedOn: timestamppb.New(time.Date(2026, time.January, 2, 3, 4, 5, 0, time.UTC)), + } + notifiers := []*runtimev1.Notifier{{Connector: "email", Properties: properties}} + if kind == ResourceKindAlert { + return &runtimev1.Resource{ + Meta: meta, + Resource: &runtimev1.Resource_Alert{Alert: &runtimev1.Alert{ + Spec: &runtimev1.AlertSpec{Notifiers: notifiers}, + }}, + } + } + return &runtimev1.Resource{ + Meta: meta, + Resource: &runtimev1.Resource_Report{Report: &runtimev1.Report{ + Spec: &runtimev1.ReportSpec{Notifiers: notifiers}, + }}, + } +} diff --git a/runtime/server/auth/claims.go b/runtime/server/auth/claims.go index 6b4da8b68849..b0dff8969c6c 100644 --- a/runtime/server/auth/claims.go +++ b/runtime/server/auth/claims.go @@ -18,10 +18,11 @@ type ClaimsProvider interface { // jwtClaims implements a ClaimsProvider that resolves claims from a JWT payload. type jwtClaims struct { jwt.RegisteredClaims - System []runtime.Permission `json:"sys,omitempty"` - Instances map[string][]runtime.Permission `json:"ins,omitempty"` - Attrs map[string]any `json:"attr,omitempty"` - Security []json.RawMessage `json:"sec,omitempty"` // []*runtimev1.SecurityRule serialized with protojson + System []runtime.Permission `json:"sys,omitempty"` + Instances map[string][]runtime.Permission `json:"ins,omitempty"` + Attrs map[string]any `json:"attr,omitempty"` + Security []json.RawMessage `json:"sec,omitempty"` // []*runtimev1.SecurityRule serialized with protojson + EnforceResourceAllowlist bool `json:"enforce_resource_allowlist,omitempty"` } var _ ClaimsProvider = (*jwtClaims)(nil) @@ -64,10 +65,11 @@ func (c *jwtClaims) Claims(instanceID string) *runtime.SecurityClaims { } return &runtime.SecurityClaims{ - UserID: c.RegisteredClaims.Subject, - UserAttributes: attrs, - Permissions: permissions, - AdditionalRules: rules, + UserID: c.RegisteredClaims.Subject, + UserAttributes: attrs, + Permissions: permissions, + AdditionalRules: rules, + EnforceResourceAllowlist: c.EnforceResourceAllowlist, } } diff --git a/runtime/server/auth/jwt.go b/runtime/server/auth/jwt.go index 89ffdd6fe39c..4a4984f69452 100644 --- a/runtime/server/auth/jwt.go +++ b/runtime/server/auth/jwt.go @@ -116,6 +116,9 @@ type TokenOptions struct { InstancePermissions map[string][]runtime.Permission Attributes map[string]any SecurityRules []*runtimev1.SecurityRule + // EnforceResourceAllowlist makes resource restrictions authoritative even + // when captured user attributes would otherwise grant built-in access. + EnforceResourceAllowlist bool } // NewToken issues a new JWT based on the provided options. @@ -144,10 +147,11 @@ func (i *Issuer) NewToken(opts TokenOptions) (string, error) { Subject: opts.Subject, Audience: []string{opts.AudienceURL}, }, - System: opts.SystemPermissions, - Instances: opts.InstancePermissions, - Attrs: opts.Attributes, - Security: sec, + System: opts.SystemPermissions, + Instances: opts.InstancePermissions, + Attrs: opts.Attributes, + Security: sec, + EnforceResourceAllowlist: opts.EnforceResourceAllowlist, } // Create token @@ -213,13 +217,14 @@ func OpenAudience(ctx context.Context, logger *zap.Logger, issuerURL, audienceUR RefreshTimeout: time.Second * 10, RefreshUnknownKID: true, }) - if err != nil { - logger.Info("JWKS fetch failed, retrying in 5s", zap.Error(err)) - select { - case <-time.After(time.Second * 5): - case <-ctx.Done(): - return nil, ctx.Err() - } + if err == nil { + break + } + logger.Info("JWKS fetch failed, retrying in 5s", zap.Error(err)) + select { + case <-time.After(time.Second * 5): + case <-ctx.Done(): + return nil, ctx.Err() } } if err != nil { diff --git a/runtime/server/auth/jwt_rotation_test.go b/runtime/server/auth/jwt_rotation_test.go new file mode 100644 index 000000000000..1ecbb02ea3cc --- /dev/null +++ b/runtime/server/auth/jwt_rotation_test.go @@ -0,0 +1,168 @@ +package auth + +import ( + "context" + "net/http" + "net/http/httptest" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/require" + "go.uber.org/zap" +) + +func TestAudienceRefreshesRotatedJWKS(t *testing.T) { + // An unknown key ID must trigger one synchronous refresh so a normal key + // rotation does not require a process restart or a timing-based retry loop. + first, err := NewEphemeralIssuer("") + require.NoError(t, err) + second, err := NewEphemeralIssuer("") + require.NoError(t, err) + + fixture := newMutableJWKSServer(t, first.publicJWKS) + first.issuerURL = fixture.server.URL + second.issuerURL = fixture.server.URL + audienceURL := "https://runtime.example.com" + audience, err := OpenAudience(t.Context(), zap.NewNop(), fixture.server.URL, audienceURL) + require.NoError(t, err) + t.Cleanup(audience.Close) + + oldToken, err := first.NewToken(TokenOptions{AudienceURL: audienceURL, Subject: "alice", TTL: time.Hour}) + require.NoError(t, err) + _, err = audience.ParseAndValidate(oldToken) + require.NoError(t, err) + + fixture.set(http.StatusOK, second.publicJWKS) + rotatedToken, err := second.NewToken(TokenOptions{AudienceURL: audienceURL, Subject: "alice", TTL: time.Hour}) + require.NoError(t, err) + _, err = audience.ParseAndValidate(rotatedToken) + require.NoError(t, err) + require.Equal(t, 2, fixture.requests()) + require.Equal(t, []string{second.signingKey.KeyID}, audience.jwks.KIDs()) + + // Once the issuer has removed the old key, tokens signed by it must fail closed. + _, err = audience.ParseAndValidate(oldToken) + require.Error(t, err) +} + +func TestAudienceRejectsMalformedJWKSRefreshAndKeepsLastGoodKeys(t *testing.T) { + // A malformed refresh is untrusted network input. It must not admit the + // unknown token or erase the last known-good key used by existing tokens. + trusted, err := NewEphemeralIssuer("") + require.NoError(t, err) + untrusted, err := NewEphemeralIssuer("") + require.NoError(t, err) + fixture := newMutableJWKSServer(t, trusted.publicJWKS) + trusted.issuerURL = fixture.server.URL + untrusted.issuerURL = fixture.server.URL + audienceURL := "https://runtime.example.com" + audience, err := OpenAudience(t.Context(), zap.NewNop(), fixture.server.URL, audienceURL) + require.NoError(t, err) + t.Cleanup(audience.Close) + + trustedToken, err := trusted.NewToken(TokenOptions{AudienceURL: audienceURL, Subject: "alice", TTL: time.Hour}) + require.NoError(t, err) + untrustedToken, err := untrusted.NewToken(TokenOptions{AudienceURL: audienceURL, Subject: "mallory", TTL: time.Hour}) + require.NoError(t, err) + + fixture.set(http.StatusOK, []byte(`{"keys":[`)) + _, err = audience.ParseAndValidate(untrustedToken) + require.Error(t, err) + _, err = audience.ParseAndValidate(trustedToken) + require.NoError(t, err) + require.Equal(t, []string{trusted.signingKey.KeyID}, audience.jwks.KIDs()) +} + +func TestAudienceValidatesIssuerAndAudience(t *testing.T) { + // Valid signatures with routing claims for another issuer or runtime must still be rejected. + issuer, audience, close := newTestIssuerAndAudience(t) + t.Cleanup(close) + + // Signature validity is insufficient: both routing claims bind a token to + // the intended control plane and runtime. + t.Run("issuer mismatch", func(t *testing.T) { + original := issuer.issuerURL + issuer.issuerURL = "https://different-issuer.example.com" + t.Cleanup(func() { issuer.issuerURL = original }) + token, err := issuer.NewToken(TokenOptions{AudienceURL: audience.audienceURL, Subject: "alice", TTL: time.Hour}) + require.NoError(t, err) + _, err = audience.ParseAndValidate(token) + require.ErrorContains(t, err, "invalid token issuer") + }) + + t.Run("audience mismatch", func(t *testing.T) { + token, err := issuer.NewToken(TokenOptions{AudienceURL: "https://different-runtime.example.com", Subject: "alice", TTL: time.Hour}) + require.NoError(t, err) + _, err = audience.ParseAndValidate(token) + require.Error(t, err) + }) +} + +func TestOpenAudienceInitialFetchFailuresHonorContext(t *testing.T) { + // Startup retry must stop on cancellation instead of imposing the full + // twenty-attempt backoff when the caller is already shutting down. + tests := []struct { + name string + status int + body string + }{ + {name: "server error", status: http.StatusInternalServerError, body: "unavailable"}, + {name: "malformed JWKS", status: http.StatusOK, body: `{"keys":[`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(tt.status) + _, _ = w.Write([]byte(tt.body)) + cancel() + })) + t.Cleanup(server.Close) + + started := time.Now() + audience, err := OpenAudience(ctx, zap.NewNop(), server.URL, "https://runtime.example.com") + require.Error(t, err) + require.Nil(t, audience) + require.Less(t, time.Since(started), time.Second) + }) + } +} + +type mutableJWKSServer struct { + server *httptest.Server + + mu sync.RWMutex + status int + body []byte + request int +} + +func newMutableJWKSServer(t *testing.T, body []byte) *mutableJWKSServer { + t.Helper() + fixture := &mutableJWKSServer{status: http.StatusOK, body: append([]byte(nil), body...)} + fixture.server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + fixture.mu.Lock() + fixture.request++ + status := fixture.status + response := append([]byte(nil), fixture.body...) + fixture.mu.Unlock() + w.WriteHeader(status) + _, _ = w.Write(response) + })) + t.Cleanup(fixture.server.Close) + return fixture +} + +func (f *mutableJWKSServer) set(status int, body []byte) { + f.mu.Lock() + defer f.mu.Unlock() + f.status = status + f.body = append([]byte(nil), body...) +} + +func (f *mutableJWKSServer) requests() int { + f.mu.RLock() + defer f.mu.RUnlock() + return f.request +} diff --git a/runtime/server/auth/jwt_test.go b/runtime/server/auth/jwt_test.go index 7ad43a3f092e..dab21f5d0851 100644 --- a/runtime/server/auth/jwt_test.go +++ b/runtime/server/auth/jwt_test.go @@ -42,15 +42,15 @@ func TestTokens(t *testing.T) { }) t.Run("Expired", func(t *testing.T) { + // A negative TTL creates an already-expired token without making the test depend on wall-clock sleeps. token, err := iss.NewToken(TokenOptions{ AudienceURL: aud.audienceURL, Subject: "alice", - TTL: time.Duration(time.Millisecond), + TTL: -time.Minute, SystemPermissions: []runtime.Permission{runtime.ReadInstance}, InstancePermissions: map[string][]runtime.Permission{"example": {runtime.ReadOLAP}}, }) - - time.Sleep(50 * time.Millisecond) + require.NoError(t, err) _, err = aud.ParseAndValidate(token) require.Error(t, err)