-
Notifications
You must be signed in to change notification settings - Fork 512
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Always validate tenant IDs and introduce max tenants setting (#6959)
This change updates dskit to a version that does _not_ rely on global state for tenant ID parsing. Specifically it pulls in grafana/dskit#445. As part of this there are a few things changing: * We use multi-tenant parsing logic everywhere which actually enforces limits on the length of tenant IDs and the legal characters in them. * Instead of relying on single tenant parsing logic when tenant federation is disabled to reject multi-tenant queries, we add a query middleware that validates the number of expected tenants based on configuration. * We introduce a new setting to limit the max number of tenant IDs that may be included in a multi-tenant query. This change will result in different behavior in a few cases. However, it brings the actual behavior of Mimir in line with the documented behavior. Specifically, the following behavior changes (copied from dskit PR): * SingleResolver did not previously enforce a limit on the length of a tenant ID. A limit of 150 characters is now enforced. This has always been the documented behavior as far back as Cortex, where this code originated. Not enforcing it was an oversight. * SingleResolver previously allowed tenant IDs to contain the | character. This is no longer allowed as part of a tenant ID and instead will be treated as a divider between multiple tenant IDs. This has always been the documented behavior as far back as Cortex, where this code originated. Not enforcing it was an oversight. See grafana/dskit#445
- Loading branch information
1 parent
dbf41d6
commit 41fd54e
Showing
27 changed files
with
322 additions
and
185 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| // SPDX-License-Identifier: AGPL-3.0-only | ||
|
|
||
| package api | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "net/http" | ||
|
|
||
| "github.com/grafana/dskit/middleware" | ||
| "github.com/grafana/dskit/tenant" | ||
| ) | ||
|
|
||
| const ( | ||
| tooManyTenantsTemplate = "too many tenant IDs present in the request. max: %d actual: %d" | ||
| ) | ||
|
|
||
| // newTenantValidationMiddleware creates a new middleware that validates the number of tenants | ||
| // being accessed in a particular request is allowed given the current tenant federation configuration. | ||
| // Note that this middleware requires that tenant ID has been set on the request context by something | ||
| // like middleware.AuthenticateUser. | ||
| func newTenantValidationMiddleware(federation bool, maxTenants int) middleware.Interface { | ||
| return middleware.Func(func(next http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| ctx := r.Context() | ||
|
|
||
| ids, err := tenant.TenantIDs(ctx) | ||
| if err != nil { | ||
| http.Error(w, err.Error(), http.StatusUnauthorized) | ||
| return | ||
| } | ||
|
|
||
| numIds := len(ids) | ||
| if !federation && numIds > 1 { | ||
| http.Error(w, fmt.Sprintf(tooManyTenantsTemplate, 1, numIds), http.StatusUnprocessableEntity) | ||
| return | ||
| } | ||
|
|
||
| if federation && maxTenants > 0 && numIds > maxTenants { | ||
| http.Error(w, fmt.Sprintf(tooManyTenantsTemplate, maxTenants, numIds), http.StatusUnprocessableEntity) | ||
| return | ||
| } | ||
|
|
||
| next.ServeHTTP(w, r.WithContext(ctx)) | ||
| }) | ||
| }) | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| // SPDX-License-Identifier: AGPL-3.0-only | ||
|
|
||
| package api | ||
|
|
||
| import ( | ||
| "io" | ||
| "net/http" | ||
| "net/http/httptest" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/grafana/dskit/middleware" | ||
| "github.com/grafana/dskit/tenant" | ||
| "github.com/grafana/dskit/user" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestNewTenantValidationMiddleware(t *testing.T) { | ||
| for _, tc := range []struct { | ||
| name string | ||
| federation bool | ||
| maxTenants int | ||
| header string | ||
| expectedHTTPStatus int | ||
| expectedBodyText string | ||
| }{ | ||
| { | ||
| name: "federation disabled, invalid tenant header", | ||
| federation: false, | ||
| maxTenants: 0, | ||
| header: strings.Repeat("123", tenant.MaxTenantIDLength), | ||
| expectedHTTPStatus: 401, | ||
| expectedBodyText: "tenant ID is too long", | ||
| }, | ||
| { | ||
| name: "federation disabled, single tenant", | ||
| federation: false, | ||
| maxTenants: 0, | ||
| header: "tenant-a", | ||
| expectedHTTPStatus: 200, | ||
| expectedBodyText: "", | ||
| }, | ||
| { | ||
| name: "federation disabled, multiple tenants", | ||
| federation: false, | ||
| maxTenants: 0, | ||
| header: "tenant-a|tenant-b", | ||
| expectedHTTPStatus: 422, | ||
| expectedBodyText: "too many tenant IDs present", | ||
| }, | ||
| { | ||
| name: "federation enabled, invalid tenant header", | ||
| federation: true, | ||
| maxTenants: 0, | ||
| header: strings.Repeat("123", tenant.MaxTenantIDLength), | ||
| expectedHTTPStatus: 401, | ||
| expectedBodyText: "tenant ID is too long", | ||
| }, | ||
| { | ||
| name: "federation enabled, single tenant no limit", | ||
| federation: true, | ||
| maxTenants: 0, | ||
| header: "tenant-a", | ||
| expectedHTTPStatus: 200, | ||
| expectedBodyText: "", | ||
| }, | ||
| { | ||
| name: "federation enabled, multiple tenants no limit", | ||
| federation: true, | ||
| maxTenants: 0, | ||
| header: "tenant-a|tenant-b|tenant-c", | ||
| expectedHTTPStatus: 200, | ||
| expectedBodyText: "", | ||
| }, | ||
| { | ||
| name: "federation enabled, multiple tenants under limit", | ||
| federation: true, | ||
| maxTenants: 2, | ||
| header: "tenant-a|tenant-b", | ||
| expectedHTTPStatus: 200, | ||
| expectedBodyText: "", | ||
| }, | ||
| { | ||
| name: "federation enabled, multiple tenants over limit", | ||
| federation: true, | ||
| maxTenants: 2, | ||
| header: "tenant-a|tenant-b|tenant-c", | ||
| expectedHTTPStatus: 422, | ||
| expectedBodyText: "too many tenant IDs present", | ||
| }, | ||
| } { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| nop := http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) {}) | ||
| // Note that we add the authentication middleware since the tenant validation middleware relies | ||
| // on tenant ID being set in the context associated with the request. | ||
| handler := middleware.Merge(middleware.AuthenticateUser, newTenantValidationMiddleware(tc.federation, tc.maxTenants)).Wrap(nop) | ||
|
|
||
| req := httptest.NewRequest("GET", "/", nil) | ||
| req.Header.Set(user.OrgIDHeaderName, tc.header) | ||
| resp := httptest.NewRecorder() | ||
|
|
||
| handler.ServeHTTP(resp, req) | ||
| body, err := io.ReadAll(resp.Body) | ||
|
|
||
| require.NoError(t, err) | ||
| require.Equal(t, tc.expectedHTTPStatus, resp.Code) | ||
|
|
||
| if tc.expectedBodyText != "" { | ||
| require.Contains(t, string(body), tc.expectedBodyText) | ||
| } | ||
| }) | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.