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

Deprecate ca_signature_algo config #13033

Merged
merged 7 commits into from Jun 6, 2022

Conversation

marcoandredinis
Copy link
Contributor

@marcoandredinis marcoandredinis commented May 31, 2022

After the merge of #12674 we no longer use the following configuration:

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:

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

@marcoandredinis marcoandredinis force-pushed the marco/deprecate_ca_signature_algo branch 3 times, most recently from a923483 to 4e4ff7b Compare June 2, 2022 07:42
@marcoandredinis marcoandredinis changed the title deprecate ca_signature_algo Deprecate ca_signature_algo config Jun 2, 2022
@marcoandredinis marcoandredinis changed the title Deprecate ca_signature_algo config Deprecate ca_signature_algo config Jun 2, 2022
@marcoandredinis marcoandredinis force-pushed the marco/deprecate_ca_signature_algo branch from 4e4ff7b to 4f14b89 Compare June 2, 2022 09:26
@marcoandredinis marcoandredinis marked this pull request as ready for review June 2, 2022 10:04
Copy link
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup, @marcoandredinis !

lib/config/configuration.go Outdated Show resolved Hide resolved
@@ -420,9 +414,6 @@ func (conf *FileConfig) CheckAndSetDefaults() error {
return trace.BadParameter("MAC algorithm %q is not supported; supported algorithms: %q", m, sc.MACs)
}
}
if conf.CASignatureAlgorithm != nil && !apiutils.SliceContainsStr(validCASigAlgos, *conf.CASignatureAlgorithm) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a deprecation notice at fileconf.go / Global.CASignatureAlgorithm too, so it's clear the field is unused and kept only for config backwards compatibility.

Is there a "Cloud"/protobuf counterpart for this setting, or just the fileconf / teleport.yaml one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deprecation notice added 🙏

Is there a "Cloud"/protobuf counterpart for this setting, or just the fileconf / teleport.yaml one?

CertAuthorityV2 has two methods that I'm pretty sure could be removed: Get/SetSigningAlg

My concern is having a version without those methods interacting with a version that has them
I don't know how that works out (backwards/forward compat)
I'll do some tests 🛠️

Copy link
Contributor

Choose a reason for hiding this comment

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

If you drop the methods and an older gRPC client tries to call them, it'll get a framework-level error saying the method doesn't exist (I don't remember the exact error, but it's something like that).

Whether it's safe to drop depends on how it's used. If you can ensure all calls are removed by version N, then it's safe to drop the method in N+1.

@marcoandredinis marcoandredinis force-pushed the marco/deprecate_ca_signature_algo branch 2 times, most recently from 39f7317 to 6bb0b52 Compare June 2, 2022 14:30
@codingllama
Copy link
Contributor

LGTM Marco, let me know if you intend to do further work in this PR, otherwise I'm happy to approve.

Copy link
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

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

Looks good, but there's also a reference to ca_signature_algo in our docs - please update that as well.

@marcoandredinis marcoandredinis force-pushed the marco/deprecate_ca_signature_algo branch 2 times, most recently from 5956d4f to 855e461 Compare June 3, 2022 10:24
@marcoandredinis
Copy link
Contributor Author

@zmb3 @codingllama
Can you please take another look?
I removed some more files, and I'm confident the only leftovers are the gRPC messages, which can be removed in the next major version

I tested using teleport built from this branch against tsh v9.3.2 and it worked:

root@moon:/workspace/teleport# tsh version
Teleport v10.0.0-dev git:teleport-connect-preview-1.0.0.dev.1-252-g855e461ca go1.17.10
Proxy version: 10.0.0-dev
root@moon:/workspace/teleport# tsh ssh root@marco.mydemo
root@marco:~# exit
logout
the connection was closed on the remote side on  03 Jun 22 10:45 UTC
root@moon:/workspace/teleport# ./tsh version
Teleport v9.3.2 git:v9.3.2-0-gb3597d12a go1.17.10
root@moon:/workspace/teleport# ./tsh ssh root@marco.mydemo
root@marco:~# exit
logout
the connection was closed on the remote side on  03 Jun 22 10:45 UTC

@marcoandredinis marcoandredinis force-pushed the marco/deprecate_ca_signature_algo branch 2 times, most recently from 3ec18c5 to c6c3a43 Compare June 3, 2022 14:15
Copy link
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

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

LGTM.

All suggestions are about removing redundant initializations. Feel free to push back/ignore as you see fit.

integration/helpers.go Outdated Show resolved Hide resolved
integration/helpers.go Outdated Show resolved Hide resolved
lib/auth/clt_test.go Outdated Show resolved Hide resolved
lib/config/configuration.go Outdated Show resolved Hide resolved
lib/reversetunnel/srv_test.go Outdated Show resolved Hide resolved
lib/services/watcher_test.go Outdated Show resolved Hide resolved
tool/tctl/common/auth_command_test.go Outdated Show resolved Hide resolved
@marcoandredinis marcoandredinis force-pushed the marco/deprecate_ca_signature_algo branch from c6c3a43 to 6661272 Compare June 3, 2022 15:29
@marcoandredinis marcoandredinis requested a review from zmb3 June 3, 2022 15:30
@marcoandredinis marcoandredinis force-pushed the marco/deprecate_ca_signature_algo branch 5 times, most recently from 53888f3 to 319d0eb Compare June 6, 2022 13:57
lib/config/configuration.go Outdated Show resolved Hide resolved
@marcoandredinis marcoandredinis force-pushed the marco/deprecate_ca_signature_algo branch from 465db76 to fa3f3d0 Compare June 6, 2022 14:45
@marcoandredinis marcoandredinis force-pushed the marco/deprecate_ca_signature_algo branch from fa3f3d0 to 147bed9 Compare June 6, 2022 14:54
@marcoandredinis marcoandredinis merged commit 306d011 into master Jun 6, 2022
@marcoandredinis marcoandredinis deleted the marco/deprecate_ca_signature_algo branch June 6, 2022 15:18
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.

Remove ca_signature_algo configuration
3 participants