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

CAPV should support SHA-256 thumbprints when connecting to the vSphere API #2805

Closed
rgcostea opened this issue Mar 11, 2024 · 8 comments
Closed
Assignees
Labels
area/govmomi Issues or PRs related to the govmomi mode kind/feature Categorizes issue or PR as related to a new feature.
Milestone

Comments

@rgcostea
Copy link

/kind feature

Describe the solution you'd like
Currently, CAPV only supports SHA-1 thumbprints when creating VSphereClusters. We would like to see support for SHA-256 thumbprints as well.

Anything else you would like to add:

Environment:

  • Cluster-api-provider-vsphere version: v1.9.2
  • Kubernetes version: (use kubectl version): 1.26.8
  • OS (e.g. from /etc/os-release): ubuntu
@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 11, 2024
@neolit123
Copy link
Member

neolit123 commented Mar 11, 2024

cc @yastij @dougm @chrischdi

@rgcostea pinged the VMware slack about this. my findings are:

  • the CAPV session client to a VC seems to have two modes, insecure (without a SHA1 thumbprint) and secure using a SHA1 thumbprint
  • the code is here
    func newClient(ctx context.Context, sessionKey string, url *url.URL, thumbprint string, feature Feature) (*govmomi.Client, error) {
    log := ctrl.LoggerFrom(ctx)
    insecure := thumbprint == ""
    soapClient := soap.NewClient(url, insecure)
    if !insecure {
    soapClient.SetThumbprint(url.Host, thumbprint)
    }
    vimClient, err := vim25.NewClient(ctx, soapClient)
    if err != nil {
    return nil, errors.Wrapf(err, "failed to create client")
    }
    vimClient.UserAgent = "k8s-capv-useragent"
    c := &govmomi.Client{
    Client: vimClient,
    SessionManager: session.NewManager(vimClient),
    }
  • cannot find a way to feed standard cert, key, CA cert - i.e. for mTLS.
  • without a thumbprint what happens is that the new client is created here with InsecureSkipVerify as Doug said https://github.com/vmware/govmomi/blob/main/vim25/soap/client.go#L136-L163
  • CAPV seems transparent to what type the SHA is, but govmomi expects SHA1.
    https://github.com/vmware/govmomi/blob/main/vim25/soap/client.go#L421

@chrischdi
Copy link
Member

xref: govmomi support for sha256 thumbprints:

Should be part of the next govmomi release.


Thanks @neolit123 for sumarizing.

Regarding support of sha256 thumbprints:

  • We now have to wait for the next govmomi release and bump the dependency (which then is v0.37.0?!)

Regarding support of using a ca certificate instead of the thumbprint:

  • I think here is room to improve CAPV
  • In CAPV we currently pass through the thumprint in the session package to the newClient function

client, err := newClient(ctx, sessionKey, soapURL, params.thumbprint, params.feature)

  • If the thumprint is set to empty string, capv passes through insecure as true to govmomi's soap.NewClient function.

insecure := thumbprint == ""
soapClient := soap.NewClient(url, insecure)

  • So there is no way of using CA based verification, if we don't want to set a thumbprint currently.

    • (Note: there is the workaround of setting a garbage thumbprint, but I don't know if that would lead to other issues)
  • It would propably be good to be able to mount a CA certificate as file and properly pass it through to tls configuration if set, without running as insecure and without having to add it to the containers root CAs.

@chrischdi
Copy link
Member

Govmomi v0.36.1 got released. So bumping to that version should bring in support for sha256 thumbprints.

@sbueringer sbueringer added the area/govmomi Issues or PRs related to the govmomi mode label Apr 18, 2024
@neolit123
Copy link
Member

i assume next step is to just phase out SHA1 in all places in CAPV, update API comments and docs too?
i can just send a PR for the main branch, but perhaps there is a need for a better plan + discussion.

@chrischdi
Copy link
Member

chrischdi commented Jun 28, 2024

Documented sha256 and using it in code now since merging:

However, not yet using it completely in CI, because it would currently break old CAPV branches and also CPI.

Action plan:

@sbueringer
Copy link
Member

sbueringer commented Jul 10, 2024

@chrischdi Assigned the issue to you, please update accordingly once done :)

Also added to the v1.11 milestone as we're basically almost done IIRC.

@sbueringer sbueringer added this to the v1.11 milestone Jul 10, 2024
@chrischdi
Copy link
Member

/close

This is done and CI still green

@k8s-ci-robot
Copy link
Contributor

@chrischdi: Closing this issue.

In response to this:

/close

This is done and CI still green

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-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/govmomi Issues or PRs related to the govmomi mode kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

No branches or pull requests

5 participants