Making `http_ssl_module` optional
mroach opened this issue · 2 comments
Currently, nginx has to be compiled --with-http_ssl_module
in order for this module to be compiled. There's just one line in the module that depends on this module being available:
I took a whack at this locally, and this works:
#ifdef NGX_HTTP_SSL
const char *scheme = (r->connection->ssl) ? "https" : "http";
#else
const char *scheme = "http";
#endif
Would this be an acceptable change to make to the module? I'm happy to open a PR if so.
My C skills are nearly nil and I've never developed an nginx module, so I wanted to check first.
Yes, sounds good -- can you submit a PR with that change?
I ran into an issue during my testing I wanted to bring up before delivering a change that could lead to headaches for other users using this behind an SSL-terminating load balancer.
In my test config, I have this:
location /secured/token/redirect {
auth_jwt_location HEADER=Authorization;
auth_jwt_redirect on;
auth_jwt_loginurl /login;
}
Next, in my test, I set both the host
and x-forwarded-proto
headers when making a request, just like an SSL-terminating load balancer would:
curl http://localhost:8123/secured/token/redirect \
--header "host: api.secureapp.test:8123" \
--header "x-forwarded-proto: https"
The issues are in the response Location
header:
< HTTP/1.1 302 Moved Temporarily
< Server: nginx
< Location: http://api.secureapp.test:8080/login?return_url=https://api.secureapp.test/secured/token/redirect%3Ftest=path%2Bsecured%2Bby%2Btoken%252c%2Bredirect%2Bon%2Bfailure%2Bwithout%2Btoken
There are two issues here:
- The scheme is
http
instead ofhttps
. - The port is wrong. No matter what I set in the
host
header, it's always using thelisten
port.
Next, I tried a more manual approach here by setting my config to use: auth_jwt_loginurl https://$host:$port/login
.
This doesn't work. The response is a literal Location: https://$host:$port/login?redirect_url=...
That seems like a separate issue to me where config values aren't supporting variable substitution. I have no idea how that's meant to work with nginx modules.
Looking at the code for this module, it doesn't appear to me that it's in control of the the scheme, host, and port part of the Location
response header. Looking around the code of nginx itself, I think it's caused by this output header filtering: https://github.com/nginx/nginx/blob/a13ed7f5ed5bebdc0b9217ffafb75ab69f835a84/src/http/ngx_http_header_filter_module.c#L327-L354
This led me to discover a workaround: use absolute_redirect off;
in the nginx configuration
My suggested patch as-is allows the module to be compiled without SSL and work great behind a load balancer as long as absolute redirects aren't used since the redirect values will be wrong. Having to switch-off absolute redirects as a workaround feels like a decently large caveat and that there's room for improvement here.
For my particular use case, I've already worked-around all of this with a sed
one-liner to replace that one r->connection-ssl
line causing my issue and I don't use redirects at all, so I don't need these changes.
Do you think it's worth including this patch even though it comes with some downsides and caveats?