HTPasswdAuth only works with clear text passwords

tardyp commented 7 years ago

https://irclogs.jackgrigg.com/out.pl?server=irc.freenode.net;channel=buildbot;date=2017-04-13 See discussion bw vincent- and tardyp

http://twistedmatrix.com/documents/current/api/twisted.cred.checkers.FilePasswordDB.html

Need to be able to configure the hash algorithm of the password so that twisted is able to check them.

gashev commented 6 years ago

Confirmed. HTPasswrdAuth doesn't work with MD5 and bcrypt encryptions and works with clear text passwords.

yan12125 commented 3 years ago

I have a proof-of-concept fix at https://github.com/yan12125/buildbot/commit/8c799425a13512d5da6eb2957b369e0dce1eaecb. Here are two questions about the design:

Hashed passwords in htpasswd files do not work with HTTP digest authentication as the latter needs to know the password in cleartexts to compute the hash HA1 [1]. I removed DigestCredentialFactory(), anyway. I think HTTP basic authentication is no longer a problem nowadays, assuming everyone moves from HTTP to HTTPS?
In the above commit, I only implement bcrypt and MD5 hashes. I omit other algorithms as Apache declares SHA1 and crypt as "insecure" [2]. Should I implement all algorithms for maximum compatibility?

[1] https://en.wikipedia.org/wiki/Digest_access_authentication [2] https://httpd.apache.org/docs/2.4/misc/password_encryptions.html

p12tic commented 3 years ago

@yan12125

Hashed passwords in htpasswd files do not work with HTTP digest authentication as the latter needs to know the password in cleartexts to compute the hash

Do I understand correctly that the DigestCredentialFactory item you removed was always broken?

yan12125 commented 3 years ago

Do I understand correctly that the DigestCredentialFactory item you removed was always broken?

I just give it a try with my Buildbot installation, and DigestCredentialFactory works with plain-text passwords in the htpasswd file.

If I understand Twisted correctly, here are how things work for the current (unpatched) Buildbot:

FilePasswordDB.requestAvatarId() calls DigestedCredentials.checkPassword() with the "password", as the hash parameter of FilePasswordDB is not set [1]. DigestedCredentials.checkPassword() then uses the received "password" to compute HA1 [2] and do remaining stuffs.
In FilePasswordDB, the "password" is anything after the delimiter : [3]. If that is a plain-text password, everything works as expected.

[1] https://github.com/twisted/twisted/blob/twisted-21.2.0/src/twisted/cred/checkers.py#L319 [2] https://github.com/twisted/twisted/blob/twisted-21.2.0/src/twisted/cred/credentials.py#L153 [3] https://github.com/twisted/twisted/blob/twisted-21.2.0/src/twisted/cred/checkers.py#L267

yan12125 commented 3 years ago

Oh, I noticed that Apache provides another utility htdigest for managing user files for digest authentication. Let me investigate it.

yan12125 commented 3 years ago

Here are some notes about enabling HTTP digest authentication in Buildbot with files generated by htdigest:

htdigest-generated files contain three fields - the username, the realm and the HA1 value, which is the result of MD5(username:realm:password)
Twisted's DigestCredential.checkHash() can be used to check credentials with pre-computed HA1 since long time ago [1]. However, nobody calls checkHash() directly, so some wrappers are needed. A possible approach is creating a class inherited from DigestCredential and override checkPassword() to call checkHash(), and then creating a new factory to use the inherited class for replacing DigestCredentialFactory.
The FilePasswordDB class only picks two fields from each line of the password file [2]. We need to extend it and override _loadCredentials() for handling 3-field htdigest files

However, there are some security implications inside HTTP digest authentication. Per specs, the hash function can be MD5, SHA-256 or SHA-512 [3]. However, neither Firefox [4] nor Chrome [5] supports SHA-256.

Due to security issues and non-trivial implementation efforts for HTTP digest authentication, I'd like to focus on only providing compatibility with older buildbot versions (<=0.8.x). In those versions, usernames and passwords are sent in plaintexts [6], so supporting only HTTP basic authenticating will not decrease the security level. For those who use HTPasswdAuth for HTTP digest authentication since 0.9.x, a big warning in release notes and/or buildbot checkconfig results may be enough.

As a side note, the HTPasswdAprAuth class in 0.8.x supports all hash algorithms supported by apr-util, the underlying library of the htpasswd utility. I will fill up algorithms not yet in my proof-of-concept patch if "for providing compatibility only" is the way to go.

[1] https://twistedmatrix.com/trac/ticket/2280 [2] https://github.com/twisted/twisted/blob/twisted-21.2.0/src/twisted/cred/checkers.py#L267 [3] https://en.wikipedia.org/wiki/Digest_access_authentication [4] https://bugzilla.mozilla.org/show_bug.cgi?id=472823 [5] https://bugs.chromium.org/p/chromium/issues/detail?id=1160478 [6] https://github.com/buildbot/buildbot/blob/v0.8.14/master/buildbot/status/web/authz.py#L169

p12tic commented 3 years ago

@yan12125 Thanks for explanation.

I wonder, what if we keep HTTP digest authentication support as is? It seems from your WIP code that it would work with plaintext passwords and we could just add a note to the documentation specifying this limitation. On Buildbot v4.0 we could drop support for digest auth altogether.

yan12125 commented 3 years ago

That sounds a possible way. Another idea comes to my mind: how about adding an option to HTPasswdAuth to choose between (plain-text passwords with digest & basic authentication) and (encrypted passwords with basic authentication)?

buildbot / buildbot

HTPasswdAuth only works with clear text passwords #3126