Repository navigation
ateapi: Implement Policy Governance CRUD Access Policy APIs - #1965
Julian Gutierrez Oschmann (juli4n) merged 8 commits into
Conversation
…ootstrapping Add PostgreSQL storage, OpenFGA policy reconciliation, and startup bootstrapping for Global and Atespace AccessPolicy subresources. - Add AccessPolicy, Binding, and CRUD request proto definitions. - Add migration 000003_access_policies.sql creating global_access_policies and atespace_access_policies tables. - Add store and PolicyManager methods for creating, reading, updating, and deleting access policies transactionally across PostgreSQL and OpenFGA. - Add --authz-bootstrap-owners flag to ateapi main for bootstrapping initial global owners on startup.
Expose GetGlobalAccessPolicy, CreateGlobalAccessPolicy, UpdateGlobalAccessPolicy, GetAtespaceAccessPolicy, CreateAtespaceAccessPolicy, UpdateAtespaceAccessPolicy, and DeleteAtespaceAccessPolicy gRPC endpoints on the Control service. - Add gRPC handler implementations, validation logic, anti-lockout rules, and error mapping in controlapi. - Register gRPC method rules mapping policy endpoints to OpenFGA check relations in defaultRPCPermissions. - Add end-to-end access policy governance integration tests in controlapi.
a193fbd to
b85d7d9
Compare
Shruti Nair (SHRUTI6991)
changed the title
AccessPolicy subresources for Global (global:root) and Atespace scopes, enabling fine-grained ReBac.
Shruti Nair (SHRUTI6991)
changed the title
Shruti Nair (SHRUTI6991)
changed the title
…AccessPolicy RPC BootstrapGlobalOwners previously merged --authz-bootstrap-owners into an existing global access policy on every startup, which would re-grant global owner to a revoked bootstrap principal on pod restart. Change bootstrapGlobalOwnersOnce to return nil immediately when a global policy with at least one owner already exists, so --authz-bootstrap-owners only seeds the initial policy when none exists yet. Remove CreateGlobalAccessPolicy from the public Control gRPC service and store.Interface since the singleton global access policy is always seeded at initialization and mutated via UpdateGlobalAccessPolicy.
bb03231 to
26d538a
Compare
Bowei Du (bowei)
added
area/api
|
|
||
| -- +goose Up | ||
|
|
||
| CREATE TABLE global_access_policies ( |
There was a problem hiding this comment.
Why do we need to duplicate the storage of these policies, rather than just relying on the tuples stored in the OpenFGA table?
There was a problem hiding this comment.
I initially started with only the uid + version in the table for OCC (and called it versions table) and the tuples store was the only source of truth for policies in my first draft.
I synched with Julian Gutierrez Oschmann (@juli4n) yesterday and he recommended to follow the Substrate API pattern for storing the proto in the Access Policy table for consistency. The proto also stored the create and update time as well which I initially defined as column fields in versions table.
As long as we commit the two tables atomically, I don't think they can go out of sync. So there is no downsides to it except duplicated storage.
Eitan Yarmush (EItanya)
left a comment
Eitan Yarmush (EItanya)
left a comment
There was a problem hiding this comment.
So my biggest issue with this PR is not the logic itself, which seems mostly sound, but rather the fact that all DB operations are artificially separated. I realize this already went in, but I think the ReconcileGlobalBindings method should take the pg.Tx explicitly. Multiple layers down that function will fail without a transaction, so why are we obfuscating it, in my opinion it just makes it harder to read, when it's actually a hard requirement
…onicalization Mark Binding.members with +k8s:listType=set so generated validation rejects duplicate members at the API boundary, and remove storage-layer binding canonicalization so persisted policies preserve caller-supplied binding and member order without silent mutation. Also drop OpenFGA implementation references from Binding proto comments and nest ErrNotFound checks inside the error branch in GetGlobalAccessPolicy and GetAtespaceAccessPolicy.
…I without authz PolicyManager tuple writes (ReconcileGlobalBindings, ReconcileAtespaceBindings, DeleteAtespacePolicies) now take the pgx.Tx as an argument instead of reading it from ctx, so the transaction requirement is visible in the signature. A nil tx returns ErrNilTransaction. Access policy store methods now return store.ErrAuthzDisabled when no PolicyManager is configured, and the AccessPolicy RPCs map it to FailedPrecondition. Previously, with --experimental-enable-authz=false, policy writes succeeded without updating OpenFGA tuples, so re-enabling authz could leave revoked principals authorized or bootstrapped owners denied. DeleteAtespace still skips tuple cleanup without a PolicyManager so atespaces remain deletable with authz disabled. FormatMember rejects control characters, which OpenFGA refuses in tuple user IDs, so such members fail validation with InvalidArgument. AccessPolicy write errors carrying nonstandard backend gRPC codes (such as OpenFGA's Code(2000)) are reported as Internal. Rename global_access_policies to global_access_policy since it holds at most one row, and truncate it along with OpenFGA tuples between atepg tests so the lifecycle tests are repeatable.
…eeding the global policy --authz-bootstrap-owners previously seeded the global AccessPolicy once at startup. After that the flag was ignored, so removing a principal from it did not revoke access, and lockout prevention relied on validation that kept at least one global owner and stopped callers removing themselves. Bootstrap owners are now held by the Authorizer and granted owner on global:root through a contextual tuple on each Check, added only for the calling principal. They are never stored, are not part of any AccessPolicy, and lose access once the server runs without them in its configuration. Startup writes nothing; ateapi refuses to start with authorization enabled unless at least one valid bootstrap owner is configured. With bootstrap owners always present, the global AccessPolicy becomes an ordinary user-managed resource: restore the CreateGlobalAccessPolicy RPC (authorized by can_create_access_policy on global:root), allow empty bindings to revoke every stored grant, and remove BootstrapGlobalOwners and the global owner retention rules.
| validAtespaceRoles = []string{authz.RoleOwner, authz.RoleEditor, authz.RoleViewer} | ||
| ) | ||
|
|
||
| func (s *RPCService) CreateGlobalAccessPolicy(ctx context.Context, req *ateapipb.CreateGlobalAccessPolicyRequest) (*ateapipb.AccessPolicy, error) { |
There was a problem hiding this comment.
Can you add basic CRUD functional tests under functionaltest/access_policy_test.go?
There was a problem hiding this comment.
Done.
There was a problem hiding this comment.
Have you consider always wiring the whole stack and making authzBootstrapOwners only control whether we enforce the authz check or not in the interceptor?
There was a problem hiding this comment.
Having a single path for all the wiring removes a bunch of complexity downstream. For example, you get the invariant that policymanager is never nil in the store, the interceptor, etc. Then we could have a single conditional to skip the check if the flag is disabled.
Note that if you do this, that only conditional gives you a nice place to write a log and do sort of "dark lunch" of the authz feature (just evaluate the permission but not enforce it, log the result).
There was a problem hiding this comment.
+1. I have implemented it this way: the enforcement is always done on check.
Since Governance RPCs are now exposed API, i add an additional check to make sure no one unauthorized can write policies.
The OpenFGA server, Authorizer and PolicyManager are now always created, and --experimental-enable-authz only controls whether the interceptor enforces checks. Policy tuples therefore stay in sync with the store in every mode, so deleting an atespace always removes its tuples. AccessPolicy RPCs are marked alwaysEnforce and are checked even when enforcement is off, so nobody can grant themselves access before it is turned on. A test fails if an AccessPolicy RPC lacks that marker. ErrAuthzDisabled, the FailedPrecondition mapping and every nil PolicyManager check are removed; test stores get a PolicyManager like the server does. Adds functional CRUD tests for both AccessPolicy scopes and a Postgres test that a policy update and its tuple writes commit or roll back together.
| // OpenFGA tuples in the same transaction as access policy and atespace | ||
| // mutations. It must be set before the store serves access policy or | ||
| // atespace writes. | ||
| func (p *Persistence) SetPolicyManager(pm *authz.PolicyManager) { |
There was a problem hiding this comment.
Can we remove this and pass PolicyManager in NewPersistence? That way we enforce that all instances of Persistence have a PolicyManager. Otherwise, this type can only work (and not crash) if always NewPersistence if followed by SetPolicyManager.
There was a problem hiding this comment.
We can't simply pass the PolicyManager to NewPersistence because PolicyManager acts on already created OpenFGA tables and NewPersistence actually apply the migrations which creates those tables.
A better way would be to actually create policy manager in Persistence after migrations are applied. There are two different layers where FGAServer is used:
Authorizer- for runtime enforcement.PolicyManager- for actual policy writes via control plane API.
Persistence can create PolicyManager. Then in main, Authorizer can be built from the same OpenFGAServer.
Wdyt?
Julian Gutierrez Oschmann (juli4n)
left a comment
Julian Gutierrez Oschmann (juli4n)
left a comment
There was a problem hiding this comment.
I'll let Taahir Ahmed (@ahmedtd) do a final pass. LGTM to me.
We can merge and iterate on minor findings.
|
Needs rebase. |
Resolve the conflict in the generated ateapi.pb.go by regenerating it from the merged proto. Main moved request validation from controlapi into apivalidation, so the AccessPolicy validators move there too: the handlers call exported apivalidation shims, and ValidateCustom_AccessPolicy_Metadata and the bindings checks live in apivalidation/access_policy.go, with tests for each exported function. Main also removed atepg's newCreateMetadata, so the access policy store uses setCreateMetadata like the other resources.
Confirmed with Taahir Ahmed (@ahmedtd) he is ok with PR. |
Working on #1563
Fixes #1697, #1701
Summary
Adds
AccessPolicysingleton subresources for the Global (global:root) andAtespacescopes, stored in PostgreSQL and enforced through OpenFGA.Changes
Controlservice)CreateGlobalAccessPolicy,GetGlobalAccessPolicy,UpdateGlobalAccessPolicy(no Delete; an update with empty bindings clears all grants).CreateAtespaceAccessPolicy,GetAtespaceAccessPolicy,UpdateAtespaceAccessPolicy,DeleteAtespaceAccessPolicy.Binding.membershas set semantics.AuthorizerandPolicyManagerare always created.--experimental-enable-authzonly controls whether the interceptor enforces checks on regular RPCs. Policy tuples therefore stay in sync with the store in every mode, and deleting an atespace always removes its tuples.alwaysEnforcein the permission registry and are checked even when enforcement is off, so nobody can grant themselves access before it is turned on. A test fails if an AccessPolicy RPC is added without that marker.--authz-bootstrap-ownerslists server-configured owners (required when enforcing). They are never written to the store:Authorizer.Checkadds a contextualowner global:roottuple for the calling user only, so access follows the server config and is dropped on restart without them. A bootstrap owner creates the global policy.000003addsglobal_access_policyandatespace_access_policies. Deleting an atespace cascades to its policy row and removes its OpenFGA tuples in the same transaction.PolicyManagertakes an explicitpgx.Tx, locks the metadata row (FOR UPDATE) and reconciles OpenFGA tuples in the same transaction. Optimistic concurrency viauid/version.*, or control-character bootstrap owner entries; non-canonical OpenFGA error codes are mapped toInternal.Testing
functionaltest/access_policy_test.go).Notes