From c19b03a3f7307deb166c34ed0a0ce162038b8a5b Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Sat, 19 Feb 2022 13:31:58 +0100 Subject: [PATCH] shares: add permission to deny sharing without password Signed-off-by: Nicola Murino --- dataprovider/share.go | 9 ++- docs/custom-actions.md | 4 +- go.mod | 2 +- go.sum | 4 +- httpd/api_shares.go | 19 +++++- httpd/httpd_test.go | 137 +++++++++++++++++++++++++++++++++++++++-- httpd/webclient.go | 12 ++++ openapi/openapi.yaml | 8 ++- 8 files changed, 177 insertions(+), 18 deletions(-) diff --git a/dataprovider/share.go b/dataprovider/share.go index dc1b7bbc..b2bb9d48 100644 --- a/dataprovider/share.go +++ b/dataprovider/share.go @@ -255,12 +255,15 @@ func (s *Share) validate() error { return nil } -// CheckPassword verifies the share password if set -func (s *Share) CheckPassword(password string) (bool, error) { +// CheckCredentials verifies the share credentials if a password if set +func (s *Share) CheckCredentials(username, password string) (bool, error) { if s.Password == "" { return true, nil } - if password == "" { + if username == "" || password == "" { + return false, ErrInvalidCredentials + } + if username != s.Username { return false, ErrInvalidCredentials } if strings.HasPrefix(s.Password, bcryptPwdPrefix) { diff --git a/docs/custom-actions.md b/docs/custom-actions.md index 99fe623f..d36cf90c 100644 --- a/docs/custom-actions.md +++ b/docs/custom-actions.md @@ -43,7 +43,7 @@ If the `hook` defines a path to an external program, then this program can read - `SFTPGO_ACTION_BUCKET`, non-empty for S3, GCS and Azure backends - `SFTPGO_ACTION_ENDPOINT`, non-empty for S3, SFTP and Azure backend if configured - `SFTPGO_ACTION_STATUS`, integer. Status for `upload`, `download` and `ssh_cmd` actions. 1 means no error, 2 means a generic error occurred, 3 means quota exceeded error -- `SFTPGO_ACTION_PROTOCOL`, string. Possible values are `SSH`, `SFTP`, `SCP`, `FTP`, `DAV`, `HTTP`, `HTTPShare`, `DataRetention` +- `SFTPGO_ACTION_PROTOCOL`, string. Possible values are `SSH`, `SFTP`, `SCP`, `FTP`, `DAV`, `HTTP`, `HTTPShare`, `OIDC`, `DataRetention` - `SFTPGO_ACTION_IP`, the action was executed from this IP address - `SFTPGO_ACTION_SESSION_ID`, string. Unique protocol session identifier. For stateless protocols such as HTTP the session id will change for each request - `SFTPGO_ACTION_OPEN_FLAGS`, integer. File open flags, can be non-zero for `pre-upload` action. If `SFTPGO_ACTION_FILE_SIZE` is greater than zero and `SFTPGO_ACTION_OPEN_FLAGS&512 == 0` the target file will not be truncated @@ -66,7 +66,7 @@ If the `hook` defines an HTTP URL then this URL will be invoked as HTTP POST. Th - `bucket`, string, inlcuded for S3, GCS and Azure backends - `endpoint`, string, included for S3, SFTP and Azure backend if configured - `status`, integer. Status for `upload`, `download` and `ssh_cmd` actions. 1 means no error, 2 means a generic error occurred, 3 means quota exceeded error -- `protocol`, string. Possible values are `SSH`, `SFTP`, `SCP`, `FTP`, `DAV`, `HTTP`, `HTTPShare`, `DataRetention` +- `protocol`, string. Possible values are `SSH`, `SFTP`, `SCP`, `FTP`, `DAV`, `HTTP`, `HTTPShare`, `OIDC`, `DataRetention` - `ip`, string. The action was executed from this IP address - `session_id`, string. Unique protocol session identifier. For stateless protocols such as HTTP the session id will change for each request - `open_flags`, integer. File open flags, can be non-zero for `pre-upload` action. If `file_size` is greater than zero and `file_size&512 == 0` the target file will not be truncated diff --git a/go.mod b/go.mod index 48f5dcb5..c51b2e40 100644 --- a/go.mod +++ b/go.mod @@ -40,7 +40,7 @@ require ( github.com/rs/cors v1.8.2 github.com/rs/xid v1.3.0 github.com/rs/zerolog v1.26.2-0.20220203140311-fc26014bd4e1 - github.com/sftpgo/sdk v0.0.0-20220201111021-563c373f8012 + github.com/sftpgo/sdk v0.1.1-0.20220219112139-4616f3c10321 github.com/shirou/gopsutil/v3 v3.22.1 github.com/spf13/afero v1.8.1 github.com/spf13/cobra v1.3.0 diff --git a/go.sum b/go.sum index 7fa660eb..07e75746 100644 --- a/go.sum +++ b/go.sum @@ -695,8 +695,8 @@ github.com/satori/go.uuid v1.2.0/go.mod h1:dA0hQrYB0VpLJoorglMZABFdXlWrHn1NEOzdh github.com/sean-/seed v0.0.0-20170313163322-e2103e2c3529/go.mod h1:DxrIzT+xaE7yg65j358z/aeFdxmN0P9QXhEzd20vsDc= github.com/secsy/goftp v0.0.0-20200609142545-aa2de14babf4 h1:PT+ElG/UUFMfqy5HrxJxNzj3QBOf7dZwupeVC+mG1Lo= github.com/secsy/goftp v0.0.0-20200609142545-aa2de14babf4/go.mod h1:MnkX001NG75g3p8bhFycnyIjeQoOjGL6CEIsdE/nKSY= -github.com/sftpgo/sdk v0.0.0-20220201111021-563c373f8012 h1:tkzS0kxhatqIVrWZePzsFlp1xQgR9q6Wt0UYKsBiCUU= -github.com/sftpgo/sdk v0.0.0-20220201111021-563c373f8012/go.mod h1:gcYbk4z578GfwbC9kJOz2rltYoPYUIcGZgV13r74MJw= +github.com/sftpgo/sdk v0.1.1-0.20220219112139-4616f3c10321 h1:woOiGu0/qrh2nzCQLlX7k3VK2s+kB+wIGaS/jh40/9o= +github.com/sftpgo/sdk v0.1.1-0.20220219112139-4616f3c10321/go.mod h1:zqCRMcwS28IViwekJHNkFu4GqSfyVmOQTlh8h3icAXE= github.com/shirou/gopsutil/v3 v3.22.1 h1:33y31Q8J32+KstqPfscvFwBlNJ6xLaBy4xqBXzlYV5w= github.com/shirou/gopsutil/v3 v3.22.1/go.mod h1:WapW1AOOPlHyXr+yOyw3uYx36enocrtSoSBy0L5vUHY= github.com/shopspring/decimal v0.0.0-20180709203117-cd690d0c9e24/go.mod h1:M+9NzErvs504Cn4c5DxATwIqPbtswREoFCre64PpcG4= diff --git a/httpd/api_shares.go b/httpd/api_shares.go index 7d17b1b6..c13249d9 100644 --- a/httpd/api_shares.go +++ b/httpd/api_shares.go @@ -11,6 +11,7 @@ import ( "github.com/go-chi/render" "github.com/rs/xid" + "github.com/sftpgo/sdk" "github.com/drakkan/sftpgo/v2/common" "github.com/drakkan/sftpgo/v2/dataprovider" @@ -76,6 +77,13 @@ func addShare(w http.ResponseWriter, r *http.Request) { if share.Name == "" { share.Name = share.ShareID } + if share.Password == "" { + if util.IsStringInSlice(sdk.WebClientShareNoPasswordDisabled, claims.Permissions) { + sendAPIResponse(w, r, nil, "You are not authorized to share files/folders without a password", + http.StatusForbidden) + return + } + } err = dataprovider.AddShare(&share, claims.Username, util.GetIPFromRemoteAddress(r.RemoteAddr)) if err != nil { sendAPIResponse(w, r, err, "", getRespStatus(err)) @@ -112,6 +120,13 @@ func updateShare(w http.ResponseWriter, r *http.Request) { if share.Password == redactedSecret { share.Password = oldPassword } + if share.Password == "" { + if util.IsStringInSlice(sdk.WebClientShareNoPasswordDisabled, claims.Permissions) { + sendAPIResponse(w, r, nil, "You are not authorized to share files/folders without a password", + http.StatusForbidden) + return + } + } if err := dataprovider.UpdateShare(&share, claims.Username, util.GetIPFromRemoteAddress(r.RemoteAddr)); err != nil { sendAPIResponse(w, r, err, "", getRespStatus(err)) return @@ -363,13 +378,13 @@ func checkPublicShare(w http.ResponseWriter, r *http.Request, shareShope datapro return share, nil, err } if share.Password != "" { - _, password, ok := r.BasicAuth() + username, password, ok := r.BasicAuth() if !ok { w.Header().Set(common.HTTPAuthenticationHeader, basicRealm) renderError(dataprovider.ErrInvalidCredentials, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized) return share, nil, dataprovider.ErrInvalidCredentials } - match, err := share.CheckPassword(password) + match, err := share.CheckCredentials(username, password) if !match || err != nil { w.Header().Set(common.HTTPAuthenticationHeader, basicRealm) renderError(dataprovider.ErrInvalidCredentials, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized) diff --git a/httpd/httpd_test.go b/httpd/httpd_test.go index 9bf64e09..ee51c071 100644 --- a/httpd/httpd_test.go +++ b/httpd/httpd_test.go @@ -10086,7 +10086,7 @@ func TestUserAPIShares(t *testing.T) { token1, err := getJWTAPIUserTokenFromTestServer(user1.Username, defaultPassword) assert.NoError(t, err) - // the share username will be set from + // the share username will be set from the claims share := dataprovider.Share{ Name: "share1", Description: "description1", @@ -10145,10 +10145,13 @@ func TestUserAPIShares(t *testing.T) { s, err := dataprovider.ShareExists(objectID, defaultUsername) assert.NoError(t, err) - match, err := s.CheckPassword(defaultPassword) + match, err := s.CheckCredentials(defaultUsername, defaultPassword) assert.True(t, match) assert.NoError(t, err) - match, err = s.CheckPassword(defaultPassword + "mod") + match, err = s.CheckCredentials(defaultUsername, defaultPassword+"mod") + assert.False(t, match) + assert.Error(t, err) + match, err = s.CheckCredentials(altAdminUsername, defaultPassword) assert.False(t, match) assert.Error(t, err) @@ -10163,10 +10166,10 @@ func TestUserAPIShares(t *testing.T) { s, err = dataprovider.ShareExists(objectID, defaultUsername) assert.NoError(t, err) - match, err = s.CheckPassword(defaultPassword) + match, err = s.CheckCredentials(defaultUsername, defaultPassword) assert.True(t, match) assert.NoError(t, err) - match, err = s.CheckPassword(defaultPassword + "mod") + match, err = s.CheckCredentials(defaultUsername, defaultPassword+"mod") assert.False(t, match) assert.Error(t, err) @@ -10283,6 +10286,58 @@ func TestUserAPIShares(t *testing.T) { assert.NoError(t, err) } +func TestUsersAPISharesNoPasswordDisabled(t *testing.T) { + u := getTestUser() + u.Filters.WebClient = []string{sdk.WebClientShareNoPasswordDisabled} + user, _, err := httpdtest.AddUser(u, http.StatusCreated) + assert.NoError(t, err) + token, err := getJWTAPIUserTokenFromTestServer(defaultUsername, defaultPassword) + assert.NoError(t, err) + + share := dataprovider.Share{ + Name: "s", + Scope: dataprovider.ShareScopeRead, + Paths: []string{"/"}, + } + asJSON, err := json.Marshal(share) + assert.NoError(t, err) + req, err := http.NewRequest(http.MethodPost, userSharesPath, bytes.NewBuffer(asJSON)) + assert.NoError(t, err) + setBearerForReq(req, token) + rr := executeRequest(req) + checkResponseCode(t, http.StatusForbidden, rr) + assert.Contains(t, rr.Body.String(), "You are not authorized to share files/folders without a password") + + share.Password = defaultPassword + asJSON, err = json.Marshal(share) + assert.NoError(t, err) + req, err = http.NewRequest(http.MethodPost, userSharesPath, bytes.NewBuffer(asJSON)) + assert.NoError(t, err) + setBearerForReq(req, token) + rr = executeRequest(req) + checkResponseCode(t, http.StatusCreated, rr) + location := rr.Header().Get("Location") + assert.NotEmpty(t, location) + objectID := rr.Header().Get("X-Object-ID") + assert.NotEmpty(t, objectID) + assert.Equal(t, fmt.Sprintf("%v/%v", userSharesPath, objectID), location) + + share.Password = "" + asJSON, err = json.Marshal(share) + assert.NoError(t, err) + req, err = http.NewRequest(http.MethodPut, location, bytes.NewBuffer(asJSON)) + assert.NoError(t, err) + setBearerForReq(req, token) + rr = executeRequest(req) + checkResponseCode(t, http.StatusForbidden, rr) + assert.Contains(t, rr.Body.String(), "You are not authorized to share files/folders without a password") + + _, err = httpdtest.RemoveUser(user, http.StatusOK) + assert.NoError(t, err) + err = os.RemoveAll(user.GetHomeDir()) + assert.NoError(t, err) +} + func TestUserAPIKey(t *testing.T) { u := getTestUser() u.Filters.AllowAPIKeyAuth = true @@ -12519,7 +12574,7 @@ func TestWebUserShare(t *testing.T) { // check the password s, err := dataprovider.ShareExists(share.ShareID, user.Username) assert.NoError(t, err) - match, err := s.CheckPassword(defaultPassword) + match, err := s.CheckCredentials(user.Username, defaultPassword) assert.NoError(t, err) assert.True(t, match) @@ -12566,6 +12621,76 @@ func TestWebUserShare(t *testing.T) { assert.NoError(t, err) } +func TestWebUserShareNoPasswordDisabled(t *testing.T) { + u := getTestUser() + u.Filters.WebClient = []string{sdk.WebClientShareNoPasswordDisabled} + user, _, err := httpdtest.AddUser(u, http.StatusCreated) + assert.NoError(t, err) + csrfToken, err := getCSRFToken(httpBaseURL + webClientLoginPath) + assert.NoError(t, err) + token, err := getJWTWebClientTokenFromTestServer(defaultUsername, defaultPassword) + assert.NoError(t, err) + userAPItoken, err := getJWTAPIUserTokenFromTestServer(defaultUsername, defaultPassword) + assert.NoError(t, err) + + share := dataprovider.Share{ + Name: "s", + Scope: dataprovider.ShareScopeRead, + Paths: []string{"/"}, + } + form := make(url.Values) + form.Set("name", share.Name) + form.Set("scope", strconv.Itoa(int(share.Scope))) + form.Set("paths", "/") + form.Set("max_tokens", "0") + form.Set(csrfFormToken, csrfToken) + req, err := http.NewRequest(http.MethodPost, webClientSharePath, bytes.NewBuffer([]byte(form.Encode()))) + assert.NoError(t, err) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + setJWTCookieForReq(req, token) + rr := executeRequest(req) + checkResponseCode(t, http.StatusForbidden, rr) + assert.Contains(t, rr.Body.String(), "You are not authorized to share files/folders without a password") + + form.Set("password", defaultPassword) + req, err = http.NewRequest(http.MethodPost, webClientSharePath, bytes.NewBuffer([]byte(form.Encode()))) + assert.NoError(t, err) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + setJWTCookieForReq(req, token) + rr = executeRequest(req) + checkResponseCode(t, http.StatusSeeOther, rr) + + req, err = http.NewRequest(http.MethodGet, userSharesPath, nil) + assert.NoError(t, err) + setBearerForReq(req, userAPItoken) + rr = executeRequest(req) + checkResponseCode(t, http.StatusOK, rr) + var shares []dataprovider.Share + err = json.Unmarshal(rr.Body.Bytes(), &shares) + assert.NoError(t, err) + if assert.Len(t, shares, 1) { + s := shares[0] + assert.Equal(t, share.Name, s.Name) + assert.Equal(t, share.Scope, s.Scope) + assert.Equal(t, share.Paths, s.Paths) + share.ShareID = s.ShareID + } + assert.NotEmpty(t, share.ShareID) + form.Set("password", "") + req, err = http.NewRequest(http.MethodPost, webClientSharePath+"/"+share.ShareID, bytes.NewBuffer([]byte(form.Encode()))) + assert.NoError(t, err) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + setJWTCookieForReq(req, token) + rr = executeRequest(req) + checkResponseCode(t, http.StatusForbidden, rr) + assert.Contains(t, rr.Body.String(), "You are not authorized to share files/folders without a password") + + err = os.RemoveAll(user.GetHomeDir()) + assert.NoError(t, err) + _, err = httpdtest.RemoveUser(user, http.StatusOK) + assert.NoError(t, err) +} + func TestWebUserProfile(t *testing.T) { user, _, err := httpdtest.AddUser(getTestUser(), http.StatusCreated) assert.NoError(t, err) diff --git a/httpd/webclient.go b/httpd/webclient.go index 574811d9..45febd21 100644 --- a/httpd/webclient.go +++ b/httpd/webclient.go @@ -982,6 +982,12 @@ func handleClientAddSharePost(w http.ResponseWriter, r *http.Request) { share.ShareID = util.GenerateUniqueID() share.LastUseAt = 0 share.Username = claims.Username + if share.Password == "" { + if util.IsStringInSlice(sdk.WebClientShareNoPasswordDisabled, claims.Permissions) { + renderClientForbiddenPage(w, r, "You are not authorized to share files/folders without a password") + return + } + } err = dataprovider.AddShare(share, claims.Username, util.GetIPFromRemoteAddress(r.RemoteAddr)) if err == nil { http.Redirect(w, r, webClientSharesPath, http.StatusSeeOther) @@ -1020,6 +1026,12 @@ func handleClientUpdateSharePost(w http.ResponseWriter, r *http.Request) { if updatedShare.Password == redactedSecret { updatedShare.Password = share.Password } + if updatedShare.Password == "" { + if util.IsStringInSlice(sdk.WebClientShareNoPasswordDisabled, claims.Permissions) { + renderClientForbiddenPage(w, r, "You are not authorized to share files/folders without a password") + return + } + } err = dataprovider.UpdateShare(updatedShare, claims.Username, util.GetIPFromRemoteAddress(r.RemoteAddr)) if err == nil { http.Redirect(w, r, webClientSharesPath, http.StatusSeeOther) diff --git a/openapi/openapi.yaml b/openapi/openapi.yaml index c5daf90e..7eb89a8b 100644 --- a/openapi/openapi.yaml +++ b/openapi/openapi.yaml @@ -4355,6 +4355,7 @@ components: - FTP - DAV - HTTP + - HTTPShare - DataRetention - OIDC description: | @@ -4364,6 +4365,7 @@ components: * `FTP` - plain FTP and FTPES/FTPS * `DAV` - WebDAV * `HTTP` - WebClient/REST API + * `HTTPShare` - the event is generated in a public share * `DataRetention` - the event is generated by a data retention check * `OIDC` - OpenID Connect WebClientOptions: @@ -4377,6 +4379,7 @@ components: - info-change-disabled - shares-disabled - password-reset-disabled + - shares-without-password-disabled description: | Options: * `publickey-change-disabled` - changing SSH public keys is not allowed @@ -4385,8 +4388,9 @@ components: * `password-change-disabled` - changing password is not allowed * `api-key-auth-change-disabled` - enabling/disabling API key authentication is not allowed * `info-change-disabled` - changing info such as email and description is not allowed - * `shares-disabled` - sharing files and directories with external users is disabled - * `password-reset-disabled` - resetting the password is disabled + * `shares-disabled` - sharing files and directories with external users is not allowed + * `password-reset-disabled` - resetting the password is not allowed + * `shares-without-password-disabled` - creating shares without password protection is not allowed RetentionCheckNotification: type: string enum: