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

Add remote_port in the audit logs when it is available #12790

Merged
merged 5 commits into from Jan 26, 2022

Conversation

remilapeyre
Copy link
Contributor

The request.remote_port field is now present in the audit log when it
is available:

{
  "time": "2021-10-10T13:53:51.760039Z",
  "type": "response",
  "auth": {
    "client_token": "hmac-sha256:1304aab0ac65747684e1b58248cc16715fa8f558f8d27e90fcbcb213220c0edf",
    "accessor": "hmac-sha256:f8cf0601dadd19aac84f205ded44c62898e3746a42108a51105a92ccc39baa43",
    "display_name": "root",
    "policies": [
      "root"
    ],
    "token_policies": [
      "root"
    ],
    "token_type": "service",
    "token_issue_time": "2021-10-10T15:53:44+02:00"
  },
  "request": {
    "id": "829c04a1-0352-2d9d-9bc9-00b928d33df5",
    "operation": "update",
    "mount_type": "system",
    "client_token": "hmac-sha256:1304aab0ac65747684e1b58248cc16715fa8f558f8d27e90fcbcb213220c0edf",
    "client_token_accessor": "hmac-sha256:f8cf0601dadd19aac84f205ded44c62898e3746a42108a51105a92ccc39baa43",
    "namespace": {
      "id": "root"
    },
    "path": "sys/audit/file",
    "data": {
      "description": "hmac-sha256:321a1d105f8c6fd62be4f34c4da4f0e6d1cdee9eb2ff4af0b59e1410950fe86b",
      "local": false,
      "options": {
        "file_path": "hmac-sha256:2421b5bf8dab1f9775b2e6e66e58d7bca99ab729f3f311782fda50717eee55b3"
      },
      "type": "hmac-sha256:30dff9607b4087e3ae6808b4a3aa395b1fc064e467748c55c25ddf0e9b150fcc"
    },
    "remote_address": "127.0.0.1",
    "remote_port": 54798
  },
  "response": {
    "mount_type": "system"
  }
}

Closes #7716

The `request.remote_port` field is now present in the audit log when it
is available:

```
{
  "time": "2021-10-10T13:53:51.760039Z",
  "type": "response",
  "auth": {
    "client_token": "hmac-sha256:1304aab0ac65747684e1b58248cc16715fa8f558f8d27e90fcbcb213220c0edf",
    "accessor": "hmac-sha256:f8cf0601dadd19aac84f205ded44c62898e3746a42108a51105a92ccc39baa43",
    "display_name": "root",
    "policies": [
      "root"
    ],
    "token_policies": [
      "root"
    ],
    "token_type": "service",
    "token_issue_time": "2021-10-10T15:53:44+02:00"
  },
  "request": {
    "id": "829c04a1-0352-2d9d-9bc9-00b928d33df5",
    "operation": "update",
    "mount_type": "system",
    "client_token": "hmac-sha256:1304aab0ac65747684e1b58248cc16715fa8f558f8d27e90fcbcb213220c0edf",
    "client_token_accessor": "hmac-sha256:f8cf0601dadd19aac84f205ded44c62898e3746a42108a51105a92ccc39baa43",
    "namespace": {
      "id": "root"
    },
    "path": "sys/audit/file",
    "data": {
      "description": "hmac-sha256:321a1d105f8c6fd62be4f34c4da4f0e6d1cdee9eb2ff4af0b59e1410950fe86b",
      "local": false,
      "options": {
        "file_path": "hmac-sha256:2421b5bf8dab1f9775b2e6e66e58d7bca99ab729f3f311782fda50717eee55b3"
      },
      "type": "hmac-sha256:30dff9607b4087e3ae6808b4a3aa395b1fc064e467748c55c25ddf0e9b150fcc"
    },
    "remote_address": "127.0.0.1",
    "remote_port": 54798
  },
  "response": {
    "mount_type": "system"
  }
}
```

Closes hashicorp#7716
@vercel vercel bot temporarily deployed to Preview – vault-storybook October 10, 2021 17:35 Inactive
@vercel vercel bot temporarily deployed to Preview – vault October 10, 2021 17:35 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook October 10, 2021 19:27 Inactive
@vercel vercel bot temporarily deployed to Preview – vault October 10, 2021 19:27 Inactive
http/logical.go Outdated Show resolved Hide resolved
@ccapurso
Copy link
Contributor

Hi @remilapeyre, thank you for the contribution! This looks like an intriguing and useful addition to audit log entries. Would you be able to add a test that verifies that the remote port is included in the audit log entry? This will help us prevent regressions. To ensure consistency, a few scenarios we might want to test are:

  • the remote host has a port and it is valid
  • the remote host has a port but it is invalid
  • the remote host does not have a port

Here is a recent example of a test that verifies audit log output based on a logical.Request that you might want to reference.

@vercel vercel bot temporarily deployed to Preview – vault November 14, 2021 18:49 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook November 14, 2021 18:49 Inactive
@remilapeyre
Copy link
Contributor Author

Hi @remilapeyre, thank you for the contribution! This looks like an intriguing and useful addition to audit log entries. Would you be able to add a test that verifies that the remote port is included in the audit log entry? This will help us prevent regressions. To ensure consistency, a few scenarios we might want to test are:

* the remote host has a port and it is valid

* the remote host has a port but it is invalid

* the remote host does not have a port

Here is a recent example of a test that verifies audit log output based on a logical.Request that you might want to reference.

Hi @ccapurso, I updated the error handling and added a test for the happy case. I'm not sure how to test for when the port is invalid or missing since when using the test client it does not happen.

http/logical_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ccapurso ccapurso left a comment

Choose a reason for hiding this comment

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

I have one very minor change request but otherwise this looks good to me.

@pmmukh
Copy link
Contributor

pmmukh commented Dec 10, 2021

Hi @remilapeyre ! Just dropping a reminder that Chris requested one more change on this PR, post which it should be good to merge !

@vercel vercel bot temporarily deployed to Preview – vault-storybook December 31, 2021 18:57 Inactive
@remilapeyre
Copy link
Contributor Author

Thanks for the ping @pmmukh, it should all be good now!

Copy link
Contributor

@HridoyRoy HridoyRoy left a comment

Choose a reason for hiding this comment

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

Hi @remilapeyre , the PR looks really good! I have some small questions primarily about int types, but otherwise I'm happy to approve.

Thanks for the contribution!

audit/format.go Show resolved Hide resolved
audit/format.go Show resolved Hide resolved
http/logical.go Show resolved Hide resolved
http/logical_test.go Show resolved Hide resolved
Copy link
Contributor

@HridoyRoy HridoyRoy left a comment

Choose a reason for hiding this comment

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

After discussing and thinking through the int type questions a little more, I think everything looks great! Lgtm!

@remilapeyre
Copy link
Contributor Author

After discussing and thinking through the int type questions a little more, I think everything looks great! Lgtm!

Thanks!

@HridoyRoy HridoyRoy merged commit 385b8e8 into hashicorp:main Jan 26, 2022
qk4l pushed a commit to qk4l/vault that referenced this pull request Feb 4, 2022
* Add remote_port in the audit logs when it is available

The `request.remote_port` field is now present in the audit log when it
is available:

```
{
  "time": "2021-10-10T13:53:51.760039Z",
  "type": "response",
  "auth": {
    "client_token": "hmac-sha256:1304aab0ac65747684e1b58248cc16715fa8f558f8d27e90fcbcb213220c0edf",
    "accessor": "hmac-sha256:f8cf0601dadd19aac84f205ded44c62898e3746a42108a51105a92ccc39baa43",
    "display_name": "root",
    "policies": [
      "root"
    ],
    "token_policies": [
      "root"
    ],
    "token_type": "service",
    "token_issue_time": "2021-10-10T15:53:44+02:00"
  },
  "request": {
    "id": "829c04a1-0352-2d9d-9bc9-00b928d33df5",
    "operation": "update",
    "mount_type": "system",
    "client_token": "hmac-sha256:1304aab0ac65747684e1b58248cc16715fa8f558f8d27e90fcbcb213220c0edf",
    "client_token_accessor": "hmac-sha256:f8cf0601dadd19aac84f205ded44c62898e3746a42108a51105a92ccc39baa43",
    "namespace": {
      "id": "root"
    },
    "path": "sys/audit/file",
    "data": {
      "description": "hmac-sha256:321a1d105f8c6fd62be4f34c4da4f0e6d1cdee9eb2ff4af0b59e1410950fe86b",
      "local": false,
      "options": {
        "file_path": "hmac-sha256:2421b5bf8dab1f9775b2e6e66e58d7bca99ab729f3f311782fda50717eee55b3"
      },
      "type": "hmac-sha256:30dff9607b4087e3ae6808b4a3aa395b1fc064e467748c55c25ddf0e9b150fcc"
    },
    "remote_address": "127.0.0.1",
    "remote_port": 54798
  },
  "response": {
    "mount_type": "system"
  }
}
```

Closes hashicorp#7716

* Add changelog entry

* Empty commit to trigger CI

* Add test and explicit error handling

* Change temporary file pattern in test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: output the client source port to audit devices.
5 participants