-
Notifications
You must be signed in to change notification settings - Fork 101
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
Support CBT Hashes larger than SHA256 #94
Conversation
Thanks for this! While it seems reasonable enough, I wonder if instead of using pyasn1 we should just move to using PyCA cryptography for this task. |
Happy to do whatever is best, I only avoided cryptography originally as I've been cautioned against adding that library as a dependency before. Just a side note I am planning on implementing a similar thing to requests-kerberos once some dependencies have been updated so whatever is done here will probably be replicated there. |
Yeah, I can understand being warned against it: in fact, when the code is very simple I'd warn against it too. But the second we need to parse ASN1 or do any serious X.509 handling I think we want to delegate that to the experts. 😉 |
Hey @Lukasa I've done a POC to prove this out and I've gotten it to work with using pyOpenSSL with the following snippet to replace the ASN1 decoding part.
In reality this change doesn't really reduce the complexity as we still need to take that signature algorithm (now not an OID but the value of the OID) and determine the correct hashing functionality. Personally I don't think adding the depencency on pyOpenSSL is warranted in this case for 3 reasons;
Let me know your thoughts on it, if you feel strongly about using pyOpenSSL I can definitely do it I would just feel remiss if I didn't mention this. |
Any reason you went for PyOpenSSL instead of PyCA cryptography? Cryptography's X.509 API is probably better for this, and may be easier to deploy. |
I did not know cryptography could do that, will have a look at that and see what it is like. Thanks for the suggestion. |
Thanks @Lukasa for the recommendation I've pushed a change that uses cryptography and it seems more dynamic to me now. I've tested it manually from Python 2.6 to 3.6 on an IIS server backed with NTLM authentication and it all works fine. Let me know if there is anything else you wish for me to change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this work, it looks a lot better! I have some notes inline.
requests_ntlm/requests_ntlm.py
Outdated
pass | ||
else: | ||
certificate_hash = self._get_certificate_hash(server_certificate) | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This else
seems overbroad, given that it'll be triggered when self.send_cbt
is falsy. We probably don't need to warn in that case, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep you are correct, will fix
@@ -33,6 +38,7 @@ def __init__(self, username, password, session=None): | |||
if self.domain: | |||
self.domain = self.domain.upper() | |||
self.password = password | |||
self.send_cbt = send_cbt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we adding this flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have added this in the first implementation, it allows an upstream library to not send CBT if they ever not want to. Really just a way to override the behaviour if the code for this doesn't work in an expected way and a temp fix is required to get it going.
requests_ntlm/requests_ntlm.py
Outdated
|
||
return certificate_hash | ||
|
||
def _get_certificate_hash(self, certificate_der): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be a method, it never needs self
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC I did this so I can easily test the method as I had issues with methods outside of the class, will have a further look. Otherwise I can make it @staticmethod
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@staticmethod
is basically a lie and never worth using. If we think a user might want to override this on a class level, we should probably make it a dependency-injected function (e.g. by giving the parent function an optional argument whose default value is this function) rather than shove it onto the class just to allow a subclassing hook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is what I have added in the latest commit acceptable or would you still like me to change something else?
setup.py
Outdated
packages=[ 'requests_ntlm' ], | ||
install_requires=[ 'requests>=2.0.0', 'ntlm-auth>=1.0.2' ], | ||
install_requires=[ 'requests>=2.0.0', 'ntlm-auth>=1.0.2', 'cryptography' ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to provide version limitations here. Mind investigating what we need?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will have a look today thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nicely done!
as always thanks for your comments and input with these pull requests. |
As per the RFC 5929 - Channel Bindings for TLS
Previously the code always hashed the server certificate with SHA256 but when runnning this on an endpoint protected with a SHA384/SHA512 cert the CBT bindings would fail. This PR allows requests-ntlm to parse the certificate DER, determine the signature mechanism and hash the cert accordingly to pass onto the NTLM builder.
It also adds an optional flag to the init constructor that will make the library skip adding the certificate hash in case this is ever wanted. By default it will continue the practice of adding in the cert hash if it is available.