Skip to content

Support listening on TLS#2963

Open
frebib wants to merge 1 commit into
google:masterfrom
frebib:feat/tls
Open

Support listening on TLS#2963
frebib wants to merge 1 commit into
google:masterfrom
frebib:feat/tls

Conversation

@frebib
Copy link
Copy Markdown
Contributor

@frebib frebib commented Oct 18, 2021

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 me@frebib.net

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.

@google-cla
Copy link
Copy Markdown

google-cla Bot commented Oct 18, 2021

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 @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla Bot added the cla: no label Oct 18, 2021
@k8s-ci-robot
Copy link
Copy Markdown
Collaborator

k8s-ci-robot commented Oct 18, 2021

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

@frebib
Copy link
Copy Markdown
Contributor Author

frebib commented Oct 18, 2021

@googlebot I signed it!

@google-cla google-cla Bot added cla: yes and removed cla: no labels Oct 18, 2021
@frebib frebib force-pushed the feat/tls branch 2 times, most recently from 36ebd6e to 60cd758 Compare November 3, 2021 15:08
@frebib
Copy link
Copy Markdown
Contributor Author

frebib commented Nov 29, 2021

Bump?

iwankgb
iwankgb requested changes Jan 9, 2022
View reviewed changes
Comment thread cmd/cadvisor.go Outdated
Comment thread cmd/cadvisor.go Outdated
Comment thread cmd/cadvisor.go Outdated
Comment thread cmd/cadvisor.go Outdated
@iwankgb
Copy link
Copy Markdown
Collaborator

iwankgb commented Jan 9, 2022

/ok-to-test

@frebib
Copy link
Copy Markdown
Contributor Author

frebib commented Jan 25, 2022

Is there some documentation or help pages I should update to go along with this change?

@frebib
Copy link
Copy Markdown
Contributor Author

frebib commented Apr 21, 2022

/retest

Test failures seem irrelevant. It was working fine before.
Bump on this

@frebib
Copy link
Copy Markdown
Contributor Author

frebib commented Apr 21, 2022

🤷🏻 It fails to build locally for me on master- it's not the fault of this PR though.

Ichabond
Ichabond reviewed Jul 15, 2022
View reviewed changes
Comment thread cmd/cadvisor.go
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.")
Copy link
Copy Markdown

@Ichabond Ichabond Jul 15, 2022

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 to require at the same time as adding the feature is a (imo) bad idea.

Comment thread cmd/cadvisor.go
klog.Fatalf("Failed to register HTTP handlers: %v", err)
}

// Generate tls.Config if it was requested by flags
Copy link
Copy Markdown

@Ichabond Ichabond Jul 15, 2022

Choose a reason for hiding this comment

The 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:
createTLSConfig returns nil if arguments are empty strings

@frebib frebib force-pushed the feat/tls branch 2 times, most recently from ea64a42 to 18b320c Compare August 20, 2023 09:08
iwankgb
iwankgb reviewed Aug 20, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@iwankgb iwankgb left a comment

Choose a reason for hiding this comment

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

See: https://github.com/google/cadvisor/pull/2963/files#r921991414, please.

@frebib
Support listening on TLS
5355b01 
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 <me@frebib.net>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Dec 10, 2025

This PR is stale because it has been open 90 days with no activity. This PR will be closed in 30 days unless new comments are made or the stale label is removed. To skip these checks, apply the "lifecycle/frozen" label.

@github-actions github-actions Bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 10, 2025
@frebib
Copy link
Copy Markdown
Contributor Author

frebib commented Dec 24, 2025

Needs rework

@github-actions github-actions Bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 25, 2025
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 25, 2026

This PR is stale because it has been open 90 days with no activity. This PR will be closed in 30 days unless new comments are made or the stale label is removed. To skip these checks, apply the "lifecycle/frozen" label.

@github-actions github-actions Bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 25, 2026
@frebib
Copy link
Copy Markdown
Contributor Author

frebib commented Mar 25, 2026

Wow? Stale bot? Really?

@github-actions github-actions Bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iwankgb iwankgb iwankgb requested changes

+1 more reviewer

@Ichabond Ichabond Ichabond left review comments

Reviewers whose approvals may not affect merge requirements

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

cla: yes ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@frebib @k8s-ci-robot @iwankgb @Ichabond