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

Setup TLS with CA Cert for vsphere cloud provider #64758

Merged
merged 19 commits into from Jun 30, 2018

Conversation

@mariantalla
Copy link
Contributor

mariantalla commented Jun 5, 2018

  • 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

This comment has been minimized.

Copy link
Member

hoegaarden commented Jun 5, 2018

/ok-to-test

@mariantalla

This comment has been minimized.

Copy link
Contributor Author

mariantalla commented Jun 5, 2018

@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

This comment has been minimized.

Copy link
Member

wgliang commented Jun 5, 2018

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

@mariantalla 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

@mariantalla

This comment has been minimized.

Copy link
Contributor Author

mariantalla commented Jun 5, 2018

@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

This comment has been minimized.

Copy link
Member

wgliang commented Jun 5, 2018

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

@hoegaarden hoegaarden force-pushed the pivotal-k8s:64222-vcp-ca-cert branch from 6a4bb3e to 630f911 Jun 6, 2018

@hoegaarden

This comment has been minimized.

Copy link
Member

hoegaarden commented Jun 6, 2018

/test pull-kubernetes-node-e2e

mariantalla added some commits Jun 5, 2018

Setup TLS with CA Cert
- 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.
Make bazel happy
./hack/update-bazel.sh
Add LICENCE header to createCerts.sh
Also remove comments that are not useful anymore.
Resolve paths of test fixtures at runtime
This will help with bazel tests, which seem to use a different working
directory from local test runs.
Add godocs for fixtures
... and rename `InvalidCaCertPath` to `InvalidCertPath`.
@dougm

This comment has been minimized.

Copy link
Member

dougm commented Jun 24, 2018

Thanks @mariantalla @hoegaarden lgtm

/approve

@dougm

dougm approved these changes Jun 24, 2018

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

func normalizeThumbprint(original string) (string, error) {

This comment has been minimized.

@dougm

dougm Jun 24, 2018

Member

What is the use case that would require this normalization?

t.Fatalf("Cannot add CA to CAPool")
}

server := httptest.NewUnstartedServer(http.HandlerFunc(handler))

This comment has been minimized.

@dougm
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]

This comment has been minimized.

@dougm

dougm Jun 24, 2018

Member

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

This comment has been minimized.

Copy link
Member

hoegaarden commented Jun 28, 2018

@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

This comment has been minimized.

Copy link
Member

dougm commented Jun 28, 2018

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

@divyenpatel

This comment has been minimized.

Copy link
Member

divyenpatel commented Jun 28, 2018

/assign @divyenpatel

@divyenpatel

This comment has been minimized.

Copy link
Member

divyenpatel commented Jun 28, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jun 28, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jun 28, 2018

[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:

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

@dougm

This comment has been minimized.

Copy link
Member

dougm commented Jun 28, 2018

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

@hoegaarden

This comment has been minimized.

Copy link
Member

hoegaarden commented Jun 28, 2018

/test pull-kubernetes-e2e-gce

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jun 29, 2018

/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.

2 similar comments
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jun 29, 2018

/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

This comment has been minimized.

Copy link

fejta-bot commented Jun 29, 2018

/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

This comment has been minimized.

Copy link
Contributor Author

mariantalla commented Jun 29, 2018

/test pull-kubernetes-e2e-gce

3 similar comments
@hoegaarden

This comment has been minimized.

Copy link
Member

hoegaarden commented Jun 29, 2018

/test pull-kubernetes-e2e-gce

@hoegaarden

This comment has been minimized.

Copy link
Member

hoegaarden commented Jun 29, 2018

/test pull-kubernetes-e2e-gce

@hoegaarden

This comment has been minimized.

Copy link
Member

hoegaarden commented Jun 29, 2018

/test pull-kubernetes-e2e-gce

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jun 29, 2018

/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.

2 similar comments
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jun 30, 2018

/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

This comment has been minimized.

Copy link

fejta-bot commented Jun 30, 2018

/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

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 30, 2018

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

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 30, 2018

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

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

16 of 17 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-kubemark-e2e-gce-big
Details
cla/linuxfoundation hoegaarden authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@marpaia

This comment has been minimized.

Copy link
Member

marpaia commented Jul 2, 2018

/sig vmware

@mariantalla mariantalla referenced this pull request Sep 18, 2018

Closed

REQUEST: New membership for mariantalla #93

6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.