From 9d837c6ae672c93fd23d40dafb615541d4bd6b2e Mon Sep 17 00:00:00 2001 From: Viktor Liu <17948409+lixmal@users.noreply.github.com> Date: Thu, 15 Oct 2026 19:14:26 +0900 Subject: [PATCH] [client, management] Fix the edge cases left open in the certificate posture check (#8174) --- .github/workflows/release.yml | 2 + .goreleaser.yaml | 32 ++++- client/internal/certproof/collect.go | 14 ++- client/internal/certproof/collect_darwin.go | 8 +- client/internal/certproof/collect_test.go | 18 +-- client/internal/certproof/collect_windows.go | 6 +- .../internal/certproof/consoleuser_darwin.go | 4 +- .../certproof/consoleuser_darwin_test.go | 4 +- .../internal/certproof/desktopuser_windows.go | 12 +- .../certproof/desktopuser_windows_test.go | 8 +- client/internal/certproof/helper_spawn.go | 45 ------- .../internal/certproof/helper_spawn_test.go | 27 ---- client/internal/certproof/helper_test.go | 11 +- client/internal/certproof/keychain_darwin.go | 27 ++-- client/internal/certproof/keystore_test.go | 6 +- client/internal/certproof/merge.go | 50 ++++++++ client/internal/certproof/merge_test.go | 54 ++++++++ client/internal/certproof/pkcs11store.go | 51 ++++---- client/internal/certproof/pkcs11store_test.go | 33 +++-- client/internal/certproof/signattempts.go | 24 ++++ .../internal/certproof/signattempts_test.go | 41 +++++++ client/internal/certproof/store.go | 12 +- client/internal/certproof/store_other.go | 6 +- client/internal/certproof/store_other_test.go | 10 +- client/internal/certproof/store_tpm_test.go | 4 +- .../internal/certproof/storefile_unix_test.go | 2 +- .../internal/certproof/systemstore_windows.go | 22 ++-- client/internal/pkcs11/driver_linux.go | 69 ++++++++--- client/internal/pkcs11/driver_linux_test.go | 116 ++++++++++++++++++ client/internal/pkcs11/pkcs11.go | 1 + client/internal/pkcs11/publickey.go | 8 +- client/internal/pkcs11/publickey_test.go | 16 +++ client/internal/pkcs11/uri.go | 57 ++++++++- client/internal/pkcs11/uri_test.go | 56 +++++++++ .../network_map_db/shared_types_test.go | 22 ++++ .../certposture/certposture_test.go | 21 ++++ shared/management/client/grpc_test.go | 24 ++++ shared/management/http/api/openapi.yml | 1 + 38 files changed, 718 insertions(+), 206 deletions(-) create mode 100644 client/internal/certproof/merge.go create mode 100644 client/internal/certproof/merge_test.go create mode 100644 client/internal/certproof/signattempts.go create mode 100644 client/internal/certproof/signattempts_test.go diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index b3ed4763c..22ab7f566 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -417,6 +417,8 @@ jobs: path: | dist/netbird_linux** dist/netbird-pkcs11_linux** + dist/netbird-deb-32bit_linux** + dist/netbird-rpm-*_linux** retention-days: 7 - name: upload windows packages id: upload_windows_packages diff --git a/.goreleaser.yaml b/.goreleaser.yaml index de5f7419c..41c330a64 100644 --- a/.goreleaser.yaml +++ b/.goreleaser.yaml @@ -40,8 +40,9 @@ builds: tags: - load_wgnt_from_rsrc - # deb and rpm packages target glibc distributions, so they carry the PKCS#11 store, which - # links libc. Tarballs and the Alpine-based images keep the static build above. + # deb and rpm packages target glibc distributions, so on amd64 and arm64, the only + # architectures with a PKCS#11 driver, they carry the PKCS#11 store, which links libc. + # Tarballs, the Alpine-based images and the other architectures keep the static build. - id: netbird-pkcs11 dir: client binary: netbird @@ -49,10 +50,8 @@ builds: goos: - linux goarch: - - arm - amd64 - arm64 - - 386 ldflags: - -s -w -X github.com/netbirdio/netbird/version.version={{.Version}} -X main.commit={{.Commit}} -X main.date={{.CommitDate}} -X main.builtBy=goreleaser mod_timestamp: "{{ .CommitTimestamp }}" @@ -60,8 +59,24 @@ builds: - load_wgnt_from_rsrc - pkcs11 - # Single-arch builds: nfpm provides is not templated, so the RPM splits per arch. They - # carry the PKCS#11 store like the deb build above. + # The 32-bit deb architectures, which have no PKCS#11 driver. + - id: netbird-deb-32bit + dir: client + binary: netbird + env: [CGO_ENABLED=0] + goos: + - linux + goarch: + - arm + - 386 + ldflags: + - -s -w -X github.com/netbirdio/netbird/version.version={{.Version}} -X main.commit={{.Commit}} -X main.date={{.CommitDate}} -X main.builtBy=goreleaser + mod_timestamp: "{{ .CommitTimestamp }}" + tags: + - load_wgnt_from_rsrc + + # Single-arch builds: nfpm provides is not templated, so the RPM splits per arch. The + # ones with a PKCS#11 driver carry the store like the deb build above. - &netbird_rpm_build id: netbird-rpm-amd64 dir: client @@ -83,10 +98,14 @@ builds: - <<: *netbird_rpm_build id: netbird-rpm-arm goarch: [arm] + tags: + - load_wgnt_from_rsrc - <<: *netbird_rpm_build id: netbird-rpm-386 goarch: [386] + tags: + - load_wgnt_from_rsrc - id: netbird-static dir: client @@ -323,6 +342,7 @@ nfpms: bindir: /usr/bin builds: - netbird-pkcs11 + - netbird-deb-32bit formats: - deb scripts: diff --git a/client/internal/certproof/collect.go b/client/internal/certproof/collect.go index 65affca71..310fd2b19 100644 --- a/client/internal/certproof/collect.go +++ b/client/internal/certproof/collect.go @@ -149,9 +149,19 @@ func prove(signer crypto.Signer, chain []*x509.Certificate, nonce, peerKey []byt } func chainFingerprint(chain []*x509.Certificate) [sha256.Size]byte { - buf := make([]byte, 0, len(chain)*sha256.Size) + ders := make([][]byte, 0, len(chain)) for _, cert := range chain { - certHash := sha256.Sum256(cert.Raw) + ders = append(ders, cert.Raw) + } + return chainDERFingerprint(ders) +} + +// chainDERFingerprint identifies a chain of DER certificates by the hashes of all of them, +// in order. +func chainDERFingerprint(chain [][]byte) [sha256.Size]byte { + buf := make([]byte, 0, len(chain)*sha256.Size) + for _, der := range chain { + certHash := sha256.Sum256(der) buf = append(buf, certHash[:]...) } return sha256.Sum256(buf) diff --git a/client/internal/certproof/collect_darwin.go b/client/internal/certproof/collect_darwin.go index 92f0471e7..cc21563c1 100644 --- a/client/internal/certproof/collect_darwin.go +++ b/client/internal/certproof/collect_darwin.go @@ -60,7 +60,7 @@ func UserContext(cfg Config) string { if os.Geteuid() != 0 || cfg.OwnerUnknown { return "" } - user, ok := CurrentConsoleUser() + user, ok := currentConsoleUser() if !ok || !user.isOwner(cfg.ProfileOwner) { return "" } @@ -72,7 +72,7 @@ func UserContext(cfg Config) string { // per-session securityd, so the helper has to enter their Mach bootstrap namespace, // which is what launchctl asuser does. func collectAsConsoleUser(ctx context.Context, owner string, challenges []*proto.CertificateChallenge, peerKey []byte) ([]certposture.Proof, error) { - user, ok := CurrentConsoleUser() + user, ok := currentConsoleUser() if !ok { return nil, nil } @@ -126,12 +126,12 @@ func helperStore() Store { home, err := os.UserHomeDir() if err != nil { log.Debugf("certificate posture: no home directory, searching the default keychain list: %v", err) - return NewKeychainStore() + return newKeychainStore() } login := filepath.Join(home, "Library", "Keychains", "login.keychain-db") if _, err := os.Stat(login); err != nil { // Keychains created before macOS 10.12 keep the old file name. login = filepath.Join(home, "Library", "Keychains", "login.keychain") } - return NewKeychainStore(login) + return newKeychainStore(login) } diff --git a/client/internal/certproof/collect_test.go b/client/internal/certproof/collect_test.go index e16ee3668..aa033dea5 100644 --- a/client/internal/certproof/collect_test.go +++ b/client/internal/certproof/collect_test.go @@ -51,7 +51,7 @@ func TestCollect_ProvesOneMatchingCertificatePerChallenge(t *testing.T) { challenge("not a pem"), } - proofs := Collect(context.Background(), NewFileStore(dir), checks, peerKey) + proofs := Collect(context.Background(), newFileStore(dir), checks, peerKey) require.Len(t, proofs, 2) var subjects []string @@ -74,10 +74,10 @@ func TestCollect_NothingToProve(t *testing.T) { store Store checks []*proto.Checks }{ - {"no checks", NewFileStore(dir), nil}, - {"files only", NewFileStore(dir), []*proto.Checks{{Files: []string{"/bin/x"}}}}, - {"challenge without nonce", NewFileStore(dir), []*proto.Checks{{CertificateChallenge: &proto.CertificateChallenge{CaCertificates: []string{ca.PEM}}}}}, - {"missing store dir", NewFileStore(filepath.Join(dir, "missing")), []*proto.Checks{{CertificateChallenge: &proto.CertificateChallenge{Nonce: make([]byte, certposture.NonceSize), CaCertificates: []string{ca.PEM}}}}}, + {"no checks", newFileStore(dir), nil}, + {"files only", newFileStore(dir), []*proto.Checks{{Files: []string{"/bin/x"}}}}, + {"challenge without nonce", newFileStore(dir), []*proto.Checks{{CertificateChallenge: &proto.CertificateChallenge{CaCertificates: []string{ca.PEM}}}}}, + {"missing store dir", newFileStore(filepath.Join(dir, "missing")), []*proto.Checks{{CertificateChallenge: &proto.CertificateChallenge{Nonce: make([]byte, certposture.NonceSize), CaCertificates: []string{ca.PEM}}}}}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -95,7 +95,7 @@ func TestFileStore_ChainWithIntermediate(t *testing.T) { dir := storeDir(t) writeFile(t, dir, "device.pem", certtest.CertPEM(leaf)+certtest.CertPEM(intermediate.Cert)+certtest.KeyPEM(t, key)) - candidates, err := NewFileStore(dir).Candidates(context.Background()) + candidates, err := newFileStore(dir).Candidates(context.Background()) require.NoError(t, err) require.Len(t, candidates, 1) require.Len(t, candidates[0].Chain, 2) @@ -124,7 +124,7 @@ func TestCollectChallenges_RefusesMalformedInput(t *testing.T) { dir := storeDir(t) key := certtest.ECDSAKey(t) writeFile(t, dir, "device.pem", certtest.CertPEM(ca.Issue(t, key, "device"))+certtest.KeyPEM(t, key)) - store := NewFileStore(dir) + store := newFileStore(dir) nonce := certposture.NewChallenger([]byte("secret")).Nonce(peerKey, time.Now()) tests := []struct { @@ -159,7 +159,7 @@ func TestFileStore_SkipsKeyOfAnotherCertificate(t *testing.T) { good := ca.Issue(t, goodKey, "good") writeFile(t, dir, "b-good.pem", certtest.CertPEM(good)+certtest.KeyPEM(t, goodKey)) - candidates, err := NewFileStore(dir).Candidates(context.Background()) + candidates, err := newFileStore(dir).Candidates(context.Background()) require.NoError(t, err) require.Len(t, candidates, 1, "only the certificate whose key matches is a candidate") assert.True(t, good.Equal(candidates[0].Chain[0]), "the matching pair is kept") @@ -167,7 +167,7 @@ func TestFileStore_SkipsKeyOfAnotherCertificate(t *testing.T) { challenger := certposture.NewChallenger([]byte("secret")) now := time.Now() nonce := challenger.Nonce(peerKey, now) - proofs := CollectChallenges(context.Background(), NewFileStore(dir), []*proto.CertificateChallenge{{Nonce: nonce, CaCertificates: []string{ca.PEM}}}, peerKey) + proofs := CollectChallenges(context.Background(), newFileStore(dir), []*proto.CertificateChallenge{{Nonce: nonce, CaCertificates: []string{ca.PEM}}}, peerKey) require.Len(t, proofs, 1) _, err = challenger.Verify(proofs[0], peerKey, now) assert.NoError(t, err, "the proof sent is one management accepts") diff --git a/client/internal/certproof/collect_windows.go b/client/internal/certproof/collect_windows.go index 8e5695539..3b21ec147 100644 --- a/client/internal/certproof/collect_windows.go +++ b/client/internal/certproof/collect_windows.go @@ -51,7 +51,7 @@ func UserContext(cfg Config) string { if !runningAsLocalSystem() || cfg.OwnerUnknown { return "" } - user, ok := CurrentDesktopUser(cfg.ProfileOwner) + user, ok := currentDesktopUser(cfg.ProfileOwner) if !ok { return "" } @@ -62,14 +62,14 @@ func UserContext(cfg Config) string { // 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 { - return NewUserStore() + return newUserStore() } // collectAsDesktopUser runs the helper inside the interactive session of the signed-in // user. Unlike a keychain on macOS, a Windows service can assume a user identity // directly, so the session token goes straight into the child process. func collectAsDesktopUser(ctx context.Context, owner string, challenges []*proto.CertificateChallenge, peerKey []byte) ([]certposture.Proof, error) { - user, ok := CurrentDesktopUser(owner) + user, ok := currentDesktopUser(owner) if !ok { return nil, nil } diff --git a/client/internal/certproof/consoleuser_darwin.go b/client/internal/certproof/consoleuser_darwin.go index a5182c7fd..1045bc014 100644 --- a/client/internal/certproof/consoleuser_darwin.go +++ b/client/internal/certproof/consoleuser_darwin.go @@ -35,10 +35,10 @@ type ConsoleUser struct { GID uint32 } -// CurrentConsoleUser reports the user sitting at the desktop. The second return value is +// currentConsoleUser reports the user sitting at the desktop. The second return value is // false when nobody is: at the login window macOS either reports no console user at all // or attributes the session to root, and neither has a login keychain to offer. -func CurrentConsoleUser() (ConsoleUser, bool) { +func currentConsoleUser() (ConsoleUser, bool) { if err := loadConsoleUser(); err != nil { log.Debugf("console user lookup unavailable: %v", err) return ConsoleUser{}, false diff --git a/client/internal/certproof/consoleuser_darwin_test.go b/client/internal/certproof/consoleuser_darwin_test.go index 9c1aaa22c..051cc4ab0 100644 --- a/client/internal/certproof/consoleuser_darwin_test.go +++ b/client/internal/certproof/consoleuser_darwin_test.go @@ -27,10 +27,10 @@ func TestConsoleUser_OnlyADesktopSessionCanBeValidated(t *testing.T) { } } -// CurrentConsoleUser runs against the real SystemConfiguration framework. A machine with +// currentConsoleUser runs against the real SystemConfiguration framework. A machine with // a desktop open must report a non-root user; a headless runner must report none. func TestCurrentConsoleUser_AgreesWithItself(t *testing.T) { - user, ok := CurrentConsoleUser() + user, ok := currentConsoleUser() if !ok { t.Log("no console user, running headless") return diff --git a/client/internal/certproof/desktopuser_windows.go b/client/internal/certproof/desktopuser_windows.go index 6f50aae4f..0a9d63b36 100644 --- a/client/internal/certproof/desktopuser_windows.go +++ b/client/internal/certproof/desktopuser_windows.go @@ -13,12 +13,8 @@ const ( noActiveSession = 0xFFFFFFFF servicesSession = 0 - // wtsCurrentServer is WTS_CURRENT_SERVER_HANDLE; wtsActive and wtsDisconnected are - // WTSActive and WTSDisconnected of WTS_CONNECTSTATE_CLASS. None is exported by - // x/sys/windows. + // wtsCurrentServer is WTS_CURRENT_SERVER_HANDLE, which x/sys/windows does not export. wtsCurrentServer = windows.Handle(0) - wtsActive = 0 - wtsDisconnected = 4 ) // DesktopUser is an interactive session and the account signed into it. The user's @@ -37,7 +33,7 @@ func (u DesktopUser) Close() { } } -// CurrentDesktopUser returns a token for the session whose certificate store should be +// currentDesktopUser returns a token for the session whose certificate store should be // asked: a session of owner, the account the active profile belongs to, with the // physical console preferred over remote sessions. With no owner only the console user // counts. Picking any signed-in user instead would let whoever else is logged in to a @@ -46,7 +42,7 @@ func (u DesktopUser) Close() { // // Obtaining the token needs SE_TCB_NAME, which the LocalSystem service has and an // ordinary process does not. -func CurrentDesktopUser(owner string) (DesktopUser, bool) { +func currentDesktopUser(owner string) (DesktopUser, bool) { console := windows.WTSGetActiveConsoleSessionId() if owner == "" { return consoleUser(console) @@ -163,7 +159,7 @@ func userSessions(console uint32) ([]uint32, error) { if console != noActiveSession && console != servicesSession { sessions = append(sessions, console) } - for _, state := range []uint32{wtsActive, wtsDisconnected} { + for _, state := range []uint32{windows.WTSActive, windows.WTSDisconnected} { for _, session := range unsafe.Slice(info, count) { if session.SessionID == servicesSession || session.SessionID == console || session.State != state { continue diff --git a/client/internal/certproof/desktopuser_windows_test.go b/client/internal/certproof/desktopuser_windows_test.go index 95d29eb5f..c3c4022e5 100644 --- a/client/internal/certproof/desktopuser_windows_test.go +++ b/client/internal/certproof/desktopuser_windows_test.go @@ -26,10 +26,10 @@ func TestSameAccountName(t *testing.T) { } } -// CurrentDesktopUser against the real session manager: an owner no session belongs to +// currentDesktopUser against the real session manager: an owner no session belongs to // must never yield a session, whoever else is signed in. func TestCurrentDesktopUser_UnknownOwnerHasNoSession(t *testing.T) { - user, ok := CurrentDesktopUser(`NO-SUCH-DOMAIN\no-such-user-netbird`) + user, ok := currentDesktopUser(`NO-SUCH-DOMAIN\no-such-user-netbird`) if ok { user.Close() } @@ -44,14 +44,14 @@ func TestCurrentDesktopUser_FindsTheOwnersSession(t *testing.T) { if owner == "" { t.Skip("set NB_TEST_DESKTOP_OWNER to a signed-in account and run as LocalSystem") } - user, ok := CurrentDesktopUser(owner) + user, ok := currentDesktopUser(owner) require.True(t, ok, "the signed-in owner %s must have a session", owner) defer user.Close() t.Logf("owner %s resolved to session %d as %s", owner, user.Session, user.Name) assert.True(t, sameAccountName(user.Name, owner), "the session found belongs to %s, not %s", owner, user.Name) assert.NotZero(t, user.Session, "session 0 hosts services and never belongs to the owner") - console, consoleOK := CurrentDesktopUser("") + console, consoleOK := currentDesktopUser("") if consoleOK { defer console.Close() t.Logf("without an owner the console user counts: session %d as %s", console.Session, console.Name) diff --git a/client/internal/certproof/helper_spawn.go b/client/internal/certproof/helper_spawn.go index 968cc3679..54ed265ec 100644 --- a/client/internal/certproof/helper_spawn.go +++ b/client/internal/certproof/helper_spawn.go @@ -3,12 +3,6 @@ package certproof import ( - "crypto/sha256" - "crypto/x509" - - log "github.com/sirupsen/logrus" - - "github.com/netbirdio/netbird/shared/management/certposture" "github.com/netbirdio/netbird/shared/management/proto" ) @@ -22,42 +16,3 @@ func helperRequest(challenges []*proto.CertificateChallenge, peerKey []byte) Hel } return req } - -// mergeProofs appends the user session proofs to the device proofs, dropping a leaf that -// both stores hold so the same certificate is proven once. -func mergeProofs(device, user []certposture.Proof) []certposture.Proof { - if len(user) == 0 { - return device - } - - seen := make(map[[sha256.Size]byte]struct{}, len(device)) - for _, proof := range device { - if len(proof.Chain) > 0 { - seen[sha256.Sum256(proof.Chain[0])] = struct{}{} - } - } - - merged := device - for _, proof := range user { - if len(proof.Chain) == 0 { - continue - } - fingerprint := sha256.Sum256(proof.Chain[0]) - if _, done := seen[fingerprint]; done { - continue - } - seen[fingerprint] = struct{}{} - merged = append(merged, proof) - logUserProof(proof) - } - return merged -} - -func logUserProof(proof certposture.Proof) { - leaf, err := x509.ParseCertificate(proof.Chain[0]) - if err != nil { - log.Debugf("certificate posture: user proof carries an unparsable leaf: %v", err) - return - } - log.Debugf("certificate posture: signed-in user proved %q issued by %q", leaf.Subject, leaf.Issuer) -} diff --git a/client/internal/certproof/helper_spawn_test.go b/client/internal/certproof/helper_spawn_test.go index 338a5f66a..74a792a49 100644 --- a/client/internal/certproof/helper_spawn_test.go +++ b/client/internal/certproof/helper_spawn_test.go @@ -3,42 +3,15 @@ package certproof import ( - "crypto/sha256" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/netbirdio/netbird/shared/management/certposture" "github.com/netbirdio/netbird/shared/management/certposture/certtest" "github.com/netbirdio/netbird/shared/management/proto" ) -func TestMergeProofs_ProvesACertificateHeldByBothStoresOnce(t *testing.T) { - ca := certtest.NewCA(t, "corp-root") - shared := ca.Issue(t, certtest.ECDSAKey(t), "shared") - userOnly := ca.Issue(t, certtest.ECDSAKey(t), "user-only") - - device := []certposture.Proof{{Chain: [][]byte{shared.Raw}}} - user := []certposture.Proof{{Chain: [][]byte{shared.Raw}}, {Chain: [][]byte{userOnly.Raw}}, {}} - - merged := mergeProofs(device, user) - - require.Len(t, merged, 2, "the shared leaf is proven once and the empty chain is dropped") - assert.Equal(t, shared.Raw, merged[0].Chain[0], "the device proof keeps its place") - assert.Equal(t, userOnly.Raw, merged[1].Chain[0], "the user-only certificate is appended") -} - -func TestMergeProofs_KeepsDeviceProofsWhenNoUserSession(t *testing.T) { - ca := certtest.NewCA(t, "corp-root") - device := []certposture.Proof{{Chain: [][]byte{ca.Issue(t, certtest.ECDSAKey(t), "device").Raw}}} - - merged := mergeProofs(device, nil) - - require.Len(t, merged, 1, "a machine with nobody signed in still sends its device proof") - assert.Equal(t, sha256.Sum256(device[0].Chain[0]), sha256.Sum256(merged[0].Chain[0]), "the device proof is unchanged") -} - func TestHelperRequest_CarriesEveryChallenge(t *testing.T) { ca := certtest.NewCA(t, "corp-root") challenges := []*proto.CertificateChallenge{ diff --git a/client/internal/certproof/helper_test.go b/client/internal/certproof/helper_test.go index 37dadead2..61fcc8532 100644 --- a/client/internal/certproof/helper_test.go +++ b/client/internal/certproof/helper_test.go @@ -14,7 +14,10 @@ import ( "github.com/netbirdio/netbird/shared/management/certposture/certtest" ) -func TestRunHelper_ProofSurvivesTheProcessBoundary(t *testing.T) { +// TestRunHelper_AnswersAChallengeFromItsStore runs the helper's request and response +// handling in-process: the proof it writes verifies as the daemon will verify it. The +// process boundary itself is covered by the runHelperCmd tests. +func TestRunHelper_AnswersAChallengeFromItsStore(t *testing.T) { ca := certtest.NewCA(t, "corp-root") dir := storeDir(t) key := certtest.ECDSAKey(t) @@ -29,7 +32,7 @@ func TestRunHelper_ProofSurvivesTheProcessBoundary(t *testing.T) { require.NoError(t, err, "request must encode") var stdout bytes.Buffer - require.NoError(t, runHelper(context.Background(), NewFileStore(dir), bytes.NewReader(request), &stdout)) + require.NoError(t, runHelper(context.Background(), newFileStore(dir), bytes.NewReader(request), &stdout)) var resp HelperResponse require.NoError(t, json.Unmarshal(stdout.Bytes(), &resp), "helper must emit decodable JSON") @@ -47,7 +50,7 @@ func TestRunHelper_NoChallengesYieldsEmptyResponse(t *testing.T) { require.NoError(t, err) var stdout bytes.Buffer - require.NoError(t, runHelper(context.Background(), NewFileStore(storeDir(t)), bytes.NewReader(request), &stdout)) + require.NoError(t, runHelper(context.Background(), newFileStore(storeDir(t)), bytes.NewReader(request), &stdout)) var resp HelperResponse require.NoError(t, json.Unmarshal(stdout.Bytes(), &resp), "an empty request must still emit valid JSON") @@ -56,7 +59,7 @@ func TestRunHelper_NoChallengesYieldsEmptyResponse(t *testing.T) { func TestRunHelper_RejectsMalformedRequest(t *testing.T) { var stdout bytes.Buffer - err := runHelper(context.Background(), NewFileStore(storeDir(t)), bytes.NewReader([]byte("not json")), &stdout) + err := runHelper(context.Background(), newFileStore(storeDir(t)), bytes.NewReader([]byte("not json")), &stdout) require.Error(t, err, "a malformed request must fail rather than emit an empty proof set") assert.Empty(t, stdout.String(), "nothing should be written to stdout on a decode failure") diff --git a/client/internal/certproof/keychain_darwin.go b/client/internal/certproof/keychain_darwin.go index 2bffdc81e..5b1426997 100644 --- a/client/internal/certproof/keychain_darwin.go +++ b/client/internal/certproof/keychain_darwin.go @@ -71,25 +71,25 @@ const systemKeychain = "/Library/Keychains/System.keychain" // by an ordinary user, it is that user's keychain search list. func DefaultStore() Store { if os.Geteuid() == 0 { - return NewKeychainStore(systemKeychain) + return newKeychainStore(systemKeychain) } - return NewKeychainStore() + return newKeychainStore() } -// KeychainStore yields the identities of a set of keychains, or of the process's keychain +// keychainStore yields the identities of a set of keychains, or of the process's keychain // search list when none is named, reached through purego so the client keeps building // with CGO_ENABLED=0. -type KeychainStore struct { +type keychainStore struct { keychains []string } -// NewKeychainStore returns a store that searches the keychain files at paths, or the +// newKeychainStore returns a store that searches the keychain files at paths, or the // process's keychain search list when no path is given. -func NewKeychainStore(paths ...string) *KeychainStore { - return &KeychainStore{keychains: paths} +func newKeychainStore(paths ...string) *keychainStore { + return &keychainStore{keychains: paths} } -func (s *KeychainStore) Candidates(_ context.Context) ([]Candidate, error) { +func (s *keychainStore) Candidates(_ context.Context) ([]Candidate, error) { if err := loadKeychain(); err != nil { return nil, err } @@ -163,17 +163,24 @@ func (s *keychainSigner) Sign(_ io.Reader, digest []byte, opts crypto.SignerOpts algorithm := keychainAlgorithm(scheme) var signature []byte + var attempts signAttempts err = eachIdentity(searchList, func(identity uintptr, der []byte) (bool, error) { if !bytes.Equal(der, s.leaf.Raw) { return false, nil } - signature, err = signWithIdentity(identity, algorithm, digest) - return true, err + // The same certificate can sit in the keychain with more than one key, of which + // only one may be usable, so a failure moves on to the next match. + var ok bool + signature, ok = attempts.try(func() ([]byte, error) { return signWithIdentity(identity, algorithm, digest) }) + return ok, nil }) if err != nil { return nil, err } if signature == nil { + if err := attempts.err(); err != nil { + return nil, err + } return nil, errors.New("certificate is no longer in the keychain") } log.Debugf("keychain signed certificate posture challenge for %q, %d bytes", s.leaf.Subject, len(signature)) diff --git a/client/internal/certproof/keystore_test.go b/client/internal/certproof/keystore_test.go index 592d7414f..7d028c4fb 100644 --- a/client/internal/certproof/keystore_test.go +++ b/client/internal/certproof/keystore_test.go @@ -56,6 +56,8 @@ func TestSchemeFor(t *testing.T) { {"ecdsa sha384", ecKey.Public(), crypto.SHA384, schemeECDSASHA384}, {"rsa pss sha256", rsaKey.Public(), pss, schemeRSAPSSSHA256}, {"rsa pkcs1v15", rsaKey.Public(), crypto.SHA256, 0}, + {"ecdsa sha512", ecKey.Public(), crypto.SHA512, 0}, + {"rsa pss sha384", rsaKey.Public(), &rsa.PSSOptions{SaltLength: rsa.PSSSaltLengthEqualsHash, Hash: crypto.SHA384}, 0}, {"ed25519", certtest.Ed25519Key(t).Public(), crypto.Hash(0), 0}, } for _, tc := range tests { @@ -83,5 +85,7 @@ func TestECDSASignatureASN1(t *testing.T) { assert.True(t, ecdsa.VerifyASN1(&key.PublicKey, digest[:], der)) _, err = ecdsaSignatureASN1(raw[:63]) - assert.Error(t, err) + assert.Error(t, err, "an odd length cannot split into r and s") + _, err = ecdsaSignatureASN1(nil) + assert.Error(t, err, "an empty signature is malformed") } diff --git a/client/internal/certproof/merge.go b/client/internal/certproof/merge.go new file mode 100644 index 000000000..d539ce927 --- /dev/null +++ b/client/internal/certproof/merge.go @@ -0,0 +1,50 @@ +package certproof + +import ( + "crypto/sha256" + "crypto/x509" + + log "github.com/sirupsen/logrus" + + "github.com/netbirdio/netbird/shared/management/certposture" +) + +// mergeProofs appends the user session proofs to the device proofs, dropping one whose +// chain the device already proved. Management checks each chain against each check's CAs, +// so the same leaf proven through another chain is kept. +func mergeProofs(device, user []certposture.Proof) []certposture.Proof { + if len(user) == 0 { + return device + } + + seen := make(map[[sha256.Size]byte]struct{}, len(device)+len(user)) + for _, proof := range device { + if len(proof.Chain) > 0 { + seen[chainDERFingerprint(proof.Chain)] = struct{}{} + } + } + + merged := device + for _, proof := range user { + if len(proof.Chain) == 0 { + continue + } + fingerprint := chainDERFingerprint(proof.Chain) + if _, done := seen[fingerprint]; done { + continue + } + seen[fingerprint] = struct{}{} + merged = append(merged, proof) + logUserProof(proof) + } + return merged +} + +func logUserProof(proof certposture.Proof) { + leaf, err := x509.ParseCertificate(proof.Chain[0]) + if err != nil { + log.Debugf("certificate posture: user proof carries an unparsable leaf: %v", err) + return + } + log.Debugf("certificate posture: signed-in user proved %q issued by %q", leaf.Subject, leaf.Issuer) +} diff --git a/client/internal/certproof/merge_test.go b/client/internal/certproof/merge_test.go new file mode 100644 index 000000000..7db2dd421 --- /dev/null +++ b/client/internal/certproof/merge_test.go @@ -0,0 +1,54 @@ +package certproof + +import ( + "crypto/sha256" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/shared/management/certposture" + "github.com/netbirdio/netbird/shared/management/certposture/certtest" +) + +func TestMergeProofs_ProvesACertificateHeldByBothStoresOnce(t *testing.T) { + ca := certtest.NewCA(t, "corp-root") + shared := ca.Issue(t, certtest.ECDSAKey(t), "shared") + userOnly := ca.Issue(t, certtest.ECDSAKey(t), "user-only") + + device := []certposture.Proof{{Chain: [][]byte{shared.Raw}}} + user := []certposture.Proof{{Chain: [][]byte{shared.Raw}}, {Chain: [][]byte{userOnly.Raw}}, {}} + + merged := mergeProofs(device, user) + + require.Len(t, merged, 2, "the shared leaf is proven once and the empty chain is dropped") + assert.Equal(t, shared.Raw, merged[0].Chain[0], "the device proof keeps its place") + assert.Equal(t, userOnly.Raw, merged[1].Chain[0], "the user-only certificate is appended") +} + +func TestMergeProofs_KeepsDeviceProofsWhenNoUserSession(t *testing.T) { + ca := certtest.NewCA(t, "corp-root") + device := []certposture.Proof{{Chain: [][]byte{ca.Issue(t, certtest.ECDSAKey(t), "device").Raw}}} + + merged := mergeProofs(device, nil) + + require.Len(t, merged, 1, "a machine with nobody signed in still sends its device proof") + assert.Equal(t, sha256.Sum256(device[0].Chain[0]), sha256.Sum256(merged[0].Chain[0]), "the device proof is unchanged") +} + +func TestMergeProofs_KeepsAnotherChainOfTheSameLeaf(t *testing.T) { + root := certtest.NewCA(t, "corp-root") + interA := root.Issue(t, certtest.ECDSAKey(t), "inter-a") + interB := root.Issue(t, certtest.ECDSAKey(t), "inter-b") + leaf := root.Issue(t, certtest.ECDSAKey(t), "leaf") + + // Management checks each chain against each check's CAs, so the same leaf proven + // through another chain is a different proof, not a duplicate. + device := []certposture.Proof{{Chain: [][]byte{leaf.Raw, interA.Raw}}} + user := []certposture.Proof{{Chain: [][]byte{leaf.Raw, interB.Raw}}, {Chain: [][]byte{leaf.Raw, interA.Raw}}} + + merged := mergeProofs(device, user) + + require.Len(t, merged, 2, "the chain only the user store holds is kept, the one both hold is not repeated") + assert.Equal(t, interB.Raw, merged[1].Chain[1], "the user's chain through the other intermediate is appended") +} diff --git a/client/internal/certproof/pkcs11store.go b/client/internal/certproof/pkcs11store.go index 1f88e7600..381f55799 100644 --- a/client/internal/certproof/pkcs11store.go +++ b/client/internal/certproof/pkcs11store.go @@ -38,24 +38,24 @@ func PKCS11FromEnv() PKCS11Config { return PKCS11Config{URI: os.Getenv(PKCS11URIEnv), PIN: os.Getenv(PINEnv)} } -// PKCS11Store yields the identities of a PKCS#11 token, which is how tpm2-pkcs11 exposes +// pkcs11Store yields the identities of a PKCS#11 token, which is how tpm2-pkcs11 exposes // TPM-held keys on Linux. Certificates on the token are paired with keys by CKA_ID, the // convention tpm2_ptool addcert and pkcs11-tool follow; certificate files in the PEM // directory by public key. Every signature happens on the token. -type PKCS11Store struct { +type pkcs11Store struct { uri *pkcs11.URI pin string certDir string } -// NewPKCS11Store parses cfg.URI, standing in the bare defaults when it is empty. Files in +// newPKCS11Store parses cfg.URI, standing in the bare defaults when it is empty. Files in // certDir without a key of their own are paired with the token's keys by public key. // // A PIN is only accepted together with a token label: without one the PIN would go to // whichever token the module lists first, which on a machine with a smartcard plugged // in may be the card, and every wrong PIN counts towards that card's lockout. -func NewPKCS11Store(cfg PKCS11Config, certDir string) (*PKCS11Store, error) { - store := &PKCS11Store{uri: &pkcs11.URI{}, pin: cfg.PIN, certDir: certDir} +func newPKCS11Store(cfg PKCS11Config, certDir string) (*pkcs11Store, error) { + store := &pkcs11Store{uri: &pkcs11.URI{}, pin: cfg.PIN, certDir: certDir} if cfg.URI != "" { parsed, err := pkcs11.ParseURI(cfg.URI) if err != nil { @@ -69,7 +69,7 @@ func NewPKCS11Store(cfg PKCS11Config, certDir string) (*PKCS11Store, error) { return store, nil } -func (s *PKCS11Store) Candidates(_ context.Context) ([]Candidate, error) { +func (s *pkcs11Store) Candidates(_ context.Context) ([]Candidate, error) { session, err := s.open() if err != nil { return nil, err @@ -80,7 +80,7 @@ func (s *PKCS11Store) Candidates(_ context.Context) ([]Candidate, error) { if err != nil { return nil, err } - fileChains, err := s.fileChains() + fileChains, fileIntermediates, err := s.fileChains() if err != nil { return nil, err } @@ -93,6 +93,7 @@ func (s *PKCS11Store) Candidates(_ context.Context) ([]Candidate, error) { for _, chain := range fileChains { pool = append(pool, chain...) } + pool = append(pool, fileIntermediates...) var candidates []Candidate for _, cert := range certs { @@ -106,9 +107,12 @@ func (s *PKCS11Store) Candidates(_ context.Context) ([]Candidate, error) { return candidates, nil } + // A token that cannot list its public keys still answers for the certificates it + // holds, so the identities found above are kept. keys, err := tokenPublicKeys(session) if err != nil { - return nil, err + log.Warnf("%s: cannot read its public keys, certificate files are not paired with them: %v", s, err) + return candidates, nil } for _, chain := range fileChains { leaf := chain[0] @@ -122,41 +126,45 @@ func (s *PKCS11Store) Candidates(_ context.Context) ([]Candidate, error) { return candidates, nil } -func (s *PKCS11Store) candidate(leaf *x509.Certificate, id []byte, pool []*x509.Certificate) Candidate { +func (s *pkcs11Store) candidate(leaf *x509.Certificate, id []byte, pool []*x509.Certificate) Candidate { chain := buildChain(leaf, pool) log.Debugf("%s candidate %q issued by %q built a chain of %d certificates", s, leaf.Subject, leaf.Issuer, len(chain)) return Candidate{Chain: chain, Signer: &pkcs11Signer{store: s, leaf: leaf, id: id}, Intermediates: pool} } // fileChains reads the certificate files in the PEM directory that carry no key of their -// own; the file store answers for the ones that do. -func (s *PKCS11Store) fileChains() ([][]*x509.Certificate, error) { +// own; the file store answers for the ones that do. The certificates of a file that does +// carry a key are returned too, since they can complete the chain of a token certificate. +func (s *pkcs11Store) fileChains() (chains [][]*x509.Certificate, intermediates []*x509.Certificate, err error) { if s.certDir == "" { - return nil, nil + return nil, nil, nil } paths, err := certFiles(s.certDir) if err != nil { - return nil, err + return nil, nil, err } - var chains [][]*x509.Certificate for _, path := range paths { chain, signer, err := loadPEM(path) - if err != nil || signer != nil { + if err != nil { + continue + } + if signer != nil { + intermediates = append(intermediates, chain...) continue } chains = append(chains, chain) } - return chains, nil + return chains, intermediates, nil } -func (s *PKCS11Store) String() string { +func (s *pkcs11Store) String() string { if s.uri.Token == "" { return "PKCS#11 token" } return fmt.Sprintf("PKCS#11 token %q", s.uri.Token) } -func (s *PKCS11Store) open() (*pkcs11.Session, error) { +func (s *pkcs11Store) open() (*pkcs11.Session, error) { module, err := pkcs11.Load(s.uri.Module()) if err != nil { return nil, err @@ -185,7 +193,7 @@ func (s *PKCS11Store) open() (*pkcs11.Session, error) { return session, err } -func (s *PKCS11Store) userPIN() ([]byte, error) { +func (s *pkcs11Store) userPIN() ([]byte, error) { if s.pin != "" { return []byte(s.pin), nil } @@ -208,7 +216,8 @@ func tokenPublicKeys(session *pkcs11.Session) (tokenKeys, error) { for _, object := range objects { id, err := session.Attribute(object, pkcs11.AttrID) if err != nil { - return nil, err + log.Debugf("skipping public key on PKCS#11 token: %v", err) + continue } public, err := session.PublicKey(object) if err != nil { @@ -287,7 +296,7 @@ func privateKey(session *pkcs11.Session, id []byte) (pkcs11.Object, error) { // pkcs11Signer holds only the certificate and its CKA_ID; the key is looked up in a fresh // session at signing time so no token handle outlives a call. type pkcs11Signer struct { - store *PKCS11Store + store *pkcs11Store leaf *x509.Certificate id []byte } diff --git a/client/internal/certproof/pkcs11store_test.go b/client/internal/certproof/pkcs11store_test.go index 485c84aae..746b54918 100644 --- a/client/internal/certproof/pkcs11store_test.go +++ b/client/internal/certproof/pkcs11store_test.go @@ -44,7 +44,7 @@ func TestStores_KeepsFileCertificatesWhenTokenFails(t *testing.T) { dir := storeDir(t) writeFile(t, dir, "device.pem", certtest.CertPEM(ca.Issue(t, key, "device"))+certtest.KeyPEM(t, key)) - candidates, err := Stores{failingStore{}, NewFileStore(dir)}.Candidates(context.Background()) + candidates, err := Stores{failingStore{}, newFileStore(dir)}.Candidates(context.Background()) require.NoError(t, err) assert.Len(t, candidates, 1, "the directory's certificate must survive a failing token") } @@ -241,13 +241,13 @@ func attr(typ uint, value []byte) pkcs11.Attribute { // pkcs11TestStore builds the store for the token NB_TEST_PKCS11_URI names, skipping when // no token is configured or this build lacks PKCS#11 support. -func pkcs11TestStore(t *testing.T, certDir string) (*PKCS11Store, string) { +func pkcs11TestStore(t *testing.T, certDir string) (*pkcs11Store, string) { t.Helper() uri := os.Getenv(testPKCS11URIEnv) if uri == "" { t.Skipf("set %s to a PKCS#11 URI with a PIN to run", testPKCS11URIEnv) } - store, err := NewPKCS11Store(PKCS11Config{URI: uri}, certDir) + store, err := newPKCS11Store(PKCS11Config{URI: uri}, certDir) require.NoError(t, err) if _, err := pkcs11.Load(store.uri.Module()); errors.Is(err, pkcs11.ErrUnsupported) { t.Skip(err) @@ -297,17 +297,30 @@ func TestPKCS11Store_FileChains(t *testing.T) { writeFile(t, dir, "sibling.key", certtest.KeyPEM(t, keyed)) writeFile(t, dir, "notes.txt", "not a certificate") - chains, err := (&PKCS11Store{uri: &pkcs11.URI{}, certDir: dir}).fileChains() + // An intermediate that only a keyed file carries still has to reach the pool a token + // certificate's chain is built from. + root := certtest.NewCA(t, "corp-root") + intermediateCA := certtest.NewIntermediate(t, root, "corp-intermediate") + chained := certtest.ECDSAKey(t) + writeFile(t, dir, "chained.pem", certtest.CertPEM(intermediateCA.Issue(t, chained, "chained"))+certtest.CertPEM(intermediateCA.Cert)+certtest.KeyPEM(t, chained)) + + chains, extra, err := (&pkcs11Store{uri: &pkcs11.URI{}, certDir: dir}).fileChains() require.NoError(t, err) + require.NotEmpty(t, extra, "the keyed file's certificates are offered for chain building") + tokenLeaf := intermediateCA.Issue(t, certtest.ECDSAKey(t), "token-leaf") + chain := buildChain(tokenLeaf, extra) + require.Len(t, chain, 2, "a token certificate is completed through the intermediate only the keyed file holds") + assert.True(t, intermediateCA.Cert.Equal(chain[1]), "the second certificate is that intermediate") var subjects []string for _, chain := range chains { subjects = append(subjects, chain[0].Subject.CommonName) } assert.ElementsMatch(t, []string{"device", "corp"}, subjects, "only key-less certificate files are left to the token") - chains, err = (&PKCS11Store{uri: &pkcs11.URI{}}).fileChains() + chains, extra, err = (&pkcs11Store{uri: &pkcs11.URI{}}).fileChains() require.NoError(t, err) assert.Empty(t, chains, "no directory configured means no file certificates") + assert.Empty(t, extra, "and no extra intermediates") } func TestNewPKCS11Store_PIN(t *testing.T) { @@ -324,7 +337,7 @@ func TestNewPKCS11Store_PIN(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - store, err := NewPKCS11Store(tt.cfg, "") + store, err := newPKCS11Store(tt.cfg, "") require.NoError(t, err) pin, err := store.userPIN() require.NoError(t, err) @@ -333,7 +346,7 @@ func TestNewPKCS11Store_PIN(t *testing.T) { }) } - _, err := NewPKCS11Store(PKCS11Config{URI: "not-a-pkcs11-uri", PIN: "1234"}, "") + _, err := newPKCS11Store(PKCS11Config{URI: "not-a-pkcs11-uri", PIN: "1234"}, "") assert.Error(t, err, "a malformed URI must not be silently replaced by the defaults") for name, cfg := range map[string]PKCS11Config{ @@ -342,7 +355,7 @@ func TestNewPKCS11Store_PIN(t *testing.T) { "inline pin-value only": {URI: "pkcs11:?pin-value=0000"}, "pin-source only": {URI: "pkcs11:?pin-source=file:/etc/netbird/pkcs11.pin"}, } { - _, err := NewPKCS11Store(cfg, "") + _, err := newPKCS11Store(cfg, "") assert.ErrorIs(t, err, errPINNeedsToken, "%s: a PIN must not go to whichever token is listed first", name) } } @@ -360,7 +373,7 @@ func TestPKCS11Store_WrongPINIsTriedOnce(t *testing.T) { _, uri := pkcs11TestStore(t, "") wrongPIN := "wrong-pin-" + t.Name() - store, err := NewPKCS11Store(PKCS11Config{URI: uri, PIN: wrongPIN}, "") + store, err := newPKCS11Store(PKCS11Config{URI: uri, PIN: wrongPIN}, "") require.NoError(t, err) _, err = store.Candidates(context.Background()) @@ -370,7 +383,7 @@ func TestPKCS11Store_WrongPINIsTriedOnce(t *testing.T) { _, err = store.Candidates(context.Background()) assert.ErrorIs(t, err, errPINRejectedBefore, "the rejected PIN is not sent to the token again") - good, err := NewPKCS11Store(PKCS11Config{URI: uri}, "") + good, err := newPKCS11Store(PKCS11Config{URI: uri}, "") require.NoError(t, err) _, err = good.Candidates(context.Background()) assert.NoError(t, err, "the correct PIN for the same token is unaffected") diff --git a/client/internal/certproof/signattempts.go b/client/internal/certproof/signattempts.go new file mode 100644 index 000000000..b056991bf --- /dev/null +++ b/client/internal/certproof/signattempts.go @@ -0,0 +1,24 @@ +package certproof + +import "errors" + +// signAttempts tracks the identities tried to sign with, for a certificate that can match +// more than one of them. +type signAttempts struct { + errs []error +} + +// try runs sign and reports its signature and whether it succeeded. A failure is kept. +func (a *signAttempts) try(sign func() ([]byte, error)) ([]byte, bool) { + signature, err := sign() + if err != nil { + a.errs = append(a.errs, err) + return nil, false + } + return signature, true +} + +// err joins the failures of the attempts, nil when there were none. +func (a *signAttempts) err() error { + return errors.Join(a.errs...) +} diff --git a/client/internal/certproof/signattempts_test.go b/client/internal/certproof/signattempts_test.go new file mode 100644 index 000000000..f5a2fef4f --- /dev/null +++ b/client/internal/certproof/signattempts_test.go @@ -0,0 +1,41 @@ +package certproof + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +var errNeedsApprovalForTest = errors.New("key needs approval") + +// TestSignAttempts_FallsThroughToTheNextIdentity: a certificate can sit in a keychain with +// more than one key, of which only some can sign. A failure moves on, and an identity that +// signs ends the search without reporting the earlier failure. +func TestSignAttempts_FallsThroughToTheNextIdentity(t *testing.T) { + var attempts signAttempts + + signature, ok := attempts.try(func() ([]byte, error) { return nil, errNeedsApprovalForTest }) + assert.False(t, ok, "a failing identity does not end the search") + assert.Nil(t, signature) + + signature, ok = attempts.try(func() ([]byte, error) { return []byte("sig"), nil }) + require.True(t, ok, "the next identity signs") + assert.Equal(t, []byte("sig"), signature) +} + +// TestSignAttempts_KeepsEveryFailure: when no identity signs, the approval a user has to +// give must not be hidden behind the failure of a later identity. +func TestSignAttempts_KeepsEveryFailure(t *testing.T) { + var attempts signAttempts + assert.NoError(t, attempts.err(), "no attempts, no error") + + attempts.try(func() ([]byte, error) { return nil, errNeedsApprovalForTest }) + attempts.try(func() ([]byte, error) { return nil, errors.New("key is gone") }) + + err := attempts.err() + require.Error(t, err) + assert.ErrorIs(t, err, errNeedsApprovalForTest, "the earlier approval error survives the later failure") + assert.ErrorContains(t, err, "key is gone") +} diff --git a/client/internal/certproof/store.go b/client/internal/certproof/store.go index 295d6caa4..85ab7cda7 100644 --- a/client/internal/certproof/store.go +++ b/client/internal/certproof/store.go @@ -46,7 +46,7 @@ func (c Candidate) issuers() []*x509.Certificate { return append(slices.Clip(c.Chain[1:]), c.Intermediates...) } -// Store yields the certificates a peer may prove possession of. FileStore is the PEM +// Store yields the certificates a peer may prove possession of. fileStore is the PEM // directory implementation; OS keystores (CNG, Keychain, PKCS#11) slot in here. type Store interface { Candidates(ctx context.Context) ([]Candidate, error) @@ -68,15 +68,15 @@ type Config struct { OwnerUnknown bool } -// FileStore reads PEM files from a directory. A file holds the chain (leaf first) and +// fileStore reads PEM files from a directory. A file holds the chain (leaf first) and // either its private key or a sibling ".key" file holds it. The key is a plain // PKCS#8, EC or RSA key, or a TSS2 key the TPM signs with. -type FileStore struct { +type fileStore struct { dir string } -func NewFileStore(dir string) *FileStore { - return &FileStore{dir: dir} +func newFileStore(dir string) *fileStore { + return &fileStore{dir: dir} } func StoreDir() string { @@ -86,7 +86,7 @@ func StoreDir() string { return defaultStoreDir } -func (s *FileStore) Candidates(_ context.Context) ([]Candidate, error) { +func (s *fileStore) Candidates(_ context.Context) ([]Candidate, error) { paths, err := certFiles(s.dir) if err != nil { return nil, err diff --git a/client/internal/certproof/store_other.go b/client/internal/certproof/store_other.go index e6646732b..eeecf3c39 100644 --- a/client/internal/certproof/store_other.go +++ b/client/internal/certproof/store_other.go @@ -14,13 +14,13 @@ var unsupportedTokenOnce sync.Once // DefaultStore is the PEM directory named by NB_CERT_STORE_DIR, or /etc/netbird/certs. func DefaultStore() Store { - return NewFileStore(StoreDir()) + return newFileStore(StoreDir()) } // storeWithToken reads the PEM directory cfg names, joined by the PKCS#11 token when cfg // names one. The token pairs the directory's key-less certificates with its own keys. func storeWithToken(cfg Config) Store { - files := NewFileStore(StoreDir()) + files := newFileStore(StoreDir()) if cfg.PKCS11.URI == "" && cfg.PKCS11.PIN == "" { return files } @@ -30,7 +30,7 @@ func storeWithToken(cfg Config) Store { }) return files } - token, err := NewPKCS11Store(cfg.PKCS11, StoreDir()) + token, err := newPKCS11Store(cfg.PKCS11, StoreDir()) if err != nil { log.Warnf("ignoring PKCS#11 URI: %v", err) return files diff --git a/client/internal/certproof/store_other_test.go b/client/internal/certproof/store_other_test.go index 47b05f058..68b4df2a6 100644 --- a/client/internal/certproof/store_other_test.go +++ b/client/internal/certproof/store_other_test.go @@ -15,12 +15,12 @@ func TestStoreWithToken(t *testing.T) { dir := storeDir(t) t.Setenv(StoreDirEnv, dir) - files, ok := storeWithToken(Config{}).(*FileStore) + files, ok := storeWithToken(Config{}).(*fileStore) require.True(t, ok, "no token configured reads the PEM directory alone") assert.Equal(t, dir, files.dir, "the directory comes from NB_CERT_STORE_DIR") - assert.IsType(t, &FileStore{}, storeWithToken(Config{PKCS11: PKCS11Config{URI: "not-a-pkcs11-uri"}}), "an invalid URI must not hide the PEM directory") - assert.IsType(t, &FileStore{}, storeWithToken(Config{PKCS11: PKCS11Config{PIN: "1234"}}), "a PIN naming no token is refused and leaves the PEM directory") + assert.IsType(t, &fileStore{}, storeWithToken(Config{PKCS11: PKCS11Config{URI: "not-a-pkcs11-uri"}}), "an invalid URI must not hide the PEM directory") + assert.IsType(t, &fileStore{}, storeWithToken(Config{PKCS11: PKCS11Config{PIN: "1234"}}), "a PIN naming no token is refused and leaves the PEM directory") configured := map[string]PKCS11Config{ "env pin with token uri": {URI: "pkcs11:token=netbird", PIN: "1234"}, @@ -28,7 +28,7 @@ func TestStoreWithToken(t *testing.T) { } if !pkcs11.Supported() { for name, cfg := range configured { - assert.IsType(t, &FileStore{}, storeWithToken(Config{PKCS11: cfg}), + assert.IsType(t, &fileStore{}, storeWithToken(Config{PKCS11: cfg}), "%s: a build without PKCS#11 support reads the PEM directory alone", name) } return @@ -37,7 +37,7 @@ func TestStoreWithToken(t *testing.T) { store, ok := storeWithToken(Config{PKCS11: cfg}).(Stores) require.True(t, ok, "%s joins the token to the PEM directory", name) require.Len(t, store, 2, name) - token, ok := store[1].(*PKCS11Store) + token, ok := store[1].(*pkcs11Store) require.True(t, ok, name) assert.Equal(t, dir, token.certDir, "%s: the token pairs certificates from the same directory", name) } diff --git a/client/internal/certproof/store_tpm_test.go b/client/internal/certproof/store_tpm_test.go index f8a7a0960..cce570891 100644 --- a/client/internal/certproof/store_tpm_test.go +++ b/client/internal/certproof/store_tpm_test.go @@ -36,7 +36,7 @@ func TestFileStore_TPMKeyFile(t *testing.T) { writeFile(t, dir, "locked.pem", certtest.CertPEM(ca.Issue(t, locked, "locked"))) writeFile(t, dir, "locked.key", tpmtest.KeyPEM(t, locked.Public().(*ecdsa.PublicKey), tpmtest.WithAuth())) - candidates, err := NewFileStore(dir).Candidates(context.Background()) + candidates, err := newFileStore(dir).Candidates(context.Background()) require.NoError(t, err) require.Len(t, candidates, 1, "only the key without an authorization value is usable") assert.True(t, leaf.Equal(candidates[0].Chain[0]), "candidate must carry the device certificate") @@ -73,7 +73,7 @@ func TestCollect_TPMKeyEndToEnd(t *testing.T) { nonce := challenger.Nonce(peerKey, now) checks := []*proto.Checks{{CertificateChallenge: &proto.CertificateChallenge{Nonce: nonce, CaCertificates: []string{ca.PEM}}}} - proofs := Collect(context.Background(), NewFileStore(dir), checks, peerKey) + proofs := Collect(context.Background(), newFileStore(dir), checks, peerKey) require.Len(t, proofs, 1, "the TPM-held key must prove the certificate") chain, err := challenger.Verify(proofs[0], peerKey, now) require.NoError(t, err) diff --git a/client/internal/certproof/storefile_unix_test.go b/client/internal/certproof/storefile_unix_test.go index d35a4e759..9932388b6 100644 --- a/client/internal/certproof/storefile_unix_test.go +++ b/client/internal/certproof/storefile_unix_test.go @@ -26,7 +26,7 @@ func devicePair(t *testing.T, dir string) { func candidates(t *testing.T, dir string) ([]Candidate, error) { t.Helper() - return NewFileStore(dir).Candidates(context.Background()) + return newFileStore(dir).Candidates(context.Background()) } func TestFileStore_AcceptsAPrivatePair(t *testing.T) { diff --git a/client/internal/certproof/systemstore_windows.go b/client/internal/certproof/systemstore_windows.go index 92aa03d4b..908899107 100644 --- a/client/internal/certproof/systemstore_windows.go +++ b/client/internal/certproof/systemstore_windows.go @@ -49,10 +49,10 @@ type bcryptPSSPaddingInfo struct { // DefaultStore is the local machine's personal certificate store, where device // certificates enrolled through AD or Intune are kept. func DefaultStore() Store { - return NewSystemStore() + return newSystemStore() } -// SystemStore yields the identities of a personal certificate store, completing their +// systemStore yields the identities of a personal certificate store, completing their // chains from the matching intermediate CA store. Keys are used through CNG and never // exported. // @@ -60,22 +60,22 @@ func DefaultStore() Store { // a service reads; the current user store lives in the signed-in user's registry hive // with keys protected against their profile, so it is only readable while running as // that user. -type SystemStore struct { +type systemStore struct { location uint32 } -// NewSystemStore reads the local machine store, which is what the daemon uses. -func NewSystemStore() *SystemStore { - return &SystemStore{location: windows.CERT_SYSTEM_STORE_LOCAL_MACHINE} +// newSystemStore reads the local machine store, which is what the daemon uses. +func newSystemStore() *systemStore { + return &systemStore{location: windows.CERT_SYSTEM_STORE_LOCAL_MACHINE} } -// NewUserStore reads the calling user's personal store. It is only useful in a process +// newUserStore reads the calling user's personal store. It is only useful in a process // already running as that user, which is what the posture helper is. -func NewUserStore() *SystemStore { - return &SystemStore{location: windows.CERT_SYSTEM_STORE_CURRENT_USER} +func newUserStore() *systemStore { + return &systemStore{location: windows.CERT_SYSTEM_STORE_CURRENT_USER} } -func (s *SystemStore) Candidates(_ context.Context) ([]Candidate, error) { +func (s *systemStore) Candidates(_ context.Context) ([]Candidate, error) { leaves, err := storeCertificates(s.location, personalStore) if err != nil { return nil, err @@ -100,7 +100,7 @@ func (s *SystemStore) Candidates(_ context.Context) ([]Candidate, error) { } // String names the store location the way the Windows documentation does. -func (s *SystemStore) String() string { +func (s *systemStore) String() string { if s.location == windows.CERT_SYSTEM_STORE_CURRENT_USER { return "CurrentUser" } diff --git a/client/internal/pkcs11/driver_linux.go b/client/internal/pkcs11/driver_linux.go index 93bf4a5ba..54a3863cb 100644 --- a/client/internal/pkcs11/driver_linux.go +++ b/client/internal/pkcs11/driver_linux.go @@ -32,6 +32,7 @@ const ( // signature is 2 KiB. maxAttributeLen = 1 << 20 maxSignatureLen = 1 << 16 + maxFoundObjects = 1 << 16 ) type version struct { @@ -103,6 +104,15 @@ const ( fnSign = 43 ) +// checkFunctionListVersion refuses a module whose function list is older than the layout +// functionList mirrors, which would otherwise be read past its end. +func checkFunctionListVersion(v version) error { + if v.major < 2 { + return fmt.Errorf("PKCS#11 function list version %d.%d is older than 2.0", v.major, v.minor) + } + return nil +} + // module holds the entry points of one loaded library, bound straight from its // CK_FUNCTION_LIST. type module struct { @@ -152,6 +162,10 @@ func load(path string) (_ driver, err error) { return nil, Error{Op: "C_GetFunctionList", Code: uint(rv)} } + if err := checkFunctionListVersion(list.version); err != nil { + return nil, fmt.Errorf("%s: %w", path, err) + } + m := &module{} for _, entry := range []struct { fn any @@ -185,21 +199,43 @@ func load(path string) (_ driver, err error) { return m, nil } +// slotListAttempts bounds how often the slot list is asked for again after it grew between +// the size query and the read. +const slotListAttempts = 3 + +// slotList returns the slots with a token present. A token plugged in between the size +// query and the read makes the module answer CKR_BUFFER_TOO_SMALL, in which case the list +// is asked for again. +func (m *module) slotList() ([]ulong, error) { + for range slotListAttempts { + var count ulong + if rv := m.cGetSlotList(1, nil, &count); rv != rvOK { + return nil, Error{Op: "C_GetSlotList", Code: uint(rv)} + } + if count == 0 { + return nil, nil + } + slots := make([]ulong, count) + rv := m.cGetSlotList(1, &slots[0], &count) + if rv == rvBufferTooSmall { + continue + } + if rv != rvOK { + return nil, Error{Op: "C_GetSlotList", Code: uint(rv)} + } + return slots[:count], nil + } + return nil, Error{Op: "C_GetSlotList", Code: rvBufferTooSmall} +} + func (m *module) tokens() ([]Token, error) { - var count ulong - if rv := m.cGetSlotList(1, nil, &count); rv != rvOK { - return nil, Error{Op: "C_GetSlotList", Code: uint(rv)} - } - if count == 0 { - return nil, nil - } - slots := make([]ulong, count) - if rv := m.cGetSlotList(1, &slots[0], &count); rv != rvOK { - return nil, Error{Op: "C_GetSlotList", Code: uint(rv)} + slots, err := m.slotList() + if err != nil { + return nil, err } - tokens := make([]Token, 0, count) - for _, slot := range slots[:count] { + tokens := make([]Token, 0, len(slots)) + for _, slot := range slots { var info tokenInfo if rv := m.cGetTokenInfo(slot, &info); rv != rvOK { continue @@ -262,11 +298,16 @@ func (m *module) findObjects(session uint, template []Attribute) ([]Object, erro if count > findBatch { return nil, fmt.Errorf("C_FindObjects reported %d handles for a batch of %d", count, findBatch) } + // Only an empty batch ends the search: a module may return fewer handles than + // asked for before it has returned them all. + if count == 0 { + return objects, nil + } for _, handle := range batch[:count] { objects = append(objects, Object(handle)) } - if count < findBatch { - return objects, nil + if len(objects) > maxFoundObjects { + return nil, fmt.Errorf("C_FindObjects returned more than %d objects", maxFoundObjects) } } } diff --git a/client/internal/pkcs11/driver_linux_test.go b/client/internal/pkcs11/driver_linux_test.go index 408a3fabc..2362ee18f 100644 --- a/client/internal/pkcs11/driver_linux_test.go +++ b/client/internal/pkcs11/driver_linux_test.go @@ -70,3 +70,119 @@ func loadFirst(t *testing.T, paths ...string) *Module { t.Skip("p11-kit trust module not installed") return nil } + +// TestSlotList_RetriesWhenTheListGrows: a token plugged in between the size query and the +// fill makes the module answer CKR_BUFFER_TOO_SMALL; the list has to be asked for again. +func TestSlotList_RetriesWhenTheListGrows(t *testing.T) { + calls := 0 + m := &module{cGetSlotList: func(_ byte, slots *ulong, count *ulong) ulong { + calls++ + switch { + case slots == nil && calls == 1: + *count = 1 + case slots != nil && calls == 2: + *count = 2 + return rvBufferTooSmall + case slots == nil: + *count = 2 + default: + *count = 2 + backing := unsafe.Slice(slots, 2) + backing[0], backing[1] = 7, 9 + } + return rvOK + }} + + slots, err := m.slotList() + + require.NoError(t, err) + assert.Equal(t, []ulong{7, 9}, slots, "the list is read again at its new size") +} + +func TestSlotList_GivesUpOnAListThatKeepsGrowing(t *testing.T) { + m := &module{cGetSlotList: func(_ byte, slots *ulong, count *ulong) ulong { + if slots == nil { + *count = 1 + return rvOK + } + return rvBufferTooSmall + }} + + _, err := m.slotList() + + assert.Error(t, err, "a module that never settles on a size is an error, not a loop") +} + +func TestCheckFunctionListVersion(t *testing.T) { + assert.NoError(t, checkFunctionListVersion(version{major: 2, minor: 40})) + assert.NoError(t, checkFunctionListVersion(version{major: 3, minor: 0})) + assert.Error(t, checkFunctionListVersion(version{major: 1, minor: 0}), "a pre-2.0 list has no entry points to index") + assert.Error(t, checkFunctionListVersion(version{}), "an empty version is not a function list") +} + +// fakeFindModule answers C_FindObjects from batches, a handle per entry, and reports how +// the search was torn down. +func fakeFindModule(batches [][]ulong, finalized *int) *module { + next := 0 + return &module{ + cFindObjectsInit: func(ulong, *attribute, ulong) ulong { return rvOK }, + cFindObjectsFinal: func(ulong) ulong { *finalized++; return rvOK }, + cFindObjects: func(_ ulong, objects *ulong, max ulong, count *ulong) ulong { + if next >= len(batches) { + *count = 0 + return rvOK + } + batch := batches[next] + next++ + copy(unsafe.Slice(objects, max), batch) + *count = ulong(len(batch)) + return rvOK + }, + } +} + +var findTemplate = []Attribute{{Type: AttrClass, Value: ULong(ClassPrivateKey)}} + +// TestFindObjects_ReadsPastAShortBatch: PKCS#11 ends a search with an empty batch, not with +// one shorter than asked for, so a module that hands out its handles in small batches must +// still have all of them returned. +func TestFindObjects_ReadsPastAShortBatch(t *testing.T) { + finalized := 0 + m := fakeFindModule([][]ulong{{1, 2}, {3}, {4, 5, 6}}, &finalized) + + objects, err := m.findObjects(1, findTemplate) + + require.NoError(t, err) + assert.Equal(t, []Object{1, 2, 3, 4, 5, 6}, objects, "every batch is read until the module returns an empty one") + assert.Equal(t, 1, finalized, "the search is finished once") +} + +func TestFindObjects_StopsAtAnEmptyBatch(t *testing.T) { + finalized := 0 + m := fakeFindModule(nil, &finalized) + + objects, err := m.findObjects(1, findTemplate) + + require.NoError(t, err) + assert.Empty(t, objects, "a search that finds nothing returns nothing") + assert.Equal(t, 1, finalized) +} + +// TestFindObjects_BoundsAModuleThatNeverEnds: a module that keeps returning handles must +// not make the daemon allocate without limit. +func TestFindObjects_BoundsAModuleThatNeverEnds(t *testing.T) { + finalized := 0 + endless := &module{ + cFindObjectsInit: func(ulong, *attribute, ulong) ulong { return rvOK }, + cFindObjectsFinal: func(ulong) ulong { finalized++; return rvOK }, + cFindObjects: func(_ ulong, _ *ulong, _ ulong, count *ulong) ulong { + *count = findBatch + return rvOK + }, + } + + _, err := endless.findObjects(1, findTemplate) + + assert.Error(t, err, "an endless search is cut off") + assert.Equal(t, 1, finalized, "and still finished") +} diff --git a/client/internal/pkcs11/pkcs11.go b/client/internal/pkcs11/pkcs11.go index da874fb62..4a34848d9 100644 --- a/client/internal/pkcs11/pkcs11.go +++ b/client/internal/pkcs11/pkcs11.go @@ -50,6 +50,7 @@ const ( rvPINExpired = 0xa3 rvPINLocked = 0xa4 rvUserAlreadyLoggedIn = 0x100 + rvBufferTooSmall = 0x150 rvAlreadyInitialized = 0x191 ) diff --git a/client/internal/pkcs11/publickey.go b/client/internal/pkcs11/publickey.go index 4d59598e6..aac78524a 100644 --- a/client/internal/pkcs11/publickey.go +++ b/client/internal/pkcs11/publickey.go @@ -70,8 +70,10 @@ func rsaPublicKey(modulus, exponent []byte) (*rsa.PublicKey, error) { // uncompressed point wrapped in a DER OCTET STRING, which some modules hand out bare. func ecPublicKey(params, point []byte) (*ecdsa.PublicKey, error) { var oid asn1.ObjectIdentifier - if _, err := asn1.Unmarshal(params, &oid); err != nil { + if rest, err := asn1.Unmarshal(params, &oid); err != nil { return nil, fmt.Errorf("CKA_EC_PARAMS: %w", err) + } else if len(rest) != 0 { + return nil, errors.New("CKA_EC_PARAMS has trailing data") } curve, ok := curvesByOID[oid.String()] if !ok { @@ -80,8 +82,10 @@ func ecPublicKey(params, point []byte) (*ecdsa.PublicKey, error) { size := (curve.Params().BitSize + 7) / 8 raw := point if len(raw) != 1+2*size { - if _, err := asn1.Unmarshal(point, &raw); err != nil { + if rest, err := asn1.Unmarshal(point, &raw); err != nil { return nil, fmt.Errorf("CKA_EC_POINT: %w", err) + } else if len(rest) != 0 { + return nil, errors.New("CKA_EC_POINT has trailing data") } } if len(raw) != 1+2*size || raw[0] != 4 { diff --git a/client/internal/pkcs11/publickey_test.go b/client/internal/pkcs11/publickey_test.go index 527d6db46..70c1c9a15 100644 --- a/client/internal/pkcs11/publickey_test.go +++ b/client/internal/pkcs11/publickey_test.go @@ -6,6 +6,7 @@ import ( "crypto/rand" "crypto/rsa" "encoding/asn1" + "math" "math/big" "testing" @@ -21,6 +22,8 @@ func TestECPublicKey(t *testing.T) { }{ {"P-256", elliptic.P256(), asn1.ObjectIdentifier{1, 2, 840, 10045, 3, 1, 7}}, {"P-384", elliptic.P384(), asn1.ObjectIdentifier{1, 3, 132, 0, 34}}, + // The wrapped P-521 point is 133 bytes, which needs the long form of the DER length. + {"P-521", elliptic.P521(), asn1.ObjectIdentifier{1, 3, 132, 0, 35}}, } for _, tt := range curves { t.Run(tt.name, func(t *testing.T) { @@ -57,6 +60,13 @@ func TestECPublicKey_Rejections(t *testing.T) { assert.Error(t, err, "a truncated point must be rejected") _, err = ecPublicKey([]byte("junk"), point) assert.Error(t, err, "malformed parameters must be rejected") + + wrapped, err := asn1.Marshal(point) + require.NoError(t, err) + _, err = ecPublicKey(append(append([]byte{}, p256...), 0), point) + assert.Error(t, err, "bytes after the curve parameters must be rejected") + _, err = ecPublicKey(p256, append(append([]byte{}, wrapped...), 0)) + assert.Error(t, err, "bytes after the wrapped point must be rejected") } func TestRSAPublicKey(t *testing.T) { @@ -69,6 +79,12 @@ func TestRSAPublicKey(t *testing.T) { _, err = rsaPublicKey(key.N.Bytes(), nil) assert.Error(t, err, "a missing exponent must be rejected") + _, err = rsaPublicKey(key.N.Bytes(), big.NewInt(math.MaxInt32).Bytes()) + assert.NoError(t, err, "the largest exponent an int holds is accepted") + _, err = rsaPublicKey(key.N.Bytes(), big.NewInt(math.MaxInt32+1).Bytes()) + assert.Error(t, err, "an exponent past int32 must be rejected") + _, err = rsaPublicKey(key.N.Bytes(), []byte{0}) + assert.Error(t, err, "a zero exponent must be rejected") } func TestULongRoundTrip(t *testing.T) { diff --git a/client/internal/pkcs11/uri.go b/client/internal/pkcs11/uri.go index c7248c10e..fb5dc2765 100644 --- a/client/internal/pkcs11/uri.go +++ b/client/internal/pkcs11/uri.go @@ -29,7 +29,7 @@ type URI struct { // module-path must be absolute and a module-name must be a bare name. Unknown query // attributes are ignored, as the RFC asks. func ParseURI(raw string) (*URI, error) { - rest, ok := strings.CutPrefix(raw, "pkcs11:") + rest, ok := cutPrefixFold(raw, "pkcs11:") if !ok { return nil, errors.New("PKCS#11 URI does not start with the pkcs11 scheme") } @@ -72,11 +72,61 @@ func (u *URI) setQueryAttribute(name, value string) error { case "pin-value": u.pinValue = &value case "pin-source": - u.pinSource = value + path, err := pinSourcePath(value) + if err != nil { + return err + } + u.pinSource = path } return nil } +// pinSourcePath turns a pin-source value into the file to read. A plain path is used as +// it is; a file URI must name a local absolute path, with no host or localhost (RFC 8089 +// section 2), so a PIN is never read from somewhere the URI only appears to point to. +func pinSourcePath(value string) (string, error) { + if value == "" { + return "", errors.New("PKCS#11 URI pin-source is empty") + } + rest, isFile := cutPrefixFold(value, "file:") + if !isFile { + return value, nil + } + authorityAndPath, hasAuthority := strings.CutPrefix(rest, "//") + if !hasAuthority { + if !isAbsolutePath(rest) { + return "", fmt.Errorf("PKCS#11 URI pin-source %q must be an absolute path", value) + } + return rest, nil + } + if isAbsolutePath(authorityAndPath) { + return authorityAndPath, nil + } + host, path, _ := strings.Cut(authorityAndPath, "/") + if host != "" && !strings.EqualFold(host, "localhost") { + return "", fmt.Errorf("PKCS#11 URI pin-source host %q is not the local machine", host) + } + if path == "" { + return "", fmt.Errorf("PKCS#11 URI pin-source %q names no file", value) + } + return "/" + path, nil +} + +// isAbsolutePath reports whether p is absolute in a file URI. A path starting with a slash +// is absolute there on every platform, including Windows, where filepath.IsAbs wants a +// drive letter. +func isAbsolutePath(p string) bool { + return strings.HasPrefix(p, "/") || filepath.IsAbs(p) +} + +// cutPrefixFold is strings.CutPrefix with the prefix matched without regard to case. +func cutPrefixFold(s, prefix string) (string, bool) { + if len(s) < len(prefix) || !strings.EqualFold(s[:len(prefix)], prefix) { + return s, false + } + return s[len(prefix):], true +} + // Module is the library to load, DefaultModule when the URI names none. func (u *URI) Module() string { if u.ModulePath == "" { @@ -99,8 +149,7 @@ func (u *URI) PIN() ([]byte, error) { if u.pinSource == "" { return nil, nil } - path := strings.TrimPrefix(strings.TrimPrefix(u.pinSource, "file://"), "file:") - pin, err := os.ReadFile(path) + pin, err := os.ReadFile(u.pinSource) if err != nil { return nil, fmt.Errorf("read PIN: %w", err) } diff --git a/client/internal/pkcs11/uri_test.go b/client/internal/pkcs11/uri_test.go index 54e8a0d91..09aede6a7 100644 --- a/client/internal/pkcs11/uri_test.go +++ b/client/internal/pkcs11/uri_test.go @@ -84,6 +84,10 @@ func TestParseURI_Rejections(t *testing.T) { "module-name with dotdot": "pkcs11:token=a?module-name=..", "empty module-name": "pkcs11:token=a?module-name=", "pin-value and pin-source": "pkcs11:token=a?pin-value=1&pin-source=file:/etc/pin", + // Without a value the URI would silently mean "no PIN" and skip the login. + "empty pin-source": "pkcs11:token=a?pin-source=", + "remote pin-source": "pkcs11:token=a?pin-source=file://example.com/etc/pin", + "relative pin-source": "pkcs11:token=a?pin-source=file:pin", } for name, raw := range tests { _, err := ParseURI(raw) @@ -91,6 +95,31 @@ func TestParseURI_Rejections(t *testing.T) { } } +// TestParseURI_SchemeIsCaseInsensitive: RFC 3986 section 3.1 makes schemes case-insensitive. +func TestParseURI_SchemeIsCaseInsensitive(t *testing.T) { + for _, raw := range []string{"PKCS11:token=netbird", "Pkcs11:token=netbird"} { + uri, err := ParseURI(raw) + require.NoError(t, err, raw) + assert.Equal(t, "netbird", uri.Token, raw) + } +} + +func TestURI_PINFromLocalhostFileURI(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("file URIs with a host name are written differently on Windows") + } + path := filepath.Join(t.TempDir(), "pin") + require.NoError(t, os.WriteFile(path, []byte("secret\n"), 0o600)) + + for _, host := range []string{"localhost", "LocalHost"} { + uri, err := ParseURI("pkcs11:token=netbird?pin-source=file://" + host + path) + require.NoError(t, err, host) + pin, err := uri.PIN() + require.NoError(t, err, host) + assert.Equal(t, []byte("secret"), pin, "RFC 8089 names the local machine as localhost") + } +} + func TestURI_PINFromFile(t *testing.T) { path := filepath.Join(t.TempDir(), "pin") require.NoError(t, os.WriteFile(path, []byte("secret\n"), 0o600)) @@ -117,3 +146,30 @@ func absModule(name string) string { } return "/lib/" + name } + +// TestPinSourcePath covers how a pin-source value becomes a file path, without touching the +// file system, so the same cases run on every platform. A path starting with a slash is +// absolute in a file URI everywhere, including Windows, where filepath.IsAbs would refuse it. +func TestPinSourcePath(t *testing.T) { + tests := []struct { + value string + want string + }{ + {"/etc/netbird/pkcs11.pin", "/etc/netbird/pkcs11.pin"}, + {"file:/etc/netbird/pkcs11.pin", "/etc/netbird/pkcs11.pin"}, + {"file:///etc/netbird/pkcs11.pin", "/etc/netbird/pkcs11.pin"}, + {"FILE:///etc/netbird/pkcs11.pin", "/etc/netbird/pkcs11.pin"}, + {"file://localhost/etc/netbird/pkcs11.pin", "/etc/netbird/pkcs11.pin"}, + {"file://LocalHost/etc/netbird/pkcs11.pin", "/etc/netbird/pkcs11.pin"}, + } + for _, tt := range tests { + got, err := pinSourcePath(tt.value) + require.NoError(t, err, tt.value) + assert.Equal(t, tt.want, got, tt.value) + } + + for _, value := range []string{"", "file:pin", "file://example.com/etc/pin", "file://localhost", "file://localhost/"} { + _, err := pinSourcePath(value) + assert.Error(t, err, "%q must be refused", value) + } +} diff --git a/management/internals/network_map_db/shared_types_test.go b/management/internals/network_map_db/shared_types_test.go index 8a1239e26..34b69d265 100644 --- a/management/internals/network_map_db/shared_types_test.go +++ b/management/internals/network_map_db/shared_types_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestRecordTypeAndRdata(t *testing.T) { @@ -36,3 +37,24 @@ func TestRecordTypeAndRdata(t *testing.T) { }) } } + +// TestConvertToNmdataPeers_CarriesTheVerifiedCertificates: the certificate posture check is +// evaluated on the chains stored in the peer meta, so a peer loaded through this path with +// its certificates dropped would silently fail every certificate check. +func TestConvertToNmdataPeers_CarriesTheVerifiedCertificates(t *testing.T) { + peers, _, err := ConvertToNmdataPeers([]Peer{ + {MetaCertificates: []byte(`["-----BEGIN CERTIFICATE-----\nleaf\n-----END CERTIFICATE-----\n","chain-2"]`)}, + {}, + }) + + require.NoError(t, err) + require.Len(t, peers, 2) + assert.Equal(t, []string{"-----BEGIN CERTIFICATE-----\nleaf\n-----END CERTIFICATE-----\n", "chain-2"}, peers[0].Meta.Certificates) + assert.Empty(t, peers[1].Meta.Certificates, "a peer that proved nothing has no certificates") +} + +func TestConvertToNmdataPeers_RejectsMalformedCertificates(t *testing.T) { + _, _, err := ConvertToNmdataPeers([]Peer{{MetaCertificates: []byte(`{"not":"a list"}`)}}) + + assert.Error(t, err, "a stored value that is not a list of chains must not load as no certificates") +} diff --git a/shared/management/certposture/certposture_test.go b/shared/management/certposture/certposture_test.go index ae31a2203..e8f3d20c1 100644 --- a/shared/management/certposture/certposture_test.go +++ b/shared/management/certposture/certposture_test.go @@ -2,6 +2,8 @@ package certposture import ( "crypto" + "crypto/ecdsa" + "crypto/elliptic" "crypto/rand" "crypto/rsa" "crypto/sha256" @@ -86,6 +88,7 @@ func TestSignVerify_RoundTripPerKeyType(t *testing.T) { ca := certtest.NewCA(t, "root") keys := map[string]crypto.Signer{ SigAlgECDSASHA256: certtest.ECDSAKey(t), + SigAlgECDSASHA384: p384Key(t), SigAlgRSAPSSSHA256: certtest.RSAKey(t), SigAlgEd25519: certtest.Ed25519Key(t), } @@ -103,6 +106,24 @@ func TestSignVerify_RoundTripPerKeyType(t *testing.T) { } } +func p384Key(t *testing.T) crypto.Signer { + t.Helper() + key, err := ecdsa.GenerateKey(elliptic.P384(), rand.Reader) + require.NoError(t, err) + return key +} + +// TestSign_RefusesCurvesTheProofDoesNotName: a proof names its algorithm from the key, and +// the verifier only knows P-256 and P-384, so P-521 is refused before anything is signed. +func TestSign_RefusesCurvesTheProofDoesNotName(t *testing.T) { + key, err := ecdsa.GenerateKey(elliptic.P521(), rand.Reader) + require.NoError(t, err) + + _, _, err = Sign(key, NewChallenger(secret).Nonce(peerKey, now), peerKey) + + assert.ErrorIs(t, err, ErrUnsupportedKey) +} + func TestVerify_Rejections(t *testing.T) { ca := certtest.NewCA(t, "root") c := NewChallenger(secret) diff --git a/shared/management/client/grpc_test.go b/shared/management/client/grpc_test.go index c947130fd..8c69cea1c 100644 --- a/shared/management/client/grpc_test.go +++ b/shared/management/client/grpc_test.go @@ -12,6 +12,8 @@ import ( "google.golang.org/grpc" "google.golang.org/grpc/credentials/insecure" + "github.com/netbirdio/netbird/client/system" + "github.com/netbirdio/netbird/shared/management/certposture" mgmtProto "github.com/netbirdio/netbird/shared/management/proto" ) @@ -93,3 +95,25 @@ func TestMaxRecvMsgSizeIntegration(t *testing.T) { assert.Len(t, resp.Key, payloadSize) }) } + +// TestInfoToMetaData_CarriesCertificateProofs: management verifies the proofs it receives +// in the peer meta, so a field dropped by the mapping fails the certificate check without +// any error on either side. +func TestInfoToMetaData_CarriesCertificateProofs(t *testing.T) { + info := &system.Info{CertificateProofs: []certposture.Proof{ + {Nonce: []byte("nonce-1"), Chain: [][]byte{[]byte("leaf"), []byte("intermediate")}, SigAlg: "ecdsa-sha256", Signature: []byte("sig-1")}, + {Nonce: []byte("nonce-2"), Chain: [][]byte{[]byte("other")}, SigAlg: "rsa-pss-sha256", Signature: []byte("sig-2")}, + }} + + meta := infoToMetaData(info) + + require.Len(t, meta.GetCertificateProofs(), 2, "every proof reaches the meta") + first := meta.GetCertificateProofs()[0] + assert.Equal(t, []byte("nonce-1"), first.GetNonce()) + assert.Equal(t, [][]byte{[]byte("leaf"), []byte("intermediate")}, first.GetChain(), "the whole chain is carried, in order") + assert.Equal(t, "ecdsa-sha256", first.GetSigAlg()) + assert.Equal(t, []byte("sig-1"), first.GetSignature()) + assert.Equal(t, "rsa-pss-sha256", meta.GetCertificateProofs()[1].GetSigAlg()) + + assert.Empty(t, infoToMetaData(&system.Info{}).GetCertificateProofs(), "a device without proofs sends none") +} diff --git a/shared/management/http/api/openapi.yml b/shared/management/http/api/openapi.yml index 8b97d5f47..fbf8bf5cd 100644 --- a/shared/management/http/api/openapi.yml +++ b/shared/management/http/api/openapi.yml @@ -1821,6 +1821,7 @@ components: ca_certificates: description: PEM encoded CA certificates the peer's certificate must chain to type: array + minItems: 1 items: type: string example: ["-----BEGIN CERTIFICATE-----\nMIIB...\n-----END CERTIFICATE-----"]