Setup TLS with CA Cert for vsphere cloud provider #64758

mariantalla · 2018-06-05T10:33:07Z

Extend config to take a path to a CA Certificate
Use the CA Cert when establishing a connection with the SOAP client

Testing
We provide certs and keys for tests as fixtures, vclib/fixtures.
Those were created (and can be regenerated) using vclib/fixtures/createCerts.sh.

At the moment it's possible to configure a CA path and at the same time allow insecure
communication between vsphere cloud provider and vcenter. This may
change in the future; we might opt for overwriting the insecure
communication if a CA is configured / log and transparently pass the
arguments to the vcenter command / other. To be discussed.

At the moment the CA is a global level configuration. In other
words, all vcenter servers need to use certificates signed by the same
CA. There might be use cases for different CA per vcenter server; to be
discussed.

What this PR does / why we need it:
This PR adds the option of configuring a trusted CA for the communication between the vsphere cloud provider and the vcenter control plane.

Which issue(s) this PR fixes:
Fixes #64222

Special notes for your reviewer:

Release note:

- Can configure the vsphere cloud provider with a trusted Root-CA

hoegaarden · 2018-06-05T11:09:25Z

/ok-to-test

mariantalla · 2018-06-05T11:17:53Z

@wgliang Thanks for the feedback. For some reason your comments disappeared when we pushed new commits.

Some points you brought up that should be hopefully sorted now:

LICENCE for test files added
comments that document functions configFromSim and configFromSimWithTLS are now accurate

Remaining points that you brought up:

More test cases: what would you like to see covered which is not already?
TableDrivenTests: we think that it will introduce more overhead than help at the moment, but will certainly consider it once we have more test cases (we still have work to do on this PR).
vclib_test vs vclib package for tests: We generally tend to lean towards blackbox testing, so we force ourselves to not touch internals of packages. Thus the package vclib_test. What were your thoughts on putting the tests in the same package as the source?

wgliang · 2018-06-05T11:27:02Z

@mariantalla
I'm very sorry, because I didn't see WIP at first, you can change it to the more obvious [WIP].

mariantalla · 2018-06-05T11:30:23Z

@wgliang Thanks, changed the title! Please keep commenting even though it's still in progress. Would love to hear your thoughts on test cases and packages in particular.

wgliang · 2018-06-05T11:33:28Z

@mariantalla
Of course, thank you very much and willingly.

hoegaarden · 2018-06-06T13:25:10Z

/test pull-kubernetes-node-e2e

- Extend config to take a path to a CA Certificate - Use the CA Cert when establishing a connection with the SOAP client Testing We provide certs and keys for tests as fixtures, `vclib/fixtures`. Those were created (and can be regenerated) using `vclib/fixtures/createCerts.sh`. At the moment it's possible to configure a CA path and at the same time allow insecure communication between vsphere cloud provider and vcenter. This may change in the future; we might opt for overwriting the insecure communication if a CA is configured / log and transparently pass the arguments to the vcenter command / other. To be discussed. At the moment the CA is a global level configuration. In other words, all vcenter servers need to use certificates signed by the same CA. There might be use cases for different CA per vcenter server; to be discussed.

./hack/update-bazel.sh

Also remove comments that are not useful anymore.

This will help with bazel tests, which seem to use a different working directory from local test runs.

... and rename `InvalidCaCertPath` to `InvalidCertPath`.

…e configured

hoegaarden · 2018-06-24T19:02:22Z

PTAL

dougm · 2018-06-24T20:40:17Z

Thanks @mariantalla @hoegaarden lgtm

/approve

dougm · 2018-06-21T21:50:55Z

pkg/cloudprovider/providers/vsphere/vclib/connection.go

@@ -175,3 +210,25 @@ func (connection *VSphereConnection) UpdateCredentials(username string, password
 	connection.Username = username
 	connection.Password = password
 }
+
+func normalizeThumbprint(original string) (string, error) {


What is the use case that would require this normalization?

dougm · 2018-06-21T21:53:15Z

pkg/cloudprovider/providers/vsphere/vclib/connection_test.go

+		t.Fatalf("Cannot add CA to CAPool")
+	}
+
+	server := httptest.NewUnstartedServer(http.HandlerFunc(handler))


Note that you could also use govmomi/simulator for these types of tests: https://github.com/vmware/govmomi/blob/b3251638696a6f4ac905fe904091e9840982c602/simulator/simulator_test.go#L353-L370

dougm · 2018-06-21T21:56:35Z

pkg/cloudprovider/providers/vsphere/vclib/connection_test.go

+	if len(server.TLS.Certificates) < 1 || len(server.TLS.Certificates[0].Certificate) < 1 {
+		t.Fatal("Expected server.TLS.Certificates not to be empty")
+	}
+	x509LeafCert := server.TLS.Certificates[0].Certificate[0]


Even without simulator, you can use HostCertificateInfo.FromCertificate https://github.com/vmware/govmomi/blob/b3251638696a6f4ac905fe904091e9840982c602/simulator/simulator_test.go#L353-L358

Maybe this also avoids the need for the normalization function?

hoegaarden · 2018-06-28T18:38:08Z

@dougm @divyenpatel @wgliang Is there anything left to do for us to get the lgtm -- i think that's missing to get this merged, isn't it?

dougm · 2018-06-28T20:20:56Z

@divyenpatel approved via review, but we need his '/lgtm' to add the label.

divyenpatel · 2018-06-28T20:30:03Z

/assign @divyenpatel

divyenpatel · 2018-06-28T20:30:10Z

/lgtm

k8s-ci-robot · 2018-06-28T20:30:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: divyenpatel, dougm, mariantalla

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/cloudprovider/providers/vsphere/OWNERS~~ [divyenpatel]

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

dougm · 2018-06-28T20:50:12Z

/test pull-kubernetes-kubemark-e2e-gce-big

hoegaarden · 2018-06-28T22:50:12Z

/test pull-kubernetes-e2e-gce

fejta-bot · 2018-06-29T02:17:43Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot · 2018-06-29T05:26:44Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot · 2018-06-29T08:35:42Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

mariantalla · 2018-06-29T10:49:13Z

/test pull-kubernetes-e2e-gce

hoegaarden · 2018-06-29T14:47:49Z

/test pull-kubernetes-e2e-gce

hoegaarden · 2018-06-29T16:07:07Z

/test pull-kubernetes-e2e-gce

hoegaarden · 2018-06-29T19:19:20Z

/test pull-kubernetes-e2e-gce

fejta-bot · 2018-06-29T22:56:46Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot · 2018-06-30T02:26:44Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot · 2018-06-30T08:02:44Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

k8s-github-robot · 2018-06-30T09:13:02Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-06-30T10:29:24Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

marpaia · 2018-07-02T19:01:35Z

/sig vmware

k8s-ci-robot requested review from j3ffml and wojtek-t June 5, 2018 10:33

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 5, 2018

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 5, 2018

mariantalla changed the title ~~wip - Setup TLS with CA Cert for vsphere cloud provider~~ [WIP] Setup TLS with CA Cert for vsphere cloud provider Jun 5, 2018

Maria Ntalla added 13 commits June 8, 2018 12:00

Make bazel happy

b3a1750

./hack/update-bazel.sh

Add LICENSE header

64f13c2

Improve godocs and testcase naming

b85d760

Add LICENCE header to createCerts.sh

f7e9b6a

Also remove comments that are not useful anymore.

Fix spelling

90012e5

Resolve paths of test fixtures at runtime

0cbe254

This will help with bazel tests, which seem to use a different working directory from local test runs.

Add godocs for fixtures

795e4c4

... and rename `InvalidCaCertPath` to `InvalidCertPath`.

Introduce thumbprints per vcenter

b830283

Pass through CA cert file to the connection when multiple vcenters ar…

7ade826

…e configured

Setup test for verifying by checking certificate fingerprints

64bc96b

Use soap clients method to load root CAs

9deaba0

Check certificate thumbprint when configured

ed4d445

dougm approved these changes Jun 24, 2018

View reviewed changes

divyenpatel approved these changes Jun 28, 2018

View reviewed changes

k8s-ci-robot assigned divyenpatel Jun 28, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 28, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 28, 2018

k8s-github-robot merged commit 64243d4 into kubernetes:master Jun 30, 2018

k8s-ci-robot added the area/provider/vmware Issues or PRs related to vmware provider label Jul 2, 2018

mariantalla mentioned this pull request Sep 18, 2018

REQUEST: New membership for mariantalla kubernetes/org#93

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setup TLS with CA Cert for vsphere cloud provider #64758

Setup TLS with CA Cert for vsphere cloud provider #64758

mariantalla commented Jun 5, 2018

hoegaarden commented Jun 5, 2018

mariantalla commented Jun 5, 2018

wgliang commented Jun 5, 2018

mariantalla commented Jun 5, 2018

wgliang commented Jun 5, 2018

hoegaarden commented Jun 6, 2018

hoegaarden commented Jun 24, 2018

dougm commented Jun 24, 2018

dougm Jun 21, 2018

dougm Jun 21, 2018

dougm Jun 21, 2018

hoegaarden commented Jun 28, 2018

dougm commented Jun 28, 2018

divyenpatel commented Jun 28, 2018

divyenpatel commented Jun 28, 2018

k8s-ci-robot commented Jun 28, 2018

dougm commented Jun 28, 2018

hoegaarden commented Jun 28, 2018

fejta-bot commented Jun 29, 2018

fejta-bot commented Jun 29, 2018

fejta-bot commented Jun 29, 2018

mariantalla commented Jun 29, 2018

hoegaarden commented Jun 29, 2018

hoegaarden commented Jun 29, 2018

hoegaarden commented Jun 29, 2018

fejta-bot commented Jun 29, 2018

fejta-bot commented Jun 30, 2018

fejta-bot commented Jun 30, 2018

k8s-github-robot commented Jun 30, 2018

k8s-github-robot commented Jun 30, 2018

marpaia commented Jul 2, 2018

Setup TLS with CA Cert for vsphere cloud provider #64758

Setup TLS with CA Cert for vsphere cloud provider #64758

Conversation

mariantalla commented Jun 5, 2018

hoegaarden commented Jun 5, 2018

mariantalla commented Jun 5, 2018

Some points you brought up that should be hopefully sorted now:

Remaining points that you brought up:

wgliang commented Jun 5, 2018

mariantalla commented Jun 5, 2018

wgliang commented Jun 5, 2018

hoegaarden commented Jun 6, 2018

hoegaarden commented Jun 24, 2018

dougm commented Jun 24, 2018

dougm Jun 21, 2018

Choose a reason for hiding this comment

dougm Jun 21, 2018

Choose a reason for hiding this comment

dougm Jun 21, 2018

Choose a reason for hiding this comment

hoegaarden commented Jun 28, 2018

dougm commented Jun 28, 2018

divyenpatel commented Jun 28, 2018

divyenpatel commented Jun 28, 2018

k8s-ci-robot commented Jun 28, 2018

dougm commented Jun 28, 2018

hoegaarden commented Jun 28, 2018

fejta-bot commented Jun 29, 2018

fejta-bot commented Jun 29, 2018

fejta-bot commented Jun 29, 2018

mariantalla commented Jun 29, 2018

hoegaarden commented Jun 29, 2018

hoegaarden commented Jun 29, 2018

hoegaarden commented Jun 29, 2018

fejta-bot commented Jun 29, 2018

fejta-bot commented Jun 30, 2018

fejta-bot commented Jun 30, 2018

k8s-github-robot commented Jun 30, 2018

k8s-github-robot commented Jun 30, 2018

marpaia commented Jul 2, 2018