[Bug 2642] [sshconnect2] publickey authentication only properly works if used first: pubkey_prepare doesn't work after pubkey_cleanup

[bugzilla-daemon at bugzilla.mindrot.org]

https://bugzilla.mindrot.org/show_bug.cgi?id=2642

Vincent Brillault <git at lerya.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2895|0                           |1
        is obsolete|                            |

--- Comment #6 from Vincent Brillault <git at lerya.net> ---
Created attachment 2900
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2900&action=edit
Only reset 'tried' count (on reset) and 'isprivate' (when freeing key)

Thanks for the review!

> I not sure I properly understand why you change modifies id->tried.

My goal was to emulate the existing behavior:
- As the key list was rebuild between two authentications, the order
was always the same, with 'tried' set to 0 (xcalloc)
- When a key is tried, it is immediately moved to the end of the list
and the 'tried' counter is increased

My first loop continues to move keys to the end until the original
order is found (all keys have been 'tried' (i.e moved) the same time)

My second loop is resetting the 'tried' count because I understand it
is used in userauth_pubkey in the while loop to make that the keys are
not used twice (see 
https://github.com/openssh/openssh-portable/blob/master/sshconnect2.c#L1438).

> Is it to move all tried keys to the end of the list? I think it
> might be clearer to do something like the attached. It's a little
> longer, but IMO easier to understand its intent.

Mmm, I understand that by design, all keys are already at the end of
the list. If resetting the order is not important, just clearing the
id->tried should be enough

>> Also, I don't understand why you reset isprivate. I think this might
>> cause re-prompting for passwords to load keys that have already been
>> loaded.
> oh, I see. You reset isprivate because the key is subsequently freed

Exactly!
I was not sure where to reset the value, in the 'if' block, where the
value is initialized (but it's not freed yet, so why?) or when it's
freed (but it was not always initialized on that execution path).

I've attached a stripped-down version of the patch, which only reset
the 'tried' count and reset the 'isprivate' id after the key has been
freed

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.