Opened 12 years ago

# [patch] Provide configurable crypt prefix

Reported by: Owned by: Mitar normal AccountManagerPlugin normal hash crypt Mitar 0.11

### Description

For crypt there should be a setting to configure hash prefix. Crypt uses this prefix to determine which algorithm it should use for hashing.

### comment:1 Changed 12 years ago by Steffen Hoffmann

Keywords: hash crypt added new → assigned Crypt prefix → [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).

### comment:2 Changed 12 years ago by Steffen Hoffmann

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

As always, test feedback would be highly appreciated.

### comment:3 Changed 12 years ago by Steffen Hoffmann

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?

### comment:4 Changed 12 years ago by Steffen Hoffmann

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 1, 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.

### comment:5 follow-up:  6 Changed 12 years ago by 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.

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?

### comment:6 in reply to:  5 ; follow-up:  7 Changed 12 years ago by Steffen Hoffmann

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.

### comment:7 in reply to:  6 ; follow-up:  8 Changed 12 years ago by Mitar

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.

### comment:8 in reply to:  7 ; follow-up:  10 Changed 12 years ago by Steffen Hoffmann

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.

### comment:9 follow-up:  11 Changed 12 years ago by Steffen Hoffmann

>>> 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')

### comment:25 Changed 12 years ago by Mitar

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

### comment:26 Changed 12 years ago by Steffen Hoffmann

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?

### comment:27 follow-up:  28 Changed 12 years ago by 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.

### comment:28 in reply to:  27 Changed 12 years ago by Steffen Hoffmann

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.

### comment:29 Changed 12 years ago by Steffen Hoffmann

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

### comment:30 Changed 12 years ago by Steffen Hoffmann

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

### comment:31 Changed 10 years ago by Steffen Hoffmann

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

### comment:32 Changed 6 years ago by Ryan J Ollos

Owner: Steffen Hoffmann deleted assigned → new

### comment:33 Changed 3 years ago by Ryan J Ollos

Cc: Ryan J Ollos removed

### Modify Ticket

Change Properties