diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index d133d87f1..22ab7f566 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -23,8 +23,50 @@ concurrency: cancel-in-progress: true jobs: + freebsd_port_trigger: + name: "FreeBSD Port / Trigger" + runs-on: ubuntu-22.04 + outputs: + run: ${{ steps.decide.outputs.run }} + version: ${{ steps.decide.outputs.version }} + steps: + - name: Checkout + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + persist-credentials: false + + - name: Decide whether the port job runs + id: decide + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: | + set -euo pipefail + run=false + case "$GITHUB_EVENT_NAME" in + push) + if [[ "$GITHUB_REF" == "refs/heads/main" ]]; then + run=true + elif [[ "$GITHUB_REF" == refs/tags/* && "$GITHUB_REF_NAME" =~ ^v[0-9]+\.[0-9]+\.[0-9]+$ ]]; then + # The port tracks stable releases only, so a canary or rc tag has nothing to submit. + run=true + echo "version=${GITHUB_REF_NAME#v}" >> "$GITHUB_OUTPUT" + fi + ;; + pull_request) + git fetch --depth=1 origin "$BASE_SHA" + if git diff --name-only "$BASE_SHA" HEAD | \ + grep -qE '^(release_files/freebsd-port-.*\.sh|\.github/workflows/release\.yml)$'; then + run=true + fi + ;; + esac + echo "run=$run" >> "$GITHUB_OUTPUT" + echo "FreeBSD port job runs: $run" + release_freebsd_port: name: "FreeBSD Port / Build & Test" + needs: freebsd_port_trigger + if: needs.freebsd_port_trigger.outputs.run == 'true' runs-on: ubuntu-22.04 steps: - name: Checkout @@ -33,18 +75,19 @@ jobs: persist-credentials: false - name: Generate FreeBSD port diff - run: bash -x release_files/freebsd-port-diff.sh - - - name: Generate FreeBSD port issue body - run: bash -x release_files/freebsd-port-issue-body.sh + env: + TARGET_VERSION: ${{ needs.freebsd_port_trigger.outputs.version }} + run: bash -x release_files/freebsd-port-diff.sh "$TARGET_VERSION" - name: Check if diff was generated id: check_diff run: | - if ls netbird-*.diff 1> /dev/null 2>&1; then - echo "diff_exists=true" >> $GITHUB_OUTPUT + shopt -s nullglob + diffs=(netbird-*.diff) + if [[ ${#diffs[@]} -gt 0 ]]; then + echo "diff_exists=true" >> "$GITHUB_OUTPUT" else - echo "diff_exists=false" >> $GITHUB_OUTPUT + echo "diff_exists=false" >> "$GITHUB_OUTPUT" echo "No diff file generated (port may already be up to date)" fi @@ -52,10 +95,18 @@ jobs: if: steps.check_diff.outputs.diff_exists == 'true' id: version run: | - VERSION=$(ls netbird-*.diff | sed 's/netbird-\(.*\)\.diff/\1/') - echo "version=$VERSION" >> $GITHUB_OUTPUT + diffs=(netbird-*.diff) + VERSION="${diffs[0]#netbird-}" + VERSION="${VERSION%.diff}" + echo "version=$VERSION" >> "$GITHUB_OUTPUT" echo "Generated files for version: $VERSION" - cat netbird-*.diff + cat "${diffs[0]}" + + - name: Generate FreeBSD port issue body + if: steps.check_diff.outputs.diff_exists == 'true' + env: + TARGET_VERSION: ${{ steps.version.outputs.version }} + run: bash -x release_files/freebsd-port-issue-body.sh "" "$TARGET_VERSION" - name: Read Go version from go.mod id: goversion @@ -366,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/cmd/up.go b/client/cmd/up.go index 5a6a3bb26..268dee930 100644 --- a/client/cmd/up.go +++ b/client/cmd/up.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "net" "net/netip" "runtime" "strings" @@ -26,16 +25,11 @@ import ( "github.com/netbirdio/netbird/client/proto" "github.com/netbirdio/netbird/client/server" "github.com/netbirdio/netbird/client/system" + "github.com/netbirdio/netbird/shared/externalipmap" "github.com/netbirdio/netbird/shared/management/domain" "github.com/netbirdio/netbird/util" ) -const ( - invalidInputType int = iota - ipInputType - interfaceInputType -) - const ( dnsLabelsFlag = "extra-dns-labels" @@ -106,7 +100,7 @@ func upFunc(cmd *cobra.Command, args []string) error { return fmt.Errorf("failed initializing log %v", err) } - err = validateNATExternalIPs(natExternalIPs) + err = externalipmap.Validate(natExternalIPs) if err != nil { return err } @@ -846,36 +840,6 @@ func setupLoginRequest(providedSetupKey string, customDNSAddressConverted []byte return &loginRequest, nil } -func validateNATExternalIPs(list []string) error { - for _, element := range list { - if element == "" { - return fmt.Errorf("empty string is not a valid input for %s", externalIPMapFlag) - } - - subElements := strings.Split(element, "/") - if len(subElements) > 2 { - return fmt.Errorf("%s is not a valid input for %s. it should be formatted as \"String\" or \"String/String\"", element, externalIPMapFlag) - } - - if len(subElements) == 1 && !isValidIP(subElements[0]) { - return fmt.Errorf("%s is not a valid input for %s. it should be formatted as \"IP\" or \"IP/IP\", or \"IP/Interface Name\"", element, externalIPMapFlag) - } - - last := 0 - for _, singleElement := range subElements { - inputType, err := validateElement(singleElement) - if err != nil { - return fmt.Errorf("%s is not a valid input for %s. it should be an IP string or a network name", singleElement, externalIPMapFlag) - } - if last == interfaceInputType && inputType == interfaceInputType { - return fmt.Errorf("%s is not a valid input for %s. it should not contain two interface names", element, externalIPMapFlag) - } - last = inputType - } - } - return nil -} - func parseInterfaceName(name string) error { if runtime.GOOS != "darwin" { return nil @@ -888,39 +852,6 @@ func parseInterfaceName(name string) error { return fmt.Errorf("invalid interface name %s. Please use the prefix utun followed by a number on MacOS. e.g., utun1 or utun199", name) } -func validateElement(element string) (int, error) { - if isValidIP(element) { - return ipInputType, nil - } - validIface, err := isValidInterface(element) - if err != nil { - return invalidInputType, fmt.Errorf("unable to validate the network interface name, error: %s", err) - } - - if validIface { - return interfaceInputType, nil - } - - return interfaceInputType, fmt.Errorf("invalid IP or network interface name not found") -} - -func isValidIP(ip string) bool { - return net.ParseIP(ip) != nil -} - -func isValidInterface(name string) (bool, error) { - netInterfaces, err := net.Interfaces() - if err != nil { - return false, err - } - for _, iface := range netInterfaces { - if iface.Name == name { - return true, nil - } - } - return false, nil -} - func parseCustomDNSAddress(modified bool) ([]byte, error) { var parsed []byte if modified { diff --git a/client/embed/embed.go b/client/embed/embed.go index 6fdddab48..208e71817 100644 --- a/client/embed/embed.go +++ b/client/embed/embed.go @@ -118,6 +118,9 @@ type Options struct { MTU *uint16 // DNSLabels defines additional DNS labels configured in the peer. DNSLabels []string + // NATExternalIPs maps external IPs to internal IPs or interfaces for ICE + // candidates, same format as the agent's --external-ip-map flag. + NATExternalIPs []string // Performance configures the tunnel's buffer pool cap and batch size. Performance Performance } @@ -224,6 +227,7 @@ func New(opts Options) (*Client, error) { WireguardPort: opts.WireguardPort, MTU: opts.MTU, DNSLabels: parsedLabels, + NATExternalIPs: opts.NATExternalIPs, } if opts.ConfigPath != "" { config, err = profilemanager.UpdateOrCreateConfig(input) diff --git a/client/iface/bind/ice_bind_test.go b/client/iface/bind/ice_bind_test.go index 0b8db7640..8dd82d7df 100644 --- a/client/iface/bind/ice_bind_test.go +++ b/client/iface/bind/ice_bind_test.go @@ -2,17 +2,22 @@ package bind import ( "fmt" + "io" "net" "net/netip" "sync" + "sync/atomic" "testing" "time" + "github.com/pion/transport/v3" "github.com/pion/transport/v3/stdnet" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/net/ipv4" "golang.org/x/net/ipv6" + "golang.zx2c4.com/wireguard/device" + "golang.zx2c4.com/wireguard/tun/netstack" "github.com/netbirdio/netbird/client/iface/wgaddr" ) @@ -326,3 +331,86 @@ func listenUDP(t *testing.T, network, addr string) *net.UDPConn { require.NoError(t, err) return conn } + +type countingNet struct { + transport.Net + calls atomic.Int64 + ifaces []*transport.Interface + before func() +} + +func (n *countingNet) Interfaces() ([]*transport.Interface, error) { + n.calls.Add(1) + if n.before != nil { + n.before() + } + return n.ifaces, nil +} + +func TestICEBind_OpenDoesNotDiscoverInterfaces(t *testing.T) { + n := newCountingNet() + iceBind := NewICEBind(n, testAddress(), 1280) + + for i := 0; i < 2; i++ { + _, port, err := iceBind.Open(0) + require.NoError(t, err) + require.NotZero(t, port, "the bind listens on a port") + assert.Equal(t, int64(0), n.calls.Load(), "opening the bind must not enumerate interfaces, attempt %d", i+1) + require.NoError(t, iceBind.Close()) + } + + _, _, err := iceBind.Open(0) + require.NoError(t, err) + t.Cleanup(func() { _ = iceBind.Close() }) + + mux, err := iceBind.GetICEMux() + require.NoError(t, err) + assert.NotEmpty(t, mux.SingleSocketUDPMux.GetListenAddresses(), "listen addresses come from the discovered interfaces") + assert.Equal(t, int64(1), n.calls.Load(), "interfaces are discovered when the listen addresses are asked for") +} + +func TestICEBind_DeviceUpDoesNotWaitOnInterfaceDiscovery(t *testing.T) { + tunDev, _, err := netstack.CreateNetTUN([]netip.Addr{netip.MustParseAddr("100.64.0.1")}, nil, 1280) + require.NoError(t, err) + + var dev *device.Device + n := newCountingNet() + n.before = func() { + _ = dev.IpcGetOperation(io.Discard) + } + iceBind := NewICEBind(n, testAddress(), 1280) + dev = device.NewDevice(tunDev, iceBind, device.NewLogger(device.LogLevelSilent, "")) + + upDone := make(chan error, 1) + go func() { + upDone <- dev.Up() + }() + + select { + case err := <-upDone: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("device Up did not return: interface discovery ran under the device lock and waited on the device itself") + } + t.Cleanup(dev.Close) + + assert.Equal(t, int64(0), n.calls.Load(), "bringing the device up must not enumerate interfaces") + + mux, err := iceBind.GetICEMux() + require.NoError(t, err) + assert.NotEmpty(t, mux.SingleSocketUDPMux.GetListenAddresses(), "discovery that talks to the device succeeds once the device is up") + assert.Equal(t, int64(1), n.calls.Load(), "interfaces are discovered outside the device lock") +} + +func newCountingNet() *countingNet { + ifc := transport.NewInterface(net.Interface{Index: 1, MTU: 1500, Name: "eth0", Flags: net.FlagUp | net.FlagMulticast}) + ifc.AddAddress(&net.IPNet{IP: net.IPv4(192, 168, 1, 10), Mask: net.CIDRMask(24, 32)}) + return &countingNet{ifaces: []*transport.Interface{ifc}} +} + +func testAddress() wgaddr.Address { + return wgaddr.Address{ + IP: netip.MustParseAddr("100.64.0.1"), + Network: netip.MustParsePrefix("100.64.0.0/10"), + } +} diff --git a/client/iface/udpmux/mux.go b/client/iface/udpmux/mux.go index 3aa0b4d88..2f7287b53 100644 --- a/client/iface/udpmux/mux.go +++ b/client/iface/udpmux/mux.go @@ -47,6 +47,7 @@ type SingleSocketUDPMux struct { // for UDP connection listen at unspecified address localAddrsForUnspecified []net.Addr + unspecified bool } const maxAddrSize = 512 @@ -157,6 +158,9 @@ func NewSingleSocketUDPMux(params Params) *SingleSocketUDPMux { if params.Logger == nil { params.Logger = getLogger() } + if params.Net == nil { + params.Net = stdnet.NewNet(context.Background(), nil, nil) + } mux := &SingleSocketUDPMux{ addressMap: map[string][]*udpMuxedConn{}, @@ -165,6 +169,7 @@ func NewSingleSocketUDPMux(params Params) *SingleSocketUDPMux { connsIPv6: make(map[string]*udpMuxedConn), candidateConnMap: make(map[string]*udpMuxedConn), closedChan: make(chan struct{}, 1), + unspecified: isUnspecified(params.UDPConn.LocalAddr()), pool: &sync.Pool{ New: func() interface{} { // big enough buffer to fit both packet and address @@ -173,7 +178,6 @@ func NewSingleSocketUDPMux(params Params) *SingleSocketUDPMux { }, } - mux.updateLocalAddresses() return mux } @@ -199,10 +203,6 @@ func (m *SingleSocketUDPMux) updateLocalAddresses() { m.params.Logger.Errorf("LocalAddr expected IPV4 or IPV6, got %T", m.params.UDPConn.LocalAddr()) } if len(networks) > 0 { - if m.params.Net == nil { - m.params.Net = stdnet.NewNet(context.Background(), nil, nil) - } - ips, err := localInterfaces(m.params.Net, m.params.InterfaceFilter, nil, networks, true) if err == nil { for _, ip := range ips { @@ -241,10 +241,7 @@ func (m *SingleSocketUDPMux) GetListenAddresses() []net.Addr { // creates the connection if an existing one can't be found func (m *SingleSocketUDPMux) GetConn(ufrag string, addr net.Addr, candidateID string) (net.PacketConn, error) { // don't check addr for mux using unspecified address - m.mu.Lock() - lenLocalAddrs := len(m.localAddrsForUnspecified) - m.mu.Unlock() - if lenLocalAddrs == 0 && m.params.UDPConn.LocalAddr().String() != addr.String() { + if !m.unspecified && m.params.UDPConn.LocalAddr().String() != addr.String() { return nil, fmt.Errorf("invalid address %s", addr.String()) } @@ -508,3 +505,8 @@ func getLogger() logging.LeveledLogger { //fac.Writer = log.StandardLogger().Writer() return fac.NewLogger("ice") } + +func isUnspecified(addr net.Addr) bool { + udpAddr, ok := addr.(*net.UDPAddr) + return ok && udpAddr.IP.IsUnspecified() +} diff --git a/client/iface/udpmux/mux_test.go b/client/iface/udpmux/mux_test.go new file mode 100644 index 000000000..2e2770282 --- /dev/null +++ b/client/iface/udpmux/mux_test.go @@ -0,0 +1,99 @@ +package udpmux + +import ( + "fmt" + "net" + "sync" + "sync/atomic" + "testing" + + "github.com/pion/transport/v3" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type countingNet struct { + transport.Net + calls atomic.Int64 + ifaces []*transport.Interface +} + +func (n *countingNet) Interfaces() ([]*transport.Interface, error) { + n.calls.Add(1) + return n.ifaces, nil +} + +func TestNewSingleSocketUDPMux_DoesNotDiscoverInterfaces(t *testing.T) { + conn := listenUDP(t, net.IPv4zero) + n := &countingNet{ifaces: []*transport.Interface{testInterface("eth0", net.IPv4(192, 168, 1, 10))}} + + mux := NewSingleSocketUDPMux(Params{UDPConn: conn, Net: n}) + t.Cleanup(func() { _ = mux.Close() }) + + assert.Equal(t, int64(0), n.calls.Load(), "constructing the mux must not enumerate interfaces") + + addrs := mux.GetListenAddresses() + assert.Equal(t, int64(1), n.calls.Load(), "listen addresses are discovered when they are asked for") + + port := conn.LocalAddr().(*net.UDPAddr).Port + require.Len(t, addrs, 1, "one address per discovered interface IP") + assert.Equal(t, fmt.Sprintf("192.168.1.10:%d", port), addrs[0].String(), "the discovered IP is paired with the socket port") +} + +func TestSingleSocketUDPMux_GetConnWithoutListenAddresses(t *testing.T) { + t.Run("unspecified socket accepts any address", func(t *testing.T) { + conn := listenUDP(t, net.IPv4zero) + n := &countingNet{} + mux := NewSingleSocketUDPMux(Params{UDPConn: conn, Net: n}) + t.Cleanup(func() { _ = mux.Close() }) + + port := conn.LocalAddr().(*net.UDPAddr).Port + pc, err := mux.GetConn("ufrag", &net.UDPAddr{IP: net.IPv4(192, 168, 1, 10), Port: port}, "candidate") + require.NoError(t, err) + assert.NotNil(t, pc, "a connection is created for a local address of an unspecified socket") + assert.Equal(t, int64(0), n.calls.Load(), "GetConn must not depend on a prior discovery") + }) + + t.Run("specific socket rejects a foreign address", func(t *testing.T) { + conn := listenUDP(t, net.IPv4(127, 0, 0, 1)) + mux := NewSingleSocketUDPMux(Params{UDPConn: conn}) + t.Cleanup(func() { _ = mux.Close() }) + + _, err := mux.GetConn("ufrag", &net.UDPAddr{IP: net.IPv4(192, 168, 1, 10), Port: 1234}, "candidate") + assert.Error(t, err, "an address the socket is not bound to must be rejected") + + pc, err := mux.GetConn("ufrag", conn.LocalAddr(), "candidate") + require.NoError(t, err) + assert.NotNil(t, pc, "the bound address is accepted") + }) +} + +func listenUDP(t *testing.T, ip net.IP) *net.UDPConn { + t.Helper() + conn, err := net.ListenUDP("udp4", &net.UDPAddr{IP: ip, Port: 0}) + require.NoError(t, err) + t.Cleanup(func() { _ = conn.Close() }) + return conn +} + +func testInterface(name string, ip net.IP) *transport.Interface { + ifc := transport.NewInterface(net.Interface{Index: 1, MTU: 1500, Name: name, Flags: net.FlagUp | net.FlagMulticast}) + ifc.AddAddress(&net.IPNet{IP: ip, Mask: net.CIDRMask(24, 32)}) + return ifc +} + +func TestSingleSocketUDPMux_ConcurrentListenAddressesWithDefaultNet(t *testing.T) { + conn := listenUDP(t, net.IPv4zero) + mux := NewSingleSocketUDPMux(Params{UDPConn: conn}) + t.Cleanup(func() { _ = mux.Close() }) + + var wg sync.WaitGroup + for i := 0; i < 8; i++ { + wg.Add(1) + go func() { + defer wg.Done() + assert.NotEmpty(t, mux.GetListenAddresses(), "every caller gets at least the socket address") + }() + } + wg.Wait() +} 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/engine.go b/client/internal/engine.go index fdf89ae06..c4bdf5121 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -387,7 +387,7 @@ func NewEngine( mgmClient: services.MgmClient, relayManager: services.RelayManager, peerStore: peerstore.NewConnStore(), - wgDetector: stdnet.NewWGDetector(), + wgDetector: stdnet.NewWGDetector(config.WgIfaceName), syncMsgMux: &sync.Mutex{}, config: config, mobileDep: mobileDep, diff --git a/client/internal/networkmonitor/check_change_linux.go b/client/internal/networkmonitor/check_change_linux.go index efd8b5884..4fbff4448 100644 --- a/client/internal/networkmonitor/check_change_linux.go +++ b/client/internal/networkmonitor/check_change_linux.go @@ -6,6 +6,7 @@ import ( "context" "errors" "fmt" + "net" "syscall" log "github.com/sirupsen/logrus" @@ -35,7 +36,7 @@ func checkChange(ctx context.Context, nexthopv4, nexthopv6 systemops.Nexthop) er // handle route changes case route := <-routeChan: // default route and main table - if route.Dst != nil || route.Table != syscall.RT_TABLE_MAIN { + if !isDefaultRoute(route.Dst) || route.Table != syscall.RT_TABLE_MAIN { continue } switch route.Type { @@ -52,3 +53,13 @@ func checkChange(ctx context.Context, nexthopv4, nexthopv6 systemops.Nexthop) er } } } + +// isDefaultRoute reports whether dst is a default route. netlink v1.3 reports +// the default as 0.0.0.0/0 or ::/0, older versions as a nil Dst. +func isDefaultRoute(dst *net.IPNet) bool { + if dst == nil { + return true + } + ones, _ := dst.Mask.Size() + return ones == 0 +} diff --git a/client/internal/networkmonitor/check_change_linux_test.go b/client/internal/networkmonitor/check_change_linux_test.go new file mode 100644 index 000000000..13e1c9e6d --- /dev/null +++ b/client/internal/networkmonitor/check_change_linux_test.go @@ -0,0 +1,43 @@ +//go:build !android + +package networkmonitor + +import ( + "net" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestIsDefaultRoute(t *testing.T) { + mustCIDR := func(s string) *net.IPNet { + t.Helper() + _, n, err := net.ParseCIDR(s) + if err != nil { + t.Fatalf("parse %s: %v", s, err) + } + return n + } + + tests := []struct { + name string + dst *net.IPNet + want bool + }{ + // netlink before v1.3 left Dst nil for the default route. + {name: "nil dst", dst: nil, want: true}, + // netlink v1.3 reports the default route with an explicit zero-length prefix. + {name: "v4 default", dst: mustCIDR("0.0.0.0/0"), want: true}, + {name: "v6 default", dst: mustCIDR("::/0"), want: true}, + // The exit node split routes and ordinary prefixes are not defaults. + {name: "v4 split half", dst: mustCIDR("0.0.0.0/1"), want: false}, + {name: "v4 subnet", dst: mustCIDR("10.0.0.0/8"), want: false}, + {name: "v4 host", dst: mustCIDR("1.1.1.1/32"), want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, isDefaultRoute(tt.dst), "default route detection") + }) + } +} 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/client/internal/routemanager/client/client.go b/client/internal/routemanager/client/client.go index b882c4d4a..66c2ed491 100644 --- a/client/internal/routemanager/client/client.go +++ b/client/internal/routemanager/client/client.go @@ -65,6 +65,12 @@ const ( // well above any usable path so that slow paths keep their order. unknownLatency = 10 * time.Second + // unmeasuredLatencyHold is how long a current routing peer without a + // latency sample is kept against a measured one. A direct connection is + // sampled within seconds, so a peer still unsampled after this is ranked + // at unknownLatency rather than held indefinitely. + unmeasuredLatencyHold = 30 * time.Second + // holdReportInterval is how long a routing peer held back by a switch rule // is reported at debug level only, after being reported at info level once. holdReportInterval = 10 * time.Minute @@ -99,6 +105,7 @@ type holdKind int const ( holdLatencyMargin holdKind = iota holdLatencyDwell + holdLatencyUnmeasured holdRelayedDelay ) @@ -124,6 +131,9 @@ type routeCandidate struct { relayed bool latency time.Duration noise time.Duration + // measured is false while the peer has no latency sample, as right after a + // direct connection comes up: latency then holds unknownLatency + measured bool } func newRouteCandidate(r *route.Route, status routerPeerStatus) routeCandidate { @@ -140,6 +150,7 @@ func newRouteCandidate(r *route.Route, status routerPeerStatus) routeCandidate { relayed: status.relayed, latency: latency, noise: status.noise, + measured: status.latency > 0, } } @@ -230,6 +241,9 @@ type WatcherConfig struct { StatusRecorder *peer.Status Route *route.Route Handler RouteHandler + // ReconcileAllowedIPs re-applies the routed allowed IPs tracked for a + // routing peer onto the interface. Optional. + ReconcileAllowedIPs func(peerKey string) error } // Watcher watches route and peer changes and updates allowed IPs accordingly. @@ -271,11 +285,20 @@ type Watcher struct { trackedCurrent route.ID trackedPeer string trackedDirect bool - // degradedSince is when the current routing peer dropped from direct to - // relayed, zero if it is direct or was already relayed when chosen + // degradedSince is when the relayedSwitch delay of the current relayed + // routing peer started: its drop from direct, or the first direct contender + // if it was never direct. Zero while it is direct or uncontended. degradedSince time.Time - // relayedHold fires when a relayed switch being held back becomes due, so it - // is applied on time rather than on the next periodic re-evaluation + // unmeasuredRoute and unmeasuredSince track since when the current route's + // peer has had no latency sample, bounding the hold in shouldSwitchRoute + unmeasuredRoute route.ID + unmeasuredSince time.Time + // neverDirect marks a degradedSince started by a direct contender rather + // than by a drop from direct, which the "never" policy does not cover + neverDirect bool + // relayedHold fires when a switch held back by the relayed delay or the + // unmeasured latency hold becomes due, so it is applied on time rather than + // on the next periodic re-evaluation relayedHold *time.Timer // heldCandidate and heldKind identify the alternative last held back by the // switch rules and the rule holding it, and heldReported is when that was @@ -298,6 +321,9 @@ type Watcher struct { stopped bool handler RouteHandler updateSerial uint64 + // reconcileAllowedIPs re-applies a routing peer's routed allowed IPs when + // it connects while keeping the route, see keepChosenRoute. May be nil. + reconcileAllowedIPs func(peerKey string) error } func newPeerStateUpdate() chan struct{} { @@ -320,6 +346,7 @@ func NewWatcher(config WatcherConfig) *Watcher { handler: config.Handler, currentChosenStatus: nil, relayedSwitch: relayedSwitch(), + reconcileAllowedIPs: config.ReconcileAllowedIPs, } return client } @@ -393,6 +420,8 @@ func (w *Watcher) getBestRouteFromStatuses(routePeerStatuses map[route.ID]router } w.trackRelayed(current, haveCurrent) + w.trackUnmeasured(current, haveCurrent) + w.trackContender(current, best, haveCurrent && haveBest) if !haveBest { w.reportUnassigned() @@ -449,9 +478,8 @@ func (w *Watcher) reportUnassigned() { // trackRelayed records when the current routing peer dropped from a direct to a // relayed connection while carrying the route, which starts the relayedSwitch -// delay. A peer that was never direct, because it was relayed when chosen or -// came up over relay from idle, is not degraded: holding on to it would only -// keep the route on the slower path. +// delay at once. A peer that was never direct starts it later, when a direct +// peer first contends for the route, see relayedSwitchDue. func (w *Watcher) trackRelayed(current routeCandidate, haveCurrent bool) { if !haveCurrent { w.trackedCurrent, w.trackedPeer, w.trackedDirect, w.degradedSince = "", "", false, time.Time{} @@ -462,7 +490,7 @@ func (w *Watcher) trackRelayed(current routeCandidate, haveCurrent bool) { case current.id != w.trackedCurrent, current.peer != w.trackedPeer, !current.relayed: w.degradedSince = time.Time{} case w.trackedDirect: - w.degradedSince = time.Now() + w.degradedSince, w.neverDirect = time.Now(), false // a new degradation, reported as such even if an earlier one held // back the same candidate w.heldCandidate = "" @@ -494,6 +522,20 @@ func (w *Watcher) shouldSwitchRoute(current, candidate routeCandidate) bool { return true } + // Without a sample on both sides there is no gain to weigh. A direct + // connection reports no latency until its first sample, so comparing it as + // unknownLatency would move the route away from a peer that just recovered. + // The hold is bounded so a peer that never gets a sample does not keep the + // route for good. + if w.unmeasuredHoldActive(current, candidate) { + if level := w.holdLevel(candidate, holdLatencyUnmeasured); log.IsLevelEnabled(level) { + log.StandardLogger().Logf(level, + "keeping routing peer %s for [%v] over %s: latency not measured on both peers yet (%s)", + current.peer, w.handler, candidate.peer, candidate) + } + return false + } + gain := current.latency - candidate.latency if margin := switchMargin(current, candidate); gain < margin { if level := w.holdLevel(candidate, holdLatencyMargin); log.IsLevelEnabled(level) { @@ -518,22 +560,75 @@ func (w *Watcher) shouldSwitchRoute(current, candidate routeCandidate) bool { return true } +// trackContender restarts the relayed delay of a peer that was never direct +// once no direct contender is left. That delay runs from the first contender, +// so keeping it would let a later contender take over without waiting. The +// delay of a peer that dropped from direct keeps running from the drop. +func (w *Watcher) trackContender(current, best routeCandidate, have bool) { + if !w.neverDirect || w.degradedSince.IsZero() { + return + } + if have && best.winsOnlyOnRelay(current) { + return + } + w.degradedSince = time.Time{} +} + +// trackUnmeasured records since when the current route's peer has had no +// latency sample, and forgets it once the peer is measured or the route moves. +func (w *Watcher) trackUnmeasured(current routeCandidate, haveCurrent bool) { + if !haveCurrent || current.measured || current.id != w.unmeasuredRoute { + w.unmeasuredRoute, w.unmeasuredSince = "", time.Time{} + } + if haveCurrent && !current.measured && w.unmeasuredSince.IsZero() { + w.unmeasuredRoute, w.unmeasuredSince = current.id, time.Now() + } +} + +// unmeasuredHoldActive reports whether a latency comparison must wait because +// a side has no sample: always for an unsampled candidate, and for an unsampled +// current peer until unmeasuredLatencyHold runs out. A pending hold arms +// relayedHold so the switch is reconsidered when it runs out. +func (w *Watcher) unmeasuredHoldActive(current, candidate routeCandidate) bool { + if !candidate.measured { + return true + } + if current.measured { + return false + } + + remaining := unmeasuredLatencyHold - time.Since(w.unmeasuredSince) + if remaining <= 0 { + return false + } + w.armRelayedHold(remaining) + + return true +} + // relayedSwitchDue reports whether a relayed current routing peer should give // way to a candidate that is better only by being direct. // -// A peer that dropped from direct to relayed while carrying the route is kept -// for the relayedSwitch delay: the relay still carries the traffic, staying -// keeps every flow through this routing peer alive, and a direct connection that -// dropped often recovers within seconds. A peer that was already relayed when -// chosen gives way at once, so the route is not stuck on the slower path when a -// direct one becomes available. A switch that is not due yet arms relayedHold so -// it is applied as soon as it becomes due. +// A relayed peer is kept for the relayedSwitch delay: the relay still carries +// the traffic, and staying keeps every flow through this routing peer alive. +// For a peer that dropped from direct to relayed the delay runs from the drop, +// since a direct connection that dropped often recovers within seconds. For a +// peer that was never direct, as when the peers of a group come up over relay +// before their direct connections, it runs from the first evaluation that +// finds a direct contender: the current peer usually turns direct within +// seconds too. A switch that is not due yet arms relayedHold so it is applied +// as soon as it becomes due. func (w *Watcher) relayedSwitchDue(current, candidate routeCandidate) bool { if w.degradedSince.IsZero() { - return true + w.degradedSince, w.neverDirect = time.Now(), true } policy := w.relayedSwitch + // "never" keeps a peer whose direct connection dropped; one that was never + // direct still moves after the default delay instead of staying relayed + if policy.never && w.neverDirect { + policy = relayedSwitchPolicy{delay: defaultRelayedSwitchDelay} + } if !policy.never { remaining := policy.delay - time.Since(w.degradedSince) if remaining <= 0 { @@ -544,7 +639,7 @@ func (w *Watcher) relayedSwitchDue(current, candidate routeCandidate) bool { if level := w.holdLevel(candidate, holdRelayedDelay); log.IsLevelEnabled(level) { log.StandardLogger().Logf(level, - "keeping relayed routing peer %s for [%v] over direct %s: dropped to relay %v ago, switch delay %s", + "keeping relayed routing peer %s for [%v] over direct %s: held for %v, switch delay %s", current.peer, w.handler, candidate.peer, time.Since(w.degradedSince).Round(time.Second), policy) } @@ -677,21 +772,46 @@ func (w *Watcher) removeAllowedIPs(route *route.Route, rsn reason) error { return nil } -// shouldSkipRecalculation checks if we can skip route recalculation for the same route without status changes -func (w *Watcher) shouldSkipRecalculation(newChosenID route.ID, newStatus routerPeerStatus) bool { - if w.currentChosen == nil { +// keepChosenRoute reports whether the evaluation keeps the current route, with +// the same definition, on the same routing peer. A status change of that peer +// is recorded without removing and re-adding its allowed IPs, which would leave +// the prefix without an owner in between. When the peer connects, its routed +// allowed IPs are re-applied instead: a peer recreated by a peer update comes +// back with its overlay addresses only. +func (w *Watcher) keepChosenRoute(newChosenID route.ID, newStatus routerPeerStatus) bool { + if w.currentChosen == nil || w.currentChosen.ID != newChosenID { return false } - isSameRoute := w.currentChosen.ID == newChosenID && w.currentChosen.Equal(w.routes[newChosenID]) - if !isSameRoute { + newChosenRoute := w.routes[newChosenID] + if !w.currentChosen.Equal(newChosenRoute) { return false } - if w.currentChosenStatus != nil { - return w.currentChosenStatus.status == newStatus.status + if w.currentChosenStatus == nil || w.currentChosenStatus.status == newStatus.status { + return true } + // a failed reconcile leaves the previous status recorded, so the next + // evaluation sees the change again and retries + if newStatus.status == peer.StatusConnected && !w.reconcileRoutedAllowedIPs(newChosenRoute.Peer) { + return true + } + + w.currentChosen = newChosenRoute + w.currentChosenStatus = &newStatus + + return true +} + +func (w *Watcher) reconcileRoutedAllowedIPs(peerKey string) bool { + if w.reconcileAllowedIPs == nil { + return true + } + if err := w.reconcileAllowedIPs(peerKey); err != nil { + log.Warnf("failed to reconcile routed allowed IPs of routing peer %s for [%v], will retry: %v", peerKey, w.handler, err) + return false + } return true } @@ -719,8 +839,7 @@ func (w *Watcher) recalculateRoutes(rsn reason, routerPeerStatuses map[route.ID] return nil } - // If we can skip recalculation for the same route without changes, do nothing - if w.shouldSkipRecalculation(newChosenID, newStatus) { + if w.keepChosenRoute(newChosenID, newStatus) { return nil } diff --git a/client/internal/routemanager/client/client_test.go b/client/internal/routemanager/client/client_test.go index 764a2f74d..641383e90 100644 --- a/client/internal/routemanager/client/client_test.go +++ b/client/internal/routemanager/client/client_test.go @@ -2,6 +2,7 @@ package client import ( "context" + "errors" "fmt" "net/netip" "sync" @@ -1480,3 +1481,84 @@ func TestLatencyJitterDoesNotSwitchRoute(t *testing.T) { require.Equal(t, route.ID("route1"), chosen, "route changed on jitter at sample %d", i) } } + +// A routing peer that keeps the route while only its connection status changes, +// as when an idle peer is woken by traffic, must keep its allowed IPs in place: +// removing and re-adding them leaves the prefix without an owner in between. +func TestRecalculateRoutesStatusChangeKeepsAllowedIPs(t *testing.T) { + w, handler := newRecalcWatcher(t) + + require.NoError(t, w.recalculateRoutes(reasonPeerUpdate, map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusIdle}, + })) + require.Equal(t, []string{"peer1"}, handler.added) + + require.NoError(t, w.recalculateRoutes(reasonPeerUpdate, map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusConnected, relayed: true}, + })) + assert.Equal(t, []string{"peer1"}, handler.added, "a status change must not re-add the allowed IPs") + assert.Equal(t, 0, handler.removed, "a status change must not remove the allowed IPs") + require.NotNil(t, w.currentChosenStatus) + assert.Equal(t, peer.StatusConnected, w.currentChosenStatus.status, "the new status must be recorded") +} + +// A routing peer recreated by a peer update comes back idle with its overlay +// addresses only. When it connects while keeping the route, its routed allowed +// IPs must be re-applied, since nothing else puts them back on the new peer. +func TestRecalculateRoutesReconcilesAllowedIPsOnConnect(t *testing.T) { + w, handler := newRecalcWatcher(t) + var reconciled []string + w.reconcileAllowedIPs = func(peerKey string) error { + reconciled = append(reconciled, peerKey) + return nil + } + + require.NoError(t, w.recalculateRoutes(reasonPeerUpdate, map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusConnected}, + })) + require.Equal(t, []string{"peer1"}, handler.added) + require.Empty(t, reconciled, "a fresh assignment adds the allowed IPs itself") + + require.NoError(t, w.recalculateRoutes(reasonPeerUpdate, map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusIdle}, + })) + assert.Empty(t, reconciled, "going idle must not reconcile") + + require.NoError(t, w.recalculateRoutes(reasonPeerUpdate, map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusConnected}, + })) + assert.Equal(t, []string{"peer1"}, reconciled, "connecting while keeping the route must reconcile its allowed IPs") + assert.Equal(t, []string{"peer1"}, handler.added, "the reconcile must not go through the refcounted add") + assert.Equal(t, 0, handler.removed, "the allowed IPs must not be removed") +} + +// A failed reconcile must be retried: the new status is only recorded once the +// routed allowed IPs are back, so the next evaluation tries again. +func TestRecalculateRoutesRetriesFailedReconcile(t *testing.T) { + w, _ := newRecalcWatcher(t) + calls := 0 + w.reconcileAllowedIPs = func(string) error { + calls++ + if calls == 1 { + return errors.New("add allowed IP: transient failure") + } + return nil + } + + connected := map[route.ID]routerPeerStatus{"route1": {status: peer.StatusConnected}} + require.NoError(t, w.recalculateRoutes(reasonPeerUpdate, connected)) + require.NoError(t, w.recalculateRoutes(reasonPeerUpdate, map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusIdle}, + })) + + require.NoError(t, w.recalculateRoutes(reasonPeerUpdate, connected)) + require.Equal(t, 1, calls, "connecting must reconcile") + assert.Equal(t, peer.StatusIdle, w.currentChosenStatus.status, "a failed reconcile must not record the new status") + + require.NoError(t, w.recalculateRoutes(reasonPeerUpdate, connected)) + assert.Equal(t, 2, calls, "the next evaluation must retry the reconcile") + assert.Equal(t, peer.StatusConnected, w.currentChosenStatus.status, "a successful reconcile records the status") + + require.NoError(t, w.recalculateRoutes(reasonPeerUpdate, connected)) + assert.Equal(t, 2, calls, "nothing to retry once the reconcile succeeded") +} diff --git a/client/internal/routemanager/client/env.go b/client/internal/routemanager/client/env.go index 398667cdf..6a7c08a1f 100644 --- a/client/internal/routemanager/client/env.go +++ b/client/internal/routemanager/client/env.go @@ -12,10 +12,11 @@ import ( const ( // envRelayedSwitchDelay sets how long a routing peer that dropped from a // direct to a relayed connection while carrying the route is kept before the - // route moves to an equivalent peer with a direct connection. Takes a - // duration ("30s", "0" to switch at once) or "never" to only leave such a - // peer when it disconnects. A routing peer that was already relayed when - // chosen always gives way to a direct one at once. + // route moves to an equivalent peer with a direct connection. A routing peer + // that was never direct is kept for the same delay from when a direct peer + // first contends for the route. Takes a duration ("30s", "0" to switch at + // once) or "never" to only leave a peer that dropped from direct when it + // disconnects; a peer that was never direct then uses the default delay. envRelayedSwitchDelay = "NB_HA_RELAYED_SWITCH_DELAY" relayedSwitchDelayNever = "never" diff --git a/client/internal/routemanager/client/relayed_switch_test.go b/client/internal/routemanager/client/relayed_switch_test.go index 8f4a67bba..f70dfff50 100644 --- a/client/internal/routemanager/client/relayed_switch_test.go +++ b/client/internal/routemanager/client/relayed_switch_test.go @@ -103,11 +103,11 @@ func TestDegradedRoutingPeerIsReplacedAfterDelay(t *testing.T) { assert.Equal(t, switchReasonDirect, w.switchReason, "the switch reason must name the relayed connection") } -// A routing peer that was relayed when it was chosen is not degraded: when an -// equivalent direct peer shows up the route must upgrade at once instead of -// staying on the slower path for the delay. -func TestRelayedRoutingPeerUpgradesAtOnce(t *testing.T) { - w := newRelayedTestWatcher(t, relayedSwitchPolicy{never: true}) +// A routing peer that was relayed when it was chosen is held for the delay when +// an equivalent direct peer shows up, from that moment on: it usually turns +// direct within seconds, and moving the route resets its masqueraded flows. +func TestRelayedRoutingPeerIsKeptDuringDelay(t *testing.T) { + w := newRelayedTestWatcher(t, relayedSwitchPolicy{delay: 30 * time.Second}) // route1 was chosen while relayed and route2 was still connecting chosen, _ := w.getBestRouteFromStatuses(map[route.ID]routerPeerStatus{ @@ -115,36 +115,57 @@ func TestRelayedRoutingPeerUpgradesAtOnce(t *testing.T) { "route2": {status: peer.StatusConnecting}, }) require.Equal(t, route.ID("route1"), chosen) - require.True(t, w.degradedSince.IsZero(), "a peer relayed from the start is not degraded") + require.True(t, w.degradedSince.IsZero(), "the delay must not run before a direct peer contends") chosen, _ = w.getBestRouteFromStatuses(currentRelayed) - assert.Equal(t, route.ID("route2"), chosen, "a direct peer must take over from one that was relayed all along") - assert.Equal(t, switchReasonDirect, w.switchReason) - assert.Nil(t, w.relayedHold, "an upgrade must not schedule anything") + assert.Equal(t, route.ID("route1"), chosen, "a relayed peer must be kept when a direct one shows up") + assert.False(t, w.degradedSince.IsZero(), "the direct contender must start the delay") + assert.NotNil(t, w.relayedHold, "the pending switch must arm a timer so it is applied when due") + + w.degradedSince = w.degradedSince.Add(-31 * time.Second) + chosen, _ = w.getBestRouteFromStatuses(currentRelayed) + assert.Equal(t, route.ID("route2"), chosen, "a peer still relayed after the delay must give way to a direct one") + assert.Equal(t, switchReasonDirect, w.switchReason, "the switch reason must name the relayed connection") } -// An idle routing peer woken by traffic whose relay connection comes up before -// the direct one was never direct: it is not degraded, and must give way to an -// equivalent direct peer at once. -func TestIdleRoutingPeerWokenOverRelayUpgradesAtOnce(t *testing.T) { - w := newRelayedTestWatcher(t, relayedSwitchPolicy{never: true}) +// The peers of an HA group woken from idle come up over relay first and turn +// direct one after the other. The route must stay on the peer it was on when +// the other one turns direct first and the current one follows within the +// delay, including before the current peer's first latency sample. +func TestIdleRoutingPeersWokenOverRelayKeepRoute(t *testing.T) { + w := newRelayedTestWatcher(t, relayedSwitchPolicy{delay: 30 * time.Second}) - chosen, _ := w.getBestRouteFromStatuses(map[route.ID]routerPeerStatus{ - "route1": {status: peer.StatusIdle}, - "route2": {status: peer.StatusIdle}, - }) - require.Equal(t, route.ID("route1"), chosen) + steps := []struct { + name string + statuses map[route.ID]routerPeerStatus + }{ + {"both idle", map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusIdle}, + "route2": {status: peer.StatusIdle}, + }}, + {"both relayed", map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusConnected, relayed: true}, + "route2": {status: peer.StatusConnected, relayed: true}, + }}, + {"other direct first", map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusConnected, relayed: true}, + "route2": {status: peer.StatusConnected, latency: 20 * time.Millisecond}, + }}, + {"current direct, not sampled", map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusConnected}, + "route2": {status: peer.StatusConnected, latency: 20 * time.Millisecond}, + }}, + {"current direct, sampled", map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusConnected, latency: 38 * time.Millisecond}, + "route2": {status: peer.StatusConnected, latency: 20 * time.Millisecond}, + }}, + } - chosen, _ = w.getBestRouteFromStatuses(map[route.ID]routerPeerStatus{ - "route1": {status: peer.StatusConnected, relayed: true}, - "route2": {status: peer.StatusConnecting}, - }) - require.Equal(t, route.ID("route1"), chosen) - require.True(t, w.degradedSince.IsZero(), "waking up over relay is not a drop from direct") - - chosen, _ = w.getBestRouteFromStatuses(currentRelayed) - assert.Equal(t, route.ID("route2"), chosen, "a direct peer must take over from one that was never direct") - assert.Nil(t, w.relayedHold, "an upgrade must not schedule anything") + for _, step := range steps { + chosen, _ := w.getBestRouteFromStatuses(step.statuses) + require.Equal(t, route.ID("route1"), chosen, "step %q must keep the route on route1", step.name) + } + assert.True(t, w.degradedSince.IsZero(), "the delay must end once the current peer is direct") } // A routing peer that drops to relay again, after recovering from an earlier @@ -356,3 +377,128 @@ func TestRoutingPeerSwitchPublishesEvent(t *testing.T) { assert.Equal(t, "No routing peer available", events[1].Message) assert.Equal(t, "peer2", events[1].Metadata["previous_peer"]) } + +// The statuses below follow the status recorder: a drop to relay clears the +// latency, and a recovered direct connection reports no latency until the new +// ICE agent's first sample lands. +var ( + flapDirect = map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusConnected, latency: 50 * time.Millisecond}, + "route2": {status: peer.StatusConnected, latency: 60 * time.Millisecond}, + } + flapRelayed = map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusConnected, relayed: true}, + "route2": {status: peer.StatusConnected, latency: 60 * time.Millisecond}, + } + flapRecoveredUnsampled = map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusConnected}, + "route2": {status: peer.StatusConnected, latency: 60 * time.Millisecond}, + } +) + +// A routing peer whose direct connection drops to relay and comes back within +// the delay must keep the route, including the window between the recovery and +// the first latency sample of the new connection. +func TestRecoveredRoutingPeerWithoutLatencySampleIsKept(t *testing.T) { + w := newRelayedTestWatcher(t, relayedSwitchPolicy{delay: 30 * time.Second}) + + chosen, _ := w.getBestRouteFromStatuses(flapDirect) + require.Equal(t, route.ID("route1"), chosen) + + chosen, _ = w.getBestRouteFromStatuses(flapRelayed) + require.Equal(t, route.ID("route1"), chosen, "a routing peer that just dropped to relay must be kept") + + chosen, _ = w.getBestRouteFromStatuses(flapRecoveredUnsampled) + assert.Equal(t, route.ID("route1"), chosen, "a recovered routing peer must not lose the route before its latency is sampled") +} + +// A routing peer that keeps dropping to relay for less than the delay, and +// staying direct for less than the delay in between, must keep the route on +// every cycle: each drop is a new degradation with its own delay. +func TestFlappingRoutingPeerIsKept(t *testing.T) { + w := newRelayedTestWatcher(t, relayedSwitchPolicy{delay: 30 * time.Second}) + + chosen, _ := w.getBestRouteFromStatuses(flapDirect) + require.Equal(t, route.ID("route1"), chosen) + + for cycle := 0; cycle < 3; cycle++ { + chosen, _ = w.getBestRouteFromStatuses(flapRelayed) + require.Equal(t, route.ID("route1"), chosen, "cycle %d: drop to relay", cycle) + + // relayed for 25s, still within the delay + w.degradedSince = w.degradedSince.Add(-25 * time.Second) + chosen, _ = w.getBestRouteFromStatuses(flapRelayed) + require.Equal(t, route.ID("route1"), chosen, "cycle %d: relayed within the delay", cycle) + + chosen, _ = w.getBestRouteFromStatuses(flapRecoveredUnsampled) + require.Equal(t, route.ID("route1"), chosen, "cycle %d: recovered, latency not sampled yet", cycle) + + chosen, _ = w.getBestRouteFromStatuses(flapDirect) + require.Equal(t, route.ID("route1"), chosen, "cycle %d: recovered and sampled", cycle) + } +} + +// "never" only keeps a routing peer that dropped from direct. A peer that was +// never direct must still move to a direct one after the default delay, so the +// route is not left on a relay for good. +func TestNeverPolicyDoesNotPinNeverDirectPeer(t *testing.T) { + w := newRelayedTestWatcher(t, relayedSwitchPolicy{never: true}) + + chosen, _ := w.getBestRouteFromStatuses(map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusConnected, relayed: true}, + "route2": {status: peer.StatusConnecting}, + }) + require.Equal(t, route.ID("route1"), chosen) + + chosen, _ = w.getBestRouteFromStatuses(currentRelayed) + require.Equal(t, route.ID("route1"), chosen, "a never-direct peer must be held for the default delay") + require.NotNil(t, w.relayedHold, "the default delay must arm the timer") + + w.degradedSince = w.degradedSince.Add(-defaultRelayedSwitchDelay - time.Second) + chosen, _ = w.getBestRouteFromStatuses(currentRelayed) + assert.Equal(t, route.ID("route2"), chosen, "with never, a never-direct peer must still give way after the default delay") +} + +// A current peer that never gets a latency sample must not keep the route +// forever against a measured peer: once the hold runs out it is ranked as +// unknown latency again. +func TestUnmeasuredRoutingPeerHoldIsBounded(t *testing.T) { + w := newRelayedTestWatcher(t, relayedSwitchPolicy{delay: 30 * time.Second}) + + chosen, _ := w.getBestRouteFromStatuses(flapRecoveredUnsampled) + require.Equal(t, route.ID("route1"), chosen, "an unsampled current peer must be held at first") + require.False(t, w.unmeasuredSince.IsZero(), "the hold must record when it started") + + w.unmeasuredSince = w.unmeasuredSince.Add(-unmeasuredLatencyHold - time.Second) + chosen, _ = w.getBestRouteFromStatuses(flapRecoveredUnsampled) + assert.Equal(t, route.ID("route2"), chosen, "a peer still unsampled after the hold must give way to a measured one") + assert.Equal(t, switchReasonLatency, w.switchReason) +} + +// The delay of a peer that was never direct runs from the first direct +// contender. When that contender goes away the delay must start over, so a +// later contender is held for the full delay instead of taking over at once. +func TestNeverDirectDelayRestartsWhenContenderGoes(t *testing.T) { + w := newRelayedTestWatcher(t, relayedSwitchPolicy{delay: 30 * time.Second}) + + relayedAlone := map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusConnected, relayed: true}, + "route2": {status: peer.StatusConnecting}, + } + chosen, _ := w.getBestRouteFromStatuses(relayedAlone) + require.Equal(t, route.ID("route1"), chosen) + + chosen, _ = w.getBestRouteFromStatuses(currentRelayed) + require.Equal(t, route.ID("route1"), chosen, "the first direct contender must be held") + require.False(t, w.degradedSince.IsZero()) + + // the first contender was seen longer ago than the delay + w.degradedSince = w.degradedSince.Add(-31 * time.Second) + + chosen, _ = w.getBestRouteFromStatuses(relayedAlone) + require.Equal(t, route.ID("route1"), chosen) + assert.True(t, w.degradedSince.IsZero(), "the delay must start over once no direct contender is left") + + chosen, _ = w.getBestRouteFromStatuses(currentRelayed) + assert.Equal(t, route.ID("route1"), chosen, "a new direct contender must be held for the full delay") +} diff --git a/client/internal/routemanager/manager.go b/client/internal/routemanager/manager.go index 981b0c987..7d699fbb4 100644 --- a/client/internal/routemanager/manager.go +++ b/client/internal/routemanager/manager.go @@ -578,12 +578,13 @@ func (m *DefaultManager) TriggerSelection(networks route.HAMap) { } config := client.WatcherConfig{ - Context: m.ctx, - DNSRouteInterval: m.dnsRouteInterval, - WGInterface: m.wgInterface, - StatusRecorder: m.statusRecorder, - Route: routes[0], - Handler: handler, + Context: m.ctx, + DNSRouteInterval: m.dnsRouteInterval, + WGInterface: m.wgInterface, + StatusRecorder: m.statusRecorder, + Route: routes[0], + Handler: handler, + ReconcileAllowedIPs: m.ReconcilePeerAllowedIPs, } clientNetworkWatcher := client.NewWatcher(config) m.clientNetworks[id] = clientNetworkWatcher @@ -628,12 +629,13 @@ func (m *DefaultManager) updateClientNetworks(updateSerial uint64, networks rout } config := client.WatcherConfig{ - Context: m.ctx, - DNSRouteInterval: m.dnsRouteInterval, - WGInterface: m.wgInterface, - StatusRecorder: m.statusRecorder, - Route: routes[0], - Handler: handler, + Context: m.ctx, + DNSRouteInterval: m.dnsRouteInterval, + WGInterface: m.wgInterface, + StatusRecorder: m.statusRecorder, + Route: routes[0], + Handler: handler, + ReconcileAllowedIPs: m.ReconcilePeerAllowedIPs, } clientNetworkWatcher = client.NewWatcher(config) m.clientNetworks[id] = clientNetworkWatcher diff --git a/client/internal/stdnet/wgdetector.go b/client/internal/stdnet/wgdetector.go index f34e469e1..8ed290190 100644 --- a/client/internal/stdnet/wgdetector.go +++ b/client/internal/stdnet/wgdetector.go @@ -34,6 +34,8 @@ type WGDetector struct { // probe is replaced in tests; it is the call this type exists to avoid repeating. probe func(string) bool + ownIface string + mu sync.RWMutex cache map[string]wgDetectorEntry @@ -41,11 +43,12 @@ type WGDetector struct { } // NewWGDetector returns a detector with the default time to live. -func NewWGDetector() *WGDetector { +func NewWGDetector(ownIface string) *WGDetector { return &WGDetector{ - ttl: wgDetectorTTL, - probe: probeWireGuard, - cache: make(map[string]wgDetectorEntry), + ttl: wgDetectorTTL, + probe: probeWireGuard, + ownIface: ownIface, + cache: make(map[string]wgDetectorEntry), } } @@ -57,6 +60,10 @@ func (d *WGDetector) IsWireGuard(iFace string) bool { return probeWireGuard(iFace) } + if d.isOwnInterface(iFace) { + return true + } + if isWireGuard, ok := d.cached(iFace); ok { return isWireGuard } @@ -104,6 +111,10 @@ func (d *WGDetector) store(iFace string, isWireGuard bool) { d.cache[iFace] = wgDetectorEntry{isWireGuard: isWireGuard, expireAt: now.Add(d.ttl)} } +func (d *WGDetector) isOwnInterface(iFace string) bool { + return d.ownIface != "" && iFace == d.ownIface +} + func probeWireGuard(iFace string) bool { wg, err := wgctrl.New() if err != nil { diff --git a/client/internal/stdnet/wgdetector_test.go b/client/internal/stdnet/wgdetector_test.go index 68260ae59..28cf85f6a 100644 --- a/client/internal/stdnet/wgdetector_test.go +++ b/client/internal/stdnet/wgdetector_test.go @@ -150,3 +150,39 @@ func TestWGDetectorDropsExpiredEntries(t *testing.T) { assert.Len(t, d.cache, 1, "expired entries of vanished interfaces must be dropped") assert.Contains(t, d.cache, "eth0", "the fresh answer must be kept") } + +func TestWGDetectorOwnInterfaceIsWireGuardWithoutProbing(t *testing.T) { + d, calls := newCountingDetector(t, time.Minute, false) + d.ownIface = "NetBird" + + assert.True(t, d.IsWireGuard("NetBird"), "the agent's own interface is a WireGuard device") + assert.Equal(t, int64(0), calls.Load(), "the own interface must never reach the probe") + + assert.False(t, d.IsWireGuard("NetBird2"), "only the exact own name is answered without probing") + assert.Equal(t, int64(1), calls.Load(), "a different interface is probed") +} + +func TestWGDetectorWithoutOwnInterfaceProbesEmptyName(t *testing.T) { + d, calls := newCountingDetector(t, time.Minute, false) + + assert.False(t, d.IsWireGuard(""), "an unnamed interface is not mistaken for the own one") + assert.Equal(t, int64(1), calls.Load(), "with no own interface set every name is probed") +} + +func TestInterfaceFilterRejectsOwnInterfaceRegardlessOfDisallowList(t *testing.T) { + d, calls := newCountingDetector(t, time.Minute, false) + d.ownIface = "NetBird" + filter := InterfaceFilter([]string{"wt"}, d) + + assert.False(t, filter("NetBird"), "the own interface is rejected even when the disallow list does not cover it") + assert.Equal(t, int64(0), calls.Load(), "the own interface is not probed") + + assert.True(t, filter("eth0"), "an ordinary interface stays available") + assert.Equal(t, int64(1), calls.Load(), "an ordinary interface is still probed") +} + +func TestNewWGDetectorKeepsTheOwnInterface(t *testing.T) { + d := NewWGDetector("NetbirdWT") + + assert.True(t, d.IsWireGuard("NetbirdWT"), "the constructor records the own interface") +} 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/management/internals/server/boot.go b/management/internals/server/boot.go index 4ced64a16..13bbaa6fa 100644 --- a/management/internals/server/boot.go +++ b/management/internals/server/boot.go @@ -204,7 +204,7 @@ func (s *BaseServer) IDPHandler() http.Handler { func (s *BaseServer) Router() *mux.Router { return Create(s, func() *mux.Router { router := mux.NewRouter() - router.Use(middleware.BuildMiddleware(s.RateLimiter(), s.AuthManager(), s.AccountManager(), s.Metrics(), s.IsValidChildAccount)...) + router.Use(middleware.BuildMiddleware(s.RateLimiter(), s.AuthManager(), s.AccountManager(), s.Metrics())...) return router }) } diff --git a/management/internals/server/modules.go b/management/internals/server/modules.go index f548e9238..3379becf6 100644 --- a/management/internals/server/modules.go +++ b/management/internals/server/modules.go @@ -248,7 +248,3 @@ func (s *BaseServer) ReverseProxyDomainManager() *manager.Manager { return &m }) } - -func (s *BaseServer) IsValidChildAccount(_ context.Context, _, _, _ string) bool { - return false -} diff --git a/management/internals/shared/grpc/peer_meta_test.go b/management/internals/shared/grpc/peer_meta_test.go new file mode 100644 index 000000000..c6336ff1b --- /dev/null +++ b/management/internals/shared/grpc/peer_meta_test.go @@ -0,0 +1,45 @@ +package grpc + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/netbirdio/netbird/shared/management/proto" +) + +func TestExtractPeerMeta_RemovesNulAndInvalidUTF8(t *testing.T) { + meta := extractPeerMeta(context.Background(), &proto.PeerSystemMeta{ + Hostname: "host\x00name", + OS: "\x00\x00\x00\x00\x00\x00s", + OSVersion: "11\xff", + SysSerialNumber: "SN\x00\x00", + Environment: &proto.Environment{Cloud: "\x00aws"}, + NetworkAddresses: []*proto.NetworkAddress{ + {NetIP: "192.168.0.1/24", Mac: "e0:d5\x00"}, + }, + Files: []*proto.File{{Path: "C:\\x\x00.exe"}}, + }) + + assert.Equal(t, "hostname", meta.Hostname) + assert.Equal(t, "s", meta.OS) + assert.Equal(t, "11", meta.OSVersion) + assert.Equal(t, "SN", meta.SystemSerialNumber) + assert.Equal(t, "aws", meta.Environment.Cloud) + assert.Equal(t, "e0:d5", meta.NetworkAddresses[0].Mac) + assert.Equal(t, "C:\\x.exe", meta.Files[0].Path) +} + +func TestCleanClientString(t *testing.T) { + tests := map[string]string{ + "": "", + "\x00\x00": "", + "Wïndows": "Wïndows", + "ssh-ed25519 AAA\x00": "ssh-ed25519 AAA", + "bad\xc3\x28utf8": "bad(utf8", + } + for in, want := range tests { + assert.Equal(t, want, cleanClientString(in), "input %q", in) + } +} diff --git a/management/internals/shared/grpc/proxy.go b/management/internals/shared/grpc/proxy.go index 8e93f77b0..439ff1dba 100644 --- a/management/internals/shared/grpc/proxy.go +++ b/management/internals/shared/grpc/proxy.go @@ -152,6 +152,10 @@ type ProxyServiceServer struct { // initial snapshot delivery. Configurable via NB_PROXY_SNAPSHOT_BATCH_SIZE. snapshotBatchSize int + // snapshotAckTimeout bounds the wait for each snapshot batch ack. + // Defaults to defaultSnapshotAckTimeout when zero. + snapshotAckTimeout time.Duration + cancel context.CancelFunc } @@ -168,6 +172,8 @@ const defaultProxyTokenTTL = 5 * time.Minute const defaultSnapshotBatchSize = 500 +const defaultSnapshotAckTimeout = 5 * time.Minute + func snapshotBatchSizeFromEnv() int { if v := os.Getenv("NB_PROXY_SNAPSHOT_BATCH_SIZE"); v != "" { if n, err := strconv.Atoi(v); err == nil && n > 0 { @@ -185,6 +191,14 @@ func (s *ProxyServiceServer) proxyTokenTTL() time.Duration { return defaultProxyTokenTTL } +// snapshotAckWait returns the configured snapshot ack timeout or the default when unset. +func (s *ProxyServiceServer) snapshotAckWait() time.Duration { + if s.snapshotAckTimeout > 0 { + return s.snapshotAckTimeout + } + return defaultSnapshotAckTimeout +} + // proxyConnection represents a connected proxy type proxyConnection struct { proxyID string @@ -484,7 +498,7 @@ func (s *ProxyServiceServer) SyncMappings(stream proto.ProxyService_SyncMappings return err } - if err := s.sendSnapshotSync(stream.Context(), conn, stream); err != nil { + if err := s.sendSnapshotSync(conn.ctx, conn, stream); err != nil { s.cleanupFailedSnapshot(stream.Context(), conn) return fmt.Errorf("send snapshot to proxy %s: %w", params.proxyID, err) } @@ -728,7 +742,7 @@ func (s *ProxyServiceServer) sendSnapshotSync(ctx context.Context, conn *proxyCo return fmt.Errorf("send snapshot batch: %w", err) } - if err := waitForAck(stream); err != nil { + if err := waitForAck(ctx, stream, s.snapshotAckWait()); err != nil { return err } } @@ -740,7 +754,7 @@ func (s *ProxyServiceServer) sendSnapshotSync(ctx context.Context, conn *proxyCo return fmt.Errorf("send snapshot completion: %w", err) } - if err := waitForAck(stream); err != nil { + if err := waitForAck(ctx, stream, s.snapshotAckWait()); err != nil { return err } } @@ -748,15 +762,38 @@ func (s *ProxyServiceServer) sendSnapshotSync(ctx context.Context, conn *proxyCo return nil } -func waitForAck(stream proto.ProxyService_SyncMappingsServer) error { - msg, err := stream.Recv() - if err != nil { - return fmt.Errorf("receive ack: %w", err) +// waitForAck waits for the proxy to ack a snapshot batch. It gives up when ctx +// is canceled or no ack arrives within timeout, since a proxy that abandoned the +// stream without closing it would otherwise block the handler indefinitely. +func waitForAck(ctx context.Context, stream proto.ProxyService_SyncMappingsServer, timeout time.Duration) error { + type recvResult struct { + msg *proto.SyncMappingsRequest + err error } - if msg.GetAck() == nil { - return fmt.Errorf("expected ack, got %T", msg.GetMsg()) + // Recv returns once the handler exits and gRPC tears the stream down. + result := make(chan recvResult, 1) + go func() { + msg, err := stream.Recv() + result <- recvResult{msg: msg, err: err} + }() + + timer := time.NewTimer(timeout) + defer timer.Stop() + + select { + case r := <-result: + if r.err != nil { + return fmt.Errorf("receive ack: %w", r.err) + } + if r.msg.GetAck() == nil { + return fmt.Errorf("expected ack, got %T", r.msg.GetMsg()) + } + return nil + case <-ctx.Done(): + return fmt.Errorf("wait for ack: %w", ctx.Err()) + case <-timer.C: + return fmt.Errorf("no ack within %s", timeout) } - return nil } // heartbeat updates the proxy's last_seen timestamp every minute and diff --git a/management/internals/shared/grpc/server.go b/management/internals/shared/grpc/server.go index 51023c34d..b48a86d55 100644 --- a/management/internals/shared/grpc/server.go +++ b/management/internals/shared/grpc/server.go @@ -553,9 +553,9 @@ func extractPeerMeta(ctx context.Context, meta *proto.PeerSystemMeta) nbpeer.Pee return nbpeer.PeerSystemMeta{} } - osVersion := meta.GetOSVersion() + osVersion := cleanClientString(meta.GetOSVersion()) if osVersion == "" { - osVersion = meta.GetCore() + osVersion = cleanClientString(meta.GetCore()) } networkAddresses := make([]nbpeer.NetworkAddress, 0, len(meta.GetNetworkAddresses())) @@ -567,36 +567,36 @@ func extractPeerMeta(ctx context.Context, meta *proto.PeerSystemMeta) nbpeer.Pee } networkAddresses = append(networkAddresses, nbpeer.NetworkAddress{ NetIP: netAddr, - Mac: addr.GetMac(), + Mac: cleanClientString(addr.GetMac()), }) } files := make([]nbpeer.File, 0, len(meta.GetFiles())) for _, file := range meta.GetFiles() { files = append(files, nbpeer.File{ - Path: file.GetPath(), + Path: cleanClientString(file.GetPath()), Exist: file.GetExist(), ProcessIsRunning: file.GetProcessIsRunning(), }) } return nbpeer.PeerSystemMeta{ - Hostname: meta.GetHostname(), - GoOS: meta.GetGoOS(), - Kernel: meta.GetKernel(), - Platform: meta.GetPlatform(), - OS: meta.GetOS(), + Hostname: cleanClientString(meta.GetHostname()), + GoOS: cleanClientString(meta.GetGoOS()), + Kernel: cleanClientString(meta.GetKernel()), + Platform: cleanClientString(meta.GetPlatform()), + OS: cleanClientString(meta.GetOS()), OSVersion: osVersion, - WtVersion: meta.GetNetbirdVersion(), - UIVersion: meta.GetUiVersion(), - KernelVersion: meta.GetKernelVersion(), + WtVersion: cleanClientString(meta.GetNetbirdVersion()), + UIVersion: cleanClientString(meta.GetUiVersion()), + KernelVersion: cleanClientString(meta.GetKernelVersion()), NetworkAddresses: networkAddresses, - SystemSerialNumber: meta.GetSysSerialNumber(), - SystemProductName: meta.GetSysProductName(), - SystemManufacturer: meta.GetSysManufacturer(), + SystemSerialNumber: cleanClientString(meta.GetSysSerialNumber()), + SystemProductName: cleanClientString(meta.GetSysProductName()), + SystemManufacturer: cleanClientString(meta.GetSysManufacturer()), Environment: nbpeer.Environment{ - Cloud: meta.GetEnvironment().GetCloud(), - Platform: meta.GetEnvironment().GetPlatform(), + Cloud: cleanClientString(meta.GetEnvironment().GetCloud()), + Platform: cleanClientString(meta.GetEnvironment().GetPlatform()), }, Flags: nbpeer.Flags{ RosenpassEnabled: meta.GetFlags().GetRosenpassEnabled(), @@ -618,6 +618,11 @@ func extractPeerMeta(ctx context.Context, meta *proto.PeerSystemMeta) nbpeer.Pee } } +// cleanClientString removes NUL bytes and invalid UTF-8 from a client-reported string, which Postgres rejects in text columns. +func cleanClientString(s string) string { + return strings.ToValidUTF8(strings.ReplaceAll(s, "\x00", ""), "") +} + func capabilitiesToInt32(caps []proto.PeerCapability) []int32 { result := make([]int32, len(caps)) for i, c := range caps { @@ -711,7 +716,7 @@ func (s *Server) Login(ctx context.Context, req *proto.EncryptedMessage) (*proto peer, network, postureChecks, enableSSH, err := s.accountManager.LoginPeer(ctx, types.PeerLogin{ WireGuardPubKey: peerKey.String(), - SSHKey: string(sshKey), + SSHKey: cleanClientString(string(sshKey)), Meta: peerMeta, UserID: userID, SetupKey: loginReq.GetSetupKey(), diff --git a/management/internals/shared/grpc/sync_mappings_test.go b/management/internals/shared/grpc/sync_mappings_test.go index 6949b79e9..753142ea0 100644 --- a/management/internals/shared/grpc/sync_mappings_test.go +++ b/management/internals/shared/grpc/sync_mappings_test.go @@ -409,3 +409,75 @@ func TestConnectionSendResponse_RoutesToLegacyStream(t *testing.T) { require.Len(t, stream.messages, 1) assert.Equal(t, "svc-2", stream.messages[0].Mapping[0].Id) } + +// blockingSyncStream is a ProxyService_SyncMappingsServer whose Recv blocks +// until the test ends, like a proxy that stopped acking without closing the stream. +type blockingSyncStream struct { + grpc.ServerStream + done chan struct{} +} + +func (s *blockingSyncStream) Recv() (*proto.SyncMappingsRequest, error) { + <-s.done + return nil, context.Canceled +} + +func (s *blockingSyncStream) Send(*proto.SyncMappingsResponse) error { return nil } + +func newBlockingSyncStream(t *testing.T) *blockingSyncStream { + t.Helper() + s := &blockingSyncStream{done: make(chan struct{})} + t.Cleanup(func() { close(s.done) }) + return s +} + +func TestWaitForAck_Ack(t *testing.T) { + stream := &syncRecordingStream{recvMsgs: []*proto.SyncMappingsRequest{ackMsg()}} + + require.NoError(t, waitForAck(context.Background(), stream, time.Second)) +} + +func TestWaitForAck_UnexpectedMessage(t *testing.T) { + stream := &syncRecordingStream{recvMsgs: []*proto.SyncMappingsRequest{ + {Msg: &proto.SyncMappingsRequest_Init{Init: &proto.SyncMappingsInit{}}}, + }} + + assert.ErrorContains(t, waitForAck(context.Background(), stream, time.Second), "expected ack") +} + +func TestWaitForAck_TimesOutWhenProxyStopsAcking(t *testing.T) { + stream := newBlockingSyncStream(t) + + start := time.Now() + err := waitForAck(context.Background(), stream, 50*time.Millisecond) + + assert.ErrorContains(t, err, "no ack within") + assert.Less(t, time.Since(start), time.Second) +} + +func TestWaitForAck_ReturnsWhenConnectionCanceled(t *testing.T) { + stream := newBlockingSyncStream(t) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + assert.ErrorIs(t, waitForAck(ctx, stream, time.Minute), context.Canceled) +} + +func TestSendSnapshotSync_GivesUpWhenProxyStopsAcking(t *testing.T) { + const cluster = "cluster.example.com" + + ctrl := gomock.NewController(t) + mgr := rpservice.NewMockManager(ctrl) + mgr.EXPECT().GetGlobalServices(gomock.Any()).Return(makeServices(1, cluster), nil) + + s := newSnapshotTestServer(t, 10) + s.serviceManager = mgr + s.snapshotAckTimeout = 50 * time.Millisecond + + stream := newBlockingSyncStream(t) + conn := &proxyConnection{proxyID: "proxy-a", address: cluster, syncStream: stream} + + err := s.sendSnapshotSync(context.Background(), conn, stream) + + assert.ErrorContains(t, err, "no ack within") +} diff --git a/management/server/account.go b/management/server/account.go index 0a8ec9a59..59991a4e4 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -770,7 +770,12 @@ func (am *DefaultAccountManager) peerLoginExpirationJob(ctx context.Context, acc ctx = context.WithValue(ctx, hook.ExecutionContextKey, fmt.Sprintf("%s-PEER-EXPIRATION", hook.SystemSource)) expiredPeers, err := am.getExpiredPeers(ctx, accountID) + if isNotFound(err) { + log.WithContext(ctx).Debugf("account %s not found, stopping peer login expiration job", accountID) + return 0, false + } if err != nil { + log.WithContext(ctx).Errorf("failed getting expired peers for account %s: %v", accountID, err) return peerSchedulerRetryInterval, true } @@ -806,8 +811,12 @@ func (am *DefaultAccountManager) schedulePeerLoginExpiration(ctx context.Context func (am *DefaultAccountManager) peerInactivityExpirationJob(ctx context.Context, accountID string) func() (time.Duration, bool) { return func() (time.Duration, bool) { inactivePeers, err := am.getInactivePeers(ctx, accountID) + if isNotFound(err) { + log.WithContext(ctx).Debugf("account %s not found, stopping peer inactivity expiration job", accountID) + return 0, false + } if err != nil { - log.WithContext(ctx).Errorf("failed getting inactive peers for account %s", accountID) + log.WithContext(ctx).Errorf("failed getting inactive peers for account %s: %v", accountID, err) return peerSchedulerRetryInterval, true } @@ -827,6 +836,14 @@ func (am *DefaultAccountManager) peerInactivityExpirationJob(ctx context.Context } } +func isNotFound(err error) bool { + if err == nil { + return false + } + sErr, ok := status.FromError(err) + return ok && sErr.Type() == status.NotFound +} + // checkAndSchedulePeerInactivityExpiration periodically checks for inactive peers to end their sessions func (am *DefaultAccountManager) checkAndSchedulePeerInactivityExpiration(ctx context.Context, accountID string) { am.peerInactivityExpiry.Cancel(ctx, []string{accountID}) @@ -978,8 +995,8 @@ func (am *DefaultAccountManager) DeleteAccount(ctx context.Context, accountID, u log.WithContext(ctx).Errorf("failed deleting account %s. error: %s", accountID, err) return err } - // cancel peer login expiry job am.peerLoginExpiry.Cancel(ctx, []string{account.Id}) + am.peerInactivityExpiry.Cancel(ctx, []string{account.Id}) am.certChallenges.Forget(account.Id) meta := map[string]any{"account_id": account.Id, "domain": account.Domain, "created_at": account.CreatedAt} @@ -1637,10 +1654,6 @@ func (am *DefaultAccountManager) GetAccountIDFromUserAuth(ctx context.Context, u return "", "", status.Errorf(status.NotFound, "user %s not found", userAuth.UserId) } - if userAuth.IsChild { - return accountID, user.Id, nil - } - ctx, err = am.permissionsManager.ValidateAccountAccess(ctx, accountID, user, false) if err != nil { return "", "", err @@ -1692,10 +1705,10 @@ func (am *DefaultAccountManager) updateUserAuthWithSingleMode(ctx context.Contex // and propagates changes to peers if group propagation is enabled. // requires userAuth to have been ValidateAndParseToken and EnsureUserAccessByJWTGroups by the AuthManager func (am *DefaultAccountManager) SyncUserJWTGroups(ctx context.Context, userAuth auth.UserAuth) error { - // Child accounts and PAT-authenticated requests do not sync JWT groups. + // PAT-authenticated requests do not sync JWT groups. // Embedded-Dex local users also skip sync because local password authentication // does not provide external IdP group claims. - if userAuth.IsChild || userAuth.IsPAT || dex.IsLocalUserID(userAuth.UserId) { + if userAuth.IsPAT || dex.IsLocalUserID(userAuth.UserId) { return nil } @@ -1874,8 +1887,6 @@ func (am *DefaultAccountManager) SyncUserJWTGroups(ctx context.Context, userAuth // Existing user + Existing account + Existing Indexed Domain -> Nothing changes // // Existing user + Existing account + Existing domain reclassified Domain as private -> Nothing changes (index domain) -// -// UserAuth IsChild -> checks that account exists func (am *DefaultAccountManager) getAccountIDWithAuthorizationClaims(ctx context.Context, userAuth auth.UserAuth) (string, error) { log.WithContext(ctx).Tracef("getting account with authorization claims. User ID: \"%s\", Account ID: \"%s\", Domain: \"%s\", Domain Category: \"%s\"", userAuth.UserId, userAuth.AccountId, userAuth.Domain, userAuth.DomainCategory) @@ -1884,14 +1895,6 @@ func (am *DefaultAccountManager) getAccountIDWithAuthorizationClaims(ctx context return "", errors.New(emptyUserID) } - if userAuth.IsChild { - exists, err := am.Store.AccountExists(ctx, store.LockingStrengthNone, userAuth.AccountId) - if err != nil || !exists { - return "", err - } - return userAuth.AccountId, nil - } - if userAuth.DomainCategory != types.PrivateCategory || !isDomainValid(userAuth.Domain) { return am.GetAccountIDByUserID(ctx, userAuth) } diff --git a/management/server/account_test.go b/management/server/account_test.go index 7aa88ee64..ef7525961 100644 --- a/management/server/account_test.go +++ b/management/server/account_test.go @@ -4990,3 +4990,27 @@ func TestUpdateUserAuthWithSingleMode(t *testing.T) { assert.Contains(t, err.Error(), "query failed") }) } + +func TestDefaultAccountManager_ExpirationJobsStopForDeletedAccount(t *testing.T) { + manager, _, err := createManager(t) + require.NoError(t, err, "unable to create account manager") + + ctx := context.Background() + accountID, err := manager.GetAccountIDByUserID(ctx, auth.UserAuth{UserId: userID}) + require.NoError(t, err, "unable to create an account") + + account, err := manager.Store.GetAccount(ctx, accountID) + require.NoError(t, err, "unable to get account") + require.NoError(t, manager.Store.DeleteAccount(ctx, account), "unable to delete account") + + jobs := map[string]func() (time.Duration, bool){ + "login": manager.peerLoginExpirationJob(ctx, accountID), + "inactivity": manager.peerInactivityExpirationJob(ctx, accountID), + } + for name, job := range jobs { + t.Run(name, func(t *testing.T) { + _, reschedule := job() + assert.False(t, reschedule, "job must not reschedule for a deleted account") + }) + } +} diff --git a/management/server/auth/manager.go b/management/server/auth/manager.go index 9498789f2..7064c094f 100644 --- a/management/server/auth/manager.go +++ b/management/server/auth/manager.go @@ -75,10 +75,10 @@ func (m *manager) ValidateAndParseToken(ctx context.Context, value string) (auth } func (m *manager) EnsureUserAccessByJWTGroups(ctx context.Context, userAuth auth.UserAuth, token *jwt.Token) (auth.UserAuth, error) { - // Child accounts and PAT-authenticated requests do not use JWT group access checks. + // PAT-authenticated requests do not use JWT group access checks. // Embedded-Dex local users also skip them because local password authentication // does not provide external IdP group claims. - if userAuth.IsChild || userAuth.IsPAT || dex.IsLocalUserID(userAuth.UserId) { + if userAuth.IsPAT || dex.IsLocalUserID(userAuth.UserId) { return userAuth, nil } diff --git a/management/server/http/middleware/auth_middleware.go b/management/server/http/middleware/auth_middleware.go index 1e831e6c3..7c21b6a55 100644 --- a/management/server/http/middleware/auth_middleware.go +++ b/management/server/http/middleware/auth_middleware.go @@ -26,8 +26,6 @@ type SyncUserJWTGroupsFunc func(ctx context.Context, userAuth auth.UserAuth) err type GetUserFromUserAuthFunc func(ctx context.Context, userAuth auth.UserAuth) (*types.User, error) -type IsValidChildAccountFunc func(ctx context.Context, userID, accountID, childAccountID string) bool - // AuthMiddleware middleware to verify personal access tokens (PAT) and JWT tokens type AuthMiddleware struct { authManager serverauth.Manager @@ -36,7 +34,6 @@ type AuthMiddleware struct { syncUserJWTGroups SyncUserJWTGroupsFunc rateLimiter *ratelimit.APIRateLimiter patUsageTracker *PATUsageTracker - isValidChildAccount IsValidChildAccountFunc } // NewAuthMiddleware instance constructor @@ -47,7 +44,6 @@ func NewAuthMiddleware( getUserFromUserAuth GetUserFromUserAuthFunc, rateLimiter *ratelimit.APIRateLimiter, meter metric.Meter, - isValidChildAccount IsValidChildAccountFunc, ) *AuthMiddleware { var patUsageTracker *PATUsageTracker if meter != nil { @@ -65,7 +61,6 @@ func NewAuthMiddleware( getUserFromUserAuth: getUserFromUserAuth, rateLimiter: rateLimiter, patUsageTracker: patUsageTracker, - isValidChildAccount: isValidChildAccount, } } @@ -127,13 +122,6 @@ func (m *AuthMiddleware) checkJWTFromRequest(r *http.Request, authHeaderParts [] return err } - if impersonate, ok := r.URL.Query()["account"]; ok && len(impersonate) == 1 { - if m.isValidChildAccount(ctx, userAuth.UserId, userAuth.AccountId, impersonate[0]) { - userAuth.AccountId = impersonate[0] - userAuth.IsChild = true - } - } - // Email is now extracted in ToUserAuth (from claims or userinfo endpoint) // Available as userAuth.Email @@ -210,13 +198,6 @@ func (m *AuthMiddleware) checkPATFromRequest(r *http.Request, authHeaderParts [] IsPAT: true, } - if impersonate, ok := r.URL.Query()["account"]; ok && len(impersonate) == 1 { - if m.isValidChildAccount(r.Context(), userAuth.UserId, userAuth.AccountId, impersonate[0]) { - userAuth.AccountId = impersonate[0] - userAuth.IsChild = true - } - } - // propagates ctx change to upstream middleware *r = *nbcontext.SetUserAuthInRequest(r, userAuth) return nil diff --git a/management/server/http/middleware/auth_middleware_test.go b/management/server/http/middleware/auth_middleware_test.go index ceefc7c45..9e60af1fa 100644 --- a/management/server/http/middleware/auth_middleware_test.go +++ b/management/server/http/middleware/auth_middleware_test.go @@ -104,7 +104,7 @@ func mockMarkPATUsed(_ context.Context, token string) error { } func mockEnsureUserAccessByJWTGroups(_ context.Context, userAuth nbauth.UserAuth, token *jwt.Token) (nbauth.UserAuth, error) { - if userAuth.IsChild || userAuth.IsPAT { + if userAuth.IsPAT { return userAuth, nil } @@ -212,7 +212,6 @@ func TestAuthMiddleware_Handler(t *testing.T) { }, disabledLimiter, nil, - func(_ context.Context, _, _, _ string) bool { return false }, ) handlerToTest := authMiddleware.Handler(nextHandler) @@ -279,7 +278,6 @@ func TestAuthMiddleware_SyncUserJWTGroupsDetachedFromRequestCancellation(t *test }, disabledLimiter, nil, - func(_ context.Context, _, _, _ string) bool { return false }, ) handlerToTest := authMiddleware.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) @@ -332,7 +330,6 @@ func TestAuthMiddleware_RateLimiting(t *testing.T) { }, ratelimit.NewAPIRateLimiter(rateLimitConfig), nil, - func(_ context.Context, _, _, _ string) bool { return false }, ) handler := authMiddleware.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -385,7 +382,6 @@ func TestAuthMiddleware_RateLimiting(t *testing.T) { }, ratelimit.NewAPIRateLimiter(rateLimitConfig), nil, - func(_ context.Context, _, _, _ string) bool { return false }, ) handler := authMiddleware.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -429,7 +425,6 @@ func TestAuthMiddleware_RateLimiting(t *testing.T) { }, ratelimit.NewAPIRateLimiter(rateLimitConfig), nil, - func(_ context.Context, _, _, _ string) bool { return false }, ) handler := authMiddleware.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -474,7 +469,6 @@ func TestAuthMiddleware_RateLimiting(t *testing.T) { }, ratelimit.NewAPIRateLimiter(rateLimitConfig), nil, - func(_ context.Context, _, _, _ string) bool { return false }, ) handler := authMiddleware.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -539,7 +533,6 @@ func TestAuthMiddleware_RateLimiting(t *testing.T) { }, ratelimit.NewAPIRateLimiter(rateLimitConfig), nil, - func(_ context.Context, _, _, _ string) bool { return false }, ) handler := authMiddleware.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -599,7 +592,6 @@ func TestAuthMiddleware_RateLimiting(t *testing.T) { }, ratelimit.NewAPIRateLimiter(rateLimitConfig), nil, - func(_ context.Context, _, _, _ string) bool { return false }, ) handler := authMiddleware.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -655,7 +647,6 @@ func TestAuthMiddleware_RateLimiting(t *testing.T) { }, ratelimit.NewAPIRateLimiter(rateLimitConfig), nil, - func(_ context.Context, _, _, _ string) bool { return false }, ) handler := authMiddleware.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -756,7 +747,6 @@ func TestAuthMiddleware_Handler_Child(t *testing.T) { }, disabledLimiter, nil, - func(_ context.Context, _, _, _ string) bool { return false }, ) for _, tc := range tt { diff --git a/management/server/http/middleware/middleware_factory.go b/management/server/http/middleware/middleware_factory.go index 3ae617774..5346301ea 100644 --- a/management/server/http/middleware/middleware_factory.go +++ b/management/server/http/middleware/middleware_factory.go @@ -10,7 +10,7 @@ import ( log "github.com/sirupsen/logrus" ) -func BuildMiddleware(rateLimiter *ratelimit.APIRateLimiter, authManager auth.Manager, accountManager account.Manager, metrics telemetry.AppMetrics, isValidChildAcctFunc IsValidChildAccountFunc) []mux.MiddlewareFunc { +func BuildMiddleware(rateLimiter *ratelimit.APIRateLimiter, authManager auth.Manager, accountManager account.Manager, metrics telemetry.AppMetrics) []mux.MiddlewareFunc { toret := make([]mux.MiddlewareFunc, 0) toret = append(toret, metrics.HTTPMiddleware().Handler) @@ -28,7 +28,6 @@ func BuildMiddleware(rateLimiter *ratelimit.APIRateLimiter, authManager auth.Man accountManager.GetUserFromUserAuth, rateLimiter, metrics.GetMeter(), - isValidChildAcctFunc, ).Handler) return toret diff --git a/management/server/http/testing/testing_tools/channel/channel.go b/management/server/http/testing/testing_tools/channel/channel.go index 1b15ba780..88ab70b9c 100644 --- a/management/server/http/testing/testing_tools/channel/channel.go +++ b/management/server/http/testing/testing_tools/channel/channel.go @@ -146,7 +146,7 @@ func BuildApiBlackBoxWithDBState(t testing_tools.TB, sqlFile string, expectedPee zoneRecordsManager := recordsManager.NewManager(store, am, permissionsManager) apiRouter := mux.NewRouter().PathPrefix("/api").Subrouter() - apiRouter.Use(middleware.BuildMiddleware(nil, authManagerMock, am, metrics, nil)...) + apiRouter.Use(middleware.BuildMiddleware(nil, authManagerMock, am, metrics)...) apiHandler, err := http2.NewAPIHandler(ctx, apiRouter, am, networksManager, resourcesManager, routersManager, groupsManager, geoMock, permissionsManager, settingsManager, customZonesManager, zoneRecordsManager, networkMapController, nil, serviceManager, nil, nil, nil, nil, nil, nil) @@ -288,7 +288,7 @@ func BuildApiBlackBoxWithDBStateAndPeerChannel(t testing_tools.TB, sqlFile strin zoneRecordsManager := recordsManager.NewManager(store, am, permissionsManager) apiRouter := mux.NewRouter().PathPrefix("/api").Subrouter() - apiRouter.Use(middleware.BuildMiddleware(nil, authManagerMock, am, metrics, nil)...) + apiRouter.Use(middleware.BuildMiddleware(nil, authManagerMock, am, metrics)...) apiHandler, err := http2.NewAPIHandler(ctx, apiRouter, am, networksManager, resourcesManager, routersManager, groupsManager, geoMock, permissionsManager, settingsManager, customZonesManager, zoneRecordsManager, networkMapController, nil, serviceManager, nil, nil, nil, nil, nil, nil) diff --git a/management/server/peer.go b/management/server/peer.go index 1e3f72a3c..f21e5ebc9 100644 --- a/management/server/peer.go +++ b/management/server/peer.go @@ -1702,6 +1702,9 @@ func (am *DefaultAccountManager) getNextPeerExpiration(ctx context.Context, acco } settings, err := am.Store.GetAccountSettings(ctx, store.LockingStrengthNone, accountID) + if isNotFound(err) { + return 0, false + } if err != nil { log.WithContext(ctx).Errorf("failed to get account settings: %v", err) return peerSchedulerRetryInterval, true @@ -1745,6 +1748,9 @@ func (am *DefaultAccountManager) getNextInactivePeerExpiration(ctx context.Conte } settings, err := am.Store.GetAccountSettings(ctx, store.LockingStrengthNone, accountID) + if isNotFound(err) { + return 0, false + } if err != nil { log.WithContext(ctx).Errorf("failed to get account settings: %v", err) return peerSchedulerRetryInterval, true diff --git a/proxy/cmd/proxy/cmd/root.go b/proxy/cmd/proxy/cmd/root.go index 765d5c05a..5734efe4e 100644 --- a/proxy/cmd/proxy/cmd/root.go +++ b/proxy/cmd/proxy/cmd/root.go @@ -6,6 +6,7 @@ import ( "os" "os/signal" "strconv" + "strings" "syscall" "time" @@ -13,6 +14,7 @@ import ( "github.com/spf13/cobra" "golang.org/x/crypto/acme" + "github.com/netbirdio/netbird/shared/externalipmap" "github.com/netbirdio/netbird/shared/management/domain" "github.com/netbirdio/netbird/shared/profiling" @@ -74,6 +76,7 @@ var ( certLockMethod string wildcardCertDir string wgPort uint16 + externalIPMap []string proxyProtocol bool preSharedKey string supportsCustomPorts bool @@ -117,6 +120,7 @@ func init() { rootCmd.Flags().StringVar(&certLockMethod, "cert-lock-method", envStringOrDefault("NB_PROXY_CERT_LOCK_METHOD", "auto"), "Certificate lock method for cross-replica coordination: auto, flock, or k8s-lease") rootCmd.Flags().StringVar(&wildcardCertDir, "wildcard-cert-dir", envStringOrDefault("NB_PROXY_WILDCARD_CERT_DIR", ""), "Directory containing wildcard certificate pairs (.crt/.key). Wildcard patterns are extracted from SANs automatically") rootCmd.Flags().Uint16Var(&wgPort, "wg-port", envUint16OrDefault("NB_PROXY_WG_PORT", 0), "WireGuard listen port (0 = random). Fixed port only works with single-account deployments") + rootCmd.Flags().StringSliceVar(&externalIPMap, "external-ip-map", envStringSliceOrDefault("NB_PROXY_EXTERNAL_IP_MAP", nil), "Comma-separated external IPs to advertise as ICE candidates, as IP or IP/internal-IP-or-interface (e.g. '203.0.113.5/10.0.0.5'). Requires a fixed --wg-port forwarded on the NAT") rootCmd.Flags().BoolVar(&proxyProtocol, "proxy-protocol", envBoolOrDefault("NB_PROXY_PROXY_PROTOCOL", false), "Enable PROXY protocol on TCP listeners to preserve client IPs behind L4 proxies") rootCmd.Flags().StringVar(&preSharedKey, "preshared-key", envStringOrDefault("NB_PROXY_PRESHARED_KEY", ""), "Define a pre-shared key for the tunnel between proxy and peers") rootCmd.Flags().BoolVar(&supportsCustomPorts, "supports-custom-ports", envBoolOrDefault("NB_PROXY_SUPPORTS_CUSTOM_PORTS", true), "Whether the proxy can bind arbitrary ports for UDP/TCP passthrough") @@ -221,6 +225,13 @@ func runServer(cmd *cobra.Command, args []string) error { return fmt.Errorf("invalid --trusted-proxies: %w", err) } + for i := range externalIPMap { + externalIPMap[i] = strings.TrimSpace(externalIPMap[i]) + } + if err := externalipmap.Validate(externalIPMap); err != nil { + return fmt.Errorf("invalid --external-ip-map: %w", err) + } + ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGTERM, syscall.SIGINT) defer stop() @@ -248,6 +259,7 @@ func runServer(cmd *cobra.Command, args []string) error { CertLockMethod: nbacme.CertLockMethod(certLockMethod), WildcardCertDir: wildcardCertDir, WireguardPort: wgPort, + NATExternalIPs: externalIPMap, Performance: perf, ProxyProtocol: proxyProtocol, PreSharedKey: preSharedKey, @@ -286,6 +298,14 @@ func envStringOrDefault(key string, def string) string { return v } +func envStringSliceOrDefault(key string, def []string) []string { + v, exists := os.LookupEnv(key) + if !exists || v == "" { + return def + } + return strings.Split(v, ",") +} + func envUint16OrDefault(key string, def uint16) uint16 { v, exists := os.LookupEnv(key) if !exists { diff --git a/proxy/internal/roundtrip/netbird.go b/proxy/internal/roundtrip/netbird.go index 07b497c46..21feb59b7 100644 --- a/proxy/internal/roundtrip/netbird.go +++ b/proxy/internal/roundtrip/netbird.go @@ -144,6 +144,8 @@ type ClientConfig struct { WGPort uint16 PreSharedKey string Performance embed.Performance + // NATExternalIPs mirrors embed.Options.NATExternalIPs. + NATExternalIPs []string // BlockInbound mirrors embed.Options.BlockInbound. Set to true on the // standalone proxy where the embedded client never accepts inbound; // set to false on the private/embedded proxy so the engine creates @@ -402,6 +404,7 @@ func (n *NetBird) createClientEntry(ctx context.Context, accountID types.Account WireguardPort: &wgPort, PreSharedKey: n.clientCfg.PreSharedKey, Performance: n.clientCfg.Performance, + NATExternalIPs: n.clientCfg.NATExternalIPs, } logEmbedOptions(n.logger, accountID, serviceID, publicKey.String(), embedOpts) client, err := embed.New(embedOpts) @@ -914,6 +917,7 @@ func logEmbedOptions(logger *log.Logger, accountID types.AccountID, serviceID ty "management_url": opts.ManagementURL, "log_level": opts.LogLevel, "wg_port": wgPort, + "nat_external_ips": opts.NATExternalIPs, "mtu": mtu, "block_inbound": opts.BlockInbound, "block_lan_access": opts.BlockLANAccess, diff --git a/proxy/lifecycle.go b/proxy/lifecycle.go index f8c74d8b5..206c296ac 100644 --- a/proxy/lifecycle.go +++ b/proxy/lifecycle.go @@ -89,6 +89,9 @@ type Config struct { // WireguardPort is the UDP port for the embedded NetBird tunnel. // Zero asks the OS for a random port. WireguardPort uint16 + // NATExternalIPs are external IPs advertised as ICE candidates by the + // embedded tunnel. Requires a fixed WireguardPort forwarded on the NAT. + NATExternalIPs []string // ProxyProtocol enables PROXY protocol (v1/v2) on TCP listeners. ProxyProtocol bool // PreSharedKey is the WireGuard pre-shared key used between the @@ -160,6 +163,7 @@ func New(ctx context.Context, cfg Config) *Server { ForwardedProto: cfg.ForwardedProto, TrustedProxies: cfg.TrustedProxies, WireguardPort: cfg.WireguardPort, + NATExternalIPs: cfg.NATExternalIPs, ProxyProtocol: cfg.ProxyProtocol, PreSharedKey: cfg.PreSharedKey, Performance: cfg.Performance, diff --git a/proxy/server.go b/proxy/server.go index 083449962..3bba38644 100644 --- a/proxy/server.go +++ b/proxy/server.go @@ -203,6 +203,9 @@ type Server struct { // single-account deployments; multiple accounts will fail to bind // the same port. WireguardPort uint16 + // NATExternalIPs maps external IPs advertised as ICE candidates by every + // embedded client. Requires a fixed WireguardPort forwarded on the NAT. + NATExternalIPs []string // Performance configures the tunnel pool/batch sizes for every // embedded client this proxy spawns. Performance embed.Performance @@ -581,10 +584,11 @@ func (s *Server) initManagementClient() error { // inbound listeners. func (s *Server) initNetBirdClient() error { s.netbird = roundtrip.NewNetBird(s.ctx, s.ID, s.ProxyURL, roundtrip.ClientConfig{ - MgmtAddr: s.ManagementAddress, - WGPort: s.WireguardPort, - PreSharedKey: s.PreSharedKey, - Performance: s.Performance, + MgmtAddr: s.ManagementAddress, + WGPort: s.WireguardPort, + PreSharedKey: s.PreSharedKey, + Performance: s.Performance, + NATExternalIPs: s.NATExternalIPs, // On --private the embedded client serves per-account inbound // listeners and must apply management's ACL: keep BlockInbound off // so the engine creates the ACL manager. On the standalone proxy @@ -1299,6 +1303,10 @@ func (s *Server) proxyCapabilities() *proto.ProxyCapabilities { } func (s *Server) tryGetMappingUpdate(ctx context.Context, client proto.ProxyServiceClient, initialSyncDone *bool, onConnected func()) error { + // Canceling closes the stream on management; a bare return leaves it open. + ctx, cancel := context.WithCancel(ctx) + defer cancel() + connectTime := time.Now() mappingClient, err := client.GetMappingUpdate(ctx, &proto.GetMappingUpdateRequest{ ProxyId: s.ID, @@ -1321,6 +1329,10 @@ func (s *Server) tryGetMappingUpdate(ctx context.Context, client proto.ProxyServ } func (s *Server) trySyncMappings(ctx context.Context, client proto.ProxyServiceClient, initialSyncDone *bool, onConnected func()) error { + // Canceling closes the stream on management; a bare return leaves it open. + ctx, cancel := context.WithCancel(ctx) + defer cancel() + connectTime := time.Now() stream, err := client.SyncMappings(ctx) if err != nil { diff --git a/release_files/freebsd-port-diff.sh b/release_files/freebsd-port-diff.sh index 77ea55520..cd3c341b9 100755 --- a/release_files/freebsd-port-diff.sh +++ b/release_files/freebsd-port-diff.sh @@ -9,62 +9,19 @@ # Usage: ./freebsd-port-diff.sh [new_version] # Example: ./freebsd-port-diff.sh 0.60.7 # -# If no version is provided, it fetches the latest from GitHub. +# If no version is provided, it uses the latest stable release from GitHub. +# Prerelease versions (canary, rc, ...) are rejected: the port only tracks +# stable releases. set -e -GITHUB_REPO="netbirdio/netbird" -PORTS_MIRROR_BASE="https://raw.githubusercontent.com/freebsd/freebsd-ports/main/security/netbird" +# shellcheck source=release_files/freebsd-port-lib.sh +source "$(dirname "${BASH_SOURCE[0]}")/freebsd-port-lib.sh" + GO_PROXY="https://proxy.golang.org/github.com/netbirdio/netbird/@v" OUTPUT_DIR="${OUTPUT_DIR:-.}" AWK_FIRST_FIELD='{print $1}' -fetch_all_tags() { - curl -sL "https://github.com/${GITHUB_REPO}/tags" 2>/dev/null | \ - grep -oE '/releases/tag/v[0-9]+\.[0-9]+\.[0-9]+([^"]+)?' | \ - grep -iv 'rc' | \ - sed 's/.*\/v//' | \ - sort -u -V - return 0 -} - -fetch_current_ports_version() { - echo "Fetching current version from FreeBSD ports..." >&2 - local makefile version - makefile=$(fetch_ports_file "Makefile") || return 1 - version=$(echo "$makefile" | \ - grep -E "^DISTVERSION=" | \ - sed 's/DISTVERSION=[[:space:]]*//' | \ - tr -d '\t ') - if [[ -z "$version" ]]; then - echo "Error: Could not extract DISTVERSION from ports Makefile" >&2 - return 1 - fi - echo "$version" - return 0 -} - -fetch_latest_github_release() { - echo "Fetching latest release from GitHub..." >&2 - fetch_all_tags | tail -1 - return 0 -} - -fetch_ports_file() { - local filename="$1" - local content - if ! content=$(curl -fsL --proto '=https' --proto-redir '=https' --retry 3 "${PORTS_MIRROR_BASE}/${filename}" 2>/dev/null); then - echo "Error: Could not fetch ${filename} from ${PORTS_MIRROR_BASE}" >&2 - return 1 - fi - if [[ "$content" == \<* ]]; then - echo "Error: Received HTML instead of ${filename} from ${PORTS_MIRROR_BASE}" >&2 - return 1 - fi - printf '%s' "$content" - return 0 -} - compute_checksums() { local version="$1" local tmpdir @@ -77,11 +34,15 @@ compute_checksums() { local mod_file="${tmpdir}/v${version}.mod" local zip_file="${tmpdir}/v${version}.zip" - curl -sL "${GO_PROXY}/v${version}.mod" -o "$mod_file" 2>/dev/null - curl -sL "${GO_PROXY}/v${version}.zip" -o "$zip_file" 2>/dev/null + local -a curl_opts=(-fsL --proto '=https' --proto-redir '=https' --retry 3) + if ! curl "${curl_opts[@]}" "${GO_PROXY}/v${version}.mod" -o "$mod_file" 2>/dev/null || \ + ! curl "${curl_opts[@]}" "${GO_PROXY}/v${version}.zip" -o "$zip_file" 2>/dev/null; then + echo "Error: Could not download v${version} from the Go module proxy" >&2 + return 1 + fi if [[ ! -s "$mod_file" ]] || [[ ! -s "$zip_file" ]]; then - echo "Error: Could not download files from Go module proxy" >&2 + echo "Error: Go module proxy returned empty files for v${version}" >&2 return 1 fi @@ -133,25 +94,12 @@ generate_new_makefile() { } # Parse arguments -NEW_VERSION="${1:-}" - -# Auto-detect versions if not provided -OLD_VERSION=$(fetch_current_ports_version) -if [[ -z "$OLD_VERSION" ]]; then - echo "Error: Could not fetch current version from FreeBSD ports" >&2 - exit 1 -fi -echo "Current FreeBSD ports version: ${OLD_VERSION}" >&2 - -if [[ -z "$NEW_VERSION" ]]; then - NEW_VERSION=$(fetch_latest_github_release) - if [[ -z "$NEW_VERSION" ]]; then - echo "Error: Could not fetch latest release from GitHub" >&2 - exit 1 - fi -fi +NEW_VERSION=$(resolve_target_version "${1:-}") echo "Target version: ${NEW_VERSION}" >&2 +OLD_VERSION=$(fetch_current_ports_version) +echo "Current FreeBSD ports version: ${OLD_VERSION}" >&2 + if [[ "$OLD_VERSION" = "$NEW_VERSION" ]]; then echo "Port is already at version ${NEW_VERSION}. Nothing to do." >&2 exit 0 diff --git a/release_files/freebsd-port-issue-body.sh b/release_files/freebsd-port-issue-body.sh index 1f0c8a567..16fe0e2b6 100755 --- a/release_files/freebsd-port-issue-body.sh +++ b/release_files/freebsd-port-issue-body.sh @@ -10,81 +10,29 @@ # # If no versions are provided, the script will: # - Fetch OLD version from the FreeBSD ports GitHub mirror (current version in ports tree) -# - Fetch NEW version from latest NetBird GitHub release tag +# - Fetch NEW version from the latest stable NetBird release tag on GitHub +# +# Prerelease versions (canary, rc, ...) are rejected: the port only tracks +# stable releases. set -e -GITHUB_REPO="netbirdio/netbird" -PORTS_MAKEFILE_URL="https://raw.githubusercontent.com/freebsd/freebsd-ports/main/security/netbird/Makefile" - -fetch_current_ports_version() { - echo "Fetching current version from FreeBSD ports..." >&2 - local makefile_content - makefile_content=$(curl -fsL --proto '=https' --proto-redir '=https' --retry 3 "$PORTS_MAKEFILE_URL" 2>/dev/null) || makefile_content="" - if [[ "$makefile_content" == \<* ]]; then - echo "Error: Received HTML instead of Makefile from ${PORTS_MAKEFILE_URL}" >&2 - return 1 - fi - if [[ -z "$makefile_content" ]]; then - echo "Error: Could not fetch Makefile from FreeBSD ports" >&2 - return 1 - fi - echo "$makefile_content" | grep -E "^DISTVERSION=" | sed 's/DISTVERSION=[[:space:]]*//' | tr -d '\t ' - return 0 -} - -fetch_all_tags() { - # Fetch tags from GitHub tags page (no rate limiting, no auth needed) - curl -sL "https://github.com/${GITHUB_REPO}/tags" 2>/dev/null | \ - grep -oE '/releases/tag/v[0-9]+\.[0-9]+\.[0-9]+([^"]+)?' | \ - grep -iv 'rc' | \ - sed 's/.*\/v//' | \ - sort -u -V - return 0 -} - -fetch_latest_github_release() { - echo "Fetching latest release from GitHub..." >&2 - local latest - - # Fetch from GitHub tags page - latest=$(fetch_all_tags | tail -1) - - if [[ -z "$latest" ]]; then - # Fallback to GitHub API - latest=$(curl -sL "https://api.github.com/repos/${GITHUB_REPO}/releases/latest" 2>/dev/null | \ - grep '"tag_name"' | sed 's/.*"tag_name": *"v\([^"]*\)".*/\1/') - fi - - if [[ -z "$latest" ]]; then - echo "Error: Could not fetch latest release from GitHub" >&2 - return 1 - fi - echo "$latest" - return 0 -} +# shellcheck source=release_files/freebsd-port-lib.sh +source "$(dirname "${BASH_SOURCE[0]}")/freebsd-port-lib.sh" OLD_VERSION="${1:-}" NEW_VERSION="${2:-}" if [[ -z "$OLD_VERSION" ]]; then OLD_VERSION=$(fetch_current_ports_version) - if [[ -z "$OLD_VERSION" ]]; then - echo "Error: Could not determine old version. Please provide it manually." >&2 - echo "Usage: $0 " >&2 - exit 1 - fi echo "Detected OLD version from FreeBSD ports: $OLD_VERSION" >&2 fi if [[ -z "$NEW_VERSION" ]]; then - NEW_VERSION=$(fetch_latest_github_release) - if [[ -z "$NEW_VERSION" ]]; then - echo "Error: Could not determine new version. Please provide it manually." >&2 - echo "Usage: $0 " >&2 - exit 1 - fi + NEW_VERSION=$(resolve_target_version "") echo "Detected NEW version from GitHub: $NEW_VERSION" >&2 +else + NEW_VERSION=$(resolve_target_version "$NEW_VERSION") fi if [[ "$OLD_VERSION" = "$NEW_VERSION" ]]; then @@ -98,8 +46,11 @@ OUTPUT_DIR="${OUTPUT_DIR:-.}" fetch_releases_between_versions() { echo "Fetching release history from GitHub..." >&2 - # Fetch all tags and filter to those between OLD and NEW versions - fetch_all_tags | \ + local tags + tags=$(fetch_stable_tags) || return 1 + + # List the stable releases after OLD up to and including NEW + echo "$tags" | \ while read -r ver; do if [[ "$(printf '%s\n' "$OLD_VERSION" "$ver" | sort -V | head -n1)" = "$OLD_VERSION" ]] && \ [[ "$(printf '%s\n' "$ver" "$NEW_VERSION" | sort -V | head -n1)" = "$ver" ]] && \ @@ -112,7 +63,7 @@ fetch_releases_between_versions() { generate_changelog_section() { local releases - releases=$(fetch_releases_between_versions) + releases=$(fetch_releases_between_versions) || return 1 echo "Changelogs:" if [[ -n "$releases" ]]; then @@ -127,7 +78,10 @@ generate_changelog_section() { OUTPUT_FILE="${OUTPUT_DIR}/netbird-${NEW_VERSION}-issue.txt" -cat << EOF > "$OUTPUT_FILE" +# Built before the heredoc: a failing substitution inside it would not stop the script. +CHANGELOG=$(generate_changelog_section) + +cat << EOT > "$OUTPUT_FILE" BUGZILLA ISSUE DETAILS ====================== @@ -139,11 +93,11 @@ Description: ------------ security/netbird: Update ${OLD_VERSION} => ${NEW_VERSION} -$(generate_changelog_section) +${CHANGELOG} Commit log: https://github.com/${GITHUB_REPO}/compare/v${OLD_VERSION}...v${NEW_VERSION} -EOF +EOT echo "=========================================" echo "Issue body saved to: ${OUTPUT_FILE}" diff --git a/release_files/freebsd-port-lib.sh b/release_files/freebsd-port-lib.sh new file mode 100644 index 000000000..20102be54 --- /dev/null +++ b/release_files/freebsd-port-lib.sh @@ -0,0 +1,98 @@ +#!/bin/bash +# +# Shared helpers for the FreeBSD port scripts. Source this file, do not run it. +# +# The FreeBSD port tracks stable NetBird releases only, so every helper here +# works on plain MAJOR.MINOR.PATCH versions and ignores canary, release +# candidate and any other prerelease tag. + +GITHUB_REPO="netbirdio/netbird" +GITHUB_REPO_URL="https://github.com/${GITHUB_REPO}.git" +PORTS_MIRROR_BASE="https://raw.githubusercontent.com/freebsd/freebsd-ports/main/security/netbird" +STABLE_VERSION_RE='^[0-9]+\.[0-9]+\.[0-9]+$' + +# is_stable_version reports whether the version is a plain MAJOR.MINOR.PATCH release. +is_stable_version() { + local version="$1" + if [[ "$version" =~ $STABLE_VERSION_RE ]]; then + return 0 + fi + return 1 +} + +# fetch_stable_tags prints the stable release versions known to GitHub, oldest +# first. git ls-remote returns every tag in one round trip, with no API +# pagination or rate limit to work around. +fetch_stable_tags() { + local refs + if ! refs=$(git ls-remote --tags --refs "$GITHUB_REPO_URL" 2>/dev/null); then + echo "Error: Could not list tags of ${GITHUB_REPO_URL}" >&2 + return 1 + fi + + echo "$refs" | \ + sed -nE 's#.*refs/tags/v([0-9]+\.[0-9]+\.[0-9]+)$#\1#p' | \ + sort -u -V + return 0 +} + +# resolve_target_version validates an explicitly requested version, or picks the +# latest stable release from GitHub when none was given. A leading "v" is +# accepted so a git tag name can be passed as is. +resolve_target_version() { + local raw="$1" + local requested="${raw#v}" + + if [[ -n "$raw" ]]; then + if ! is_stable_version "$requested"; then + echo "Error: '${raw}' is not a stable release. The FreeBSD port only tracks MAJOR.MINOR.PATCH versions." >&2 + return 1 + fi + echo "$requested" + return 0 + fi + + echo "Fetching latest stable release from GitHub..." >&2 + local latest + latest=$(fetch_stable_tags | tail -1) + if [[ -z "$latest" ]]; then + echo "Error: Could not determine the latest stable release from GitHub" >&2 + return 1 + fi + echo "$latest" + return 0 +} + +# fetch_ports_file prints a file from the security/netbird port on the FreeBSD +# ports GitHub mirror. +fetch_ports_file() { + local filename="$1" + local content + if ! content=$(curl -fsL --proto '=https' --proto-redir '=https' --retry 3 "${PORTS_MIRROR_BASE}/${filename}" 2>/dev/null); then + echo "Error: Could not fetch ${filename} from ${PORTS_MIRROR_BASE}" >&2 + return 1 + fi + if [[ "$content" == \<* ]]; then + echo "Error: Received HTML instead of ${filename} from ${PORTS_MIRROR_BASE}" >&2 + return 1 + fi + printf '%s' "$content" + return 0 +} + +# fetch_current_ports_version prints the DISTVERSION currently in the ports tree. +fetch_current_ports_version() { + echo "Fetching current version from FreeBSD ports..." >&2 + local makefile version + makefile=$(fetch_ports_file "Makefile") || return 1 + version=$(echo "$makefile" | \ + grep -E "^DISTVERSION=" | \ + sed 's/DISTVERSION=[[:space:]]*//' | \ + tr -d '\t ') + if [[ -z "$version" ]]; then + echo "Error: Could not extract DISTVERSION from ports Makefile" >&2 + return 1 + fi + echo "$version" + return 0 +} diff --git a/shared/externalipmap/validate.go b/shared/externalipmap/validate.go new file mode 100644 index 000000000..30517007c --- /dev/null +++ b/shared/externalipmap/validate.go @@ -0,0 +1,70 @@ +// Package externalipmap validates the --external-ip-map flag shared by the +// agent and the proxy. +package externalipmap + +import ( + "fmt" + "net" + "strings" +) + +const flagName = "external-ip-map" + +// Validate checks each element is formatted as "IP", "IP/IP" or +// "IP/interface". The engine drops every mapping when one element fails to +// parse, so callers should reject bad input up front. +func Validate(list []string) error { + for _, element := range list { + if element == "" { + return fmt.Errorf("empty string is not a valid input for %s", flagName) + } + + subElements := strings.Split(element, "/") + if len(subElements) > 2 { + return fmt.Errorf("%s is not a valid input for %s. it should be formatted as \"String\" or \"String/String\"", element, flagName) + } + + if !isValidIP(subElements[0]) { + return fmt.Errorf("%s is not a valid input for %s. it should be formatted as \"IP\" or \"IP/IP\", or \"IP/Interface Name\"", element, flagName) + } + + if len(subElements) == 2 { + if err := validateInternal(subElements[1]); err != nil { + return fmt.Errorf("%s is not a valid input for %s. it should be an IP string or a network name: %w", subElements[1], flagName, err) + } + } + } + return nil +} + +func validateInternal(element string) error { + if isValidIP(element) { + return nil + } + validIface, err := isValidInterface(element) + if err != nil { + return fmt.Errorf("validate network interface name: %w", err) + } + if !validIface { + return fmt.Errorf("invalid IP or network interface name not found") + } + return nil +} + +// isValidIP uses net.ParseIP to match the engine's parser exactly. +func isValidIP(ip string) bool { + return net.ParseIP(ip) != nil +} + +func isValidInterface(name string) (bool, error) { + netInterfaces, err := net.Interfaces() + if err != nil { + return false, err + } + for _, iface := range netInterfaces { + if iface.Name == name { + return true, nil + } + } + return false, nil +} diff --git a/shared/externalipmap/validate_test.go b/shared/externalipmap/validate_test.go new file mode 100644 index 000000000..baf3cf787 --- /dev/null +++ b/shared/externalipmap/validate_test.go @@ -0,0 +1,48 @@ +package externalipmap + +import ( + "net" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestValidate(t *testing.T) { + ifaces, err := net.Interfaces() + require.NoError(t, err) + require.NotEmpty(t, ifaces, "host must have at least one interface") + iface := ifaces[0].Name + + tests := []struct { + name string + input []string + wantErr bool + }{ + {name: "nil", input: nil}, + {name: "single IP", input: []string{"203.0.113.5"}}, + {name: "IP to IP", input: []string{"203.0.113.5/10.0.0.5"}}, + {name: "IP to interface", input: []string{"203.0.113.5/" + iface}}, + {name: "several", input: []string{"203.0.113.5", "203.0.113.6/10.0.0.6"}}, + {name: "empty element", input: []string{""}, wantErr: true}, + // The engine rejects padded values, so validation must too. + {name: "space padded", input: []string{"203.0.113.5", " 203.0.113.6"}, wantErr: true}, + {name: "not an IP", input: []string{"not-an-ip"}, wantErr: true}, + {name: "too many slashes", input: []string{"203.0.113.5/10.0.0.5/10.0.0.6"}, wantErr: true}, + {name: "unknown interface", input: []string{"203.0.113.5/nb-no-such-iface0"}, wantErr: true}, + {name: "external interface", input: []string{iface}, wantErr: true}, + {name: "interface to IP", input: []string{iface + "/10.0.0.5"}, wantErr: true}, + {name: "two interfaces", input: []string{iface + "/" + iface}, wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := Validate(tt.input) + if tt.wantErr { + assert.Error(t, err, "input %q should be rejected", tt.input) + return + } + assert.NoError(t, err, "input %q should be accepted", tt.input) + }) + } +} 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-----"]