Fixes to fleetctl debug connection and TLS certs documentation #20166
Conversation
lucasmrod
requested review from
rachaelshaw,
a team,
getvictor,
roperzh and
gillespi314
as code owners
lucasmrod
requested review from
lukeheath,
georgekarrv and
sharon-fdm
as code owners
| @@ -57,9 +57,15 @@ func ValidateConnectionContext(ctx context.Context, pool *x509.CertPool, targetU | |||
| } | |||
|
|
|||
| cert := state.PeerCertificates[0] | |||
| intermediates := x509.NewCertPool() | |||
| for _, intermediate := range state.PeerCertificates[1:] { | |||
There was a problem hiding this comment.
can we run into an out of range err here?
There was a problem hiding this comment.
Thanks for checking!
It seems that at this point we have len(state.PeerCertificates) > 0, so with a slice of 1 this is a-ok (won't loop)
There was a problem hiding this comment.
ah right, that will produce an empty slice if len()==1
orbit/docs/TUF-Notes.md
Outdated
There was a problem hiding this comment.
I'm deleting this file because it contains outdated docs.
|
@lucasmrod, Can you see any scenario where we removed (from the server) any ability that could prevent old agents from communicating? |
There was a problem hiding this comment.
The changes LGTM.
Before merging please answer this question:
@lucasmrod, Can you see any scenario where we removed (from the server) any ability that could prevent old agents from communicating?
Do you mean changes in the past or in the changes on this PR? What could happen with very old agent is that their pinned certs (e.g. |
|
@lucasmrod I mean changes in this PR that could prevent old agents from communicating |
|
If so, we need to communicate this very clear to customer support! |
This has to be understood by customer support. And maybe allow feature-flagging it. |
Ah no server changes on this PR. Changes in this PR only impact client side:
|
|
If it only affects new agents then NP. |
Maybe fleetd users in the far future will find issues with outdated certs. But that's something unrelated to changes in this PR. |
|
Understood. |
![greptile-apps[bot]](https://github.com/avatars/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
- Added new documentation file
articles/certificates-in-fleetd.mdfor configuring TLS CA root certificates. - Enhanced
cmd/fleetctl/debug.goto handle various TLS certificate scenarios and improve output messages. - Updated
cmd/fleetctl/debug_test.gowith comments for test case context. - Introduced a new section in
docs/Using Fleet/enroll-hosts.mdon debugging TLS certificates. - Modified
pkg/certificate/certificate.goto support intermediate certificates in TLS verification.
9 file(s) reviewed, 21 comment(s)
Edit PR Review Bot Settings
| @@ -0,0 +1,62 @@ | |||
| # Certificates in fleetd | |||
|
|
|||
| There are three components in fleetd connecting to the Fleet server using TLS: `orbit`, `Fleet Desktop` and `osqueryd`. | |||
There was a problem hiding this comment.
ℹ️ info: Clarified the components in fleetd that connect to the Fleet server using TLS.
| - By default, `orbit` and `Fleet Desktop` will use the system's CA root store to connect to Fleet. | ||
| - `osqueryd` doesn't support using the system's CA root store, it requires passing in a certificate file with the root CA store (via the `--tls_server_certs` flag). The `fleetctl` executable contains an embedded `certs.pem` file generated from https://curl.se/docs/caextract.html [0]. When generating a fleetd package with `fleetctl package` such embedded `certs.pem` file is added to the package [1]. Fleetd configures `osqueryd` to use the `certs.pem` file as CA root store by setting the `--tls_server_certs` argument to such path. |
There was a problem hiding this comment.
ℹ️ info: Described the default behavior for 'orbit', 'Fleet Desktop', and 'osqueryd' regarding CA root store usage.
|
|
||
| ## Using `--fleet-certificate` in `fleetctl package` | ||
|
|
||
| When using `--fleet-certificate` in `fleetctl package`, such certificate file is used as a CA root store by `orbit`, `Fleet Desktop` and `osqueryd` (the system's CA store is not used when generating the fleetd package this way). |
There was a problem hiding this comment.
ℹ️ info: Explained the effect of using the '--fleet-certificate' flag in 'fleetctl package'.
| TLS clients require the CA root and all intermediate certificates that signed the leaf server certificate to be verified. | ||
| This means that if the bundled certificate in fleetd [1] doesn't have intermediate certificates that signed the leaf certificate, then the Fleet server will have to be configured to serve the "fullchain". | ||
| Here's a list of some scenarios assuming your Fleet server certificate has an intermediate signing certificate: | ||
| - ✅ Using fullchain in the Fleet server and root CA only client side. | ||
| - ✅ Using fullchain in the Fleet server and root+intermediate bundle client side. | ||
| - ✅ Using the leaf certificate in the Fleet server and root+intermediate bundle client side. | ||
| - ✅ Using the leaf certificate + intermediate bundle in the Fleet server and root CA only client side. | ||
| - ❌ Using the leaf certificate in the Fleet server and root CA only client side. In this scenario the client side (fleetd) doesn't know of the intermediate certificate and thus cannot verify it. |
There was a problem hiding this comment.
ℹ️ info: Outlined scenarios for verifying certificates with intermediate signing certificates.
| When there are certificate issues you will see the following kind of errors in server logs: | ||
| ``` | ||
| 2024/07/05 15:03:52 http: TLS handshake error from <remote_ip>:<remote_port>: remote error: tls: bad certificate | ||
| 2024/07/05 15:03:53 http: TLS handshake error from <remote_ip>:<remote_port>: local error: tls: bad record MAC | ||
| ``` | ||
| and the following kind of errors on the client side (fleetd): | ||
| ``` | ||
| 2024-07-05T15:04:52-03:00 DBG get config error="POST /api/fleet/orbit/config: Post \"https://fleet.example.com/api/fleet/orbit/config\": tls: failed to verify certificate: x509: certificate signed by unknown authority" | ||
| ``` | ||
| ``` | ||
| W0705 15:16:44.739495 1251102656 init.cpp:760] Error reading config: Request error: certificate verify failed | ||
| ``` |
There was a problem hiding this comment.
ℹ️ info: Provided example error messages for diagnosing certificate issues.
| // | ||
| //go:embed certs.pem | ||
| var osqueryCerts []byte | ||
| var OsqueryCerts []byte |
There was a problem hiding this comment.
ℹ️ info: Changed variable name from osqueryCerts to OsqueryCerts to follow Go naming conventions for exported variables.
|
|
||
| func writeOsqueryCertPEM(opt Options, orbitRoot string) error { | ||
| path := filepath.Join(orbitRoot, "certs.pem") | ||
| if err := secure.MkdirAll(filepath.Dir(path), constant.DefaultDirMode); err != nil { | ||
| return fmt.Errorf("mkdir: %w", err) | ||
| } | ||
|
|
||
| if err := os.WriteFile(path, osqueryCerts, 0o644); err != nil { | ||
| if err := os.WriteFile(path, OsqueryCerts, 0o644); err != nil { |
There was a problem hiding this comment.
ℹ️ info: Updated to use OsqueryCerts instead of osqueryCerts when writing the file.
| intermediates := x509.NewCertPool() | ||
| for _, intermediate := range state.PeerCertificates[1:] { | ||
| intermediates.AddCert(intermediate) | ||
| } |
There was a problem hiding this comment.
ℹ️ info: Added support for intermediate certificates by creating a new cert pool and adding all intermediate certificates to it.
| DNSName: parsed.Hostname(), | ||
| Roots: pool, | ||
| Intermediates: intermediates, |
There was a problem hiding this comment.
ℹ️ info: Included the intermediates cert pool in the verification options to ensure a complete chain of trust.
| @@ -172,7 +172,6 @@ func newBaseClient( | |||
| // Ignoring "G402: TLS InsecureSkipVerify set true", needed for development/testing. | |||
| tlsConfig.InsecureSkipVerify = true //nolint:gosec | |||
| default: | |||
There was a problem hiding this comment.
ℹ️ info: Removed comment about system certs not working on Windows.
| @@ -238,7 +238,7 @@ jobs: | |||
|
|
|||
| - name: Uninstall Orbit | |||
| run: | | |||
| ./orbit/tools/cleanup/cleanup_macos.sh | |||
| sudo ./orbit/tools/cleanup/cleanup_macos.sh | |||
There was a problem hiding this comment.
noted this in slack, over-communicating here that there's an alternative fix https://github.com/fleetdm/fleet/pull/20226/files
I wouldn't change it so you don't lose all the reviews
#6085
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.