Project

General

Profile

Actions

Bug #21086

closed

sdk/go/arvados should use TLS certificates from /etc/arvados/ca-certificates.crt if it exists

Added by Tom Clegg 5 months ago. Updated 4 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
SDKs
Story points:
0.5
Release relationship:
Auto

Description

Move the "choose a cert source" logic from sdk/go/arvadosclient to sdk/go/arvados and make sure both libraries use it.


Subtasks 1 (0 open1 closed)

Task #21131: Review 21086-cert-sourceResolvedTom Clegg10/19/2023Actions
Actions #2

Updated by Tom Clegg 5 months ago

Out of the box, Go checks the following locations:

// Possible certificate files; stop after finding one.                                                                                                  
var certFiles = []string{
        "/etc/ssl/certs/ca-certificates.crt",                // Debian/Ubuntu/Gentoo etc.                                                               
        "/etc/pki/tls/certs/ca-bundle.crt",                  // Fedora/RHEL 6                                                                           
        "/etc/ssl/ca-bundle.pem",                            // OpenSUSE                                                                                
        "/etc/pki/tls/cacert.pem",                           // OpenELEC                                                                                
        "/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem", // CentOS/RHEL 7                                                                           
        "/etc/ssl/cert.pem",                                 // Alpine Linux                                                                            
}

// Possible directories with certificate files; all will be read.                                                                                       
var certDirectories = []string{
        "/etc/ssl/certs",               // SLES10/SLES11, https://golang.org/issue/12139                                                                
        "/etc/pki/tls/certs",           // Fedora/RHEL                                                                                                  
        "/system/etc/security/cacerts", // Android                                                                                                      
}

SSL_CERT_FILE and SSL_CERT_DIR can be used to override these locations.

Currently, the arvadosclient library modifies its http client to effectively replace certFiles with its own list ("/etc/arvados/ca-certificates.crt" + debian + fedora paths), drop the other default locations, disable the SSL_CERT_FILE and SSL_CERT_DIR env var mechanism, and also disable loading certs from the default certDirectories. I suspect disabling the env vars and certDirectories was accidental -- in fact, it seems our customization (cc1dbda8f559ab43f326c77595d4af87e8ca7a33) predates those stdlib features (https://cs.opensource.google/go/go/+/e83bcd95a4a86e4caf2faa78158170d512dd9de5) by a few months.

Also, the way the current code modifies the http client doesn't align very well with the way the arvados package lets the caller provide its own http client.

So I'm thinking another way to load from our custom location, which would be more compatible with the other OSes and other local customizations, would be to automatically set SSL_CERT_FILE to /etc/arvados/ca-certificates.crt only if that file exists and SSL_CERT_FILE is not already set to a different readable file. This would also have a side effect that child processes that support those env vars would also use the certificates, which seems desirable if anything.

As an aside, is there even a reason why we use the custom path /etc/arvados/ca-certificates.crt instead of adding certs to /etc/ssl/certs/ where unmodified SSL clients would already know how to find them?

Actions #3

Updated by Lucas Di Pentima 5 months ago

Tom Clegg wrote in #note-2:

So I'm thinking another way to load from our custom location, which would be more compatible with the other OSes and other local customizations, would be to automatically set SSL_CERT_FILE to /etc/arvados/ca-certificates.crt only if that file exists and SSL_CERT_FILE is not already set to a different readable file. This would also have a side effect that child processes that support those env vars would also use the certificates, which seems desirable if anything.

I agree this would be good to have.

As an aside, is there even a reason why we use the custom path /etc/arvados/ca-certificates.crt instead of adding certs to /etc/ssl/certs/ where unmodified SSL clients would already know how to find them?

The only thing I can think could be a reason is to avoid the standard ca-certificates.crt overwrite the customized one when installing the related package.

Actions #4

Updated by Tom Clegg 5 months ago

21086-cert-source @ eccb6ba135ebe1bc73bdeeed89150d3c4ee26126 -- developer-run-tests: #3868

  • All agreed upon points are implemented / addressed.
    • ✅ now both Go SDKs will load /etc/arvados/ca-certificates.crt if it exists. (arvadosclient imports arvados, so the init() func will run if a program imports either arvadosclient or arvados)
  • Anything not implemented (discovered or discussed during work) has a follow-up story.
    • N/A
  • Code is tested and passing, both automated and manual, what manual testing was done is described
    • ✅ There's a test that only works if /etc/arvados/ca-certificates.crt exists on the test host (otherwise it gets skipped). Created that file & tested manually on my dev machine.
  • Documentation has been updated.
    • N/A
  • Behaves appropriately at the intended scale (describe intended scale).
    • N/A
  • Considered backwards and forwards compatibility issues between client and server.
    • N/A
  • Follows our coding standards and GUI style guidelines.
Actions #5

Updated by Tom Clegg 4 months ago

  • Status changed from New to In Progress
Actions #6

Updated by Lucas Di Pentima 4 months ago

This LGTM, thanks!

Actions #7

Updated by Tom Clegg 4 months ago

  • % Done changed from 0 to 100
  • Status changed from In Progress to Resolved
Actions

Also available in: Atom PDF