Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 41 additions & 5 deletions client/internal/auth/pkce_flow.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"net"
"net/http"
"net/url"
"strconv"
"strings"
"time"

Expand Down Expand Up @@ -46,9 +47,10 @@ type PKCEAuthorizationFlow struct {
func NewPKCEAuthorizationFlow(config internal.PKCEAuthProviderConfig) (*PKCEAuthorizationFlow, error) {
var availableRedirectURL string

// find the first available redirect URL
excludedRanges := getSystemExcludedPortRanges()

for _, redirectURL := range config.RedirectURLs {
if !isRedirectURLPortUsed(redirectURL) {
if !isRedirectURLPortUsed(redirectURL, excludedRanges) {
availableRedirectURL = redirectURL
break
}
Expand Down Expand Up @@ -282,15 +284,22 @@ func createCodeChallenge(codeVerifier string) string {
return base64.RawURLEncoding.EncodeToString(sha2[:])
}

// isRedirectURLPortUsed checks if the port used in the redirect URL is in use.
func isRedirectURLPortUsed(redirectURL string) bool {
// isRedirectURLPortUsed checks if the port used in the redirect URL is in use or excluded on Windows.
func isRedirectURLPortUsed(redirectURL string, excludedRanges []excludedPortRange) bool {
parsedURL, err := url.Parse(redirectURL)
if err != nil {
log.Errorf("failed to parse redirect URL: %v", err)
return true
Comment thread
lixmal marked this conversation as resolved.
}

addr := fmt.Sprintf(":%s", parsedURL.Port())
port := parsedURL.Port()

if isPortInExcludedRange(port, excludedRanges) {
log.Warnf("port %s is in Windows excluded port range, skipping", port)
return true
}

addr := fmt.Sprintf(":%s", port)
conn, err := net.DialTimeout("tcp", addr, 3*time.Second)
if err != nil {
return false
Expand All @@ -304,6 +313,33 @@ func isRedirectURLPortUsed(redirectURL string) bool {
return true
}

// excludedPortRange represents a range of excluded ports.
type excludedPortRange struct {
start int
end int
}

// isPortInExcludedRange checks if the given port is in any of the excluded ranges.
func isPortInExcludedRange(port string, excludedRanges []excludedPortRange) bool {
if len(excludedRanges) == 0 {
return false
}

portNum, err := strconv.Atoi(port)
if err != nil {
log.Debugf("invalid port number %s: %v", port, err)
return false
}

for _, r := range excludedRanges {
if portNum >= r.start && portNum <= r.end {
return true
}
}

return false
}

func renderPKCEFlowTmpl(w http.ResponseWriter, authError error) {
tmpl, err := template.New("pkce-auth-flow").Parse(templates.PKCEAuthMsgTmpl)
if err != nil {
Expand Down
8 changes: 8 additions & 0 deletions client/internal/auth/pkce_flow_other.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
//go:build !windows

package auth

// getSystemExcludedPortRanges returns nil on non-Windows platforms.
func getSystemExcludedPortRanges() []excludedPortRange {
return nil
}
147 changes: 147 additions & 0 deletions client/internal/auth/pkce_flow_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@ package auth

import (
"context"
"fmt"
"net"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/netbirdio/netbird/client/internal"
Expand Down Expand Up @@ -69,3 +72,147 @@ func TestPromptLogin(t *testing.T) {
})
}
}

func TestIsPortInExcludedRange(t *testing.T) {
tests := []struct {
name string
port string
excludedRanges []excludedPortRange
expectedBlocked bool
}{
{
name: "Port in excluded range",
port: "8080",
excludedRanges: []excludedPortRange{{start: 8000, end: 8100}},
expectedBlocked: true,
},
{
name: "Port at start of range",
port: "8000",
excludedRanges: []excludedPortRange{{start: 8000, end: 8100}},
expectedBlocked: true,
},
{
name: "Port at end of range",
port: "8100",
excludedRanges: []excludedPortRange{{start: 8000, end: 8100}},
expectedBlocked: true,
},
{
name: "Port before range",
port: "7999",
excludedRanges: []excludedPortRange{{start: 8000, end: 8100}},
expectedBlocked: false,
},
{
name: "Port after range",
port: "8101",
excludedRanges: []excludedPortRange{{start: 8000, end: 8100}},
expectedBlocked: false,
},
{
name: "Empty excluded ranges",
port: "8080",
excludedRanges: []excludedPortRange{},
expectedBlocked: false,
},
{
name: "Nil excluded ranges",
port: "8080",
excludedRanges: nil,
expectedBlocked: false,
},
{
name: "Multiple ranges - port in second range",
port: "9050",
excludedRanges: []excludedPortRange{
{start: 8000, end: 8100},
{start: 9000, end: 9100},
},
expectedBlocked: true,
},
{
name: "Multiple ranges - port not in any range",
port: "8500",
excludedRanges: []excludedPortRange{
{start: 8000, end: 8100},
{start: 9000, end: 9100},
},
expectedBlocked: false,
},
{
name: "Invalid port string",
port: "invalid",
excludedRanges: []excludedPortRange{{start: 8000, end: 8100}},
expectedBlocked: false,
},
{
name: "Empty port string",
port: "",
excludedRanges: []excludedPortRange{{start: 8000, end: 8100}},
expectedBlocked: false,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := isPortInExcludedRange(tt.port, tt.excludedRanges)
assert.Equal(t, tt.expectedBlocked, result, "Port exclusion check mismatch")
})
}
}

func TestIsRedirectURLPortUsed(t *testing.T) {
listener, err := net.Listen("tcp", "127.0.0.1:0")
require.NoError(t, err)
defer func() {
_ = listener.Close()
}()

usedPort := listener.Addr().(*net.TCPAddr).Port

tests := []struct {
name string
redirectURL string
excludedRanges []excludedPortRange
expectedUsed bool
}{
{
name: "Port in excluded range",
redirectURL: "http://127.0.0.1:8080/",
excludedRanges: []excludedPortRange{{start: 8000, end: 8100}},
expectedUsed: true,
},
{
name: "Port actually in use",
redirectURL: fmt.Sprintf("http://127.0.0.1:%d/", usedPort),
excludedRanges: nil,
expectedUsed: true,
},
{
name: "Port not in use and not excluded",
redirectURL: "http://127.0.0.1:65432/",
excludedRanges: nil,
expectedUsed: false,
},
{
name: "Invalid URL without port",
redirectURL: "not-a-valid-url",
excludedRanges: nil,
expectedUsed: false,
},
{
name: "Port excluded even if not in use",
redirectURL: "http://127.0.0.1:8050/",
excludedRanges: []excludedPortRange{{start: 8000, end: 8100}},
expectedUsed: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := isRedirectURLPortUsed(tt.redirectURL, tt.excludedRanges)
assert.Equal(t, tt.expectedUsed, result, "Port usage check mismatch")
})
}
}
137 changes: 137 additions & 0 deletions client/internal/auth/pkce_flow_test_windows.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
//go:build windows

package auth

import (
"fmt"
"net"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"golang.org/x/sys/windows/registry"

"github.com/netbirdio/netbird/client/internal"
)

func TestNewPKCEAuthorizationFlow_ExcludedPorts(t *testing.T) {

Check warning on line 17 in client/internal/auth/pkce_flow_test_windows.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

This function has 110 lines of code, which is greater than the 100 authorized. Split it into smaller functions.

See more on https://sonarcloud.io/project/issues?id=netbirdio_netbird&issues=AZq3SCLpgAbn1_O1Gixq&open=AZq3SCLpgAbn1_O1Gixq&pullRequest=4853
k, err := registry.OpenKey(registry.LOCAL_MACHINE,
`SYSTEM\CurrentControlSet\Services\Tcpip\Parameters`,
registry.QUERY_VALUE|registry.SET_VALUE)
if err != nil {
t.Skipf("Cannot open registry key (may need admin privileges): %v", err)
return
}
defer func() {
_ = k.Close()
}()

originalReservedPorts, _, err := k.GetStringsValue("ReservedPorts")
if err != nil && err != registry.ErrNotExist {
t.Skipf("Cannot read ReservedPorts from registry: %v", err)
return
}

defer func() {
if err == registry.ErrNotExist {
_ = k.DeleteValue("ReservedPorts")
} else {
_ = k.SetStringsValue("ReservedPorts", originalReservedPorts)
}
}()

testExcludedRanges := []string{
"8080-8090",
"9000-9010",
}

if err := k.SetStringsValue("ReservedPorts", testExcludedRanges); err != nil {
t.Skipf("Cannot write ReservedPorts to registry (may need admin privileges): %v", err)
return
}

listener1, err := net.Listen("tcp", "127.0.0.1:0")
require.NoError(t, err)
defer func() {
_ = listener1.Close()
}()
usedPort1 := listener1.Addr().(*net.TCPAddr).Port

listener2, err := net.Listen("tcp", "127.0.0.1:0")
require.NoError(t, err)
defer func() {
_ = listener2.Close()
}()
usedPort2 := listener2.Addr().(*net.TCPAddr).Port

availablePort := 65432

tests := []struct {
name string
redirectURLs []string
expectError bool
expectedPort int
}{
{
name: "Skip excluded port range, use next available",
redirectURLs: []string{
"http://127.0.0.1:8085/",
fmt.Sprintf("http://127.0.0.1:%d/", availablePort),
},
expectError: false,
expectedPort: availablePort,
},
{
name: "Skip multiple excluded ranges",
redirectURLs: []string{
"http://127.0.0.1:8082/",
"http://127.0.0.1:9005/",
fmt.Sprintf("http://127.0.0.1:%d/", availablePort),
},
expectError: false,
expectedPort: availablePort,
},
{
name: "Skip port in use, use next available",
redirectURLs: []string{
fmt.Sprintf("http://127.0.0.1:%d/", usedPort1),
fmt.Sprintf("http://127.0.0.1:%d/", availablePort),
},
expectError: false,
expectedPort: availablePort,
},
{
name: "All ports excluded or in use",
redirectURLs: []string{
fmt.Sprintf("http://127.0.0.1:%d/", usedPort1),
fmt.Sprintf("http://127.0.0.1:%d/", usedPort2),
},
expectError: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
config := internal.PKCEAuthProviderConfig{
ClientID: "test-client-id",
Audience: "test-audience",
TokenEndpoint: "https://test-token-endpoint.com/token",
Scope: "openid email profile",
AuthorizationEndpoint: "https://test-auth-endpoint.com/authorize",
RedirectURLs: tt.redirectURLs,
UseIDToken: true,
}

flow, err := NewPKCEAuthorizationFlow(config)

if tt.expectError {
assert.Error(t, err, "Expected error when no ports available")
assert.Nil(t, flow)
} else {
require.NoError(t, err)
require.NotNil(t, flow)
assert.Contains(t, flow.oAuthConfig.RedirectURL, fmt.Sprintf(":%d", tt.expectedPort))
}
})
}
}
Loading
Loading