Make services GroupIDs must also be target of APE checks #1193

aarifullin · 2024-06-20T12:51:32Z

aarifullin commented

2024-06-20 12:51:32 +00:00

tree: GroupIDs must also be target of APE checks
object: GroupIDs must also be target of APE checks
container: GroupIDs must also be target of APE checks
ape: Introduce Groups util function to retrieve actor's groupIDs
Also add new test case for ape middleware in container service.

* tree: GroupIDs must also be target of APE checks * object: GroupIDs must also be target of APE checks * container: GroupIDs must also be target of APE checks * ape: Introduce `Groups` util function to retrieve actor's groupIDs * Also add new test case for ape middleware in container service. Close #1190

aarifullin requested review from dkirillov 2024-06-20 12:54:48 +00:00

aarifullin requested review from storage-core-committers 2024-06-20 12:54:52 +00:00

aarifullin requested review from storage-core-developers 2024-06-20 12:55:56 +00:00

aarifullin force-pushed feat/groupids_target from c1ae4640ad to 6feb58978c

2024-06-20 13:04:39 +00:00

Compare

aarifullin force-pushed feat/groupids_target from 6feb58978c to 9ab5801c29

2024-06-20 13:07:16 +00:00

Compare

dkirillov reviewed 2024-06-20 13:57:12 +00:00

pkg/ape/request/frostfsid.go

					
				@ -37,0 +37,4 @@

				// Groups return the actor's group ids from frostfsid contract.

				func Groups(frostFSIDClient frostfsidcore.SubjectProvider, pk *keys.PublicKey) ([]string, error) {

					subj, err := frostFSIDClient.GetSubjectExtended(pk.GetScriptHash())

dkirillov commented

2024-06-20 13:54:20 +00:00

We have cache in frostfsIDClient that's why we can freely invoke Groups function along with FormFrostfsIDRequestProperties ?

We have cache in `frostfsIDClient` that's why we can freely invoke `Groups` function along with `FormFrostfsIDRequestProperties` ?

aarifullin commented

2024-06-20 14:40:15 +00:00

we can freely

Sorry, I don't get your point. Do you mean we can retrieve groups after FormFrostfsIDRequestProperties? Yes, we can but this requires some hurtful refactor in all services. For me that was easier to perform some duplication as the cache is used

> we can freely Sorry, I don't get your point. Do you mean we can retrieve groups after `FormFrostfsIDRequestProperties`? Yes, we can but this requires some hurtful refactor in all services. For me that was easier to perform some duplication as the cache is used

👍 1

pkg/services/container/ape.go Outdated

					
				@ -171,1 +176,4 @@

					}

					rt.Groups = make([]policyengine.Target, len(groups))

					for i := range groups {

						rt.Groups[i] = policyengine.GroupTarget(groups[i])

dkirillov commented

2024-06-20 13:48:22 +00:00

As I know, group target must be <ns>:<gropuID> rather than just <groupID>

As I know, group target must be `<ns>:<gropuID>` rather than just `<groupID>`

aarifullin commented

2024-06-21 07:25:22 +00:00

Didn't think about that - fixed

👍 1

pkg/services/object/ape/checker.go Outdated

					
				@ -167,2 +169,4 @@

						return fmt.Errorf("failed to get group ids: %w", err)

					}

					if prm.BearerToken != nil && !prm.BearerToken.Impersonate() {

dkirillov commented

2024-06-20 13:57:01 +00:00

I'm not sure the following question relates to this PR but still.
Why do we check only container target (policyengine.NewRequestTargetWithContainer) in case of APE chains from bearer token? As I understand user can set any target in APEOverride

I'm not sure the following question relates to this PR but still. Why do we check only container target (`policyengine.NewRequestTargetWithContainer`) in case of APE chains from bearer token? As I understand user can set any target in `APEOverride`

aarifullin commented

2024-06-20 14:42:33 +00:00

This is great that you've asked this. Because I was wondering can be other targets used in BT? In some sense I "inherited" this scheme from eACL tables in BT where eACL table's target was only container. Let's discuss

dkirillov commented

2024-06-24 06:16:44 +00:00

Probably we should ask @fyrchik @alexvanin

fyrchik commented

2024-06-24 06:38:27 +00:00

The current implementation was indeed intended to serve as a replacement for EACL.
For namespaces there are some security considerations:

Container owner should not be able to override rules set by the namespace admin. Or should it?
If namespace admin allowed user A to get an object with a bearer token, can user A allow user B to get the same object if namespace rules prohibit user B any operations?

Another thing is that overriding namespace rules in isolation is probably not that useful, we would like to probably combine that with other overrides.

The current implementation was indeed intended to serve as a replacement for EACL. For namespaces there are some security considerations: 1. Container **owner** should not be able to override rules set by the namespace **admin**. Or should it? 2. If namespace admin allowed user A to get an object with a bearer token, can user A allow user B to get the same object if namespace rules prohibit user B any operations? Another thing is that overriding namespace rules in isolation is probably not that useful, we would like to probably combine that with other overrides.

👍 1

dkirillov commented

2024-06-24 07:48:36 +00:00

I've just meant that if we allow form APE chain with any target in bearer token we should be able process such token in node

I've just meant that if we allow form APE chain with [any target](https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/src/commit/1a5886e776de79fc6598838242e4dc7ff21e7bea/bearer/bearer.go#L52) in bearer token we should be able process such token in node

aarifullin commented

2024-06-24 11:13:30 +00:00

if we allow form APE chain with

Yes, sdk freely sets a target for APEOverride within a bearer token but sdk doesn't care would a service that uses this token should allow or deny such target. The question is do we really or will we want to override other targets with BT

> if we allow form APE chain with Yes, `sdk` freely sets a target for `APEOverride` within a bearer token but `sdk` doesn't care would a service that uses this token should allow or deny such target. The question is do we really or will we want to override other targets with BT