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

TLS1.3 support #1364

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

TLS1.3 support #1364

wants to merge 5 commits into from

Conversation

sanchezl
Copy link
Contributor

  • bump library-go
  • use new NewStaticPodOperatorClient signature
  • add ETCD_TLS_MIN_VERSION env to etcd pods
  • add listen-tls-min-version flag to readyz cmd
  • do not start etcd with --cipher-suites with tls1.3

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 31, 2024
Copy link
Contributor

openshift-ci bot commented Oct 31, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link
Contributor

openshift-ci bot commented Oct 31, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sanchezl
Once this PR has been reviewed and has the lgtm label, please assign tjungblu for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Comment on lines +183 to +189
var tlsMinVersion uint16
switch r.tlsMinVersion {
case "TLS1.3":
tlsMinVersion = tls.VersionTLS13
case "TLS1.2":
tlsMinVersion = tls.VersionTLS12
}
Copy link
Contributor

Choose a reason for hiding this comment

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

feel free to use "go.etcd.io/etcd/client/pkg/v3/tlsutil" here too

@@ -304,6 +308,33 @@ func getCipherSuites(envVarContext envVarContext) (map[string]string, error) {
}, nil
}

func getTLSMinVersion(envVarContext envVarContext) (map[string]string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

given your extraordinary gotpl skills, I'm wondering whether we should just check this inside of the cipher suite function above in conjunction.

That way we can enable/disable the suites depending on the TLS version. I'm mostly curious about how we can surface this back to a user somehow. I know some (especially FIPS) customers run compliance scanners that will alert them on not setting those ciphers.

@@ -173,7 +174,7 @@ func RunOperator(ctx context.Context, controllerContext *controllercmd.Controlle
// we keep the default behavior of exiting the controller once a gate changes
featureGateAccessor.SetChangeHandler(featuregates.ForceExit)

operatorClient, dynamicInformers, err := genericoperatorclient.NewStaticPodOperatorClient(controllerContext.KubeConfig, operatorv1.GroupVersion.WithResource("etcds"), operatorv1.GroupVersion.WithKind("Etcd"), ExtractStaticPodOperatorSpec, ExtractStaticPodOperatorStatus)
operatorClient, dynamicInformers, err := genericoperatorclient.NewStaticPodOperatorClient(clock.RealClock{}, controllerContext.KubeConfig, operatorv1.GroupVersion.WithResource("etcds"), operatorv1.GroupVersion.WithKind("Etcd"), ExtractStaticPodOperatorSpec, ExtractStaticPodOperatorStatus)
Copy link
Contributor

Choose a reason for hiding this comment

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

do you mind to split out the library-go update into a separate PR? we can merge that earlier, I see there is a bunch of static pod changes there too, which we might want to separate out - just in case they mess up CI somehow.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh you already got #1365 - resolved :) thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants