Skip to content

Commit 55f98da

Browse files
committed
fix(auth/api): require every requested region to be permitted
The Regions dimension of a permission constraint was matched with containsAny, the same any-overlap rule the other dimensions use. That rule is safe for AccountIDs, Providers and Services because a request names exactly one value on each of them, so any-match and all-match coincide. Regions is the one dimension a single request legitimately spans several values on: an Azure RI exchange submits every target's location at once via targetLocations. The result was an authorization bypass on a money path. A caller permitted only in eastus could attach a westus target to the exchange; containsAny found eastus in the permitted set, returned true, and the irreversible exchange executed for BOTH regions. "Regions limits to specific regions" cannot mean "limits to requests that mention at least one permitted region". matchAllRegionsConstraint now backs that dimension: every requested region must be permitted, comparison is case- and whitespace- insensitive, and the empty-list semantics are unchanged (an unconstrained permission, or a request naming no region, still matches). Every other dimension keeps containsAny. Single-region callers (the AWS reshape execute path, purchaseConstraintSets) are unaffected: for a one-element request all-match and any-match are the same test, and the case-insensitivity only widens what they match. Alongside it, targetLocations canonicalizes each location to trimmed lower case before de-duplicating, so the constraint set names each target region exactly once instead of demanding a permission for two spellings of one place ("EastUS", the casing the Azure portal shows, and "eastus", the casing its APIs return). Target validation now rejects a whitespace-only location as well, which previously survived the empty check and would reach the permission check as "", a region no permission can name. Regression tests cover the bypass through both matchConstraints and the real HasPermission entry point, the case and whitespace handling on both sides, and the targetLocations normalization both directly and through executeAzureExchange's submitted constraint set. Each fails against the pre-fix code. Refs #596
1 parent 1d01d6d commit 55f98da

4 files changed

Lines changed: 350 additions & 24 deletions

File tree

‎internal/api/handler_ri_exchange.go‎

Lines changed: 44 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -406,7 +406,11 @@ func validateAzureExchangeTargets(targets []AzureExchangeTargetBody, subscriptio
406406
if t.SKU == "" {
407407
return NewClientError(400, fmt.Sprintf("targets[%d].sku is required", i))
408408
}
409-
if t.Location == "" {
409+
// Blank-but-not-empty is rejected too: targetLocations trims before
410+
// it builds the Regions constraint, so a whitespace-only location
411+
// would otherwise reach the permission check as "" -- a region no
412+
// permission can name.
413+
if strings.TrimSpace(t.Location) == "" {
410414
return NewClientError(400, fmt.Sprintf("targets[%d].location is required", i))
411415
}
412416
if t.BillingScopeID != "" && !strings.EqualFold(t.BillingScopeID, scope) {
@@ -494,17 +498,34 @@ func toAzureExchangeTargets(targets []AzureExchangeTargetBody, subscriptionID st
494498
return out, nil
495499
}
496500

497-
// targetLocations returns the de-duplicated set of target locations, used to
498-
// populate the Regions dimension of the execute:ri-exchange constraint
499-
// check. Callers must have already validated that every target has a
500-
// non-empty Location.
501+
// targetLocations returns the de-duplicated, canonically lower-cased set of
502+
// target locations, used to populate the Regions dimension of the
503+
// execute:ri-exchange constraint check. Callers must have already validated
504+
// that every target has a non-blank Location.
505+
//
506+
// Azure treats location names case-insensitively and its own APIs return the
507+
// lower-case form, so lower case is the canonical spelling. Normalizing
508+
// before the dedup collapses "EastUS" -- the casing the Azure portal
509+
// displays -- and "eastus" into the single region they actually are, so the
510+
// constraint set names each target region exactly once rather than demanding
511+
// a permission for two spellings of one place.
512+
//
513+
// auth.matchAllRegionsConstraint compares case-insensitively as well, so a
514+
// permission stored in either casing matches either way. Normalizing here
515+
// keeps the two layers agreeing on what a region is instead of leaving the
516+
// constraint set's accuracy resting on the matcher's leniency.
517+
//
518+
// toAzureExchangeTargets sends Azure the raw, un-normalized Location. That
519+
// is not a gap: Azure resolves either casing to the same region, so the
520+
// region authorized here is the region the exchange lands in.
501521
func targetLocations(targets []AzureExchangeTargetBody) []string {
502522
seen := make(map[string]bool, len(targets))
503523
out := make([]string, 0, len(targets))
504524
for _, t := range targets {
505-
if !seen[t.Location] {
506-
seen[t.Location] = true
507-
out = append(out, t.Location)
525+
location := strings.ToLower(strings.TrimSpace(t.Location))
526+
if !seen[location] {
527+
seen[location] = true
528+
out = append(out, location)
508529
}
509530
}
510531
return out
@@ -814,21 +835,6 @@ func checkAzureExchangeMoneyGuardrails(preview *azurecompute.ExchangePreview, ma
814835
return nil
815836
}
816837

817-
// executeAzureExchange executes an Azure RI exchange with mandatory
818-
// spend-cap and currency guardrails. Requires "execute:ri-exchange"
819-
// (deliberately separate from execute:purchases), mirroring the AWS
820-
// executeExchange handler: RI exchanges are financially irreversible once
821-
// submitted.
822-
//
823-
// Unlike a design that executes a client-supplied session_id, this handler
824-
// never trusts the caller's own pricing: it re-runs CalculateExchange itself
825-
// against the caller's sources/targets, validates the FRESH quote against
826-
// every guardrail in checkAzureExchangeMoneyGuardrails, and only then calls
827-
// ExecuteExchange with the SessionID *that fresh call returned*. A
828-
// client-supplied or stale session would bypass every guardrail below, so
829-
// the server always re-quotes immediately before committing.
830-
//
831-
// POST /api/ri-exchange/azure-instances/exchange.
832838
// parseAzureExecuteRequest decodes and validates an execute request body and
833839
// parses its spend cap into an exact rational.
834840
//
@@ -851,6 +857,21 @@ func parseAzureExecuteRequest(rawBody string) (AzureExecuteExchangeRequestBody,
851857
return body, maxRat, nil
852858
}
853859

860+
// executeAzureExchange executes an Azure RI exchange with mandatory
861+
// spend-cap and currency guardrails. Requires "execute:ri-exchange"
862+
// (deliberately separate from execute:purchases), mirroring the AWS
863+
// executeExchange handler: RI exchanges are financially irreversible once
864+
// submitted.
865+
//
866+
// Unlike a design that executes a client-supplied session_id, this handler
867+
// never trusts the caller's own pricing: it re-runs CalculateExchange itself
868+
// against the caller's sources/targets, validates the FRESH quote against
869+
// every guardrail in checkAzureExchangeMoneyGuardrails, and only then calls
870+
// ExecuteExchange with the SessionID *that fresh call returned*. A
871+
// client-supplied or stale session would bypass every guardrail below, so
872+
// the server always re-quotes immediately before committing.
873+
//
874+
// POST /api/ri-exchange/azure-instances/exchange.
854875
func (h *Handler) executeAzureExchange(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) {
855876
session, err := h.requirePermission(ctx, req, "execute", "ri-exchange")
856877
if err != nil {

‎internal/api/handler_ri_exchange_azure_test.go‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,8 @@ func TestValidateAzureOfferingsBody(t *testing.T) {
120120
missingSKU.SKU = ""
121121
missingLocation := azureOfferingsTarget()
122122
missingLocation.Location = ""
123+
blankLocation := azureOfferingsTarget()
124+
blankLocation.Location = " "
123125
foreignBillingScope := azureOfferingsTarget()
124126
foreignBillingScope.BillingScopeID = "/subscriptions/someone-elses-sub"
125127
zeroTargetQty := azureOfferingsTarget()
@@ -177,6 +179,14 @@ func TestValidateAzureOfferingsBody(t *testing.T) {
177179
AzureCompatibleOfferingsRequestBody{SubscriptionID: "sub-1", Sources: []AzureExchangeSourceBody{azureOfferingsSource()}, Targets: []AzureExchangeTargetBody{missingLocation}},
178180
"targets[0].location is required",
179181
},
182+
{
183+
// Whitespace-only survives the empty check but trims to "" in
184+
// targetLocations, i.e. a Regions constraint entry no permission
185+
// can name. Reject it at the boundary instead.
186+
"target location is whitespace only",
187+
AzureCompatibleOfferingsRequestBody{SubscriptionID: "sub-1", Sources: []AzureExchangeSourceBody{azureOfferingsSource()}, Targets: []AzureExchangeTargetBody{blankLocation}},
188+
"targets[0].location is required",
189+
},
180190
{
181191
"target names a foreign billing scope",
182192
AzureCompatibleOfferingsRequestBody{SubscriptionID: "sub-1", Sources: []AzureExchangeSourceBody{azureOfferingsSource()}, Targets: []AzureExchangeTargetBody{foreignBillingScope}},
@@ -1256,6 +1266,124 @@ func TestExecuteAzureExchange_ConstraintSetPinsAllDimensions(t *testing.T) {
12561266
assert.InDelta(t, 100.00, c.MaxPurchaseAmount, 0.0001)
12571267
}
12581268

1269+
// TestTargetLocations_CanonicalizesCase pins the normalization targetLocations
1270+
// applies before its output becomes the Regions dimension of the
1271+
// execute:ri-exchange constraint check.
1272+
//
1273+
// The permission side compares region names literally, so an un-normalized
1274+
// "EastUS" -- the spelling the Azure portal shows -- would not match a
1275+
// permission stored as "eastus", and the caller would be 403'd out of a
1276+
// region they legitimately hold. Normalizing before the dedup additionally
1277+
// collapses "EastUS" and "eastus" into the single region they actually are,
1278+
// instead of submitting two entries that must both be permitted.
1279+
func TestTargetLocations_CanonicalizesCase(t *testing.T) {
1280+
tests := []struct {
1281+
name string
1282+
targets []AzureExchangeTargetBody
1283+
want []string
1284+
}{
1285+
{
1286+
name: "no targets yields an empty set",
1287+
targets: nil,
1288+
want: []string{},
1289+
},
1290+
{
1291+
name: "already-canonical locations pass through in first-seen order",
1292+
targets: []AzureExchangeTargetBody{{Location: "westeurope"}, {Location: "eastus"}},
1293+
want: []string{"westeurope", "eastus"},
1294+
},
1295+
{
1296+
name: "mixed-case location is lower-cased",
1297+
targets: []AzureExchangeTargetBody{{Location: "EastUS"}},
1298+
want: []string{"eastus"},
1299+
},
1300+
{
1301+
name: "case variants of one region collapse into a single entry",
1302+
targets: []AzureExchangeTargetBody{{Location: "EastUS"}, {Location: "eastus"}, {Location: "EASTUS"}},
1303+
want: []string{"eastus"},
1304+
},
1305+
{
1306+
name: "surrounding whitespace is trimmed before dedup",
1307+
targets: []AzureExchangeTargetBody{{Location: " EastUS "}, {Location: "eastus"}},
1308+
want: []string{"eastus"},
1309+
},
1310+
{
1311+
name: "distinct regions survive while their case variants collapse",
1312+
targets: []AzureExchangeTargetBody{
1313+
{Location: "EastUS"},
1314+
{Location: "WestEurope"},
1315+
{Location: "eastus"},
1316+
},
1317+
want: []string{"eastus", "westeurope"},
1318+
},
1319+
}
1320+
1321+
for _, tt := range tests {
1322+
t.Run(tt.name, func(t *testing.T) {
1323+
assert.Equal(t, tt.want, targetLocations(tt.targets))
1324+
})
1325+
}
1326+
}
1327+
1328+
// TestExecuteAzureExchange_ConstraintRegionsAreLowerCased drives the same
1329+
// normalization through the handler, so the constraint set actually submitted
1330+
// for a portal-cased request is pinned and not merely the helper's return
1331+
// value. Without it, a body naming "EastUS" and "eastus" would submit two
1332+
// Regions entries in the portal's casing and the permission check would
1333+
// compare them literally against the lower-case names permissions store.
1334+
func TestExecuteAzureExchange_ConstraintRegionsAreLowerCased(t *testing.T) {
1335+
ctx := context.Background()
1336+
var captured []auth.PermissionConstraints
1337+
1338+
mockAuth := new(MockAuthService)
1339+
mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil)
1340+
mockAuth.On("HasPermissionAPI", ctx, "user-1", "execute", "ri-exchange").Return(true, nil)
1341+
allowAnyAccountScope(mockAuth)
1342+
mockAuth.On("HasPermissionForConstraintsAPI", ctx, "user-1", "execute", "ri-exchange",
1343+
mock.MatchedBy(func(sets []auth.PermissionConstraints) bool {
1344+
captured = sets
1345+
return true
1346+
})).Return(true, nil)
1347+
t.Cleanup(func() { mockAuth.AssertExpectations(t) })
1348+
1349+
mockStore := &MockConfigStore{}
1350+
mockStore.GetCloudAccountByExternalIDFn = func(_ context.Context, _, _ string) (*config.CloudAccount, error) {
1351+
return &config.CloudAccount{ID: "acct-sub-1"}, nil
1352+
}
1353+
1354+
opsClient := new(mockAzureExchangeOpsClient)
1355+
ownsAzureSource(opsClient)
1356+
opsClient.On("CalculateExchange", ctx, mock.Anything, mock.Anything).Return(
1357+
&azurecompute.ExchangePreview{SessionID: "sess-case", NetPayable: toPtr(10.00), NetPayableCurrency: "USD"},
1358+
[]azurecompute.CompatibleOffering{}, nil,
1359+
)
1360+
opsClient.On("ExecuteExchange", ctx, "sess-case").Return(
1361+
&azurecompute.ExchangeResult{SessionID: "sess-case", Status: "Succeeded"}, nil,
1362+
)
1363+
t.Cleanup(func() { opsClient.AssertExpectations(t) })
1364+
1365+
h := &Handler{auth: mockAuth, config: mockStore, azureExchangeFactory: func(_ string) azureExchangeClient { return opsClient }}
1366+
_, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{
1367+
Headers: map[string]string{"authorization": "Bearer tok"},
1368+
Body: `{
1369+
"subscription_id": "sub-1",
1370+
"sources": [{"reservation_id": "res-1", "quantity": 1}],
1371+
"targets": [
1372+
{"sku": "Standard_D4s_v3", "location": "EastUS", "term": "P1Y", "quantity": 1},
1373+
{"sku": "Standard_D8s_v3", "location": "WestEurope", "term": "P3Y", "quantity": 2},
1374+
{"sku": "Standard_D2s_v3", "location": "eastus", "term": "P1Y", "quantity": 1}
1375+
],
1376+
"max_payment_due": "100.00",
1377+
"currency": "USD"
1378+
}`,
1379+
})
1380+
require.NoError(t, err)
1381+
1382+
require.Len(t, captured, 1)
1383+
assert.Equal(t, []string{"eastus", "westeurope"}, captured[0].Regions,
1384+
"portal-cased target locations must reach the permission check lower-cased and de-duplicated")
1385+
}
1386+
12591387
// TestExecuteAzureExchange_LowercaseUSDTakesUSDCapPath closes the other half
12601388
// of the currency case-insensitivity pairing.
12611389
//

‎internal/auth/service_group.go‎

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"errors"
66
"fmt"
7+
"strings"
78
"time"
89

910
"github.com/google/uuid"
@@ -241,7 +242,7 @@ func (s *Service) matchConstraints(permConstraints, reqConstraints *PermissionCo
241242
return s.matchStringListConstraints(permConstraints.AccountIDs, reqConstraints.AccountIDs) &&
242243
s.matchStringListConstraints(permConstraints.Providers, reqConstraints.Providers) &&
243244
s.matchStringListConstraints(permConstraints.Services, reqConstraints.Services) &&
244-
s.matchStringListConstraints(permConstraints.Regions, reqConstraints.Regions) &&
245+
s.matchAllRegionsConstraint(permConstraints.Regions, reqConstraints.Regions) &&
245246
s.matchPurchaseAmountConstraint(permConstraints.MaxPurchaseAmount, reqConstraints.MaxPurchaseAmount)
246247
}
247248

@@ -265,6 +266,43 @@ func (s *Service) matchStringListConstraints(permList, reqList []string) bool {
265266
return true
266267
}
267268

269+
// matchAllRegionsConstraint is the Regions dimension's matcher: EVERY
270+
// requested region must be permitted, not merely one of them.
271+
//
272+
// Regions is the one dimension where a single request legitimately spans
273+
// several values: an Azure RI exchange takes a list of targets, and
274+
// handler_ri_exchange.go's targetLocations feeds all of their locations in
275+
// at once. Under the generic containsAny rule that made the region
276+
// constraint bypassable -- a caller permitted only in eastus could attach a
277+
// westus target, containsAny would find eastus in the permitted set, return
278+
// true, and the exchange would execute for BOTH regions. "Regions limits to
279+
// specific regions" (types.go) cannot mean "limits to requests that mention
280+
// at least one permitted region".
281+
//
282+
// Every other dimension keeps containsAny: a request names one provider,
283+
// one service, one account, so for them any-match and all-match coincide.
284+
//
285+
// Comparison is case-insensitive so a permission stored as "EastUS" still
286+
// matches the canonical lower-case form callers are normalized to; the
287+
// empty-list semantics are unchanged from matchStringListConstraints (an
288+
// unconstrained permission, or a request that does not name a region, still
289+
// matches).
290+
func (s *Service) matchAllRegionsConstraint(permRegions, reqRegions []string) bool {
291+
if len(permRegions) == 0 || len(reqRegions) == 0 {
292+
return true
293+
}
294+
permitted := make(map[string]bool, len(permRegions))
295+
for _, r := range permRegions {
296+
permitted[strings.ToLower(strings.TrimSpace(r))] = true
297+
}
298+
for _, r := range reqRegions {
299+
if !permitted[strings.ToLower(strings.TrimSpace(r))] {
300+
return false
301+
}
302+
}
303+
return true
304+
}
305+
268306
// matchPurchaseAmountConstraint checks if requested amount is within permitted limit.
269307
func (s *Service) matchPurchaseAmountConstraint(permMax, reqMax float64) bool {
270308
if permMax > 0 && reqMax > permMax {

0 commit comments

Comments
 (0)