Skip to content

Commit 5331b0a

Browse files
balcsidabalcsida
committed
Improve ABAC registry handling
1 parent 2abb822 commit 5331b0a

File tree

4 files changed

+169
-21
lines changed

4 files changed

+169
-21
lines changed

‎cmd/repository/image_functions.go‎

Lines changed: 44 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -104,8 +104,18 @@ func GetRepositoryAndTagRegex(filter string) (string, string, error) {
104104
// CollectTagFilters collects all matching repos and collects the associated tag filters
105105
func CollectTagFilters(ctx context.Context, rawFilters []string, client acrapi.BaseClientAPI, regexMatchTimeout int64, repoPageSize int32) (map[string]string, error) {
106106
allRepoNames, err := GetAllRepositoryNames(ctx, client, repoPageSize)
107+
isABACRegistry := false
108+
109+
// If catalog listing fails (common in ABAC registries), we'll handle filters differently
107110
if err != nil {
108-
return nil, err
111+
// Check if this is likely an ABAC registry catalog listing permission issue
112+
if strings.Contains(err.Error(), "UNAUTHORIZED") || strings.Contains(err.Error(), "401") ||
113+
strings.Contains(err.Error(), "403") || strings.Contains(err.Error(), "FORBIDDEN") {
114+
isABACRegistry = true
115+
allRepoNames = []string{} // Start with empty list for ABAC handling
116+
} else {
117+
return nil, err // Return other errors as-is
118+
}
109119
}
110120

111121
tagFilters := map[string]string{}
@@ -114,10 +124,26 @@ func CollectTagFilters(ctx context.Context, rawFilters []string, client acrapi.B
114124
if err != nil {
115125
return nil, err
116126
}
117-
repoNames, err := GetMatchingRepos(allRepoNames, "^"+repoRegex+"$", regexMatchTimeout)
118-
if err != nil {
119-
return nil, err
127+
128+
var repoNames []string
129+
130+
if isABACRegistry {
131+
// For ABAC registries, treat repository patterns as exact names if they don't contain regex metacharacters
132+
// This handles common cases where users specify exact repository names
133+
if isLikelyExactRepoName(repoRegex) {
134+
repoNames = []string{repoRegex}
135+
} else {
136+
// For complex repo patterns in ABAC registries, we can't list repositories,
137+
// so return an error with helpful message
138+
return nil, fmt.Errorf("ABAC registry detected: complex repository patterns (%s) require catalog listing permissions. Use exact repository names or add 'Container Registry Repository Catalog Lister' role", repoRegex)
139+
}
140+
} else {
141+
repoNames, err = GetMatchingRepos(allRepoNames, "^"+repoRegex+"$", regexMatchTimeout)
142+
if err != nil {
143+
return nil, err
144+
}
120145
}
146+
121147
for _, repoName := range repoNames {
122148
if _, ok := tagFilters[repoName]; ok {
123149
// To only iterate through a repo once a big regex filter is made of all the filters of a particular repo.
@@ -131,6 +157,20 @@ func CollectTagFilters(ctx context.Context, rawFilters []string, client acrapi.B
131157
return tagFilters, nil
132158
}
133159

160+
// isLikelyExactRepoName checks if a repository pattern is likely an exact repository name
161+
// rather than a regex pattern by looking for common regex metacharacters
162+
func isLikelyExactRepoName(repoPattern string) bool {
163+
// Common regex metacharacters that would indicate this is a pattern, not an exact name
164+
regexChars := []string{".", "*", "+", "?", "^", "$", "[", "]", "(", ")", "|", "\\", "{", "}"}
165+
166+
for _, char := range regexChars {
167+
if strings.Contains(repoPattern, char) {
168+
return false
169+
}
170+
}
171+
return true
172+
}
173+
134174
// GetLastTagFromResponse extracts the last tag from pagination headers in the response.
135175
func GetLastTagFromResponse(resultTags *acr.RepositoryTagsType) string {
136176
// The lastTag is updated to keep the for loop going.

‎internal/api/acrsdk.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ const (
3939
", " + manifestOCIImageIndexContentType +
4040
", " + manifestImageContentType +
4141
", " + manifestListContentType
42+
registryCatalogScope = "registry:catalog:*"
4243
)
4344

4445
// The AcrCLIClient is the struct that will be in charge of doing the http requests to the registry.
@@ -101,13 +102,13 @@ func newAcrCLIClientWithBearerAuth(loginURL string, refreshToken string) (AcrCLI
101102
ctx := context.Background()
102103
// Try to get a token with both catalog and repository wildcard scope for non-ABAC registries
103104
// This maintains backward compatibility while supporting ABAC registries
104-
scope := "registry:catalog:* repository:*:pull"
105+
scope := registryCatalogScope + " repository:*:pull"
105106
accessTokenResponse, err := newAcrCLIClient.AutorestClient.GetAcrAccessToken(ctx, loginURL, scope, refreshToken)
106107
isABAC := false
107108
if err != nil {
108109
// If the above fails (likely ABAC registry), fallback to catalog-only scope
109110
// Repository-specific scopes will be requested when needed
110-
accessTokenResponse, err = newAcrCLIClient.AutorestClient.GetAcrAccessToken(ctx, loginURL, "registry:catalog:*", refreshToken)
111+
accessTokenResponse, err = newAcrCLIClient.AutorestClient.GetAcrAccessToken(ctx, loginURL, registryCatalogScope, refreshToken)
111112
if err != nil {
112113
return newAcrCLIClient, err
113114
}
@@ -204,7 +205,7 @@ func refreshAcrCLIClientToken(ctx context.Context, c *AcrCLIClient, scope string
204205
func refreshTokenForRepository(ctx context.Context, c *AcrCLIClient, repoName string) error {
205206
// For ABAC-enabled registries, we need to specify exact permissions
206207
// Using pull,push,delete covers all necessary operations
207-
scope := fmt.Sprintf("repository:%s:pull,push,delete", repoName)
208+
scope := fmt.Sprintf("%s repository:%s:pull,push,delete", registryCatalogScope, repoName)
208209
return refreshAcrCLIClientToken(ctx, c, scope)
209210
}
210211

‎internal/api/acrsdk_test.go‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
package api
55

66
import (
7+
"context"
78
"encoding/base64"
89
"fmt"
910
"net/http"
@@ -13,7 +14,9 @@ import (
1314
"reflect"
1415
"strings"
1516
"testing"
17+
"time"
1618

19+
acrapi "github.com/Azure/acr-cli/acr"
1720
"github.com/Azure/go-autorest/autorest"
1821
"github.com/Azure/go-autorest/autorest/adal"
1922
)
@@ -234,3 +237,79 @@ func TestGetAcrCLIClientWithAuth(t *testing.T) {
234237
})
235238
}
236239
}
240+
241+
func TestRefreshTokenForRepositoryIncludesCatalogScope(t *testing.T) {
242+
repoName := "library/test"
243+
refreshToken := "test-refresh-token"
244+
exp := time.Now().Add(time.Hour).Unix()
245+
payload := fmt.Sprintf(`{"exp":%d,"access":[{"type":"registry","name":"catalog","actions":["*"]},{"type":"repository","name":"%s","actions":["pull","push","delete"]}]}`, exp, repoName)
246+
testAccessToken := strings.Join([]string{
247+
base64.RawURLEncoding.EncodeToString([]byte(`{"alg":"RS256"}`)),
248+
base64.RawURLEncoding.EncodeToString([]byte(payload)),
249+
"",
250+
}, ".")
251+
expectedScope := fmt.Sprintf("%s repository:%s:pull,push,delete", registryCatalogScope, repoName)
252+
253+
var authServer *httptest.Server
254+
authServer = httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
255+
if r.URL.Path != "/oauth2/token" {
256+
t.Fatalf("unexpected request path %s", r.URL.Path)
257+
}
258+
if r.Method != http.MethodPost {
259+
t.Fatalf("unexpected request method %s", r.Method)
260+
}
261+
if err := r.ParseForm(); err != nil {
262+
t.Fatalf("unable to parse form: %v", err)
263+
}
264+
if got := r.PostForm.Get("scope"); got != expectedScope {
265+
t.Fatalf("unexpected scope %q", got)
266+
}
267+
if got := r.PostForm.Get("refresh_token"); got != refreshToken {
268+
t.Fatalf("unexpected refresh token %q", got)
269+
}
270+
if got := r.PostForm.Get("service"); got != authServer.URL {
271+
t.Fatalf("unexpected service %q", got)
272+
}
273+
if _, err := fmt.Fprintf(w, `{"access_token":%q}`, testAccessToken); err != nil {
274+
t.Fatalf("unable to write access token: %v", err)
275+
}
276+
}))
277+
defer authServer.Close()
278+
279+
client := AcrCLIClient{
280+
AutorestClient: acrapi.NewWithoutDefaults(authServer.URL),
281+
manifestTagFetchCount: manifestTagFetchCount,
282+
loginURL: authServer.URL,
283+
token: &adal.Token{
284+
RefreshToken: refreshToken,
285+
},
286+
currentScopes: []string{registryCatalogScope},
287+
isABAC: true,
288+
}
289+
290+
sender := autorest.CreateSender()
291+
httpClient, ok := sender.(*http.Client)
292+
if !ok {
293+
t.Fatalf("unexpected sender type %T", sender)
294+
}
295+
httpClient.Transport = authServer.Client().Transport
296+
client.AutorestClient.Sender = sender
297+
298+
if err := refreshTokenForRepository(context.Background(), &client, repoName); err != nil {
299+
t.Fatalf("refreshTokenForRepository() error = %v", err)
300+
}
301+
302+
if client.token == nil || client.token.AccessToken != testAccessToken {
303+
t.Fatalf("unexpected access token %v", client.token)
304+
}
305+
expectedScopes := []string{
306+
registryCatalogScope,
307+
fmt.Sprintf("repository:%s:pull,push,delete", repoName),
308+
}
309+
if !reflect.DeepEqual(client.currentScopes, expectedScopes) {
310+
t.Fatalf("unexpected scopes %v", client.currentScopes)
311+
}
312+
if client.accessTokenExp != exp {
313+
t.Fatalf("unexpected expiration %d", client.accessTokenExp)
314+
}
315+
}

‎scripts/experimental/test-abac-performance.sh‎

Lines changed: 42 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ NUM_REPOS="${3:-5}"
1212

1313
# Path configuration
1414
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
15-
ACR_CLI="${SCRIPT_DIR}/../bin/acr"
15+
ACR_CLI="${SCRIPT_DIR}/../../bin/acr"
1616

1717
# Source registry utilities
1818
source "${SCRIPT_DIR}/registry-utils.sh"
@@ -29,8 +29,36 @@ CYAN='\033[0;36m'
2929
MAGENTA='\033[0;35m'
3030
NC='\033[0m'
3131

32-
# Performance metrics
33-
declare -A METRICS
32+
# Performance metrics - check if associative arrays are supported
33+
ASSOCIATIVE_ARRAYS_SUPPORTED=true
34+
declare -A METRICS 2>/dev/null || {
35+
ASSOCIATIVE_ARRAYS_SUPPORTED=false
36+
# Fallback for shells that don't support associative arrays
37+
METRICS_token_refresh_sequential=""
38+
METRICS_token_refresh_parallel=""
39+
METRICS_token_refresh_rapid=""
40+
METRICS_concurrent_operations=""
41+
}
42+
43+
# Helper functions for metrics
44+
set_metric() {
45+
local key="$1"
46+
local value="$2"
47+
if [ "$ASSOCIATIVE_ARRAYS_SUPPORTED" = true ]; then
48+
METRICS["$key"]="$value"
49+
else
50+
eval "METRICS_$key=\"$value\""
51+
fi
52+
}
53+
54+
get_metric() {
55+
local key="$1"
56+
if [ "$ASSOCIATIVE_ARRAYS_SUPPORTED" = true ]; then
57+
echo "${METRICS[$key]:-}"
58+
else
59+
eval "echo \${METRICS_$key:-}"
60+
fi
61+
}
3462

3563
# Helper to measure execution time
3664
measure_time() {
@@ -77,7 +105,7 @@ validate_setup() {
77105

78106
if [ ! -f "$ACR_CLI" ]; then
79107
echo "Building ACR CLI..."
80-
(cd "$SCRIPT_DIR/.." && make binaries)
108+
(cd "$SCRIPT_DIR/../.." && make binaries)
81109
fi
82110
}
83111

@@ -133,7 +161,7 @@ test_token_refresh_performance() {
133161
total_time=$(awk -v t="$total_time" -v d="$duration" 'BEGIN {printf "%.3f", t+d}')
134162
done
135163

136-
METRICS["token_refresh_sequential"]="$total_time"
164+
set_metric "token_refresh_sequential" "$total_time"
137165
echo -e "${GREEN}Total sequential time: ${total_time}s${NC}"
138166

139167
# Test rapid switching between repositories
@@ -147,7 +175,7 @@ test_token_refresh_performance() {
147175
done
148176
")
149177

150-
METRICS["token_refresh_rapid"]="$switch_time"
178+
set_metric "token_refresh_rapid" "$switch_time"
151179
echo -e "${GREEN}Rapid switching time (30 operations): ${switch_time}s${NC}"
152180

153181
# Clean up
@@ -185,7 +213,7 @@ test_repository_permission_performance() {
185213
}')
186214

187215
echo " $repo ($tag_count tags): ${duration}s (${throughput} tags/sec)"
188-
METRICS["list_${repo}"]="$duration"
216+
set_metric "list_${repo}" "$duration"
189217
done
190218

191219
# Test deletion performance
@@ -205,7 +233,7 @@ test_repository_permission_performance() {
205233
}')
206234

207235
echo " $repo ($tag_count tags): ${duration}s (${throughput} tags/sec)"
208-
METRICS["purge_dryrun_${repo}"]="$duration"
236+
set_metric "purge_dryrun_${repo}" "$duration"
209237
done
210238

211239
# Clean up
@@ -249,7 +277,7 @@ test_concurrent_cross_repository() {
249277
echo " Throughput: ${throughput} deletions/sec"
250278
echo " Repositories affected: $NUM_REPOS"
251279

252-
METRICS["concurrent_${concurrency}"]="$duration"
280+
set_metric "concurrent_${concurrency}" "$duration"
253281

254282
# Recreate deleted images for next test
255283
if [ "$concurrency" -lt 20 ]; then
@@ -311,7 +339,7 @@ test_pattern_matching_performance() {
311339
--ago 0d \
312340
--dry-run >/dev/null 2>&1)
313341
echo " Time: ${duration}s"
314-
METRICS["pattern_simple"]="$duration"
342+
set_metric "pattern_simple" "$duration"
315343

316344
# Medium complexity pattern
317345
echo -e "\n${BLUE}Medium pattern (v1\.[0-9]+\.0):${NC}"
@@ -321,7 +349,7 @@ test_pattern_matching_performance() {
321349
--ago 0d \
322350
--dry-run >/dev/null 2>&1)
323351
echo " Time: ${duration}s"
324-
METRICS["pattern_medium"]="$duration"
352+
set_metric "pattern_medium" "$duration"
325353

326354
# Complex pattern
327355
echo -e "\n${BLUE}Complex pattern ((dev|staging)-[0-9]{3}):${NC}"
@@ -331,7 +359,7 @@ test_pattern_matching_performance() {
331359
--ago 0d \
332360
--dry-run >/dev/null 2>&1)
333361
echo " Time: ${duration}s"
334-
METRICS["pattern_complex"]="$duration"
362+
set_metric "pattern_complex" "$duration"
335363

336364
# Very complex pattern
337365
echo -e "\n${BLUE}Very complex pattern (build-2024[0-9]{4}-0[0-1][0-9]):${NC}"
@@ -341,7 +369,7 @@ test_pattern_matching_performance() {
341369
--ago 0d \
342370
--dry-run >/dev/null 2>&1)
343371
echo " Time: ${duration}s"
344-
METRICS["pattern_very_complex"]="$duration"
372+
set_metric "pattern_very_complex" "$duration"
345373

346374
# Clean up
347375
"$ACR_CLI" purge --registry "$REGISTRY" --filter "$repo:.*" --ago 0d >/dev/null 2>&1
@@ -369,7 +397,7 @@ test_scale_performance() {
369397
# Create images
370398
local create_time=$(measure_time create_test_images_batch "$repo" "$scale")
371399
echo " Creation time: ${create_time}s"
372-
METRICS["scale_${scale}_create"]="$create_time"
400+
set_metric "scale_${scale}_create" "$create_time"
373401

374402
# List performance
375403
local list_time=$(measure_time "$ACR_CLI" tag list \

0 commit comments

Comments
 (0)