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 adding untrusted root CA certificates (corp certs) #5015

Merged
merged 7 commits into from
Aug 15, 2019

Conversation

laozc
Copy link
Contributor

@laozc laozc commented Aug 8, 2019

This PR allows users to populate Corp Root CA certificates by adding certificates in PEM format to ~/.mininkube/certs.
These certificates will be added to system certificate store, which will be trusted by all processes running in the minikube VM.

Closes: #1408

Signed-off-by: Zhongcheng Lao <Zhongcheng.Lao@microsoft.com>
@k8s-ci-robot k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 8, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @laozc. Thanks for your PR.

I'm waiting for a kubernetes 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.

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.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: laozc
To complete the pull request process, please assign ra489
You can assign the PR to them by writing /assign @ra489 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 8, 2019
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 8, 2019
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

This PR allows users to add root CA certificates in minikube
VM. CA certificates in $HOME/.minikube/certs will be populated
to system certificate store.

Note: This requires a change to minikube ISO so you may need to
delete and start a brand new minikube VM.

Signed-off-by: Zhongcheng Lao <Zhongcheng.Lao@microsoft.com>
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Aug 8, 2019
@RA489
Copy link

RA489 commented Aug 8, 2019

@minikube-bot OK to test

@laozc laozc force-pushed the populate-ca branch 2 times, most recently from 8b58763 to 002956f Compare August 8, 2019 12:31
Signed-off-by: Zhongcheng Lao <Zhongcheng.Lao@microsoft.com>
@medyagh
Copy link
Member

medyagh commented Aug 8, 2019

@laozc Thank you so much for this PR ! this is actually very exciting !

do you mind sharing if you have tested this for yourself?

and I am very curious to find out how I could test it myself?

is there a way I could simulate a corp environment root CA and first see it fail and then after this PR see it succeed ?

@laozc
Copy link
Contributor Author

laozc commented Aug 9, 2019

I've verified the PR on both Linux and Windows (Hyper-V) myself.

I used the PR against my own CA inside a private network (mynetwork.net), which runs a private Docker image server (docker.mynetwork.net) with TLS enabled.

  1. curl MINIKUBE_IP:8443 (both in the VM and within the containers)

  2. curl https://mynetwork.net (website which has an in-house corporate CA as the Root CA)

  3. docker pull docker.mynetwork.net/ubuntu (Note: the image server needs to install a SSL cert which has an in-house corporate CA as the Root CA)

There operations should not produce any errors like
curl: (60) SSL certificate problem: self signed certificate in certificate chain
or
Error response from daemon: Get https://docker.mynetwork.net/v2/: x509: certificate signed by unknown authority

You may use a self-signed root CA, generate a server cert with the root and OpenSSL to verify this PR.

Copy link
Contributor

@tstromberg tstromberg left a comment

Choose a reason for hiding this comment

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

This PR is wonderful. I just have small nitpicks.

pkg/minikube/bootstrapper/certs.go Outdated Show resolved Hide resolved
pkg/minikube/bootstrapper/certs.go Outdated Show resolved Hide resolved
pkg/minikube/bootstrapper/certs.go Outdated Show resolved Hide resolved
pkg/minikube/bootstrapper/certs.go Outdated Show resolved Hide resolved
pkg/minikube/bootstrapper/certs.go Outdated Show resolved Hide resolved
pkg/minikube/constants/constants.go Outdated Show resolved Hide resolved
Signed-off-by: Zhongcheng Lao <Zhongcheng.Lao@microsoft.com>
@medyagh
Copy link
Member

medyagh commented Aug 9, 2019

I've verified the PR on both Linux and Windows (Hyper-V) myself.

I used the PR against my own CA inside a private network (mynetwork.net), which runs a private Docker image server (docker.mynetwork.net) with TLS enabled.

  1. curl MINIKUBE_IP:8443 (both in the VM and within the containers)
  2. curl https://mynetwork.net (website which has an in-house corporate CA as the Root CA)
  3. docker pull docker.mynetwork.net/ubuntu (Note: the image server needs to install a SSL cert which has an in-house corporate CA as the Root CA)

There operations should not produce any errors like
curl: (60) SSL certificate problem: self signed certificate in certificate chain
or
Error response from daemon: Get https://docker.mynetwork.net/v2/: x509: certificate signed by unknown authority

You may use a self-signed root CA, generate a server cert with the root and OpenSSL to verify this PR.

I would love to see an integration tests for this, do you have scipts that I could run to exactly simulate those steps.

Signed-off-by: Zhongcheng Lao <Zhongcheng.Lao@microsoft.com>
@RA489
Copy link

RA489 commented Aug 11, 2019

@minikube-bot OK to test.

@laozc
Copy link
Contributor Author

laozc commented Aug 12, 2019

Unfortunately except for generating SSL certificates, I don't have the script to launch a image repository with SSL certificate.

@medyagh
Copy link
Member

medyagh commented Aug 14, 2019

t for generating SSL certificates, I don't have the script to launch a image repository with SSL certificate.

This is great work ! I know many people whose lifes would be happier when this PR gets merged ! very thoughtful !

could you add a tutorial on the cert maybe in the site?
like if I have a corp laptop with certs, which files do I have to copy and to where, exactly ?

btw this needs a rebase. I am excited to see this PR !

@medyagh
Copy link
Member

medyagh commented Aug 14, 2019

laozc added 2 commits August 15, 2019 20:38
Signed-off-by: Zhongcheng Lao <Zhongcheng.Lao@microsoft.com>
Signed-off-by: Zhongcheng Lao <Zhongcheng.Lao@microsoft.com>
@medyagh
Copy link
Member

medyagh commented Aug 15, 2019

@laozc This is beautiful work ! it looks good to be merged after the tests pass ! I feel impatient to merge it ! thank you for this !

@medyagh medyagh changed the title Populate untrusted Root CA certificates Support adding untrusted root CA certificates (corp certs) Aug 15, 2019
@medyagh
Copy link
Member

medyagh commented Aug 15, 2019

@laozc do you happen to know the exact error messages that minikube crashes on if they don't add the corp certs ? I think it would be nice to add a Solution message to the user and advice them to read this great tutorial you wrote. something to be added here:

like this one: https://github.com/kubernetes/minikube/blob/master/pkg/minikube/problem/err_map.go#L97

BTW please feel free to add your name to the tutorial on the site as the author ! you deserve all the credit .

Copy link
Contributor

@tstromberg tstromberg left a comment

Choose a reason for hiding this comment

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

Thank you!

@tstromberg tstromberg merged commit b0e9a23 into kubernetes:master Aug 15, 2019
@laozc
Copy link
Contributor Author

laozc commented Aug 16, 2019

@laozc do you happen to know the exact error messages that minikube crashes on if they don't add the corp certs ? I think it would be nice to add a Solution message to the user and advice them to read this great tutorial you wrote. something to be added here:

like this one: https://github.com/kubernetes/minikube/blob/master/pkg/minikube/problem/err_map.go#L97

Let me reproduce it and make another change for a more user-friendly error prompt later.

BTW please feel free to add your name to the tutorial on the site as the author ! you deserve all the credit .

Thanks. Happy to help.
Let me know if there is any concern.

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. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. 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.

Document how to add corporate root SSL certificates
6 participants