mirror of
https://github.com/netbirdio/netbird.git
synced 2026-09-23 23:29:08 +02:00
[client] Keep the mTLS pair off the gate's dry run (review item)
WouldChange runs the real apply() against a throwaway copy, and apply() loads the client mTLS certificate and key from disk whenever the config names them. So every gated SetConfig and Login read the pair — twice per request, once for the normalization pass and once for the verdict — including requests that were about to be refused or that changed nothing, and logged an error per request when the files were missing. The gate used to be presence-based and never called apply(), so this was new work on a request path. The loaded pair feeds the connection and never the comparison: nothing in apply() reads it back, and it does not move the `updated` verdict. A config built only to be compared against now says so, and apply() skips the load for it. Reported by cubic on the PR.
This commit is contained in:
@@ -198,6 +198,11 @@ type Config struct {
|
||||
|
||||
MTU uint16
|
||||
|
||||
// probing marks a config that exists only to be compared against and then
|
||||
// thrown away, so apply() can skip the work that feeds no verdict.
|
||||
// Unexported, so it never reaches the JSON.
|
||||
probing bool
|
||||
|
||||
// policy is the MDM policy that produced the currently-set values
|
||||
// for any MDM-enforced fields. Set by ApplyMDMPolicy on every
|
||||
// invocation. Never persisted to disk. Callers query enforcement
|
||||
@@ -796,7 +801,11 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) {
|
||||
updated = true
|
||||
}
|
||||
|
||||
if config.ClientCertPath != "" && config.ClientCertKeyPath != "" {
|
||||
// Not on a probe: the loaded pair feeds the connection, never the
|
||||
// comparison, and this would otherwise run on every gated SetConfig and
|
||||
// Login — twice per request — including those that are refused or change
|
||||
// nothing, logging an error per request when the files are missing.
|
||||
if !config.probing && config.ClientCertPath != "" && config.ClientCertKeyPath != "" {
|
||||
cert, err := tls.LoadX509KeyPair(config.ClientCertPath, config.ClientCertKeyPath)
|
||||
if err != nil {
|
||||
log.Error("Failed to load mTLS cert/key pair: ", err)
|
||||
@@ -1099,6 +1108,7 @@ func (config *Config) WouldChange(input ConfigInput) (bool, error) {
|
||||
}
|
||||
probe = baseline
|
||||
}
|
||||
probe.probing = true
|
||||
|
||||
// Normalize before measuring. apply() reports two different things through
|
||||
// one bool: an input that changed a value, and a field it had to fill in
|
||||
|
||||
@@ -0,0 +1,96 @@
|
||||
package profilemanager
|
||||
|
||||
import (
|
||||
"crypto/ecdsa"
|
||||
"crypto/elliptic"
|
||||
"crypto/rand"
|
||||
"crypto/x509"
|
||||
"crypto/x509/pkix"
|
||||
"encoding/pem"
|
||||
"math/big"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
// writeCertPair writes a throwaway certificate and key, so apply() has
|
||||
// something real to load rather than a missing file it would only log about.
|
||||
func writeCertPair(t *testing.T) (certPath, keyPath string) {
|
||||
t.Helper()
|
||||
|
||||
key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
|
||||
require.NoError(t, err)
|
||||
|
||||
template := x509.Certificate{
|
||||
SerialNumber: big.NewInt(1),
|
||||
Subject: pkix.Name{CommonName: "probe-test"},
|
||||
NotBefore: time.Now().Add(-time.Hour),
|
||||
NotAfter: time.Now().Add(time.Hour),
|
||||
}
|
||||
der, err := x509.CreateCertificate(rand.Reader, &template, &template, &key.PublicKey, key)
|
||||
require.NoError(t, err)
|
||||
|
||||
keyDER, err := x509.MarshalECPrivateKey(key)
|
||||
require.NoError(t, err)
|
||||
|
||||
dir := t.TempDir()
|
||||
certPath = filepath.Join(dir, "client.crt")
|
||||
keyPath = filepath.Join(dir, "client.key")
|
||||
require.NoError(t, os.WriteFile(certPath, pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der}), 0o600))
|
||||
require.NoError(t, os.WriteFile(keyPath, pem.EncodeToMemory(&pem.Block{Type: "EC PRIVATE KEY", Bytes: keyDER}), 0o600))
|
||||
return certPath, keyPath
|
||||
}
|
||||
|
||||
// The dry run behind the update-settings gate must not read the mTLS pair off
|
||||
// disk. The loaded pair feeds the connection, never the comparison, and the
|
||||
// gate runs it on every SetConfig and Login — twice per request — including the
|
||||
// ones it refuses.
|
||||
func TestProbeDoesNotLoadTheCertificatePair(t *testing.T) {
|
||||
certPath, keyPath := writeCertPair(t)
|
||||
|
||||
t.Run("a real apply loads it", func(t *testing.T) {
|
||||
config := newConfigSkeleton()
|
||||
config.ClientCertPath, config.ClientCertKeyPath = certPath, keyPath
|
||||
|
||||
_, err := config.apply(ConfigInput{})
|
||||
require.NoError(t, err)
|
||||
require.NotNil(t, config.ClientCertKeyPair, "the connection would have no client certificate")
|
||||
})
|
||||
|
||||
t.Run("a probe does not", func(t *testing.T) {
|
||||
config := newConfigSkeleton()
|
||||
config.ClientCertPath, config.ClientCertKeyPath = certPath, keyPath
|
||||
config.probing = true
|
||||
|
||||
_, err := config.apply(ConfigInput{})
|
||||
require.NoError(t, err)
|
||||
require.Nil(t, config.ClientCertKeyPair, "the dry run read the certificate off disk")
|
||||
})
|
||||
|
||||
// And the verdict is the same either way, which is the only thing the gate
|
||||
// asks of the probe.
|
||||
t.Run("the verdict is unaffected", func(t *testing.T) {
|
||||
path := filepath.Join(t.TempDir(), "mtls.json")
|
||||
_, err := UpdateOrCreateConfig(ConfigInput{
|
||||
ConfigPath: path,
|
||||
ManagementURL: DefaultManagementURL,
|
||||
ClientCertPath: certPath,
|
||||
ClientCertKeyPath: keyPath,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
|
||||
stored, err := GetExistingConfig(path)
|
||||
require.NoError(t, err)
|
||||
|
||||
changed, err := stored.WouldChange(ConfigInput{ClientCertPath: certPath, ClientCertKeyPath: keyPath})
|
||||
require.NoError(t, err)
|
||||
require.False(t, changed, "restating the stored certificate paths is not a change")
|
||||
|
||||
changed, err = stored.WouldChange(ConfigInput{ClientCertPath: filepath.Join(t.TempDir(), "other.crt")})
|
||||
require.NoError(t, err)
|
||||
require.True(t, changed, "a different certificate path is a change")
|
||||
})
|
||||
}
|
||||
Reference in New Issue
Block a user