Repository navigation
objectstore-plugin: move the credential exchange's actor checks into actorauthz - #3138
Anish Ramasekar (aramase) wants to merge 2 commits into
Conversation
22b6cc3 to
b147d4a
Compare
|
cc ILLUM1N0X |
|
A few things:
|
|
ILLUM1N0X updated, PTAL! |
841b626 to
e36a75f
Compare
|
Julian Gutierrez Oschmann (@juli4n) can you take a look re: exposing APIs to enable external storage plugins? |
|
Given the code-slush, can you help me understand if this is fixing a CRITICAL P0 issue, or if this should queue up for post-1.0 (just a couple weeks) |
|
Tim Hockin (@thockin) it doesn't fix a bug, it's step 1 of #3095 for the provider-neutral work in #3161: no behavior change, it moves the actor checks out of the GCS exchange into |
Tim Hockin (thockin)
added
prio/P0
Bowei Du (bowei)
added
area/security
e36a75f to
f2fa59c
Compare
ExchangeActorJwt verified the JWT, looked the actor up, checked it is still on the token's node and derived its grants inline. Move those checks into actorauthz.Authorizer so another provider's exchange can apply the same checks. The exchange now calls Authorize and then the issuer. Codes and messages are unchanged. Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>
…uthz Every exchange serving the objectstore plugin API requires this audience, so it belongs with the checks every exchange runs. Rename exchange.ActorJWTAudience to actorauthz.Audience and update atelet, the plugin and the e2e suite. The value is unchanged. Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>
f2fa59c to
66033a5
Compare
|
The run-tests failure here is the |
| } | ||
| // Vendor mode is what the build uses, and keeps a GOFLAGS=-mod=mod | ||
| // environment from rewriting go.sum. | ||
| out, err := exec.Command("go", "list", "-mod=vendor", "-deps", "-f", "{{.ImportPath}}", ".").Output() |
There was a problem hiding this comment.
we already have hack/verify/ateapi-deps.sh doing this kind of check for cmd/ateapi, across every platform in .ko.yaml. Can we extend that (or add a sibling script) instead of shelling out to go list from a unit test? This only checks the host GOOS/GOARCH, and it's the only unit test in the repo that runs the go toolchain.
alternatively for both we can restrict imports via golangci-lint ... we really shouldn't need to homebrew something here
| func New(verifier Verifier, lookup Lookup, issuer Issuer) *Server { | ||
| return &Server{verifier: verifier, lookup: lookup, issuer: issuer, now: time.Now} | ||
| // New returns a Server. The verifier must accept actorauthz.Audience. | ||
| func New(verifier actorauthz.Verifier, lookup actorauthz.Lookup, issuer Issuer) *Server { |
There was a problem hiding this comment.
non-blocking: can New take the *actorauthz.Authorizer (or a one-method interface) instead of building it here?
Then exchange doesn't need to know about the verifier or the lookup, the audience precondition is only written once (this says "must accept", actorauthz.New says "must accept only"), and the exchange tests could fake the decision instead of rebuilding a tagged running actor on node-1 just to reach the issuer.
| if got != nil { | ||
| t.Errorf("decision = %+v, want nil", got) | ||
| } | ||
| if st := status.Convert(err); st.Code() != tc.want || st.Message() != tc.wantMsg { |
There was a problem hiding this comment.
nit: the old exchange tests checked codes only, these pin the full message, including grpc's rpc error: code = ... desc = ... formatting, and notValid re-runs the real verifier to build the expected string. That's handy for showing the move didn't change anything, but going forward I think code + lookedUp is enough, and won't break when oidcjwt or grpc error text changes.
Step 1 of #3095. No behavior change.
ExchangeActorJwtruns the checks that guard the GCS credential exchange inline. A second provider's exchange would have to copy them and could drift from them.internal/objectstoreplugin/actorauthz.Authorizer.Authorizeverifies the worker-for-actor JWT, looks the actor up, checks it is still on the token's node, and derives its grants. ItsDecisioncarries onlyActorUIDandGrants. Codes and messages are unchanged.exchange.Verifierandexchange.Lookupare deleted.exchange.Newtakes the actorauthz interfaces, andexchange.ServercallsAuthorize, then the issuer.exchange.ActorJWTAudiencebecomesactorauthz.Audience, with the same value. atelet, the plugin and the snapshotcredentials e2e suite use it.Tested with
go test -raceon./internal/objectstoreplugin/...,./cmd/objectstore-plugin/...,./cmd/atelet/...and./internal/resources/...on Linux.AI use: I used Claude Code to implement and review this change, and reviewed the diff myself.