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

Introducing vault sharing and the restrictions of pulling and cloning with vault permissions #266

Merged
merged 10 commits into from
Mar 9, 2022

Conversation

CMCDragonkai
Copy link
Member

@CMCDragonkai CMCDragonkai commented Oct 25, 2021

Description

This pull request aims to allow vaults to be shared and unshared with other Node Ids. Specification and discussion can be found inside the Tasklist issues.

Secret Commands

There are some changes to secrets commands here. These should be extracted out of this PR and moved to another PR as they have different specification requirements.

Issues Fixed

Tasks

Reference for residual TODOs is here #266 (comment) .

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@CMCDragonkai
Copy link
Member Author

This will require squashing before rebase on master.

@scottmmorris
Copy link
Contributor

I'm going to remove the src/vaults/old folder in this MR along with the GitRequest stuff

@scottmmorris
Copy link
Contributor

I'm also going to handle the scan vaults function here. To see more info on this see: #259 (comment)

@scottmmorris
Copy link
Contributor

vault scan has been reintroduced and some testing has been done but there still needs to be fixes for the CLI and then the testing of the CLI commands.

After discussing with Roger we now have a solution to handle the errors coming through isomorphic git. More details can be found in #259. So I've updated the task list with the relevant tasks.

After those are done this PR should be ready for review. Probably wont finish it this week because ILL only be doing a half day Friday but should be ready by end of Monday next week.

@scottmmorris
Copy link
Contributor

I'm getting a connection warning when testing the scan vaults function on node manager:

WARN:Connection 127.0.0.1:41241:Connection Error: Error: This socket has been ended by the other party
WARN:Connection 0.0.0.0:56613:Connection Error: Error [ERR_STREAM_WRITE_AFTER_END]: write after end

I haven't been able to trace it yet but it doesn't seem to be affecting the output from the grpc stream as the tests are still passing. I believe the first error message shouldn't have the '127.0.0.1' address, it should be the default '0.0.0.0' so maybe this is contributing to the issue.

@scottmmorris
Copy link
Contributor

scottmmorris commented Oct 29, 2021

@CMCDragonkai

I'm struggling a little with the error handling. At the moment this is what I'm doing. On the server side I am generating and throwing the error and returning.

genWritable.throw(new vaultsErrors.ErrorVaultPermissionDenied);
return;

Then on the client side inside the request object I catch the error from the stream (there is a bit of a hack I'm using to get the what the error is because its coming out weird), set the error flag and then throw the error again.

response.stream.on('error', (err) => {
  // set the error flag
  // throw the error
});

Then I catch the error in the git clone command and do some checks to verify and then throw the correct error.

      try {
        await git.clone({
          ...
        });
      } catch (err) {
        if (err instanceof git.Errors.SmartHttpError && error) {
          if (error === 'ErrorVaultPermissionDenied') throw new vaultsErrors.ErrorVaultPermissionDenied;
        }
        throw err;
      }

However, the issue is that its not executing the try catch wrapper for the git clone method until after my test is finished.

  ●  Cannot log after tests are done. Did you forget to wait for something async in your test?
    Attempted to log "ErrorVaultPermissionDenied: 
        at /home/scottmorris/Documents/js-polykey/src/vaults/VaultManager.ts:495:61
        at processTicksAndRejections (internal/process/task_queues.js:95:5)
        at Object._transaction (/home/scottmorris/Documents/js-polykey/src/vaults/VaultManager.ts:198:14)
        at Object.cloneVault (/home/scottmorris/Documents/js-polykey/src/vaults/VaultManager.ts:371:12)
        at Object.<anonymous> (/home/scottmorris/Documents/js-polykey/tests/vaults/VaultManager.test.ts:867:11) {
      description: 'ErrorVaultPermissionDenied: Polykey error',
      exitCode: 1,
      data: {}
    }".

      493 |         // isomorphic git then we need to throw the polykey error
      494 |         if (err instanceof git.Errors.SmartHttpError && error) {
    > 495 |           if (error === 'ErrorVaultPermissionDenied') throw new vaultsErrors.ErrorVaultPermissionDenied;
          |                                                             ^
      496 |         }
      497 |         throw err;
      498 |       }

It appears that the test shuts down after the stream error is caught and rethrown. So instead of failing with the exception in the try catch its failing within the request object and I assume it isn't being caught there?

EDIT: The alternative I have found is that if I don't throw an error to isomorphic git then it will continue until it realises its not being sent any data from the stream and then will throw and error which is correctly caught and handled. I'm not too sure why there is a difference between these two approaches? By my reckoning they should both cause the process to exit gracefully.

@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Oct 29, 2021 via email

@scottmmorris
Copy link
Contributor

Yep, good point. Although I've changed it and i'm still encountering the async error.

I tried stream.emit as well before as well and it didnt work and that isnt async.

@scottmmorris
Copy link
Contributor

I'm now running a try and catch wrapper like this:

              const response = client.vaultsGitInfoGet(request, meta);
              // The polykey error is caught and the error flag set
              try {
                for await (const resp of response) {
                  yield resp.getChunk_asU8();
                }
              } catch (err) {
                error = err;
                throw err;
              }

and it is working fine as opposed to using the response.stream.on('error').

@scottmmorris
Copy link
Contributor

The only thing is, that isomorphic git is logging out an error here: https://github.com/isomorphic-git/isomorphic-git/blob/89c0da78d5ebf3c9f2754b3c8d557155dd70c8d7/src/models/GitPktLine.js#L89

Which is this error

    error TypeError: Cannot read property 'length' of undefined
        at StreamReader.read (/home/scottmorris/Documents/js-polykey/node_modules/isomorphic-git/index.cjs:2299:39)
        at processTicksAndRejections (internal/process/task_queues.js:95:5)
        at read (/home/scottmorris/Documents/js-polykey/node_modules/isomorphic-git/index.cjs:6374:22)
        at parseRefsAdResponse (/home/scottmorris/Documents/js-polykey/node_modules/isomorphic-git/index.cjs:6425:17)
        at Function.discover (/home/scottmorris/Documents/js-polykey/node_modules/isomorphic-git/index.cjs:6625:28)
        at _fetch (/home/scottmorris/Documents/js-polykey/node_modules/isomorphic-git/index.cjs:7257:22)
        at _clone (/home/scottmorris/Documents/js-polykey/node_modules/isomorphic-git/index.cjs:7634:42)
        at Object.clone (/home/scottmorris/Documents/js-polykey/node_modules/isomorphic-git/index.cjs:7757:12)
        at /home/scottmorris/Documents/js-polykey/src/vaults/VaultManager.ts:475:9
        at Object._transaction (/home/scottmorris/Documents/js-polykey/src/vaults/VaultManager.ts:198:14)

I think this is because when we throw an error the buffers collected from the generator are undefined

@CMCDragonkai
Copy link
Member Author

That's the correct way to handle errors when using the promise/generator utilities.

The only place we are using on event handling is for metadata, and soon that should be turned into a promise too.

@scottmmorris
Copy link
Contributor

scottmmorris commented Nov 1, 2021

The only thing is, that isomorphic git is logging out an error here: https://github.com/isomorphic-git/isomorphic-git/blob/89c0da78d5ebf3c9f2754b3c8d557155dd70c8d7/src/models/GitPktLine.js#L89

Which is this error

    error TypeError: Cannot read property 'length' of undefined
        at StreamReader.read (/home/scottmorris/Documents/js-polykey/node_modules/isomorphic-git/index.cjs:2299:39)
        at processTicksAndRejections (internal/process/task_queues.js:95:5)
        at read (/home/scottmorris/Documents/js-polykey/node_modules/isomorphic-git/index.cjs:6374:22)
        at parseRefsAdResponse (/home/scottmorris/Documents/js-polykey/node_modules/isomorphic-git/index.cjs:6425:17)
        at Function.discover (/home/scottmorris/Documents/js-polykey/node_modules/isomorphic-git/index.cjs:6625:28)
        at _fetch (/home/scottmorris/Documents/js-polykey/node_modules/isomorphic-git/index.cjs:7257:22)
        at _clone (/home/scottmorris/Documents/js-polykey/node_modules/isomorphic-git/index.cjs:7634:42)
        at Object.clone (/home/scottmorris/Documents/js-polykey/node_modules/isomorphic-git/index.cjs:7757:12)
        at /home/scottmorris/Documents/js-polykey/src/vaults/VaultManager.ts:475:9
        at Object._transaction (/home/scottmorris/Documents/js-polykey/src/vaults/VaultManager.ts:198:14)

I think this is because when we throw an error the buffers collected from the generator are undefined

Maybe this is because they use a stream reader, and this error occurs essentially when initialising the stream reader because there are no buffers sent by the generator. So then it doesn't have a length and gives this error. So isomorphic git isn't actually reacting to the error we are throwing in the request object. I tried sending some buffers before throwing but the error still occurs.

I then moved onto this structure:

              try {
                for await (const resp of response) {
                  yield resp.getChunk_asU8();
                }
              } catch (err) {
                error = err;
                throw err;
              } finally {
                return {
                  url: url,
                  method: method,
                  body: [],
                  headers: headers,
                  statusCode: 2,
                  statusMessage: 'NOT OK',
                };
              }

so that isomorhic git knows ahead of time that there is an error. This fixes the logging of the type error. I'm not sure if there is a convention I should be using for the status code and message here.

E: actually this doesn't fix it, for some reason it doesn't actually receive the return object in the finally statement

@CMCDragonkai
Copy link
Member Author

I think you're translating GRPC exceptions/errors straight into HTTP errors there. This is definitely doable. HTTP has both error status codes as as well as status messages. https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#4xx_client_errors

The question is what does isogit do with this error information? Does it even throw it or it's just dropped. I think the snippet you showed before just dropped the error. In which case these codes aren't even really read.

Note that our exceptions do have a exitCode and description, that could be used if you want some default values to be returned. See src/errors.ts ErrorPolykey.

@scottmmorris
Copy link
Contributor

scottmmorris commented Nov 1, 2021

They do check in some places, e.g. here https://github.com/isomorphic-git/isomorphic-git/blob/89c0da78d5ebf3c9f2754b3c8d557155dd70c8d7/src/managers/GitRemoteHTTP.js#L131

However, when I log out the response on the isomorphic git side, it always comes out with the status 200.

Anyway, going back to the original issue which was the iso git error being logged out, the reason its logging out is because the thrown error resulted in no buffers being collected as I mentioned in the previous post. And I think the pk error is only thrown when the buffers are read in from the generator. So in the finally statement I just sent an empty buffer which meant that the length error wasn't thrown but the pk error was.

E: The return statement in the finally block overwrites the error thrown to iso-git but instead iso-git throws another error (emptyServerResponse). Because iso-git generalises errors here to SmartHttpErrors it doesn't really matter what the error is does it? Also eslint complains about the return in the finally block but I have disabled for now. Maybe in the future if what I'm doing is ok then we can remove the throw in the catch statement and just return an empty response instead.

@scottmmorris
Copy link
Contributor

Update here: The rebase has caused quite a few test failures. The ones that I'm still struggling with are the agent ones. This is probably due to the fact that fresh wasn't really being used to start the polykey agent but required when starting the vault manager due to the vault key now being stored there

@CMCDragonkai
Copy link
Member Author

The API change mostly affected src/proto/schemas. All other changes elsewhere should have been just import path changes and renaming of the protobuf types being used. Although this is subject more change from: #249 (comment)

So it shouldn't be causing new test failures. I did run most of the tests prior to merging including the vaults domain. So not sure why fresh should be involved here.

@scottmmorris
Copy link
Contributor

scottmmorris commented Nov 4, 2021

I think its because of the change to the vault key being stored inside vault manager now (So not the rebase changes). I was doing it so that if fresh was specified in the vault manager it would create the key but otherwise it should read the key. But in the keys domain when it was setup it would check if the key exists and then if it didnt it would create it, regardless of whether fresh was specified or not (hence why there were no issues before).

Now that Ive correctly updated it everywhere it should be working. Also there wasn't an option to start the polykey agent from scratch (fresh)? I'm not sure if this is intentional and if then I should just do the vault key the way it was done before. i.e. not rely on the fresh option and instead just check if the key exists and if not create it. I feel like this would run into a lot of problems though.

@CMCDragonkai
Copy link
Member Author

@scottmmorris after you finish testing this. Can you go to MatrixAI/Polykey-Docs#3 and update the vault architecture diagrams and the sharing/unsharing sequence diagrams in relation the work done here. Just make a comment there regarding updates to the diagrams, and then coordinate with @joshuakarp in terms of fixing up the wiki.

src/agent/utils.ts Outdated Show resolved Hide resolved
src/bin/agent/start.ts Outdated Show resolved Hide resolved
@scottmmorris scottmmorris force-pushed the vaultspermissions branch 2 times, most recently from 8872ffd to 6d9c3cc Compare November 11, 2021 01:30
- NodeId fixes for vaults tests
- Replaced makeVaultIdPretty` with `encodeVaultId`
@tegefaulkes
Copy link
Contributor

I have re-based on master. I discovered the interactive re-base feature so I cleaned up the commit history into single commits for each issue as well.

@CMCDragonkai
Copy link
Member Author

@tegefaulkes great rebasing however please write descriptive commit messages. You also don't need to reference the issue if it's part of a MR/PR that is committed together.

@tegefaulkes
Copy link
Contributor

tegefaulkes commented Feb 28, 2022

I'll add better descriptions but I'm going to keep the issue numbers in the commits. This is just so I can track what each change relates to better and when it comes to the last stage rebase-merge step I can squash each issue together nicely. anything labelled fixes for #X are non-specific fixes that will be squashed at some point.

@tegefaulkes
Copy link
Contributor

MatrixAI/Polykey-CLI#80 will likely be moved outside of the PR into it's own PR. The changes are not required here and don't fix anything. So to keep things neat it will be seperate.

@tegefaulkes
Copy link
Contributor

On review the remaining issues It looks like the rest of the implementation should take 2-3 days. fixing and updating tests should be 2-3 days aswell. Review will possibly add a day or two on top. I'm expecting about to be done halfway through next week, Possibly the end of next week.

@tegefaulkes
Copy link
Contributor

All tests are passing now. all that is left is a small review of #336 and an overall review of the code before squashing and merging.

@tegefaulkes
Copy link
Contributor

Only one test failing in pipeline. It's working in my tests so i'll take a closer look.

tegefaulkes and others added 9 commits March 9, 2022 19:11
- `VaultId` usage,
- updated validation
- removal of `GenericIdTypes.ts`
- selective imports
- error descriptions updated
- Using google `Timestamp` message in `LogEntry` message
- fixing comments
…ltsPermissionGet` and `vaultsPermissionUnset`.
- prototyping committing history
- prototyping committing, EOD
- Linear and branching committing implemented
- recovery from dirty state and expanding tests
- added global garbage collection for git objects.
@tegefaulkes tegefaulkes marked this pull request as ready for review March 9, 2022 09:10
@tegefaulkes tegefaulkes merged commit 9983ae7 into master Mar 9, 2022
@tegefaulkes tegefaulkes deleted the vaultspermissions branch March 9, 2022 09:12
@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Mar 10, 2022

A few things I found that seems to be missing from this PR:

  1. The GRPC tests for vaults have not yet been split like the other GRPC tests. See tests/client/rpcVaults.test.ts. These should go into the tests/service calls. - Split Secrets and Vault Command Tests into Separate Files Polykey-CLI#80
  2. The bin CLI tests for vaults have not been split, see tests/bin/vaults/vaults.test.ts. These should be split into their individual commands like the other tests. - Split Secrets and Vault Command Tests into Separate Files Polykey-CLI#80
  3. The usage of jest.mock is @/keys/utils should be replaced with the usage of the setupGlobalKeypair utility.
  4. Vault ops and secrets-related commands review will be a large epic before we can provide general usage.

@tegefaulkes can you make sure that 1 - 3 have issues tracking them? @emmacasolin can deal with the test splitting soon.

@tegefaulkes
Copy link
Contributor

Note that for 3, that some of the tests require different NodeIds to work. This is because we are trying pull, clone or verify permissions in some tests.

As for 4, can you elaborate on the scope and end goal of the issue?

@CMCDragonkai
Copy link
Member Author

Note that for 3, that some of the tests require different NodeIds to work. This is because we are trying pull, clone or verify permissions in some tests.

As for 4, can you elaborate on the scope and end goal of the issue?

  1. Ok in that case it's just a matter of create new agents with different keys. It depends on the test abstraction level. Can be rolled into solving 1. and 2.

  2. This requires a overall review of the commands, there are some existing issues in this area but I haven't had a deep look yet. Can you link them into 1 epic? I think there were some things about matching the Unix command API.

@CMCDragonkai
Copy link
Member Author

I think MatrixAI/Polykey-CLI#32 can be expanded/elaborated to incorporate point 4. similar to MatrixAI/Polykey-CLI#6.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment