Skip to content

Fix/missing ssl params #3152

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

Merged
merged 2 commits into from
Nov 16, 2017
Merged

Conversation

technosophos
Copy link
Member

Fix several issues with SSL, including:

  • Add parameters back to helm get *
  • Correctly process params for helm list
  • Fix security issue that allows self-signed certs to auth
  • Add documentation

@technosophos technosophos self-assigned this Nov 16, 2017
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 16, 2017
Copy link
Member

@bacongobbler bacongobbler left a comment

Choose a reason for hiding this comment

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

reviewed in a pairing session, LGTM

Copy link
Member

@bacongobbler bacongobbler left a comment

Choose a reason for hiding this comment

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

docs need updating :)

helm docs are out of date. Please run "make docs"

@technosophos technosophos force-pushed the fix/missing-ssl-params branch 2 times, most recently from c0a2d71 to 43977d9 Compare November 16, 2017 00:37
@bacongobbler
Copy link
Member

mind rebasing your PR as well? looks like a merge commit strayed in here. Sorry!

During a recent refactor, several TLS flags stopped being processed for
a few of the commands. This fixes those commands, and documents how to
set up TLS.
The older version of Tiller allowed a weaker set of certificate checks
than we intended. This version requires a client certificate, and then
requires that that certificate be signed by a known CA. This works
around the situation where a user could provide a self-signed
certificate.
@technosophos technosophos force-pushed the fix/missing-ssl-params branch from 43977d9 to 1096813 Compare November 16, 2017 00:41
@technosophos
Copy link
Member Author

Ah! Yes, I rebased off the wrong commit. I think I fixed it.

@technosophos technosophos merged commit e8e6ac5 into helm:master Nov 16, 2017
@technosophos technosophos deleted the fix/missing-ssl-params branch November 16, 2017 00:52
bacongobbler pushed a commit that referenced this pull request Nov 16, 2017
* fix(helm): add TLS params back

During a recent refactor, several TLS flags stopped being processed for
a few of the commands. This fixes those commands, and documents how to
set up TLS.

* fix(tiller): add stricter certificate verification

The older version of Tiller allowed a weaker set of certificate checks
than we intended. This version requires a client certificate, and then
requires that that certificate be signed by a known CA. This works
around the situation where a user could provide a self-signed
certificate.

(cherry picked from commit e8e6ac5)
bacongobbler pushed a commit that referenced this pull request Nov 16, 2017
* fix(helm): add TLS params back

During a recent refactor, several TLS flags stopped being processed for
a few of the commands. This fixes those commands, and documents how to
set up TLS.

* fix(tiller): add stricter certificate verification

The older version of Tiller allowed a weaker set of certificate checks
than we intended. This version requires a client certificate, and then
requires that that certificate be signed by a known CA. This works
around the situation where a user could provide a self-signed
certificate.

(cherry picked from commit e8e6ac5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants