Skip to content

Commit

Permalink
Merging headers instead of overwriting blindly
Browse files Browse the repository at this point in the history
Adding extra check for header smuggling

Signed-off-by: Danny Kopping <danny.kopping@grafana.com>
  • Loading branch information
Danny Kopping committed Oct 7, 2021
1 parent de2d601 commit e7b5404
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 13 deletions.
12 changes: 9 additions & 3 deletions pkg/ruler/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ func (r *walRegistry) getTenantConfig(tenant string) (instance.Config, error) {

// ensure that no variation of the X-Scope-OrgId header can be added, which might trick authentication
for k, _ := range rwCfg.Client.Headers {
if strings.ToLower(user.OrgIDHeaderName) == strings.ToLower(k) {
if strings.ToLower(user.OrgIDHeaderName) == strings.ToLower(strings.TrimSpace(k)) {
delete(rwCfg.Client.Headers, k)
}
}
Expand Down Expand Up @@ -248,9 +248,15 @@ func (r *walRegistry) getTenantRemoteWriteConfig(tenant string, base RemoteWrite
overrides.Client.RemoteTimeout = model.Duration(v)
}

// TODO: this will override not merge
// merge headers with the base
if v := r.overrides.RulerRemoteWriteHeaders(tenant); len(v) > 0 {
overrides.Client.Headers = v
if overrides.Client.Headers == nil {
overrides.Client.Headers = make(map[string]string, len(v))
}

for k, val := range v {
overrides.Client.Headers[k] = val
}
}

relabelConfigs, err := r.createRelabelConfigs(tenant)
Expand Down
22 changes: 12 additions & 10 deletions pkg/ruler/registry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,11 @@ func newFakeLimits() fakeLimits {
additionalHeadersRWTenant: {
RulerRemoteWriteHeaders: validation.OverwriteMarshalingStringMap{
M: map[string]string{
user.OrgIDHeaderName: "overridden",
strings.ToLower(user.OrgIDHeaderName): "overridden-lower",
strings.ToUpper(user.OrgIDHeaderName): "overridden-upper",
"Additional": "Header",
user.OrgIDHeaderName: "overridden",
fmt.Sprintf(" %s ", user.OrgIDHeaderName): "overridden",
strings.ToLower(user.OrgIDHeaderName): "overridden-lower",
strings.ToUpper(user.OrgIDHeaderName): "overridden-upper",
"Additional": "Header",
},
},
},
Expand All @@ -67,9 +68,8 @@ func newFakeLimits() fakeLimits {
},
},
},
nilRelabelsTenant: {}, // zero value will be nil
nilRelabelsTenant: {},
emptySliceRelabelsTenant: {
// zero value will be nil
RulerRemoteWriteRelabelConfigs: []*util.RelabelConfig{},
},
badRelabelsTenant: {
Expand All @@ -94,6 +94,9 @@ func setupRegistry(t *testing.T, dir string) *walRegistry {
QueueConfig: config.QueueConfig{
Capacity: defaultCapacity,
},
Headers: map[string]string{
"Base": "value",
},
WriteRelabelConfigs: []*relabel.Config{
{
SourceLabels: []model.LabelName{"__name__"},
Expand Down Expand Up @@ -178,9 +181,11 @@ func TestTenantRemoteWriteHeaderOverride(t *testing.T) {
tenantCfg, err := reg.getTenantConfig(additionalHeadersRWTenant)
require.NoError(t, err)

assert.Len(t, tenantCfg.RemoteWrite[0].Headers, 2)
assert.Len(t, tenantCfg.RemoteWrite[0].Headers, 3)
// ensure that tenant cannot override X-Scope-OrgId header
assert.Equal(t, tenantCfg.RemoteWrite[0].Headers[user.OrgIDHeaderName], additionalHeadersRWTenant)
// and that the base header defined is set
assert.Equal(t, tenantCfg.RemoteWrite[0].Headers["Base"], "value")
// but that the additional header defined is set
assert.Equal(t, tenantCfg.RemoteWrite[0].Headers["Additional"], "Header")

Expand Down Expand Up @@ -223,9 +228,6 @@ func TestRelabelConfigOverridesEmptySliceWriteRelabels(t *testing.T) {
reg := setupRegistry(t, walDir)
defer os.RemoveAll(walDir)

tenantCfgNil, err := reg.getTenantConfig(nilRelabelsTenant)
require.NotNil(t, tenantCfgNil)

tenantCfg, err := reg.getTenantConfig(emptySliceRelabelsTenant)
require.NoError(t, err)

Expand Down

0 comments on commit e7b5404

Please sign in to comment.