-
Notifications
You must be signed in to change notification settings - Fork 181
RUST-2245 Implement GSSAPI auth support for Windows #1444
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
base: main
Are you sure you want to change the base?
Conversation
Assigned |
Is it possible to make exceptions to the |
I investigated further and am not confident SSPI support can be implemented the way our drivers require if we do not use these There are limited crate options for kerberos auth. We've ruled out Using the native Windows API inherently requires |
I haven't had the chance to look into this PR in detail, but it should work to use a |
error::{Error, Result}, | ||
}; | ||
|
||
pub(super) struct GssapiAuthenticator { |
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.
Not changed in this PR, but this being a fresh file made me realize - this is only ever either
GssapiAuthenticator { pending_ctx: Some(...), established_ctx: None, is_complete: false, ... }
or
GssapiAuthenticator { pending_ctx: None, established_ctx: Some(...), is_complete: true, ... }
right? That seems like it would be better modeled via a state enum, e.g.
enum CtxState {
Pending(PendingClientCtx),
Established(ClientCtx),
}
pub(super) struct GssapiAuthenticator {
ctx: CtxState,
user_principal: String,
}
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.
Ah that's a nice refactor. I'll make this change.
src/client/auth/gssapi/windows.rs
Outdated
} | ||
|
||
fn acquire_credentials_and_init(&mut self, password: Option<String>) -> Result<Vec<u8>> { | ||
unsafe { |
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 believe the only unsafe portion of this is calling AcquireCredentialsHandleW
, right? I'd rather see the unsafe
blocks as narrowly scoped as possible.
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.
Good point! I started broad to allow for whatever had to be done, but I'll narrow these down now like you suggested 👍
|
||
// Perform the final step of Kerberos authentication by gss_unwrap-ing the | ||
// final server challenge, then wrapping the protocol bytes + user principal. | ||
// The resulting token must be sent to the server. |
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 is like a monad transformer 😂
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.
one tiny comment, I did a pass over the C-style code but will similarly defer to @kevinAlbs on the details
src/client/auth/gssapi/mod.rs
Outdated
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 filename should be able to stay the same - we stopped using mod.rs
files a while ago (https://doc.rust-lang.org/reference/items/modules.html#module-source-filenames)
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.
Thank you for pointing this out, I updated this to use the appropriate naming conventions.
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.
The contribution is much appreciated! I have mostly minor comments and questions.
I suspect the "project fails validation: sync with base branch & run |
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!
I retagged everyone, but feel free to just take a look at the parts you commented on previously. I know this is a big and confusing PR. Plus, with 4 reviewers on this it could get hectic having too many cooks. Kevin's LGTM on the core functionality will be the primary indicator this is ready to merge 👍 Thanks for all the review and guidance on this one. |
This PR implements GSSAPI auth support for Windows as described by the spec. Similar to the Linux and Mac implementation in #1413, this one took longer than expected. In the end, it turned out that the
sspi
crate was not as amenable to this task as initially thought at scope time. I decided to attempt to implement this using the native Windows api (viawindows-sys
) and modeling it exactly off oflibmongoc
since I know the C implementation works. Of course, that results in a bit more C-style code than Rust-style code, but after several weeks of investigating and implementing, this is finally working.I updated the GSSAPI codepath to share common code between Linux/Mac and Windows as much as possible. I only separated out the platform-specific implementations and guarded each with the relevant
cfg
attributes.This PR also updates the e2e tests to run on Windows. I observed this implementation passing my local testing on a Windows host, and the e2e tests passing on this patch. I anticipate the evergreen patch associated with this PR to succeed off the bat, as well.