[patch] Provide configurable crypt prefix

I've done a quick research about it.

Seems reasonable. Because we're in a short freeze period right now, let's do it after the release of acct_mgr-0.3, and in conjunction with another extension to the hash logic, that I've prepared recently (see #8791).

I'll go for it now with minor changes like appropriate i18n markup.

As always, test feedback would be highly appreciated.

This is harder to implement than I thought initially, still struggling. Please allow and answer some questions:

Did you really test (with arbitrary prefix, including unittests) your patch successfully? My patch is growing much bigger now, especially for inclusion of hash check modifications, that seem to be missed in your patch.

Is an arbitrary prefix even required, or is it more about switching $1$ on/off? A boolean option would be enough then, you know.

Wouldn't be '' a better default value than $1$, for backwards-compatibility? What has been the/your reason for suggesting that value?

Got it, finally. And another time I wonder, how it happened to be working before.

Certainly a good deal of the time spent on it has been due to my shallow understanding of character sets and encoding (the never-ending Unicode theme). Thanks to resources like stackoverflow.com ¹, so at least I had a good chance to get a grip on it after hours of reading and testing in try-n-error style.

Regarding the default values I'm still looking forward to your response.

¹ http://stackoverflow.com/questions/3375238

Hm. I am testing my patch on a server where I require crypt md5 hashes so that HTTP server can also authenticate against the same file (for Mercurial HTTP authentication). It seems to work there.

I do not see why would we limit the prefix to only $1$. The idea is that crypt is extendable with various hash types so why would we limit it? Especially concerning that MD5 have some crypto weaknesses. For example, if I would not be bound to the HTTP server MD5 support limitation, I would use $5$ or $6$ for SHA-256 or SHA-512 respectively. There is also $2a$ for Blowfish which is supported on FreeBSD and some Linux distributions.

Definitely '' is the worst possible choice as it makes crypt uses obsolete and relatively simple to crack DES. Anything more secure is better. And I chose $1$ because it is the most widely available (SHA is supported only in glibc 2.7+, but this is also now probably everywhere).

Probably it is true that Trac admin could put some stupid things into the prefix and break everything. But this is then her problem.

What do you mean by hash check modifications?

Yes, this is why Django provides safe_unicode. ;-) Now I understand why. Thanks. And thanks for all your effort for pulling this in.

Maybe Trac also provides something like this? Under some other name?

Replying to mitar:

Hm. I am testing my patch on a server where I require crypt md5 hashes so that HTTP server can also authenticate against the same file (for Mercurial HTTP authentication). It seems to work there.

But md5 is a different setting and not handled directly by crypt here.

I do not see why would we limit the prefix to only $1$. The idea is that crypt is extendable with various hash types so why would we limit it? Especially concerning that MD5 have some crypto weaknesses. For example, if I would not be bound to the HTTP server MD5 support limitation, I would use $5$ or $6$ for SHA-256 or SHA-512 respectively. There is also $2a$ for Blowfish which is supported on FreeBSD and some Linux distributions.

Because algo/hash type selection isn't meant to be done this way in AcctMgr right now. This is what we have the htpasswd_hash_type/db_htpasswd_hash_type option for. And sha512 is actually the extension I mentioned before (see [10492]).

Definitely '' is the worst possible choice as it makes crypt uses obsolete and relatively simple to crack DES. Anything more secure is better.

Sure, but...

And I chose $1$ because it is the most widely available (SHA is supported only in glibc 2.7+, but this is also now probably everywhere).

as stated above, we're just speaking about a (cosmetic) prefix here. You may have mistaken this implication, right? Consider, that it's just that, a prefix and reconsider my opinion about it. Does this knowledge even render the change request obsolete?

Probably it is true that Trac admin could put some stupid things into the prefix and break everything. But this is then her problem.

And mine, to waste time with tickets I might have been able to prevent by different design decisions. All of us should care to ease doing the right thing for everyone. Not exactly child-safety, but skipping avoidable complexity - but stop this. Most probably we do agree without more reasoning.

What do you mean by hash check modifications?

If the MAGIC/prefix is the hint on hash type, and crypt_hash_prefix is just that (without changing the hash used by crypt, like you seem to imply), then changing the prefix would do just that: change what prefix would trigger a crypt hash processing, and what not. Clearer?

Yes, this is why Django provides safe_unicode. ;-) Now I understand why.

But I don't know anything about this, wait... maybe got it. Do you mean mark_safe? While meant to sanitize for HTML output, it could do well for other output like for our password files as well, only I'm not sure, never really dealt with Django for really experience, and I'm not skilled to deduce this from code review alone.

Thanks. And thanks for all your effort for pulling this in.

You're welcome. Still consider, if we're doing the right thing here, like I hinted above, please.

Maybe Trac also provides something like this? Under some other name?

Trac has advanced file management with an AtomicFile class (or similar named) for trac.ini, but I hesitated to switch the password file store implementations that radical by now. Never looked closely into Trac's native auth redarding hash types used, so I can't comment on this.

Replying to hasienda:

But md5 is a different setting and not handled directly by crypt here.

No, $apr1$ is this different setting. And $1$ is different than $apr1$. ;-)

Because algo/hash type selection isn't meant to be done this way in AcctMgr right now. This is what we have the htpasswd_hash_type/db_htpasswd_hash_type option for.

Why not? If you have crypto, then you should also have this prefix selection. Otherwise you have crypto support for crypto from 1990. ;-)

as stated above, we're just speaking about a (cosmetic) prefix here.

A cosmetic prefix which makes things interoperable or not. I opt for interoperable. If some other program is using the same file for authentication, it is good that you can configure it in a way that it is interoperable.

If the MAGIC/prefix is the hint on hash type, and crypt_hash_prefix is just that (without changing the hash used by crypt, like you seem to imply), then changing the prefix would do just that: change what prefix would trigger a crypt hash processing, and what not. Clearer?

Hm, not really. You are saying that it is necessary to check if the prefix will trigger a different hash for crypt? Why would this be necessary to know/check?

Do you mean mark_safe?

Oh, sorry. I mixed with force_unicode. But forget that. It was just a comment that they provide things to easier convert to unicode instead of just using unicode() which can break.

Maybe Trac also provides something like this? Under some other name?

Never looked closely into Trac's native auth redarding hash types used, so I can't comment on this.

I thought for unicode handling in general.

Replying to mitar:

Replying to hasienda:

But md5 is a different setting and not handled directly by crypt here.

No, $apr1$ is this different setting. And $1$ is different than $apr1$. ;-)

In fact htpasswd function in pwhash.py does the hash method selection for us now, not crypt on it's own. I'll have to check, if it's capable of reading the MAGIC from the beginning of the salted hash here. But this implies a different concept for aforementioned htpasswd, right?

Because algo/hash type selection isn't meant to be done this way in AcctMgr right now. This is what we have the htpasswd_hash_type/db_htpasswd_hash_type option for.

Why not? If you have crypto, then you should also have this prefix selection. Otherwise you have crypto support for crypto from 1990. ;-)

Not sure about how ancient it is (Debian is really conservative, but RHEL is even more as I learned recently), but I use always the current stable of that OS and switched to md5 too after recognizing that only first 8 chars of password actually got into the hashes with default 'crypt' setting...

as stated above, we're just speaking about a (cosmetic) prefix here.

A cosmetic prefix which makes things interoperable or not. I opt for interoperable. If some other program is using the same file for authentication, it is good that you can configure it in a way that it is interoperable.

interoperability is fine, but the question remains: Is there any value other than '$1$', that makes sense for now and therefor would justify the additional flexibility of an (arbitrary string) Option?

If the MAGIC/prefix is the hint on hash type, and crypt_hash_prefix is just that (without changing the hash used by crypt, like you seem to imply), then changing the prefix would do just that: change what prefix would trigger a crypt hash processing, and what not. Clearer?

Hm, not really. You are saying that it is necessary to check if the prefix will trigger a different hash for crypt? Why would this be necessary to know/check?

YES, I think so, as mentioned above. At least to take extra care for not breaking existing setups. My "make current hard coded prefix value '' option the suggested default" attitude has been driven by the same thoughts about not forcing changes upon plugin updates, even not improvements like hashes promising better security. Doesn't it make sense to you at all?

Ok, would love to talk to you more directly (IRC #trac, sometimes?), since we're already discussing that many things in parallel now. Hard to remember, keep track, even if layed town in tickets. (I think) Ticket comments are just not good for any (basic) discussions beside the actual ticket subject.

>>> from crypt import crypt
>>> crypt()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: crypt() takes exactly 2 arguments (0 given)
>>> crypt('pass', 'MAGIC')
'MAiBWuRAOXSgo'
>>> crypt('pass', '$1$MAGIC')
'$1$MAGIC$e3aszg2ukyy6hkzR1QvDH/'
>>> crypt('pass', '$apr1$MAGIC')
'$aJ9yLvr8jbsE'
>>> crypt('pass', '$6$MAGIC')
'$6$MAGIC$bFh/1V94Mp7iawFn0kvIs.YZMOutrkR.6.J9mejgreP.7Rnljx9mfFl2Bft9FDKiTae0/8oT0zfYu7gLebBuE1'

Well, seems like you predicted, but why on earth is htpasswd in pwhash.py that complicated then? We actually split MAGIC hash prefixes and delegate to different classes (of hashlib since Python2.5). Only reason I see is portablility, and I can't comment on that neither for Win nor Mac OSes. Do you have a clue?

Replying to hasienda:

... and switched to md5 too after recognizing that only first 8 chars of password actually got into the hashes with default 'crypt' setting...

Exactly. And we have to let also users of Trac be capable of using something else than DES. Otherwise crypto support does not really have much value, because nobody really wants DES this days when they are talking about crypto.

Is there any value other than '$1$', that makes sense for now and therefor would justify the additional flexibility of an (arbitrary string) Option?

As I said: crypto allows also additional non-standard prefixes to add. So some special installation could want it. And also there is one big reason: SHA support. I would use SHA if I would not be tied by other applications to use MD5. But MD5 is a good default as it is most widely used.

But I could agree that for compatibility sake a default could still be an empty prefix. But it should really be documented (like in an example on wiki) that you set ti to $1$ or even $6$.

(I think) Ticket comments are just not good for any (basic) discussions beside the actual ticket subject.

I tend to kindly disagree here. I mostly require in all my projects that people discuss everything in tickets so that everything is one place. Trac even has threaded discussions now. But yes, we can use also some other mean. It is OK with me.

Replying to hasienda:

Only reason I see is portablility, and I can't comment on that neither for Win nor Mac OSes. Do you have a clue?

Yes, portability. And I think it is OK to have that. So to have build-in methods. But account manager is pluggable and you can also have other methods. One of them is crypt support. And so I think we should support it fully, with prefixes. I do not see anything wrong if things overlap a bit. Because everything has its own uses.

But we could also do like crypto-only way with failback to build-in if they are missing. Maybe there is ever already a Python-only crypto cross-platform module which does this for ourselves.

Replying to mitar:

As I said: crypto allows also additional non-standard prefixes to add. So some special installation could want it. And also there is one big reason: SHA support. I would use SHA if I would not be tied by other applications to use MD5. But MD5 is a good default as it is most widely used.

We already have got sha512 in trunk by now (did you check it?), anyway I'm thinking towards a major change now.

But I could agree that for compatibility sake a default could still be an empty prefix. But it should really be documented (like in an example on wiki) that you set ti to $1$ or even $6$.

Good, so I'd like to go that route. Ship with almost obsolete default, but make this extra clear in (wiki) docs. Standard crypt with DES should be avoided like a plague and even md5 and sha1 are said to be no longer totally secure these days.

[...] I mostly require in all my projects that people discuss everything in tickets so that everything is one place. Trac even has threaded discussions now. But yes, we can use also some other mean. It is OK with me.

Point for you, as long as it's not too much OT.

Would you do me a favor and tell me, what you get back from your Python interactive console with the following commands, please?

import hashlib
[algo for algo in dir(hashlib) if not algo.startswith('_') and algo not in ('new')]

Replying to mitar:

But we could also do like crypto-only way with failback to build-in if they are missing. Maybe there is ever already a Python-only crypto cross-platform module which does this for ourselves.

hashlib, what else? - tested with Python2.6 on old WinXP, just works.

Python 2.6.1 (r261:67515, Jun 24 2010, 21:47:49) 
[GCC 4.2.1 (Apple Inc. build 5646)] on darwin
...
['md5', 'sha1', 'sha224', 'sha256', 'sha384', 'sha512']

Mac OS X 10.6.

I think the apr and blowfish are the only ones from common missing.

Ideal, could not dream it better. So we have verification for GNU/Linux, WinXP and MacOSX backing up a new approach to use crypt as well as hashlib including support for more recent hash types ('sha224', 'sha256', 'sha384'). I'll come up with a rework of the htpasswd function from pwhash.py covering many of the aspects discussed before. Thanks for discussion and pushing me to the current level of understanding the subject.

Replying to mitar:

I think the apr and blowfish are the only ones from common missing.

By 'apr' you refer to the $apr1$ prefix, that is supposed to be equal to $1$ alias htpasswd method provided by md5crypt.py for this plugin as well? This would be just an additional overlap. Blowfish OTOH, well, do you mean to obsolete your own HtBlowfishStore? Until now I've found no good alternative than python-bcrypt. But this works fine, as tested in the Python console.

apr is not the same as md5. I think apr has different (more?) hash iterations.

python-bcrypt works nice, and HtBlowfishStore? too. I think there is no need to support Blowfish as it is not so common.

Replying to mitar:

apr is not the same as md5. I think apr has different (more?) hash iterations.

I know, it's not md5, in fact called md5crypt here.

$apr1$
 Identical to $1$.
 This prefix is generated by the Apache Portable Runtime library
 (used by htpasswd, for example)

Q.: wiki.call-cc.org).

But no need to worry anyway, since we have/support it here.

python-bcrypt works nice, and HtBlowfishStore too. I think there is no need to support Blowfish as it is not so common.

Ok, since it comes with a really not so common dependency on bcrypt, I'm fine with current state.

Note, that with current 'migration mode' password move from - in this example - blowfish to another store (set as primary) would even happen on successful user login. Did you see/check this before? I'm curious, since I never got any feedback about this by now.

Replying to hasienda:

Note, that with current 'migration mode' password move from - in this example - blowfish to another store (set as primary) would even happen on successful user login. Did you see/check this before? I'm curious, since I never got any feedback about this by now.

I do not understand you here. What you mean by even happen on successful user login? The idea is that password is migrated on successful login?

Replying to mitar:

I do not understand you here. What you mean by even happen on successful user login? The idea is that password is migrated on successful login?

Exactly, no need to nudge someone to change his/her password for migration. And it's doing that not every time but just once per user. I thought that would be a cool feature, never seen somewhere else before. Any feedback appreciated.

Oh, I thought this is the common way of migrating. At least Django did the same. You support checking against both hash types, but when you do login, you change to the new hash type.

Good to know this is done elsewhere too.

But as far as I see, with Django this is for a seemless upgrade scenario only, so AcctMgr is still more flexible by providing a number of concurrent password stores. It's perfectly normal to have a user base served from a mixture of authentication backends forever. Migration is not an implicit default like with the Django code but a special operation, and we support it forth and back as many times as needed.

Sounds a bit like I want to sell this to you, right? ;-) Sorry, not meant to do so, this should better go into the wiki docs to explain this for everyone.

Since the so called modular crypt format (MCF) has no definitive specification document, I welcomed another reference ¹ to verify prior findings.

Well, sort of, looks like there is no full agreement. At least we have {SHA} vs. $sha1$ here, and a common identifier for sha224 and sha384 is never mentioned, so I deduce it does not exist.

¹ http://packages.python.org/passlib/modular_crypt_format.html#mcf-identifiers

As I said, I would just let admin specify any prefix she wants. And this is it.

There is more. Did you recognize, there is an optional rounds=<N> parameter, that could be prepended to the salt? And SHA256 and SHA512 (so most probably the other SHA's as well) can swallow up to 16 chars of salt. It would be silly to call for cryptographically stronger hashes but OTOH not use as much salt as we can, right?

I do not understand the problem here? You let the user specify any prefix and we always provide salts of for example 100 characters. What is then used by the chosen algorithm is its thing.

Replying to mitar:

I do not understand the problem here? You let the user specify any prefix and we always provide salts of for example 100 characters. What is then used by the chosen algorithm is its thing.

No problem, just wanted to note the we'll have to generate salts longer than now (8 chars) for a 'general propose' implementation. In fact his is already an issue for the current sha512 support.

(In [10523]) AccountManagerPlugin: Forget about pure sha512, refs #5464, #8791 and #8933.

sha512_crypt is the module, which we need for conformant hash calculation. However, this is not a Python standard, so try to import from passlib and fallback to crypt as last resort, if this is capable at all.

(In [10524]) AccountManagerPlugin: Add configurable salt string char count, refs #7396 and #8933.

Newer hash algorithms are capable of using more than 8 characters of salt. For improved hash protection we'll feed them at maximum length.

(In [12398]) AccountManagerPlugin: Releasing version 0.4, pushing development to acct_mgr-0.5dev.

Availability of that code as stable release closes #874, #3459, #4677, #5295, #5691, #6616, #7577, #8076, #8685, #8770, #8791, #8990, #9052, #9079, #9090, #9139, #9246, #9252, #9547, #9618, #9676, #9843, #9852, #9940, #10023, #10028, #10123, #10142, #10204, #10276, #10397, #10412, #10594, #10625 and #10644.

Some more issues have been worked-on, yet without confirmed resolution, refs #5464 (for JiraToTracIntegration), #8927 and #10134.

And finally there are some issues and enhancement requests showing progress, but known to require more work to resolve them satisfactorily, refs #843, #1600, #5964, #8217, #8933.

Thanks to all contributors and followers, that enabled and encouraged a good portion of this development work.