-
Notifications
You must be signed in to change notification settings - Fork 31
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 imap authenticator #20
base: master
Are you sure you want to change the base?
Conversation
def hashString(s): | ||
"""Helper function to convert strings to a fixed integer""" | ||
result = 0 | ||
hash = hashlib.md5(s.encode('utf8')).digest() |
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.
There are probably better hash algorithms with a severely less chance of collisions. I’d prefer one of those.
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.
Which would be one of those? Remember that the user id must not be too long, otherwise it won't work (see line 369). Or has there been a change with user id length in the latest (really great) release?
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.
A sha algorithm for example. Not sure what python provides in its standard lib.
But I’m not even sure how the conversion/reduction to int will interact with that. Without doing some risk analysis/probability reasoning I think it’s probably better to introduce an explicit user mapping.
I am willing to accept this with a strong warning in the description/header.
I’d feel more comfortable with some idea of how likely collisions are though. If it is very unlikely the warning will not have to be as strongly worded.
return (AUTH_REFUSED, None, None) | ||
|
||
# As IMAP hasn't got such thing as a USERID, we use the given name to calculate a hash | ||
# TODO this might be highly insecure when two mails have coindently the same hash (note that we are cutting the integer off as otherwise we would get an error from mumble) |
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.
As this is a TODO, how would one fix it?
I think this is a technical limitation that is unfixable.
So I would rather put this into the scripts description in the file header as a strong caveat/warning.
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 think that's better, yes.
from https://github.com/dadosch/mumble_auth_imap