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

Remove SHA-1 signed keys #16912

Closed
wants to merge 20 commits into from
Closed

Remove SHA-1 signed keys #16912

wants to merge 20 commits into from

Conversation

jakule
Copy link
Contributor

@jakule jakule commented Sep 30, 2022

This PR switches Go crypto library to our fork that supports extension negotiation (RFC 8308).
It also removes PubkeyAcceptedAlgorithms [email protected] from the SSH config file as this option is no longer needed.
I also refactored the SSH config generation logic as we had to separate generators. One for tsh config and the second for tbot that were doing the same thing.

Closes #10918
Closes #15149
Closes #15633

@r0mant r0mant linked an issue Sep 30, 2022 that may be closed by this pull request
@jakule jakule force-pushed the jakule/remove-sha-1 branch from 6e3166b to 2a2226e Compare October 2, 2022 21:05
@jakule jakule marked this pull request as ready for review October 3, 2022 05:24
@github-actions github-actions bot added machine-id tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Oct 3, 2022
@jakule jakule requested a review from timothyb89 October 3, 2022 05:32
go.mod Outdated Show resolved Hide resolved
lib/config/openssh.go Outdated Show resolved Hide resolved
lib/config/openssh.go Show resolved Hide resolved
UserKnownHostsFile "{{ $dot.KnownHostsPath }}"
IdentityFile "{{ $dot.IdentityFilePath }}"
CertificateFile "{{ $dot.CertificateFilePath }}"{{- if $dot.NewerHostKeyAlgorithmsSupported }}
HostKeyAlgorithms [email protected],[email protected]{{- else }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to include these explicitly? Can't just not set host key algos in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because of 3rd party libraries that I don't want to mention here. I changed the logic to work with SHA-2 and SHA-1 if the first one is not supported.

@jakule jakule requested a review from r0mant October 4, 2022 21:09
@jakule jakule force-pushed the jakule/remove-sha-1 branch from 5d44945 to fb29329 Compare October 5, 2022 18:21
Copy link
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

lgtm but let's please wait for @timothyb89 to review as well

go.mod Outdated Show resolved Hide resolved
Co-authored-by: Roman Tkachenko <[email protected]>
@github-actions github-actions bot removed the request for review from timothyb89 October 5, 2022 19:06
Copy link
Contributor

@timothyb89 timothyb89 left a comment

Choose a reason for hiding this comment

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

This is a huge improvement, and works perfectly on my machine. Thank you!

@jakule jakule enabled auto-merge (squash) October 5, 2022 19:35
@jakule jakule marked this pull request as draft October 7, 2022 01:29
auto-merge was automatically disabled October 7, 2022 01:29

Pull request was converted to draft

@jakule
Copy link
Contributor Author

jakule commented Oct 7, 2022

I converted this PR back to a draft as we found out that crypto SSH patch breaks older OpenSSH clients.

@jakule
Copy link
Contributor Author

jakule commented Nov 7, 2022

I'm closing this PR. Most of the changes introduced in this PR have been merged as #17138 the rest went to #18221

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
machine-id tsh tsh - Teleport's command line tool for logging into nodes running Teleport.
Projects
None yet
4 participants