Support listening on TLS #2963
Support listening on TLS #2963
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
Hi @frebib. Thanks for your PR. I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@googlebot I signed it! |
frebib
force-pushed
the
feat/tls
branch
2 times, most recently
from
36ebd6e to
60cd758
Compare
|
Bump? |
|
/ok-to-test |
|
Is there some documentation or help pages I should update to go along with this change? |
|
/retest Test failures seem irrelevant. It was working fine before. |
|
🤷🏻 It fails to build locally for me on |
| 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.") |
There was a problem hiding this comment.
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 to require at the same time as adding the feature is a (imo) bad idea.
| @@ -153,6 +160,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.
Comment is inaccurate. Implies the existence of a conditional that isn't there. Instead, consider something like:
createTLSConfig returns nil if arguments are empty strings
frebib
force-pushed
the
feat/tls
branch
2 times, most recently
from
ea64a42 to
18b320c
Compare
Allows specifying a certificate&key pair by file paths, as well as an optional CA file. Enforce/allow mutual-TLS handshake behaviour too. Signed-off-by: Joe Groocock <[email protected]>
Allows specifying a certificate&key pair by file paths, as well as an
optional CA file. Enforce/allow mutual-TLS handshake behaviour too.
Signed-off-by: Joe Groocock [email protected]
I appreciate that this implementation is rather rudimentary, and I don't expect it to be accepted immediately but I figured it was a good start and a discussion point.