Skip to content

Validate tenantID on single resolver #6727

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Validate tenantID on single resolver #6727

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

## master / unreleased
* [CHANGE] StoreGateway/Alertmanager: Add default 5s connection timeout on client. #6603
* [CHANGE] Validate a tenantID when to use a single tenant resolver. #6727
* [FEATURE] Query Frontend: Add dynamic interval size for query splitting. This is enabled by configuring experimental flags `querier.max-shards-per-query` and/or `querier.max-fetched-data-duration-per-query`. The split interval size is dynamically increased to maintain a number of shards and total duration fetched below the configured values. #6458
* [FEATURE] Querier/Ruler: Add `query_partial_data` and `rules_partial_data` limits to allow queries/rules to be evaluated with data from a single zone, if other zones are not available. #6526
* [FEATURE] Update prometheus alertmanager version to v0.28.0 and add new integration msteamsv2, jira, and rocketchat. #6590
Expand Down
16 changes: 3 additions & 13 deletions pkg/tenant/resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package tenant

import (
"context"
"errors"
"net/http"
"strings"

Expand Down Expand Up @@ -67,24 +66,18 @@ type SingleResolver struct {
// `\` can be found.
func containsUnsafePathSegments(id string) bool {
// handle the relative reference to current and parent path.
if id == "." || id == ".." {
return true
}

return strings.ContainsAny(id, "\\/")
return id == "." || id == ".."
}

var errInvalidTenantID = errors.New("invalid tenant ID")

func (t *SingleResolver) TenantID(ctx context.Context) (string, error) {
//lint:ignore faillint wrapper around upstream method
id, err := user.ExtractOrgID(ctx)
if err != nil {
return "", err
}

if containsUnsafePathSegments(id) {
return "", errInvalidTenantID
if err := ValidTenantID(id); err != nil {
return "", err
}

return id, nil
Expand Down Expand Up @@ -134,9 +127,6 @@ func (t *MultiResolver) TenantIDs(ctx context.Context) ([]string, error) {
if err := ValidTenantID(orgID); err != nil {
return nil, err
}
if containsUnsafePathSegments(orgID) {
return nil, errInvalidTenantID
}
}

return NormalizeTenantIDs(orgIDs), nil
Expand Down
70 changes: 38 additions & 32 deletions pkg/tenant/resolver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,37 +67,49 @@ var commonResolverTestCases = []resolverTestCase{
{
name: "parent-dir",
headerValue: strptr(".."),
errTenantID: errInvalidTenantID,
errTenantIDs: errInvalidTenantID,
errTenantID: errTenantIDUnsafe,
errTenantIDs: errTenantIDUnsafe,
},
{
name: "current-dir",
headerValue: strptr("."),
errTenantID: errInvalidTenantID,
errTenantIDs: errInvalidTenantID,
errTenantID: errTenantIDUnsafe,
errTenantIDs: errTenantIDUnsafe,
},
{
name: "white space",
headerValue: strptr(" "),
errTenantID: &errTenantIDUnsupportedCharacter{pos: 0, tenantID: " "},
errTenantIDs: &errTenantIDUnsupportedCharacter{pos: 0, tenantID: " "},
},
{
name: "containing-forward-slash",
headerValue: strptr("forward/slash"),
errTenantID: &errTenantIDUnsupportedCharacter{pos: 7, tenantID: "forward/slash"},
errTenantIDs: &errTenantIDUnsupportedCharacter{pos: 7, tenantID: "forward/slash"},
},
{
name: "containing-backward-slash",
headerValue: strptr(`backward\slash`),
errTenantID: &errTenantIDUnsupportedCharacter{pos: 8, tenantID: "backward\\slash"},
errTenantIDs: &errTenantIDUnsupportedCharacter{pos: 8, tenantID: "backward\\slash"},
},
}

func TestSingleResolver(t *testing.T) {
r := NewSingleResolver()
for _, tc := range append(commonResolverTestCases, []resolverTestCase{
{
name: "multi-tenant",
headerValue: strptr("tenant-a|tenant-b"),
tenantID: "tenant-a|tenant-b",
tenantIDs: []string{"tenant-a|tenant-b"},
},
{
name: "containing-forward-slash",
headerValue: strptr("forward/slash"),
errTenantID: errInvalidTenantID,
errTenantIDs: errInvalidTenantID,
name: "multi-tenant",
headerValue: strptr("tenant-a|tenant-b"),
errTenantID: &errTenantIDUnsupportedCharacter{pos: 8, tenantID: "tenant-a|tenant-b"},
errTenantIDs: &errTenantIDUnsupportedCharacter{pos: 8, tenantID: "tenant-a|tenant-b"},
},
{
name: "containing-backward-slash",
headerValue: strptr(`backward\slash`),
errTenantID: errInvalidTenantID,
errTenantIDs: errInvalidTenantID,
name: "containing-invalid-character",
headerValue: strptr("+"),
errTenantID: &errTenantIDUnsupportedCharacter{pos: 0, tenantID: "+"},
errTenantIDs: &errTenantIDUnsupportedCharacter{pos: 0, tenantID: "+"},
},
}...) {
t.Run(tc.name, tc.test(r))
Expand Down Expand Up @@ -126,22 +138,16 @@ func TestMultiResolver(t *testing.T) {
tenantIDs: []string{"tenant-a", "tenant-b"},
},
{
name: "multi-tenant-with-relative-path",
headerValue: strptr("tenant-a|tenant-b|.."),
errTenantID: errInvalidTenantID,
errTenantIDs: errInvalidTenantID,
name: "multi-tenant-with-unsafe-path-segment(.)",
headerValue: strptr("tenant-a|tenant-b|."),
errTenantID: errTenantIDUnsafe,
errTenantIDs: errTenantIDUnsafe,
},
{
name: "containing-forward-slash",
headerValue: strptr("forward/slash"),
errTenantID: &errTenantIDUnsupportedCharacter{pos: 7, tenantID: "forward/slash"},
errTenantIDs: &errTenantIDUnsupportedCharacter{pos: 7, tenantID: "forward/slash"},
},
{
name: "containing-backward-slash",
headerValue: strptr(`backward\slash`),
errTenantID: &errTenantIDUnsupportedCharacter{pos: 8, tenantID: "backward\\slash"},
errTenantIDs: &errTenantIDUnsupportedCharacter{pos: 8, tenantID: "backward\\slash"},
name: "multi-tenant-with-unsafe-path-segment(..)",
headerValue: strptr("tenant-a|tenant-b|.."),
errTenantID: errTenantIDUnsafe,
errTenantIDs: errTenantIDUnsafe,
},
}...) {
t.Run(tc.name, tc.test(r))
Expand Down
5 changes: 5 additions & 0 deletions pkg/tenant/tenant.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (

var (
errTenantIDTooLong = errors.New("tenant ID is too long: max 150 characters")
errTenantIDUnsafe = errors.New("tenant ID is '.' or '..'")
)

type errTenantIDUnsupportedCharacter struct {
Expand Down Expand Up @@ -65,6 +66,10 @@ func ValidTenantID(s string) error {
return errTenantIDTooLong
}

if containsUnsafePathSegments(s) {
return errTenantIDUnsafe
}

return nil
}

Expand Down
8 changes: 8 additions & 0 deletions pkg/tenant/tenant_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,14 @@ func TestValidTenantIDs(t *testing.T) {
name: strings.Repeat("a", 151),
err: strptr("tenant ID is too long: max 150 characters"),
},
{
name: ".",
err: strptr("tenant ID is '.' or '..'"),
},
{
name: "..",
err: strptr("tenant ID is '.' or '..'"),
},
} {
t.Run(tc.name, func(t *testing.T) {
err := ValidTenantID(tc.name)
Expand Down
Loading