Re: [PATCH v2] chpasswd.cgi: Fixes bug12755 - v2 with password verification correction

[Michael Tremer <michael.tremer@ipfire.org>]

Hello Adolf,

> On 9 May 2025, at 13:15, Adolf Belka <adolf.belka@ipfire.org> wrote:
> 
> Hi Michael,
> 
> On 08/05/2025 15:11, Michael Tremer wrote:
>> Hello Adolf,
>> I just gave this a try:
>> ipfire build chroot (x86_64) root:~$ htpasswd -vb /var/ipfire/auth/users admin ipfire; echo $?
>> User admin not found
>> 6
>> ipfire build chroot (x86_64) root:~$ htpasswd -b /var/ipfire/auth/users admin ipfire
>> Adding password for user admin
>> ipfire build chroot (x86_64) root:~$ htpasswd -vb /var/ipfire/auth/users admin ipfire; echo $?
>> Password for user admin correct.
>> 0
>> ipfire build chroot (x86_64) root:~$ htpasswd -vb /var/ipfire/auth/users admin ipfire2; echo $?
>> password verification failed
>> 3
>> ipfire build chroot (x86_64) root:~$ htpasswd -vb /var/ipfire/auth/users admin2 ipfire2; echo $?
>> User admin2 not found
>> 6
>> This is in the dev system, so the password file was empty to start with.
>> Basically if the username and password match the return code is zero. If something else happened it isn’t. And this is exactly what I would check for: Okay on zero, not okay on anything else. I would not even case why htpasswd was upset because it does not matter in our use-case.
> 
> Okay, so the $? contains the status of the bash command htpasswd but how do I access that via the perl system_output command. I have tried making the result of the system_output command be fed to a $variable but I just get a 1 for all the different conditions you tried.
> I also tried making the system_output command be fed to an array variable and then printing all the elements of that array variable out but there was no status value.

You don’t use the system_output() function, you should use the &General::system() function. On there, we fetch the return code and return it:

  https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=config/cfgroot/general-functions.pl;h=8ba6e3f79f0a9660ba8f8630ad0c7f1a3f6c988d;hb=HEAD#l32

Perl stores the value in $? like bash, but weirdly shifts it by 8 bits, so we have to shift it back.

So your code should read a bit like:

  if (&General::system(“htpasswd”, “-B”, …) == 0) {
    // success
  } else {
    // fail
  }

> It will probably turn out to be very simple but I am afraid I am not having any luck figuring out how to do it.

Not a problem :)

The &General::system_output() function does not return the return code because it is already returning the output of the command. If the command fails there won't be any output - and that is not a very good way to check things normally.

-Michael

> All feedback gladly welcomed.
> 
> Regards,
> Adolf.
> 
>> -Michael
>>> On 7 May 2025, at 15:02, Adolf Belka <adolf.belka@ipfire.org> wrote:
>>> 
>>> Hi Michael,
>>> 
>>> On 07/05/2025 15:52, Adolf Belka wrote:
>>>> Hi Michael,
>>>> On 07/05/2025 14:44, Michael Tremer wrote:
>>>>> Hello Adolf,
>>>>> 
>>>>> Thanks for the patch. Is there no return code that we get from htpasswd instead of parsing the output?
>>>> It gives a return code for everything, with numbers of 0 to 7, except for the use of the -v option to verify the password.
>>> There might be a status code returned. The man page says
>>> 
>>> 3 if the password was entered interactively and the verification entry didn't match
>>> 
>>> but elsewhere it does suggest that interactively is not via the -bv option but where you just use -v and manually type the password when requested on the command line.
>>> 
>>> If the status 3 is a valid status code, how can I access that from the output of the &General::system_output subroutine?
>>> 
>>> I could give it a try out and if it does work then I could do a v3 patch.
>>> 
>>> Regards,
>>> 
>>> Adolf.
>>> 
>>>> This gives
>>>> password verification failed
>>>> if the existing password for the specified user is not correct and
>>>> Password for user fred correct.
>>>> if the user specified was fred and the specified password was correct.
>>>> It does the above for both the interactive -v and the batch mode using the command line of -bv
>>>> I had to use the check for if the string was found in the return variable because if I checked if the string matched the contents of the variable it always failed so I think there is a hidden Carriage Return or something in the output from htpasswd for the verification test.
>>>> Regards,
>>>> Adolf.
>>>>> 
>>>>> -Michael
>>>>> 
>>>>>> On 7 May 2025, at 13:42, Adolf Belka <adolf.belka@ipfire.org> wrote:
>>>>>> 
>>>>>> - Realised that I had not tested the old password beinhg correct or not. Previous check
>>>>>>    gave the same answer irrespective of the output coming from the htpasswd verification.
>>>>>> - This changes the variable used for the system_output result to an array and then
>>>>>>    checks if the first element contains the failure message that htpasswd gives if
>>>>>>    password verification fails.
>>>>>> - Tested out with correct and incorrect old passwords and gave the correct answer in
>>>>>>    both cases. Confirmed also that the check for the user being present works correctly
>>>>>>    for both an existing and new user name, which it did.
>>>>>> 
>>>>>> Fixes: bug12755
>>>>>> Tested-by: Adolf Belka <adolf.belka@ipfire.org>
>>>>>> Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
>>>>>> ---
>>>>>> html/cgi-bin/chpasswd.cgi | 4 ++--
>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>> 
>>>>>> diff --git a/html/cgi-bin/chpasswd.cgi b/html/cgi-bin/chpasswd.cgi
>>>>>> index c00caca20..46c3e02f6 100644
>>>>>> --- a/html/cgi-bin/chpasswd.cgi
>>>>>> +++ b/html/cgi-bin/chpasswd.cgi
>>>>>> @@ -77,11 +77,11 @@ if ($cgiparams{'SUBMIT'} eq $tr{'advproxy chgwebpwd change password'})
>>>>>>         # Check if a user with this name and password exists in the userdb file
>>>>>>         # and if it does then change the password to the new one
>>>>>>         my $user = &General::system_output("grep", "$cgiparams{'USERNAME'}", "$userdb");
>>>>>> -       my $old_password = &General::system_output("/usr/bin/htpasswd", "-bv", "$userdb", "$cgiparams{'USERNAME'}", "$cgiparams{'OLD_PASSWORD'}");
>>>>>> +       my @old_password = &General::system_output("/usr/bin/htpasswd", "-bv", "$userdb", "$cgiparams{'USERNAME'}", "$cgiparams{'OLD_PASSWORD'}");
>>>>>>         if (!$user) {
>>>>>>                 $errormessage = $tr{'advproxy errmsg invalid user'};
>>>>>>                 goto ERROR;
>>>>>> -       } elsif (!$old_password) {
>>>>>> +       } elsif (@old_password[0] =~ /password verification failed/) {
>>>>>>                  $errormessage = $tr{'advproxy errmsg password incorrect'};
>>>>>>                  goto ERROR;
>>>>>>         } else {
>>>>>> -- 
>>>>>> 2.49.0