Skip to content

Support for .htpasswd Authentication (auth_plugin)#593

Open
d0tiKs wants to merge 2 commits intonovnc:masterfrom
d0tiKs:master
Open

Support for .htpasswd Authentication (auth_plugin)#593
d0tiKs wants to merge 2 commits intonovnc:masterfrom
d0tiKs:master

Conversation

@d0tiKs
Copy link

@d0tiKs d0tiKs commented Feb 12, 2025

This is a draft PR that implements authentication using a .htpasswd file database hosted on the server.
Is it connected to #590. It also allows to authenticate multiple users.

Changes:

  • Added a new HtpasswdAuth class that reads a .htpasswd file database specified by --auth-source.
  • Implemented password verification using the passlib library to handle hashed passwords.
  • Added bcrypt and passlib as a extra dependency in setup.py and test-requirements.txt.
  • Added tests that use the new mechanism

@CendioZeijlon
Copy link
Contributor

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 BasicHTTPAuth class.

@CendioZeijlon CendioZeijlon self-assigned this Feb 14, 2025
Copy link
Contributor

@CendioZeijlon CendioZeijlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@d0tiKs
Copy link
Author

d0tiKs commented Feb 26, 2025

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.

@CendioZeijlon
Copy link
Contributor

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 👍

@d0tiKs
Copy link
Author

d0tiKs commented Mar 2, 2025

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 v1.7.4 with the sources available here, seems to have been deserted, and libpass (it's confusing because the python module v1.9.0 is named libpass, but the repository is still passlib) looks maintained, that said an issue about if it's a takeover is opened without real answer.

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 bcrypt version to 4.0.1 or by using libpass.

@d0tiKs d0tiKs force-pushed the master branch 2 times, most recently from f2b3dc7 to 23d7211 Compare March 2, 2025 20:39
@CendioZeijlon
Copy link
Contributor

CendioZeijlon commented Mar 10, 2025

While looking around the documentation and implementing the change I found that passlib v1.7.4 with the sources available here, seems to have been deserted.

Good finding! It looks like maybe the fork was renamed to avoid name collisions.

It can be fixed by pinning the bcrypt version to 4.0.1 or by using libpass.

Since the libpass project is alive, I think you can go with that!

@d0tiKs
Copy link
Author

d0tiKs commented Mar 10, 2025

@ThinLinc-Zeijlon

it would be nice if it could support htpasswd fully

Which method should be used to determine the type of encryption of the database file :

  • An additional argument ?
  • Determining the type at runtime ?
  • Using a configuration file / environment variable ?
  • Using the default ?

It seems that the default encryption scheme is md5 from this documentation

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.

@CendioZeijlon
Copy link
Contributor

Which method should be used to determine the type of encryption of the database file :

  • An additional argument ?
  • Determining the type at runtime ?
  • Using a configuration file / environment variable ?
  • Using the default ?

It seems that the default encryption scheme is md5 from this documentation

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 passlib.apache.HtpasswdFile() and then verify the check_password()-method. See passlib docs.

@notypecheck
Copy link

notypecheck commented Mar 11, 2025

I think you need to install bcrypt extra here (libpass[bcrypt]), since I have removed built-in bcrypt backend because there's a well established implementation on pypi.

Copy link
Contributor

@CendioZeijlon CendioZeijlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things need to be looked at, but nicely done overall! 🙂

@d0tiKs d0tiKs changed the title Support for .htpasswd Authentication in BasicHTTPAuth (auth_plugin) Support for .htpasswd Authentication (auth_plugin) Mar 12, 2025
@CendioZeijlon
Copy link
Contributor

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 self.skipTest("...") in your test class if libpass can't be imported.

@CendioZeijlon
Copy link
Contributor

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.

@d0tiKs
Copy link
Author

d0tiKs commented Apr 22, 2025

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.
I squashed all my commits into one but I don't know if it was what you wanted. I can split it back into something in between if needed.

@CendioZeijlon
Copy link
Contributor

Well it was a first indeed.
I squashed all my commits into one, but I don't know if it was what you wanted. I can split it back into something in between if needed.

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. 😄

@CendioZeijlon
Copy link
Contributor

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 self.skipTest("...") in your test class if libpass can't be imported.

Have you looked anything at this?

@CendioZeijlon CendioZeijlon marked this pull request as ready for review May 6, 2025 14:16
@d0tiKs
Copy link
Author

d0tiKs commented May 6, 2025

Not yet, it slipped out of my mind, I'll check as soon as I have time.

@d0tiKs d0tiKs requested a review from CendioZeijlon May 8, 2025 14:00
Copy link
Contributor

@CendioZeijlon CendioZeijlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of minor changes, and then this is ready to be merged!

Also, squash the commits, otherwise we have a semi broken state before test-requirements.txt is updated.

Comment on lines +6 to +7
libpass;python_versions>="3.8"
bcrypt;python_versions>="3.8"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original passlib is available, so maybe it should say "libpass package is not available for this Python version" here, just to avoid confusion?

@CendioZeijlon
Copy link
Contributor

@d0tiKs Have you had time to look at the latest review comments?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants