-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Support listening on TLS #2963
Open
frebib
wants to merge
1
commit into
google:master
Choose a base branch
from
frebib:feat/tls
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Support listening on TLS #2963
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,8 +16,10 @@ package main | |
|
||
import ( | ||
"crypto/tls" | ||
"crypto/x509" | ||
"flag" | ||
"fmt" | ||
"net" | ||
"net/http" | ||
"net/http/pprof" | ||
"os" | ||
|
@@ -55,6 +57,11 @@ var httpAuthRealm = flag.String("http_auth_realm", "localhost", "HTTP auth realm | |
var httpDigestFile = flag.String("http_digest_file", "", "HTTP digest file for the web UI") | ||
var httpDigestRealm = flag.String("http_digest_realm", "localhost", "HTTP digest file for the web UI") | ||
|
||
var tlsCertPath = flag.String("tls_cert_path", "", "TLS certificate file path to serve cadvisor HTTPS with. Must be specified along with -tls_key_path") | ||
var tlsKeyPath = flag.String("tls_key_path", "", "TLS key file path to serve cadvisor HTTPS with. Must be specified along with -tls_cert_path") | ||
var tlsCAPath = flag.String("tls_ca_path", "", "TLS certificate authority file path. Used to verify TLS clients connecting to cadvisor. System CA roots will be used if this is not specified.") | ||
var tlsClientAuth = flag.String("tls_client_auth", "require", "TLS authentication mode, must be one of request, optional, requireany, require or none.") | ||
|
||
var prometheusEndpoint = flag.String("prometheus_endpoint", "/metrics", "Endpoint to expose Prometheus metrics on") | ||
|
||
var enableProfiling = flag.Bool("profiling", false, "Enable profiling via web interface host:port/debug/pprof/") | ||
|
@@ -152,6 +159,9 @@ func main() { | |
klog.Fatalf("Failed to register HTTP handlers: %v", err) | ||
} | ||
|
||
// Generate tls.Config if it was requested by flags | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment is inaccurate. Implies the existence of a conditional that isn't there. Instead, consider something like: |
||
tlsConfig := createTLSConfig(*tlsCertPath, *tlsKeyPath, *tlsCAPath, *tlsClientAuth) | ||
|
||
containerLabelFunc := metrics.DefaultContainerLabels | ||
if !*storeContainerLabels { | ||
whitelistedLabels := strings.Split(*whitelistedContainerLabels, ",") | ||
|
@@ -179,7 +189,23 @@ func main() { | |
rootMux.Handle(*urlBasePrefix+"/", http.StripPrefix(*urlBasePrefix, mux)) | ||
|
||
addr := fmt.Sprintf("%s:%d", *argIP, *argPort) | ||
klog.Fatal(http.ListenAndServe(addr, rootMux)) | ||
listener, err := net.Listen("tcp", addr) | ||
if err != nil { | ||
klog.Fatal(err) | ||
} | ||
|
||
// Wrap in a TLS listener if any TLS configuration was specified | ||
if tlsConfig != nil { | ||
listener = tls.NewListener(listener, tlsConfig) | ||
klog.V(1).Infof("Listening for TLS connections") | ||
} | ||
|
||
server := &http.Server{ | ||
Addr: addr, | ||
Handler: rootMux, | ||
TLSConfig: tlsConfig, | ||
} | ||
klog.Fatal(server.Serve(listener)) | ||
} | ||
|
||
func setMaxProcs() { | ||
|
@@ -240,3 +266,64 @@ func createCollectorHTTPClient(collectorCert, collectorKey string) http.Client { | |
|
||
return http.Client{Transport: transport} | ||
} | ||
|
||
func createTLSConfig(tlsCertPath, tlsKeyPath, tlsCAPath, tlsClientAuth string) *tls.Config { | ||
var tlsConfig *tls.Config | ||
|
||
if tlsCertPath != "" || tlsKeyPath != "" { | ||
if tlsCertPath == "" || tlsKeyPath == "" { | ||
// Fail if one is set but not the other | ||
klog.Fatal("Both tls_cert_path and tls_key_path are required at the same time") | ||
} | ||
|
||
// Verify that the key/certificate load and are valid before starting up | ||
_, err := tls.LoadX509KeyPair(tlsCertPath, tlsKeyPath) | ||
if err != nil { | ||
klog.Fatalf("Failed to load TLS certificate/key pair: %v", err) | ||
} | ||
|
||
tlsConfig = &tls.Config{ | ||
GetCertificate: func(*tls.ClientHelloInfo) (*tls.Certificate, error) { | ||
cert, err := tls.LoadX509KeyPair(tlsCertPath, tlsKeyPath) | ||
if err != nil { | ||
return nil, err | ||
} | ||
return &cert, nil | ||
}, | ||
} | ||
} | ||
|
||
if tlsCAPath != "" { | ||
clientCAPool := x509.NewCertPool() | ||
clientCAFile, err := os.ReadFile(tlsCAPath) | ||
if err != nil { | ||
klog.Fatalf("Failed to load TLS CA file: %v", err) | ||
} | ||
clientCAPool.AppendCertsFromPEM(clientCAFile) | ||
|
||
if tlsConfig == nil { | ||
tlsConfig = new(tls.Config) | ||
} | ||
tlsConfig.ClientCAs = clientCAPool | ||
} | ||
|
||
// This is only meaningful if we have TLS server certs, a CA cert to verify clients, or both. | ||
if tlsConfig != nil { | ||
switch tlsClientAuth { | ||
case "request": | ||
tlsConfig.ClientAuth = tls.RequestClientCert | ||
case "optional": | ||
tlsConfig.ClientAuth = tls.VerifyClientCertIfGiven | ||
case "requireany": | ||
tlsConfig.ClientAuth = tls.RequireAnyClientCert | ||
case "require": | ||
tlsConfig.ClientAuth = tls.RequireAndVerifyClientCert | ||
case "none": | ||
tlsConfig.ClientAuth = tls.NoClientCert | ||
default: | ||
klog.Fatalf("Invalid tls_client_auth: %s", tlsClientAuth) | ||
} | ||
} | ||
|
||
return tlsConfig | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defaulting this flag to
require
implies TLS to be mandatory. While 346cad4#diff-cec39746c40e86227962aa52ed9ac22cf95c1504cef42cb16c0dd5c16a8cf6caR308 makes it clear that's not the case, I think this is a logic bug.If this flag is
require
, the program should terminate if the other required parameters aren't provided (so fail close, not fail open). As such, defaulting this flag torequire
at the same time as adding the feature is a (imo) bad idea.