From 8d4964c16d00dca630549d45912863d029b51008 Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Thu, 1 Aug 2019 22:42:46 +0200 Subject: [PATCH] convert public key from newline delimited string to a real array Added a compatibility layer that will convert newline delimited keys to array when the user is fetched from the database. This code will be removed in future versions please update your public keys, you only need to resave the users using the REST API. --- README.md | 2 +- api/api_test.go | 12 ++++++------ api/internal_test.go | 4 ++-- api/schema/openapi.yaml | 22 ++++++++++++---------- api/user.go | 4 ++-- config/config.go | 1 + dataprovider/dataprovider.go | 13 +++++-------- dataprovider/sqlcommon.go | 35 +++++++++++++++++++++++++++++------ dataprovider/user.go | 9 +++++++-- sftpd/sftpd_test.go | 12 ++++++------ 10 files changed, 71 insertions(+), 43 deletions(-) diff --git a/README.md b/README.md index 7cde11ea..b2c19cc6 100644 --- a/README.md +++ b/README.md @@ -162,7 +162,7 @@ For each account the following properties can be configured: - `username` - `password` used for password authentication. For users created using SFTPGo REST API the password will be stored using argon2id hashing algo. SFTPGo supports checking passwords stored with bcrypt too. Currently, as fallback, there is a clear text password checking but you should not store passwords as clear text and this support could be removed at any time, so please don't depend on it. -- `public_key` used for public key authentication. At least one between password and public key is mandatory. Multiple public keys are supported, newline delimited (unix line break unescaped). +- `public_key` array of public keys. At least one public key or the password is mandatory. - `home_dir` The user cannot upload or download files outside this directory. Must be an absolute path - `uid`, `gid`. If sftpgo runs as root system user then the created files and directories will be assigned to this system uid/gid. Ignored on windows and if sftpgo runs as non root user: in this case files and directories for all SFTP users will be owned by the system user that runs sftpgo. - `max_sessions` maximum concurrent sessions. 0 means unlimited diff --git a/api/api_test.go b/api/api_test.go index e2b28a31..c048ad6d 100644 --- a/api/api_test.go +++ b/api/api_test.go @@ -126,7 +126,7 @@ func TestBasicUserHandling(t *testing.T) { func TestAddUserNoCredentials(t *testing.T) { u := getTestUser() u.Password = "" - u.PublicKey = "" + u.PublicKey = []string{} _, err := api.AddUser(u, http.StatusBadRequest) if err != nil { t.Errorf("unexpected error adding user with no credentials: %v", err) @@ -182,22 +182,22 @@ func TestUserPublicKey(t *testing.T) { u := getTestUser() invalidPubKey := "invalid" validPubKey := "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC03jj0D+djk7pxIf/0OhrxrchJTRZklofJ1NoIu4752Sq02mdXmarMVsqJ1cAjV5LBVy3D1F5U6XW4rppkXeVtd04Pxb09ehtH0pRRPaoHHlALiJt8CoMpbKYMA8b3KXPPriGxgGomvtU2T2RMURSwOZbMtpsugfjYSWenyYX+VORYhylWnSXL961LTyC21ehd6d6QnW9G7E5hYMITMY9TuQZz3bROYzXiTsgN0+g6Hn7exFQp50p45StUMfV/SftCMdCxlxuyGny2CrN/vfjO7xxOo2uv7q1qm10Q46KPWJQv+pgZ/OfL+EDjy07n5QVSKHlbx+2nT4Q0EgOSQaCTYwn3YjtABfIxWwgAFdyj6YlPulCL22qU4MYhDcA6PSBwDdf8hvxBfvsiHdM+JcSHvv8/VeJhk6CmnZxGY0fxBupov27z3yEO8nAg8k+6PaUiW1MSUfuGMF/ktB8LOstXsEPXSszuyXiOv4DaryOXUiSn7bmRqKcEFlJusO6aZP0= nicola@p1" - u.PublicKey = invalidPubKey + u.PublicKey = []string{invalidPubKey} _, err := api.AddUser(u, http.StatusBadRequest) if err != nil { t.Errorf("unexpected error adding user with invalid pub key: %v", err) } - u.PublicKey = validPubKey + u.PublicKey = []string{validPubKey} user, err := api.AddUser(u, http.StatusOK) if err != nil { t.Errorf("unable to add user: %v", err) } - user.PublicKey = validPubKey + "\n" + invalidPubKey + user.PublicKey = []string{validPubKey, invalidPubKey} _, err = api.UpdateUser(user, http.StatusBadRequest) if err != nil { t.Errorf("update user with invalid public key must fail: %v", err) } - user.PublicKey = validPubKey + "\n" + validPubKey + "\n" + validPubKey + user.PublicKey = []string{validPubKey, validPubKey, validPubKey} _, err = api.UpdateUser(user, http.StatusOK) if err != nil { t.Errorf("unable to update user: %v", err) @@ -238,7 +238,7 @@ func TestUpdateUserNoCredentials(t *testing.T) { t.Errorf("unable to add user: %v", err) } user.Password = "" - user.PublicKey = "" + user.PublicKey = []string{} // password and public key will be omitted from json serialization if empty and so they will remain unchanged // and no validation error will be raised _, err = api.UpdateUser(user, http.StatusOK) diff --git a/api/internal_test.go b/api/internal_test.go index 49fe639c..db7abf68 100644 --- a/api/internal_test.go +++ b/api/internal_test.go @@ -42,12 +42,12 @@ func TestCheckUser(t *testing.T) { t.Errorf("actual password must be nil") } actual.Password = "" - actual.PublicKey = "pub key" + actual.PublicKey = []string{"pub key"} err = checkUser(expected, actual) if err == nil { t.Errorf("actual public key must be nil") } - actual.PublicKey = "" + actual.PublicKey = []string{} err = checkUser(expected, actual) if err == nil { t.Errorf("actual ID must be > 0") diff --git a/api/schema/openapi.yaml b/api/schema/openapi.yaml index 863dfdc2..9dd3fc98 100644 --- a/api/schema/openapi.yaml +++ b/api/schema/openapi.yaml @@ -12,7 +12,7 @@ paths: tags: - connections summary: Get the active sftp users and info about their uploads/downloads - operationId: get_connections + operationId: get_sftp_connections responses: 200: description: successful operation @@ -27,7 +27,7 @@ paths: tags: - connections summary: Terminate an active SFTP connection - operationId: close_connection + operationId: close_sftp_connection parameters: - name: connectionID in: path @@ -81,7 +81,7 @@ paths: tags: - quota summary: Get the active quota scans - operationId: get_quota_scan + operationId: get_quota_scans responses: 200: description: successful operation @@ -297,7 +297,7 @@ paths: - users summary: Find user by ID description: For security reasons password and public key are empty in the response - operationId: getUserByID + operationId: get_user_by_id parameters: - name: userID in: path @@ -356,8 +356,8 @@ paths: put: tags: - users - summary: Update an user - operationId: updateUser + summary: Update an existing user + operationId: update_user parameters: - name: userID in: path @@ -426,8 +426,8 @@ paths: delete: tags: - users - summary: Delete an user - operationId: deleteUser + summary: Delete an existing user + operationId: delete_user parameters: - name: userID in: path @@ -524,9 +524,11 @@ components: nullable: true description: password or public key are mandatory. For security reasons this field is omitted when you search/get users public_key: - type: string + type: array + items: + type: string nullable: true - description: password or public key are mandatory. For security reasons this field is omitted when you search/get users. Multiple public keys are supported, newline delimited, when adding or modifying an user + description: a password or at least one public key are mandatory. For security reasons this field is omitted when you search/get users. home_dir: type: string description: path to the user home directory. The user cannot upload or download files outside this directory. SFTPGo tries to automatically create this folder if missing. Must be an absolute path diff --git a/api/user.go b/api/user.go index 5129bb4a..e2071513 100644 --- a/api/user.go +++ b/api/user.go @@ -65,7 +65,7 @@ func getUserByID(w http.ResponseWriter, r *http.Request) { user, err := dataprovider.GetUserByID(dataProvider, userID) if err == nil { user.Password = "" - user.PublicKey = "" + user.PublicKey = []string{} render.JSON(w, r, user) } else if err == sql.ErrNoRows { sendAPIResponse(w, r, err, "", http.StatusNotFound) @@ -86,7 +86,7 @@ func addUser(w http.ResponseWriter, r *http.Request) { user, err = dataprovider.UserExists(dataProvider, user.Username) if err == nil { user.Password = "" - user.PublicKey = "" + user.PublicKey = []string{} render.JSON(w, r, user) } else { sendAPIResponse(w, r, err, "", http.StatusInternalServerError) diff --git a/config/config.go b/config/config.go index ab059d5a..56a827d6 100644 --- a/config/config.go +++ b/config/config.go @@ -46,6 +46,7 @@ func init() { Command: "", HTTPNotificationURL: "", }, + Keys: []sftpd.Key{}, }, ProviderConf: dataprovider.Config{ Driver: "sqlite", diff --git a/dataprovider/dataprovider.go b/dataprovider/dataprovider.go index d0bfbcb6..9efe176e 100644 --- a/dataprovider/dataprovider.go +++ b/dataprovider/dataprovider.go @@ -226,22 +226,19 @@ func validateUser(user *User) error { return &ValidationError{err: fmt.Sprintf("Invalid permission: %v", p)} } } - if !strings.HasPrefix(user.Password, argonPwdPrefix) { + if len(user.Password) > 0 && !strings.HasPrefix(user.Password, argonPwdPrefix) { pwd, err := argon2id.CreateHash(user.Password, argon2id.DefaultParams) if err != nil { return err } user.Password = pwd } - if len(user.PublicKey) > 0 { - for i, k := range strings.Split(user.PublicKey, "\n") { - _, _, _, _, err := ssh.ParseAuthorizedKey([]byte(k)) - if err != nil { - return &ValidationError{err: fmt.Sprintf("Could not parse key nr. %d: %s", i, err)} - } + for i, k := range user.PublicKey { + _, _, _, _, err := ssh.ParseAuthorizedKey([]byte(k)) + if err != nil { + return &ValidationError{err: fmt.Sprintf("Could not parse key nr. %d: %s", i, err)} } } - return nil } diff --git a/dataprovider/sqlcommon.go b/dataprovider/sqlcommon.go index 98372afe..b77e4ef2 100644 --- a/dataprovider/sqlcommon.go +++ b/dataprovider/sqlcommon.go @@ -39,6 +39,11 @@ func sqlCommonValidateUserAndPass(username string, password string) (User, error if err != nil { logger.Warn(logSender, "error authenticating user: %v, error: %v", username, err) } else { + // even if the password is empty inside the database an empty user password + // will be refused anyway so it cannot match, additional check to be paranoid + if len(user.Password) == 0 { + return user, errors.New("Credentials cannot be null or empty") + } var match bool if strings.HasPrefix(user.Password, argonPwdPrefix) { match, err = argon2id.ComparePasswordAndHash(password, user.Password) @@ -77,7 +82,7 @@ func sqlCommonValidateUserAndPubKey(username string, pubKey string) (User, error return user, errors.New("Invalid credentials") } - for i, k := range strings.Split(user.PublicKey, "\n") { + for i, k := range user.PublicKey { storedPubKey, _, _, _, err := ssh.ParseAuthorizedKey([]byte(k)) if err != nil { logger.Warn(logSender, "error parsing stored public key %d for user %v: %v", i, username, err) @@ -170,7 +175,11 @@ func sqlCommonAddUser(user User) error { if err != nil { return err } - _, err = stmt.Exec(user.Username, user.Password, user.PublicKey, user.HomeDir, user.UID, user.GID, user.MaxSessions, user.QuotaSize, + publicKeys, err := user.GetPublicKeysAsJSON() + if err != nil { + return err + } + _, err = stmt.Exec(user.Username, user.Password, string(publicKeys), user.HomeDir, user.UID, user.GID, user.MaxSessions, user.QuotaSize, user.QuotaFiles, string(permissions), user.UploadBandwidth, user.DownloadBandwidth) return err } @@ -191,8 +200,12 @@ func sqlCommonUpdateUser(user User) error { if err != nil { return err } - _, err = stmt.Exec(user.Password, user.PublicKey, user.HomeDir, user.UID, user.GID, user.MaxSessions, user.QuotaSize, - user.QuotaFiles, permissions, user.UploadBandwidth, user.DownloadBandwidth, user.ID) + publicKeys, err := user.GetPublicKeysAsJSON() + if err != nil { + return err + } + _, err = stmt.Exec(user.Password, string(publicKeys), user.HomeDir, user.UID, user.GID, user.MaxSessions, user.QuotaSize, + user.QuotaFiles, string(permissions), user.UploadBandwidth, user.DownloadBandwidth, user.ID) return err } @@ -229,7 +242,7 @@ func sqlCommonGetUsers(limit int, offset int, order string, username string) ([] u, err := getUserFromDbRow(nil, rows) // hide password and public key u.Password = "" - u.PublicKey = "" + u.PublicKey = []string{} if err == nil { users = append(users, u) } else { @@ -264,7 +277,17 @@ func getUserFromDbRow(row *sql.Row, rows *sql.Rows) (User, error) { user.Password = password.String } if publicKey.Valid { - user.PublicKey = publicKey.String + var list []string + err = json.Unmarshal([]byte(publicKey.String), &list) + if err == nil { + user.PublicKey = list + } else { + // compatibility layer: initially we store public keys as string newline delimited + // we need to remove this code in future + user.PublicKey = strings.Split(publicKey.String, "\n") + logger.Warn(logSender, "public keys loaded using compatibility mode, this will not work in future versions! "+ + "Number of public keys loaded: %v, username: %v", len(user.PublicKey), user.Username) + } } if permissions.Valid { var list []string diff --git a/dataprovider/user.go b/dataprovider/user.go index 2a4570fc..a89c4193 100644 --- a/dataprovider/user.go +++ b/dataprovider/user.go @@ -39,8 +39,8 @@ type User struct { // Currently, as fallback, there is a clear text password checking but you should not store passwords // as clear text and this support could be removed at any time, so please don't depend on it. Password string `json:"password,omitempty"` - // PublicKey used for public key authentication. At least one between password and public key is mandatory - PublicKey string `json:"public_key,omitempty"` + // PublicKey used for public key authentication. At least one between password and a public key is mandatory + PublicKey []string `json:"public_key,omitempty"` // The user cannot upload or download files outside this directory. Must be an absolute path HomeDir string `json:"home_dir"` // If sftpgo runs as root system user then the created files and directories will be assigned to this system UID @@ -80,6 +80,11 @@ func (u *User) GetPermissionsAsJSON() ([]byte, error) { return json.Marshal(u.Permissions) } +// GetPublicKeysAsJSON returns the public keys as json byte array +func (u *User) GetPublicKeysAsJSON() ([]byte, error) { + return json.Marshal(u.PublicKey) +} + // GetUID returns a validate uid, suitable for use with os.Chown func (u *User) GetUID() int { if u.UID <= 0 || u.UID > 65535 { diff --git a/sftpd/sftpd_test.go b/sftpd/sftpd_test.go index 6892399e..7dfb74f3 100644 --- a/sftpd/sftpd_test.go +++ b/sftpd/sftpd_test.go @@ -465,7 +465,7 @@ func TestHomeSpecialChars(t *testing.T) { func TestLogin(t *testing.T) { u := getTestUser(false) - u.PublicKey = testPubKey + u.PublicKey = []string{testPubKey} user, err := api.AddUser(u, http.StatusOK) if err != nil { t.Errorf("unable to add user: %v", err) @@ -497,7 +497,7 @@ func TestLogin(t *testing.T) { defer client.Close() } // testPubKey1 is not authorized - user.PublicKey = testPubKey1 + user.PublicKey = []string{testPubKey1} user.Password = "" _, err = api.UpdateUser(user, http.StatusOK) if err != nil { @@ -509,7 +509,7 @@ func TestLogin(t *testing.T) { defer client.Close() } // login a user with multiple public keys, only the second one is valid - user.PublicKey = testPubKey1 + "\n" + testPubKey + user.PublicKey = []string{testPubKey1, testPubKey} user.Password = "" _, err = api.UpdateUser(user, http.StatusOK) if err != nil { @@ -538,7 +538,7 @@ func TestLoginAfterUserUpdateEmptyPwd(t *testing.T) { t.Errorf("unable to add user: %v", err) } user.Password = "" - user.PublicKey = "" + user.PublicKey = []string{} // password and public key should remain unchanged _, err = api.UpdateUser(user, http.StatusOK) if err != nil { @@ -571,7 +571,7 @@ func TestLoginAfterUserUpdateEmptyPubKey(t *testing.T) { t.Errorf("unable to add user: %v", err) } user.Password = "" - user.PublicKey = "" + user.PublicKey = []string{} // password and public key should remain unchanged _, err = api.UpdateUser(user, http.StatusOK) if err != nil { @@ -1178,7 +1178,7 @@ func getTestUser(usePubKey bool) dataprovider.User { Permissions: allPerms, } if usePubKey { - user.PublicKey = testPubKey + user.PublicKey = []string{testPubKey} user.Password = "" } return user