[patch] HtPasswdStore changes ownership of htpasswd file (due to bad file IO in Python fileinput used here)

[Dav Clark]

TSSIA! But for completeness:

First off, you could kind of call this a bug in python's fileinput.input( ... , inplace=True)... this actually moves the existing file to <filename>.bak and then writes a new file. This has the additional effect of deleting obviously named backup files that might be living there! At a minimum, I'd say you should supply a less likely argument to fileinput.input's backup argument!

That's really screwed me over once now! The plugin should definitely not be deleting such files!

When I manage an htpasswd file with AccountManagerPlugin, it changes group and user to whatever that process is running as. This is a big problem for shared hosting, where for example, an htpasswd file needs to be readable by the shared apache server. This server might be running under something like an "apache" group, but then this plugin changes the group to my personal user's default group.

Anyway, I've attached a diff against the htfile.py which uses more "old-school" file IO. I'm using it now without troubles. It could certainly be improved, and doesn't actually handle as many corner cases (e.g. duplicate entries) as the old version. But it's a step in the right direction, and I'm not personally worried about such corner cases!

[anonymous]

I'm revisiting fixing the problems that my attached solution has and I've come up with a much simpler solution (conceptually). Specifically, if a file is opened in mode 'w', it is automatically truncated to empty, but otherwise retains unix permissions and ownership. So, I'd recommend the following model (and this is more or less what I'm using in my existing code):

--- password_lines = open('trac.htpasswd').readlines()

(changed, password_lines) = process(password_lines) # i.e. change a password or delete a line or add a line

if changed:

open('trac.htpasswd', 'w').writelines(password_lines)

---

You should of course break things out and trap file IO exceptions, but this allows you to do a quick proof of concept in ipython (if that's your fancy).

Thanks! Dav

[Dav Clark]

A feature identical diff for htfile.py, apart from fixing this bug

[Steffen Hoffmann]

Effects confirmed by own tests even with trunk version. Sorry that we where unable to react on this issue for such a long time.

Since this is a DoS for shared use as mentioned in the report, I'll escalate this bug now.

BTW, thanks for the comments inside second patch file. I appreciate them a lot and will try to improve trunk code along these lines.

Q: While Trac is at 0.12 right now, how urgent would it be to back-port to 0.10 branch these days?

[Steffen Hoffmann]

preview on suggested fix based on last patch

[Steffen Hoffmann]

I've tested the proposed solution and made it the core of a changeset, that I've tested successfully so far and would like to commit really soon.

Dav, sorry for the long time of practically no support here. I'll try my best to catch up. Would you be so kind as to review my proposal as your time permits? Certainly anyone else's comments are welcome as well!

I've seen your comments and therefor got the impression, that you had some ideas on how to improve beyond the bare fix. I'm interested to hear them, since I'm not sure, if I got it already right. Would be great to do a good step forward with this code, since something like (good) credential file management is really basic for AccountManagerPlugin, right?

By own code studies I've got the impression, that we should use universal newline support, so line endings are always seen as '\n' by our code regardless of real formatting (Macintosh convention '\r', Windows convention '\r\n'). Some code relies on the '\n' ending. For write mode there is a 'w+' mode, that might be better for our purpose, but I'm not entirely clear about the differences. Do you know more than I could read in Python docs? It seems a file could be left open for an indeterminated amount of time under some circumstances, what wasn't reported to be an issue, but should be fixed as a precaution.

Finally read my comment regarding a suspected race-condition, please. Do you agree on the assumption? All I've read about file locking implementation is certainly non-trivial to achieve, if we demand some portable code. This could make up another ticket, but I raise the topic here as I see it somewhat related.

[Steffen Hoffmann]

#7401 has been closed as a duplicate of this ticket.

[Dav Clark]

Replying to hasienda:

I've seen your comments and therefor got the impression, that you had some ideas on how to improve beyond the bare fix. I'm interested to hear them, since I'm not sure, if I got it already right. Would be great to do a good step forward with this code, since something like (good) credential file management is really basic for AccountManagerPlugin, right?

Golly - it's been long enough that I don't remember the extra details of what I was thinking at the time. Certainly I don't have any code for you beyond what you already did (putting the 'open' calls in try blocks). I agree that using the with statement is nice going forwards, given the large amount of code and number of nested blocks. But not a big deal - the code as is seems pretty safe now in this regard.

By own code studies I've got the impression, that we should use universal newline support, so line endings are always seen as '\n' by our code regardless of real formatting (Macintosh convention '\r', Windows convention '\r\n'). Some code relies on the '\n' ending.

Given that you're using universal newline support, it'd be marginally more robust to check f.newlines to find the newline convention used by the current file. This would be another step up from the old htfile code.

For write mode there is a 'w+' mode, that might be better for our purpose, but I'm not entirely clear about the differences. Do you know more than I could read in Python docs?

The python docs use poor language, I think. The modes are generally well-described in the C call, 'fopen.' But basically, the '+' means for reading and writing - almost never done anymore that I know of. Here's an example that illustrates the difference between 'w' and 'w+':

In [12]: f = open('test2.txt', 'w+')

In [13]: f.writelines(['This is a ...\n', 'Test!\n'])

In [14]: f.seek(0)

In [15]: f.readlines()
Out[15]: ['This is a ...\n', 'Test!\n']

-- vs. --

In [4]: f = open('test.txt', 'w')

In [5]: f.writelines(['This is a ...\n', 'Test!\n'])

In [6]: f.seek(0)

In [7]: f.readlines()

---

IOError Traceback (most recent call last)
/Users/dav/tmp/<ipython console> in <module>()
IOError: [Errno 9] Bad file descriptor

... i.e., the file is *only* open for writing. So, I think 'w' is marginally more appropriate in our case here.

It seems a file could be left open for an indeterminated amount of time under some circumstances, what wasn't reported to be an issue, but should be fixed as a precaution.

Finally read my comment regarding a suspected race-condition, please. Do you agree on the assumption? All I've read about file locking implementation is certainly non-trivial to achieve, if we demand some portable code. This could make up another ticket, but I raise the topic here as I see it somewhat related.

Given bug #7401, it seems that at least one other person is using this in a shared hosting environment (dreamhost) where possibly subversion, Trac and who knows what else are sharing a htpasswd file. I don't think there is any way to prevent race conditions between systems.

In the end, this really is a hack (if you want concurrency, you should probably be using a database backend or at least an OS service). I've been running my patch 2 above for almost 2 years without incident now, and your patch is even more robust... so I wouldn't worry that much about it. These are going to be small deployments.

But certainly, you could implement some sort of lockfile that this code specifically would check. Could be a fun exploration, but I suspect we're only talking about a handful of headaches you'd be saving here.

[Steffen Hoffmann]

Replying to davclark@berkeley.edu:

![...] Given that you're using universal newline support, it'd be marginally more robust to check f.newlines to find the newline convention used by the current file. This would be another step up from the old htfile code.

Indeed it seems better to check for a hint on actual newline format in f.newlines and use that instead of always '\n'. I didn't bother with this before, but it seems perfectly right. I'll try to implement this, as it could avoid some hassle for Trac with AccountManagerPlugin running on Windows and MacOS <= 9, right? But I'll not (yet) try file locking, as certainly this shouldn't be a big issue. I'll add a prominent warning to better go with any db backend for bigger application with concurrent credential use.

Your example in 'w' vs 'w+' was enlightening, and the whole response really did encourage me a lot to go on with this code. Thanks so much.

[Dav Clark]

Thank you!

[Steffen Hoffmann]

(In [9272]) AccountManagerPlugin: Improve password file handling, closes #4276 and #4897.

htpasswd.bak files are not deleted anymore when updating htpasswd file in the same directory and it's ownership is preserved as well, preventing a DoS by inaccessible user file in shared use. Furthermore we use universal newline support, if build-in, so line endings are always seen as '\n' by our code regardless of real formatting (Macintosh convention '\r', Windows convention '\r\n'). However the actual end-of-line style is probed and preserved on file updates. Finally the file is not left open for an indeterminated amount of time after file access, what wasn't reported but deduced by own code studies to be a potential issue and therefor fixed as a precaution.