Skip to content
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.

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

feat: add plugin metadata to audit logging #19814

Merged
merged 10 commits into from
Apr 6, 2023

Conversation

thyton
Copy link
Contributor

@thyton thyton commented Mar 29, 2023

Add plugin metadata (name, type, version, sha256, whether the plugin is running externally) to audit logging

@thyton thyton force-pushed the VAULT-11536-add-plugin-metadata-to-audit-logging branch 2 times, most recently from fee59cf to bbac92c Compare March 29, 2023 23:37
@thyton thyton added this to the 1.13.2 milestone Mar 30, 2023
@thyton thyton requested review from a team and yhyakuna as code owners March 30, 2023 16:32
@thyton thyton force-pushed the VAULT-11536-add-plugin-metadata-to-audit-logging branch 2 times, most recently from c3ff2e9 to 2206548 Compare March 30, 2023 17:17
@thyton thyton changed the title feat: add plugin name, type, version, and sha256 to audit logging feat: add plugin metadata to audit logging Mar 30, 2023
Copy link
Contributor

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking pretty good to me. It would be great to get some tests. There is an example of a test that enables audit logging to a file here that you could use as a basis:

tempDir := t.TempDir()
f, err := os.CreateTemp(tempDir, "")
if err != nil {
t.Fatal(err)
}
// Enable audit logging.
req := logical.TestRequest(t, logical.UpdateOperation, "audit/file")
req.Data = map[string]any{
"type": "file",
"options": map[string]any{
"file_path": f.Name(),
},
}

audit/format.go Outdated Show resolved Hide resolved
vault/mount.go Outdated Show resolved Hide resolved
audit/format.go Outdated Show resolved Hide resolved
Copy link
Contributor

@swenson swenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Echoing what Tom said, adding some tests would be great.

sdk/logical/request.go Outdated Show resolved Hide resolved
vault/mount.go Outdated Show resolved Hide resolved
Copy link
Contributor

@swenson swenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Just a question and a few small nits.

audit/format.go Outdated

// getMountClass returns the mount class based the mount accessor of a logical.Request.
func getMountClass(req *logical.Request) string {
if req.MountAccessor == "" || strings.HasPrefix(req.Path, "sys/") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work if the request is under a namespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this works if the request is under a namespace since req.Path has the namespace trimmed further up in the call stack. For example, in buildLogicalRequestNoAuth() that constructs logical.Request , logical.Request.Path is made from namespace-trimmed http.Request.URL.Path https://github.com/hashicorp/vault/blob/main/http/logical.go#L53.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to use add MountClass() to MountEntry instead of using logical.Request.Path as MountEntry.Path doesn't have a namespace.

// APIPath returns the full API Path for the given mount entry
func (e *MountEntry) APIPath() string {
	path := e.Path
	if e.Table == credentialTableType {
		path = credentialRoutePrefix + path
	}
	return e.namespace.Path + path
}

// APIPathNoNamespace returns the API Path without the namespace for the given mount entry
func (e *MountEntry) APIPathNoNamespace() string {
	path := e.Path
	if e.Table == credentialTableType {
		path = credentialRoutePrefix + path
	}
	return path
}

vault/auth.go Outdated Show resolved Hide resolved
vault/external_tests/plugin/external_plugin_test.go Outdated Show resolved Hide resolved
@thyton thyton force-pushed the VAULT-11536-add-plugin-metadata-to-audit-logging branch from e67dfff to a0891d2 Compare April 4, 2023 15:38
Copy link
Contributor

@swenson swenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I pulled this down and tested it locally with namespaces as well, and it seems to work,

e.g.,

$ VAULT_NAMESPACE=testns vault write auth/approle/role/example-dot-com ...
...
  "request": {
    "id": "9bd6a564-6046-6e16-5ccb-5b40dbdad29d",
    "client_id": "0DHqvq2D77kL2/JTPSZkTMJbkFVmUu0TzMi0jiXcFy8=",
    "operation": "create",
    "mount_type": "approle",
    "mount_accessor": "auth_approle_316cf542",
    "mount_running_version": "v1.14.0+builtin.vault",
    "mount_class": "auth",
    "client_token": "hmac-sha256:70849cbddaebad80649e4d1845fccfd2c4a12694b590a627d6d12d21ec34a5b4",
    "client_token_accessor": "hmac-sha256:6bdd69aa740335b8a0d2b9588c60f79b7907c51091301d6a5f0d5289ed243e59",
    "namespace": {
      "id": "x3PCA",
      "path": "testns/"
    },
...
  "response": {
    "mount_type": "approle",
    "mount_accessor": "auth_approle_316cf542",
    "mount_running_plugin_version": "v1.14.0+builtin.vault",
    "mount_class": "auth"
  }    

Copy link
Contributor

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice tests! 👍

@thyton thyton merged commit 0e9b3b0 into main Apr 6, 2023
@thyton thyton deleted the VAULT-11536-add-plugin-metadata-to-audit-logging branch April 6, 2023 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants