From 59140a6d51ca3c08ccdadab822354b317b3b4fd7 Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Sun, 5 Sep 2021 14:10:12 +0200 Subject: [PATCH] add additional data to MFA secrets and fix pointers management --- dataprovider/admin.go | 6 +- dataprovider/dataprovider.go | 6 +- docs/full-configuration.md | 14 ++-- httpd/api_admin.go | 2 + httpd/api_mfa.go | 6 +- httpd/api_user.go | 2 + httpd/httpd_test.go | 138 +++++++++++++++++++++++++++++++++-- 7 files changed, 152 insertions(+), 22 deletions(-) diff --git a/dataprovider/admin.go b/dataprovider/admin.go index 4d9f4136..15e47560 100644 --- a/dataprovider/admin.go +++ b/dataprovider/admin.go @@ -53,7 +53,7 @@ type TOTPConfig struct { Secret *kms.Secret `json:"secret,omitempty"` } -func (c *TOTPConfig) validate() error { +func (c *TOTPConfig) validate(username string) error { if !c.Enabled { c.ConfigName = "" c.Secret = kms.NewEmptySecret() @@ -69,6 +69,7 @@ func (c *TOTPConfig) validate() error { return util.NewValidationError("totp: secret is mandatory") } if c.Secret.IsPlain() { + c.Secret.SetAdditionalData(username) if err := c.Secret.Encrypt(); err != nil { return util.NewValidationError(fmt.Sprintf("totp: unable to encrypt secret: %v", err)) } @@ -161,6 +162,7 @@ func (a *Admin) validateRecoveryCodes() error { return util.NewValidationError("mfa: recovery code cannot be empty") } if code.Secret.IsPlain() { + code.Secret.SetAdditionalData(a.Username) if err := code.Secret.Encrypt(); err != nil { return util.NewValidationError(fmt.Sprintf("mfa: unable to encrypt recovery code: %v", err)) } @@ -196,7 +198,7 @@ func (a *Admin) validate() error { if a.hasRedactedSecret() { return util.NewValidationError("cannot save an admin with a redacted secret") } - if err := a.Filters.TOTPConfig.validate(); err != nil { + if err := a.Filters.TOTPConfig.validate(a.Username); err != nil { return err } if err := a.validateRecoveryCodes(); err != nil { diff --git a/dataprovider/dataprovider.go b/dataprovider/dataprovider.go index 1e4f1cfb..d3e208a9 100644 --- a/dataprovider/dataprovider.go +++ b/dataprovider/dataprovider.go @@ -1339,7 +1339,7 @@ func validateUserVirtualFolders(user *User) error { return nil } -func validateUserTOTPConfig(c *sdk.TOTPConfig) error { +func validateUserTOTPConfig(c *sdk.TOTPConfig, username string) error { if !c.Enabled { c.ConfigName = "" c.Secret = kms.NewEmptySecret() @@ -1356,6 +1356,7 @@ func validateUserTOTPConfig(c *sdk.TOTPConfig) error { return util.NewValidationError("totp: secret is mandatory") } if c.Secret.IsPlain() { + c.Secret.SetAdditionalData(username) if err := c.Secret.Encrypt(); err != nil { return util.NewValidationError(fmt.Sprintf("totp: unable to encrypt secret: %v", err)) } @@ -1379,6 +1380,7 @@ func validateUserRecoveryCodes(user *User) error { return util.NewValidationError("mfa: recovery code cannot be empty") } if code.Secret.IsPlain() { + code.Secret.SetAdditionalData(user.Username) if err := code.Secret.Encrypt(); err != nil { return util.NewValidationError(fmt.Sprintf("mfa: unable to encrypt recovery code: %v", err)) } @@ -1675,7 +1677,7 @@ func ValidateUser(user *User) error { if user.hasRedactedSecret() { return util.NewValidationError("cannot save a user with a redacted secret") } - if err := validateUserTOTPConfig(&user.Filters.TOTPConfig); err != nil { + if err := validateUserTOTPConfig(&user.Filters.TOTPConfig, user.Username); err != nil { return err } if err := validateUserRecoveryCodes(user); err != nil { diff --git a/docs/full-configuration.md b/docs/full-configuration.md index 8e5b4c15..295c206c 100644 --- a/docs/full-configuration.md +++ b/docs/full-configuration.md @@ -306,20 +306,16 @@ The configuration can be read from JSON, TOML, YAML, HCL, envfile and Java prope ## Binding to privileged ports -On Linux, if you want to use Internet domain privileged ports (port numbers less than 1024) instead of running the SFTPGo service as root user you can set the `cap_net_bind_service` capability on the `sftpgo` binary. To set the capability you need to be root: +On Linux, if you want to use Internet domain privileged ports (port numbers less than 1024) instead of running the SFTPGo service as root user you can set the `cap_net_bind_service` capability on the `sftpgo` binary. To set the capability you can use the following command: ```shell -root@myhost # setcap cap_net_bind_service=+ep /usr/bin/sftpgo -``` - -Check that the capability is added: - -```shell -root@myhost # getcap /usr/bin/sftpgo +$ sudo setcap cap_net_bind_service=+ep /usr/bin/sftpgo +# Check that the capability is added: +$ getcap /usr/bin/sftpgo /usr/bin/sftpgo cap_net_bind_service=ep ``` -Now you can use privileged ports such as 21, 22, 443 etc.. without running the SFTPGo service as root user. +Now you can use privileged ports such as 21, 22, 443 etc.. without running the SFTPGo service as root user. You have to set the `cap_net_bind_service` capability each time you update the `sftpgo` binary. ## Environment variables diff --git a/httpd/api_admin.go b/httpd/api_admin.go index c2c8746e..1d6c451a 100644 --- a/httpd/api_admin.go +++ b/httpd/api_admin.go @@ -95,6 +95,8 @@ func updateAdmin(w http.ResponseWriter, r *http.Request) { adminID := admin.ID totpConfig := admin.Filters.TOTPConfig recoveryCodes := admin.Filters.RecoveryCodes + admin.Filters.TOTPConfig = dataprovider.TOTPConfig{} + admin.Filters.RecoveryCodes = nil err = render.DecodeJSON(r.Body, &admin) if err != nil { sendAPIResponse(w, r, err, "", http.StatusBadRequest) diff --git a/httpd/api_mfa.go b/httpd/api_mfa.go index 8a96a97b..61724f44 100644 --- a/httpd/api_mfa.go +++ b/httpd/api_mfa.go @@ -207,11 +207,12 @@ func saveUserTOTPConfig(username string, r *http.Request, recoveryCodes []sdk.Re return err } currentTOTPSecret := user.Filters.TOTPConfig.Secret + user.Filters.TOTPConfig.Secret = nil err = render.DecodeJSON(r.Body, &user.Filters.TOTPConfig) if err != nil { return util.NewValidationError(fmt.Sprintf("unable to decode JSON body: %v", err)) } - if user.Filters.TOTPConfig.Secret != nil && !user.Filters.TOTPConfig.Secret.IsPlain() { + if user.Filters.TOTPConfig.Secret == nil || !user.Filters.TOTPConfig.Secret.IsPlain() { user.Filters.TOTPConfig.Secret = currentTOTPSecret } if user.CountUnusedRecoveryCodes() < 5 && user.Filters.TOTPConfig.Enabled { @@ -226,6 +227,7 @@ func saveAdminTOTPConfig(username string, r *http.Request, recoveryCodes []sdk.R return err } currentTOTPSecret := admin.Filters.TOTPConfig.Secret + admin.Filters.TOTPConfig.Secret = nil err = render.DecodeJSON(r.Body, &admin.Filters.TOTPConfig) if err != nil { return util.NewValidationError(fmt.Sprintf("unable to decode JSON body: %v", err)) @@ -233,7 +235,7 @@ func saveAdminTOTPConfig(username string, r *http.Request, recoveryCodes []sdk.R if admin.CountUnusedRecoveryCodes() < 5 && admin.Filters.TOTPConfig.Enabled { admin.Filters.RecoveryCodes = recoveryCodes } - if admin.Filters.TOTPConfig.Secret != nil && !admin.Filters.TOTPConfig.Secret.IsPlain() { + if admin.Filters.TOTPConfig.Secret == nil || !admin.Filters.TOTPConfig.Secret.IsPlain() { admin.Filters.TOTPConfig.Secret = currentTOTPSecret } return dataprovider.UpdateAdmin(&admin) diff --git a/httpd/api_user.go b/httpd/api_user.go index 0c98a058..031bdc19 100644 --- a/httpd/api_user.go +++ b/httpd/api_user.go @@ -160,6 +160,8 @@ func updateUser(w http.ResponseWriter, r *http.Request) { user.FsConfig.GCSConfig = vfs.GCSFsConfig{} user.FsConfig.CryptConfig = vfs.CryptFsConfig{} user.FsConfig.SFTPConfig = vfs.SFTPFsConfig{} + user.Filters.TOTPConfig = sdk.TOTPConfig{} + user.Filters.RecoveryCodes = nil user.VirtualFolders = nil err = render.DecodeJSON(r.Body, &user) if err != nil { diff --git a/httpd/httpd_test.go b/httpd/httpd_test.go index af374253..2bbdf749 100644 --- a/httpd/httpd_test.go +++ b/httpd/httpd_test.go @@ -661,6 +661,83 @@ func TestHTTPUserAuthentication(t *testing.T) { assert.NoError(t, err) } +func TestPermMFADisabled(t *testing.T) { + u := getTestUser() + u.Filters.WebClient = []string{sdk.WebClientMFADisabled} + user, _, err := httpdtest.AddUser(u, http.StatusCreated) + assert.NoError(t, err) + + configName, _, secret, _, err := mfa.GenerateTOTPSecret(mfa.GetAvailableTOTPConfigNames()[0], user.Username) + assert.NoError(t, err) + token, err := getJWTAPIUserTokenFromTestServer(defaultUsername, defaultPassword) + assert.NoError(t, err) + userTOTPConfig := sdk.TOTPConfig{ + Enabled: true, + ConfigName: configName, + Secret: kms.NewPlainSecret(secret), + Protocols: []string{common.ProtocolSSH}, + } + asJSON, err := json.Marshal(userTOTPConfig) + assert.NoError(t, err) + req, err := http.NewRequest(http.MethodPost, userTOTPSavePath, bytes.NewBuffer(asJSON)) + assert.NoError(t, err) + setBearerForReq(req, token) + rr := executeRequest(req) + checkResponseCode(t, http.StatusForbidden, rr) // MFA is disabled for this user + + user.Filters.WebClient = []string{sdk.WebClientWriteDisabled} + user, _, err = httpdtest.UpdateUser(user, http.StatusOK, "") + assert.NoError(t, err) + + token, err = getJWTAPIUserTokenFromTestServer(defaultUsername, defaultPassword) + assert.NoError(t, err) + req, err = http.NewRequest(http.MethodPost, userTOTPSavePath, bytes.NewBuffer(asJSON)) + assert.NoError(t, err) + setBearerForReq(req, token) + rr = executeRequest(req) + checkResponseCode(t, http.StatusOK, rr) + // now we cannot disable MFA for this user + user.Filters.WebClient = []string{sdk.WebClientMFADisabled} + _, resp, err := httpdtest.UpdateUser(user, http.StatusBadRequest, "") + assert.NoError(t, err) + assert.Contains(t, string(resp), "multi-factor authentication cannot be disabled for a user with an active configuration") + + saveReq := make(map[string]bool) + saveReq["enabled"] = false + asJSON, err = json.Marshal(saveReq) + assert.NoError(t, err) + req, err = http.NewRequest(http.MethodPost, userTOTPSavePath, bytes.NewBuffer(asJSON)) + assert.NoError(t, err) + setBearerForReq(req, token) + rr = executeRequest(req) + checkResponseCode(t, http.StatusOK, rr) + + user.Filters.RecoveryCodes = []sdk.RecoveryCode{ + { + Secret: kms.NewPlainSecret(shortuuid.New()), + }, + } + user, resp, err = httpdtest.UpdateUser(user, http.StatusOK, "") + assert.NoError(t, err, string(resp)) + assert.Contains(t, user.Filters.WebClient, sdk.WebClientMFADisabled) + assert.Len(t, user.Filters.RecoveryCodes, 12) + + req, err = http.NewRequest(http.MethodGet, user2FARecoveryCodesPath, nil) + assert.NoError(t, err) + setBearerForReq(req, token) + rr = executeRequest(req) + checkResponseCode(t, http.StatusOK, rr) + var recCodes []recoveryCode + err = json.Unmarshal(rr.Body.Bytes(), &recCodes) + assert.NoError(t, err) + assert.Len(t, recCodes, 12) + + _, err = httpdtest.RemoveUser(user, http.StatusOK) + assert.NoError(t, err) + err = os.RemoveAll(user.GetHomeDir()) + assert.NoError(t, err) +} + func TestLoginUserAPITOTP(t *testing.T) { user, _, err := httpdtest.AddUser(getTestUser(), http.StatusCreated) assert.NoError(t, err) @@ -4811,11 +4888,19 @@ func TestAdminTOTP(t *testing.T) { assert.NotEmpty(t, admin.Filters.TOTPConfig.Secret.GetPayload()) assert.Equal(t, kms.SecretStatusSecretBox, admin.Filters.TOTPConfig.Secret.GetStatus()) admin.Filters.TOTPConfig = dataprovider.TOTPConfig{ - Enabled: false, + Enabled: false, + ConfigName: shortuuid.New(), + Secret: kms.NewEmptySecret(), } - admin, _, err = httpdtest.UpdateAdmin(admin, http.StatusOK) - assert.NoError(t, err) + admin.Filters.RecoveryCodes = []sdk.RecoveryCode{ + { + Secret: kms.NewEmptySecret(), + }, + } + admin, resp, err := httpdtest.UpdateAdmin(admin, http.StatusOK) + assert.NoError(t, err, string(resp)) assert.True(t, admin.Filters.TOTPConfig.Enabled) + assert.Len(t, admin.Filters.RecoveryCodes, 12) // if we use token we should get no recovery codes req, err = http.NewRequest(http.MethodGet, admin2FARecoveryCodesPath, nil) assert.NoError(t, err) @@ -5378,14 +5463,15 @@ func TestMFAErrors(t *testing.T) { setBearerForReq(req, userToken) rr = executeRequest(req) checkResponseCode(t, http.StatusBadRequest, rr) - assert.Contains(t, rr.Body.String(), "cannot save a user with a redacted secret") + // previous secret will be preserved and we have no secret saved + assert.Contains(t, rr.Body.String(), "totp: secret is mandatory") req, err = http.NewRequest(http.MethodPost, adminTOTPSavePath, bytes.NewBuffer(asJSON)) assert.NoError(t, err) setBearerForReq(req, adminToken) rr = executeRequest(req) checkResponseCode(t, http.StatusBadRequest, rr) - assert.Contains(t, rr.Body.String(), "cannot save an admin with a redacted secret") + assert.Contains(t, rr.Body.String(), "totp: secret is mandatory") _, err = httpdtest.RemoveUser(user, http.StatusOK) assert.NoError(t, err) @@ -5612,16 +5698,18 @@ func TestWebUserTOTP(t *testing.T) { assert.NoError(t, err) totpCfg := user.Filters.TOTPConfig assert.True(t, totpCfg.Enabled) + secretPayload := totpCfg.Secret.GetPayload() assert.Equal(t, totpGenResp.ConfigName, totpCfg.ConfigName) assert.Empty(t, totpCfg.Secret.GetKey()) assert.Empty(t, totpCfg.Secret.GetAdditionalData()) - assert.NotEmpty(t, totpCfg.Secret.GetPayload()) + assert.NotEmpty(t, secretPayload) assert.Equal(t, kms.SecretStatusSecretBox, totpCfg.Secret.GetStatus()) assert.Len(t, totpCfg.Protocols, 1) assert.Contains(t, totpCfg.Protocols, common.ProtocolSSH) // update protocols only userTOTPConfig = sdk.TOTPConfig{ Protocols: []string{common.ProtocolSSH, common.ProtocolFTP}, + Secret: kms.NewEmptySecret(), } asJSON, err = json.Marshal(userTOTPConfig) assert.NoError(t, err) @@ -5634,6 +5722,7 @@ func TestWebUserTOTP(t *testing.T) { // update the user, TOTP should not be affected user.Filters.TOTPConfig = sdk.TOTPConfig{ Enabled: false, + Secret: kms.NewEmptySecret(), } _, _, err = httpdtest.UpdateUser(user, http.StatusOK, "") assert.NoError(t, err) @@ -5644,7 +5733,7 @@ func TestWebUserTOTP(t *testing.T) { assert.Equal(t, totpCfg.ConfigName, user.Filters.TOTPConfig.ConfigName) assert.Empty(t, user.Filters.TOTPConfig.Secret.GetKey()) assert.Empty(t, user.Filters.TOTPConfig.Secret.GetAdditionalData()) - assert.Equal(t, totpCfg.Secret.GetPayload(), user.Filters.TOTPConfig.Secret.GetPayload()) + assert.Equal(t, secretPayload, user.Filters.TOTPConfig.Secret.GetPayload()) assert.Equal(t, kms.SecretStatusSecretBox, user.Filters.TOTPConfig.Secret.GetStatus()) assert.Len(t, user.Filters.TOTPConfig.Protocols, 2) assert.Contains(t, user.Filters.TOTPConfig.Protocols, common.ProtocolSSH) @@ -10033,6 +10122,41 @@ func TestWebAdminBasicMock(t *testing.T) { admin, _, err = httpdtest.GetAdminByUsername(altAdminUsername, http.StatusOK) assert.NoError(t, err) assert.True(t, admin.Filters.TOTPConfig.Enabled) + secretPayload := admin.Filters.TOTPConfig.Secret.GetPayload() + assert.NotEmpty(t, secretPayload) + + adminTOTPConfig = dataprovider.TOTPConfig{ + Enabled: true, + ConfigName: configName, + Secret: kms.NewEmptySecret(), + } + asJSON, err = json.Marshal(adminTOTPConfig) + assert.NoError(t, err) + req, err = http.NewRequest(http.MethodPost, webAdminTOTPSavePath, bytes.NewBuffer(asJSON)) + assert.NoError(t, err) + setJWTCookieForReq(req, altToken) + setCSRFHeaderForReq(req, csrfToken) + rr = executeRequest(req) + checkResponseCode(t, http.StatusOK, rr) + + admin, _, err = httpdtest.GetAdminByUsername(altAdminUsername, http.StatusOK) + assert.NoError(t, err) + assert.True(t, admin.Filters.TOTPConfig.Enabled) + assert.Equal(t, secretPayload, admin.Filters.TOTPConfig.Secret.GetPayload()) + + adminTOTPConfig = dataprovider.TOTPConfig{ + Enabled: true, + ConfigName: configName, + Secret: nil, + } + asJSON, err = json.Marshal(adminTOTPConfig) + assert.NoError(t, err) + req, err = http.NewRequest(http.MethodPost, webAdminTOTPSavePath, bytes.NewBuffer(asJSON)) + assert.NoError(t, err) + setJWTCookieForReq(req, altToken) + setCSRFHeaderForReq(req, csrfToken) + rr = executeRequest(req) + checkResponseCode(t, http.StatusOK, rr) req, _ = http.NewRequest(http.MethodGet, webAdminsPath+"?qlimit=a", nil) setJWTCookieForReq(req, token)