Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix compatibility with sshd 7.x #18181

Merged
merged 9 commits into from
Nov 10, 2022
Merged

Fix compatibility with sshd 7.x #18181

merged 9 commits into from
Nov 10, 2022

Conversation

jakule
Copy link
Contributor

@jakule jakule commented Nov 4, 2022

This change introduces a workaround for OpenSSH 7.x bug related to SHA-2 signed certificates. Teleport signs all certificates with SHA-2-512, which prevents us from connecting to older sshd. As a workaround, we will send the same certificate signed with SHA-2-512 (as we do right now) and SHA-1 if the first authentication fails. We always send the SHA-2 certificates first, so newer OpenSSH versions should always use SHA-2 certs as they do right now.

@jakule jakule changed the title [WIP] Fix compatibility with sshd 7.x Fix compatibility with sshd 7.x Nov 8, 2022
@jakule jakule marked this pull request as ready for review November 8, 2022 18:31
Comment on lines 653 to 666
combinedSigners := make([]ssh.Signer, len(signers), 2*len(signers))
// Copy the original signers. All of them should be using SHA2-512 for signing.
// Order is important here as we want SHA2-signers to be used first.
copy(combinedSigners[:], signers)

for _, signer := range signers {
if s, ok := signer.(ssh.AlgorithmSigner); ok && s.PublicKey().Type() == ssh.CertAlgoRSAv01 {
// If certificate signer supports Sign(rand io.Reader, data []byte) method,
// add SHA-1 signer.
combinedSigners = append(combinedSigners, &sshutils.LegacySHA1Signer{Signer: s})
}
}

return combinedSigners, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the copy(combinedSigners[:], signers) call is required ? Is still makes the shallow copy of Singer object.
What would you you say about following simplification:

Suggested change
combinedSigners := make([]ssh.Signer, len(signers), 2*len(signers))
// Copy the original signers. All of them should be using SHA2-512 for signing.
// Order is important here as we want SHA2-signers to be used first.
copy(combinedSigners[:], signers)
for _, signer := range signers {
if s, ok := signer.(ssh.AlgorithmSigner); ok && s.PublicKey().Type() == ssh.CertAlgoRSAv01 {
// If certificate signer supports Sign(rand io.Reader, data []byte) method,
// add SHA-1 signer.
combinedSigners = append(combinedSigners, &sshutils.LegacySHA1Signer{Signer: s})
}
}
return combinedSigners, nil
var ss []ssh.Signer
for _, signer := range signers {
if s, ok := signer.(ssh.AlgorithmSigner); ok && s.PublicKey().Type() == ssh.CertAlgoRSAv01 {
// If certificate signer supports Sign(rand io.Reader, data []byte) method,
// add SHA-1 signer.
ss = append(ss, &sshutils.LegacySHA1Signer{Signer: s})
}
}
return apend(signers, ss...), nil

lib/sshutils/signer.go Show resolved Hide resolved
// signersWithSHA1Fallback returns the signers provided by signersCb and appends
// the same signers with forced SHA-1 signature to the end. This behavior is
// required as older OpenSSH version <= 7.6 don't support SHA-2 signed certificates.
func signersWithSHA1Fallback(signersCb func() ([]ssh.Signer, error)) func() ([]ssh.Signer, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I don't have strong opinion but for me the function callback funcations are hard to track so wonder if we can consider calling signersCb before entering the signersWithSHA1Fallback function.

Suggested change
func signersWithSHA1Fallback(signersCb func() ([]ssh.Signer, error)) func() ([]ssh.Signer, error) {
func signersWithSHA1Fallback(signers []ssh.Signer) func() ([]ssh.Signer, error) {
signers, err  := ssh.PublicKeysCallback(signersWithSHA1Fallback(s.userAgent.Signers))
if err != nil {
	return nil, trace.Wrap(err)
}
authMethod := ssh.PublicKeysCallback(signersWithSHA1Fallback(signers))

"golang.org/x/crypto/ssh"
)

func Test_signersWithSHA1Fallback(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a new standard for naming tests? I've been doing TestName, but see that some have been doing Test_name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used my IDE to generate that test. I'll rename it.

lib/srv/forward/sshserver.go Show resolved Hide resolved
@jakule jakule enabled auto-merge (squash) November 10, 2022 22:34
@jakule jakule merged commit d68c909 into master Nov 10, 2022
@github-actions
Copy link

@jakule See the table below for backport results.

Branch Result
branch/v10 Create PR
branch/v11 Create PR

jakule added a commit that referenced this pull request Nov 14, 2022
jakule added a commit that referenced this pull request Nov 18, 2022
@zmb3 zmb3 deleted the jakule/sha1-sshd branch May 7, 2024 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants