Introduce a new service: ape_manager #1105

Merged
fyrchik merged 3 commits from aarifullin/frostfs-node:feat/ape_manager into master 2024-05-27 09:34:25 +00:00
Member

The introduced service ape_manager manages APE policies for native-specific targets like containers.
Unlike control service, that manages only local overrides, ape_manager allows a client to set rule chains in Policy contract for resources it owns.

This PR:

  • Introduces new .proto-s, generates protobufs for a new service "ape_manager";
  • Introduces base58-string randomizer in util package;
  • Introduces contract storage with proxy contract verification. ProxyVerificationContractStorage uses Proxy contract as a cosigner. ProxyVerificationContractStorage has an implementation feature: it recreates a contract storage for each handler invocation because of an issue: rpc-actor from morph client may be expired. Such way won't create a bottlenecks because it is expected that this contract storage implementation will be used not so often. This approach requires to make morph client return RPCActor (wsClient in fact).

To implement ape_manager service this PR:

  • Introduces stubbed APE middleware for ape_service. Checks will be performed as soon as specific verbs will be introduced in policy-engine.
  • Introduce signature middleware to check signatures.
  • Introduce APE manager for container targets: a client can set/remove chains only if 1) the actor is the container's owner; 2) CID is correct; 3) a storage signs a transaction for Policy contract (a storage's account is added to Proxy contract).

Also, it

  • Introduces commands for ape_manager in frostfs-cli
The introduced service `ape_manager` manages APE policies for native-specific targets like containers. Unlike `control` service, that manages only **local** overrides, `ape_manager` allows a client to set rule chains in Policy contract for resources it **owns**. This PR: - Introduces new .proto-s, generates protobufs for a new service "ape_manager"; - Introduces base58-string randomizer in `util` package; - Introduces contract storage with proxy contract verification. `ProxyVerificationContractStorage` uses Proxy contract as a cosigner. `ProxyVerificationContractStorage` has an implementation *feature*: it recreates a contract storage for each handler invocation because of an issue: rpc-actor from morph client may be expired. Such way won't create a bottlenecks because it is expected that this contract storage implementation will be used not so often. This approach requires to make morph client return `RPCActor` (`wsClient` in fact). To implement `ape_manager` service this PR: - Introduces stubbed APE middleware for ape_service. Checks will be performed **as soon as specific verbs** will be introduced in policy-engine. - Introduce signature middleware to check signatures. - Introduce APE manager for container targets: a client can set/remove chains only if 1) the actor is the container's owner; 2) CID is correct; 3) a storage signs a transaction for Policy contract (a storage's account is added to Proxy contract). Also, it - Introduces commands for `ape_manager` in `frostfs-cli`
aarifullin reviewed 2024-04-23 12:21:54 +00:00
@ -0,0 +23,4 @@
)
// ContainerAPEChainManager interface provides methods to manage rule chains for containers in
// Policy contract.
Author
Member

This comment will be fixed

This comment will be fixed
aarifullin marked this conversation as resolved
aarifullin force-pushed feat/ape_manager from 84a56abbcb to e2d8ca4616 2024-04-23 12:25:50 +00:00 Compare
aarifullin requested review from storage-core-committers 2024-04-23 12:31:04 +00:00
aarifullin requested review from storage-core-developers 2024-04-23 12:31:28 +00:00
Owner

Why are *.proto files not in the frostfs-api repo?

Why are `*.proto` files not in the frostfs-api repo?
Author
Member

Why are *.proto files not in the frostfs-api repo?

Should they?

  1. I considered tree service's approach where .proto-s are placed beside implementation
  2. If they are placed in frostfs-api, we need to drag it over this cycle frostfs-api -> frostfs-api-go -> frostfs-sdk-go creating wrappers etc. But I doubt this lightweight service requires this cycle: it barely needs SDK as it won't interract with other nodes

If you convince me that tree service is a specific case, then I'll move it to frostfs-api

> Why are `*.proto` files not in the frostfs-api repo? Should they? 1. I considered tree service's approach where `.proto`-s are placed beside implementation 2. If they are placed in `frostfs-api`, we need to drag it over this cycle `frostfs-api -> frostfs-api-go -> frostfs-sdk-go` creating wrappers etc. But I doubt this lightweight service requires this cycle: it barely needs SDK as it won't interract with other nodes If you convince me that tree service is a specific case, then I'll move it to `frostfs-api`
aarifullin force-pushed feat/ape_manager from e2d8ca4616 to 63fa659132 2024-04-23 13:16:25 +00:00 Compare
Owner

Tree service is not a part of public API (yet) for a reason (unsolved problems related to security and scalability in the untrusted environment).

This service should replace eACL and thus should be a part of the public API.

Tree service is not a part of public API (yet) for a reason (unsolved problems related to security and scalability in the untrusted environment). This service should replace eACL and thus should be a part of the public API.
aarifullin changed title from Introduce a new service: `ape_manager` to WIP: Introduce a new service: `ape_manager` 2024-04-23 13:44:33 +00:00
aarifullin force-pushed feat/ape_manager from 63fa659132 to e9e7f8cd95 2024-05-13 16:08:42 +00:00 Compare
aarifullin force-pushed feat/ape_manager from e9e7f8cd95 to 4b32432a9f 2024-05-15 10:28:49 +00:00 Compare
aarifullin changed title from WIP: Introduce a new service: `ape_manager` to Introduce a new service: `ape_manager` 2024-05-15 10:30:21 +00:00
aarifullin force-pushed feat/ape_manager from 4b32432a9f to bac0efa68a 2024-05-15 10:43:21 +00:00 Compare
aarifullin force-pushed feat/ape_manager from bac0efa68a to bd92279e75 2024-05-15 11:04:18 +00:00 Compare
fyrchik requested changes 2024-05-15 11:24:08 +00:00
@ -0,0 +39,4 @@
)
var (
errUnknownTargetType = errors.New("unknown target type")
Owner

It is used only once and in the place where %w is essentially useless, to we need a variable?

It is used only once and in the place where `%w` is essentially useless, to we need a variable?
@ -0,0 +16,4 @@
errEmptyChainID = errors.New("chain id cannot be empty")
removeChainCmd = &cobra.Command{
Use: "rm-rule-chain",
Owner

We have rm-rule-chain, add-rule-chain and list-rule-chains
I don't think we should have anything besides rule-chain here, so why not remove, add and list?
If we want to mirror api, then rule should be removed.

In any case, I think remove is a better choice than rm (and it is in the api proto definitions too)

We have `rm-rule-chain`, `add-rule-chain` and `list-rule-chains` I don't think we should have anything besides `rule-chain` here, so why not `remove`, `add` and `list`? If we want to mirror api, then `rule` should be removed. In any case, I think `remove` is a better choice than `rm` (and it is in the api proto definitions too)
Author
Member

Agreed. Using add/list/remove names are unambiguous as apemanager manages only chains. So, I have renamed them

Agreed. Using `add/list/remove` names are unambiguous as `apemanager` manages only chains. So, I have renamed them
fyrchik marked this conversation as resolved
@ -0,0 +21,4 @@
}
func (s *APEService) AddChain(ctx context.Context, req *apemanager_v2.AddChainRequest) (*apemanager_v2.AddChainResponse, error) {
// TODO (aarifullin): make this handler perform APE checks as soon as relevant verbs will be introduced in policy-engine.
Owner

If we have a task let's write a link here.
If not, let's remove this TODO.

If we have a task let's write a link here. If not, let's remove this TODO.
Author
Member

TODOs are removed

`TODO`s are removed
fyrchik marked this conversation as resolved
@ -0,0 +22,4 @@
// APEChainManager interface provides methods to manage rule chains for containers in
// Policy contract.
type APEChainManager interface {
Owner

APEManager service is resource-agnostic, why have you decided to create a separate package for 1 out of 4 resource types?

It seems as simple as a single validation check which can be done via switch in the main service.

APEManager service is resource-agnostic, why have you decided to create a separate package for 1 out of 4 resource types? It seems as simple as a single validation check which can be done via `switch` in the main service.
Author
Member

I have simplified the service implementation - I have removed container package and moved this logic to the main service

I have simplified the service implementation - I have removed `container` package and moved this logic to the main service
@ -0,0 +167,4 @@
}
func getSignaturePublicKey(vh *session.RequestVerificationHeader) (*keys.PublicKey, error) {
for vh.GetOrigin() != nil {
Owner

Why do we need this loop?

Why do we need this loop?
Author
Member

Oh, my bad!

Oh, my bad!
Author
Member
I didn't notice that `GetOrigin` is being performed in the loop. But, I copied that from here: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/container/ape.go#L421 This loop can be found in these places: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/object/acl/v2/util.go#L63 https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/object/acl/eacl/v2/xheader.go#L33 Probably, that makes sense?
@ -0,0 +7,4 @@
)
// Bytes generate random bytes.
func Bytes(n int) ([]byte, error) {
Owner

It is unused, why it is public?

It is unused, why it is public?
Author
Member

Have removed

Have removed
fyrchik marked this conversation as resolved
@ -0,0 +17,4 @@
}
// Base58Str generate random base58 string.
func Base58Str(n int) (string, error) {
Owner

It is used exactly once, do we need a separate package for it?

It is used exactly once, do we need a separate package for it?
Author
Member

Alright. I supposed that could be useful, but it is really used in one place. So, I have removed it

Alright. I supposed that could be useful, but it is really used in one place. So, I have removed it
fyrchik marked this conversation as resolved
aarifullin requested review from dkirillov 2024-05-15 14:13:33 +00:00
aarifullin force-pushed feat/ape_manager from bd92279e75 to 6a7d5a4de9 2024-05-15 19:48:32 +00:00 Compare
aarifullin force-pushed feat/ape_manager from 6a7d5a4de9 to 6de48cdf65 2024-05-15 20:00:06 +00:00 Compare
aarifullin force-pushed feat/ape_manager from 6de48cdf65 to 647697a20e 2024-05-15 20:03:36 +00:00 Compare
aarifullin force-pushed feat/ape_manager from 647697a20e to 9fdad39811 2024-05-15 20:08:44 +00:00 Compare
aarifullin force-pushed feat/ape_manager from 9fdad39811 to e6d3b9b65d 2024-05-15 20:12:08 +00:00 Compare
fyrchik approved these changes 2024-05-16 08:07:27 +00:00
@ -0,0 +8,4 @@
// APE Manager manages chains for native protocol targets like containers, but,
// as a service, it is also required to be checked with APE.
type APEService struct {
Owner

Now this wrapper seems completely useless, what about removing it?
We will properly introduce it when the time comes.

Now this wrapper seems completely useless, what about removing it? We will properly introduce it when the time comes.
Author
Member

We will properly introduce it when the time comes.

Alright. Removed

> We will properly introduce it when the time comes. Alright. Removed
aarifullin force-pushed feat/ape_manager from e6d3b9b65d to 61cc4b48d4 2024-05-16 15:02:06 +00:00 Compare
aarifullin force-pushed feat/ape_manager from 61cc4b48d4 to 54bc1ee58a 2024-05-20 14:10:38 +00:00 Compare
aarifullin requested review from fyrchik 2024-05-20 14:12:08 +00:00
dkirillov approved these changes 2024-05-20 14:42:19 +00:00
fyrchik approved these changes 2024-05-24 08:41:34 +00:00
acid-ant approved these changes 2024-05-27 07:24:52 +00:00
fyrchik merged commit 2b02f52cd9 into master 2024-05-27 09:34:25 +00:00
fyrchik referenced this pull request from a commit 2024-05-27 09:34:33 +00:00
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-core-developers
No milestone
No project
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#1105
No description provided.