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

Vault Scanning #339

Closed
6 tasks done
tegefaulkes opened this issue Feb 15, 2022 · 10 comments · Fixed by #266
Closed
6 tasks done

Vault Scanning #339

tegefaulkes opened this issue Feb 15, 2022 · 10 comments · Fixed by #266
Assignees
Labels
development Standard development r&d:polykey:core activity 1 Secret Vault Sharing and Secret History Management

Comments

@tegefaulkes
Copy link
Contributor

tegefaulkes commented Feb 15, 2022

Specification

Scanning vaults is both a client service handler and agent service handler, but they do different things.

Scanning should be implemented in the VaultManager and not the NodeManager.

The only thing is that the agent service handler must have a scan method.

  • client/service/vaultsScan.ts - performs a scan on another node, triggered by pk vaults scan which is bin/vaults/CommandScan.ts
  • agent/service/vaultsScan.ts - allows another node to scan what vaults they are allowed to clone or pull, should provide a structured data structure that indicates each vault id, vault name and the vault permissions set

We need to know how to separate the logic between:

client/service/vaultsScan.ts
agent/service/vaultsScan.ts
bin/vaults/CommandScan.ts
vaults/VaultManager.scanVaults

For example, what does VaultManager.scanVaults do? Is a "client-side" logic or "server-side" logic? The answer to this question is neither. If we pursue the pattern of composition, then we instead lower level plumbing methods should be exposed by VaultManager that allows it to answer the calls of agent/service/vaultsScan.ts. The calls of client/service/vaultsScan.ts would be different instead calling the other node's agent service.

At the very least NodeManager.scanNodeVaults should not exist, because it's not the job of NodeManager.

VaultManager must provide two functions. scanVaults and handleScanVaults. scanVaults makes the GRPC call to a remote agent to get the list of vaults. This is called by the clientService scanVaults handler to provide this data to the client. handleScanVaults is called by the agentService handler to obtain the data in the first place.

The structure follows this diagram:




  ┌────────┐ client service ┌───────┐  agent service ┌───────┐
  │ Client ├────────────────► Agent ├────────────────► Agent │
  └────────┘   vaultsScan   └──┬─▲──┘   vaultsScan   └───┬───┘
                               │ │                       │
                               │ │                       │
                         ┌─────▼─┴──────┐        ┌───────▼────────┐
                         │ VaultManager │        │  VaultManager  │
                         │  scanVaults  │        │handleScanVaults│
                         └──────────────┘        └────────────────┘


The nodeId we are checking should be gotten from the connection info and not the message.

changes made

  • vaultsScan logic was moved into the vaultManager. vaultsScan was updated to be a generator to properly stream the information and handleVaultScan was created to hold the logic that collects the expected information.
  • The NodeId was removed from the agent vaultScan request message since this is retrived via the connection info now. Agent service utility to acquire trusted ConnectionInfo from connecting client node #342.
  • Logic was updated to only return a list of vaults that the requester has permissions on. we also return what permissions each vault has now.
  • The bin command was updated with the new information, minor change.

Additional context

Tasks

  • 1. scan vaults functionality needs to be moved into the vaults domain.
  • 2. data returned should be structured as a (VaultsId, VaultName, vaultPermission[])
  • 3. Add VaultManager.handleScanVaults to collect the expected data to be used by the agent service handler.
  • 4. Update vaultManager.scanVaults to be a generator to allow for streaming.
  • 6. NodeId to scan against should be obtained via connection info and not the message.
  • 5. Review and update tests.
@tegefaulkes
Copy link
Contributor Author

tegefaulkes commented Feb 15, 2022

I will need to review the code to spec this out further.

@tegefaulkes
Copy link
Contributor Author

I'm going to change up the logic here a little bit.

We have this chain that handle this information. bin/vaults/CommandScan.ts -> client/service/vaultsScan.ts -> vaults/VaultManager.scanVaults -> agent/service/vaultsScan.ts -> vaults/VaultManager. It seems odd that the VaultManager.scanVaults` handles the intermediate step but it also doesn't handle the streaming properly.

I'm going to make client/service/vaultsScan.ts handle the collection and formatting of the needed data and just have client/service/vaultsScan.ts directly stream the results from agent/service/vaultsScan.ts.

@CMCDragonkai
Copy link
Member

Because we originally designed the domains (classes) to be standalone objects that can provide all the methods necessary to do work, the domain classes themselves therefore expose methods that ultimately call other nodes, and that's why we inject NodeConnectionManager into various classes.

So the architecture of vault scanning is like this:

┌────────┐ client service ┌───────┐  agent service ┌───────┐
│ Client ├────────────────┤ Agent ├────────────────► Agent │
└────────┘   vaultsScan   └──┬─▲──┘   vaultsScan   └───────┘
                             │ │
                             │ │
                       ┌─────▼─┴──────┐
                       │ VaultManager │
                       │  scanVaults  │
                       └──────────────┘

There are 2 vaultsScan calls. One on the client service, one on the agent service and they do different things.

The client service vaultsScan just calls the VaultManager.scanVaults.

The agent service vaultsScan does the actual work gathering data from the VaultManager using other methods.

This is similar to pullVaults and cloneVaults as well.

A consequence of this design is that ACL checks have to be done on the agent service handler, but not inside the VaultManager.

@CMCDragonkai
Copy link
Member

As an example when ACL is injected into VaultManager, it's used only by methods that mutate the permissions of the ACL.

However on the agent service handler for vaultsScan, it will need to directly contact the ACL to ask for that permission, as there's no handleVaultsScan on the VaultManager call.

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Feb 22, 2022

Thinking about this a bit more, I think there is room for handleX methods in our domain classes when there's an equivalent X method that deals with other nodes. For example:

VaultManager
  scanVaults
  handleScanVaults
  pullVault
  handlePullVault
  cloneVault
  handleCloneVault

NodeManager
  getClosestNodes
  handleGetClosestNodes

Notifications
  send
  receive (maybe handleSend to be consistent)

We wouldn't create handler methods for any method that is only targeting itself. So for example in ACL, the getNodePerms is only about the local permissions, not about other node's permissions. So there's no handleGetNodePerms.

@emmacasolin

@CMCDragonkai
Copy link
Member

If we proceed with that design, we should write that into our wiki for our domain class design.

@tegefaulkes
Copy link
Contributor Author

We have another case of needing to get the NodeId from the connection info. To finish this we are blocked by #342 .

@tegefaulkes
Copy link
Contributor Author

tegefaulkes commented Feb 22, 2022

I'm going to need to review the agent and client service tests to make sure all of the GRPC for the vaults and secrets domains are tested properly. I think I'll make a new issue for this. @tegefaulkes

For the case of vaultsScan it seems like an excessive amount of testing though. each stage needs a test.

  • bin command
  • client scanVaults call
  • VaultManager.ScanVaults
  • agent scanVaults call
  • vaultManager.handleScanVaults

It seems like a bit of duplication of testing here since the only things we really need to test are the Bin command, VaultManager.scanVaults and VaultManager.handleScanVaults.

tegefaulkes added a commit that referenced this issue Feb 22, 2022
@emmacasolin
Copy link
Contributor

Thinking about this a bit more, I think there is room for handleX methods in our domain classes when there's an equivalent X method that deals with other nodes. For example:

VaultManager
  scanVaults
  handleScanVaults
  pullVault
  handlePullVault
  cloneVault
  handleCloneVault

NodeManager
  getClosestNodes
  handleGetClosestNodes

Notifications
  send
  receive (maybe handleSend to be consistent)

We wouldn't create handler methods for any method that is only targeting itself. So for example in ACL, the getNodePerms is only about the local permissions, not about other node's permissions. So there's no handleGetNodePerms.

@emmacasolin

So would the X method be equivalent to a request and the handleX method be equivalent to a response? e.g.

scanVaults // request to scan another node's vault
handleScanVaults // respond to the request (i.e. handle it) by returning the requested info

In this case I'm not sure if notifications' send and receive belongs in this grouping, since receive isn't returning any information to the sender. Communication is one-way in this case, rather than two-way like the other examples above.

@tegefaulkes
Copy link
Contributor Author

I think it's more about handling an action than handling a response, it doesn't matter how it responds. The thinking here is that when we have the agent-agent communication. The action is started by the NotificationManager.send and for symmetry it is handled by a NotificationManager.receive method. Just to keep things tidy we are keeping the logic for both ends in the same domain. For code consistency and convention anything working like this should be named X and handleX.

tegefaulkes added a commit that referenced this issue Feb 23, 2022
tegefaulkes added a commit that referenced this issue Feb 24, 2022
tegefaulkes added a commit that referenced this issue Feb 28, 2022
tegefaulkes added a commit that referenced this issue Feb 28, 2022
tegefaulkes added a commit that referenced this issue Mar 8, 2022
tegefaulkes added a commit that referenced this issue Mar 8, 2022
tegefaulkes added a commit that referenced this issue Mar 8, 2022
tegefaulkes added a commit that referenced this issue Mar 9, 2022
tegefaulkes added a commit that referenced this issue Mar 9, 2022
tegefaulkes added a commit that referenced this issue Mar 9, 2022
tegefaulkes added a commit that referenced this issue Mar 9, 2022
tegefaulkes added a commit that referenced this issue Mar 9, 2022
tegefaulkes added a commit that referenced this issue Mar 9, 2022
@teebirdy teebirdy added the r&d:polykey:core activity 1 Secret Vault Sharing and Secret History Management label Jul 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Standard development r&d:polykey:core activity 1 Secret Vault Sharing and Secret History Management
4 participants