[Twisted-Python] twisted.cred interface deficiences
The twisted.cred.IUsernamePassword interface declares: * IUsernamePassword.username - "The username associated with these credentials." * IUsernamePassword.password - "The password associated with these credentials." * IUsernamePassword.checkPassword(password) - "Validate these credentials against the correct password." The issue is that the interface (according to exarkun) allows you to implement checkPassword() to do things other than the obvious "password == self.password". Now, this is an issue because Twisted then explicitly supports (again, according to exarkun) two different uses of this interface by the credentials checker: * Call the checkPassword() method, passing it the correct password * Just take the password out and do whatever you want with it (which is necessary in any secure system) Now, imagine I write a version of checkPassword() in a library which does something security-centric (what would this be? shouldn't it be part of the checker?), assuming that it'll be used by a credentials checker which calls checkPassword(). Except... then, a library user uses it with a credentials checker which checks the password itself. Now they're skipping over my security-centric code! So I have to tell my library users that they have to use my library with a credentials checker which makes sure to call checkPassword(), not just one which accepts the correct interface. IUsernamePassword's docstring claims that the stored password must be reversible to plaintext to be compared with the password, which implies that taking the password out and doing other things is incorrect, unlike what exarkun suggests. In this case, exposing password in the interface makes little sense. In addition, twisted.cred.checkers.FilePasswordDB apparently ignores this docstring entirely already (http://twistedmatrix.com/trac/browser/tags/releases/twisted-12.3.0/twisted/c...). I propose that IUsernamePassword should be split into at least two interfaces: * IUsernamePassword, with only username and password, no methods, which allows password to be used in any way * Another interface, which only defines username and checkPassword() - possibly just a rename of IUsernameHashedPassword, which declares a similar interface However, this has the issue that any credential checker which can use the second interface would also be able to use an IUsernamePassword here if there were an adapter between the two, but support for this would have to go into every credential checker which supports the second interface at present. Maybe the Portal could automatically search for adapters if it can't find a direct match? Thanks, Cameron
It actually might be the appropriate thing already. There's a couple of possible reasons for renaming; one is that the password might not be hashed but the credentials object wants to insert additional logic (exarkun's statement in IRC) anyway, but technically that's just hashing using the identity function, so it should be OK. The other is that the credentials object might want to collect the plaintext password, and perform some logic based on that *before* collecting something which can be compared to something derived from the plaintext password from the other end of a protocol. I don't know of any protocols which do this off the top of my head, but I have a suspicion that at least a couple exist, and they'd fit perfectly into this interface. If none do, then keeping it with the same name sounds fine to me. Cameron On 1 April 2013 22:12, Laurens Van Houtven <_@lvh.cc> wrote:
On Apr 1, 2013, at 2:34 PM, Shell <cam.turn@gmail.com> wrote:
These don't sound like bad ideas, but I think that if you're going to try to fix cred's built-in credentials, this is a pretty labor-intensive and not particularly rewarding path to go down. Further refining the sad username+password credential interface will provide only some internal factoring improvements to existing types of authentication, at the cost of retrofitting all of them; not to mention dealing with broad-spectrum deprecations. A better use of energy would be directed at getting a generic SASL credentials implementation; in other words, fixing this fairly ancient ticket: <https://twistedmatrix.com/trac/ticket/2015>. A well-implemented SASL credentials interface will address these issues, as well as allowing for proper challenge-response authentication, digest auth, external auth mechanisms like TLS, et cetera. Your idea about adapters is well taken though; having the portal do that when SASL-ified checkers are available seems reasonable, provided that it won't break anything. -glyph
On Tue, Apr 2, 2013 at 6:04 PM, Glyph <glyph@twistedmatrix.com> wrote:
For what it's worth, I worked on this back when, and while I don't have time to work on it right now (and probably won't for at least a couple of weeks), I'm happy to answer questions, if anybody has them. Mostly what needed to happen was to merge the code in the branch with the code in trunk which had diverged somewhat, and create some documentation on how to use them. My recollection was that integrating the results of that with specific protocols (and possibly with Cred using a "higher level" interface) would happen on a subsequent ticket, which is probably a good idea.
-- Kevin Horn
It actually might be the appropriate thing already. There's a couple of possible reasons for renaming; one is that the password might not be hashed but the credentials object wants to insert additional logic (exarkun's statement in IRC) anyway, but technically that's just hashing using the identity function, so it should be OK. The other is that the credentials object might want to collect the plaintext password, and perform some logic based on that *before* collecting something which can be compared to something derived from the plaintext password from the other end of a protocol. I don't know of any protocols which do this off the top of my head, but I have a suspicion that at least a couple exist, and they'd fit perfectly into this interface. If none do, then keeping it with the same name sounds fine to me. Cameron On 1 April 2013 22:12, Laurens Van Houtven <_@lvh.cc> wrote:
On Apr 1, 2013, at 2:34 PM, Shell <cam.turn@gmail.com> wrote:
These don't sound like bad ideas, but I think that if you're going to try to fix cred's built-in credentials, this is a pretty labor-intensive and not particularly rewarding path to go down. Further refining the sad username+password credential interface will provide only some internal factoring improvements to existing types of authentication, at the cost of retrofitting all of them; not to mention dealing with broad-spectrum deprecations. A better use of energy would be directed at getting a generic SASL credentials implementation; in other words, fixing this fairly ancient ticket: <https://twistedmatrix.com/trac/ticket/2015>. A well-implemented SASL credentials interface will address these issues, as well as allowing for proper challenge-response authentication, digest auth, external auth mechanisms like TLS, et cetera. Your idea about adapters is well taken though; having the portal do that when SASL-ified checkers are available seems reasonable, provided that it won't break anything. -glyph
On Tue, Apr 2, 2013 at 6:04 PM, Glyph <glyph@twistedmatrix.com> wrote:
For what it's worth, I worked on this back when, and while I don't have time to work on it right now (and probably won't for at least a couple of weeks), I'm happy to answer questions, if anybody has them. Mostly what needed to happen was to merge the code in the branch with the code in trunk which had diverged somewhat, and create some documentation on how to use them. My recollection was that integrating the results of that with specific protocols (and possibly with Cred using a "higher level" interface) would happen on a subsequent ticket, which is probably a good idea.
-- Kevin Horn
participants (5)
-
exarkun＠twistedmatrix.com -
Glyph -
Kevin Horn -
Laurens Van Houtven -
Shell