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

✨ Allow TLS minimum version to be configured #1548

Merged
merged 2 commits into from Jun 9, 2021
Merged

✨ Allow TLS minimum version to be configured #1548

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c8ecd75
Allow TLS minimum version to be configured
willthames Jun 7, 2021
acdafe1
Add error handling to tls version conversion
willthames Jun 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 20 additions & 0 deletions pkg/webhook/server.go
View file
Open in desktop
Expand Up @@ -70,13 +70,21 @@ type Server struct {
// Defaults to "", which means server does not verify client's certificate.
ClientCAName string

// TLSVersion is the minimum version of TLS supported. Accepts
// "1.1", "1.2" and "1.3" - anything else will result in "1.0"
TLSMinVersion string

// WebhookMux is the multiplexer that handles different webhooks.
WebhookMux *http.ServeMux

// webhooks keep track of all registered webhooks for dependency injection,
// and to provide better panic messages on duplicate webhook registration.
webhooks map[string]http.Handler

// tlsMinVersion is the result of the conversion from human-readable TLS version (for example "1.1")
// to the values accepted by tls.Config (for example 0x301)
tlsMinVersion uint16

// setFields allows injecting dependencies from an external source
setFields inject.Func

Expand Down Expand Up @@ -109,6 +117,17 @@ func (s *Server) setDefaults() {
if len(s.KeyName) == 0 {
s.KeyName = "tls.key"
}

switch s.TLSMinVersion {
case "1.1":
s.tlsMinVersion = tls.VersionTLS11
case "1.2":
s.tlsMinVersion = tls.VersionTLS12
case "1.3":
s.tlsMinVersion = tls.VersionTLS13
default:
Copy link
Member

Choose a reason for hiding this comment

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

IMHO we should error if TLSMinVersion is set but not in the list, silently picking an old version is not a good idea IMHO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mind suggesting a mechanism for this (setDefaults doesn't currently have any error handling so I'm not sure what pattern to follow)

Copy link
Member

Choose a reason for hiding this comment

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

I would just move this into Start. The only thing that really qualifies as defaulting is to set it to tls.VersionTLS10 if s.TLSMinVersion is an empty string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for that suggestion - hopefully this iteration does what you're after.

s.tlsMinVersion = tls.VersionTLS10
}
}

// NeedLeaderElection implements the LeaderElectionRunnable interface, which indicates
Expand Down Expand Up @@ -200,6 +219,7 @@ func (s *Server) Start(ctx context.Context) error {
cfg := &tls.Config{
NextProtos: []string{"h2"},
GetCertificate: certWatcher.GetCertificate,
MinVersion: s.tlsMinVersion,
}

// load CA to verify client certificate
Expand Down