Edgewall Software
Modify

Opened 8 years ago

Closed 5 years ago

Last modified 5 years ago

#10975 closed enhancement (fixed)

Enhancement for the contrib/htpasswd.py script

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.

Attachments (1)

htpasswd.diff (3.7 KB ) - added by thomasbilk@… 8 years ago.
Patch for htpasswd.py

Download all attachments as: .zip

Change History (20)

by thomasbilk@…, 8 years ago

Attachment: htpasswd.diff added

Patch for htpasswd.py

comment:1 by Ryan J Ollos <ryan.j.ollos@…>, 8 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@…, 8 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, 8 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, 6 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, 6 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, 6 years ago

I'll add unit tests before committing the changes.

comment:7 by Ryan J Ollos, 5 years ago

Release Notes: modified (diff)

in reply to:  6 ; comment:8 by Ryan J Ollos, 5 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, 5 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, 5 years ago

API Changes: modified (diff)

comment:11 by Jun Omae, 5 years ago

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

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

comment:12 by Peter Suter, 5 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, 5 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, 5 years ago

Resolution: fixed
Status: assignedclosed

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

comment:15 by Jun Omae, 5 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, 5 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, 5 years ago

API Changes: modified (diff)

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

comment:18 by Ryan J Ollos, 5 years ago

Do we have any expectation that the modules in contrib (or at least htpasswd.py and htdigest.py) are "standalone"? The dependencies added to htpasswd require running in an environment with the egg installed: trunk/contrib/htpasswd.py@14023:20-22#L16

in reply to:  18 comment:19 by Ryan J Ollos, 5 years ago

Replying to rjollos:

Do we have any expectation that the modules in contrib (or at least htpasswd.py and htdigest.py) are "standalone"?

#8361 discusses a possible solution to this.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.