SSH client must prove key ownership before proceeding with MFA checks

[cthach]

Resolves #61557.

This change integrates our regular, forward and git SSH servers with x/crypto/ssh#ServiceConfig.VerifiedPublicKeyCallback.

Backport?

No, since in-band MFA is going to be introduced in v19, this change doesn't benefit v18 or v17.

Manual Test Plan

Test Environment

Deployed locally to my Mac since I can use my Mac as my target SSH host.

I used a trial GitHub org named cthach-dev for my tsh git ... tests.

Test Cases

List the various tests that were run to validate the change. Cases should be as exhaustive as possible.

Per-session MFA NOT required, node recording

• SSH to host via root cluster
• SSH to host via leaf cluster
• Execute tsh git clone command using SSH proxying

Per-session MFA required, node recording

• SSH to host via root cluster
• SSH to host via leaf cluster
• Execute tsh git clone command using SSH proxying

Per-session MFA NOT required, proxy recording

• SSH to host via root cluster
• SSH to host via leaf cluster
• Execute tsh git clone command using SSH proxying

Per-session MFA required, proxy recording

This scenario doesn't currently work with the legacy public key auth method (see #8843), so no tests performed for this scenario.

[cthach]

@espadolini @rosstimothy can you both take a preliminary look just so I can confirm whether this approach is sound to integrating x/crypto/ssh#ServerConfig. VerifiedPublicKeyCallback?

I have still have automated tests to write and manual tests to do, but before I do that, I wanted your input before barreling forward.

[espadolini]

I don't think this approach is sound; it will work without being too problematic as long as signatures are not gated behind user confirmation (which they might be when using PIV, or if someone has an agent configured to always ask for confirmation) but it will still cause additional roundtrips for any key that passes the initial check.

Other than factors outside of our direct influence, PublicKeyCallback and VerifiedPublicKeyCallback should let the same keys through; VerifiedPublicKeyCallback should be used if we actively have to do something in response to a key being used (since a key being proposed through PublicKeyCallback doesn't actually prove ownership) such as opening a connection to a remote server to forward a connection for example - and, since it happens during auth, it lets us steer the auth towards some additional auth methods, for example if said remote server asks for in-band MFA or maybe straight up PAM challenge-response things.

[espadolini]

I'm also not sure if there's SSH clients in the wild that will react poorly to being told that a key is valid at the initial check but rejected after the signature - since that's something that normally never happens, I bet that we're going to hit some strange behaviors by doing this.

[cthach]

Really stellar feedback, @espadolini, thank you 🏆

Taking your feedback into account, I've come up with a counter proposal to address your points and just to make sure I'm interpreting your feedback correctly, I put it into bullet points.

New approach:

1. PublicKeyCallback continues to do full cert and RBAC checks like we already currently do
2. We extract the auth steering (e.g., keyboard-interactive) and put it into VerifiedPublicKeyCallback
3. PublicKeyCallback and VerifiedPublicKeyCallback WILL NOT duplicate each others auth logic
4. VerifiedPublicKeyCallback WILL NOT reject a key that was accepted by PublicKeyCallback
5. PublicKeyCallback will pass permits and other auth metadata via ssh.Permissions struct like we currently already do

I don't think this approach is sound; it will work without being too problematic as long as signatures are not gated behind user confirmation (which they might be when using PIV, or if someone has an agent configured to always ask for confirmation) but it will still cause additional roundtrips for any key that passes the initial check.

Because we're keeping the status quo in PublicKeyCallback and it continues to do the same core auth logic, I believe this is no longer a concern.

Other than factors outside of our direct influence, PublicKeyCallback and VerifiedPublicKeyCallback should let the same keys through;

Points 1, 2, 3, 4 should cover this this. PublicKeyCallback does core auth and VerifiedPublicKeyCallback does post-verification auth steering.

VerifiedPublicKeyCallback should be used if we actively have to do something in response to a key being used (since a key being proposed through PublicKeyCallback doesn't actually prove ownership) such as opening a connection to a remote server to forward a connection for example - and, since it happens during auth, it lets us steer the auth towards some additional auth methods, for example if said remote server asks for in-band MFA or maybe straight up PAM challenge-response things.

Yes, covered in point 2

I'm also not sure if there's SSH clients in the wild that will react poorly to being told that a key is valid at the initial check but rejected after the signature - since that's something that normally never happens, I bet that we're going to hit some strange behaviors by doing this.

Shouldn't be an issue anymore with this new approach primarily because of point 4.

I'm just being super thorough to make sure I covered all your concerns. Let me know if I missed anything.

I'll start working on refactoring.

[espadolini]

That should be good, the entire point of passing the ssh.Permissions to VerifiedPublicKeyCallback was to reuse "trusted" data that was already calculated.

If during your refactor you find it convenient to get rid of the stupid serialization/deserialization we're currently doing to shove data through ssh.Permissions now that we have a whole map[any]any to work with, that'd be very appreciated. 🙏

[cthach]

That should be good, the entire point of passing the ssh.Permissions to VerifiedPublicKeyCallback was to reuse "trusted" data that was already calculated.

If during your refactor you find it convenient to get rid of the stupid serialization/deserialization we're currently doing to shove data through ssh.Permissions now that we have a whole map[any]any to work with, that'd be very appreciated. 🙏

I pushed another revision. I left out the refactor of the serialization/deserialization of passing data through ssh.Permissions. It would have been much more change and risk given how much it is used.

I'll proceed with adding automated tests and doing manual regression testing according to the defined test plan in the description. If everything passes, I'll open this for review.

[cthach]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b0b6de15bd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Avoid re-triggering MFA on fallback publickey auth

This unconditionally sends any permit with preconditions back through KeyboardInteractiveAuth, but x/crypto/ssh invokes VerifiedPublicKeyCallback after every successful public-key callback (including the Next.PublicKeyCallback from a prior PartialSuccessError). In the legacy fallback path, the second public-key attempt therefore gets converted into another partial success instead of completing, which can trap clients without keyboard-interactive support in an auth loop and break per-session-MFA logins that previously relied on the compatibility callback.

Useful? React with 👍 / 👎.

[cthach]

This is really good feedback. It is true there is an auth loop in certain cases. Working on a fix now.

[cthach]

Fixed in 69c2480