Configure the request headers that are output to the audit log #2321

briankassouf · 2017-02-01T22:00:40Z

Adds a header object to the audit log's request object:

"request": {
        "client_token": "",
        "data": null,
        "operation": "read",
        "path": "auth/token/lookup-self",
        "remote_address": "192.168.5.5",
        "headers": {
           "X-Forwarded-For: "127.0.0.1",
           "Content-Type": "application/json"
         }
  },

implements #1451

…that will be audited

jefferai · 2017-02-02T00:37:40Z

vault/audited_headers.go

+	// Key used in the BarrierView to store and retrieve the header config
+	auditedHeadersEntry = "audited_headers"
+	// Path used to create a sub view off of BarrierView
+	auditedHeadersSubPath = "auditedHeadersConfig/"


Minor nitpick but we don't usually camelcase paths in the data store, but rather use hyphens. So it'd be sys/audited-headers-config/audited-headers.

jefferai · 2017-02-02T00:42:15Z

vault/audited_headers.go

+	a.RLock()
+	defer a.RUnlock()
+
+	result = make(map[string][]string)


You can make this a tad faster by using a second argument to make and use the length of the existing map, so in this case len(a.Headers) would be a hard upper bound -- might be a bit overallocated but better than a second allocation.

vishalnayak · 2017-02-02T06:03:37Z

vault/audited_headers.go

+// AuditedHeadersConfig is used by the Audit Broker to write only approved
+// headers to the audit logs. It uses a BarrierView to persist the settings.
+type AuditedHeadersConfig struct {
+	Headers map[string]*auditedHeaderSettings


Can we rename this to HeaderSettings or something, to better indicate what this actually holds?

vishalnayak · 2017-02-02T06:04:45Z

vault/audited_headers.go

+func (a *AuditedHeadersConfig) add(header string, hmac bool) error {
+	a.Lock()
+	defer a.Unlock()
+


Can we check for header being ""?

vishalnayak · 2017-02-02T06:10:32Z

vault/audited_headers.go

+func (a *AuditedHeadersConfig) ApplyConfig(headers map[string][]string, hashFunc func(string) string) (result map[string][]string) {
+	a.RLock()
+	defer a.RUnlock()
+


Can we add more comments in this function?

vishalnayak · 2017-02-02T06:18:40Z

vault/audited_headers.go

+}
+
+// add adds or overwrites a header in the config and updates the barrier view
+func (a *AuditedHeadersConfig) add(header string, hmac bool) error {


[Optional] Since there is only one setting and since this is an internal implementation, I guess this looks okay. I was wondering if this should take in *auditedHeaderSettings.

vishalnayak · 2017-02-02T06:24:40Z

vault/audited_headers_test.go

+		t.Fatalf("Error decoding header view: %s", err)
+	}
+
+	expected["X-Vault-Header"] = &auditedHeaderSettings{


Correct me if I am missing something here. Shouldn't we expect two header entries at this point? One for X-Test-Header and one for X-Vault-Header?

Correct! This line is adding a "X-Vault-Header" key to the existing expected map

vishalnayak · 2017-02-02T06:31:22Z

vault/audited_headers_test.go

+
+}
+
+func BenchmarkAuditedHeaderConfig_ApplyConfig(b *testing.B) {


This is super nice! I guess we should adopt more of such functions while writing tests. 👍

👍 It's nice to know how much time we could potentially be adding to each request

vishalnayak · 2017-02-02T06:41:11Z

vault/logical_system.go

+		return logical.ErrorResponse("missing header name"), nil
+	}
+
+	headerConfig.add(header, hmac)


Can we handle the error responded by add here?

vishalnayak · 2017-02-02T06:41:27Z

vault/logical_system.go

+	if header == "" {
+		return logical.ErrorResponse("missing header name"), nil
+	}
+	headerConfig.remove(header)


Can we handle the return values of remove?

vishalnayak · 2017-02-02T06:43:37Z

vault/logical_system.go

+
+// handleAuditedHeaderRead returns the header configuration for the given header name
+func (b *SystemBackend) handleAuditedHeaderRead(req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
+	headerConfig := b.Core.AuditedHeadersConfig()


In all the functions, this can be moved after the input validation.

jefferai · 2017-02-02T14:20:10Z

vault/logical_system.go

@@ -621,6 +622,38 @@ func NewSystemBackend(core *Core, config *logical.BackendConfig) (logical.Backen
 				HelpSynopsis:    strings.TrimSpace(sysHelp["rewrap"][0]),
 				HelpDescription: strings.TrimSpace(sysHelp["rewrap"][1]),
 			},
+
+			&framework.Path{
+				Pattern: "config/audited-headers/(?P<header>.+)",


Thinking about this, I wonder if we should actually have any public-facing parts of this refer to auditing configuration at a top-level and header configuration at a lower level. So this would become config/auditing/headers (or maybe config/auditing/request-headers to be explicit). That way if we want to add any other auditing-related configuration at a later time we don't need a totally separate path. (For instance, if we want to configure the system default for HMAC-ing accessors.)

jefferai · 2017-02-02T14:21:35Z

vault/logical_system.go

@@ -40,6 +40,7 @@ func NewSystemBackend(core *Core, config *logical.BackendConfig) (logical.Backen
 				"audit/*",
 				"raw/*",
 				"rotate",
+				"config/*",


I wonder if this really needs sudo. e.g. for config/cors it probably won't be needed. Maybe just sudo on the relevant auditing paths under config/?

jefferai

There's one test to fix (root paths), other than that LGTM

vishalnayak

LGTM!

Brian Kassouf added 11 commits January 27, 2017 17:12

Add /sys/config/audited-headers endpoint for configuring the headers …

d917c5c

…that will be audited

Remove some debug lines

e3dba9a

Add a persistant layer and refactor a bit

ad212d3

update the api endpoints to be more restful

365d24c

Add comments and clean up a few functions

209eba9

Remove unneeded hash structure functionaility

bde581a

Fix existing tests

3da24ef

Add tests

78ec29f

Add test for Applying the header config

b336bec

Add Benchmark for the ApplyConfig method

e2987ea

ResetTimer on the benchmark:

29ba94a

briankassouf added this to the 0.6.5 milestone Feb 1, 2017

Brian Kassouf added 2 commits February 1, 2017 14:15

Update the headers comment

6c50928

Add test for audit broker

f6b153f

jefferai reviewed Feb 2, 2017

View reviewed changes

Brian Kassouf added 3 commits February 1, 2017 16:44

Use hyphens instead of camel case

3106030

Add size paramater to the allocation of the result map

1a3b64a

Fix the tests for the audit broker

1e3494f

vishalnayak reviewed Feb 2, 2017

View reviewed changes

PR feedback

31ddc27

jefferai reviewed Feb 2, 2017

View reviewed changes

update the path and permissions on config/* paths

fe455bc

jefferai approved these changes Feb 2, 2017

View reviewed changes

Brian Kassouf added 2 commits February 2, 2017 11:12

Add docs file

35bf200

Fix TestSystemBackend_RootPaths test

f02177a

vishalnayak approved these changes Feb 2, 2017

View reviewed changes

briankassouf merged commit 590b568 into master Feb 2, 2017

briankassouf deleted the audit-headers branch February 2, 2017 19:49

stevenscg mentioned this pull request Feb 16, 2017

feature request: Allow user-supplied request_id as HTTP header #2382

Closed

stevenscg mentioned this pull request Mar 24, 2017

Feature request: Add metadata to vault client requests hashicorp/nomad#2475

Closed

jantman mentioned this pull request Jan 17, 2018

PROXY Protocol v2 logging support #3807

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configure the request headers that are output to the audit log #2321

Configure the request headers that are output to the audit log #2321

briankassouf commented Feb 1, 2017 •

edited

Loading

jefferai Feb 2, 2017 •

edited

Loading

jefferai Feb 2, 2017

vishalnayak Feb 2, 2017

vishalnayak Feb 2, 2017

vishalnayak Feb 2, 2017

vishalnayak Feb 2, 2017

vishalnayak Feb 2, 2017

briankassouf Feb 2, 2017

vishalnayak Feb 2, 2017

briankassouf Feb 2, 2017

vishalnayak Feb 2, 2017

vishalnayak Feb 2, 2017

vishalnayak Feb 2, 2017

jefferai Feb 2, 2017

jefferai Feb 2, 2017

jefferai left a comment

vishalnayak left a comment


		}

		func BenchmarkAuditedHeaderConfig_ApplyConfig(b *testing.B) {

Configure the request headers that are output to the audit log #2321

Configure the request headers that are output to the audit log #2321

Conversation

briankassouf commented Feb 1, 2017 • edited Loading

jefferai Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jefferai left a comment

Choose a reason for hiding this comment

vishalnayak left a comment

Choose a reason for hiding this comment

briankassouf commented Feb 1, 2017 •

edited

Loading

jefferai Feb 2, 2017 •

edited

Loading