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

Add support for GSS-SPNEGO bind #33

Closed
wants to merge 2 commits into from

Conversation

awakecoding
Copy link

This pull request adds support for the GSS-SPNEGO bind type with NTLM. There is no support for integrated Windows authentication or Kerberos at this point. This has been tested against Windows Active Directory, compared against sample Wireshark captures.

@inejge
Copy link
Owner

inejge commented Jun 5, 2019

@awakecoding Thanks for the PR! I gave it a brief once-over since I don't have the time for much more ATM. It's OK with two caveats: I think I'd prefer a different way of parsing (more like exops), and I'd place the whole thing behind a compile-time feature (sspi is a rather substantial dependency). In any case, leave the PR as it is now, and I'll circle back with more detailed comments later.

@awakecoding
Copy link
Author

@inejge thanks, I was expecting to have to make modifications before it is clean enough to be merged. I need to make one unexpected small change before we can merge this: yes, the bind works, but in its current form GSS-SPNEGO confidentiality and integrity are negotiated.

In other words, all messages following the GSS-SPNEGO bind are encapsulated in SASL buffers with support for confidentiality (encryption) and integrity (signing) based on a session key derived from the NTLM or Kerberos exchange. I've checked MS-ADTS and this feature works only without TLS, if it is used over TLS, Active Directory will reject the requests.

I've managed to disable confidentiality and integrity and it works, but I'd like your input on how such a feature could be added to the current code. This is a significant change: a mutable context (the SPNEGO context) needs to be preserved for the entire LDAP connection. All messages being sent need to be wrapped in SASL buffers with optional encryption and signing. All messages being received need to handle the SASL buffer encapsulation layer, optionally decrypt and validate the signatures.

For this PR, I'll add the necessary options to sspi-rs to disable confidentiality and integrity negotiation in the NTLM exchange, but after that I'd look into supporting this feature. Active Directory does not enforce LDAPS by default, which means this is likely the best protection against MITM attacks on LDAP traffic when TLS is not in use (TLS remains the preferred option, always).

@inejge
Copy link
Owner

inejge commented Jun 6, 2019

@awakecoding I've opened the ntlm branch, which you can fetch and cherry-pick f2320f2 to see the direction I propose to follow. Roughly, I elminated the special-case parsing of Bind responses and the treatment of SASL credentials as pseudo-controls: they are structurally similar but semantically different, and I'd like to keep the distinction in the code.

For the rest, there are two-and-a-half changes I'd prefer to see:

  1. In the example, eliminate argument processing and introduce string literal constants for the connection parameters. They can have dummy values. All other examples are self-contained in that way.

  2. In sasl_spnego_bind(), there are a couple of unwrap()s after SSPI support library calls which should be converted to error returns, via map_err() if necessary. Checking the expected return code of the first Bind call should also be added there.

  3. (Optional) I've noticed that calls to sspi::Credentials::new() take String arguments and don't accept &str. The latter is customary for better flexibility. Ditto for create_bind_request(): it can take &[u8] to avoid superfluous clone()s.

I'll take care of hiding all this behind a compile-time feature.

@inejge
Copy link
Owner

inejge commented Jun 6, 2019

Re confidentiality/integrity: technically, it can be handled similarly to TLS. See how STARTTLS is implemented in src/tls_client.rs. Tokio needs an I/O object which implements AsyncRead and AsyncWrite, and, once the initial setup is complete, all protocol traffic should work transparently. It's not an insignificant amount of work, but it shouldn't change the shape of the crate radically.

Now, I don't recommend starting that work until I see how the crate can be migrated from the original tokio_core and tokio_proto to the more recent version of Tokio. Furthermore, async/await is nearing stabilization, which is another unknown in the whole setup. Therefore, I'd recommend only solving the most pressing interop problems first.

@awakecoding
Copy link
Author

@inejge thanks a lot for taking the time to reorganize and cleanup my changes :) Yes, there are a few improvements that should be done in the sspi-rs crate (I agree about accepting &str). The original code came from somebody else, my long term goal is to refactor it to match the original SSPI API much more closely, and eventually support native SSPI modules on Windows, which should get us integrated windows authentication support for free.

@inejge
Copy link
Owner

inejge commented Jun 13, 2019

@awakecoding Since this PR went quiet, do you plan to continue work on it? I can wrap it up myself, but it'll take a week or two.

@awakecoding
Copy link
Author

@inejge please go ahead and wrap it up, even if it takes a week or two, that's fine. I'll just rebranch from master after the cleaned up version has been merged, and then we can close this PR without merging my original changes. Thanks a lot!

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.

None yet

2 participants