Skip to content

Commit 5403956

Browse files
committed
Enforce password policy in HashPassword
1 parent 21d755e commit 5403956

File tree

4 files changed

+64
-28
lines changed

4 files changed

+64
-28
lines changed

ground-control/internal/auth/password.go

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,21 @@ import (
88
"github.com/container-registry/harbor-satellite/ground-control/pkg/crypto"
99
)
1010

11+
type PasswordPolicyError struct {
12+
Err error
13+
}
14+
15+
func (e *PasswordPolicyError) Error() string { return e.Err.Error() }
16+
func (e *PasswordPolicyError) Unwrap() error { return e.Err }
17+
1118
// HashPassword creates an Argon2id hash of the password.
1219
func HashPassword(password string) (string, error) {
13-
return crypto.HashSecret(password)
20+
policy := LoadPolicyFromEnv()
21+
if err := policy.Validate(password); err != nil {
22+
return "", &PasswordPolicyError{Err: err}
23+
}
24+
25+
return crypto.HashSecret(password)
1426
}
1527

1628
// VerifyPassword compares a password against an Argon2id hash.

ground-control/internal/auth/password_test.go

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import (
77
)
88

99
func TestVerifyPassword(t *testing.T) {
10-
password := "test-password-123"
10+
password := "Test-password-123"
1111
hash, err := HashPassword(password)
1212
require.NoError(t, err)
1313

@@ -61,28 +61,41 @@ func TestHashPassword(t *testing.T) {
6161
tests := []struct {
6262
name string
6363
password string
64+
wantErr bool
6465
}{
6566
{
6667
name: "normal password",
6768
password: "MySecurePassword123!",
69+
wantErr: false,
6870
},
6971
{
7072
name: "empty password",
7173
password: "",
74+
wantErr: true,
7275
},
7376
{
77+
// Make it long AND policy-compliant (upper/lower/number) so if it fails,
78+
// it's likely due to max length, not missing character classes.
79+
// NOTE: if this ends up under 128 chars in your policy, it will pass.
7480
name: "long password",
75-
password: "a very long password that exceeds typical password length requirements for testing purposes",
81+
password: "A1" + "this-is-a-very-long-password-used-for-testing-length-behavior-and-it-has-lowercase-too",
82+
wantErr: false, // flip to true if you change this to exceed policy MaxLength
7683
},
7784
{
7885
name: "special characters",
79-
password: "p@$$w0rd!#%&*()[]{}",
86+
password: "Test@#$%^&*123",
87+
wantErr: false,
8088
},
8189
}
8290

8391
for _, tt := range tests {
8492
t.Run(tt.name, func(t *testing.T) {
8593
hash, err := HashPassword(tt.password)
94+
if tt.wantErr {
95+
require.Error(t, err)
96+
return
97+
}
98+
8699
require.NoError(t, err)
87100
require.NotEmpty(t, hash)
88101
require.Contains(t, hash, "$argon2id$")
@@ -95,7 +108,7 @@ func TestHashPassword(t *testing.T) {
95108
}
96109

97110
func TestHashPassword_UniqueHashes(t *testing.T) {
98-
password := "same-password"
111+
password := "SamePassword1!"
99112
hash1, err := HashPassword(password)
100113
require.NoError(t, err)
101114

ground-control/internal/server/bootstrap.go

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"fmt"
66
"log"
77
"os"
8+
"errors"
89

910
"github.com/container-registry/harbor-satellite/ground-control/internal/auth"
1011
"github.com/container-registry/harbor-satellite/ground-control/internal/database"
@@ -29,15 +30,16 @@ func (s *Server) BootstrapSystemAdmin(ctx context.Context) error {
2930
return fmt.Errorf("ADMIN_PASSWORD environment variable is required for initial setup")
3031
}
3132

32-
if err := s.passwordPolicy.Validate(password); err != nil {
33-
return fmt.Errorf("ADMIN_PASSWORD invalid: %w", err)
34-
}
35-
3633
hash, err := auth.HashPassword(password)
3734
if err != nil {
35+
var pe *auth.PasswordPolicyError
36+
if errors.As(err, &pe) {
37+
return fmt.Errorf("ADMIN_PASSWORD invalid: %w", pe)
38+
}
3839
return fmt.Errorf("failed to hash admin password: %w", err)
3940
}
4041

42+
4143
_, err = s.dbQueries.CreateUser(ctx, database.CreateUserParams{
4244
Username: systemAdminUsername,
4345
PasswordHash: hash,

ground-control/internal/server/user_handlers.go

Lines changed: 28 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -57,17 +57,19 @@ func (s *Server) createUserHandler(w http.ResponseWriter, r *http.Request) {
5757
return
5858
}
5959

60-
if err := s.passwordPolicy.Validate(req.Password); err != nil {
61-
WriteJSONError(w, err.Error(), http.StatusBadRequest)
62-
return
63-
}
64-
6560
hash, err := auth.HashPassword(req.Password)
6661
if err != nil {
62+
var pe *auth.PasswordPolicyError
63+
if errors.As(err, &pe) {
64+
WriteJSONError(w, pe.Error(), http.StatusBadRequest)
65+
return
66+
}
67+
6768
WriteJSONError(w, "Internal server error", http.StatusInternalServerError)
6869
return
6970
}
7071

72+
7173
user, err := s.dbQueries.CreateUser(r.Context(), database.CreateUserParams{
7274
Username: req.Username,
7375
PasswordHash: hash,
@@ -201,11 +203,19 @@ func (s *Server) changeOwnPasswordHandler(w http.ResponseWriter, r *http.Request
201203
return
202204
}
203205

204-
if err := s.passwordPolicy.Validate(req.NewPassword); err != nil {
205-
WriteJSONError(w, err.Error(), http.StatusBadRequest)
206+
hash, err := auth.HashPassword(req.NewPassword)
207+
if err != nil {
208+
var pe *auth.PasswordPolicyError
209+
if errors.As(err, &pe) {
210+
WriteJSONError(w, pe.Error(), http.StatusBadRequest)
211+
return
212+
}
213+
214+
WriteJSONError(w, "Internal server error", http.StatusInternalServerError)
206215
return
207216
}
208217

218+
209219
// Verify current password
210220
user, err := s.dbQueries.GetUserByUsername(r.Context(), currentUser.Username)
211221
if err != nil {
@@ -219,11 +229,7 @@ func (s *Server) changeOwnPasswordHandler(w http.ResponseWriter, r *http.Request
219229
return
220230
}
221231

222-
hash, err := auth.HashPassword(req.NewPassword)
223-
if err != nil {
224-
WriteJSONError(w, "Internal server error", http.StatusInternalServerError)
225-
return
226-
}
232+
227233

228234
if err := s.dbQueries.UpdateUserPassword(r.Context(), database.UpdateUserPasswordParams{
229235
Username: currentUser.Username,
@@ -253,11 +259,19 @@ func (s *Server) changeUserPasswordHandler(w http.ResponseWriter, r *http.Reques
253259
return
254260
}
255261

256-
if err := s.passwordPolicy.Validate(req.NewPassword); err != nil {
257-
WriteJSONError(w, err.Error(), http.StatusBadRequest)
262+
hash, err := auth.HashPassword(req.NewPassword)
263+
if err != nil {
264+
var pe *auth.PasswordPolicyError
265+
if errors.As(err, &pe) {
266+
WriteJSONError(w, pe.Error(), http.StatusBadRequest)
267+
return
268+
}
269+
270+
WriteJSONError(w, "Internal server error", http.StatusInternalServerError)
258271
return
259272
}
260273

274+
261275
// Check if user exists
262276
user, err := s.dbQueries.GetUserByUsername(r.Context(), username)
263277
if err != nil {
@@ -269,11 +283,6 @@ func (s *Server) changeUserPasswordHandler(w http.ResponseWriter, r *http.Reques
269283
return
270284
}
271285

272-
hash, err := auth.HashPassword(req.NewPassword)
273-
if err != nil {
274-
WriteJSONError(w, "Internal server error", http.StatusInternalServerError)
275-
return
276-
}
277286

278287
if err := s.dbQueries.UpdateUserPassword(r.Context(), database.UpdateUserPasswordParams{
279288
Username: username,

0 commit comments

Comments
 (0)