From fbe7a784e84a4116b1d47f443fe6fa7aa487922e Mon Sep 17 00:00:00 2001 From: Denis Kirillov Date: Tue, 5 Mar 2024 09:56:12 +0300 Subject: [PATCH] [#301] Support GetBucketPolicyStatus Signed-off-by: Denis Kirillov --- CHANGELOG.md | 1 + api/errors/errors.go | 7 +++++ api/handler/acl.go | 47 ++++++++++++++++++++++++++++++++ api/handler/acl_test.go | 52 ++++++++++++++++++++++++++++++++++++ api/handler/handlers_test.go | 37 ++++++++++++++++++++++--- api/handler/response.go | 13 +++++++++ api/middleware/constants.go | 2 ++ api/router.go | 4 +++ api/router_mock_test.go | 5 ++++ docs/aws_s3_compat.md | 43 +++++++++++++++++++++-------- 10 files changed, 196 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5447d81..eea46e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ This document outlines major changes between releases. - Support `proxy` contract (#287) - Authmate: support custom attributes (#292) - Add new `reconnect_interval` config param (#291) +- Support `GetBucketPolicyStatus` (#301) ### Changed - Generalise config param `use_default_xmlns_for_complete_multipart` to `use_default_xmlns` so that use default xmlns for all requests (#221) diff --git a/api/errors/errors.go b/api/errors/errors.go index 3304e67..096b25b 100644 --- a/api/errors/errors.go +++ b/api/errors/errors.go @@ -91,6 +91,7 @@ const ( ErrBucketNotEmpty ErrAllAccessDisabled ErrMalformedPolicy + ErrMalformedPolicyNotPrincipal ErrMissingFields ErrMissingCredTag ErrCredMalformed @@ -665,6 +666,12 @@ var errorCodes = errorCodeMap{ Description: "Policy has invalid resource.", HTTPStatusCode: http.StatusBadRequest, }, + ErrMalformedPolicyNotPrincipal: { + ErrCode: ErrMalformedPolicyNotPrincipal, + Code: "MalformedPolicy", + Description: "Allow with NotPrincipal is not allowed.", + HTTPStatusCode: http.StatusBadRequest, + }, ErrMissingFields: { ErrCode: ErrMissingFields, Code: "MissingFields", diff --git a/api/handler/acl.go b/api/handler/acl.go index ae8877a..fdfdf6b 100644 --- a/api/handler/acl.go +++ b/api/handler/acl.go @@ -650,6 +650,48 @@ func (h *handler) PutObjectACLHandler(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) } +func (h *handler) GetBucketPolicyStatusHandler(w http.ResponseWriter, r *http.Request) { + reqInfo := middleware.GetReqInfo(r.Context()) + + bktInfo, err := h.getBucketAndCheckOwner(r, reqInfo.BucketName) + if err != nil { + h.logAndSendError(w, "could not get bucket info", reqInfo, err) + return + } + + jsonPolicy, err := h.ape.GetBucketPolicy(reqInfo.Namespace, bktInfo.CID) + if err != nil { + if strings.Contains(err.Error(), "not found") { + err = fmt.Errorf("%w: %s", errors.GetAPIError(errors.ErrNoSuchBucketPolicy), err.Error()) + } + h.logAndSendError(w, "failed to get policy from storage", reqInfo, err) + return + } + + var bktPolicy engineiam.Policy + if err = json.Unmarshal(jsonPolicy, &bktPolicy); err != nil { + h.logAndSendError(w, "could not parse bucket policy", reqInfo, err) + return + } + + policyStatus := &PolicyStatus{ + IsPublic: PolicyStatusIsPublicFalse, + } + + for _, st := range bktPolicy.Statement { + // https://docs.aws.amazon.com/AmazonS3/latest/userguide/access-control-block-public-access.html#access-control-block-public-access-policy-status + if _, ok := st.Principal[engineiam.Wildcard]; ok { + policyStatus.IsPublic = PolicyStatusIsPublicTrue + break + } + } + + if err = middleware.EncodeToResponse(w, policyStatus); err != nil { + h.logAndSendError(w, "encode and write response", reqInfo, err) + return + } +} + func (h *handler) GetBucketPolicyHandler(w http.ResponseWriter, r *http.Request) { reqInfo := middleware.GetReqInfo(r.Context()) @@ -731,6 +773,11 @@ func (h *handler) PutBucketPolicyHandler(w http.ResponseWriter, r *http.Request) return } + if len(stat.NotPrincipal) != 0 && stat.Effect == engineiam.AllowEffect { + h.logAndSendError(w, "invalid NotPrincipal", reqInfo, errors.GetAPIError(errors.ErrMalformedPolicyNotPrincipal)) + return + } + for _, resource := range stat.Resource { if reqInfo.BucketName != strings.Split(strings.TrimPrefix(resource, arnAwsPrefix), "/")[0] { h.logAndSendError(w, "policy resource mismatched bucket", reqInfo, errors.GetAPIError(errors.ErrMalformedPolicy)) diff --git a/api/handler/acl_test.go b/api/handler/acl_test.go index 7d02c33..52f5a0c 100644 --- a/api/handler/acl_test.go +++ b/api/handler/acl_test.go @@ -7,6 +7,7 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" + "encoding/xml" "fmt" "io" "net/http" @@ -1467,6 +1468,41 @@ func TestBucketPolicy(t *testing.T) { require.Equal(t, newPolicy, bktPolicy) } +func TestBucketPolicyStatus(t *testing.T) { + hc := prepareHandlerContext(t) + bktName := "bucket-for-policy" + + createTestBucket(hc, bktName) + + getBucketPolicy(hc, bktName, s3errors.ErrNoSuchBucketPolicy) + + newPolicy := engineiam.Policy{ + Statement: []engineiam.Statement{{ + NotPrincipal: engineiam.Principal{engineiam.Wildcard: {}}, + Effect: engineiam.AllowEffect, + Action: engineiam.Action{"s3:PutObject"}, + Resource: engineiam.Resource{arnAwsPrefix + bktName + "/*"}, + }}, + } + + putBucketPolicy(hc, bktName, newPolicy, s3errors.ErrMalformedPolicyNotPrincipal) + + newPolicy.Statement[0].NotPrincipal = nil + newPolicy.Statement[0].Principal = map[engineiam.PrincipalType][]string{engineiam.Wildcard: {}} + putBucketPolicy(hc, bktName, newPolicy) + bktPolicyStatus := getBucketPolicyStatus(hc, bktName) + require.True(t, PolicyStatusIsPublicTrue == bktPolicyStatus.IsPublic) + + key, err := keys.NewPrivateKey() + require.NoError(t, err) + hc.Handler().frostfsid.(*frostfsidMock).data["devenv"] = key.PublicKey() + + newPolicy.Statement[0].Principal = map[engineiam.PrincipalType][]string{engineiam.AWSPrincipalType: {"arn:aws:iam:::user/devenv"}} + putBucketPolicy(hc, bktName, newPolicy) + bktPolicyStatus = getBucketPolicyStatus(hc, bktName) + require.True(t, PolicyStatusIsPublicFalse == bktPolicyStatus.IsPublic) +} + func TestBucketPolicyUnmarshal(t *testing.T) { for _, tc := range []struct { name string @@ -1557,6 +1593,22 @@ func getBucketPolicy(hc *handlerContext, bktName string, errCode ...s3errors.Err return policy } +func getBucketPolicyStatus(hc *handlerContext, bktName string, errCode ...s3errors.ErrorCode) PolicyStatus { + w, r := prepareTestRequest(hc, bktName, "", nil) + hc.Handler().GetBucketPolicyStatusHandler(w, r) + + var policyStatus PolicyStatus + if len(errCode) == 0 { + assertStatus(hc.t, w, http.StatusOK) + err := xml.NewDecoder(w.Result().Body).Decode(&policyStatus) + require.NoError(hc.t, err) + } else { + assertS3Error(hc.t, w, s3errors.GetAPIError(errCode[0])) + } + + return policyStatus +} + func putBucketPolicy(hc *handlerContext, bktName string, bktPolicy engineiam.Policy, errCode ...s3errors.ErrorCode) { body, err := json.Marshal(bktPolicy) require.NoError(hc.t, err) diff --git a/api/handler/handlers_test.go b/api/handler/handlers_test.go index 259f787..3e56bf0 100644 --- a/api/handler/handlers_test.go +++ b/api/handler/handlers_test.go @@ -4,8 +4,10 @@ import ( "bytes" "context" "crypto/rand" + "encoding/hex" "encoding/xml" "errors" + "fmt" "io" "net/http" "net/http/httptest" @@ -177,10 +179,11 @@ func prepareHandlerContextBase(t *testing.T, cacheCfg *layer.CachesConfig) *hand defaultPolicy: pp, } h := &handler{ - log: l, - obj: layer.NewLayer(l, tp, layerCfg), - cfg: cfg, - ape: newAPEMock(), + log: l, + obj: layer.NewLayer(l, tp, layerCfg), + cfg: cfg, + ape: newAPEMock(), + frostfsid: newFrostfsIDMock(), } return &handlerContext{ @@ -301,6 +304,32 @@ func (a *apeMock) SaveACLChains(ns string, chains []*chain.Chain) error { return nil } +type frostfsidMock struct { + data map[string]*keys.PublicKey +} + +func newFrostfsIDMock() *frostfsidMock { + return &frostfsidMock{data: map[string]*keys.PublicKey{}} +} + +func (f *frostfsidMock) GetUserAddress(account, user string) (string, error) { + res, ok := f.data[account+user] + if !ok { + return "", fmt.Errorf("not found") + } + + return res.Address(), nil +} + +func (f *frostfsidMock) GetUserKey(account, user string) (string, error) { + res, ok := f.data[account+user] + if !ok { + return "", fmt.Errorf("not found") + } + + return hex.EncodeToString(res.Bytes()), nil +} + func createTestBucket(hc *handlerContext, bktName string) *data.BucketInfo { info := createBucket(hc, bktName) return info.BktInfo diff --git a/api/handler/response.go b/api/handler/response.go index c4edc84..e1ed08d 100644 --- a/api/handler/response.go +++ b/api/handler/response.go @@ -55,6 +55,19 @@ type Bucket struct { CreationDate string // time string of format "2006-01-02T15:04:05.000Z" } +// PolicyStatus contains status of bucket policy. +type PolicyStatus struct { + XMLName xml.Name `xml:"http://s3.amazonaws.com/doc/2006-03-01/ PolicyStatus" json:"-"` + IsPublic PolicyStatusIsPublic `xml:"IsPublic"` +} + +type PolicyStatusIsPublic string + +const ( + PolicyStatusIsPublicFalse = "FALSE" + PolicyStatusIsPublicTrue = "TRUE" +) + // AccessControlPolicy contains ACL. type AccessControlPolicy struct { XMLName xml.Name `xml:"http://s3.amazonaws.com/doc/2006-03-01/ AccessControlPolicy" json:"-"` diff --git a/api/middleware/constants.go b/api/middleware/constants.go index b13be8d..47f6532 100644 --- a/api/middleware/constants.go +++ b/api/middleware/constants.go @@ -9,6 +9,7 @@ const ( HeadBucketOperation = "HeadBucket" ListMultipartUploadsOperation = "ListMultipartUploads" GetBucketLocationOperation = "GetBucketLocation" + GetBucketPolicyStatusOperation = "GetBucketPolicyStatus" GetBucketPolicyOperation = "GetBucketPolicy" GetBucketLifecycleOperation = "GetBucketLifecycle" GetBucketEncryptionOperation = "GetBucketEncryption" @@ -77,6 +78,7 @@ const ( const ( UploadsQuery = "uploads" LocationQuery = "location" + PolicyStatusQuery = "policyStatus" PolicyQuery = "policy" LifecycleQuery = "lifecycle" EncryptionQuery = "encryption" diff --git a/api/router.go b/api/router.go index c873b71..86e0362 100644 --- a/api/router.go +++ b/api/router.go @@ -37,6 +37,7 @@ type ( PutObjectHandler(http.ResponseWriter, *http.Request) DeleteObjectHandler(http.ResponseWriter, *http.Request) GetBucketLocationHandler(http.ResponseWriter, *http.Request) + GetBucketPolicyStatusHandler(http.ResponseWriter, *http.Request) GetBucketPolicyHandler(http.ResponseWriter, *http.Request) GetBucketLifecycleHandler(http.ResponseWriter, *http.Request) GetBucketEncryptionHandler(http.ResponseWriter, *http.Request) @@ -230,6 +231,9 @@ func bucketRouter(h Handler, log *zap.Logger) chi.Router { Add(NewFilter(). Queries(s3middleware.LocationQuery). Handler(named(s3middleware.GetBucketLocationOperation, h.GetBucketLocationHandler))). + Add(NewFilter(). + Queries(s3middleware.PolicyStatusQuery). + Handler(named(s3middleware.GetBucketPolicyStatusOperation, h.GetBucketPolicyStatusHandler))). Add(NewFilter(). Queries(s3middleware.PolicyQuery). Handler(named(s3middleware.GetBucketPolicyOperation, h.GetBucketPolicyHandler))). diff --git a/api/router_mock_test.go b/api/router_mock_test.go index dfe5c41..e8fe457 100644 --- a/api/router_mock_test.go +++ b/api/router_mock_test.go @@ -186,6 +186,11 @@ func (h *handlerMock) GetBucketLocationHandler(http.ResponseWriter, *http.Reques panic("implement me") } +func (h *handlerMock) GetBucketPolicyStatusHandler(http.ResponseWriter, *http.Request) { + //TODO implement me + panic("implement me") +} + func (h *handlerMock) GetBucketPolicyHandler(http.ResponseWriter, *http.Request) { //TODO implement me panic("implement me") diff --git a/docs/aws_s3_compat.md b/docs/aws_s3_compat.md index 7d0c07f..39bd868 100644 --- a/docs/aws_s3_compat.md +++ b/docs/aws_s3_compat.md @@ -207,17 +207,38 @@ See also `GetObject` and other method parameters. ## Policy and replication -| | Method | Comments | -|----|-------------------------|-----------------------------| -| 🔵 | DeleteBucketPolicy | | -| 🔵 | DeleteBucketReplication | | -| 🔵 | DeletePublicAccessBlock | | -| 🟡 | GetBucketPolicy | See ACL limitations | -| 🔵 | GetBucketPolicyStatus | | -| 🔵 | GetBucketReplication | | -| 🟢 | PostPolicyBucket | Upload file using POST form | -| 🟡 | PutBucketPolicy | See ACL limitations | -| 🔵 | PutBucketReplication | | +Bucket policy has the following limitations +* Supports only AWS principals in format `arn:aws:iam:::user/` or wildcard `*`. +* No complex conditions (only conditions for groups now supported) + +Simple valid policy example: +```json +{ + "Version": "2012-10-17", + "Statement": [{ + "Principal": {"AWS": ["arn:aws:iam::111122223333:role/JohnDoe"]}, + "Effect": "Allow", + "Action": ["s3:GetObject","s3:GetObjectVersion"], + "Resource": ["arn:aws:s3:::DOC-EXAMPLE-BUCKET/*"] + }] +} +``` + +Bucket policy status determines using the following scheme: +* If policy has statement with principal that is wildcard (`*`) then policy is considered as public + + +| | Method | Comments | +|-----|-------------------------|-----------------------------| +| 🟡 | DeleteBucketPolicy | See Policy limitations | +| 🔵 | DeleteBucketReplication | | +| 🔵 | DeletePublicAccessBlock | | +| 🟡 | GetBucketPolicy | See Policy limitations | +| 🟡 | GetBucketPolicyStatus | | +| 🔵 | GetBucketReplication | | +| 🟢 | PostPolicyBucket | Upload file using POST form | +| 🟡 | PutBucketPolicy | See Policy limitations | +| 🔵 | PutBucketReplication | | ## Request payment