On Wed, Oct 03, 2012 at 10:07:42AM +0200, Michal Židek wrote:
On 10/02/2012 04:23 PM, Stephen Gallagher wrote:
>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)/
>
New patches attached.
Thanks
Michal
[PATCH 1/4] sss_seed: Option --debug did not work in sss_seed tool.
Ack
[PATCH 2/4] sss_seed: Show error message when interactive input
Ack
[PATCH 3/4] sss_seed: Make only first line of password file valid.
Ack
[PATCH 4/4] sss_seed: Passwords longer then PASS_MAX not allowed.
Ack