Add support for hashing time.Time within slices, which unbreaks auditing #6767

ncabatoff · 2019-05-21T15:02:54Z

of requests returning the request counters. Modified HashStructure
signature to make clear that hashWalker is really intended to work on
map[string]interface{}. It may be capable of doing more than that,
but it really doesn't handle arbitrary data.

Also removed some dead code, and got rid of the hashWalker.csData field
since it's the same as the last element of csKey.

of requests returning the request counters. Modified HashStructure signature to make clear that hashWalker is really intended to work on map[string]interface{}. It may be capable of doing more than that, but it really doesn't handle arbitrary data. Also removed some dead code, and got rid of the hashWalker.csData field since it's the same as the last element of csKey.

jefferai · 2019-05-21T15:28:09Z

audit/hashstructure.go

@@ -108,18 +107,18 @@ func Hash(salter *salt.Salt, raw interface{}, nonHMACDataKeys []string) error {
 // the structure. Only _values_ are hashed: keys of objects are not.
 //
 // For the HashCallback, see the built-in HashCallbacks below.
-func HashStructure(s interface{}, cb HashCallback, ignoredKeys []string) (interface{}, error) {
-	s, err := copystructure.Copy(s)
+func HashStructure(s map[string]interface{}, cb HashCallback, ignoredKeys []string) (interface{}, error) {


Please revert this change. The fact that the only things we pass in right now are map[string]interface{} doesn't change the fact that both copystructure nor reflectwalk are able to work just fine on structs, and we may well want to pass in full structs at some future point.

copystructure and reflectwalk work fine on structs, yes. That doesn't hold true for hashWalker.

Currently. It doesn't really bother me either way, it can be changed back, just seems like an unnecessary change.

Yes, totally unnecessary. I felt that without this I'd have to do more work to fill in the gaps, but if you prefer I can leave the signature alone and add a big disclaimer instead.

Either way. Nobody is expecting you to code in support for non-maps. Keep this or leave a comment.

jefferai · 2019-05-21T15:32:12Z

audit/hashstructure.go

-func HashStructure(s interface{}, cb HashCallback, ignoredKeys []string) (interface{}, error) {
-	s, err := copystructure.Copy(s)
+func HashStructure(s map[string]interface{}, cb HashCallback, ignoredKeys []string) (interface{}, error) {
+	scopy, err := copystructure.Copy(s)


What's the reason for this copy? It assumes that you'd never want to modify the actual value coming in. Plus, we are already providing a copy in the first place.

The copy was already there. I wondered myself why we're copying twice. The reason I introduced a new variable here was a side-effect of the signature change.

Sure, it's already there -- the question is, what's the reason for it :-)

It seems like either we should figure out if we're copying in all cases already and remove this, or remove the places where we're already copying.

Sounds good. I'm thinking I'll remove the copying from HashStructure and FormatRequest/FormatResponse, instead doing it in Hash. Rationale: HashStructure is lower level, maybe someday a caller will come along that doesn't need/want copying and this provides that option. Meanwhile FormatRequest and FormatResponse are already doing a lot, and this detail feels like it should be Hash's responsibility.

Sounds reasonable.

jefferai · 2019-05-21T15:37:10Z

audit/hashstructure.go

+	// Second to last element of w.loc is location that contains this struct.
+	switch w.loc[len(w.loc)-2] {
+	case reflectwalk.MapKey:
+		return errors.New("time.Time value in a map key cannot be hashed for audits")


I assume the behavior generally is we only hash values, not keys. In which case throwing an error here seems overkill...why not just not hash it but not throw an error and then when it's marshaled it'll become a string key value?

Sure, I was trying to maintain the existing behaviour except where it had to change to fix the error, but I can let it through. But then the question may arise "why are you only hashing some time.Time values and not others?" Suppose for example someday someone puts a birthdate (PII) in a map key, that could be a problem.

Suppose for example someday someone puts a birthdate (PII) in a map key, that could be a problem.

Sure -- but my understanding is if it had been preconverted to a string for the key, we'd let it through, because we only hash values. If I'm wrong and we hash keys already, then I guess we should hash it.

Sure, I was trying to maintain the existing behaviour

So do we already throw errors for other types that appear in keys? I am assuming not since much of what comes in is a map[string]interface{}

You're right, we don't hash keys already. I'm perhaps being over-paranoid about failing to hash something we should.

So do we already throw errors for other types that appear in keys? I am assuming not since much of what comes in is a map[string]interface{}

No, I simply meant that right now we already return an error if there's a time in a map key, so it seemed reasonable to maintain that. I'm okay with letting it through unhashed instead, will try that.

Move all the copying/hashing logic from FormatRequest/FormatResponse into the new Hash* funcs. Be a bit more selective about where we use copystructure. HashStructure now modifies in place instead of copying.

time.Time, ignore them, i.e. pass them through unhashed.

audit backends. If they do, they're responsible for setting it up.

part of the merge I had to resolve an impedance mismatch between the new notion of allowing OptMarshaler on a per-data-key basis, and the existing expectation that hashWalker will be invoked on the top-level data map. # Conflicts: # audit/format.go

It turns out that hashWalker can't handle be made to work with raw primitive values, because there's no reflect mechanism to write them in place. The reason we were trying to do that is because we now need to handle the top-level data map ourselves, in order to handle the case where any top-level fields are OptMarshalers. The good news is that we now do a bit less copying as a result. The bad news is that there's some duplication between hashWalker and hashMap.

ncabatoff · 2019-06-03T14:24:17Z

@jefferai Could you have a look at the revised hashMap in hashstructure.go please? I had to duplicate some of what hashwalker does because I couldn't find a good way to make it handle OptMarshaler, and because hashwalker's approach of in-place modification precludes it working with primitives as direct inputs.

jefferai · 2019-06-03T20:26:53Z

audit/hashstructure.go

+		} else if strutil.StrListContains(nonHMACDataKeys, k) {
+			newData[k] = v
+		} else if t, ok := v.(time.Time); ok {
+			newData[k] = t.Format(time.RFC3339Nano)


You don't need to call this specifically as marshaling JSON will automatically format time.Time as RFC3339Nano.

It might be better to do this in a type switch where the StrListContains case is moved down into default with what else is in the else block. Either way works though.

…github.com-ncabatoff/hashicorp/vault into fix-auditing-counter-requests

ncabatoff · 2019-06-17T19:29:11Z

Compared audit log with master build for vault kv put action and verified that the same fields get HMAC'd after these changes. This PR is ready to be reviewed again.

# Conflicts: # go.sum

audit/hashstructure.go

jefferai

I think it's looking pretty good, but would like some other reviews

audit/hashstructure.go

…-counter-requests

jefferai reviewed May 21, 2019

View reviewed changes

ncabatoff and others added 8 commits May 21, 2019 15:23

Break Hash into struct-specific func like HashAuth, HashRequest.

dd3c221

Move all the copying/hashing logic from FormatRequest/FormatResponse into the new Hash* funcs. Be a bit more selective about where we use copystructure. HashStructure now modifies in place instead of copying.

Instead of returning an error when trying to hash map keys of type

e0dce92

time.Time, ignore them, i.e. pass them through unhashed.

Enable auditing on test clusters by default.

ada8200

Only enable auditing automatically if the caller didn't specify any

d86e6b6

audit backends. If they do, they're responsible for setting it up.

Fix nil panic when no base provided.

f6490f4

Merge branch 'master' into fix-auditing-counter-requests

90ba51e

jefferai reviewed Jun 3, 2019

View reviewed changes

ncabatoff added 6 commits June 3, 2019 17:07

Revert unnecessary change to how we hash data maps.

a4437d7

noop auditing

abdbdc8

Cleanup sdk module now that I've removed the audit file backend dep.

77354c3

Cleanup api module now that I've removed the audit file backend dep.

7562f0f

Merge branches 'fix-auditing-counter-requests' and 'master' of ssh://…

c8ec52c

…github.com-ncabatoff/hashicorp/vault into fix-auditing-counter-requests

Fix users of testCoreConfig.

d1ae223

Merge branch 'master' into fix-auditing-counter-requests

9fa7da3

# Conflicts: # go.sum

tyrannosaurus-becks reviewed Jul 2, 2019

View reviewed changes

audit/hashstructure.go Show resolved Hide resolved

tyrannosaurus-becks previously approved these changes Jul 2, 2019

View reviewed changes

Add godoc for AuditFormatWriter methods.

4b31235

ncabatoff dismissed tyrannosaurus-becks’s stale review via 4b31235 July 2, 2019 19:29

jefferai reviewed Jul 2, 2019

View reviewed changes

mgaffney reviewed Jul 2, 2019

View reviewed changes

audit/hashstructure.go Outdated Show resolved Hide resolved

tyrannosaurus-becks previously approved these changes Jul 2, 2019

View reviewed changes

Update godoc.

aa85762

ncabatoff dismissed tyrannosaurus-becks’s stale review via aa85762 July 2, 2019 21:24

ncabatoff added this to the 1.2 milestone Jul 2, 2019

ncabatoff added beta and removed beta labels Jul 2, 2019

mgaffney approved these changes Jul 2, 2019

View reviewed changes

tyrannosaurus-becks approved these changes Jul 2, 2019

View reviewed changes

Merge branch 'master' of github.com/hashicorp/vault into fix-auditing…

d92c798

…-counter-requests

ncabatoff merged commit 056e90b into master Jul 2, 2019

ncabatoff deleted the fix-auditing-counter-requests branch July 23, 2019 19:12

jefferai mentioned this pull request Aug 1, 2019

time.Time value in a non map key cannot be hashed for audits #7228

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for hashing time.Time within slices, which unbreaks auditing #6767

Add support for hashing time.Time within slices, which unbreaks auditing #6767

ncabatoff commented May 21, 2019

jefferai May 21, 2019 •

edited

Loading

ncabatoff May 21, 2019

jefferai May 21, 2019

ncabatoff May 21, 2019

jefferai May 21, 2019

jefferai May 21, 2019

ncabatoff May 21, 2019

jefferai May 21, 2019

ncabatoff May 21, 2019

jefferai May 21, 2019

jefferai May 21, 2019

ncabatoff May 21, 2019

jefferai May 21, 2019

ncabatoff May 21, 2019

ncabatoff commented Jun 3, 2019

jefferai Jun 3, 2019

ncabatoff commented Jun 17, 2019

jefferai left a comment

Add support for hashing time.Time within slices, which unbreaks auditing #6767

Add support for hashing time.Time within slices, which unbreaks auditing #6767

Conversation

ncabatoff commented May 21, 2019

jefferai May 21, 2019 • 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

ncabatoff commented Jun 3, 2019

Choose a reason for hiding this comment

ncabatoff commented Jun 17, 2019

jefferai left a comment

Choose a reason for hiding this comment

jefferai May 21, 2019 •

edited

Loading