Re: Feature Proposal: Transparent upgrade of crypt() algorithms

Hi all,

Thanks for your attention to the matter/threads.  I have thought 
a bit about this, and I hope I can add some value to the current 
conversation, below:

On 03/07/2014 07:36 PM, Xin Li wrote:
> On 03/07/14 14:50, A.J. Kehoe IV (Nanoman) wrote:
>> Xin Li wrote:
>>> On 03/07/14 13:52, A.J. Kehoe IV (Nanoman) wrote:
>>>> Allan Jude wrote:
>>>>> On 2014-03-07 11:13, A.J. Kehoe IV (Nanoman) wrote:
>>>>>> Allan Jude wrote:
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> Honestly, my use case is just silently upgrading the
>>>>>>> strength of the hashing algorithm (when combined with my
>>>>>>> other feature request). Updating my bcrypt hashes from
>>>>>>> $2a$04$ to $2b$12$ or something. Same applies for the
>>>>>>> default sha512, maybe I want to update to rounds=15000
>>>>>>
>>>>>> Like this?
>>>>>>
>>>>>> http://www.freebsd.org/cgi/query-pr.cgi?pr=182518
>>>>>>
>>>>>> Request for comments:
>>>>>>
>>>>>> http://docs.freebsd.org/cgi/mid.cgi?20140106205156.GD4903
>>
>> [...]
>>
>>> Speaking for adding rounds, the only problem that needs to be
>>> fixed is that the proposed patch makes it possible to create
>>> conflicting configuration (passwd_format and passwd_modular can
>>> use different hashing algorithms) and need to be fixed and
>>> polished.  I like the idea of making it possible to use more
>>> rounds though.
>>
>> This was deliberate for backward compatibility.  passwd_format will
>> be used by default if passwd_modular isn't defined.  If
>> passwd_modular is defined as "disabled", then passwd_format will be
>> used.
>
> Well, my point is that the two shouldn't be allowed to exist together
> if they can mean something conflicting.  Allowing passwd_format=sha512
> AND passwd_modular=$2a$08$ in the same configuration creates confusion
> and it's not good.
>

Agreed.  My original intention was to create a patch that didn't 
touch a lot of code.

My reasons for this were first to see if there was any interest 
from a committer to take this further.  Much more likely to have 
a 15 or so line patch looked at, than one that touches stuff all 
over the place - I think.

We are now at least having a conversation about it.

It seemed to be a lot of work to specify rounds via 
login_setcryptfmt, with a bunch of changes also required in 
libcrypt.

I don't have the resources to test for regressions in libcrypt, 
beyond the scope of whether login.conf works as expected 
(specifically, the ports tree, yp, ldap, or any other areas that 
I don't know about).

If other developers were willing to work together on the api/abi 
changes, I would feel a lot better about spending my time there 
and doing it right.  Without support from other, more 
knowledgeable people (as far as what will break if we do XYZ), 
who will eventually merge productive changes, I would be wasting 
my time.

I don't want to be the libcrypt api changing pixie, scattering 
patches into /dev/null. :)

> My suggestion is that we either have:
>
> a) passwd_format and passwd_round (so that they don't conflict), or
>

I recommend against this.  By example, based on current scrypt 
modular crypt RFCs, there are multiple tunable parameters.  It's 
conceivable that other future algorithms will have different 
functional and named parameters.

Additionally, I think having all the parsing code for this 
scattered about actually makes things less clear.  For example, 
$2a$08$ means a lot more to people (across different *nix 
backgrounds) than blf, is concise, and is/already should be well 
documented in crypt(3). Likewise with sha512.  Looking at 
login.conf, you can't tell exactly what it means.

Modular crypt is something that developers are working to stay 
compatible with (e.g. $5$, $6$, $2y$, etc), is understood outside 
of the context of FreeBSD system administration, and would be 
understood by people who are knowledgeable enough to seek to 
change this aspect of their system.

> b) extend passwd_format in a compatible manner to allow specifying a
> round, or,
>
> c) make passwd_format and passwd_modular conflict so we don't silently
> accept it and instead bail out when doing pwd_mkdb.
>

As jmg suggested, by supplying the modular format for 
passwd_format, we eliminate this conflict, and make it obvious. 
I definitely support this notion.

That means touching login_setcryptfmt and friends, I think.

>> What do you think of the idea of putting this into libcrypt instead
>> of pam_unix.c, and then patching pam_unix.c and pw_user.c to
>> reference libcrypt?
>
> Which part of the idea?  I think it's a bad idea to make libcrypt to
> depend on libutil (for login_cap(3)) but we should probably provide
> new wrappers in login_cap(3) to do the common things when requested
> for various password manipulating tools to reduce duplicated code.
>

Specifically:

The makesalt aspect can/should be put into libcrypt, refined 
appropriately, and exposed publicly.  It is a terrible little 
piece of code as it is now, twice (or more!), and it could be 
cleaned up considerably.  This could be a nice little api.

Secondly, since the digests are used externally, I think it would 
be good to push the custom base64 code out to a library 
somewhere, so there is the standard way to do it, documented. 
Maybe libcrypt is the right place for this function too, since 
that is the context in which I have seen it.  I forget for sure 
now, but I think each algorithm is also responsible for base64 
encoding their output.  Not that I'm saying we should just rip it 
out, but it might be worthwhile to look case by case, if it's 
appropriate.


As far as autotuning the work-factor, I think that just being 
able to set it at all is a huge improvement, and autotuning is 
Just Details.  We can see that this will be fraught with problems 
establishing consensus, and could stall making progress with the 
other good work.  Even if every couple of years, the default in 
login.conf gets bumped to whatever.  When people run mergemaster, 
it'll show, and the admin can decide then.  As it is right now, 
rounds are fixed, that's not appropriate for any use-case, small 
or large.


Finally, I agree the ability to auto-update existing digests is 
desirable.  That and the other policy stuff can happen totally 
separate from the discussion around exposing the tunables.


Thanks for considering my input,

- Derek