Fix: Support ABAC-enabled registries #504
Conversation
balcsida
requested review from
Wwwsylvia,
estebanreyl,
northtyphoon,
sajayantony,
shizhMSFT and
wangxiaoxuan273
as code owners
balcsida
force-pushed
the
fix/abac-registry-support
branch
from
2ed16de to
8b52681
Compare
balcsida
changed the title
balcsida
force-pushed
the
fix/abac-registry-support
branch
from
8b52681 to
112ab91
Compare
internal/api/acrsdk.go
Outdated
| // This supports both ABAC and non-ABAC registries. | ||
| func refreshTokenForRepository(ctx context.Context, c *AcrCLIClient, repoName string) error { | ||
| // For specific repository operations, request full permissions on that repository | ||
| scope := fmt.Sprintf("repository:%s:*", repoName) |
There was a problem hiding this comment.
I think in order to support ABAC we need to make sure that the tighter permissions are selected for the scope. The way to do this typically is to send a challenge message and get the actual permissions for each type of operation but that's a bit expensive here and probably amounts to a larger rework. I'll follow up with my teammates who are more familiar with the path.
Actually, I think I understand now that we just cant request all the scopes at once like before. In that case this lgtm but I'll still double check and run a test on it
There was a problem hiding this comment.
Sorry, I (again) forgot to push some commits and didn't open the PR as draft. I'm still finishing up the test-abac-registry.sh
There was a problem hiding this comment.
Additionally, I'm thinking about using testscript instead of using bash scripts for these... functional test? I don't really know the correct terminology here
There was a problem hiding this comment.
"repository:%s:*" does not work for ABAC-enabled registries either. For ABAC registries, the request must include the specific scope, such as repository:%s:pull,push.
…utput, and credential consistency
…ping for exact tag matching
… age filtering, retry logic, and count sanitization
balcsida
force-pushed
the
fix/abac-registry-support
branch
from
8f77998 to
49686b0
Compare
internal/api/acrsdk.go
Outdated
| accessTokenResponse, err := newAcrCLIClient.AutorestClient.GetAcrAccessToken(ctx, loginURL, "registry:catalog:* repository:*:*", refreshToken) | ||
| // For ABAC-enabled registries, only request catalog scope initially | ||
| // Repository-specific scopes will be requested when needed | ||
| accessTokenResponse, err := newAcrCLIClient.AutorestClient.GetAcrAccessToken(ctx, loginURL, "registry:catalog:*", refreshToken) |
There was a problem hiding this comment.
We can't simply remove the repository scope check from newAcrCLIClientWithBearerAuth. The new refreshTokenForRepository method is only invoked when the existing token has expired. Other files still call this method to obtain the CLI client with the token (example)
…rove test execution speed
…and use specific ABAC permissions - Try repository:*:pull scope first for non-ABAC registries to maintain backward compatibility - Fallback to catalog-only scope for ABAC registries - Use specific permissions (pull,push,delete) instead of wildcard for ABAC repository operations - Addresses lizMSFT review comments on PR Azure#504
…and use specific ABAC permissions - Try repository:*:pull scope first for non-ABAC registries to maintain backward compatibility - Fallback to catalog-only scope for ABAC registries - Use specific permissions (pull,push,delete) instead of wildcard for ABAC repository operations
balcsida
force-pushed
the
fix/abac-registry-support
branch
from
9be8375 to
e2ff253
Compare
balcsida
force-pushed
the
fix/abac-registry-support
branch
from
5331b0a to
b88473d
Compare
3 participants
Summary
repository:*:*) that are incompatible with ABAC-enabled registriesregistry:catalog:*scope onlyChanges
registry:catalog:*scoperepository:<name>:*) for each operationrefreshTokenForRepositoryandrefreshTokenForCatalogfor cleaner scope managementTest Plan
go test ./internal/api/...)go test ./cmd/acr/...)go build)acr purgecommand works with identity having full data plane permissionsFixes #501