-
Notifications
You must be signed in to change notification settings - Fork 402
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes to fleetctl debug connection and TLS certs documentation #20166
Changes from 5 commits
9e37b62
daa8061
e7bc828
896d88d
9016dae
a84d7f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
* Fixed `fleetctl debug connection` to support server TLS certificates with intermediates. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ℹ️ info: Documented the fix for supporting server TLS certificates with intermediates. |
||
* Added support to `fleetctl debug connection` to test TLS connection with the embedded certs.pem in the fleetctl executable (default root CA used to generate fleetd packages). This can help find issues during package generation instead of during package installation. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ℹ️ info: Documented the addition of testing TLS connections with embedded certs.pem. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,9 +12,11 @@ import ( | |
"net/http" | ||
"net/url" | ||
"os" | ||
"path/filepath" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ℹ️ info: Added import for |
||
"strings" | ||
"time" | ||
|
||
"github.com/fleetdm/fleet/v4/orbit/pkg/packaging" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ℹ️ info: Added import for |
||
"github.com/fleetdm/fleet/v4/pkg/certificate" | ||
"github.com/fleetdm/fleet/v4/pkg/secure" | ||
"github.com/fleetdm/fleet/v4/server/fleet" | ||
|
@@ -447,10 +449,39 @@ or provide an <address> argument to debug: fleetctl debug connection localhost:8 | |
cc.Address = "https://" + cc.Address | ||
} | ||
|
||
if certPath := getFleetCertificate(c); certPath != "" { | ||
// if a certificate is provided, use it as root CA | ||
cc.RootCA = certPath | ||
cc.TLSSkipVerify = false | ||
usingHTTPS := strings.HasPrefix(cc.Address, "https://") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ℹ️ info: Introduced |
||
|
||
// | ||
// Scenarios: | ||
// - If a --fleet-certificate is provided, use it as root CA. | ||
// - If a --fleet-certificate is not provided, but a cc.RootCA is set in the configuration, use it as root CA. | ||
// - If a --fleet-certificate is not provided and there isn't a cc.RootCA set in the configuration, use the embedded certs as root CA. | ||
// | ||
usingEmbeddedCA := false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ℹ️ info: Implemented logic to handle different scenarios for using TLS certificates, including using embedded certificates if none are provided. |
||
if usingHTTPS { | ||
certPath := getFleetCertificate(c) | ||
if certPath != "" { | ||
// if a certificate is provided, use it as root CA | ||
cc.RootCA = certPath | ||
cc.TLSSkipVerify = false | ||
} else { // --fleet-certificate is not set | ||
if cc.RootCA == "" { | ||
// If a certificate is not provided and a cc.RootCA is not set in the configuration, | ||
// then use the embedded root CA which is used by osquery to connect to Fleet. | ||
usingEmbeddedCA = true | ||
tmpDir, err := os.MkdirTemp("", "") | ||
if err != nil { | ||
return fmt.Errorf("failed to create temporary directory: %w", err) | ||
} | ||
certPath := filepath.Join(tmpDir, "certs.pem") | ||
if err := os.WriteFile(certPath, packaging.OsqueryCerts, 0o600); err != nil { | ||
return fmt.Errorf("failed to create temporary certs.pem file: %s", err) | ||
} | ||
defer os.RemoveAll(certPath) | ||
cc.RootCA = certPath | ||
cc.TLSSkipVerify = false | ||
} | ||
} | ||
} | ||
|
||
cli, baseURL, err := rawHTTPClientFromConfig(cc) | ||
|
@@ -460,16 +491,20 @@ or provide an <address> argument to debug: fleetctl debug connection localhost:8 | |
|
||
// print a summary of the address and TLS context that is investigated | ||
fmt.Fprintf(c.App.Writer, "Debugging connection to %s; Configuration context: %s; ", baseURL.Hostname(), configContext) | ||
rootCA := "(system)" | ||
if cc.RootCA != "" { | ||
rootCA = cc.RootCA | ||
} | ||
fmt.Fprintf(c.App.Writer, "Root CA: %s; ", rootCA) | ||
tlsMode := "secure" | ||
if cc.TLSSkipVerify { | ||
tlsMode = "insecure" | ||
|
||
if usingHTTPS { | ||
rootCA := cc.RootCA | ||
if usingEmbeddedCA { | ||
rootCA += " (embedded certs used by default to generate fleetd packages)" | ||
} | ||
fmt.Fprintf(c.App.Writer, "Root CA: %s; ", rootCA) | ||
|
||
tlsMode := "secure" | ||
if cc.TLSSkipVerify { | ||
tlsMode = "insecure" | ||
} | ||
fmt.Fprintf(c.App.Writer, "TLS: %s.\n", tlsMode) | ||
} | ||
fmt.Fprintf(c.App.Writer, "TLS: %s.\n", tlsMode) | ||
|
||
// Check that the url's host resolves to an IP address or is otherwise | ||
// a valid IP address directly. | ||
|
@@ -479,14 +514,19 @@ or provide an <address> argument to debug: fleetctl debug connection localhost:8 | |
fmt.Fprintf(c.App.Writer, "Success: can resolve host %s.\n", baseURL.Hostname()) | ||
|
||
// Attempt a raw TCP connection to host:port. | ||
if err := dialHostPort(c.Context, timeoutPerCheck, baseURL.Host); err != nil { | ||
dialURL := baseURL.Host | ||
if baseURL.Port() == "" { | ||
fmt.Fprintf(c.App.Writer, "Assumming port 443.\n") | ||
dialURL += ":443" | ||
Comment on lines
+518
to
+520
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ℹ️ info: Added logic to assume port 443 if no port is specified in the URL. |
||
} | ||
if err := dialHostPort(c.Context, timeoutPerCheck, dialURL); err != nil { | ||
return fmt.Errorf("Fail: dial server: %w", err) | ||
} | ||
fmt.Fprintf(c.App.Writer, "Success: can dial server at %s.\n", baseURL.Host) | ||
|
||
if cert := getFleetCertificate(c); cert != "" { | ||
// Run some validations on the TLS certificate. | ||
if err := checkFleetCert(c.Context, timeoutPerCheck, cert, baseURL.Host); err != nil { | ||
// Run some validations on the TLS certificate. | ||
if usingHTTPS { | ||
if err := checkFleetCert(c.Context, timeoutPerCheck, cc.RootCA, baseURL.Host); err != nil { | ||
return fmt.Errorf("Fail: certificate: %w", err) | ||
} | ||
fmt.Fprintln(c.App.Writer, "Success: TLS certificate seems valid.") | ||
Comment on lines
+528
to
532
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ℹ️ info: Added validation for TLS certificates when using HTTPS. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,7 +44,8 @@ oug6edBNpdhp8r2/4t6n3AouK0/zG2naAlmXV0JoFuEvy2bX0BbbbPg+v4WNZIsC | |
) | ||
|
||
func TestDebugConnectionCommand(t *testing.T) { | ||
t.Run("without certificate", func(t *testing.T) { | ||
t.Run("without certificate, plain http server", func(t *testing.T) { | ||
// Plain HTTP server | ||
Comment on lines
+47
to
+48
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ℹ️ info: Added a comment to indicate that a plain HTTP server is being used in this test case. |
||
_, ds := runServerWithMockedDS(t) | ||
|
||
ds.VerifyEnrollSecretFunc = func(ctx context.Context, secret string) (*fleet.EnrollSecret, error) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
# fleetd certificates | ||
|
||
There are three components in fleetd connecting to the Fleet server (TLS): `orbit`, `Fleet Desktop` and `osqueryd`. | ||
This document aims to describe how TLS CA root certificates are configured in fleetd to connect to a Fleet server securely. | ||
|
||
## Default | ||
|
||
The default behavior is using the `fleetctl package` command without the `--fleet-certificate` flag. | ||
|
||
- 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. | ||
|
||
## 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). | ||
|
||
## Issues with internal and/or intermediates certificates | ||
|
||
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. | ||
|
||
We've seen TLS certificate issues in the following configurations: (for more information see https://github.com/fleetdm/fleet/issues/6085): | ||
- Certificates signed by internal CA/intermediates. | ||
- Certificates issued by Let's Encrypt (that do not serve the fullchain certificate). | ||
|
||
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 | ||
``` | ||
|
||
To troubleshoot issues with certificates you can use `fleetctl debug connection` command, e.g.: | ||
```sh | ||
fleetctl debug connection \ | ||
--fleet-certificate ./your-ca-root.pem \ | ||
https://fleet.example.com | ||
``` | ||
|
||
[0]: We have a Github CI action that runs daily that updates the [certs.pem on the repository](https://github.com/fleetdm/fleet/blob/main/orbit/pkg/packaging/certs.pem) whenever there's a new version of `cacert.pem` in https://curl.se/docs/caextract.html. Such file is embedded into the `fleetctl` executable and used when generating fleetd packages. | ||
[1]: The bundled certificate in fleetd is installed in `/opt/orbit` in macOS/Linux and `C:\Program Files\Orbit` on Windows. By default its name is `certs.pem`, but it will have a different name if the `--fleet-certificate` flag was used when generating the package (`fleetctl package`). | ||
|
||
<meta name="pageOrderInSection" value="1511"> | ||
<meta name="title" value="TLS certificates in fleetd"> | ||
<meta name="description" value="Learn about TLS server certificates in fleetd."> | ||
<meta name="navSection" value="Dig deeper"> |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm deleting this file because it contains outdated docs. |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -279,18 +279,18 @@ func writeOsqueryFlagfile(opt Options, orbitRoot string) error { | |
} | ||
|
||
// Embed the certs file that osquery uses so that we can drop it into our installation packages. | ||
// This file copied from https://github.com/raw/osquery/osquery/master/tools/deployment/certs.pem | ||
// This file is generated and updated by .github/workflows/update-certs.yml. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ℹ️ info: Updated comment to reflect that the certs file is generated and updated by |
||
// | ||
//go:embed certs.pem | ||
var osqueryCerts []byte | ||
var OsqueryCerts []byte | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ℹ️ info: Changed variable name from |
||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ℹ️ info: Updated to use |
||
return fmt.Errorf("write file: %w", err) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we run into an out of range err here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for checking! It seems that at this point we have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah right, that will produce an empty slice if len()==1 |
||
intermediates.AddCert(intermediate) | ||
} | ||
Comment on lines
+60
to
+63
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ℹ️ info: Added support for intermediate certificates by creating a new cert pool and adding all intermediate certificates to it. |
||
|
||
if _, err := cert.Verify(x509.VerifyOptions{ | ||
DNSName: parsed.Hostname(), | ||
Roots: pool, | ||
DNSName: parsed.Hostname(), | ||
Roots: pool, | ||
Intermediates: intermediates, | ||
Comment on lines
+66
to
+68
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ℹ️ info: Included the intermediates cert pool in the verification options to ensure a complete chain of trust. |
||
}); err != nil { | ||
return ctxerr.Wrap(ctx, err, "verify certificate") | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ℹ️ info: Removed comment about system certs not working on Windows. |
||
// Use only the system certs (doesn't work on Windows) | ||
rootCAPool, err = x509.SystemCertPool() | ||
if err != nil { | ||
return nil, fmt.Errorf("loading system cert pool: %w", err) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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