Skip to content
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

Replace deprecated Ntlm() with NtlmContext() #116

Closed
wants to merge 8 commits into from
Closed

Replace deprecated Ntlm() with NtlmContext() #116

wants to merge 8 commits into from

Conversation

ecederstrand
Copy link
Contributor

@ecederstrand ecederstrand commented May 23, 2019

Fixes #111

Fixes #108

@ecederstrand
Copy link
Contributor Author

ecederstrand commented May 23, 2019

Tests are failing, but it looks unrelated to my changes: pluggy.PluginValidationError: Plugin 'pytest_cov' could not be loaded: (coverage 4.0.3 (/home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages), Requirement.parse('coverage>=4.4'))!

Upstream issue: z4r/python-coveralls#66

@ecederstrand
Copy link
Contributor Author

ecederstrand commented May 27, 2019

Tests are now green. I had to add some unrelated changes to the Travis setup to get the tests through.

I think someone needs to add this project to the setup at https://coveralls.io/github/requests if we want to see the coverage report again.

@Lukasa Can you have a look at this PR?

@ecederstrand
Copy link
Contributor Author

Ping?

@ecederstrand
Copy link
Contributor Author

@jborean93 Can you have a look at this PR?

Copy link
Contributor

@jborean93 jborean93 left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, I've added some comments.

server_certificate_hash=server_certificate_hash
)
authenticate_message = authenticate_message.decode('ascii')
context._server_certificate_hash = server_certificate_hash
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of setting this we should be passing in a CBT object to NtlmContext on initialisation through the cbt_data kwarg. The _server_certificate_hash attribute is only used for backwards compat with Ntlm() in ntlm-auth and will be removed if I ever get to removing Ntlm().

What you need to do instead is change _get_certificate_hash() to return certificate_hash_bytes and don't worry about using hexlify there. Then you can create the CBT struct with:

from ntlm_auth.gss_channel_bindings import GssChannelBindingsStruct

cbt_data = GssChannelBindingsStruct()
cbt_data[cbt_data.APPLICATION_DATA] = b'tls-server-end-point:' + server_certificate_hash

It might be a good idea to change the variable name server_certificate_hash to b_server_cert_hash to make sure people are aware that it is a byte string.

@@ -131,7 +131,7 @@ def retry_using_http_NTLM_auth(self, auth_header_field, auth_header,
response3.history.append(response2)

# Get the session_security object created by ntlm-auth for signing and sealing of messages
self.session_security = context.session_security
self.session_security = context._session_security
Copy link
Contributor

Choose a reason for hiding this comment

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

This one is a tricky one, technically the wrap and unwrap methods of NtlmContext() should be used instead but unfortunately unwrap on NtlmContext has a different signature than the one of session_security. While this works I don't really want to rely on _ prefixed attributes because they aren't really guaranteed to stay stable over different releases.

What I recommend is to add a session_security property to this class like so:

@property
def session_security(self):
    warnings.warn("Deprecated property session_security, the wrap and unwrap methods should be accessed using the host context ntlm.contexts['hostname'].wrap/unwrap instead.", DeprecationWarning)
    return next(iter(self.contexts))._session_security

This also requires us to store the NtlmContext as a property in requests-ntlm. This is done in a similar way with requests-kerberos where the context is stored in a dict that's indexed with the hostname of the request. You can see that requests-kerberos uses urlparse from requests.compat import urlparse to get the hostname from the request and that is passed into the retry method. From there the context that is initialized is stored in self.contexts[hostname].

Doing this means that other libraries that rely on the wrapping and unwrapping functions provided by session_security aren't broken in the next release, are warned about it being deprecated here and are offered the alternative.

@jborean93
Copy link
Contributor

This has been superseded by #126.

@jborean93 jborean93 closed this Feb 15, 2023
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.

Ntlm warnings Deprecation Warning
2 participants