Session Management Commands

[CMCDragonkai]

Specification

1. Session Management is the matter of acquiring an authenticated session for the PK CLI/GUI against the PK agent
2. It should work for multiple clients connecting to the agent
3. It should work over the network
4. It should be integrated into our GRPC client service
5. We should be able to create and destroy our own session
6. And we should be able to destroy all other client's session
7. Sessions don't represent separate users to the PK agent, they are currently just different uses of the same PK agent
8. We may be able to extend sessions to deal with OAuth2 and HTTP API Third Party Integration - HTTP API, OAuth2 Provider API, Plugin API, Library vs Framework, Smart Tokens #166
9. We should educate the user on using the CLI with one space ahead of the command to avoid saving it in the CLI somehow (how should this education be done), it needs to be mentioned in all our docs
10. Repeated runs of authenticated command should refresh the session expiry

Additional context

This is all done as part of client-refactoring for now.

Tasks

1. Understand that there should be 3 input methods: parameter, prompt with EOF CTRL+D, and file descriptor
2. Use all 3 input methods to allow root password to be used to authenticate a session
3. Specify and test pk agent unlock - for authenticating a session for the CLI
   • Allow all 3 input methods to be used here
4. Specify and test pk agent lock - for destroying the current session for the CLI (delete client's session token)
5. Specify and test pk agent lockall - for changing the session key and ensuring that all sessions should be made invalid
6. All other commands should expect to be authenticated, and if not authenticated, an exception is thrown from the agent, and the client must then acquire the password using the prompt method, and then retry the previous operation after the prompt works
7. Integrate lockfile mechanism to the session token to avoid clobbering during session refresh

[CMCDragonkai]

# parameter style
 pk agent unlock 'rootpassword'

# prompt style
pk agent unlock
> ...
> EOF

# file style
pk agent unlock -pf ./file

For all CLI commands, the exact options -pf should always have a long form like --password-file.

[CMCDragonkai]

When running pk agent unlock twice or more, it's idempotent.

pk agent unlock '...'
pk agent unlock '...'

Basically a new token is created.

[CMCDragonkai]

When unlocking the agent and creating a session, you should show the information about the session. In particular show that the session has an expiry date.

One question do you refresh the session on each command. So if you do:

 pk agent unlock '...'
pk vaults list
pk vaults list

Is each pk vault list refreshing the session? Ideally it should be, that way the more you use the client, you don't expire it inadvertently.

However if you are refreshing the session each time, you have you write the session file onto the disk.

Note that when you do this, you may clobber the file or be writing at the same time as some other command.

pk vault list &
pk vault list &
wait

The above will run the command twice at the same time. We would want to avoid refreshing the session file and clobbering each other.

One way to resolve this is to acquire a lock on the session file. This very similar to the lockfile being used by the agent to ensure that one node state has one agent instance.

You can have a procedure like:

• Lock the file
• If yes, proceed to write new session
• If you couldn't get the lock, assume that another PK client process is refreshing it, and don't bother doing i

You will also need to resolve how to lock the file if the file doesn't exist.

Perhaps you can use the same lockfile mechanism and abstract it for this usecase?

[CMCDragonkai]

Also @DrFacepalm please ensure you have a mutex in your SessionManager and wrap all your public API commands with await this._transaction(...).

You have to beware of concurrency here, all our code is running asynchronously!

[DrFacepalm]

https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/189 should fix everything except for making normal commands refresh the session, and dealing with the concurrency issues that come with that.

[CMCDragonkai]

Already merged, so this just requires review.

@tegefaulkes I think since you are going to be rebasing on the client-refactoring to make use of this for identities CLI, your comments are going to be relevant.

[CMCDragonkai]

@joshuakarp since you're starting on the Nodes CLI. You can check basically whether everything here is addressed, and if not, the remaining issue is #211 (automatic session refresh on commands and expiry information).

If so, then this issue can be closed. Otherwise subissues should be created.

[CMCDragonkai]

So for the bin CLI commands, they should require session authentication now.

This means for all the tests/bin tests, they should have a fixture like beforeAll or beforeEach, there needs to be an a session authentication with pk agent unlock, and then using that token for all subsequent CLI commands.

Instead of relying on a default file path for the session token, it should be able to be passed in directly via a command line parameter or an environment variable, or a path to a file containing the session information.

[CMCDragonkai]

@joshuakarp best done while you're working on the Nodes CLI: https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/198

@tegefaulkes might have to add some additional stuff to https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/201

[CMCDragonkai]

Some review notes for https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/189:

1. This MR brought in the prompts package
2. The tests/utils.test.ts has some unnecessary imports, should be fixed up later in a lintfix.
3. The tests/sessions/SessionManager.ts should be described with SessionManager.
4. It should be src/sessions not src/session. You are using sessions in other places to describe this domain. This should be standardised.
5. The bit size for session manager should be 4096 not 4069. The 4096 is the correct size.
6. The tests/client/clientService.test.ts constructs callCredentials manually. This should either be a function available inside src/client/utils.ts or if it is test-specific, it goes into tests/client/utils.ts. This construction looks complicated and should be abstracted.
7. Was the removal of the credentials property for openTestClientClient in tests/client/utils.ts necessary? How does this interact with the call credentials?
8. Do not use sigchain claim type in Session.ts the tests is currently using them. This is currently affecting all tests involving sessions and also the source code.
9. The Session class represents a client that constructs it (when receiving the underlying token from the client service), and also the SessionManager that constructs the object from the agent side (when the client wants to authenticate). It's just an abstraction, the tests for Session doesn't seem to accurately represent this abstraction.
10. I don't it's necessary to have the aliases for lock, lockall, and unlock commands, we can reduce the complexity.
11. We should abstract the call credential into our credential-specific type into src/client/types.ts instead of just using: let callCredentials: Partial<grpc.CallOptions>;.
12. All the bin tests are going to require some form of session-management token related tests. However I don't think this is necessary. For the bin tests I believe all session management should be done as just a fixture. It would having an n+1 problem which every command we need a session token specific tests. It should just be assumed to work. Instead you have tests/bin/agent.test.ts for session-token specific tests, and put them all there specifying the behaviour.
13. The test groups and test descriptions for tests/bin/agent.test.ts should be more descriptive and use our abstraction representations as keywords. Like "unlocking by constructing a session" or "locking by destroying the session" or "lockall by destroying all sessions through changing the session key".
14. What is the function generateUserToken for in src/utils.ts?
15. Are there tests for testing the checking of stateVersion? Right now we don't do anything different, but the stateVersion should be checked. That is we need to check if the state version is ahead or behind the current state version that's programmed for the Polykey. So src/config.ts means this is our current state version. There should be functions that check this against the lock file in our PK node state. Which is why Lockfile is sort of a strange name, it is more specific to our usecase, unless we abstract them into PolykeyAgent and PolykeyClient (and can use it for both session tokens and polykey node state). I'd call it the StateFile and document its generic use.
16. When constructing the session, we can either receive the token from the grpc client service during unlock, or we can read it from a file in the PK client state. Usually we would program the read from file to the async start method of Session. But I can see that would conflict from receiving the grpc client service. So if the Session is constructed with a passed in token, then it shouldn't have an async start or stop method at all, it's just a class wrapped POJO and should only have a constructor. In that sense it's only a slightly more advanced Opaque type with additional methods. Alternatively when you start the Session, if you pass in the token, then it just uses that, but if you don't it will perform the actual read itself.
17. The src/session/errors.ts errors can be more abstractly named. No need to mention JWT. That's an implementation detail.
18. Try to ensure there's only one right way, one right place to do anything that constructing call credentials.. etc.
19. The Client.proto should have a naming standardisation process applied. New issue for this.
20. I feel like the password checking functions in src/client/utils.ts should really be in src/session/utils.ts. But you can keep functions that transform from call credentials and back there... Furthermore the usage of throwing exceptions in the utility functions should be minimised, it deally utility functions are pure and return true/false or whatever, and the throwing of exceptions should be the domain classes.
21. Utility functions should not be injected with the domain class. Utility functions should be pure and easy to use. So utils.verifyToken call should be inverted.
22. We should preserve the naming of the actions across CLI commands and GRPC service commands as much as possible. We have lock, unlock and lockall. But the client service has sessionRequestJWT and sessionChangeKey. You can do something like sessionUnlock, sessionLock and sessionLockAll. The JWT is an implementation detail. The usage of password is an implementation detail. Functions/commands like sessionUnlock should just be requesting the password as a parameter.
23. The lifetime of the Session should be tied to the creation of the actual session file... but that's point 16 anyway.

[CMCDragonkai]

Changes addressing the above should go into client-refactoring.

[CMCDragonkai]

This will go into new issue.

[CMCDragonkai]

Closing this, review points going to #211.