-
Notifications
You must be signed in to change notification settings - Fork 5
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
Docker based examples #8
base: master
Are you sure you want to change the base?
Conversation
Leaving this as a draft, as I want to look at it with a fresh set of eyes tomorrow before finalizing. Comments welcome at this stage :) |
Overall, this looks good. My main dislike is that it relies on some opaque docker image in the cloud, rather than just having a |
Well, any docker solution would involve layers pulled from the docker registry, at the very least up to the osixia/docker-openldap layer. The one I'm using in the script builds on that, but preconfigures the data... |
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'm not a maintainer on this project, but stumbled upon this PR while filing #9, so here we go.
// | ||
// This particular filter allows the user to sign in with either | ||
// uid or email | ||
let filter = format!("(|(uid={})(mail={}))", who, who); |
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 vulnerable to LDAP injection. It's actually a problem of the library since it does not provide the proper tools for escaping, so I opened #9 to track this issue.
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 don’t think the C LDAP library provided this functionality, and since this is a thin wrapper around it it was never present. The C library likely assumed that you escaped your queries yourself, as I did when I was using it for the project the fork was made for.
That being said, I do not think it’s a bad idea to add that ability if there is a standardized way to perform it and it is not required to be done if you can prove that your queries are otherwise ‘safe’.
if let Ok(fry_dn) = ldap_dn_lookup(&ldap, user_to_authenticate.as_str()) { | ||
// Now, perform a bind with the DN we found matching the user attempting to sign in | ||
// and the password provided in the authentication request | ||
do_simple_bind(&ldap, fry_dn.as_str(), pwd_to_authenticate.as_str()).unwrap(); | ||
|
||
println!("Successfully signed in as fry"); | ||
} |
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 vulnerable to a timing side-channel attack. An attacker that does not have a valid set of credentials should not be able to learn which user accounts exist. However, if the attacker tries to authenticate with a bunch of credentials, they may notice that certain authentication attempts take longer than others. That's because the second simple-bind is only performed if ldap_dn_lookup
returns Some
. Therefore, the attacker can infer that a long request duration means that the username exists, even if the password is still wrong. This allows the attacker to perform a brute-force search for valid credentials much more efficiently.
Since this is a very common problem with authentication code, the example should demonstrate how to do it more safely. The basic idea is that you do the second simple-bind even if ldap_dn_lookup
does not return any results. But you set a flag to remind yourself that you have to ignore the result of that second simple-bind. Here's an example from one of my programs how that looks (although in Go, not in Rust): https://github.com/majewsky/alltag/blob/df161b55fa4c7eba0abec82d2cf0df34e49b0ad4/internal/auth/ldap.go#L96-L115
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 catch! I'll see if I get time to update the example this weekend!
…ping, and to protect against timing attacks for user discovery
I created an example on how to authenticate with simple bind. Panning to add one on authorization at a later stage, but for now I think this will serve as a good intro.