auth: Fix multiple 'sk' keys configured, fails on first

[jac-cbi]

When a user has configured two or more 'sk' keys, eg Yubikeys, pam-ssh-agent will fail if the first configured key isn't currently plugged in.

Standard SSH utilities gracefully try the next key in this situation, so we should too. This is very helpful for users attempting to dogfood their backup key. ;-)

We correct this failure by catching the error, and trying the next key IFF the failed key was of the 'sk' type.

[jac-cbi]

merf. f^%$#ing GitHub. I was testing out signed commits on my fork of your repo. It decided to be helpful and open the PR on your repo. sigh.

oh well, let me know if you want any changes. I could easily omit the 'sk' restriction to retry, as other SSH tools aren't that strict, but want to default conservative on the first pass.

[nresare]

Hi! This is brilliant, and exactly the kind of usability improvements that I would like to add. I'd like the following, if you have the time to update your PR:

• I think that it makes sense to try the next key until success, regardless of key type
• I'd like the actual error to be logged on debug level, as it might very well be that this masks some other failure
• If we want to be fancy, it would be nice if the failure (s) was logged on error level if there is no successful authentication.
  It is not obvious how to implement this, but I think it would be neat
• Maybe adding a test case

If you don't have the bandwidth to make these changes I would be happy to land your change as-is and make the suggested changes on top of it.

[jac-cbi]

I'm a little rusty with rust (ha!), but most of that should be easy. I'll update what I can and see how it looks.

Out of curiosity (I'm old-school git, not too familiar with GitHub), I just force-push the branch to update this PR, correct?

[jac-cbi]

Ok, sorry for the double-force. I forgot that rebase blows out the commit signatures...

With this version of the branch, prior to the fix commit, we get the following, as expected:

$ cargo test -- --show-output
...
     Running tests/sk_not_present.rs (target/debug/deps/sk_not_present-5eb4af9fc80a4a5e)

running 1 test
test test_sk_not_present ... FAILED

successes:

successes:

failures:

---- test_sk_not_present stdout ----
DEBUG: found a matching key: SHA256:1UwbHkBzDpgQWYVRkrZRHKRx9n0Gzfld+alc8pD7jwg
thread 'test_sk_not_present' panicked at tests/sk_not_present.rs:71:71:
called `Result::unwrap()` on an `Err` value: The remote ssh agent returned the failure message
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    test_sk_not_present

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
...

After the fix commit:

$ cargo test -- --show-output
...
     Running tests/sk_not_present.rs (target/debug/deps/sk_not_present-5eb4af9fc80a4a5e)

running 1 test
test test_sk_not_present ... ok

successes:

---- test_sk_not_present stdout ----
DEBUG: found a matching key: SHA256:1UwbHkBzDpgQWYVRkrZRHKRx9n0Gzfld+alc8pD7jwg
DEBUG: SSHAgent: RemoteFailure; trying next key
DEBUG: found a matching key: SHA256:5RBDOSvATUadn+TDt8S320/ozyfQqY525Cv70tGwCOE


successes:
    test_sk_not_present

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s
...

[nresare]

Thank you for spending the time addressing my comments, I am genuinely happy that someone got around to play around with my code. Please see some minor comments below.

Also, I'd like all these changes to go in as a single commit, to have the git log be neat and tidy, so please sqash them into one and force push onto your PR branch

[jac-cbi]

yup, good catch with the test case, I def over-engineered that pig 😂

[nresare]

Thank you!

[nresare]

And now its just some tiny rustfmt fixes and a trivial clippy thing to address :)

[jac-cbi]

hmm, I did neglect to run rustfmt this last pass, but it did pass before.. I'll check both and re-push

[jac-cbi]

ok, should pass now.

[nresare]

so close. It is a bit obnoxious with the automated tests, but on the other hand it brings some value. Let me create a Makefile target that runs the same checks locally as is done by the github action, and the process should be a bit smoother next time

[nresare]

I just landed a make check target in the main branch. If you rebase your diff you should be able to verify locally that the change is likely to pass the automated check

[jac-cbi]

I love that cargo fmt removed the assert_eq(true ... in one round, then in the next round put it on a single line... sigh. 🤣

Ran make check several times, just to make sure. anything worth doing once is worth doing three times

[nresare]

🥳