Skip to content

Support GSSAPI channel bindings #57

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 3 commits into
base: main
Choose a base branch
from
Open

Conversation

simo5
Copy link

@simo5 simo5 commented Jul 14, 2025

This change adds support for GSSAPI channel bindings, which helps to protect against man-in-the-middle relay attacks by tying the authentication to the underlying secure channel.

A channel_bindings parameter is added to HTTPSPNEGOAuth. When set to 'tls- server-end-point', the server's TLS certificate is retrieved from the socket, hashed, and used to create the GSSAPI channel bindings.

This feature requires the cryptography library as an optional dependency. If it's not available, channel bindings cannot be used and a warning is logged.

Fixes #56

@simo5
Copy link
Author

simo5 commented Jul 14, 2025

@cryptomilk can you test if this patch works for you?

I am not absolutely certain it will because it is largely untested.

In order to get the cert locally I had to set stream=True on the requests.get call in order to preserve the HTTPConnection struct which holds the ssl scoket necessary to retrieve the peer cert.

@simo5 simo5 marked this pull request as draft July 14, 2025 14:09
@simo5 simo5 requested a review from jborean93 July 14, 2025 15:17
@simo5
Copy link
Author

simo5 commented Jul 14, 2025

@jborean93 before merging we should wait for @cryptomilk to confirm it works as needed, we know there is still some issue in the test environment we are trying to figure out. The Windows Server seem not fully happy with this yet, although it is doing the same thing request-kerberos also does ...

@cryptomilk
Copy link

It would be great to have a release as soon as channel bindings support is added.

This change adds support for GSSAPI channel bindings, which helps to protect
against man-in-the-middle relay attacks by tying the authentication to the
underlying secure channel.

A `channel_bindings` parameter is added to `HTTPSPNEGOAuth`. When set to 'tls-
server-end-point', the server's TLS certificate is retrieved from the socket,
hashed, and used to create the GSSAPI channel bindings.

This feature requires the `cryptography` library as an optional dependency. If
it's not available, channel bindings cannot be used and a warning is logged.

Co-authored-by: Gemini <[email protected]>
Signed-off-by: Simo Sorce <[email protected]>
@simo5 simo5 marked this pull request as ready for review July 16, 2025 09:21
@jborean93
Copy link
Contributor

I can have a test against a real env tomorrow with CBT enforced to verify but the impl at a glance looks good.

@cryptomilk
Copy link

I've tested it against a Windows Server 2025 which requires it and it works.

This is my WIP branch: https://github.com/cryptomilk/cepces/tree/asn-gssapi

@jborean93
Copy link
Contributor

I've tested this against just a plain pip install requests and it seems like the logic for getting the socket isn't right. I know that the whole urllib backend thing with requests is tricky and that potentially might be overridden by some other backend if installed as a system package.

I've done this in requests-kerberos for nearly 10 years now and it hasn't failed me but I also haven't really tested it against requests being installed by something like dnf. It's probably worth investigating some more to see if there is a more public way of getting the socket but this does work for me

diff --git a/src/requests_gssapi/gssapi_.py b/src/requests_gssapi/gssapi_.py
index cb24378..f3556aa 100644
--- a/src/requests_gssapi/gssapi_.py
+++ b/src/requests_gssapi/gssapi_.py
@@ -154,18 +154,21 @@ class HTTPSPNEGOAuth(AuthBase):
 
         gss_cb = None
         if self.channel_bindings == "tls-server-end-point":
+            try:
+                sock = response.raw._fp.fp.raw._sock
+            except AttributeError:
+                sock = None
+
             if is_preemptive:
                 raise SPNEGOExchangeError(
                     "channel_bindings were requested, but are unavailable for opportunistic authentication"
                 )
             # The 'connection' attribute on raw is a public urllib3 API
             # and can be None if the connection has been released.
-            elif getattr(response.raw, "connection", None) and getattr(response.raw.connection, "sock", None):
+            elif sock:
                 # Defer import so it's not a hard dependency.
                 from cryptography import x509
 
-                sock = response.raw.connection.sock
-
                 der_cert = sock.getpeercert(binary_form=True)
                 cert = x509.load_der_x509_certificate(der_cert)
                 hash = cert.signature_hash_algorithm

@simo5
Copy link
Author

simo5 commented Jul 17, 2025

@jborean93 Why would you use internal undocumented attributes, when there are perfectly valid public ones?

@jborean93
Copy link
Contributor

jborean93 commented Jul 17, 2025

Because at least the ones in the PR right now seem to be set to None meaning it doesn’t work at all. I wrote the requests-kerberos code many years ago before there were some of these public attributes.

I also only just saw your original comments saying stream=True is needed. We should add that to the docs.

I’m not saying we should use my example, if yours works with stream=True then we should just document and avoid using internal APIs.

@simo5
Copy link
Author

simo5 commented Jul 17, 2025

stream=True is needed only if you need access to the socket after the get operation is completed, but it should not be needed during a gssapi request as the gssapi operation happens after we connect to the server but before we are done with the negotiate, so for request-gssapi it should not be needed and our tests seem to confirm that.

@jborean93
Copy link
Contributor

I’ll have a dig in tomorrow but the PR as it stands didn’t have a socket to get the cert from and thus failed to authenticate. I’m not sure whether it’s due to a different backend, the lack of stream=True or something else but it certainly didn’t work just by adding the kwarg.

@jborean93
Copy link
Contributor

jborean93 commented Jul 17, 2025

Here is a capture of a debug session showing you what I can see where response.raw.connection is None.

image

The response.raw is urllib3.response.HTTPResponse so it'll be interesting to see if your's is any different.

I just ran the following script to test this out against a WSMan endpoint that has CBT set to Strict.

import sys

import requests
import urllib3

from requests_gssapi import HTTPSPNEGOAuth

urllib3.disable_warnings()

chan_bin = None
if len(sys.argv) > 1 and sys.argv[1] == 'true':
    chan_bin = 'tls-server-end-point'

gssapi_auth = HTTPSPNEGOAuth(channel_bindings=chan_bin)
r = requests.post(
    "https://server2022.domain.test:5986/wsman",
    auth=gssapi_auth,
    verify=False,
    stream=True,  # I've tried without this as well
)
print(r)

I also just tested this in a Fedora 42 container with dnf install -y python3 python3-cryptography python3-requests and it also has the same problem so it's not a system package vs pypi requests difference. Can you potentially see any differences with how I am running things here?

I wonder if it's the POST request rather than GET where it hasn't kept it alive from the initial 401 response that triggered the authentication?

@simo5
Copy link
Author

simo5 commented Jul 17, 2025

I wonder if it could be an HTTP 1.0 vs HTTP 1.1 thing maybe? But POST may also be the issue here, I wonder if we have a way to interject a capturing event right during the request, to ensure the sock is present ...

@simo5
Copy link
Author

simo5 commented Jul 17, 2025

The response.raw is urllib3.response.HTTPResponse so it'll be interesting to see if your's is any different.

My manual tests also used urllib3.response.HTTPResponse not sure if @cryptomilk has something else in his environment.

@simo5
Copy link
Author

simo5 commented Jul 17, 2025

Maybe we should recommend use of Sessions which will cause urllib3 to use connection pooling and keep connections open ?

@cryptomilk
Copy link

Looking at the cepces code it does requests.post() using HTTP 1.1.

send: b'POST /ADPolicyProvider_CEP_Kerberos/service.svc/CEP HTTP/1.1\r\nHost: win-ca01.mars.milkyway.site\r\nUser-Agent: python-requests/2.32.3\r\nAccept-Encoding: gzip, deflate\r\
nAccept: */*\r\nConnection: keep-alive\r\nContent-Type: application/soap+xml; charset=utf-8\r\nContent-Length: 915\r\n\r\n'

@cryptomilk
Copy link

Debug log, cepces with channel bindings:

https://cpaste.org/?71dafeb41c002b98#Hc1u3vQMrCtZCqzgWLfYb1mfdAzFsZvagDAkW1HnXLaU

@cryptomilk
Copy link

Maybe ask requests upstream what the right way is to get access to that certificate?

@cryptomilk
Copy link

What about this: cryptomilk@bdadc2f

@cryptomilk
Copy link

@jborean93 could you try with commit: cryptomilk@bdadc2f ?

@jborean93
Copy link
Contributor

jborean93 commented Jul 24, 2025

It works but seems to be using non-public attributes and now also relies on methods being called in certain orders internally inside requests. The diff I shared in #57 (comment) also uses non-public attributes but isn't reliant on any patching of functions inside requests or that those functions are called in a certain way for the connection/cert to be present.

It certainly would be nice if there was a public way to get this but it seems like requests is in the exact same scenario from 8 years ago when I first implemented support for CB in requests-kerberos and went the non-public way.

@jborean93
Copy link
Contributor

I've stepped through the code and can confirm it is the presence of Connection: close in the WSMan response to the initial request that causes urllib3 to set the sock on the raw response's connection to None. This makes sense as the connection is closed according to urllib3. Strictly speaking even if we were to use some non-public APIs to extract the certificate in this case theoretically the certificate context that will be tied to the auth is for a separate channel and not the one actually being used. In 99% of the cases this wouldn't matter as the cert never changes but I wonder if this might have issues with things like load balancers or anything else that might cause the TLS context to happen on another host after the initial authentication failure.

I think from an initial implementation we should just stick with what @simo5 has here and just document that CBT won't work if the initial response closes the connection through Connection: close. We already have a stipulation that this won't work for opportunistic_auth=True.

As for the future I unfortunately don't see this being fixed for these scenarios. I see three things that could happen

  • Do nothing and wait until there's a requirement for this
  • Use non-public APIs to access the cert like requests-kerberos does
  • Try and make changes upstream to support this scenario

The second option will work but strictly speaking the cert will be for the old connection TLS context and not the one actually used for the subsequent auth. I've never encountered such a scenario where this actually matters but it's worth considering.

The third option would be best but the way requests and urllib3 work make it next to impossible without some changes in how it works. What would need to happen is

  • urllib3 exposes a way to connect and do the TLS handshake before the request is ready
  • urllib3 would need to expose that connection/socket to requests
  • requests call this new method in urllib3 before the auth handler is called
  • requests provides the connection/socket (or just cert data) to the auth handler

This would solve the opportunistic_auth=True scenario (and when Connection: close would have been received on a bad auth) as the TLS data is available before any request is sent. That's a lot of moving parts and how it interacts with the connection pooling in requests/urllib3 would need to be adjusted to support this connection setup pre request scenario.

@simo5
Copy link
Author

simo5 commented Aug 1, 2025

@jborean93 what about we merge this as is and see how it goes?
We can always change it later if needed

@simo5
Copy link
Author

simo5 commented Aug 1, 2025

@jborean93 if we need to request a change upstream it would be to give us an option to ensure the connection is not close (at least on a 401 response), so that the HttpConnection object stays around and most importantly we do not end up changing host we are speaking to.
This would be incompatible with opportunistic authentication (although we could cache the cert fingerprint and allow it for subsequent attempts), but would be good enough for most uses.

@jborean93
Copy link
Contributor

what about we merge this as is and see how it goes

I'm fine with that, it at least seems to solve @cryptomilk's scenario so better than nothing. I think we should at least document in the readme that it won't work for connections that are closed on the initial auth failure.

if we need to request a change upstream it would be to give us an option to ensure the connection is not close (at least on a 401 response),

I haven't read the RFC's but I'm not sure if that's compliant to the spec. If the server has responded with Connection: close it most likely has closed its end of the socket and trying to reuse it would result in a failure.

While definitely a bigger ask I think fundamentally the urllib3/requests model just doesn't work in a way right now to achieve this for all scenarios. We can certainly use some non public APIs to solve the 401 close scenario like requests-kerberos does but that sounds wrong anyway. The only proper way around this is see is if requests has done the TLS handshake first before calling the auth library but that would involve a few changes in both requests and urllib3.

@cryptomilk
Copy link

psf/requests#4847 is the requests upstream issue.

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.

Add Channel Binding support (Extended Protection for Authentication (EPA))
3 participants