From 7d9f687768e0ad643fca36e7d80308d560bda7a1 Mon Sep 17 00:00:00 2001 From: Philipp Heckel Date: Tue, 25 Jan 2022 21:57:28 -0500 Subject: [PATCH] Tests --- auth/auth.go | 8 +- auth/auth_sqlite.go | 77 ++++++------- auth/auth_sqlite_test.go | 238 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 278 insertions(+), 45 deletions(-) create mode 100644 auth/auth_sqlite_test.go diff --git a/auth/auth.go b/auth/auth.go index d41843a..16a4167 100644 --- a/auth/auth.go +++ b/auth/auth.go @@ -22,7 +22,7 @@ type Manager interface { type User struct { Name string - Pass string // hashed + Hash string // password hash (bcrypt) Role Role Grants []Grant } @@ -57,6 +57,8 @@ func AllowedRole(role Role) bool { } var ( - ErrUnauthorized = errors.New("unauthorized") - ErrNotFound = errors.New("not found") + ErrUnauthenticated = errors.New("unauthenticated") + ErrUnauthorized = errors.New("unauthorized") + ErrInvalidArgument = errors.New("invalid argument") + ErrNotFound = errors.New("not found") ) diff --git a/auth/auth_sqlite.go b/auth/auth_sqlite.go index 7c9f5cd..f9794b9 100644 --- a/auth/auth_sqlite.go +++ b/auth/auth_sqlite.go @@ -2,24 +2,14 @@ package auth import ( "database/sql" + _ "github.com/mattn/go-sqlite3" // SQLite driver "golang.org/x/crypto/bcrypt" + "regexp" ) -/* - -SELECT * FROM user; -SELECT * FROM access; - -INSERT INTO user VALUES ('phil','$2a$06$.4W0LI5mcxzxhpjUvpTaNeu0MhRO0T7B.CYnmAkRnlztIy7PrSODu', 'admin'); -INSERT INTO user VALUES ('ben','$2a$06$skJK/AecWCUmiCjr69ke.Ow/hFA616RdvJJPxnI221zyohsRlyXL.', 'user'); -INSERT INTO user VALUES ('marian','$2a$10$8U90swQIatvHHI4sw0Wo7.OUy6dUwzMcoOABi6BsS4uF0x3zcSXRW', 'user'); - -INSERT INTO access VALUES ('ben','alerts',1,1); -INSERT INTO access VALUES ('marian','alerts',1,0); -INSERT INTO access VALUES ('','announcements',1,0); -INSERT INTO access VALUES ('','write-all',1,1); - -*/ +const ( + bcryptCost = 11 +) // Auther-related queries const ( @@ -77,6 +67,10 @@ type SQLiteAuth struct { defaultWrite bool } +var ( + allowedUsernameRegex = regexp.MustCompile(`^[-_.@a-zA-Z0-9]+$`) +) + var _ Auther = (*SQLiteAuth)(nil) var _ Manager = (*SQLiteAuth)(nil) @@ -105,28 +99,18 @@ func setupNewAuthDB(db *sql.DB) error { func (a *SQLiteAuth) Authenticate(username, password string) (*User, error) { if username == Everyone { - return nil, ErrUnauthorized + return nil, ErrUnauthenticated } - rows, err := a.db.Query(selectUserQuery, username) + user, err := a.User(username) if err != nil { - return nil, err + bcrypt.CompareHashAndPassword([]byte("$2a$11$eX15DeF27FwAgXt9wqJF0uAUMz74XywJcGBH3kP93pzKYv6ATk2ka"), + []byte("intentional slow-down to avoid timing attacks")) + return nil, ErrUnauthenticated } - defer rows.Close() - var hash, role string - if rows.Next() { - if err := rows.Scan(&hash, &role); err != nil { - return nil, err - } else if err := rows.Err(); err != nil { - return nil, err - } + if err := bcrypt.CompareHashAndPassword([]byte(user.Hash), []byte(password)); err != nil { + return nil, ErrUnauthenticated } - if err := bcrypt.CompareHashAndPassword([]byte(hash), []byte(password)); err != nil { - return nil, err - } - return &User{ - Name: username, - Role: Role(role), - }, nil + return user, nil } func (a *SQLiteAuth) Authorize(user *User, topic string, perm Permission) error { @@ -167,7 +151,10 @@ func (a *SQLiteAuth) resolvePerms(read, write bool, perm Permission) error { } func (a *SQLiteAuth) AddUser(username, password string, role Role) error { - hash, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost) + if !allowedUsernameRegex.MatchString(username) || (role != RoleAdmin && role != RoleUser) { + return ErrInvalidArgument + } + hash, err := bcrypt.GenerateFromPassword([]byte(password), bcryptCost) if err != nil { return err } @@ -178,6 +165,9 @@ func (a *SQLiteAuth) AddUser(username, password string, role Role) error { } func (a *SQLiteAuth) RemoveUser(username string) error { + if !allowedUsernameRegex.MatchString(username) || username == Everyone { + return ErrInvalidArgument + } if _, err := a.db.Exec(deleteUserQuery, username); err != nil { return err } @@ -224,18 +214,18 @@ func (a *SQLiteAuth) User(username string) (*User, error) { if username == Everyone { return a.everyoneUser() } - urows, err := a.db.Query(selectUserQuery, username) + rows, err := a.db.Query(selectUserQuery, username) if err != nil { return nil, err } - defer urows.Close() + defer rows.Close() var hash, role string - if !urows.Next() { + if !rows.Next() { return nil, ErrNotFound } - if err := urows.Scan(&hash, &role); err != nil { + if err := rows.Scan(&hash, &role); err != nil { return nil, err - } else if err := urows.Err(); err != nil { + } else if err := rows.Err(); err != nil { return nil, err } grants, err := a.readGrants(username) @@ -244,7 +234,7 @@ func (a *SQLiteAuth) User(username string) (*User, error) { } return &User{ Name: username, - Pass: hash, + Hash: hash, Role: Role(role), Grants: grants, }, nil @@ -257,7 +247,7 @@ func (a *SQLiteAuth) everyoneUser() (*User, error) { } return &User{ Name: Everyone, - Pass: "", + Hash: "", Role: RoleAnonymous, Grants: grants, }, nil @@ -288,7 +278,7 @@ func (a *SQLiteAuth) readGrants(username string) ([]Grant, error) { } func (a *SQLiteAuth) ChangePassword(username, password string) error { - hash, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost) + hash, err := bcrypt.GenerateFromPassword([]byte(password), bcryptCost) if err != nil { return err } @@ -299,6 +289,9 @@ func (a *SQLiteAuth) ChangePassword(username, password string) error { } func (a *SQLiteAuth) ChangeRole(username string, role Role) error { + if !allowedUsernameRegex.MatchString(username) || (role != RoleAdmin && role != RoleUser) { + return ErrInvalidArgument + } if _, err := a.db.Exec(updateUserRoleQuery, string(role), username); err != nil { return err } diff --git a/auth/auth_sqlite_test.go b/auth/auth_sqlite_test.go new file mode 100644 index 0000000..761b41d --- /dev/null +++ b/auth/auth_sqlite_test.go @@ -0,0 +1,238 @@ +package auth_test + +import ( + "github.com/stretchr/testify/require" + "heckel.io/ntfy/auth" + "path/filepath" + "strings" + "testing" + "time" +) + +func TestSQLiteAuth_FullScenario_Default_DenyAll(t *testing.T) { + a := newTestAuth(t, false, false) + require.Nil(t, a.AddUser("phil", "phil", auth.RoleAdmin)) + require.Nil(t, a.AddUser("ben", "ben", auth.RoleUser)) + require.Nil(t, a.AllowAccess("ben", "mytopic", true, true)) + require.Nil(t, a.AllowAccess("ben", "readme", true, false)) + require.Nil(t, a.AllowAccess("ben", "writeme", false, true)) + require.Nil(t, a.AllowAccess("ben", "everyonewrite", false, false)) // How unfair! + require.Nil(t, a.AllowAccess(auth.Everyone, "announcements", true, false)) + require.Nil(t, a.AllowAccess(auth.Everyone, "everyonewrite", true, true)) + + phil, err := a.Authenticate("phil", "phil") + require.Nil(t, err) + require.Equal(t, "phil", phil.Name) + require.True(t, strings.HasPrefix(phil.Hash, "$2a$11$")) + require.Equal(t, auth.RoleAdmin, phil.Role) + require.Equal(t, []auth.Grant{}, phil.Grants) + + ben, err := a.Authenticate("ben", "ben") + require.Nil(t, err) + require.Equal(t, "ben", ben.Name) + require.True(t, strings.HasPrefix(ben.Hash, "$2a$11$")) + require.Equal(t, auth.RoleUser, ben.Role) + require.Equal(t, []auth.Grant{ + {"mytopic", true, true}, + {"readme", true, false}, + {"writeme", false, true}, + {"everyonewrite", false, false}, + }, ben.Grants) + + notben, err := a.Authenticate("ben", "this is wrong") + require.Nil(t, notben) + require.Equal(t, auth.ErrUnauthenticated, err) + + // Admin can do everything + require.Nil(t, a.Authorize(phil, "sometopic", auth.PermissionWrite)) + require.Nil(t, a.Authorize(phil, "mytopic", auth.PermissionRead)) + require.Nil(t, a.Authorize(phil, "readme", auth.PermissionWrite)) + require.Nil(t, a.Authorize(phil, "writeme", auth.PermissionWrite)) + require.Nil(t, a.Authorize(phil, "announcements", auth.PermissionWrite)) + require.Nil(t, a.Authorize(phil, "everyonewrite", auth.PermissionWrite)) + + // User cannot do everything + require.Nil(t, a.Authorize(ben, "mytopic", auth.PermissionWrite)) + require.Nil(t, a.Authorize(ben, "mytopic", auth.PermissionRead)) + require.Nil(t, a.Authorize(ben, "readme", auth.PermissionRead)) + require.Equal(t, auth.ErrUnauthorized, a.Authorize(ben, "readme", auth.PermissionWrite)) + require.Equal(t, auth.ErrUnauthorized, a.Authorize(ben, "writeme", auth.PermissionRead)) + require.Nil(t, a.Authorize(ben, "writeme", auth.PermissionWrite)) + require.Nil(t, a.Authorize(ben, "writeme", auth.PermissionWrite)) + require.Equal(t, auth.ErrUnauthorized, a.Authorize(ben, "everyonewrite", auth.PermissionRead)) + require.Equal(t, auth.ErrUnauthorized, a.Authorize(ben, "everyonewrite", auth.PermissionWrite)) + require.Nil(t, a.Authorize(ben, "announcements", auth.PermissionRead)) + require.Equal(t, auth.ErrUnauthorized, a.Authorize(ben, "announcements", auth.PermissionWrite)) + + // Everyone else can do barely anything + require.Equal(t, auth.ErrUnauthorized, a.Authorize(nil, "sometopicnotinthelist", auth.PermissionRead)) + require.Equal(t, auth.ErrUnauthorized, a.Authorize(nil, "sometopicnotinthelist", auth.PermissionWrite)) + require.Equal(t, auth.ErrUnauthorized, a.Authorize(nil, "mytopic", auth.PermissionRead)) + require.Equal(t, auth.ErrUnauthorized, a.Authorize(nil, "mytopic", auth.PermissionWrite)) + require.Equal(t, auth.ErrUnauthorized, a.Authorize(nil, "readme", auth.PermissionRead)) + require.Equal(t, auth.ErrUnauthorized, a.Authorize(nil, "readme", auth.PermissionWrite)) + require.Equal(t, auth.ErrUnauthorized, a.Authorize(nil, "writeme", auth.PermissionRead)) + require.Equal(t, auth.ErrUnauthorized, a.Authorize(nil, "writeme", auth.PermissionWrite)) + require.Equal(t, auth.ErrUnauthorized, a.Authorize(nil, "announcements", auth.PermissionWrite)) + require.Nil(t, a.Authorize(nil, "announcements", auth.PermissionRead)) + require.Nil(t, a.Authorize(nil, "everyonewrite", auth.PermissionRead)) + require.Nil(t, a.Authorize(nil, "everyonewrite", auth.PermissionWrite)) +} + +func TestSQLiteAuth_AddUser_Invalid(t *testing.T) { + a := newTestAuth(t, false, false) + require.Equal(t, auth.ErrInvalidArgument, a.AddUser(" invalid ", "pass", auth.RoleAdmin)) + require.Equal(t, auth.ErrInvalidArgument, a.AddUser("validuser", "pass", "invalid-role")) +} + +func TestSQLiteAuth_AddUser_Timing(t *testing.T) { + a := newTestAuth(t, false, false) + start := time.Now().UnixMilli() + require.Nil(t, a.AddUser("user", "pass", auth.RoleAdmin)) + require.GreaterOrEqual(t, time.Now().UnixMilli()-start, int64(100)) // Ideally should be > 200ms, but let's not make a brittle +} + +func TestSQLiteAuth_Authenticate_Timing(t *testing.T) { + a := newTestAuth(t, false, false) + require.Nil(t, a.AddUser("user", "pass", auth.RoleAdmin)) + + // Timing a correct attempt + start := time.Now().UnixMilli() + _, err := a.Authenticate("user", "pass") + require.Nil(t, err) + require.GreaterOrEqual(t, time.Now().UnixMilli()-start, int64(100)) // Ideally should be > 200ms, but let's not make a brittle + + // Timing an incorrect attempt + start = time.Now().UnixMilli() + _, err = a.Authenticate("user", "INCORRECT") + require.Equal(t, auth.ErrUnauthenticated, err) + require.GreaterOrEqual(t, time.Now().UnixMilli()-start, int64(100)) // Ideally should be > 200ms, but let's not make a brittle + + // Timing a non-existing user attempt + start = time.Now().UnixMilli() + _, err = a.Authenticate("DOES-NOT-EXIST", "hithere") + require.Equal(t, auth.ErrUnauthenticated, err) + require.GreaterOrEqual(t, time.Now().UnixMilli()-start, int64(100)) // Ideally should be > 200ms, but let's not make a brittle +} + +func TestSQLiteAuth_UserManagement(t *testing.T) { + a := newTestAuth(t, false, false) + require.Nil(t, a.AddUser("phil", "phil", auth.RoleAdmin)) + require.Nil(t, a.AddUser("ben", "ben", auth.RoleUser)) + require.Nil(t, a.AllowAccess("ben", "mytopic", true, true)) + require.Nil(t, a.AllowAccess("ben", "readme", true, false)) + require.Nil(t, a.AllowAccess("ben", "writeme", false, true)) + require.Nil(t, a.AllowAccess("ben", "everyonewrite", false, false)) // How unfair! + require.Nil(t, a.AllowAccess(auth.Everyone, "announcements", true, false)) + require.Nil(t, a.AllowAccess(auth.Everyone, "everyonewrite", true, true)) + + // Query user details + phil, err := a.User("phil") + require.Nil(t, err) + require.Equal(t, "phil", phil.Name) + require.True(t, strings.HasPrefix(phil.Hash, "$2a$11$")) + require.Equal(t, auth.RoleAdmin, phil.Role) + require.Equal(t, []auth.Grant{}, phil.Grants) + + ben, err := a.User("ben") + require.Nil(t, err) + require.Equal(t, "ben", ben.Name) + require.True(t, strings.HasPrefix(ben.Hash, "$2a$11$")) + require.Equal(t, auth.RoleUser, ben.Role) + require.Equal(t, []auth.Grant{ + {"mytopic", true, true}, + {"readme", true, false}, + {"writeme", false, true}, + {"everyonewrite", false, false}, + }, ben.Grants) + + everyone, err := a.User(auth.Everyone) + require.Nil(t, err) + require.Equal(t, "*", everyone.Name) + require.Equal(t, "", everyone.Hash) + require.Equal(t, auth.RoleAnonymous, everyone.Role) + require.Equal(t, []auth.Grant{ + {"announcements", true, false}, + {"everyonewrite", true, true}, + }, everyone.Grants) + + // Ben: Before revoking + require.Nil(t, a.AllowAccess("ben", "mytopic", true, true)) + require.Nil(t, a.AllowAccess("ben", "readme", true, false)) + require.Nil(t, a.AllowAccess("ben", "writeme", false, true)) + require.Nil(t, a.Authorize(ben, "mytopic", auth.PermissionRead)) + require.Nil(t, a.Authorize(ben, "mytopic", auth.PermissionWrite)) + require.Nil(t, a.Authorize(ben, "readme", auth.PermissionRead)) + require.Nil(t, a.Authorize(ben, "writeme", auth.PermissionWrite)) + + // Revoke access for "ben" to "mytopic", then check again + require.Nil(t, a.ResetAccess("ben", "mytopic")) + require.Equal(t, auth.ErrUnauthorized, a.Authorize(ben, "mytopic", auth.PermissionWrite)) // Revoked + require.Equal(t, auth.ErrUnauthorized, a.Authorize(ben, "mytopic", auth.PermissionRead)) // Revoked + require.Nil(t, a.Authorize(ben, "readme", auth.PermissionRead)) // Unchanged + require.Nil(t, a.Authorize(ben, "writeme", auth.PermissionWrite)) // Unchanged + + // Revoke rest of the access + require.Nil(t, a.ResetAccess("ben", "")) + require.Equal(t, auth.ErrUnauthorized, a.Authorize(ben, "readme", auth.PermissionRead)) // Revoked + require.Equal(t, auth.ErrUnauthorized, a.Authorize(ben, "wrtiteme", auth.PermissionWrite)) // Revoked + + // User list + users, err := a.Users() + require.Nil(t, err) + require.Equal(t, 3, len(users)) + require.Equal(t, "phil", users[0].Name) + require.Equal(t, "ben", users[1].Name) + require.Equal(t, "*", users[2].Name) + + // Remove user + require.Nil(t, a.RemoveUser("ben")) + _, err = a.User("ben") + require.Equal(t, auth.ErrNotFound, err) + + users, err = a.Users() + require.Nil(t, err) + require.Equal(t, 2, len(users)) + require.Equal(t, "phil", users[0].Name) + require.Equal(t, "*", users[1].Name) +} + +func TestSQLiteAuth_ChangePassword(t *testing.T) { + a := newTestAuth(t, false, false) + require.Nil(t, a.AddUser("phil", "phil", auth.RoleAdmin)) + + _, err := a.Authenticate("phil", "phil") + require.Nil(t, err) + + require.Nil(t, a.ChangePassword("phil", "newpass")) + _, err = a.Authenticate("phil", "phil") + require.Equal(t, auth.ErrUnauthenticated, err) + _, err = a.Authenticate("phil", "newpass") + require.Nil(t, err) +} + +func TestSQLiteAuth_ChangeRole(t *testing.T) { + a := newTestAuth(t, false, false) + require.Nil(t, a.AddUser("ben", "ben", auth.RoleUser)) + require.Nil(t, a.AllowAccess("ben", "mytopic", true, true)) + require.Nil(t, a.AllowAccess("ben", "readme", true, false)) + + ben, err := a.User("ben") + require.Nil(t, err) + require.Equal(t, auth.RoleUser, ben.Role) + require.Equal(t, 2, len(ben.Grants)) + + require.Nil(t, a.ChangeRole("ben", auth.RoleAdmin)) + + ben, err = a.User("ben") + require.Nil(t, err) + require.Equal(t, auth.RoleAdmin, ben.Role) + require.Equal(t, 0, len(ben.Grants)) +} + +func newTestAuth(t *testing.T, defaultRead, defaultWrite bool) *auth.SQLiteAuth { + filename := filepath.Join(t.TempDir(), "user.db") + a, err := auth.NewSQLiteAuth(filename, defaultRead, defaultWrite) + require.Nil(t, err) + return a +}