evanmiller / mod_zip

Streaming ZIP archiver for nginx 📦
https://www.nginx.com/resources/wiki/modules/zip/
BSD 3-Clause "New" or "Revised" License
215 stars 64 forks source link

Pass credentials in sub-requests #87

Closed devgs closed 3 years ago

devgs commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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

Any chance of test coverage on this one?

devgs commented 3 years ago

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 commented 3 years ago

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

Please ensure that the default behavior remains unchanged.

devgs commented 3 years ago

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 commented 3 years ago

Added test coverage.

evanmiller commented 3 years ago

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

devgs commented 3 years ago

As happy as I can get :) Thanks!