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.

Sign up for GitHub

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

Jump to bottom

Bump x/crypto to 20220518 and remove custom algorithm signer #12674

Merged
merged 3 commits into from May 25, 2022
Merged

Bump x/crypto to 20220518 and remove custom algorithm signer #12674

merged 3 commits into from May 25, 2022

Conversation

marcoandredinis
Copy link
Contributor

@marcoandredinis marcoandredinis commented May 16, 2022 •
edited

This commit upgrades the version of x/crypto we use, to the current latest
go get -u golang.org/x/crypto

We also replaced the deprecated variables and updated the tests to match the
current default KEX Algos

The x/crypto didn't support RSA-SHA2 algos, so we developed our own algorithm
signer. This is no longer the case, and after upgrading x/crypto to 20220518 we
can safely remove the custom code we have.

TODO (from #11178 (comment))

  • Set ca_signature_algo to ssh-rsa and make sure that:
    • OpenSSH clients < 8.2 work fine.
    • OpenSSH 8.2 which deprecated SHA1 work with PubkeyAcceptedKeyTypes workaround mentioned above.
    • OpenSSH 8.8 removed support for SHA1, does it still work with the workaround above?
  • Set ca_signature_algo to rsa-sha2-512 and make sure that:
    • All OpenSSH clients work fine if we apply the workaround above, otherwise 8.8+ clients fail

@marcoandredinis marcoandredinis force-pushed the marco/bump_crypto_20220513 branch 3 times, most recently from 0d129e4 to 602b9fc Compare May 18, 2022 14:14
@marcoandredinis
Copy link
Contributor Author

marcoandredinis commented May 18, 2022 •
edited

After a couple of tests, I got the following matrix

Teleport Version ca_signature_algo SSH V Result With Workaround
9.2.3 not set 8.2 ✅ ✅
9.2.3 not set 8.9 ❌ ✅
9.2.3 rsa-sha2-512 8.2 ✅ ✅
9.2.3 rsa-sha2-512 8.9 ❌ ✅
marco/bump_crypto_20220513 not set 8.2 ✅ ✅
marco/bump_crypto_20220513 not set 8.9 ❌ ✅
marco/bump_crypto_20220513 rsa-sha2-512 8.2 ✅ ✅
marco/bump_crypto_20220513 rsa-sha2-512 8.9 ❌ ✅

The good news is that, it looks like we didn't break anything with the lib bump
The bad news is that, it seems we don't handle OpenSSH v8.9 clients (probably, anything greater than v8.8)

From OpenSSH 8.8+, it works if we explicitly add the older algorithm
Somthing like this: ./ssh -vvv -oPubkeyAcceptedAlgorithms=+ssh-rsa-cert-v01@openssh.com teleportadmin@moon.marco.mydemo

@marcoandredinis marcoandredinis force-pushed the marco/bump_crypto_20220513 branch 2 times, most recently from eb36bfa to 3f711f5 Compare May 23, 2022 18:17
@marcoandredinis marcoandredinis marked this pull request as ready for review May 23, 2022 18:19
@marcoandredinis marcoandredinis changed the title Bump x/crypto to 20220513 Bump x/crypto to 20220518 and remove custom algorithm signer May 23, 2022
@marcoandredinis marcoandredinis marked this pull request as draft May 23, 2022 18:44
@marcoandredinis marcoandredinis marked this pull request as ready for review May 23, 2022 21:45
rosstimothy
rosstimothy approved these changes May 24, 2022
View reviewed changes
Copy link
Contributor

@rosstimothy rosstimothy left a comment

Choose a reason for hiding this comment

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

The changes here all seem reasonable to me. Just a couple questions around the OpenSSH 8.9 incompatibility:

  • Do we have an open issue regarding OpenSSH 8.9 clients not functioning without the workaround?
  • Is the workaround documented anywhere else but here?

@marcoandredinis
Copy link
Contributor Author

We have one issue here, with the workaround
I failed to find he workaround in our docs

Do you think we should add it?

@rosstimothy
Copy link
Contributor

We have one issue here, with the workaround I failed to find he workaround in our docs

Do you think we should add it?

Maybe just add a comment to that issue with the workaround?

r0mant
r0mant reviewed May 24, 2022
View reviewed changes
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.

@marcoandredinis Thank you for pushing this through, great job!

One question I have is regarding backwards compatibility. Since we've removed the "alg signers" workaround, will clusters that set ca_signature_algo configuration field be impacted in any way?

@marcoandredinis
Update x/crypto to 20220513
29c6fec 
This commit upgrades the version of x/crypto we use, to the current latest
`go get -u golang.org/x/crypto`

We also replaced the deprecated variables and updated the tests to match the
current default KEX Algos
@marcoandredinis
Remove custom signer
8587a66 
The x/crypto didn't support RSA-SHA2 algos, so we developed our own algorithm
signer.

This is no longer the case, and after upgrading x/crypto to 20220513 we can
safely remove the custom code we have.
@marcoandredinis
update x/crypto to new version
e0b384b
@marcoandredinis
Copy link
Contributor Author

@marcoandredinis Thank you for pushing this through, great job!

One question I have is regarding backwards compatibility. Since we've removed the "alg signers" workaround, will clusters that set ca_signature_algo configuration field be impacted in any way?

They should not be impacted
I did some tests locally

  • starting with no explicit algorithm
  • restarting with ssh-rsa
  • restarting with rsa-sha2-512

In between each step, I connected with the OpenSSH client successfully.

I'm pretty confident we can remove that property
After this PR, we will rely on the x/crypto to decide what Signature Algorithm to use (default is sha512), during the server/client negotiation

Removing the configuration option in this PR will delay the version bump which is more important, so I don't think we should do it know.
I'll open an issue for that and merge this PR

@marcoandredinis marcoandredinis merged commit 2493448 into master May 25, 2022
@marcoandredinis marcoandredinis mentioned this pull request May 25, 2022
Closed
@marcoandredinis marcoandredinis deleted the marco/bump_crypto_20220513 branch May 25, 2022 13:51
@marcoandredinis marcoandredinis mentioned this pull request Jun 2, 2022
Merged
marcoandredinis added a commit that referenced this pull request Jun 6, 2022
@marcoandredinis
Deprecate ca_signature_algo config (#13033)
306d011 
After the merge of #12674 we no longer use the following configuration:
```yaml
teleport:
    ca_signature_algo: "rsa-sha2-512"
```
As we now rely upon the `x/crypto` package to choose the signing algorithm (it defaults to `rsa-sha2-512`)

**Demo**
If we set `ca_signature_algo` (the value is irrelevant) and start `teleport` we get:
```shell
root@marco:/workspace# teleport start --debug
2022-06-02T09:33:58Z WARN             ca_signing_algo config option is deprecated and will be ignored, we'll always default to rsa-sha2-512. config/configuration.go:348
2022-06-02T09:33:58Z INFO             Generating new host UUID: b001159a-10e0-49a7-b4dc-61c73fbe9e42. service/service.go:726
...
```

Fixes #12905
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@r0mant r0mant r0mant left review comments

@xacrimon xacrimon xacrimon approved these changes

@rosstimothy rosstimothy rosstimothy approved these changes

@Joerger Joerger Awaiting requested review from Joerger

@timothyb89 timothyb89 Awaiting requested review from timothyb89

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@marcoandredinis @rosstimothy @r0mant @xacrimon