Skip to content

url: fix reuse of connections using HTTP Negotiate#20534

Closed
bagder wants to merge 5 commits intomasterfrom
bagder/nego
Closed

url: fix reuse of connections using HTTP Negotiate#20534
bagder wants to merge 5 commits intomasterfrom
bagder/nego

Conversation

@bagder
Copy link
Member

@bagder bagder commented Feb 6, 2026

Assume Negotiate means connection-based

Reported-by: Zhicheng Chen

Assume Negotiate means connection-based

Reported-by: Zhicheng Chen
@bagder bagder marked this pull request as ready for review February 6, 2026 13:52
@bagder bagder requested a review from Copilot February 6, 2026 13:59
@bagder
Copy link
Member Author

bagder commented Feb 6, 2026

@aisle-analyzer thoughts?

@aisle-research-bot
Copy link

aisle-research-bot bot commented Feb 6, 2026

🔒 Aisle Security Analysis

✅ We scanned this PR and did not find any security vulnerabilities.
Aisle supplements but does not replace security review.


Analyzed PR: #20534 at commit 8ad8422

@bagder
Copy link
Member Author

bagder commented Feb 6, 2026

augment review

Copy link

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 adjusts connection reuse selection so that HTTP Negotiate (SPNEGO) authentication is treated as connection-based during connection pool matching, similar to existing NTLM handling.

Changes:

  • Added connection-match flags to represent “want HTTP Negotiate” and “want proxy HTTP Negotiate”.
  • Introduced url_match_auth_nego() to exclude/allow reuse based on Negotiate auth state and credentials.
  • Integrated Negotiate matching into the existing connection reuse decision path in url_match_conn().

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@augmentcode
Copy link

augmentcode bot commented Feb 6, 2026

🤖 Augment PR Summary

Summary: Adjust connection reuse rules for HTTP Negotiate (SPNEGO), treating Negotiate authentication as connection-based.

Changes:

  • Add host/proxy Negotiate intent flags to the connection-pool match context.
  • Introduce `url_match_auth_nego()` to avoid reusing connections whose Negotiate state or credentials don’t match the new transfer.
  • Integrate the Negotiate matcher into `url_match_conn()` and initialize the new flags in `url_attach_existing()`.

🤖 Was this summary useful? React with 👍 or 👎

Copy link

@augmentcode augmentcode bot left a comment

Choose a reason for hiding this comment

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

Review completed. 1 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

@github-actions github-actions bot added the tests label Feb 6, 2026
@michael-o
Copy link
Contributor

Assume Negotiate means connection-based

This assumption is not correct. In a SPNEGO/Kerberos loop one roundtrip is sufficient and can be request-legged. See also https://github.com/gssapi/mod_auth_gssapi?tab=readme-ov-file#gssapiconnectionbound

@bagder
Copy link
Member Author

bagder commented Feb 7, 2026

Yes, but Negotiate can also mean NTLM which is, and we can't easily tell the difference. So better safe than sorry I think.

@michael-o
Copy link
Contributor

Yes, but Negotiate can also mean NTLM which is, and we can't easily tell the difference. So better safe than sorry I think.

Sure ot can, bu this can be quieried from the security context, but given that this should not be the norm and only on Windows I'd at least would try to cover both.

@bagder
Copy link
Member Author

bagder commented Feb 7, 2026

this can be quieried from the security context

If you say so. I don't know how, and we don't have test cases for either way.

but given that this should not be the norm and only on Windows I'd at least would try to cover both.

NTLM is certainly not only on Windows. curl supports it on all platforms. And yes, it would be great to support both ways but in this PR I focus on fixing the current bug and leave the improvements for a separate follow-up work.

@michael-o
Copy link
Contributor

but given that this should not be the norm and only on Windows I'd at least would try to cover both.

NTLM is certainly not only on Windows. curl supports it on all platforms. And yes, it would be great to support both ways but in this PR I focus on fixing the current bug and leave the improvements for a separate follow-up work.

You are confusing curl's custom NTLM impl and the GSS-API mech. Only SSPI implements NTLM natively through SPNEGO when Kerberos fails. MIT Kerberos and Heimdal require https://github.com/gssapi/gss-ntlmssp to be installed and a credential file placed. Cannot use the Kerberos credential cache. This quite boils down a token starting with "TlRMTVNTUAAB" is rare outside of SSPI.

@michael-o
Copy link
Contributor

this can be quieried from the security context

If you say so. I don't know how, and we don't have test cases for either way.

Ideomatic way with GSS-API if SPNEGO is used:

/* Check if the underlying mechanism is already ready */
if (ret_flags & GSS_C_PROT_READY_FLAG) {
    printf("Proto-ready: inner mechanism is established\n");

    /* Now we can safely query the mechanism */
    maj = gss_inquire_context(&min,
                              ctx,
                              NULL,
                              NULL,
                              NULL,
                              &mech,
                              NULL,
                              NULL,
                              NULL);

    if (maj == GSS_S_COMPLETE || maj == GSS_S_CONTINUE_NEEDED) {
        printf("Mechanism OID: { ");
        for (size_t i = 0; i < mech->length; i++) {
            printf("%u ", (unsigned char) mech->elements[i]);
        }
        printf("}\n");
    }
}

Of course, I leave the upto you. Just wanted to let you know the options...

@bagder bagder closed this in 34fa034 Feb 7, 2026
@bagder bagder deleted the bagder/nego branch February 7, 2026 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants