diff --git a/client/internal/certproof/README.md b/client/internal/certproof/README.md index 726bc213b..d99bb171a 100644 --- a/client/internal/certproof/README.md +++ b/client/internal/certproof/README.md @@ -224,6 +224,11 @@ currently open**. Consequences worth designing around: - **Signing out changes the answer.** Posture can flip between compliant and non-compliant across a sign-out, so management should treat "no proof" as its own state rather than as a failed check, or users get disconnected at the sign-in screen. +- **The engine notices the change.** Every minute it checks whether the profile owner's + session came or went, and collects again when it did. A collection that proved nothing + is retried every five minutes, for a keychain unlocked after login or a TPM resource + manager started after the daemon. Losing every proof, and regaining one, is published + as a system event, so the UI and `netbird status` show why access changed. - **Only the profile owner is asked.** The user certificate belongs to whoever owns the active NetBird profile. macOS asks the console user only when that user owns the profile, so a fast-user-switched account never answers for someone else. Windows asks diff --git a/client/internal/certproof/collect.go b/client/internal/certproof/collect.go index 367ec7dc6..65affca71 100644 --- a/client/internal/certproof/collect.go +++ b/client/internal/certproof/collect.go @@ -108,6 +108,11 @@ func CollectChallenges(ctx context.Context, store Store, challenges []*proto.Cer return proofs } +// HasChallenges reports whether any of checks asks for a certificate proof. +func HasChallenges(checks []*proto.Checks) bool { + return len(certificateChallenges(checks)) > 0 +} + func certificateChallenges(checks []*proto.Checks) []*proto.CertificateChallenge { var challenges []*proto.CertificateChallenge for _, check := range checks { diff --git a/client/internal/certproof/collect_darwin.go b/client/internal/certproof/collect_darwin.go index 99019ce5b..28fa1c183 100644 --- a/client/internal/certproof/collect_darwin.go +++ b/client/internal/certproof/collect_darwin.go @@ -45,6 +45,20 @@ func CollectProofs(ctx context.Context, checks []*proto.Checks, peerKey []byte, return mergeProofs(proofs, userProofs) } +// UserContext identifies the user whose keychain a collection would include: the console +// user when it owns the active profile, or empty when no user keychain would be asked. A +// change means a collection made earlier no longer reflects what this Mac can prove. +func UserContext(cfg Config) string { + if os.Geteuid() != 0 { + return "" + } + user, ok := CurrentConsoleUser() + if !ok || !user.isOwner(cfg.ProfileOwner) { + return "" + } + return strconv.FormatUint(uint64(user.UID), 10) + ":" + user.Name +} + // collectAsConsoleUser runs the helper inside the desktop session of the logged-in // user. Dropping to their uid is not enough: keychain access is an XPC call to a // per-session securityd, so the helper has to enter their Mach bootstrap namespace, diff --git a/client/internal/certproof/collect_other.go b/client/internal/certproof/collect_other.go index 852be0680..757566ac3 100644 --- a/client/internal/certproof/collect_other.go +++ b/client/internal/certproof/collect_other.go @@ -17,6 +17,12 @@ func CollectProofs(ctx context.Context, checks []*proto.Checks, peerKey []byte, return Collect(ctx, storeWithToken(cfg), checks, peerKey) } +// UserContext identifies the user whose certificates a collection would include. These +// platforms have no per-user store, so it never changes. +func UserContext(Config) string { + return "" +} + // helperStore is the store the helper reads. Nothing launches a helper on these // platforms, so it is the platform default. func helperStore() Store { diff --git a/client/internal/certproof/collect_windows.go b/client/internal/certproof/collect_windows.go index d3e005276..dcdd2d320 100644 --- a/client/internal/certproof/collect_windows.go +++ b/client/internal/certproof/collect_windows.go @@ -44,6 +44,21 @@ func CollectProofs(ctx context.Context, checks []*proto.Checks, peerKey []byte, return mergeProofs(proofs, userProofs) } +// UserContext identifies the session whose store a collection would include: a session +// of the profile owner, or empty when no user store would be asked. A change means a +// collection made earlier no longer reflects what this machine can prove. +func UserContext(cfg Config) string { + if !runningAsLocalSystem() { + return "" + } + user, ok := CurrentDesktopUser(cfg.ProfileOwner) + if !ok { + return "" + } + defer user.Close() + return fmt.Sprintf("%d:%s", user.Session, user.Name) +} + // helperStore is the store the helper reads. It runs as the signed-in user, so it wants // that user's store rather than the machine store the service already read. func helperStore() Store { diff --git a/client/internal/engine.go b/client/internal/engine.go index fcadbb939..6433849cc 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -276,8 +276,10 @@ type Engine struct { // checks are the client-applied posture checks that need to be evaluated on the client checks []*mgmProto.Checks - // certProofs answers the certificate challenges in checks within a bounded time. + // certProofs answers the certificate challenges in checks within a bounded time, and + // certState remembers what it last proved. certProofs certproof.Collector + certState certPostureState infoSource system.InfoSource @@ -678,6 +680,12 @@ func (e *Engine) Start(netbirdConfig *mgmProto.NetbirdConfig, mgmtURL *url.URL) e.portForwardManager.Start(e.ctx, uint16(e.config.WgPort)) }() + e.shutdownWg.Add(1) + go func(ctx context.Context) { + defer e.shutdownWg.Done() + e.watchCertificatePosture(ctx) + }(e.ctx) + // Set the WireGuard interface for rosenpass after interface is up if e.rpManager != nil { e.rpManager.SetInterface(e.wgInterface) @@ -1258,17 +1266,11 @@ func (e *Engine) updateChecksIfNew(checks []*mgmProto.Checks) error { if isChecksEqual(e.checks, checks) { return nil } - info, ok := e.infoSource.Refresh(e.ctx, systemInfoTimeout, checks, e.overlayAddresses()...) - if !ok { - // Gathering timed out; skip the meta sync this cycle rather than blocking the - // sync loop (and syncMsgMux) on a stuck system call. A later sync will retry. - return nil - } - e.applyInfoFlags(info) - e.attachCertificateProofs(info, checks) - - if err := e.mgmClient.SyncMeta(info); err != nil { - return fmt.Errorf("could not sync meta: error %s", err) + if err := e.syncChecksMeta(checks); err != nil { + if errors.Is(err, errSystemInfoTimeout) { + return nil + } + return err } e.checks = checks return nil @@ -1297,14 +1299,6 @@ func (e *Engine) applyInfoFlags(info *system.Info) { ) } -// attachCertificateProofs answers the certificate challenges in checks with the -// certificates reachable on this device, signing each challenge nonce for our peer key. -// Collection is bounded in time because callers hold the sync loop while it runs. -func (e *Engine) attachCertificateProofs(info *system.Info, checks []*mgmProto.Checks) { - peerKey := e.config.WgPrivateKey.PublicKey() - info.CertificateProofs = e.certProofs.Collect(e.ctx, checks, peerKey[:], e.config.CertStore) -} - func (e *Engine) currentSystemInfo(ctx context.Context) *system.Info { info := e.infoSource.Current(ctx, e.overlayAddresses()...) e.applyInfoFlags(info) diff --git a/client/internal/engine_certposture.go b/client/internal/engine_certposture.go new file mode 100644 index 000000000..dff5fa2ed --- /dev/null +++ b/client/internal/engine_certposture.go @@ -0,0 +1,159 @@ +package internal + +import ( + "context" + "errors" + "fmt" + "sync" + "time" + + log "github.com/sirupsen/logrus" + + "github.com/netbirdio/netbird/client/internal/certproof" + cProto "github.com/netbirdio/netbird/client/proto" + "github.com/netbirdio/netbird/client/system" + mgmProto "github.com/netbirdio/netbird/shared/management/proto" +) + +const ( + // certContextPollInterval is how often the engine checks whether the user who can + // answer certificate challenges has changed, such as a login after an autostart. + certContextPollInterval = time.Minute + // certRetryInterval is how long after a collection that proved nothing it is tried + // again with the same user, for stores that come up late: a keychain unlocked after + // login, or a TPM resource manager started after the daemon. + certRetryInterval = 5 * time.Minute +) + +// errSystemInfoTimeout reports that gathering the system info for a meta sync timed out, +// so the sync was skipped rather than holding syncMsgMux on a stuck system call. +var errSystemInfoTimeout = errors.New("system info gathering timed out") + +// certPostureState remembers what the last certificate proof collection saw, so the +// engine can collect again when it went stale and tell the user when proofs go missing. +type certPostureState struct { + mu sync.Mutex + attempted bool + attemptedAt time.Time + userContext string + proven bool +} + +// record stores the outcome of a collection and reports whether it changed from proven +// to unproven or back. The first collection only counts as a change when it proved nothing. +func (s *certPostureState) record(userContext string, proven bool, now time.Time) (changed bool) { + s.mu.Lock() + defer s.mu.Unlock() + + changed = (s.attempted && s.proven != proven) || (!s.attempted && !proven) + s.attempted = true + s.attemptedAt = now + s.userContext = userContext + s.proven = proven + return changed +} + +// stale reports whether the last collection no longer reflects what the device can +// prove: the user who can answer has changed, or nothing was proven and the retry +// interval has passed. Before the first collection the sync itself collects. +func (s *certPostureState) stale(userContext string, now time.Time) bool { + s.mu.Lock() + defer s.mu.Unlock() + + if !s.attempted { + return false + } + if userContext != s.userContext { + return true + } + return !s.proven && now.Sub(s.attemptedAt) >= certRetryInterval +} + +// attachCertificateProofs answers the certificate challenges in checks with the +// certificates reachable on this device, signing each challenge nonce for our peer key. +// Collection is bounded in time because callers hold the sync loop while it runs. +func (e *Engine) attachCertificateProofs(info *system.Info, checks []*mgmProto.Checks) { + if !certproof.HasChallenges(checks) { + info.CertificateProofs = nil + return + } + userContext := certproof.UserContext(e.config.CertStore) + peerKey := e.config.WgPrivateKey.PublicKey() + info.CertificateProofs = e.certProofs.Collect(e.ctx, checks, peerKey[:], e.config.CertStore) + + proven := len(info.CertificateProofs) > 0 + if e.certState.record(userContext, proven, time.Now()) { + e.publishCertificatePostureEvent(proven) + } +} + +// publishCertificatePostureEvent tells the user when the device stops proving any +// certificate, which management treats as failing every certificate posture check, and +// when it proves one again. Without it the loss of access would have no visible cause. +func (e *Engine) publishCertificatePostureEvent(proven bool) { + if e.statusRecorder == nil { + return + } + if proven { + e.statusRecorder.PublishEvent(cProto.SystemEvent_INFO, cProto.SystemEvent_SYSTEM, + "certificate posture: a certificate is proven again", + "A certificate required by your organization's device policy is available again.", nil) + return + } + e.statusRecorder.PublishEvent(cProto.SystemEvent_WARNING, cProto.SystemEvent_SYSTEM, + "certificate posture: no certificate could be proven", + "NetBird could not use a certificate required by your organization's device policy. "+ + "Access to some resources may be blocked until one is available.", nil) +} + +// watchCertificatePosture collects certificate proofs again when the last collection +// went stale, until ctx is done. Proofs are otherwise only collected when the checks +// change or the sync stream reconnects, so a daemon started before anyone logged in +// would not prove a user certificate until the next network map. +func (e *Engine) watchCertificatePosture(ctx context.Context) { + ticker := time.NewTicker(certContextPollInterval) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + if err := e.recollectCertificateProofsIfStale(); err != nil && !errors.Is(err, errSystemInfoTimeout) { + log.Warnf("failed to refresh certificate posture proofs: %v", err) + } + } + } +} + +func (e *Engine) recollectCertificateProofsIfStale() error { + userContext := certproof.UserContext(e.config.CertStore) + if !e.certState.stale(userContext, time.Now()) { + return nil + } + + e.syncMsgMux.Lock() + defer e.syncMsgMux.Unlock() + if e.ctx.Err() != nil || !certproof.HasChallenges(e.checks) { + return nil + } + log.Debugf("certificate posture: proofs are stale, collecting again") + return e.syncChecksMeta(e.checks) +} + +// syncChecksMeta gathers the system info that checks evaluate, with its certificate +// proofs, and sends it to management. The caller holds syncMsgMux. +func (e *Engine) syncChecksMeta(checks []*mgmProto.Checks) error { + info, ok := e.infoSource.Refresh(e.ctx, systemInfoTimeout, checks, e.overlayAddresses()...) + if !ok { + // Gathering timed out; skip the meta sync this cycle rather than blocking the + // sync loop (and syncMsgMux) on a stuck system call. A later sync will retry. + return errSystemInfoTimeout + } + e.applyInfoFlags(info) + e.attachCertificateProofs(info, checks) + + if err := e.mgmClient.SyncMeta(info); err != nil { + return fmt.Errorf("sync meta: %w", err) + } + return nil +} diff --git a/client/internal/engine_certposture_test.go b/client/internal/engine_certposture_test.go new file mode 100644 index 000000000..9b0e36309 --- /dev/null +++ b/client/internal/engine_certposture_test.go @@ -0,0 +1,104 @@ +//go:build !windows && !darwin + +package internal + +import ( + "context" + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.zx2c4.com/wireguard/wgctrl/wgtypes" + + "github.com/netbirdio/netbird/client/internal/certproof" + "github.com/netbirdio/netbird/client/internal/peer" + cProto "github.com/netbirdio/netbird/client/proto" + "github.com/netbirdio/netbird/client/system" + "github.com/netbirdio/netbird/shared/management/certposture" + "github.com/netbirdio/netbird/shared/management/certposture/certtest" + mgmProto "github.com/netbirdio/netbird/shared/management/proto" +) + +func TestCertPostureState_Record(t *testing.T) { + now := time.Now() + + var proven certPostureState + assert.False(t, proven.record("", true, now), "a first collection that proves something is not news") + + var unproven certPostureState + assert.True(t, unproven.record("", false, now), "a first collection that proves nothing is reported") + assert.False(t, unproven.record("", false, now), "the same outcome again is not reported twice") + assert.True(t, unproven.record("", true, now), "proving again is reported") + assert.True(t, unproven.record("", false, now), "losing the proof again is reported") +} + +func TestCertPostureState_Stale(t *testing.T) { + now := time.Now() + var s certPostureState + + assert.False(t, s.stale("", now), "before any collection the sync itself collects") + + s.record("501:alice", true, now) + assert.False(t, s.stale("501:alice", now.Add(time.Hour)), "a proven collection for the same user stays fresh") + assert.True(t, s.stale("", now), "the user logging out changes what can be proven") + assert.True(t, s.stale("502:bob", now), "another owner session changes what can be proven") + + s.record("", false, now) + assert.False(t, s.stale("", now.Add(certRetryInterval-time.Second)), "an unproven collection is not retried before the interval") + assert.True(t, s.stale("", now.Add(certRetryInterval)), "an unproven collection is retried once the interval passed") +} + +// attachCertificateProofs against a real PEM directory and status recorder: a device that +// cannot prove its certificate gets one warning, and one notice when it can again. +func TestEngine_AttachCertificateProofsReportsLostAndRegainedProofs(t *testing.T) { + dir := t.TempDir() + key, err := wgtypes.GeneratePrivateKey() + require.NoError(t, err) + + recorder := peer.NewRecorder("") + e := &Engine{ + ctx: context.Background(), + config: &EngineConfig{WgPrivateKey: key, CertStore: certproof.Config{Dir: dir}}, + statusRecorder: recorder, + } + + ca := certtest.NewCA(t, "corp-root") + peerKey := key.PublicKey() + nonce := certposture.NewChallenger([]byte("secret")).Nonce(peerKey[:], time.Now()) + checks := []*mgmProto.Checks{{CertificateChallenge: &mgmProto.CertificateChallenge{Nonce: nonce, CaCertificates: []string{ca.PEM}}}} + + warnings := func() int { + n := 0 + for _, ev := range recorder.GetEventHistory() { + if ev.Severity == cProto.SystemEvent_WARNING && ev.Category == cProto.SystemEvent_SYSTEM { + n++ + } + } + return n + } + + info := &system.Info{} + e.attachCertificateProofs(info, checks) + assert.Empty(t, info.CertificateProofs, "no certificate in the store yet") + assert.Equal(t, 1, warnings(), "the user is told the device proves no certificate") + + e.attachCertificateProofs(info, checks) + assert.Equal(t, 1, warnings(), "an unchanged outcome is not reported again") + + deviceKey := certtest.ECDSAKey(t) + pem := certtest.CertPEM(ca.Issue(t, deviceKey, "device")) + certtest.KeyPEM(t, deviceKey) + require.NoError(t, os.WriteFile(filepath.Join(dir, "device.pem"), []byte(pem), 0o600)) + + e.attachCertificateProofs(info, checks) + assert.Len(t, info.CertificateProofs, 1, "the certificate is proven once it is in the store") + events := recorder.GetEventHistory() + require.NotEmpty(t, events) + last := events[len(events)-1] + assert.Equal(t, cProto.SystemEvent_INFO, last.Severity, "regaining the proof is reported as good news") + + e.attachCertificateProofs(&system.Info{}, []*mgmProto.Checks{{Files: []string{"/bin/agent"}}}) + assert.Len(t, recorder.GetEventHistory(), len(events), "checks without a certificate challenge collect and report nothing") +}