add insecureSkipVerify to http source type by xphyr · Pull Request #4167 · kubevirt/containerized-data-importer

xphyr · 2026-06-09T16:02:19Z

What this PR does / why we need it:
This PR adds an insecureSkipVerify field to http DavaVolume imports. This helps to support connections to https datasources that have corrupt, or incomplete TLS certificates but the user still wants to connect anyway. For example, connections to sources that have a self signed certificate, but no SAN defined for the IP address, will not import, even if you have defined a certConfigMap. This option allows you to bypass this check on a specific DataVolume. This also simplifies the process for downloading from a source using a self-signed certificate, IF you are OK with completely bypassing the check.

This leverages the existing code/process for skipping TLS verification that is used by the imageio importer.

I have updated the documentation, as well as the go unit tests, and functional test.

Which issue(s) this PR fixes *:
Fixes #4166

Special notes for your reviewer:
This work was discussed on Slack in the following thread: https://kubernetes.slack.com/archives/C8ED7RKFE/p1780500582897189

Release note:

Added insecureSkipVerify to http source type to support connecting to https endpoints with invalid certificates. 
By default this option is set to false, and must be explicitly set to "true" to use. 
This will not break any existing datavolume definitions.

This option allows a user to explicitly override TLS for self-signed certificates on specific data sources This option uses existing framework for overriding TLS settings in storageio importer for consistency All existing go tests have been updated to use the default setting "false" to maintain the existing tests Signed-off-by: xphyr <xphyr@users.noreply.github.com>

Signed-off-by: xphyr <xphyr@users.noreply.github.com>

kubevirt-bot · 2026-06-09T16:02:47Z

Hi @xphyr. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Details

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.

kubevirt-bot · 2026-06-09T16:03:01Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign aglitke for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

Signed-off-by: xphyr <xphyr@users.noreply.github.com>

akalenyu

Thank you!

Overall this makes sense, but I am slightly worried on the security side;
There is no admin level config to say "no one in this cluster may skip TLS on imports".
This is also a problem for existing ImageIO but arguably those are more controlled environments to begin with. Maybe we could require checksum when using insecure imports?
/cc @Acedus
Network policies may be reducing the blast radius here

EDIT:
I guess we could also put a insecureHTTPEndpoints next to existing CDI CR insecureRegistries and inherit it from the platform (OpenShift configuration CR, but from a quick search there isn't one).

akalenyu · 2026-06-10T11:14:16Z

+	if http.InsecureSkipVerify != nil && *http.InsecureSkipVerify {
+		annotations[AnnInsecureSkipVerify] = "true"
+	}


could we get some unit tests that validate AnnInsecureSkipVerify is correctly set when InsecureSkipVerify is true, or correctly absent when nil/false?

OK, I think I now have this covered here: 8b34ebd Please be sure to take a close look, writing unit tests is NOT a strength of mine, so I relied on the help of AI to build this.

This is also a solid unit test, but I think we could take a similar approach to my suggestion #4167 (comment) in pkg/controller/populators/import-populator_test.go

For this unit test, I feel like we are testing at the proper level... reviewing the rest of the tests for this package none of them appear to be going through the process of checking the environment variables assigned to a pod. While I agree testing for this in the import-controller makes sense, since this is a helper package/function should we not just be testing the results of calling the helper function? Perhaps I am misunderstanding how you would like me to test this. Please advise and thanks for the assistance.

akalenyu · 2026-06-10T11:26:20Z

 	source, sourceOk := pvc.Annotations[cc.AnnSource]
-	if sourceOk && source == cc.SourceImageio {
+	if sourceOk && (source == cc.SourceImageio || source == cc.SourceHTTP) {
 		if insecureSkipVerify, ok := pvc.Annotations[cc.AnnInsecureSkipVerify]; ok && insecureSkipVerify == "true" {


Some unit tests for this would also be great, specifically that no AnnInsecureSkipVerify results in no env var for TLS bypass. Just trying to cover everything that guards the runtime importer app receiving insecure=true

I have also written a test here: 2aea8ad with the help of AI. I think that does what you are suggesting.

This is good! I was hoping to assert on the resulting pod env vars though to cover more ground. We should have existing tests that do something along the lines of

_, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "testPvc1", Namespace: "default"}}) Expect(err).ToNot(HaveOccurred()) pod := &corev1.Pod{} err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "importer-testPvc1", Namespace: "default"}, pod) <ASSERTION>

I am attempting to re-write the test as you are looking for. Before I move forward with the other unit test, can you review this to see if it is what you were looking for. fixed the issue with my first unit test If this is the proper direction, I will review the unit test for util.go. Please note I have not yet removed the other test. I wanted to ensure that this was the way you wanted to go, before deleting that code. If this is good, I will delete the first test that I wrote.

Yeah this was the direction. I was looking for something along those lines:
akalenyu@d6c22d0

OK, I have integrated that into the most recent change. I think we are all set. Thank you for your continued assistance.

akalenyu · 2026-06-10T12:19:55Z

/test all

coveralls · 2026-06-10T12:30:17Z

coverage: 49.85% (+0.07%) from 49.783% — xphyr:skiptlsverify into kubevirt:main

Acedus · 2026-06-10T13:12:44Z

Thank you!

Overall this makes sense, but I am slightly worried on the security side; There is no admin level config to say "no one in this cluster may skip TLS on imports". This is also a problem for existing ImageIO but arguably those are more controlled environments to begin with. Maybe we could require checksum when using insecure imports? /cc @Acedus Network policies may be reducing the blast radius here

EDIT: I guess we could also put a insecureHTTPEndpoints next to existing CDI CR insecureRegistries and inherit it from the platform (OpenShift configuration CR, but from a quick search there isn't one).

I'd say it would be the admin's responsibility to prevent this using something like a VAP.

…Verify Signed-off-by: xphyr <xphyr@users.noreply.github.com>

Signed-off-by: Mark DeNeve <3779715+xphyr@users.noreply.github.com>

xphyr · 2026-06-10T17:07:26Z

I'd say it would be the admin's responsibility to prevent this using something like a VAP.

This was my thought as well. I feel like an admission policy would be the way to ensure that end user(s) cant use an insecureHTTP endpoint. I feel like this is a less complicated and more universal way to handle this sort of control. Something like this would handle it base on some quick testing:

apiVersion: admissionregistration.k8s.io/v1
kind: ValidatingAdmissionPolicy
metadata:
  name: deny-datavolume-insecure-http
spec:
  failurePolicy: Fail
  matchConstraints:
    resourceRules:
    - apiGroups: ["cdi.kubevirt.io"]
      apiVersions: ["v1beta1"]
      operations: ["CREATE", "UPDATE"]
      resources: ["datavolumes"]
  validations:
    - expression: >
        !has(object.spec.source) || 
        !has(object.spec.source.http) || 
        !has(object.spec.source.http.insecureSkipVerify) || 
        object.spec.source.http.insecureSkipVerify == false
      message: "Setting insecureSkipVerify to true on an HTTP source is forbidden for security compliance."
---
apiVersion: admissionregistration.k8s.io/v1
kind: ValidatingAdmissionPolicyBinding
metadata:
  name: deny-datavolume-insecure-http-binding
spec:
  policyName: deny-datavolume-insecure-http
  validationActions: [Deny]
  matchResources:
    namespaceSelector: {}

kubevirt-bot · 2026-06-11T02:56:54Z

@xphyr: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-containerized-data-importer-e2e-s390x	`02e1bee`	link	unknown	`/test pull-containerized-data-importer-e2e-s390x`

Details

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. I understand the commands that are listed here.

akalenyu · 2026-06-11T10:37:10Z

Yeah VAP is good but we could follow the admin controlled flow similarly to insecureRegistries (insecureHTTPEndpoints?). What do folks think? I know we went with DV spec for ImageIO and I realize that I am effectively challenging that too.
/cc @Acedus @awels

akalenyu · 2026-06-11T10:37:52Z

/cc @mhenriks

xphyr · 2026-06-11T13:50:46Z

@akalenyu Here are my thoughts on insecureHTTPEndpoints and why I think defining the override as part of DataSource definition:

Primary concern is that this does not allow for dynamic endpoints. Every time someone wants/needs to pull from an insecure endpoint they will need to contact/work with a cluster admin to pull from an insecure site. This will be the only way to do it. This puts extra burden on the end user and cluster admin in all scenarios.
The secondary concern is that I think that this adds extra complexity to the code base (not a lot) needing to check the config and validating the config.
I would also argue that the ImageIO code should be updated to follow the same pattern so that the process is the same across multiple providers
The OpenStack importer also uses "insecureSkipVerify" as part of its process https://github.com/kubevirt/containerized-data-importer/blob/main/cmd/openstack-populator/openstack-populator.go#L49 so to maintain consistency this would probably need to be updated as well
Finally, it just feels cleaner ... if you want to understand a DataSource everything about it is defined in one place, with this implementation, if I want to know if insecureSkipVerify is required, I need only look at the datasource definition. To understand the same using the insecureHTTPEndpoints solution I need to look at the DataSource definition as well as the CDIConfig. As an end user, I wont have permissions to view the CDIConfig to see if my endpoint is even allowed.

Leveraging VAP is an existing solution that requires no extra code to write/test/maintain and is arguably more flexible allowing a Cluster Admin to:

do nothing, and let the end user "do the right thing"
disable it entirely with a VAP
allow specific endpoints as part of the VAP definition if this level of control is required

Signed-off-by: Mark DeNeve <3779715+xphyr@users.noreply.github.com>

akalenyu · 2026-06-29T09:23:50Z

/test all

akalenyu

Thanks for sticking through! I went ahead and prompted Claude to help with the unit test direction, something along those lines should be good

akalenyu · 2026-06-28T13:55:49Z

 	checksumValidator *ChecksumValidator
-
-	n image.NbdkitOperation
+	n                 image.NbdkitOperation


my editor corrects this back, how come you had to pad the spaces?

Its funny, my editor puts all the spaces in. Even when I remove them, it puts them back. (vscode). If you want I will edit the file outside vscode and commit to remove this change.

I dug into this, its go fmt that is making this formatting change. I am using go1.25 on my dev machine ...

akalenyu · 2026-06-29T09:33:28Z

 	source, sourceOk := pvc.Annotations[cc.AnnSource]
-	if sourceOk && source == cc.SourceImageio {
+	if sourceOk && (source == cc.SourceImageio || source == cc.SourceHTTP) {
 		if insecureSkipVerify, ok := pvc.Annotations[cc.AnnInsecureSkipVerify]; ok && insecureSkipVerify == "true" {


Yeah this was the direction. I was looking for something along those lines:
akalenyu@d6c22d0

Signed-off-by: xphyr <xphyr@users.noreply.github.com>

akalenyu · 2026-07-07T08:19:40Z

/test all

kubevirt-prow · 2026-07-07T08:48:20Z

@xphyr: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-containerized-data-importer-e2e-s390x	`ab32873`	link	false	`/test pull-containerized-data-importer-e2e-s390x`

Details

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. I understand the commands that are listed here.

akalenyu · 2026-07-07T12:45:45Z

/approve
/cc @Acedus

kubevirt-prow · 2026-07-07T12:45:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akalenyu

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [akalenyu]

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

xphyr added 4 commits June 5, 2026 12:31

updated tests to check for working insecureSkipVerify

3bdcb7f

Signed-off-by: xphyr <xphyr@users.noreply.github.com>

adding in functional test

8e9ba34

Signed-off-by: xphyr <xphyr@users.noreply.github.com>

updating docs to reflect the new insecureSkipVerify option

4649eeb

Signed-off-by: xphyr <xphyr@users.noreply.github.com>

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jun 9, 2026

kubevirt-bot added the size/L label Jun 9, 2026

simplifying the datavolume functional test

02e1bee

Signed-off-by: xphyr <xphyr@users.noreply.github.com>

akalenyu reviewed Jun 10, 2026

View reviewed changes

kubevirt-bot requested a review from Acedus June 10, 2026 11:32

xphyr and others added 2 commits June 10, 2026 12:01

adding in unit test for validating proper handling of AnnInsecureSkip…

8b34ebd

…Verify Signed-off-by: xphyr <xphyr@users.noreply.github.com>

added unit test to import-controller around isSecureTLS function

2aea8ad

Signed-off-by: Mark DeNeve <3779715+xphyr@users.noreply.github.com>

kubevirt-bot requested a review from awels June 11, 2026 10:37

kubevirt-bot requested a review from mhenriks June 11, 2026 10:37

kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jun 12, 2026

xphyr and others added 2 commits June 16, 2026 10:38

working on new tests, still not quite there yet

32285f2

Signed-off-by: Mark DeNeve <3779715+xphyr@users.noreply.github.com>

fixed the issue with my first unit test

d7620a4

Signed-off-by: Mark DeNeve <3779715+xphyr@users.noreply.github.com>

xphyr force-pushed the skiptlsverify branch from 3d2fd5e to d7620a4 Compare June 16, 2026 14:39

kubevirt-bot removed the dco-signoff: no Indicates the PR's author has not DCO signed all their commits. label Jun 16, 2026

kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Jun 16, 2026

akalenyu reviewed Jun 29, 2026

View reviewed changes

kubevirt-prow Bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jul 1, 2026

xphyr force-pushed the skiptlsverify branch from 2d93bfd to ec4cb75 Compare July 6, 2026 20:07

xphyr added 2 commits July 6, 2026 16:11

working on new tests, still not quite there yet

e73d9a4

Signed-off-by: xphyr <xphyr@users.noreply.github.com>

adding in additional tests from @akalenyu

ab32873

Signed-off-by: xphyr <xphyr@users.noreply.github.com>

xphyr force-pushed the skiptlsverify branch from ec4cb75 to ab32873 Compare July 6, 2026 20:12

kubevirt-prow Bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Jul 6, 2026

xphyr marked this pull request as ready for review July 7, 2026 12:32

kubevirt-prow Bot requested a review from arnongilboa July 7, 2026 12:33

kubevirt-prow Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 7, 2026

Uh oh!

Conversation

xphyr commented Jun 9, 2026

Uh oh!

kubevirt-bot commented Jun 9, 2026

Uh oh!

kubevirt-bot commented Jun 9, 2026

Uh oh!

akalenyu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akalenyu Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akalenyu Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xphyr Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akalenyu commented Jun 10, 2026

Uh oh!

coveralls commented Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Acedus commented Jun 10, 2026

Uh oh!

xphyr commented Jun 10, 2026

Uh oh!

kubevirt-bot commented Jun 11, 2026

Uh oh!

akalenyu commented Jun 11, 2026

Uh oh!

akalenyu commented Jun 11, 2026

Uh oh!

xphyr commented Jun 11, 2026

Uh oh!

akalenyu commented Jun 29, 2026

Uh oh!

akalenyu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akalenyu commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kubevirt-prow Bot commented Jul 7, 2026

Uh oh!

akalenyu commented Jul 7, 2026

Uh oh!

kubevirt-prow Bot commented Jul 7, 2026

Uh oh!

Reviewers

Assignees

Labels

akalenyu left a comment •

edited

Loading

akalenyu Jun 11, 2026 •

edited

Loading

akalenyu Jun 11, 2026 •

edited

Loading

xphyr Jun 12, 2026 •

edited

Loading

coveralls commented Jun 10, 2026 •

edited

Loading

akalenyu commented Jul 7, 2026 •

edited

Loading