mirror of
https://github.com/UNITRONIX/BetterDesk.git
synced 2026-09-10 09:35:39 +00:00
feat(auth): enhance device authentication and relay handling
- Added tests to ensure disabled devices cannot authenticate using device tokens. - Updated authentication logic to reject disabled devices, banned devices, and soft-deleted devices during relay requests. - Implemented server public key pinning for enhanced security in API HTTP client. - Introduced normalization for server certificate pins to ensure valid formats. - Enhanced branding profile validation to check for valid certificate pins.
This commit is contained in:
@@ -212,7 +212,7 @@ func (g *Gateway) authDeviceToken(p AuthPayload, clientIP string) (string, strin
|
||||
return "", "", fmt.Errorf("device token is not bound to this device")
|
||||
}
|
||||
peerInfo, err := g.db.GetPeer(p.DeviceID)
|
||||
if err != nil || peerInfo == nil || peerInfo.Banned || peerInfo.SoftDeleted {
|
||||
if err != nil || peerInfo == nil || peerInfo.Disabled || peerInfo.Banned || peerInfo.SoftDeleted {
|
||||
g.auditAction("cdap_auth_failed", clientIP, map[string]string{
|
||||
"device_id": p.DeviceID,
|
||||
"reason": "device not enrolled or unavailable",
|
||||
|
||||
@@ -80,3 +80,21 @@ func TestDeviceTokenCannotAuthenticateAnotherDevice(t *testing.T) {
|
||||
t.Fatal("device token bound to AGENT001 authenticated OTHER001")
|
||||
}
|
||||
}
|
||||
|
||||
func TestDeviceTokenCannotAuthenticateDisabledDevice(t *testing.T) {
|
||||
gateway, database, token := newDeviceTokenAuthGateway(t)
|
||||
if err := database.UpsertPeer(&db.Peer{
|
||||
ID: "AGENT001",
|
||||
Status: "ONLINE",
|
||||
Disabled: true,
|
||||
}); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
if _, _, err := gateway.authDeviceToken(AuthPayload{
|
||||
Token: token,
|
||||
DeviceID: "AGENT001",
|
||||
}, "127.0.0.1"); err == nil {
|
||||
t.Fatal("disabled device token unexpectedly authenticated")
|
||||
}
|
||||
}
|
||||
|
||||
@@ -97,6 +97,22 @@ func (s *Server) targetAcceptsInboundSession(targetID string) bool {
|
||||
return state != db.PeerIDSoftDeleted
|
||||
}
|
||||
|
||||
// requiresRelayOnlyCompatibility keeps the temporary RustDesk-compatible
|
||||
// support-agent path on relay transport. Direct transport has no equivalent
|
||||
// server-bound session grant yet, so allowing P2P would create an
|
||||
// authorization bypass around the passive-session policy.
|
||||
func (s *Server) requiresRelayOnlyCompatibility(peerID string) bool {
|
||||
if peerID == "" || s.db == nil {
|
||||
return false
|
||||
}
|
||||
p, err := s.db.GetPeer(peerID)
|
||||
if err != nil {
|
||||
log.Printf("[signal] Compatibility peer %s lookup failed: %v", peerID, err)
|
||||
return true
|
||||
}
|
||||
return isInboundOnlyPeer(p)
|
||||
}
|
||||
|
||||
// handleUDPMessage dispatches a UDP message to the appropriate handler.
|
||||
func (s *Server) handleUDPMessage(msg *pb.RendezvousMessage, raddr *net.UDPAddr) {
|
||||
switch {
|
||||
@@ -737,8 +753,9 @@ func (s *Server) handlePunchHoleRequest(msg *pb.PunchHoleRequest, raddr *net.UDP
|
||||
return
|
||||
}
|
||||
|
||||
// Target is banned
|
||||
if target.Banned {
|
||||
// Target policy is durable; do not trust a stale live-peer entry after an
|
||||
// administrator has disabled, banned, or removed the device.
|
||||
if target.Banned || !s.targetAcceptsInboundSession(targetID) {
|
||||
resp := &pb.RendezvousMessage{
|
||||
Union: &pb.RendezvousMessage_PunchHoleResponse{
|
||||
PunchHoleResponse: &pb.PunchHoleResponse{
|
||||
@@ -779,7 +796,9 @@ func (s *Server) handlePunchHoleRequest(msg *pb.PunchHoleRequest, raddr *net.UDP
|
||||
targetID, target.UDPAddr, target.StatusTier, time.Since(target.LastReg), relayServer)
|
||||
|
||||
// If force relay or always use relay
|
||||
if msg.ForceRelay || s.cfg.AlwaysUseRelay || hairpin || s.shouldForceRelayForPeers(initiatorID, targetID) {
|
||||
if msg.ForceRelay || s.cfg.AlwaysUseRelay || hairpin ||
|
||||
s.shouldForceRelayForPeers(initiatorID, targetID) ||
|
||||
s.requiresRelayOnlyCompatibility(targetID) {
|
||||
log.Printf("[signal] PunchHole: force relay for %s", targetID)
|
||||
s.sendRelayResponse(target, raddr, msg, relayServer, initiatorID)
|
||||
return
|
||||
@@ -919,9 +938,9 @@ func (s *Server) handlePunchHoleRequestTCP(msg *pb.PunchHoleRequest, raddr *net.
|
||||
}
|
||||
}
|
||||
|
||||
// Target is banned — report as offline to initiator
|
||||
if target.Banned {
|
||||
log.Printf("[signal] PunchHole (TCP): target %s is banned, rejecting", targetID)
|
||||
// Reject disabled, banned, or soft-deleted targets as offline.
|
||||
if target.Banned || !s.targetAcceptsInboundSession(targetID) {
|
||||
log.Printf("[signal] PunchHole (TCP): target %s is unavailable, rejecting", targetID)
|
||||
return &pb.RendezvousMessage{
|
||||
Union: &pb.RendezvousMessage_PunchHoleResponse{
|
||||
PunchHoleResponse: &pb.PunchHoleResponse{
|
||||
@@ -959,7 +978,9 @@ func (s *Server) handlePunchHoleRequestTCP(msg *pb.PunchHoleRequest, raddr *net.
|
||||
// PunchHoleResponse), generate their own UUID, and connect to relay with it
|
||||
// — while the target connects with the server's UUID. This broke relay
|
||||
// pairing every time (Issue #66).
|
||||
if msg.ForceRelay || s.cfg.AlwaysUseRelay || hairpin || s.shouldForceRelayForPeers(initiatorID, targetID) {
|
||||
if msg.ForceRelay || s.cfg.AlwaysUseRelay || hairpin ||
|
||||
s.shouldForceRelayForPeers(initiatorID, targetID) ||
|
||||
s.requiresRelayOnlyCompatibility(targetID) {
|
||||
log.Printf("[signal] PunchHole (TCP): force relay for %s (returning SYMMETRIC to let client drive relay UUID)", targetID)
|
||||
|
||||
var signedPk []byte
|
||||
@@ -1238,9 +1259,9 @@ func (s *Server) handleRequestRelay(msg *pb.RequestRelay, raddr *net.UDPAddr) {
|
||||
return
|
||||
}
|
||||
|
||||
// Target is banned — reject relay as if offline
|
||||
if target.Banned {
|
||||
log.Printf("[signal] RequestRelay: target %s is banned, rejecting", targetID)
|
||||
// Reject disabled, banned, or soft-deleted targets as offline.
|
||||
if target.Banned || !s.targetAcceptsInboundSession(targetID) {
|
||||
log.Printf("[signal] RequestRelay: target %s is unavailable, rejecting", targetID)
|
||||
resp := &pb.RendezvousMessage{
|
||||
Union: &pb.RendezvousMessage_RelayResponse{
|
||||
RelayResponse: &pb.RelayResponse{
|
||||
@@ -1413,9 +1434,9 @@ func (s *Server) handleRequestRelayTCP(msg *pb.RequestRelay, raddr *net.UDPAddr,
|
||||
}
|
||||
}
|
||||
|
||||
// Target is banned — reject relay as if offline
|
||||
if target.Banned {
|
||||
log.Printf("[signal] RequestRelay (TCP): target %s is banned, rejecting", targetID)
|
||||
// Reject disabled, banned, or soft-deleted targets as offline.
|
||||
if target.Banned || !s.targetAcceptsInboundSession(targetID) {
|
||||
log.Printf("[signal] RequestRelay (TCP): target %s is unavailable, rejecting", targetID)
|
||||
return &pb.RendezvousMessage{
|
||||
Union: &pb.RendezvousMessage_RelayResponse{
|
||||
RelayResponse: &pb.RelayResponse{
|
||||
|
||||
@@ -1029,6 +1029,83 @@ func TestInboundOnlyAgentCanBeConnectionTarget(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestInboundOnlySupportTargetForcesRelay(t *testing.T) {
|
||||
srv, database := newTestSignalServer(t, config.EnrollmentModeOpen)
|
||||
if err := database.UpsertPeer(&db.Peer{ID: "CLIENTRELAY1", DeviceType: "desktop"}); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if err := database.UpsertPeer(&db.Peer{
|
||||
ID: "SUPPORTRELAY1",
|
||||
DeviceType: "os_agent",
|
||||
Tags: "support-agent",
|
||||
}); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
putOnlinePeer(srv, "CLIENTRELAY1", "198.51.100.160", 51000, peer.ConnTCP)
|
||||
putOnlinePeer(srv, "SUPPORTRELAY1", "203.0.113.160", 52000, peer.ConnTCP)
|
||||
|
||||
response := srv.handlePunchHoleRequestTCP(
|
||||
&pb.PunchHoleRequest{Id: "SUPPORTRELAY1"},
|
||||
udpAddr("198.51.100.160", 51000),
|
||||
)
|
||||
punch := response.GetPunchHoleResponse()
|
||||
if punch == nil {
|
||||
t.Fatalf("response = %+v, want PunchHoleResponse", response)
|
||||
}
|
||||
if punch.GetNatType() != pb.NatType_SYMMETRIC {
|
||||
t.Fatalf("NatType = %v, want forced relay (%v)", punch.GetNatType(), pb.NatType_SYMMETRIC)
|
||||
}
|
||||
}
|
||||
|
||||
func TestUnavailableTargetCannotReceiveRelay(t *testing.T) {
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
setup func(t *testing.T, database db.Database, targetID string)
|
||||
}{
|
||||
{
|
||||
name: "disabled",
|
||||
setup: func(t *testing.T, database db.Database, targetID string) {
|
||||
t.Helper()
|
||||
if err := database.UpsertPeer(&db.Peer{ID: targetID, Disabled: true}); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "soft deleted",
|
||||
setup: func(t *testing.T, database db.Database, targetID string) {
|
||||
t.Helper()
|
||||
if err := database.UpsertPeer(&db.Peer{ID: targetID}); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if err := database.DeletePeer(targetID); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
},
|
||||
},
|
||||
} {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
srv, database := newTestSignalServer(t, config.EnrollmentModeOpen)
|
||||
const initiatorID = "ACTIVEINIT1"
|
||||
const targetID = "UNAVAILTGT1"
|
||||
if err := database.UpsertPeer(&db.Peer{ID: initiatorID, DeviceType: "desktop"}); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
tc.setup(t, database, targetID)
|
||||
putOnlinePeer(srv, initiatorID, "198.51.100.150", 51000, peer.ConnTCP)
|
||||
putOnlinePeer(srv, targetID, "203.0.113.150", 52000, peer.ConnTCP)
|
||||
|
||||
response := srv.handleRequestRelayTCP(&pb.RequestRelay{
|
||||
Id: targetID,
|
||||
Uuid: "unavailable-target-" + strings.ReplaceAll(tc.name, " ", "-"),
|
||||
}, udpAddr("198.51.100.150", 51000), peer.ConnTCP)
|
||||
if relay := response.GetRelayResponse(); relay == nil || relay.RefuseReason != "Target offline" {
|
||||
t.Fatalf("relay response = %+v, want unavailable target rejection", response)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestPanelProxyLoopbackCanPunchHoleWithoutPeer(t *testing.T) {
|
||||
srv, _ := newTestSignalServer(t, config.EnrollmentModeManaged)
|
||||
putOnlinePeer(srv, "TGTWEB1", "203.0.113.90", 52000, peer.ConnTCP)
|
||||
|
||||
@@ -3,7 +3,11 @@ package main
|
||||
import (
|
||||
"bytes"
|
||||
"context"
|
||||
"crypto/sha256"
|
||||
"crypto/subtle"
|
||||
"crypto/tls"
|
||||
"crypto/x509"
|
||||
"encoding/hex"
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"io"
|
||||
@@ -25,15 +29,62 @@ func tlsInsecureEnabled() bool {
|
||||
|
||||
// apiHTTPClient returns an HTTP client for BetterDesk API calls.
|
||||
func apiHTTPClient(timeout time.Duration) *http.Client {
|
||||
client := &http.Client{Timeout: timeout}
|
||||
if tlsInsecureEnabled() {
|
||||
client.Transport = &http.Transport{
|
||||
TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, //nolint:gosec // opt-in dev
|
||||
}
|
||||
pin := ""
|
||||
if b := GetBranding(); b.Server != nil {
|
||||
pin = b.Server.CertPin
|
||||
}
|
||||
return apiHTTPClientWithPin(timeout, pin)
|
||||
}
|
||||
|
||||
func apiHTTPClientWithPin(timeout time.Duration, pin string) *http.Client {
|
||||
client := &http.Client{Timeout: timeout}
|
||||
pin = normalizeServerCertPin(pin)
|
||||
if pin == "" && !tlsInsecureEnabled() {
|
||||
return client
|
||||
}
|
||||
|
||||
tlsConfig := &tls.Config{MinVersion: tls.VersionTLS12}
|
||||
if pin != "" {
|
||||
// Pinning the leaf SPKI is an authentication check stronger than
|
||||
// platform trust alone. The profile's endpoint allowlist prevents this
|
||||
// key from being used for an arbitrary destination.
|
||||
tlsConfig.InsecureSkipVerify = true //nolint:gosec // verified below
|
||||
tlsConfig.VerifyPeerCertificate = func(rawCerts [][]byte, _ [][]*x509.Certificate) error {
|
||||
if len(rawCerts) == 0 {
|
||||
return fmt.Errorf("tls: server presented no certificate")
|
||||
}
|
||||
leaf, err := x509.ParseCertificate(rawCerts[0])
|
||||
if err != nil {
|
||||
return fmt.Errorf("tls: parse leaf certificate: %w", err)
|
||||
}
|
||||
sum := sha256.Sum256(leaf.RawSubjectPublicKeyInfo)
|
||||
got := hex.EncodeToString(sum[:])
|
||||
if subtle.ConstantTimeCompare([]byte(got), []byte(pin)) != 1 {
|
||||
return fmt.Errorf("tls: server public-key pin mismatch")
|
||||
}
|
||||
return nil
|
||||
}
|
||||
} else {
|
||||
// Development-only self-signed test mode. tlsInsecureEnabled() cannot
|
||||
// become true in a release build.
|
||||
tlsConfig.InsecureSkipVerify = true //nolint:gosec // opt-in development mode
|
||||
}
|
||||
client.Transport = &http.Transport{TLSClientConfig: tlsConfig}
|
||||
return client
|
||||
}
|
||||
|
||||
func normalizeServerCertPin(pin string) string {
|
||||
pin = strings.ToLower(strings.TrimSpace(pin))
|
||||
pin = strings.NewReplacer("sha256:", "", ":", "", " ", "", "\t", "", "\n", "").Replace(pin)
|
||||
if len(pin) != sha256.Size*2 {
|
||||
return ""
|
||||
}
|
||||
if _, err := hex.DecodeString(pin); err != nil {
|
||||
return ""
|
||||
}
|
||||
return pin
|
||||
}
|
||||
|
||||
// apiBaseURL resolves the Go server API base (…/api) from branding.
|
||||
func apiBaseURL(b Branding) string {
|
||||
if b.Server != nil && strings.TrimSpace(b.Server.APIURL) != "" {
|
||||
|
||||
@@ -1,7 +1,11 @@
|
||||
package main
|
||||
|
||||
import (
|
||||
"crypto/sha256"
|
||||
"encoding/hex"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
)
|
||||
@@ -40,3 +44,33 @@ func TestHTTPGetKeepsVerificationEnabledUnlessDevelopmentOptIn(t *testing.T) {
|
||||
t.Fatal("development health probe did not configure insecure TLS")
|
||||
}
|
||||
}
|
||||
|
||||
func TestAPIHTTPClientWithPinVerifiesServerSPKI(t *testing.T) {
|
||||
server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
|
||||
w.WriteHeader(http.StatusNoContent)
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
sum := sha256.Sum256(server.Certificate().RawSubjectPublicKeyInfo)
|
||||
pin := hex.EncodeToString(sum[:])
|
||||
|
||||
response, err := apiHTTPClientWithPin(time.Second, pin).Get(server.URL)
|
||||
if err != nil {
|
||||
t.Fatalf("pinned request failed: %v", err)
|
||||
}
|
||||
_ = response.Body.Close()
|
||||
|
||||
if _, err := apiHTTPClientWithPin(time.Second, strings.Repeat("0", 64)).Get(server.URL); err == nil {
|
||||
t.Fatal("mismatched server pin unexpectedly succeeded")
|
||||
}
|
||||
}
|
||||
|
||||
func TestNormalizeServerCertPin(t *testing.T) {
|
||||
pin := "sha256:AA:BB " + strings.Repeat("0", 60)
|
||||
if got := normalizeServerCertPin(pin); got != "aabb"+strings.Repeat("0", 60) {
|
||||
t.Fatalf("normalized pin = %q", got)
|
||||
}
|
||||
if got := normalizeServerCertPin("not-a-pin"); got != "" {
|
||||
t.Fatalf("invalid pin normalized to %q", got)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -213,6 +213,9 @@ func (b Branding) validateReleaseProfile(now time.Time) error {
|
||||
if !allSecureAndAllowed(b.AllowedEndpoints, b.Server.Address, b.Server.APIURL, b.Server.CDAPURL) {
|
||||
return fmt.Errorf("release branding profile has unauthorized endpoint")
|
||||
}
|
||||
if b.Server.CertPin != "" && normalizeServerCertPin(b.Server.CertPin) == "" {
|
||||
return fmt.Errorf("release branding profile has invalid certificate pin")
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
|
||||
@@ -5,6 +5,7 @@ import (
|
||||
"crypto/rand"
|
||||
"fmt"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/unitronix/betterdesk-support-agent/internal/brandprofile"
|
||||
)
|
||||
@@ -47,6 +48,10 @@ func TestReleaseBrandingRequiresValidSignedProfile(t *testing.T) {
|
||||
branding.Server.Address != "https://support.example.test" {
|
||||
t.Fatalf("unexpected branding: %+v", branding)
|
||||
}
|
||||
branding.Server.CertPin = "invalid-pin"
|
||||
if err := branding.validateReleaseProfile(time.Now()); err == nil {
|
||||
t.Fatal("release profile unexpectedly accepted an invalid certificate pin")
|
||||
}
|
||||
|
||||
if _, err := decodeBrandingProfile(profile, []byte(publicKeyResource), true); err == nil {
|
||||
t.Fatal("release profile unexpectedly accepted unsigned branding")
|
||||
|
||||
@@ -2,7 +2,6 @@ package main
|
||||
|
||||
import (
|
||||
"context"
|
||||
"crypto/tls"
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"net/http"
|
||||
@@ -135,11 +134,12 @@ func httpGet(endpoint string) ([]byte, time.Duration, error) {
|
||||
}
|
||||
|
||||
func healthHTTPClient(endpoint string) *http.Client {
|
||||
client := &http.Client{Timeout: 8 * time.Second}
|
||||
if strings.HasPrefix(endpoint, "https://") && tlsInsecureEnabled() {
|
||||
client.Transport = &http.Transport{
|
||||
TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, //nolint:gosec // opt-in dev only
|
||||
}
|
||||
if !strings.HasPrefix(strings.ToLower(endpoint), "https://") {
|
||||
return &http.Client{Timeout: 8 * time.Second}
|
||||
}
|
||||
return client
|
||||
pin := ""
|
||||
if b := GetBranding(); b.Server != nil {
|
||||
pin = b.Server.CertPin
|
||||
}
|
||||
return apiHTTPClientWithPin(8*time.Second, pin)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user