monitor ssh connection live-ness

[jbardin]

An unresponsive server or dead tcp connection can hang a provisioner indefinitely.
Monitor the ssh connection status, and abort if the server becomes unresponsive for more than 120s.

Unfortunately the connection timeout parameter is already taken to mean "retry timeout", and is often set to an inappropriate value to use for monitoring the connection status. Rather than providing another similarly named option for users, we'll set a conservative timeout of 120s (somewhat arbitrarily chosen to match the arbitrarily chosen default TCP MSL value).

Fixes #12139

[pselle]

Looks good to me, and cool with it, but any notes on updating the crypto dependencies?

[pselle]

Nice comment, and nicely done

[jbardin]

The ssh package is part of the golang.org/x/crypto module, so they come as one unit, and it's also generally a good idea to keep the crypto stuff up to date. The golang.org/x/ packages tend to be developed in concert with the std lib, and I like to update them along with Go releases (though they don't have specific releases of their own).

[apparentlymart]

I agree that keeping these dependencies up-to-date is good, but I'd say let's also review the changelog for the library to see if there are any changes that would effect emergent behavior for our end-users.

The SSH features in particular have been a source of minor churn in the past, I seem to recall, because the support for various SSH features in the upstream library has shifted over time. In this case, just from skimming the diff it looks like e.g. the library may now have GSSAPI (Kerberos) support, which would be great to declare in the changelog if we can verify that is so. (Getting a Kerberos-enabled sshd running to test it is likely bothersome, though. 😖 )

[jbardin]

There weren't many changes:

$ git log --oneline a29dc8fdc734..4def268fd1a4
4def268 (HEAD -> master, origin/master, origin/HEAD) ssh: skip testHandshakeErrorHandlingN on js/wasm
cc06ce4 acme: send User-Agent and add Client.UserAgent
ea8f1a3 ed25519: turn into a wrapper for crypto/ed25519 beginning with Go 1.13
57b3e21 ssh: add diffie-hellman-group-exchange-sha256
5c40567 internal/chacha20: fix variable naming
f99c8df poly1305: improve performance with asm for ppc64le
20be4c3 internal/chacha20: improve performance for ppc64le
22d7a77 sha3: fix bug in cSHAKE Clone()
cbcb750 ssh/gss: support kerberos authentication for ssh server and client
e1dfcc5 openpgp: replace "currentTime" with "creationTime" as appropriate

The diffie-hellman-group-exchange-sha256 kex was primarily what I wanted to keep compatibility as broad as possible. The gssapi support might be nice to have in the future, but not something we want to try and implement right now.

[apparentlymart]

If upgrading doesn't automatically introduce any support for GSSAPI without other changes on our end then that's fine. I just worry about new features we don't expect being introduced and becoming compatibility constraints even though we didn't get a chance to design how they interact with Terraform.

The intent of my previous comment was just to ask that we make sure we know about any new features this might introduce automatically, not to make any special effort to opt into new features explicitly.

[jbardin]

Gotcha. Yes, the only new code automatically used will be the new key exchange. The auth methods need to be explicitly configured.

[apparentlymart]

Great! In that case, it sounds like just mentioning that we now support that key exchange method in the changelog will do the job here.

[pselle]

Putting an approval on this, because the comment about CHANGELOG should be done after a merge

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.