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

feature request - Ability to add HTTP headers to the audit log #1451

Closed
maguec opened this issue May 25, 2016 · 5 comments
Closed

feature request - Ability to add HTTP headers to the audit log #1451

maguec opened this issue May 25, 2016 · 5 comments
Assignees
Milestone

Comments

@maguec
Copy link
Contributor

maguec commented May 25, 2016

Currently the audit log will display

    "request": {
        "client_token": "",
        "data": null,
        "operation": "read",
        "path": "auth/token/lookup-self",
        "remote_address": "172.19.56.5"
    },

However, in the case where this is behind a load balancer, it would be helpful if the log could grab the X-Forwarded-For header for more detailed information so that something like

    "request": {
        "client_token": "",
        "data": null,
        "operation": "read",
        "path": "auth/token/lookup-self",
        "remote_address": "192.168.5.5",
        "HTTP_x_forwarded_for: "192.168.0.47"
    },
@jefferai
Copy link
Member

We should definitely do this once we finally get some proper support for X-Forwarded-For/X-Real-IP/etc.

@jefferai jefferai added this to the future milestone May 26, 2016
@bkrodgers
Copy link
Contributor

Just wanted to add a +1 on this, and ask if the "proper support" for X-Forwarded-For you refer to is addressing the spoofing concerns that have been raised on #291 and #799. Those are legit concerns as far as using these headers for security decisions. However, do we really need to wait to do this until that's resolved? It'd be useful right now to have it there for audit logging, and there aren't really any major security concerns in doing so, AFAIK.

We could also broaden it and just have it log all incoming headers in a headers object under the request one. i.e., the above might look like:

  "request": {
        "client_token": "",
        "data": null,
        "operation": "read",
        "path": "auth/token/lookup-self",
        "remote_address": "192.168.5.5",
        "headers": {
           "x_forwarded_for: "192.168.0.47",
           "content-type": "application/json"
         }
    },

You'd want to HMAC or just exclude X-Vault-Token, of course. Perhaps "Authentication" too just in case someone sends a token on that by mistake.

@jsoap
Copy link

jsoap commented Jan 9, 2017

Also want to add a +1 on this issue.

Currently I have an instance of mod_security which reverse proxies to vault. However the audit logs do not expose the origin IP but rather the WAF's IP, this makes the auditors understandably nervous.

@jefferai
Copy link
Member

jefferai commented Jan 9, 2017

@jsoap Shouldn't auditors be nervous about you having a MITM between your client and your Vault server? :-D

In a general sense, I'm not sure adding all headers to the audit log is a good idea...or at least if we did, we'd probably want to HMAC them for safety, which then limits utility since you'd have to know the IP you want to check for ahead of time.

There are also issues around trusting X-Forwarded-For and ilk, and even the PROXY protocol -- see #1340 (comment)

I wonder if a good approach would be to simply harcode a whitelist of headers (potentially with a future enhancement making it configurable by the administrator) that should be audit logged. Although relying on X-Forwarded-For and ilk for making security decisions still seems like a bad idea, it currently makes sense to me that these particular headers should get logged if they exist.

@jefferai
Copy link
Member

As an update, did some internal discussion and I think the approach for now will be to add specific headers to the audit logs. Possibly in the future it could be configurable, but simply adding X-Forwarded-For/X-Real-IP and co will make a lot of people happy.

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

No branches or pull requests

5 participants