Support for .htpasswd Authentication (auth_plugin)#593
Support for .htpasswd Authentication (auth_plugin)#593d0tiKs wants to merge 2 commits intonovnc:masterfrom
Conversation
|
Nice idea! I think it would be better to have a separate auth plugin for handling auth against a password-db-file, so that your code doesn't have to be intermingled with the code in the |
CendioZeijlon
left a comment
There was a problem hiding this comment.
Since you are indicating that this should/could use htpasswd as a db-backend, it would be nice if it could support htpasswd fully, and not just bcrypt.
A dependency to passlib would help implement all the different kinds of encryption that htpasswd can use. Which are plaintext, md5, sha1, sha256, sha512, bcrypt and crypt. The check could also be implemented more manually. I think in all cases (including bcrypt which I used so far) the requirements of their respective license may apply, I think they still applies even if it is just a requirement and not a redistribution but I far from an expert on the subject. The package passlib use a custom license and bcrypt is using Apache2.0 for example. |
Passlib sounds like a great alternative. The license is OK to use 👍 |
While looking around the documentation and implementing the change I found that passlib I checked the repo because I have a warning when using the module with the latest version: (trapped) error reading bcrypt version
Traceback (most recent call last):
File "/usr/local/lib/python3.9/site-packages/passlib/handlers/bcrypt.py", line 620, in _load_backend_mixin
version = _bcrypt.__about__.__version__
AttributeError: module 'bcrypt' has no attribute '__about__'It can be fixed by pinning the |
f2b3dc7 to
23d7211
Compare
Good finding! It looks like maybe the fork was renamed to avoid name collisions.
Since the libpass project is alive, I think you can go with that! |
|
@ThinLinc-Zeijlon
Which method should be used to determine the type of encryption of the database file :
It seems that the default encryption scheme is From what I gathered to stay inlined with the rest, it should be an additional argument, it entails to modify other files so I prefer to be sure of the proper method to follow. |
I don't think you have to do any check for which encryption is used. A htpasswd-file can contain multiple passwords with different encryption algorithms. Libpass (and htpasswd) determines which algorithm to use from a header string in the stored password hashes. From your class, it should be enough to open the htpasswd-file with |
|
I think you need to install |
CendioZeijlon
left a comment
There was a problem hiding this comment.
A few things need to be looked at, but nicely done overall! 🙂
|
Alright, got side tracked a bit there! 😄 Since the dependencies for your code are optional, I think it's fine to skip your tests for Python <= 3.8. You can look at an older test-requirements.txt for how to make libpass download depend on Python version. Then you also need to skip your tests, e.g. calling |
|
Can you also look at squishing some of your commits to make the git history cleaner? If you haven't done this before, lookup git's interactive rebase. |
Well it was a first indeed. |
Thanks! In this case, I think squishing everything into one commit works well, since you are adding one new feature with a separate class. When you get used to it, interactive rebases and force pushing the new and updated truth, is actually a quite nice way of working with PRs. 😄 |
Have you looked anything at this? |
|
Not yet, it slipped out of my mind, I'll check as soon as I have time. |
| libpass;python_versions>="3.8" | ||
| bcrypt;python_versions>="3.8" |
There was a problem hiding this comment.
A couple of things here:
python_versions->python_version- Libpass supports Python 3.9 and onward?
libpass;python_version>="3.9"
bcrypt;python_version>="3.9"
| except ImportError: | ||
| PASSLIB_AVAILABLE = False | ||
|
|
||
| @unittest.skipUnless(PASSLIB_AVAILABLE, "passlib package is not available") |
There was a problem hiding this comment.
The original passlib is available, so maybe it should say "libpass package is not available for this Python version" here, just to avoid confusion?
|
@d0tiKs Have you had time to look at the latest review comments? |
This is a draft PR that implements authentication using a
.htpasswdfile database hosted on the server.Is it connected to #590. It also allows to authenticate multiple users.
Changes:
HtpasswdAuthclass that reads a.htpasswdfile database specified by--auth-source.setup.pyandtest-requirements.txt.