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.

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 skipping the validity check of keystone server's certificate #35280

Closed
wants to merge 1 commit into from

Conversation

dixudx
Copy link
Member

@dixudx dixudx commented Oct 21, 2016

What this PR does / why we need it:

Sometimes the keystone server's certificate is self-signed, mainly used for internal development, testing and etc.

Of course, a trusted root certificate for keystone server can be added manually.

Otherwise, below error will occur.

x509: certificate signed by unknown authority

However, this patch provides an alternative and easy way to support such scenario.

Which issue this PR fixes : fixes #22695, #24984

Special notes for your reviewer:

Release note:


This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

Can a kubernetes member verify that this patch is reasonable to test? If so, please reply with "@k8s-bot ok to test" on its own line.

Regular contributors should join the org to skip this step.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. 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, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@dixudx
Copy link
Member Author

dixudx commented Oct 21, 2016

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Oct 21, 2016
@krousey
Copy link
Contributor

krousey commented Oct 21, 2016

cc @kubernetes/sig-auth

@krousey krousey assigned liggitt and cjcullen and unassigned krousey Oct 21, 2016
@deads2k
Copy link
Contributor

deads2k commented Oct 21, 2016

I'm opposed to this change. Authenticators should always verify who they are speaking to. Plumbing through an option to allow a custom CA for verifying the keystone server's serving cert would be fine with me, but I think that bypassing it entirely is unreasonable.

Any flag like this that is added as a "never use this in production" will almost certainly be used in production and the barrier to adding your cert as a CA is relatively minor.

@liggitt
Copy link
Member

liggitt commented Oct 21, 2016

I'm opposed to this change. Authenticators should always verify who they are speaking to. Plumbing through an option to allow a custom CA for verifying the keystone server's serving cert would be fine with me, but I think that bypassing it entirely is unreasonable.

I agree.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 22, 2016
@dixudx dixudx force-pushed the keystone-insecure-tls branch from b9ed704 to 267ae1a Compare October 24, 2016 02:31
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 24, 2016
@dixudx dixudx force-pushed the keystone-insecure-tls branch from 267ae1a to 2323a51 Compare October 24, 2016 06:11
@liggitt
Copy link
Member

liggitt commented Oct 24, 2016

if you'd like to rework this to pass in a custom CA bundle to use to verify the keystone server, I think that would be fine, but I don't see adding this option.

@dixudx
Copy link
Member Author

dixudx commented Oct 25, 2016

@liggitt I've already created another PR #35488 to enable passing a ca file to verify the keystone server.

@liggitt
Copy link
Member

liggitt commented Oct 25, 2016

closing in favor of #35488

@liggitt liggitt closed this Oct 25, 2016
@dixudx dixudx deleted the keystone-insecure-tls branch September 9, 2017 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optional support for unsecure keystone API URL
8 participants