Skip to content

eve support datastore https certs#2136

Merged
eriknordmark merged 1 commit into
lf-edge:masterfrom
naiming-zededa:naiming-datastore-ssl-certs
Jul 1, 2021
Merged

eve support datastore https certs#2136
eriknordmark merged 1 commit into
lf-edge:masterfrom
naiming-zededa:naiming-datastore-ssl-certs

Conversation

@naiming-zededa
Copy link
Copy Markdown
Contributor

Signed-off-by: Naiming Shen naiming@zededa.com

  • support for datastore cert chain from user configuration
  • if the ds is local App based, skip verification for server cert

@naiming-zededa naiming-zededa force-pushed the naiming-datastore-ssl-certs branch from 5300b12 to 15bc1d4 Compare June 18, 2021 19:14
Comment thread libs/zedUpload/datastore_azure.go Outdated
Comment thread libs/zedUpload/datastore_aws.go Outdated
Comment thread libs/zedUpload/datastore_oci.go Outdated
Comment thread libs/zedUpload/datastore_sftp.go Outdated
Comment thread pkg/pillar/cmd/downloader/syncop.go Outdated
Copy link
Copy Markdown
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Writing the cert to the ShareCertDirName means that other comunication such as other downloads will also trust the added cert, and here we just want to allow it for a particular download using that datastore.

Thus I think it is a lot safer to use the pattern which the zedcloud package uses, where we have a GetTlsConfig which loads certificates to get a tlsconfig struct, then this is used to form a transport and the transport is used to client := &http.Client{Transport: transport}
Here we have a ep.hClient which can be modified to have a particular TlsConfig with the specified certificate.
Alternatively, you can add an argument to httpClientSrcIP() to takes an optional certificate to use to create the TlsConfig. That ensures that the added cert only applies to this particular http client.

Also, presumably we should do this for sftp and the other transports; we have the same approach for all that we create a client with a source IP hence we should be able to specify the TlsConfig for all of them as well.

@naiming-zededa
Copy link
Copy Markdown
Contributor Author

Writing the cert to the ShareCertDirName means that other comunication such as other downloads will also trust the added cert, and here we just want to allow it for a particular download using that datastore.

Thus I think it is a lot safer to use the pattern which the zedcloud package uses, where we have a GetTlsConfig which loads certificates to get a tlsconfig struct, then this is used to form a transport and the transport is used to client := &http.Client{Transport: transport}
Here we have a ep.hClient which can be modified to have a particular TlsConfig with the specified certificate.
Alternatively, you can add an argument to httpClientSrcIP() to takes an optional certificate to use to create the TlsConfig. That ensures that the added cert only applies to this particular http client.

Also, presumably we should do this for sftp and the other transports; we have the same approach for all that we create a client with a source IP hence we should be able to specify the TlsConfig for all of them as well.

Sure. I'll make the cert specific to the datastore attached for downloading. the sftp is not http/https related. we can have a separate story on that

@naiming-zededa naiming-zededa force-pushed the naiming-datastore-ssl-certs branch 2 times, most recently from fc60213 to 2bb4bd5 Compare June 24, 2021 00:30
Comment thread libs/zedUpload/datastore_http.go Outdated
Comment on lines +91 to +97
caCertPool := x509.NewCertPool()
for _, pem := range certs {
if !caCertPool.AppendCertsFromPEM(pem) {
return fmt.Errorf("Failed to append datastore certs")
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine with me, but then we need to document that the certs in the UI are not added to the base Linux set of root CAs. Thus different than what we do for the proxy where we add the cert(s) to that base set.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right. in proxy case, there is a transport.Proxy item and also the proxy cert chain append into the linux certs directory. In this datastore case, the user explicitly want to have this cert chain to verify for this endpoint of https.

Comment thread libs/zedUpload/datastore_http.go
Copy link
Copy Markdown
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let it test

@naiming-zededa naiming-zededa force-pushed the naiming-datastore-ssl-certs branch from 2bb4bd5 to 8f48b72 Compare June 24, 2021 18:58
Copy link
Copy Markdown
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run eden tests again

@naiming-zededa naiming-zededa force-pushed the naiming-datastore-ssl-certs branch 2 times, most recently from 6907e3a to df47099 Compare June 28, 2021 16:56
Copy link
Copy Markdown
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run eden again

@naiming-zededa naiming-zededa force-pushed the naiming-datastore-ssl-certs branch from bd0a30a to eb98305 Compare June 29, 2021 17:06
Copy link
Copy Markdown
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run run again

@giggsoff
Copy link
Copy Markdown
Contributor

@naiming-zededa as I can see, we should also modify url for oci transport as well as sftp, because we split it

serverURL, remoteName, err = ociRepositorySplit(dsCtx.DownloadURL)

Seems, something like should work

diff --git a/pkg/pillar/zedcloud/lookupproxy.go b/pkg/pillar/zedcloud/lookupproxy.go
index 03a75f6b6..59500f5e6 100644
--- a/pkg/pillar/zedcloud/lookupproxy.go
+++ b/pkg/pillar/zedcloud/lookupproxy.go
@@ -131,9 +131,10 @@ func IntfLookupProxyCfg(log *base.LogObject, status *types.DeviceNetworkStatus,
        // if download URL has "http://" or "https://" then no change here regardless of proxy
        // if there is proxy on this intf, treat empty url scheme as for https or http but prefer https,
        // replace the passed-in download-url scheme "docker://" and maybe "sftp://" later, to https
-       // XXX for sftp, currently the FQDN can not have scheme attached, but url.Parse will fail without it
-       if trType == zedUpload.SyncSftpTr && !strings.Contains(downloadURL, "://") {
-               downloadURL = "sftp://" + downloadURL
+       // XXX for sftp and oci, currently the FQDN can not have scheme attached, but url.Parse will fail without it
+       if (trType == zedUpload.SyncSftpTr || trType == zedUpload.SyncOCIRegistryTr) &&
+               !strings.Contains(downloadURL, "://") {
+               downloadURL = fmt.Sprintf("%s://%s", trType, downloadURL)
        }
        passURL, err := url.Parse(downloadURL)
        if err != nil {

@naiming-zededa
Copy link
Copy Markdown
Contributor Author

@naiming-zededa as I can see, we should also modify url for oci transport as well as sftp, because we split it

serverURL, remoteName, err = ociRepositorySplit(dsCtx.DownloadURL)

Seems, something like should work

diff --git a/pkg/pillar/zedcloud/lookupproxy.go b/pkg/pillar/zedcloud/lookupproxy.go
index 03a75f6b6..59500f5e6 100644
--- a/pkg/pillar/zedcloud/lookupproxy.go
+++ b/pkg/pillar/zedcloud/lookupproxy.go
@@ -131,9 +131,10 @@ func IntfLookupProxyCfg(log *base.LogObject, status *types.DeviceNetworkStatus,
        // if download URL has "http://" or "https://" then no change here regardless of proxy
        // if there is proxy on this intf, treat empty url scheme as for https or http but prefer https,
        // replace the passed-in download-url scheme "docker://" and maybe "sftp://" later, to https
-       // XXX for sftp, currently the FQDN can not have scheme attached, but url.Parse will fail without it
-       if trType == zedUpload.SyncSftpTr && !strings.Contains(downloadURL, "://") {
-               downloadURL = "sftp://" + downloadURL
+       // XXX for sftp and oci, currently the FQDN can not have scheme attached, but url.Parse will fail without it
+       if (trType == zedUpload.SyncSftpTr || trType == zedUpload.SyncOCIRegistryTr) &&
+               !strings.Contains(downloadURL, "://") {
+               downloadURL = fmt.Sprintf("%s://%s", trType, downloadURL)
        }
        passURL, err := url.Parse(downloadURL)
        if err != nil {

@giggsoff Right, i did change those in my workspace, but i'm waiting for eden error we have seen on 'route unreachable', any ida?

Signed-off-by: Naiming Shen <naiming@zededa.com>
@naiming-zededa naiming-zededa force-pushed the naiming-datastore-ssl-certs branch from eb98305 to 52cc2b8 Compare June 30, 2021 17:34
Copy link
Copy Markdown
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Let's run eden tests once more

@eriknordmark eriknordmark merged commit c0d9a1c into lf-edge:master Jul 1, 2021
@naiming-zededa naiming-zededa deleted the naiming-datastore-ssl-certs branch July 1, 2021 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants