Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove attach of bearer token when bucket owner is not an issuer of the bearer token #487

Merged
merged 3 commits into from
Jun 3, 2022

Conversation

masterSplinter01
Copy link
Contributor

The test_object_copy_canned_acl test still not passing because main_wallet creates a private container in tests and alt_wallet can't get-object from private container, even the object was put with public-read acl.

I tried to make main-wallet create public-read container and the test began to pass.

I suggest to fix the test and place it in test_s3_neofs.py.

Closes #459

masterSplinter01 added a commit to masterSplinter01/neofs-s3-gw that referenced this pull request Jun 1, 2022
When bucket owner is not an issuer of the bearer token

Signed-off-by: Angira Kekteeva <kira@nspcc.ru>
@@ -168,7 +168,6 @@ func (h *handler) GetObjectHandler(w http.ResponseWriter, r *http.Request) {
ObjectInfo: info,
Writer: w,
Range: params,
VersionID: p.VersionID,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like the field is not used, also we have versionID in ObjectInfo field

if bd, ok := ctx.Value(api.BoxData).(*accessbox.Box); ok && bd != nil && bd.Gate != nil {
prm.BearerToken = bd.Gate.BearerToken
return
if issuer, ok := bd.Gate.BearerToken.Issuer(); ok && bktOwner.Equals(issuer) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we can adopt #485

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to rebase tree-service branch after this PR. Adopt it in separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I messed up with the branches. Both SDK update and this PR were into master branch, so we could adopt it. For some reason I thought this PR has tree-service target.

I pushed 4ed9397 into master branch.

Comment on lines -383 to +384
params.oid = p.ObjectInfo.ID
params.cid = p.ObjectInfo.CID
params.objInfo = p.ObjectInfo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we can keep the old getParams fields. And extend GetObjectParams by bktInfo and pass it (bktInfo) as the second parameter to n.initObjectPayloadReader function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see huge difference between these solutions.

I suggested to make getParams keeping *data.ObjectInfo because in all usages (in both master and tree-service) we extract oid and cid from objectInfo anyway, so we can easily put objectInfo into getParams and pass all necessary information inside getParams what looks a little clearer than passing getParams with cid and bktInfo to n.initObjectPayloadReader.

Copy link
Contributor

@KirillovDenis KirillovDenis Jun 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Let's extend getParams.
The main point was using bktinfo somehow to avoid:

	// should be taken from cache
	bktInfo, err := n.GetBucketInfo(ctx, p.objInfo.Bucket)
	if err != nil {
		return nil, err
	}

in n.initObjectPayloadReader function

Copy link
Contributor Author

@masterSplinter01 masterSplinter01 Jun 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this with dragging of bktInfo through params. Added a separated commit to make it easier to view changes, will squash it later

@KirillovDenis
Copy link
Contributor

Please fix tests

When bucket owner is not an issuer of the bearer token

Signed-off-by: Angira Kekteeva <kira@nspcc.ru>
@alexvanin
Copy link
Contributor

alexvanin commented Jun 2, 2022

can't get-object from private container, even the object was put with public-read acl.

I suggest to fix the test and place it in test_s3_neofs.py.

It's more like new S3 issue to fix object ACLs, isn't it?

  1. main_wallet creates private container -- extended ACL with 7 deny rules for target OTHER
  2. main_wallet sets public read acl for the object -- extended ACL has one more additional allow rule to this object for target OTHER, it is a first rule in the table
  3. alt_wallet tries to get object -- without bearer token NeoFS node applies container EACL to request => first rule allows to get object.

Signed-off-by: Angira Kekteeva <kira@nspcc.ru>
Signed-off-by: Angira Kekteeva <kira@nspcc.ru>
@codecov
Copy link

codecov bot commented Jun 2, 2022

Codecov Report

❗ No coverage uploaded for pull request base (master@2ca4dbb). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #487   +/-   ##
=========================================
  Coverage          ?   27.92%           
=========================================
  Files             ?       46           
  Lines             ?     5454           
  Branches          ?        0           
=========================================
  Hits              ?     1523           
  Misses            ?     3729           
  Partials          ?      202           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ca4dbb...2469291. Read the comment docs.

@alexvanin
Copy link
Contributor

can't get-object from private container, even the object was put with public-read acl.
I suggest to fix the test and place it in test_s3_neofs.py.

It's more like new S3 issue to fix object ACLs, isn't it?

1. `main_wallet` creates private container -- extended ACL with 7 `deny` rules for target `OTHER`

2. `main_wallet` sets public read acl for the object -- extended ACL has one more additional `allow` rule to this object for target `OTHER`, it is a first rule in the table

3. `alt_wallet` tries to get object -- without bearer token NeoFS node applies container EACL to request => first rule allows to get object.

As @masterSplinter01 mentioned, we got access deny error because of search request. Which is fair enough, because we do search in the bucket. tree-service branch avoids extra search, so maybe the issue should be gone. Let's investigate that and fix per object public ACL in private containers.

@alexvanin alexvanin merged commit dbfac29 into nspcc-dev:master Jun 3, 2022
@masterSplinter01 masterSplinter01 deleted the fix-copy-test branch August 9, 2022 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix test_object_copy_canned_acl
3 participants