fix(audit): timestamp field to first in log

[saipranav]

Description

PR modifies the struct of entry in audit log event and makes time as first field so after encoding / marshalling into json, time still stays as first field.

PR fixes #31435

TODO only if you're a HashiCorp employee

• Backport Labels: If this fix needs to be backported, use the appropriate backport/ label that matches the desired release branch. Note that in the CE repo, the latest release branch will look like backport/x.x.x, but older release branches will be backport/ent/x.x.x+ent.
  • LTS: If this fixes a critical security vulnerability or severity 1 bug, it will also need to be backported to the current LTS versions of Vault. To ensure this, use all available enterprise labels.
• ENT Breakage: If this PR either 1) removes a public function OR 2) changes the signature
  of a public function, even if that change is in a CE file, double check that
  applying the patch for this PR to the ENT repo and running tests doesn't
  break any tests. Sometimes ENT only tests rely on public functions in CE
  files.
• Jira: If this change has an associated Jira, it's referenced either
  in the PR description, commit message, or branch name.
• RFC: If this change has an associated RFC, please link it in the description.
• ENT PR: If this change has an associated ENT PR, please link it in the
  description. Also, make sure the changelog is in this PR, not in your ENT PR.

PCI review checklist

• I have documented a clear reason for, and description of, the change I am making.
• If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
• If applicable, I've documented the impact of any changes to security controls.

Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.

[vercel]

@saipranav is attempting to deploy a commit to the HashiCorp Team on Vercel.

A member of the Team first needs to authorize it.

[aslamovamir]

Suggested change

	audit: Log entry has time as first field. Useful for log parsers, forwarders for accurate timestamps of events.
	audit: Log entry fields are reorganized so time is the first field, which is useful for log parsers and forwarders for accurate timestamps of events.

[aslamovamir]

Suggested change

	```release-note:bug
	```release-note:improvement

Is it better to say it is an improvement?

[VioletHynes]

I think this would need to be called out as a change, since this might have breaking impact for some customers

[aslamovamir]

That's a good point!

[saipranav]

I am actually wondering how other customers and even enterprise customers are not affected before by this audit log change?
Most of customers should have been using default settings in log parsers, this change was probably introduced in 1.16.x for audit logging refactor.

[saipranav]

I assume, change can be merged even for minor releases and not just for major releases. Is my assumption correct?

[VioletHynes]

If we did want to merge this (I'm not yet sure if we do, due to breakage considerations), then this would have to be a major release. We don't release breaking changes in minor releases

[saipranav]

Understood. It is just that we need this badly for upgrading our enterprise servers and we prefer to do it quickly.

Nice idea : if feasible if you can ask enterprise customers if they are affected by the previous change and log parse of event timestamps.

I would call this one a fix or improvement, but I will leave it to you as you all know the system better.

[aslamovamir]

Left a minor suggestion, but looks good otherwise

[aslamovamir]

I was advised that this can break things for others so we need to consult more people.

[saipranav]

adding @peteski22 for additional review, since he has refactored the file and for sure knows the system better than me

[saipranav]

@VioletHynes @aslamovamir
Hi all, updated changelog to make it as change instead of bug or improvement.

[peteski22]

Apologies @saipranav for the delay in replying, I completely missed being tagged in this.

The JSON spec (both 2014 and 2017 versions) specifies that keys (fields) in the object (struct) are unordered:

2014: https://www.rfc-editor.org/rfc/rfc7159.html#section-1

2017: https://datatracker.ietf.org/doc/html/rfc8259#section-1

An object is an unordered collection of zero or more name/value pairs, where a name is a string and a value is a string, number, boolean, null, object, or array.

The Go library encoding/json follows the 2014 spec, with encoding/json/v2 following the 2017 spec. However, as described by the intention of the PR:

In v1, a Go map is marshalled in a deterministic order. In contrast, in v2 a Go map is marshalled in a non-deterministic order. The jsonv2.Deterministic option controls this behaviour difference.

Whilst the Go v1 JSON encoder (which is being used in Vault) does ensure that the order of the fields is maintained when serialized, any JSON consuming services should be written against the spec, not a Go (or similar) specific implementation.

I'd question that this is actually a 'bug' in Vault (I think @VioletHynes already mentioned changing this), as despite the changes around 1.15.x when audit work was on-going, JSON spec wasn't violated. More that Splunk (itself, or due to customer configuration) is encountering issues dealing with the JSON and time field.

I'm not a Splunk admin/operator so I'm not 100% on this, but could the issue be resolved by configuring Splunk props.conf - Structured Data Header Extraction and configuration to handle JSON and the timestamp field (time) explicitly?

[_json]
INDEXED_EXTRACTIONS = json
KV_MODE = json
TIMESTAMP_FIELDS = time

This is something I'd look into as a solution for any customers that are reporting this issue as it may resolve it, or at least be a workaround until a newer version of Vault containing this PR is released and can be deployed in the customer's environments.

Whilst I'm no longer at HashiCorp, I don't really see a problem with the intent of the change 'as-is'. I don't think I'd consider it a breaking change for existing users, as all it is doing is allowing Splunk customers to handle the audit log entries, any consumer of the log files that actually follows the JSON spec is not impacted.

I'd recommend for HashiCorp reviewers to 'request changes' to the current PR however, to address the following:

• Adding some more context to the actual struct/field to explain why time needs to be the first field in the struct (e.g. in a comment)
• Adding unit tests to ensure there's no regression at a later date

Hope this helps. 😄