[SSSD] [PATCHES] sss_seed related patches

Stephen Gallagher sgallagh at redhat.com
Tue Oct 2 14:23:46 UTC 2012


On Tue 02 Oct 2012 09:21:46 AM EDT, Michal Židek wrote:
> On 10/02/2012 01:56 PM, Stephen Gallagher wrote:
>> On Mon 01 Oct 2012 05:32:50 PM EDT, Jakub Hrozek wrote:
>>> On Mon, Oct 01, 2012 at 07:45:17PM +0200, Pavel Březina wrote:
>>>> On 10/01/2012 06:25 PM, Michal Židek wrote:
>>>>> Patch 01: The debug_level variable was set before the parameters were
>>>>> processed, so the default debug level was used no matter what number
>>>>> was
>>>>> specified with --debug.
>>>>>
>>>>> Patch 02: When interactive input fails, error message should be
>>>>> displayed.
>>>>> https://fedorahosted.org/sssd/ticket/1549
>>>>>
>>>>> Patch 03: When file is used to specify a password in sss_seed, then
>>>>> only
>>>>> first line of this file is used. Also empty passwords are treated as
>>>>> errors. (My local test environment is broken so I have not tested
>>>>> this,
>>>>> please test it when reviewing.)
>>>>> https://fedorahosted.org/sssd/ticket/1548
>>>>>
>>>>> Patches are in attachment.
>>>>>
>>>>> Michal
>>>>
>>>> Patch 1: nack
>>>> Instead of moving debug level initialization further in the code,
>>>> you should parse the parameters earlier. The other tools uses
>>>> following pattern:
>>>> - set debug program name
>>>> - set locale
>>>> - parse parameters
>>>> - set debug level
>>>> See sss_groupadd for example.
>>>>
>>>> Patch 2: ack
>>>> Patch 3: I'll do this one tomorrow.
>>>
>>> My take on Patch 3:
>>>
>>> Instead of stripping everything after the first line, I would prefer
>>> simply failing if there's any non-whitespace character after the first
>>> line break ('\n').
>>>
>>> Stripping anything after the first line unconditionally may seem like
>>> everything was OK to the user..while he wouldn't be able to easily
>>> input
>>> a password like that.
>>>
>>> Also a code-style nitpick -- please put a newline after "}" and before
>>> the comment saying "/* Do not allow empty passwords */" in
>>> seed_password_input_prompt().
>>
>>
>> One more comment: please change the atomic read to be PASS_MAX+1 and
>> then check whether len > PASS_MAX. If it is, we need to fail because
>> we've been passed a password-file too long. The current code would just
>> use the first PASS_MAX characters as the password, which is dangerous
>> and fails to meet the users' expectation.
>>
>> Also, the PASS_MAX limitation needs to be specified in the manpage for
>> sss_seed(8).
>
> Added 4th patch to fix the PASS_MAX issue.
>
> New patches attached.
>

Nack,

You're leaking the file descriptor. You should close it before the 
check.

In the manpage, you have several typos:
s/lenght/length/
s/less then/less than/

And for wording:
s/(64 on systems where no PASS_MAX is specified)/(64 characters on 
systems with no globally-defined PASS_MAX value)/




More information about the sssd-devel mailing list