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

Pass credentials in sub-requests #87

Merged
merged 9 commits into from
Nov 18, 2021

Conversation

devgs
Copy link
Contributor

@devgs devgs commented Nov 17, 2021

Recently we have tried to update mod_zip on our servers and have faced an obstacle. At some point, sub-request logic was changed in such a way that no header fields of the original request were supplied to the sub-requests. Specifically important for us were the ones that communicate some credentials. We, ourselves, use a combination of Cookie, Authorization and some X-* header fields. So these are the ones that I've 'whitelisted'.

@evanmiller
Copy link
Owner

Since this is a pretty significant behavior change, I think it would make sense to offer configuration options here. E.g. a switch to turn on header-passing and some way to control the whitelist.

@devgs
Copy link
Contributor Author

devgs commented Nov 18, 2021

@evanmiller I've added a dedicated HTTP header filed that will communicate the whitelist, if necessary:

X-Archive-Pass-Headers: <header-name>[:<header-name>]*

@evanmiller
Copy link
Owner

@devgs Wouldn't it make more sense to have the whitelist in the configuration file rather than from the upstream response? E.g. zip_pass_header, similar to proxy_pass_header?

@devgs
Copy link
Contributor Author

devgs commented Nov 18, 2021

@evanmiller It just seems that an upstream has more knowledge about the kinds of header fields that it requires in order to authorize file access. This can be tightly coupled to each specific request. From my experience, you can have a generic location /api/ that simply implements a proxy logic and all the request logic is handler by the upstream.

If you see a benefit in adding zip_pass_header, I would suggest making it a complementary feature, along with X-Archive-Pass-Headers. Personally, I wouldn't want to pollute my nginx config, just be cause a bunch out of the hundreds of requests uses mod_zip.

What do you say?

@evanmiller
Copy link
Owner

OK, fine with me since you are the one using it :-).

Any chance of test coverage on this one?

@devgs
Copy link
Contributor Author

devgs commented Nov 18, 2021

OK, fine with me since you are the one using it :-).

So, you want me to add zip_pass_header or leave it as is?

Any chance of test coverage on this one?

Yeah, I think I can come up with something.

@evanmiller
Copy link
Owner

Leave as is; we can explore a configuration option if there is demand later.

Please ensure that the default behavior remains unchanged.

@devgs
Copy link
Contributor Author

devgs commented Nov 18, 2021

Yes, I have replaced my original changes. Now, only the requested headers are passed and only if anything is requested. This means that by default (when X-Archive-Pass-Headers is not present) the list of sub-request headers will be empty, as is the default behavior in a current master.

Now I'm going to come up with some tests.

@devgs
Copy link
Contributor Author

devgs commented Nov 18, 2021

Added test coverage.

@evanmiller
Copy link
Owner

Let me know when you're happy with it and I'll merge it in.

@devgs
Copy link
Contributor Author

devgs commented Nov 18, 2021

As happy as I can get :) Thanks!

@evanmiller evanmiller merged commit 555d3b3 into evanmiller:master Nov 18, 2021
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.

None yet

2 participants