Edgewall Software

Opened 10 years ago

Closed 7 years ago

Last modified 7 years ago

#10975 closed enhancement (fixed)

Enhancement for the contrib/htpasswd.py script — at Version 17

Reported by: thomasbilk@… Owned by: Ryan J Ollos
Priority: normal Milestone: 1.1.5
Component: contrib Version: 1.0dev
Severity: trivial Keywords: htpasswd
Cc: Branch:
Release Notes:

htpasswd.py prompts user for password when the password is not specified as an argument.

API Changes:
  • trac.util.compat provides an import of the crypt.crypt method that will fallback to fcrypt module when crypt module is not available. None is returned if fcrypt is not available.
  • trac.util provides the salt function for use with cryptographic hash functions.
Internal Changes:

Description

I have updated the htpasswd.py from contrib.

Change History (18)

by thomasbilk@…, 10 years ago

Attachment: htpasswd.diff added

Patch for htpasswd.py

comment:1 by Ryan J Ollos <ryan.j.ollos@…>, 10 years ago

It would be helpful if you described how you've updated and enhanced the script. That is generally considered good practice when creating a ticket and submitting a patch.

comment:2 by thomasbilk@…, 10 years ago

Oh yes. Sorry. So what I did was that:

  • Improve the salt method by adding an optional parameter length and using two constants from the string package
  • Improve the load and save methods of the HtpasswdFile class by using the with statement for proper closing of the file
  • Rename the -b parameter to -a for adding new users to the file and changing the deletion of users to -d (lower case)
  • Improved the adding of users by prompting for the password in a save way instead of reading it as a command line arg

comment:3 by Christian Boos, 10 years ago

Keywords: htpasswd verify added
Milestone: next-dev-1.1.x
Owner: set to Christian Boos
Status: newassigned
Version: 1.0dev

in reply to:  2 comment:4 by Ryan J Ollos, 7 years ago

Milestone: next-dev-1.1.x1.1.5
Owner: changed from Christian Boos to Ryan J Ollos

Replying to thomasbilk@…:

Oh yes. Sorry. So what I did was that:

  • Improve the salt method by adding an optional parameter length and using two constants from the string package
  • Improve the load and save methods of the HtpasswdFile class by using the with statement for proper closing of the file

Those changes look good.

  • Rename the -b parameter to -a for adding new users to the file and changing the deletion of users to -d (lower case)

Those options mirror the options for the htpasswd command line tool, so we should keep them as-is for consistency.

$ htpasswd
Usage:
	htpasswd [-cimBdpsDv] [-C cost] passwordfile username
	htpasswd -b[cmBdpsDv] [-C cost] passwordfile username password

	htpasswd -n[imBdps] [-C cost] username
	htpasswd -nb[mBdps] [-C cost] username password
 -c  Create a new file.
 -n  Don't update file; display results on stdout.
 -b  Use the password from the command line rather than prompting for it.
 -i  Read password from stdin without verification (for script usage).
 -m  Force MD5 encryption of the password (default).
 -B  Force bcrypt encryption of the password (very secure).
 -C  Set the computing time used for the bcrypt algorithm
     (higher is more secure but slower, default: 5, valid: 4 to 31).
 -d  Force CRYPT encryption of the password (8 chars max, insecure).
 -s  Force SHA encryption of the password (insecure).
 -p  Do not encrypt the password (plaintext, insecure).
 -D  Delete the specified user.
 -v  Verify password for the specified user.
On other systems than Windows and NetWare the '-p' flag will probably not work.
The SHA algorithm does not use a salt and is less secure than the MD5 algorithm.
  • Improved the adding of users by prompting for the password in a save way instead of reading it as a command line arg

It could be okay to prompt for a password when the user hasn't specified it.

comment:5 by Ryan J Ollos, 7 years ago

Keywords: verify removed

Proposed changes in log:rjollos.git:t10975. The changes make htpasswd.py behave more like htpasswd.

comment:6 by Ryan J Ollos, 7 years ago

I'll add unit tests before committing the changes.

comment:7 by Ryan J Ollos, 7 years ago

Release Notes: modified (diff)

in reply to:  6 ; comment:8 by Ryan J Ollos, 7 years ago

Replying to rjollos:

I'll add unit tests before committing the changes.

It would be nice to have tests since I've already discovered a minor issue with the proposed changes. Also, I'm considering some refactoring of htdigest.py. However, should the tests in contrib be invoked from trac.test? contrib is not a package and I haven't explored whether that will effect the ability to easily write a tests package in contrib/tests. Or perhaps it makes more sense to just have test modules in a tests directory, rather than a tests package.

A more drastic approach would be to move htpasswd.py and htdigest.py from contrib into the trac package.

in reply to:  8 comment:9 by Ryan J Ollos, 7 years ago

Replying to rjollos:

A more drastic approach would be to move htpasswd.py and htdigest.py from contrib into the trac package.

Perhaps trac.auth could be a new package? I'm considering integrating some AccountManagerPlugin functionality in Trac 1.3.x, and will think about revisiting this.

For now, latest proposed changes in log:rjollos.git:t10975.2.

comment:10 by Ryan J Ollos, 7 years ago

API Changes: modified (diff)

comment:11 by Jun Omae, 7 years ago

Undefined name 'self' is used in trac/util/compat.py.

     except ImportError:
-        self.crypt = None
+        crypt = None

comment:12 by Peter Suter, 7 years ago

In salt(), ascii_letters + digits does not include / and . while in the previous set 0123456789/. they were included. Was that intentional?

comment:13 by Ryan J Ollos, 7 years ago

According to multiple sources in a Google search, including here, valid characters include . and /. It was an oversight that they were removed. I'll add back. Thanks for noticing.

comment:14 by Ryan J Ollos, 7 years ago

Resolution: fixed
Status: assignedclosed

Committed to trunk in [14014:14015] after fixing noted issues.

comment:15 by Jun Omae, 7 years ago

Sorry, I missed the following issue. When fcrypt isn't available on Windows, tracd with basic auth option crashes.

C> python.exe -c "import fcrypt"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ImportError: No module named fcrypt

C> python.exe trac/web/standalone.py --basic-auth="*,C:/usr/var/trac/htpasswd.txt,auth" -s C:/usr/var/trac/1.1.5dev
Traceback (most recent call last):
  File "trac/web/standalone.py", line 341, in <module>
    main()
  File "trac/web/standalone.py", line 236, in main
    options, args = parser.parse_args()
  File "C:\usr\apps\python26\lib\optparse.py", line 1394, in parse_args
    stop = self._process_args(largs, rargs, values)
  File "C:\usr\apps\python26\lib\optparse.py", line 1434, in _process_args
    self._process_long_opt(rargs, values)
  File "C:\usr\apps\python26\lib\optparse.py", line 1509, in _process_long_opt
    option.process(opt, value, values, self)
  File "C:\usr\apps\python26\lib\optparse.py", line 788, in process
    self.action, self.dest, opt, value, values, parser)
  File "C:\usr\apps\python26\lib\optparse.py", line 808, in take_action
    self.callback(self, opt, value, parser, *args, **kwargs)
  File "trac/web/standalone.py", line 136, in _auth_callback
    auths[env_name] = cls(os.path.abspath(filename), realm)
  File "C:\usr\src\trac\trac.git\trac\web\auth.py", line 303, in __init__
    self.crypt = crypt.crypt
AttributeError: 'NoneType' object has no attribute 'crypt'

I think it would be simple to import crypt method rather than crypt module.

  • contrib/htpasswd.py

    diff --git a/contrib/htpasswd.py b/contrib/htpasswd.py
    index 7fa2aee..9e3e89e 100755
    a b class HtpasswdFile:  
    5555
    5656    def update(self, username, password):
    5757        """Replace the entry for the given user, or add it if new."""
    58         pwhash = crypt.crypt(password, salt())
     58        pwhash = crypt(password, salt())
    5959        matching_entries = [entry for entry in self.entries
    6060                            if entry[0] == username]
    6161        if matching_entries:
  • trac/util/compat.py

    diff --git a/trac/util/compat.py b/trac/util/compat.py
    index 9ddfa31..f242c17 100644
    a b from trac.util.text import cleandoc  
    2525
    2626# Windows doesn't have a crypt module by default.
    2727try:
    28     import crypt
     28    from crypt import crypt
    2929except ImportError:
    3030    try:
    31         import fcrypt as crypt
     31        from fcrypt import crypt
    3232    except ImportError:
    3333        crypt = None
    3434
  • trac/web/auth.py

    diff --git a/trac/web/auth.py b/trac/web/auth.py
    index 88ee297..4e73309 100644
    a b class BasicAuthentication(PasswordFileAuthentication):  
    300300    def __init__(self, htpasswd, realm):
    301301        # FIXME pass a logger
    302302        self.realm = realm
    303         self.crypt = crypt.crypt
     303        self.crypt = crypt
    304304        PasswordFileAuthentication.__init__(self, htpasswd)
    305305
    306306    def load(self, filename):

comment:16 by Ryan J Ollos, 7 years ago

I should have tested on Windows, but I don't have easy access to a Windows system this weekend. Would you mind pushing the change?

comment:17 by Jun Omae, 7 years ago

API Changes: modified (diff)

Okay. I've pushed the changes in [14023].

Note: See TracTickets for help on using tickets.