From e0b6a38aa29caad543e41a6f0d4f2478e2665ab2 Mon Sep 17 00:00:00 2001
From: Viktor Liu <viktor@netbird.io>
Date: Fri, 2 Oct 2026 12:27:00 +0200
Subject: [PATCH] Require a token label whenever a PKCS#11 PIN is set

---
 client/internal/certproof/README.md           | 22 +++++++++----------
 client/internal/certproof/pinlatch.go         |  3 +++
 client/internal/certproof/pkcs11store.go      | 18 ++++++++++-----
 client/internal/certproof/pkcs11store_test.go | 16 +++++++++++---
 client/internal/certproof/store_other_test.go |  5 +++--
 client/internal/pkcs11/uri.go                 |  5 +++++
 6 files changed, 46 insertions(+), 23 deletions(-)

diff --git a/client/internal/certproof/README.md b/client/internal/certproof/README.md
index 2c63c78a9..8a62849b5 100644
--- a/client/internal/certproof/README.md
+++ b/client/internal/certproof/README.md
@@ -147,23 +147,21 @@ NB_TPM_DEVICE=/tmp/swtpm.sock go test ./client/internal/certproof/ -run TestColl
 
 Distributions that follow Red Hat's guidance reach the TPM through tpm2-pkcs11, a PKCS#11
 module whose token holds both the key and, after `tpm2_ptool addcert`, the certificate.
-The store reads that token when the daemon's environment carries the token's user PIN in
-`NB_TPM_PIN`. The PIN is never read from the profile config or a command-line flag; set it
-on the service instead:
-
-```sh
-netbird service install --service-env NB_TPM_PIN=1234
-```
-
-That alone opens the first token the p11-kit proxy exposes, which is tpm2-pkcs11 on a
-stock setup that has registered it. `CertPKCS11URI`, an RFC 7512 URI, narrows that down
-on a host with several tokens or without p11-kit:
+The store reads that token when `CertPKCS11URI` in the profile config, an RFC 7512 URI,
+names it, and the daemon's environment carries the token's user PIN in `NB_TPM_PIN`. The
+PIN is not a profile config field or a command-line flag; set it on the service:
 
 ```json
 "CertPKCS11URI": "pkcs11:token=netbird?module-path=/usr/lib/x86_64-linux-gnu/libtpm2_pkcs11.so"
 ```
 
-`token` selects the token by label, or the first token present when absent. `module-path`
+```sh
+netbird service install --service-env NB_TPM_PIN=1234
+```
+
+`token` selects the token by label and is required whenever a PIN is set, from any source:
+a PIN that names no token would go to whichever token is listed first, which may be a
+plugged-in smartcard, and each wrong PIN counts towards that card's lockout. `module-path`
 names the library to load; `module-name=tpm2_pkcs11` resolves to `libtpm2_pkcs11.so` on
 the loader's search path, and with neither the p11-kit proxy is loaded, which exposes every
 module the system has registered. The URI may carry the PIN itself, as `pin-value` inline
diff --git a/client/internal/certproof/pinlatch.go b/client/internal/certproof/pinlatch.go
index 1dd7d03d1..1e02540f8 100644
--- a/client/internal/certproof/pinlatch.go
+++ b/client/internal/certproof/pinlatch.go
@@ -12,6 +12,9 @@ import (
 // shared with everything else on the machine, and proofs are collected on every sync.
 var errPINRejectedBefore = errors.New("PKCS#11 token rejected this PIN before, not trying it again")
 
+// errPINNeedsToken refuses a PIN that names no token to log in to.
+var errPINNeedsToken = errors.New("a PKCS#11 PIN needs the token named in CertPKCS11URI, as token=<label>")
+
 // rejectedPINs outlives a single store, since a store is built for each collection.
 var rejectedPINs = &pinLatch{keys: map[[sha256.Size]byte]struct{}{}}
 
diff --git a/client/internal/certproof/pkcs11store.go b/client/internal/certproof/pkcs11store.go
index cf3e3bd50..1a9c353b9 100644
--- a/client/internal/certproof/pkcs11store.go
+++ b/client/internal/certproof/pkcs11store.go
@@ -44,16 +44,22 @@ type PKCS11Store struct {
 
 // 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}
-	if cfg.URI == "" {
-		return store, nil
+	if cfg.URI != "" {
+		parsed, err := pkcs11.ParseURI(cfg.URI)
+		if err != nil {
+			return nil, err
+		}
+		store.uri = parsed
 	}
-	parsed, err := pkcs11.ParseURI(cfg.URI)
-	if err != nil {
-		return nil, err
+	if (cfg.PIN != "" || store.uri.HasPIN()) && store.uri.Token == "" {
+		return nil, errPINNeedsToken
 	}
-	store.uri = parsed
 	return store, nil
 }
 
diff --git a/client/internal/certproof/pkcs11store_test.go b/client/internal/certproof/pkcs11store_test.go
index 2a99bb296..e36f800ac 100644
--- a/client/internal/certproof/pkcs11store_test.go
+++ b/client/internal/certproof/pkcs11store_test.go
@@ -310,9 +310,9 @@ func TestNewPKCS11Store_PIN(t *testing.T) {
 		wantPIN    []byte
 		wantModule string
 	}{
-		{"pin alone opens the first p11-kit token", PKCS11Config{PIN: "1234"}, []byte("1234"), pkcs11.DefaultModule},
-		{"pin field wins over pin-value", PKCS11Config{URI: "pkcs11:?module-path=/lib/x.so&pin-value=0000", PIN: "1234"}, []byte("1234"), "/lib/x.so"},
-		{"uri pin-value stands in for a missing field", PKCS11Config{URI: "pkcs11:?pin-value=0000"}, []byte("0000"), pkcs11.DefaultModule},
+		{"pin with a token label opens that token through p11-kit", PKCS11Config{URI: "pkcs11:token=netbird", PIN: "1234"}, []byte("1234"), pkcs11.DefaultModule},
+		{"pin field wins over pin-value", PKCS11Config{URI: "pkcs11:token=netbird?module-path=/lib/x.so&pin-value=0000", PIN: "1234"}, []byte("1234"), "/lib/x.so"},
+		{"uri pin-value stands in for a missing field", PKCS11Config{URI: "pkcs11:token=netbird?pin-value=0000"}, []byte("0000"), pkcs11.DefaultModule},
 		{"no pin at all means no login", PKCS11Config{URI: "pkcs11:token=netbird"}, nil, pkcs11.DefaultModule},
 	}
 	for _, tt := range tests {
@@ -328,4 +328,14 @@ func TestNewPKCS11Store_PIN(t *testing.T) {
 
 	_, 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{
+		"env pin without uri":      {PIN: "1234"},
+		"env pin, uri lacks token": {URI: "pkcs11:?module-path=/lib/x.so", PIN: "1234"},
+		"inline pin-value only":    {URI: "pkcs11:?pin-value=0000"},
+		"pin-source only":          {URI: "pkcs11:?pin-source=file:/etc/netbird/pkcs11.pin"},
+	} {
+		_, err := NewPKCS11Store(cfg, "")
+		assert.ErrorIs(t, err, errPINNeedsToken, "%s: a PIN must not go to whichever token is listed first", name)
+	}
 }
diff --git a/client/internal/certproof/store_other_test.go b/client/internal/certproof/store_other_test.go
index 6d1c4d52c..ba0aeb033 100644
--- a/client/internal/certproof/store_other_test.go
+++ b/client/internal/certproof/store_other_test.go
@@ -21,10 +21,11 @@ func TestStoreWithToken(t *testing.T) {
 	assert.Equal(t, StoreDir(), files.dir, "no directory configured falls back to the environment or the default")
 
 	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")
 
 	for name, cfg := range map[string]PKCS11Config{
-		"pin alone": {PIN: "1234"},
-		"uri alone": {URI: "pkcs11:token=netbird?pin-value=1234"},
+		"env pin with token uri": {URI: "pkcs11:token=netbird", PIN: "1234"},
+		"uri alone":              {URI: "pkcs11:token=netbird?pin-value=1234"},
 	} {
 		store, ok := storeWithToken(Config{Dir: dir, PKCS11: cfg}).(Stores)
 		require.True(t, ok, "%s joins the token to the PEM directory", name)
diff --git a/client/internal/pkcs11/uri.go b/client/internal/pkcs11/uri.go
index 8794b7020..f8d674bbd 100644
--- a/client/internal/pkcs11/uri.go
+++ b/client/internal/pkcs11/uri.go
@@ -80,6 +80,11 @@ func (u *URI) Module() string {
 	return u.ModulePath
 }
 
+// HasPIN reports whether the URI carries a PIN, inline or as a pin-source.
+func (u *URI) HasPIN() bool {
+	return u.pinValue != nil || u.pinSource != ""
+}
+
 // PIN returns the user PIN, or nil when the URI carries none and no login should happen.
 // A pin-source names a file whose single line is the PIN.
 func (u *URI) PIN() ([]byte, error) {