diff --git a/management/internals/modules/reverseproxy/reverseproxy.go b/management/internals/modules/reverseproxy/reverseproxy.go index 9708139f7..ded9c13cc 100644 --- a/management/internals/modules/reverseproxy/reverseproxy.go +++ b/management/internals/modules/reverseproxy/reverseproxy.go @@ -10,8 +10,8 @@ import ( "github.com/rs/xid" log "github.com/sirupsen/logrus" - "golang.org/x/crypto/bcrypt" + "github.com/netbirdio/netbird/shared/hash/argon2id" "github.com/netbirdio/netbird/util/crypt" "github.com/netbirdio/netbird/shared/management/http/api" @@ -78,19 +78,19 @@ type AuthConfig struct { func (a *AuthConfig) HashSecrets() error { if a.PasswordAuth != nil && a.PasswordAuth.Enabled && a.PasswordAuth.Password != "" { - hash, err := bcrypt.GenerateFromPassword([]byte(a.PasswordAuth.Password), 12) + hashedPassword, err := argon2id.Hash(a.PasswordAuth.Password) if err != nil { - return err + return fmt.Errorf("hash password: %w", err) } - a.PasswordAuth.Password = string(hash) + a.PasswordAuth.Password = hashedPassword } if a.PinAuth != nil && a.PinAuth.Enabled && a.PinAuth.Pin != "" { - hash, err := bcrypt.GenerateFromPassword([]byte(a.PinAuth.Pin), 12) + hashedPin, err := argon2id.Hash(a.PinAuth.Pin) if err != nil { - return err + return fmt.Errorf("hash pin: %w", err) } - a.PinAuth.Pin = string(hash) + a.PinAuth.Pin = hashedPin } return nil diff --git a/management/internals/modules/reverseproxy/reverseproxy_test.go b/management/internals/modules/reverseproxy/reverseproxy_test.go index 9fe1eeb43..5468cd018 100644 --- a/management/internals/modules/reverseproxy/reverseproxy_test.go +++ b/management/internals/modules/reverseproxy/reverseproxy_test.go @@ -1,12 +1,15 @@ package reverseproxy import ( + "errors" "fmt" + "strings" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/netbirdio/netbird/shared/hash/argon2id" "github.com/netbirdio/netbird/shared/management/proto" ) @@ -14,7 +17,7 @@ func validProxy() *ReverseProxy { return &ReverseProxy{ Name: "test", Domain: "example.com", - Targets: []Target{ + Targets: []*Target{ {TargetId: "peer-1", TargetType: TargetTypePeer, Host: "10.0.0.1", Port: 80, Protocol: "http", Enabled: true}, }, } @@ -56,9 +59,9 @@ func TestValidate_InvalidTargetType(t *testing.T) { func TestValidate_ResourceTarget(t *testing.T) { rp := validProxy() - rp.Targets = append(rp.Targets, Target{ + rp.Targets = append(rp.Targets, &Target{ TargetId: "resource-1", - TargetType: TargetTypeResource, + TargetType: TargetTypeHost, Host: "example.org", Port: 443, Protocol: "https", @@ -69,7 +72,7 @@ func TestValidate_ResourceTarget(t *testing.T) { func TestValidate_MultipleTargetsOneInvalid(t *testing.T) { rp := validProxy() - rp.Targets = append(rp.Targets, Target{ + rp.Targets = append(rp.Targets, &Target{ TargetId: "", TargetType: TargetTypePeer, Host: "10.0.0.2", @@ -171,7 +174,7 @@ func TestToProtoMapping_PortInTargetURL(t *testing.T) { ID: "test-id", AccountID: "acc-1", Domain: "example.com", - Targets: []Target{ + Targets: []*Target{ { TargetId: "peer-1", TargetType: TargetTypePeer, @@ -194,7 +197,7 @@ func TestToProtoMapping_DisabledTargetSkipped(t *testing.T) { ID: "test-id", AccountID: "acc-1", Domain: "example.com", - Targets: []Target{ + Targets: []*Target{ {TargetId: "peer-1", TargetType: TargetTypePeer, Host: "10.0.0.1", Port: 8080, Protocol: "http", Enabled: false}, {TargetId: "peer-2", TargetType: TargetTypePeer, Host: "10.0.0.2", Port: 9090, Protocol: "http", Enabled: true}, }, @@ -221,3 +224,182 @@ func TestToProtoMapping_OperationTypes(t *testing.T) { }) } } + +func TestAuthConfig_HashSecrets(t *testing.T) { + tests := []struct { + name string + config *AuthConfig + wantErr bool + validate func(*testing.T, *AuthConfig) + }{ + { + name: "hash password successfully", + config: &AuthConfig{ + PasswordAuth: &PasswordAuthConfig{ + Enabled: true, + Password: "testPassword123", + }, + }, + wantErr: false, + validate: func(t *testing.T, config *AuthConfig) { + if !strings.HasPrefix(config.PasswordAuth.Password, "$argon2id$") { + t.Errorf("Password not hashed with argon2id, got: %s", config.PasswordAuth.Password) + } + // Verify the hash can be verified + if err := argon2id.Verify("testPassword123", config.PasswordAuth.Password); err != nil { + t.Errorf("Hash verification failed: %v", err) + } + }, + }, + { + name: "hash PIN successfully", + config: &AuthConfig{ + PinAuth: &PINAuthConfig{ + Enabled: true, + Pin: "123456", + }, + }, + wantErr: false, + validate: func(t *testing.T, config *AuthConfig) { + if !strings.HasPrefix(config.PinAuth.Pin, "$argon2id$") { + t.Errorf("PIN not hashed with argon2id, got: %s", config.PinAuth.Pin) + } + // Verify the hash can be verified + if err := argon2id.Verify("123456", config.PinAuth.Pin); err != nil { + t.Errorf("Hash verification failed: %v", err) + } + }, + }, + { + name: "hash both password and PIN", + config: &AuthConfig{ + PasswordAuth: &PasswordAuthConfig{ + Enabled: true, + Password: "password", + }, + PinAuth: &PINAuthConfig{ + Enabled: true, + Pin: "9999", + }, + }, + wantErr: false, + validate: func(t *testing.T, config *AuthConfig) { + if !strings.HasPrefix(config.PasswordAuth.Password, "$argon2id$") { + t.Errorf("Password not hashed with argon2id") + } + if !strings.HasPrefix(config.PinAuth.Pin, "$argon2id$") { + t.Errorf("PIN not hashed with argon2id") + } + if err := argon2id.Verify("password", config.PasswordAuth.Password); err != nil { + t.Errorf("Password hash verification failed: %v", err) + } + if err := argon2id.Verify("9999", config.PinAuth.Pin); err != nil { + t.Errorf("PIN hash verification failed: %v", err) + } + }, + }, + { + name: "skip disabled password auth", + config: &AuthConfig{ + PasswordAuth: &PasswordAuthConfig{ + Enabled: false, + Password: "password", + }, + }, + wantErr: false, + validate: func(t *testing.T, config *AuthConfig) { + if config.PasswordAuth.Password != "password" { + t.Errorf("Disabled password auth should not be hashed") + } + }, + }, + { + name: "skip empty password", + config: &AuthConfig{ + PasswordAuth: &PasswordAuthConfig{ + Enabled: true, + Password: "", + }, + }, + wantErr: false, + validate: func(t *testing.T, config *AuthConfig) { + if config.PasswordAuth.Password != "" { + t.Errorf("Empty password should remain empty") + } + }, + }, + { + name: "skip nil password auth", + config: &AuthConfig{ + PasswordAuth: nil, + PinAuth: &PINAuthConfig{ + Enabled: true, + Pin: "1234", + }, + }, + wantErr: false, + validate: func(t *testing.T, config *AuthConfig) { + if config.PasswordAuth != nil { + t.Errorf("PasswordAuth should remain nil") + } + if !strings.HasPrefix(config.PinAuth.Pin, "$argon2id$") { + t.Errorf("PIN should still be hashed") + } + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.config.HashSecrets() + if (err != nil) != tt.wantErr { + t.Errorf("HashSecrets() error = %v, wantErr %v", err, tt.wantErr) + return + } + if tt.validate != nil { + tt.validate(t, tt.config) + } + }) + } +} + +func TestAuthConfig_HashSecrets_VerifyIncorrectSecret(t *testing.T) { + config := &AuthConfig{ + PasswordAuth: &PasswordAuthConfig{ + Enabled: true, + Password: "correctPassword", + }, + } + + if err := config.HashSecrets(); err != nil { + t.Fatalf("HashSecrets() error = %v", err) + } + + // Verify with wrong password should fail + err := argon2id.Verify("wrongPassword", config.PasswordAuth.Password) + if !errors.Is(err, argon2id.ErrMismatchedHashAndPassword) { + t.Errorf("Expected ErrMismatchedHashAndPassword, got %v", err) + } +} + +func TestAuthConfig_ClearSecrets(t *testing.T) { + config := &AuthConfig{ + PasswordAuth: &PasswordAuthConfig{ + Enabled: true, + Password: "hashedPassword", + }, + PinAuth: &PINAuthConfig{ + Enabled: true, + Pin: "hashedPin", + }, + } + + config.ClearSecrets() + + if config.PasswordAuth.Password != "" { + t.Errorf("Password not cleared, got: %s", config.PasswordAuth.Password) + } + if config.PinAuth.Pin != "" { + t.Errorf("PIN not cleared, got: %s", config.PinAuth.Pin) + } +} diff --git a/management/internals/shared/grpc/proxy.go b/management/internals/shared/grpc/proxy.go index 235110927..6f6d66f94 100644 --- a/management/internals/shared/grpc/proxy.go +++ b/management/internals/shared/grpc/proxy.go @@ -16,7 +16,6 @@ import ( "github.com/coreos/go-oidc/v3/oidc" log "github.com/sirupsen/logrus" - "golang.org/x/crypto/bcrypt" "golang.org/x/oauth2" "google.golang.org/grpc/codes" "google.golang.org/grpc/peer" @@ -29,6 +28,7 @@ import ( "github.com/netbirdio/netbird/management/server/types" "github.com/netbirdio/netbird/management/server/users" proxyauth "github.com/netbirdio/netbird/proxy/auth" + "github.com/netbirdio/netbird/shared/hash/argon2id" "github.com/netbirdio/netbird/shared/management/proto" ) @@ -441,9 +441,9 @@ func (s *ProxyServiceServer) Authenticate(ctx context.Context, req *proto.Authen // Break here and use the default authenticated == false. break } - err = bcrypt.CompareHashAndPassword([]byte(auth.Pin), []byte(v.Pin.GetPin())) + err = argon2id.Verify(v.Pin.GetPin(), auth.Pin) if err != nil { - if errors.Is(err, bcrypt.ErrMismatchedHashAndPassword) { + if errors.Is(err, argon2id.ErrMismatchedHashAndPassword) { log.WithContext(ctx).Tracef("PIN authentication failed: invalid PIN") } else { log.WithContext(ctx).Errorf("PIN authentication error: %v", err) @@ -460,9 +460,9 @@ func (s *ProxyServiceServer) Authenticate(ctx context.Context, req *proto.Authen // Break here and use the default authenticated == false. break } - err = bcrypt.CompareHashAndPassword([]byte(auth.Password), []byte(v.Password.GetPassword())) + err = argon2id.Verify(v.Password.GetPassword(), auth.Password) if err != nil { - if errors.Is(err, bcrypt.ErrMismatchedHashAndPassword) { + if errors.Is(err, argon2id.ErrMismatchedHashAndPassword) { log.WithContext(ctx).Tracef("Password authentication failed: invalid password") } else { log.WithContext(ctx).Errorf("Password authentication error: %v", err) diff --git a/shared/hash/argon2id/argon2id.go b/shared/hash/argon2id/argon2id.go new file mode 100644 index 000000000..8d493aaba --- /dev/null +++ b/shared/hash/argon2id/argon2id.go @@ -0,0 +1,136 @@ +package argon2id + +import ( + "crypto/rand" + "crypto/subtle" + "encoding/base64" + "errors" + "fmt" + "strings" + + "golang.org/x/crypto/argon2" +) + +const ( + argon2Memory = 19456 + argon2Iterations = 2 + argon2Parallelism = 1 + argon2SaltLength = 16 + argon2KeyLength = 32 +) + +var ( + // ErrInvalidHash is returned when the hash string format is invalid + ErrInvalidHash = errors.New("invalid hash format") + + // ErrIncompatibleVersion is returned when the Argon2 version is not supported + ErrIncompatibleVersion = errors.New("incompatible argon2 version") + + // ErrMismatchedHashAndPassword is returned when password verification fails + ErrMismatchedHashAndPassword = errors.New("password does not match hash") +) + +func Hash(secret string) (string, error) { + salt := make([]byte, argon2SaltLength) + if _, err := rand.Read(salt); err != nil { + return "", fmt.Errorf("failed to generate salt: %w", err) + } + + hash := argon2.IDKey( + []byte(secret), + salt, + argon2Iterations, + argon2Memory, + argon2Parallelism, + argon2KeyLength, + ) + + encodedSalt := base64.RawStdEncoding.EncodeToString(salt) + encodedHash := base64.RawStdEncoding.EncodeToString(hash) + + return fmt.Sprintf( + "$argon2id$v=%d$m=%d,t=%d,p=%d$%s$%s", + argon2.Version, + argon2Memory, + argon2Iterations, + argon2Parallelism, + encodedSalt, + encodedHash, + ), nil +} + +func Verify(secret, encodedHash string) error { + params, salt, hash, err := decodeHash(encodedHash) + if err != nil { + return err + } + + computedHash := argon2.IDKey( + []byte(secret), + salt, + params.iterations, + params.memory, + params.parallelism, + params.keyLength, + ) + + if subtle.ConstantTimeCompare(hash, computedHash) == 1 { + return nil + } + + return ErrMismatchedHashAndPassword +} + +type hashParams struct { + memory uint32 + iterations uint32 + parallelism uint8 + keyLength uint32 + version int +} + +func decodeHash(encodedHash string) (*hashParams, []byte, []byte, error) { + parts := strings.Split(encodedHash, "$") + + if len(parts) != 6 { + return nil, nil, nil, ErrInvalidHash + } + + if parts[1] != "argon2id" { + return nil, nil, nil, ErrInvalidHash + } + + var version int + if _, err := fmt.Sscanf(parts[2], "v=%d", &version); err != nil { + return nil, nil, nil, fmt.Errorf("%w: invalid version: %v", ErrInvalidHash, err) + } + if version != argon2.Version { + return nil, nil, nil, ErrIncompatibleVersion + } + + var memory, iterations uint32 + var parallelism uint8 + if _, err := fmt.Sscanf(parts[3], "m=%d,t=%d,p=%d", &memory, &iterations, ¶llelism); err != nil { + return nil, nil, nil, fmt.Errorf("%w: invalid parameters: %v", ErrInvalidHash, err) + } + + salt, err := base64.RawStdEncoding.DecodeString(parts[4]) + if err != nil { + return nil, nil, nil, fmt.Errorf("%w: invalid salt encoding: %v", ErrInvalidHash, err) + } + + hash, err := base64.RawStdEncoding.DecodeString(parts[5]) + if err != nil { + return nil, nil, nil, fmt.Errorf("%w: invalid hash encoding: %v", ErrInvalidHash, err) + } + + params := &hashParams{ + memory: memory, + iterations: iterations, + parallelism: parallelism, + keyLength: uint32(len(hash)), + version: version, + } + + return params, salt, hash, nil +} diff --git a/shared/hash/argon2id/argon2id_test.go b/shared/hash/argon2id/argon2id_test.go new file mode 100644 index 000000000..f907a1687 --- /dev/null +++ b/shared/hash/argon2id/argon2id_test.go @@ -0,0 +1,327 @@ +package argon2id + +import ( + "errors" + "strings" + "testing" + + "golang.org/x/crypto/argon2" +) + +func TestHash(t *testing.T) { + tests := []struct { + name string + secret string + }{ + { + name: "simple password", + secret: "password123", + }, + { + name: "complex password with special chars", + secret: "P@ssw0rd!#$%^&*()", + }, + { + name: "long password", + secret: strings.Repeat("a", 100), + }, + { + name: "empty password", + secret: "", + }, + { + name: "unicode password", + secret: "пароль密码🔐", + }, + { + name: "numeric PIN", + secret: "123456", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + hash, err := Hash(tt.secret) + if err != nil { + t.Fatalf("Hash() error = %v", err) + } + + // Verify hash format + if !strings.HasPrefix(hash, "$argon2id$") { + t.Errorf("Hash() = %v, want hash starting with $argon2id$", hash) + } + + // Verify hash has correct number of components + parts := strings.Split(hash, "$") + if len(parts) != 6 { + t.Errorf("Hash() has %d parts, want 6", len(parts)) + } + + // Verify version is present + if !strings.HasPrefix(hash, "$argon2id$v=") { + t.Errorf("Hash() missing version, got %v", hash) + } + + // Verify each hash is unique (different salt) + hash2, err := Hash(tt.secret) + if err != nil { + t.Fatalf("Hash() second call error = %v", err) + } + if hash == hash2 { + t.Error("Hash() produces identical hashes for same input (salt not random)") + } + }) + } +} + +func TestVerify(t *testing.T) { + tests := []struct { + name string + secret string + wantError error + }{ + { + name: "valid password", + secret: "correctPassword", + wantError: nil, + }, + { + name: "valid PIN", + secret: "1234", + wantError: nil, + }, + { + name: "empty secret", + secret: "", + wantError: nil, + }, + { + name: "unicode secret", + secret: "密码🔐", + wantError: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Generate hash + hash, err := Hash(tt.secret) + if err != nil { + t.Fatalf("Hash() error = %v", err) + } + + // Verify correct secret + err = Verify(tt.secret, hash) + if !errors.Is(err, tt.wantError) { + t.Errorf("Verify() error = %v, wantError %v", err, tt.wantError) + } + }) + } +} + +func TestVerifyIncorrectPassword(t *testing.T) { + secret := "correctPassword" + wrongSecret := "wrongPassword" + + hash, err := Hash(secret) + if err != nil { + t.Fatalf("Hash() error = %v", err) + } + + err = Verify(wrongSecret, hash) + if !errors.Is(err, ErrMismatchedHashAndPassword) { + t.Errorf("Verify() error = %v, want %v", err, ErrMismatchedHashAndPassword) + } +} + +func TestVerifyInvalidHashFormat(t *testing.T) { + tests := []struct { + name string + invalidHash string + expectedError error + }{ + { + name: "empty hash", + invalidHash: "", + expectedError: ErrInvalidHash, + }, + { + name: "wrong algorithm", + invalidHash: "$bcrypt$v=19$m=19456,t=2,p=1$c2FsdA$aGFzaA", + expectedError: ErrInvalidHash, + }, + { + name: "missing parts", + invalidHash: "$argon2id$v=19$m=19456", + expectedError: ErrInvalidHash, + }, + { + name: "too many parts", + invalidHash: "$argon2id$v=19$m=19456,t=2,p=1$salt$hash$extra", + expectedError: ErrInvalidHash, + }, + { + name: "invalid version format", + invalidHash: "$argon2id$vXX$m=19456,t=2,p=1$c2FsdA$aGFzaA", + expectedError: ErrInvalidHash, + }, + { + name: "invalid parameters format", + invalidHash: "$argon2id$v=19$mXX,tYY,pZZ$c2FsdA$aGFzaA", + expectedError: ErrInvalidHash, + }, + { + name: "invalid salt base64", + invalidHash: "$argon2id$v=19$m=19456,t=2,p=1$not-valid-base64!@#$aGFzaA", + expectedError: ErrInvalidHash, + }, + { + name: "invalid hash base64", + invalidHash: "$argon2id$v=19$m=19456,t=2,p=1$c2FsdA$not-valid-base64!@#", + expectedError: ErrInvalidHash, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := Verify("password", tt.invalidHash) + if err == nil { + t.Errorf("Verify() expected error, got nil") + return + } + + if !errors.Is(err, tt.expectedError) && !strings.Contains(err.Error(), tt.expectedError.Error()) { + t.Errorf("Verify() error = %v, want error containing %v", err, tt.expectedError) + } + }) + } +} + +func TestVerifyIncompatibleVersion(t *testing.T) { + // Manually craft a hash with wrong version + invalidVersionHash := "$argon2id$v=18$m=19456,t=2,p=1$c2FsdDEyMzQ1Njc4OTA$aGFzaDEyMzQ1Njc4OTBhYmNkZWZnaGlqa2xtbm9w" + + err := Verify("password", invalidVersionHash) + if !errors.Is(err, ErrIncompatibleVersion) { + t.Errorf("Verify() error = %v, want %v", err, ErrIncompatibleVersion) + } +} + +func TestHashDeterminism(t *testing.T) { + // Ensure different hashes for same password (random salt) + password := "testPassword" + hashes := make(map[string]bool) + + for i := 0; i < 10; i++ { + hash, err := Hash(password) + if err != nil { + t.Fatalf("Hash() error = %v", err) + } + if hashes[hash] { + t.Error("Hash() produced duplicate hash (salt generation may be broken)") + } + hashes[hash] = true + } + + if len(hashes) != 10 { + t.Errorf("Expected 10 unique hashes, got %d", len(hashes)) + } +} + +func TestOWASPCompliance(t *testing.T) { + // Test that generated hashes use OWASP-recommended parameters + secret := "testPassword" + hash, err := Hash(secret) + if err != nil { + t.Fatalf("Hash() error = %v", err) + } + + params, _, _, err := decodeHash(hash) + if err != nil { + t.Fatalf("decodeHash() error = %v", err) + } + + // Verify OWASP minimum baseline parameters + if params.memory != 19456 { + t.Errorf("memory = %d, want 19456 (OWASP baseline)", params.memory) + } + if params.iterations != 2 { + t.Errorf("iterations = %d, want 2 (OWASP baseline)", params.iterations) + } + if params.parallelism != 1 { + t.Errorf("parallelism = %d, want 1 (OWASP baseline)", params.parallelism) + } + if params.keyLength != 32 { + t.Errorf("keyLength = %d, want 32", params.keyLength) + } + if params.version != argon2.Version { + t.Errorf("version = %d, want %d", params.version, argon2.Version) + } +} + +func TestConstantTimeComparison(t *testing.T) { + // This test verifies that Verify() is using constant-time comparison + // by ensuring it doesn't fail differently for similar vs different hashes + secret := "password123" + wrongSecret := "password124" // One character different + + hash, err := Hash(secret) + if err != nil { + t.Fatalf("Hash() error = %v", err) + } + + // Both wrong passwords should return the same error + err1 := Verify(wrongSecret, hash) + err2 := Verify("completelydifferent", hash) + + if !errors.Is(err1, ErrMismatchedHashAndPassword) { + t.Errorf("Verify() error = %v, want %v", err1, ErrMismatchedHashAndPassword) + } + if !errors.Is(err2, ErrMismatchedHashAndPassword) { + t.Errorf("Verify() error = %v, want %v", err2, ErrMismatchedHashAndPassword) + } + + // Errors should be identical (same error type and message) + if err1.Error() != err2.Error() { + t.Error("Verify() returns different errors for different wrong passwords (potential timing attack)") + } +} + +func TestCaseSensitivity(t *testing.T) { + // Passwords should be case-sensitive + secret := "Password123" + wrongSecret := "password123" + + hash, err := Hash(secret) + if err != nil { + t.Fatalf("Hash() error = %v", err) + } + + // Correct password should verify + if err := Verify(secret, hash); err != nil { + t.Errorf("Verify() with correct password error = %v, want nil", err) + } + + // Wrong case should not verify + if err := Verify(wrongSecret, hash); !errors.Is(err, ErrMismatchedHashAndPassword) { + t.Errorf("Verify() with wrong case error = %v, want %v", err, ErrMismatchedHashAndPassword) + } +} + +// Benchmark tests +func BenchmarkHash(b *testing.B) { + secret := "benchmarkPassword123" + b.ResetTimer() + for i := 0; i < b.N; i++ { + _, _ = Hash(secret) + } +} + +func BenchmarkVerify(b *testing.B) { + secret := "benchmarkPassword123" + hash, _ := Hash(secret) + b.ResetTimer() + for i := 0; i < b.N; i++ { + _ = Verify(secret, hash) + } +}