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

Consider ACL filter optimizations #576

Open
alexvanin opened this issue Jul 5, 2022 · 2 comments
Open

Consider ACL filter optimizations #576

alexvanin opened this issue Jul 5, 2022 · 2 comments
Labels
enhancement Improving existing functionality I4 No visible changes performance More of something per second S4 Routine U4 Nothing urgent

Comments

@alexvanin
Copy link
Contributor

Part one: simplify eacl -> ast decoding by using system filters

Use system filters to define grantees and permissions, like

SYSTEM: permission STRING_EQUAL FULL_CONTROL
SYSTEM: group STRING_EQUAL AllUsers

These values may be used during encoding and decoding of rules

Part two: reduce number of records in eacl

Some EACL records may not produce behaviour changes, but they required for decoding ast. We can try to remove such records and replace then by storing extra information in system filters of EACL records.

@alexvanin
Copy link
Contributor Author

alexvanin commented Jul 21, 2022

In #605 a tried to fix some issues but acl parse order kicked me in the guts. Here I briefly describe what I tried.

bucketACLToTable upgrade

I want to use AST to provide correct bucket ACL, so I come up with something like this

func bucketACLToTable(acp *AccessControlPolicy, resInfo *resourceInfo) (*eacl.Table, error) {
	if !resInfo.IsBucket() {
		return nil, fmt.Errorf("allowed only bucket acl")
	}

	ownerKey, err := keys.NewPublicKeyFromString(acp.Owner.ID)
	if err != nil {
		return nil, fmt.Errorf("public key from string: %w", err)
	}

	r := astResource{
		resourceInfo: *resInfo,
		Operations:   make([]*astOperation, 0, len(acp.AccessControlList)),
	}

	// first loop
	for _, grant := range acp.AccessControlList {
		if !isValidGrant(grant) {
			return nil, stderrors.New("unsupported grantee")
		}
		getRecord, err := getRecordFunction(grant.Grantee)
		if err != nil {
			return nil, fmt.Errorf("record func from grantee: %w", err)
		}
		for _, op := range permissionToOperations(grant.Permission) {
			rec := getRecord(op)
			r.Operations = addToList(r.Operations, *rec, rec.Targets()[0])
		}
	}

	// second loop 
	for _, op := range fullOps {
		rec := getOthersRecord(op, eacl.ActionDeny)
		r.Operations = addToList(r.Operations, *rec, rec.Targets()[0])
	}

	// third loop
	for _, op := range fullOps {
		rec := getAllowRecord(op, ownerKey)
		r.Operations = addToList(r.Operations, *rec, rec.Targets()[0])
	}

	return astToTable(&ast{Resources: []*astResource{&r}})
}

To make it work, remove mandatory full control grant from

acp.AccessControlList = []*Grant{{
Grantee: &Grantee{
ID: hex.EncodeToString(key.Bytes()),
DisplayName: key.Address(),
Type: acpCanonicalUser,
},
Permission: aclFullControl,
}}

It works very well when creating buckets with canned ACL without grants. However when grant appears, it turns out that DENY record is going to be placed above grant rule, which is incorrect.

This happens because DENY rules appear on the second loop after processing grants. If they go before grants, then there will be useless DENY rules in eACL.

Public object table record

After #605 private objects have correct eACL field order: owner rules above others rules.

ALLOW GET          [ SERVICE:Resource MATCH_TYPE_UNSPECIFIED mid400/foo/versioned-object1:HPVZPnTpkByWybYcPGUCcxyt3qpvgqo4gZkbvTSatJSS, SERVICE:GroupLength MATCH_TYPE_UNSPECIFIED 10 ] [ SYSTEM:{} ]
ALLOW GETRANGEHASH [ OBJECT:$Object:objectID STRING_EQUAL HPVZPnTpkByWybYcPGUCcxyt3qpvgqo4gZkbvTSatJSS ]                                                                                [ ROLE_UNSPECIFIED:{031a6c6fbbdf02ca351745fa86b9ba5a9452d785ac4f7fc2b7548ca2a46c4fcf4a} 
ALLOW GETRANGE     [ OBJECT:$Object:objectID STRING_EQUAL HPVZPnTpkByWybYcPGUCcxyt3qpvgqo4gZkbvTSatJSS ]                                                                                [ ROLE_UNSPECIFIED:{031a6c6fbbdf02ca351745fa86b9ba5a9452d785ac4f7fc2b7548ca2a46c4fcf4a} 
ALLOW SEARCH       [ OBJECT:$Object:objectID STRING_EQUAL HPVZPnTpkByWybYcPGUCcxyt3qpvgqo4gZkbvTSatJSS ]                                                                                [ ROLE_UNSPECIFIED:{031a6c6fbbdf02ca351745fa86b9ba5a9452d785ac4f7fc2b7548ca2a46c4fcf4a} 
ALLOW HEAD         [ OBJECT:$Object:objectID STRING_EQUAL HPVZPnTpkByWybYcPGUCcxyt3qpvgqo4gZkbvTSatJSS ]                                                                                [ ROLE_UNSPECIFIED:{031a6c6fbbdf02ca351745fa86b9ba5a9452d785ac4f7fc2b7548ca2a46c4fcf4a} 
ALLOW GET          [ OBJECT:$Object:objectID STRING_EQUAL HPVZPnTpkByWybYcPGUCcxyt3qpvgqo4gZkbvTSatJSS ]                                                                                [ ROLE_UNSPECIFIED:{031a6c6fbbdf02ca351745fa86b9ba5a9452d785ac4f7fc2b7548ca2a46c4fcf4a} 
DENY  GETRANGEHASH [ OBJECT:$Object:objectID STRING_EQUAL HPVZPnTpkByWybYcPGUCcxyt3qpvgqo4gZkbvTSatJSS ]                                                                                [ OTHERS:{} ]
DENY  GETRANGE     [ OBJECT:$Object:objectID STRING_EQUAL HPVZPnTpkByWybYcPGUCcxyt3qpvgqo4gZkbvTSatJSS ]                                                                                [ OTHERS:{} ]
DENY  SEARCH       [ OBJECT:$Object:objectID STRING_EQUAL HPVZPnTpkByWybYcPGUCcxyt3qpvgqo4gZkbvTSatJSS ]                                                                                [ OTHERS:{} ]
DENY  HEAD         [ OBJECT:$Object:objectID STRING_EQUAL HPVZPnTpkByWybYcPGUCcxyt3qpvgqo4gZkbvTSatJSS ]                                                                                [ OTHERS:{} ]
DENY  GET          [ OBJECT:$Object:objectID STRING_EQUAL HPVZPnTpkByWybYcPGUCcxyt3qpvgqo4gZkbvTSatJSS ]                                                                                [ OTHERS:{} ]

However public objects produce reversed order. It doesn't affect access control, but it looks odd.

To fix that we can set owner related record as last statement. But I am not sure if the change in the order is safe.

func aclToPolicy(acl *AccessControlPolicy, resInfo *resourceInfo) (*bucketPolicy, error) {
	. . .
	results := make([]statement, 0, len(acl.AccessControlList)+1)
	. . .
	results = append(results, getAllowStatement(resInfo, acl.Owner.ID, aclFullControl))

	return &bucketPolicy{
		Statement: results,
		Bucket:    resInfo.Bucket,
	}, nil
}

@alexvanin
Copy link
Contributor Author

All optimization will make more sense after nspcc-dev/neofs-api#241

@roman-khimov roman-khimov added enhancement Improving existing functionality U4 Nothing urgent S4 Routine I4 No visible changes performance More of something per second and removed triage labels Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improving existing functionality I4 No visible changes performance More of something per second S4 Routine U4 Nothing urgent
Projects
None yet
Development

No branches or pull requests

2 participants