Restore compatibility in specifying custom CAs by using Go client #1735

maya-r · 2021-04-04T16:39:01Z

This restores support for the following scenarios:

Now the system certs are considered as valid when a custom CA is used.
The custom CA will be accepted regardless of the key value used in the configmap.

Add a test for the second scenario.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1946100

Special notes for your reviewer:
The scenario of system-wide certs is not tested because it requires using an external server.

Release note:

Restore compatibility: users are once again able to specify custom CAs for use in import in names other than tls.crt

maya-r · 2021-04-04T16:39:18Z

Needs tests.

awels · 2021-04-05T11:29:40Z

Also needs to fix the unit tests and imageio functional tests that are now broken.

awels

So why not use x509.SystemCertPool to create a cert pool, and then append the certs from the other directories to it. Then pass this cert pool to the http client somehow instead of writing everything to a file somewhere, and re-reading it into a cert pool.

awels · 2021-04-05T15:46:00Z

pkg/importer/http-datasource.go

+// CURL either supports a hashed-directory of certificates or a single file certificate.
+// Create a single file containing all our certificates.
+func createCAFile(certDir string) error {
+	systemCertDir := "/etc/pki/tls/certs/"


Why are we hard coding some path that may or may not exist in various variations of linux. That is why we used x509.SystemCertPool, so we don't have to care about the paths of the system certs.

I mainly want to create a file later to be used by nbdkit curl plugin, I didn't find a way to go from a certpool to a file

awels · 2021-04-05T15:46:58Z

pkg/importer/http-datasource.go

-	// append the user-provided trusted CA certificates bundle when making egress connections using proxy
-	if files, err := ioutil.ReadDir(common.ImporterProxyCertDir); err == nil {
+	for dir := range certDirs {
+		files, err := ioutil.ReadDir(certDir)


Why are looping over the different dirs, and then only reading certDir, I am assuming you want

files, err := ioutils.ReadDir(dir)

maya-r · 2021-04-08T13:45:02Z

I went back because I had trouble making the more complex patch work.
We used to use the Go client before the nbdkit changes - the catch-all before used to be "use scratch space" and it stopped being that.

maya-r · 2021-04-08T13:53:35Z

oops, a rebase accident... I wanted to test that the test fails before the fix, and accidentally pushed that version :-)

maya-r · 2021-04-08T15:05:56Z

/test pull-cdi-unit-test
doesn't seem to fail locally. wondering if this is a time-based flake.

tomob · 2021-04-16T09:18:04Z

tests/utils/configmaps.go

+	for _, value := range srcCm.Data {
+		certBytes = value
+	}


Why the iteration? It gets the last item, so let's just to that.

I wasn't sure about the idiomatic way to do it, there's usually only going to be one element here, I could add a break for clarity.

This restores support for the following scenarios: - Now the system certs are considered as valid when a custom CA is used. - The custom CA will be accepted regardless of the key value used in the configmap. Add a test for the second scenario. Signed-off-by: Maya Rashish <mrashish@redhat.com>

Signed-off-by: Maya Rashish <mrashish@redhat.com>

tomob · 2021-04-26T11:40:55Z

/lgtm

aglitke · 2021-04-26T19:59:52Z

/retest

maya-r · 2021-04-29T11:45:11Z

/test pull-containerized-data-importer-e2e-k8s-1.17-ceph

maya-r · 2021-05-04T13:03:25Z

/cherry-pick release-v1.34

kubevirt-bot · 2021-05-04T13:03:26Z

@maya-r: once the present PR merges, I will cherry-pick it on top of release-v1.34 in a new PR and assign it to you.

In response to this:

/cherry-pick release-v1.34

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.

maya-r · 2021-05-06T07:44:20Z

ping @awels

awels · 2021-05-06T18:57:52Z

So basically this is reverting some of the usage of nbdkit because we can't get it to properly read the certs?

maya-r · 2021-05-09T05:49:32Z

It's switching the custom cert case to use the Go client for downloading, so we have the same behaviour as we did before.
Nbdkit is still used later on.

Had a hard time replicating the cert behaviour with nbdkit (if a cert is specified, global certs are still accepted. if multiple certs are specified, they are all accepted).

Signed-off-by: Maya Rashish <mrashish@redhat.com>

awels · 2021-05-10T11:28:40Z

/test pull-containerized-data-importer-e2e-k8s-1.17-ceph

awels · 2021-05-10T12:33:18Z

/approve

kubevirt-bot · 2021-05-10T12:34:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awels

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:

~~OWNERS~~ [awels]

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

brybacki · 2021-05-12T11:59:05Z

/lgtm

kubevirt-bot · 2021-05-12T14:33:41Z

@maya-r: new pull request created: #1787

In response to this:

/cherry-pick release-v1.34

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.

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. size/M labels Apr 4, 2021

kubevirt-bot requested review from awels and mhenriks April 4, 2021 16:40

awels requested changes Apr 5, 2021

View reviewed changes

kubevirt-bot assigned awels Apr 5, 2021

maya-r force-pushed the nbdkit-compat-custom-ca branch from 5875f0e to e2a43ed Compare April 8, 2021 13:42

maya-r force-pushed the nbdkit-compat-custom-ca branch from e2a43ed to 89d2b85 Compare April 8, 2021 13:59

tomob reviewed Apr 16, 2021

View reviewed changes

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 21, 2021

maya-r force-pushed the nbdkit-compat-custom-ca branch from 89d2b85 to 227f5b6 Compare April 21, 2021 10:07

kubevirt-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 21, 2021

maya-r added 2 commits April 25, 2021 10:21

Try to make it clearer that we only want one value

cc2917b

Signed-off-by: Maya Rashish <mrashish@redhat.com>

maya-r force-pushed the nbdkit-compat-custom-ca branch from 227f5b6 to cc2917b Compare April 25, 2021 12:11

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 25, 2021

kubevirt-bot assigned tomob Apr 26, 2021

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 26, 2021

maya-r changed the title ~~Restore compatibility in specifying custom CAs~~ Restore compatibility in specifying custom CAs by using Go client May 2, 2021

Update docs, pointed out by Alex Kalenyuk.

9c75193

Signed-off-by: Maya Rashish <mrashish@redhat.com>

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label May 9, 2021

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 10, 2021

kubevirt-bot assigned brybacki May 12, 2021

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label May 12, 2021

kubevirt-bot merged commit c735fd5 into kubevirt:main May 12, 2021

kubevirt-bot mentioned this pull request May 12, 2021

[release-v1.34] Restore compatibility in specifying custom CAs by using Go client #1787

Merged

awels mentioned this pull request Jun 4, 2021

Error setting certificate verify locations when create datavolume #1817

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore compatibility in specifying custom CAs by using Go client #1735

Restore compatibility in specifying custom CAs by using Go client #1735

maya-r commented Apr 4, 2021 •

edited

maya-r commented Apr 4, 2021

awels commented Apr 5, 2021

awels left a comment

awels Apr 5, 2021

maya-r Apr 6, 2021

awels Apr 5, 2021

maya-r commented Apr 8, 2021 •

edited

maya-r commented Apr 8, 2021

maya-r commented Apr 8, 2021

tomob Apr 16, 2021

maya-r Apr 20, 2021

tomob commented Apr 26, 2021

aglitke commented Apr 26, 2021

maya-r commented Apr 29, 2021

maya-r commented May 4, 2021

kubevirt-bot commented May 4, 2021

maya-r commented May 6, 2021

awels commented May 6, 2021

maya-r commented May 9, 2021 •

edited

awels commented May 10, 2021

awels commented May 10, 2021

kubevirt-bot commented May 10, 2021

brybacki commented May 12, 2021

kubevirt-bot commented May 12, 2021

Restore compatibility in specifying custom CAs by using Go client #1735

Restore compatibility in specifying custom CAs by using Go client #1735

Conversation

maya-r commented Apr 4, 2021 • edited

maya-r commented Apr 4, 2021

awels commented Apr 5, 2021

awels left a comment

Choose a reason for hiding this comment

awels Apr 5, 2021

Choose a reason for hiding this comment

maya-r Apr 6, 2021

Choose a reason for hiding this comment

awels Apr 5, 2021

Choose a reason for hiding this comment

maya-r commented Apr 8, 2021 • edited

maya-r commented Apr 8, 2021

maya-r commented Apr 8, 2021

tomob Apr 16, 2021

Choose a reason for hiding this comment

maya-r Apr 20, 2021

Choose a reason for hiding this comment

tomob commented Apr 26, 2021

aglitke commented Apr 26, 2021

maya-r commented Apr 29, 2021

maya-r commented May 4, 2021

kubevirt-bot commented May 4, 2021

maya-r commented May 6, 2021

awels commented May 6, 2021

maya-r commented May 9, 2021 • edited

awels commented May 10, 2021

awels commented May 10, 2021

kubevirt-bot commented May 10, 2021

brybacki commented May 12, 2021

kubevirt-bot commented May 12, 2021

maya-r commented Apr 4, 2021 •

edited

maya-r commented Apr 8, 2021 •

edited

maya-r commented May 9, 2021 •

edited