Skip to content

add support for proxy_connect module #43

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mredolatti
Copy link

@mredolatti mredolatti commented Jun 24, 2025

https://github.com/chobits/ngx_http_proxy_connect_module adds support for proxy CONNECT requests to nginx.
This patch adds support to this Digest Authentication module to work along with the aforementioned one.
New behaviour will only be enabled at compile time, if such module is configured, and will (should) only affect requests whose method name is CONNECT

When ngx_http_proxy_connect_module is used, a patch is applied to rc/http/ngx_http_request.c, which prevents r->unparsed_uri from ever being set (on that initial requests). This causes digest auth to fail immediately when comparing the uri field from the Authorization header with the one from the request.
For CONNECT requests, the uri is expected to match the server & port, so the comparison is now done against those in this case.****

@erikdubbelboer erikdubbelboer requested a review from Copilot June 27, 2025 07:46
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds conditional support for validating Digest Authentication on CONNECT requests when compiled with NGX_HTTP_PROXY_CONNECT. It extracts the host and port from the Authorization URI field and compares them against r->connect_host and r->connect_port.

  • Introduces an #ifdef NGX_HTTP_PROXY_CONNECT block to handle CONNECT method digest validation.
  • Parses the fields->uri to separate host and optional port.
  • Falls back to original URI comparison logic for other methods.

@@ -640,6 +640,32 @@ ngx_http_auth_digest_verify_hash(ngx_http_request_t *r,
ngx_md5_t md5;
u_char hash[16];

#ifdef NGX_HTTP_PROXY_CONNECT
if (r->method_name.len == 7 && ngx_strncmp(r->method_name.data, "CONNECT", 7) == 0) {
Copy link
Preview

Copilot AI Jun 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of comparing r->method_name bytes, consider using the NGINX request method enum (r->method == NGX_HTTP_CONNECT) for clarity and to avoid string comparisons.

Suggested change
if (r->method_name.len == 7 && ngx_strncmp(r->method_name.data, "CONNECT", 7) == 0) {
if (r->method == NGX_HTTP_CONNECT) {

Copilot uses AI. Check for mistakes.

Comment on lines 647 to 654
size_t uri_len = 0;
while (uri_len < fields->uri.len && fields->uri.data[uri_len++] != ':');
if (uri_len < fields->uri.len && fields->uri.data[uri_len] == ':') {
uri_len--;
}
if (!((r->connect_host.len == (uri_len - 1)) &&
(ngx_strncmp(r->connect_host.data, fields->uri.data,
uri_len) == 0))) {
Copy link
Preview

Copilot AI Jun 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The host-length check subtracts 1 from uri_len, likely causing an off-by-one error. Calculate the host segment length explicitly (e.g., host_len = colon_pos - uri_start) and compare against r->connect_host.len.

Suggested change
size_t uri_len = 0;
while (uri_len < fields->uri.len && fields->uri.data[uri_len++] != ':');
if (uri_len < fields->uri.len && fields->uri.data[uri_len] == ':') {
uri_len--;
}
if (!((r->connect_host.len == (uri_len - 1)) &&
(ngx_strncmp(r->connect_host.data, fields->uri.data,
uri_len) == 0))) {
size_t colon_pos = 0;
while (colon_pos < fields->uri.len && fields->uri.data[colon_pos] != ':') {
colon_pos++;
}
size_t host_len = colon_pos; // Host segment length is up to the colon
if (!((r->connect_host.len == host_len) &&
(ngx_strncmp(r->connect_host.data, fields->uri.data,
host_len) == 0))) {

Copilot uses AI. Check for mistakes.

Comment on lines 662 to 663
if (!((uri_port_len != r->connect_port.len) &&
(ngx_strncmp(uri_port, r->connect_port.data, ngx_min(uri_port_len, r->connect_port.len)) == 0))) {
Copy link
Preview

Copilot AI Jun 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition uses uri_port_len != r->connect_port.len combined with && and negated, but it should reject if lengths differ OR contents differ. Replace with:

if (uri_port_len != r->connect_port.len
    || ngx_strncmp(uri_port, r->connect_port.data, uri_port_len) != 0) {
    return NGX_DECLINED;
}
Suggested change
if (!((uri_port_len != r->connect_port.len) &&
(ngx_strncmp(uri_port, r->connect_port.data, ngx_min(uri_port_len, r->connect_port.len)) == 0))) {
if (uri_port_len != r->connect_port.len ||
ngx_strncmp(uri_port, r->connect_port.data, uri_port_len) != 0) {

Copilot uses AI. Check for mistakes.

@mredolatti
Copy link
Author

Hi, Thanks for taking a look at this. I have a couple of things i want to fix, so i'll push another commit most likely on monday. it should address those comments as well

@mredolatti
Copy link
Author

Hi i made some improvements as well as addressing the AI comments. the only thing i did not change is the syntax used in comparisons, which i agree that it can be a bit convoluted, but follows the code style of the rest of the file. I'm ok with changing it if you prefer so though

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.

1 participant