Specify allowed headers explicitly instead of using a wildcard

[chaitanyagupta]

While testing out the recently merged CORS support I faced this error in both Chrome and Firefox:

XMLHttpRequest cannot load http://127.0.0.1:8200/v1/secret/path. Request header field X-Vault-Token is not allowed by Access-Control-Allow-Headers in preflight response.

Vault sets the value of Access-Control-Allow-Headers to *. This, it turns out, is still an open spec issue and has not yet been merged in either Chrome or Firefox: whatwg/fetch#548

The fix is to explicitly specify the list of headers that should be allowed e.g. Content-Type, X-Vault-Token, etc. The changes here fix that.

[jefferai]

This isn't sufficient as there are more headers vault uses and more it may use in the future. We could maybe set it to the current list but then I'd want to refactor things so that we have less risk of leaving something out later. But even then a newer client against an older vault server may have problems.

[chaitanyagupta]

I was wondering if there are other headers that Vault uses. So how do you want to go about it? Because in its current form, CORS support is pretty much not working.

[chaitanyagupta]

I've added two more headers to the allowed list: Content-Type to send a JSON body in POST requests, and X-Requested-With to support libs like jquery which set it by default.

[jefferai]

This is illustrating my point -- you've added two more headers, but you've still omitted actual Vault headers that Vault clients can legitimately set.

[chaitanyagupta]

Yes, I get what you are saying. No need to merge this PR (though I will keep updating my branch to ensure things work for me).

That said, I don't think CORS support is usable until this issue is fixed.

I also grepped though the source to figure out which other headers vault clients might use. Is this all, or are there any headers that I've missed?

X-Vault-AWS-IAM-Server-ID
X-Vault-Internal-No-Request-Forwarding
X-Vault-No-Request-Forwarding
X-Vault-Token
X-Vault-Wrap-Format
X-Vault-Wrap-TTL

[jefferai]

All of those can be supplied by a client except the Internal-No-Request-Forwarding one.

However, thinking about this, I think really the right thing to do would be to have that be a default list, but expand the CORS support to allow specifying allowed headers in addition to allowed origins. This is especially useful since there is now support to allow specifying headers to be audit-logged for request tracing purposes.

It's more work but in the end ensures that all uses can be satisfied. Any chance you'd be willing to dive in on that?

[chaitanyagupta]

All of those can be supplied by a client except the Internal-No-Request-Forwarding one.

However, thinking about this, I think really the right thing to do would be to have that be a default list, but expand the CORS support to allow specifying allowed headers in addition to allowed origins. This is especially useful since there is now support to allow specifying headers to be audit-logged for request tracing purposes.

Yes, this makes sense.

It's more work but in the end ensures that all uses can be satisfied. Any chance you'd be willing to dive in on that?

Unfortunately I can't. I am unfamiliar with both Go and Vault internals, and with no time to spare, it doesn't look possible for me to pick up.

(As for the status of my fork, I will use it for the time being. However, to stay upto date with upstream over the long term, unless this issue gets fixed, it makes sense to use a reverse proxy that will serve both vault and the UI from the same origin).

[naunga]

I'd be happy to work on this PR if someone else isn't already.

[jefferai]

@naunga Nobody is working on this currently, just an open issue. Thanks!

[jefferai]

Fixed by #3023

[chaitanyagupta]

That's great! Thanks @naunga @jefferai