Opened 12 years ago
Last modified 8 years ago
#4398 enhancement new
PBServerFactory should support plain password credentials
Reported by: | victorlin | Owned by: | Louis |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | pb | Keywords: | |
Cc: | Louis, Grindaizer | Branch: | |
Author: |
Description
Due to the authentication method build-in in PBServerFactory, it is impossible to login with a plain password like this: factory.login(credentials.UsernamePassword("victor", "abc")). Therefore, I can't use a hashed password file at server side. Like this one: checkers.FilePasswordDB(htpasswordFile, hash=hashFunction). The server will said that there is no checker for the credentials.
Surely, it is not secure to send a plain password over internet, but how about through SSL? I think the PBServerFactory should support both plain and hashed authentication methods. Otherwise, it is also insecure to store passwords as plain text on server side.
Attachments (3)
Change History (18)
comment:1 Changed 12 years ago by
Resolution: | → invalid |
---|---|
Status: | new → closed |
comment:2 Changed 12 years ago by
While this ticket is invalid, I'm slightly curious why the submitter didn't immediately figure out the right way to do this with PB. Maybe there's a doc bug lurking here?
comment:3 Changed 11 years ago by
Owner: | Glyph deleted |
---|
Changed 10 years ago by
Attachment: | IUsernamePassword_over_pb.patch added |
---|
allow to defined custom hash metod to PB authentication.
Changed 10 years ago by
Attachment: | pbHashMethod.tar.gz added |
---|
example wich custom hash method and FilePasswordBD
comment:4 Changed 10 years ago by
Resolution: | invalid |
---|---|
Status: | closed → reopened |
Hi to all.
Replying to exarkun:
You can support any additional credentials types you want via a PBServerFactory subclass. I suggest implementing the scheme you want and then, if it still seems to make sense as a general extension to PB, re-proposing.
We've faced almost the same problem recently, and i wanted to submit a patch which could also resolve the problem described here.
Actually the solution suggested here is to allow plain text to be transmitted across the network, but it's not what we've found the most interesting. In deed, most of the application that deal with authentication have two main requirements:
- The password should never be plain text when it cross the network.
- The password should never be stored as plain text
And most of the time password hash algorithm and storage format is specific to application. When we look at pb code we see that password hashing behaviour is defined by two function respond and challenge which are always the same.
So the idea here is to allow user to define their own "hashing" behaviour. (a kind of custom respond and challenge functions).
The password will still be transmitted hashed, but as pb hash the password is the same way the application does, identification procedure will be possible without having the plain text password on server side.
ex: (complete example attached below).
server side: define hashMetod and challenge on checker.
from common import MD5doubleHash class PBFilePasswordChecker(checkers.FilePasswordDB): implements(pb.IChallenger) def challengeFor(self, username): try: u, p = self.getUser(username) except KeyError: return str(len(username)) _, salt = p.split('$$') return salt def hashMethod(self, salt, secret): return MD5doubleHash(salt, secret)
client side: define the same hashMethod on credentials class
!#python from common import MD5doubleHash class PBUsernamePassword(credentials.UsernamePassword): implements(pb.IHashMethod) def hashMethod(self, salt, secret): return MD5doubleHash(salt, secret)
Changed 10 years ago by
Attachment: | IUsernamePassword_over_pb2.patch added |
---|
Patch allowing to use custom hash method for PB authentication. Improves IUsernamePassword_over_pb.patch
comment:5 Changed 10 years ago by
Component: | core → pb |
---|---|
Keywords: | review added |
Patch
I improved Grindaizer's patch in several ways.
- It is backward compatible, as class
spread.pb._PortalAuthChallenger
still implementsspread.pb.IUsernameMD5Password
. - It separates two hash methods, which have no obligation to be the same: the method used to hash the password (crypt(3) for Unix shadow password, for example), which we will call password hash, and the method used during the challenge/response step of authentication, which we will call challenge hash. With this implementation, a default challenge hash is given, but both password and challenge hashes may be redefined, and they can be different.
- It separates the two salts used with the two hash methods. The first one (challenge salt) is a randomly generated each time a client tries to authenticate; the second one (password salt) is always the same for a given hashed password. With my implementation, the two salts are different: the challenge salt is still randomly generated, while a method must be defined, which generate the password salt from the username and the hashed password (depending on the password hash used, the salt may depends on the username, the hashed password, or both).
- More tests are provided.
See the patch.
How does this work now?
One can define a credendial and a checker that implements a custom hash method to authenticate users.
Here is a sequence diagram that explains what happens.
Client knows a username, Server knows the hashed password, a password, and the challenge a method getSalt that, given and password hash methods username and hashed password, returns the salt, and the challenge hash method. Client Server ====== ====== | "I want to authenticate as" username | |--------------------------------------------------------->| | | | challenge, salt = getSalt(username, hashedPassword) | |<---------------------------------------------------------| | | | challengeHash(challenge, passwordHash(password, salt)) | |--------------------------------------------------------->| Now, server only has to compare that what it got from the client is equal to challengeHash(challenge, hashedPassword)
Example
Here is an example of credential, which uses the default challenge hash, and defines a very easy hash method. We note that on this side, we do not need to specify how the salt is defined, since the salt is given by the server.
class DummyCredential(pb.UsernameHashPassword): """A dummy credential.""" def pwHashMethod(self, secret, salt): return secret + salt
On the other side, here is the corresponding checker. It is a subclass of InMemoryUsernamePasswordDatabaseDontUse
, in which passwords are stored, hashed used the dummy hash method we gave above. Here, the salt is the length of the username.
class DummyChallengerChecker(checkers.InMemoryUsernamePasswordDatabaseDontUse): implements(checkers.IChallenger) def __init__(self, **users): hashed = dict([(user, users[user] + str(len(user))) for user in users]) return checkers.InMemoryUsernamePasswordDatabaseDontUse(**hashed) def challengeHashMethod(self, challenge, data): return pb.challengeHash(challenge, data) def getSalt(self, username): return str(len(username))
Default
I also added two default credentials, which I think might be the most used ones: spread.pb.UsernameHashlibPassword
and spread.pb.UsernameCryptPassword
, which use respectively hash method from the Python hashlib library, and crypt.
Documentation?
If this patch is accepted, I offer to write some documentation about it. That is, some page which would fit in the Perspective Broker documentation, and would include my beautiful sequence diagram and explain how to build and use these credentials and checkers.
comment:6 Changed 10 years ago by
Cc: | Louis added |
---|
comment:7 follow-up: 8 Changed 10 years ago by
It appears that the algorithm described doesn't require the client to actually have the original password, merely the hashed password. Which means that the storing the hashed passwords on the server is no more secure than storing the plain-text passwords. Given that, it seems that using this is just going to give users a false sense of security.
comment:8 Changed 9 years ago by
Replying to tom.prince:
It appears that the algorithm described doesn't require the client to actually have the original password, merely the hashed password. Which means that the storing the hashed passwords on the server is no more secure than storing the plain-text passwords. Given that, it seems that using this is just going to give users a false sense of security.
I agree, this is a flaw. Although I would still say that this is an improvement compared to the original version, given that the plain password is not needed on the server side (as a system administrator, I do not like to be able to read my users' passwords).
An improved solution is given here. Using this algorithm, the hash stored on the server side is not sufficient to log in.
If you consider it safe, I can implement it (although I do not have that much available time these days, so it might take a few weeks to be done).
comment:9 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Louis |
Status: | reopened → new |
I'm not a security expert, so I don't have enough knowledge, or confidence to analyze the safety of the proposed solution. The linked algorithms don't appear to be standard, and don't have any references to external analysis of the security of the protocol.
Any proposed patch implementing a security protocol like this should contain sufficient documentation of the security implications, that somebody can comfortably asses both the implementation and the algorithms. This is particularly true for custom protocols like PB, that aren't standardized, and don't have alternative implementations.
comment:10 Changed 9 years ago by
Cc: | Grindaizer added |
---|
comment:11 follow-up: 14 Changed 8 years ago by
Hello, I worked on this issue almost two years ago, and this problem has been in my mind since then. I have some time right now (and the next few days), and I will have some more next summer (starting mid-June). Here is a proposal to address this issue. It would be great if it was reviewed in the next couple of months, so that I can implement it next summer.
The Salted Challenge Response Authentication Mechanism (SCRAM) seems to be a reliable authentication mechanism that solves this issue: it is a challenge authentication scheme, allowing one to authenticate to a server, given that the server only stores a hash of the password that is not sufficient to authenticate. I offer to (try to) implement it in Twisted, which means:
- implement SCRAM in Twisted so that one can authenticate using PB with a challenge-response authentication scheme, where only hashed passwords are stored by the server;
- one of my goals is to be able to authenticate against the Unix /etc/passwd (or /etc/shadow);
- write some tests;
- ensure that it is retro-compatible;
- write some documentation (if it is relevant, and if I am able to do so as a non-native English speaker).
SCRAM is implemented by Python library Passlib (compatible with Python 2.6, 2.7 and 3), so it migth be easier to use this.
If you agree, I should start working on it next summer.
comment:12 Changed 8 years ago by
Keywords: | review added |
---|
comment:13 Changed 8 years ago by
Owner: | Louis deleted |
---|
comment:14 follow-up: 15 Changed 8 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Louis |
Replying to spalax:
Sorry, did you mean to post new code? I'm not clear on why you put the review keyword back on this ticket, unless you forgot to post a new patch or wanted a review of your plan.
Regarding your plan, I'm not intimately familiar with PB, but it seems like implementing SCRAM would be changing fundamentals of PB's implementation. How exactly do you plan to fit SCRAM's client/server authentication messages into PB?
comment:15 Changed 8 years ago by
Replying to habnabit:
Replying to spalax:
Sorry, did you mean to post new code? I'm not clear on why you put the review keyword back on this ticket, unless you forgot to post a new patch or wanted a review of your plan.
I followed the documentation, and, as this is not a small fix, I described my idea before starting to write some code. The 'review' keyword was a bit too much; I thought I had read that I had to put it for this case, but I cannot find any reference to it. Sorry.
Regarding your plan, I'm not intimately familiar with PB, but it seems like implementing SCRAM would be changing fundamentals of PB's implementation. How exactly do you plan to fit SCRAM's client/server authentication messages into PB?
The last time I digged into PB is more than one year ago, but, if you look at the (bad) patch I submitted earlier, I implemented some "conversation" between server and client to authentify client. So I guess I can do the same with SCRAM.
The only problem is that I seem to have looked too fast at the passlib library: it does not implement the whole SCRAM protocol. I could not find a complete implementation of the SCRAM protocol in Python. This would mean that if I want to carry on with my patch, I would have to implement it. Unless I find a simpler yet robust way for a client to authenticate to a server, transmitting only a hash of the password.
This is not accurate. You can log in using a UsernamePassword.
This doesn't follow. When you log in with
UsernamePassword
(which is allowed), PB actually uses a challenge-response negotiation which does not transmit the cleartext password to the server. This is why you can't use a hashing function in the checker: because the password has already been hashed by the client.You can support any additional credentials types you want via a PBServerFactory subclass. I suggest implementing the scheme you want and then, if it still seems to make sense as a general extension to PB, re-proposing.