Encapsulate WebSocketClient in PolykeyClient #582
Conversation
addievo
changed the title
|
Don't forget to update all code and test that depend on the |
|
Make 4. a separate PR, keep this PR to only PK client encapsulating WS client. |
|
Can you address all comments. |
|
Rebase, squash down to 1 commit, and test what you're doing here, fix all tests. |
|
Note in the future, we don't use reverts in feature branches. You're just supposed to move ahead, and eventually squash down. |
CMCDragonkai
changed the title
Only the
|
addievo
force-pushed
the
feature-websocket_client-encapsulation
branch
from
4079fdf to
2c0ca01
Compare
|
Needs rebase. |
|
|
|
Don't resolve unless you've pushed. |
addievo
force-pushed
the
feature-websocket_client-encapsulation
branch
from
2c0ca01 to
56c3139
Compare
|
I was going to look into this but it's actually pretty relevant to this PR. The There needs to be some tests added to test the custom verification logic for certain cases.
|
|
This PR is not done.
|
tests/PolykeyClient.test.ts
Outdated
| tokenBase: config.paths.tokenBase, | ||
| rpc: { | ||
| callTimeoutTime: config.defaultsSystem.rpcCallTimeoutTime, | ||
| parserBufferSize: config.defaultsSystem.rpcParserBufferSize, | ||
| }, |
There was a problem hiding this comment.
Unless they need to be set for the test to work, don't bother setting them. Less noise for the test.
|
Taking over this PR. I'm going to standardise the option names between the agent and client. And it's important to know that domain options are not composed into the agent/client options. They are abstracted from each other. |
|
All options must be top-level to ensure it's easy to compare between |
|
The reason is like this.
|
|
I'm reverting: 0f81b34 |
|
Reverting my old All removed now except of course Oh yea and this is another reason to use |
|
I noticed that in The Now In QUIC: Is used on the server side. @amydevs how is this done in WS? |
|
@amydevs Ok so:
|
const webSocketClient = await WebSocketClient.createWebSocketClient(
{
host: host,
port: port,
config: {
verifyPeer: true,
keepAliveTimeoutTime: optionsDefaulted.keepAliveTimeoutTime,
keepAliveIntervalTime: optionsDefaulted.keepAliveIntervalTime,
},
logger: logger.getChild(WebSocketClient.name),
},
{
// this makes sense
timer: optionsDefaulted.connectTimeoutTime,
},
);
const rpcClient = new RPCClient({
manifest: clientClientManifest,
streamFactory: () => webSocketClient.connection.newStream(),
middlewareFactory: rpcMiddleware.defaultClientMiddlewareWrapper(
clientMiddleware.middlewareClient(session),
optionsDefaulted.rpcParserBufferSize,
),
toError: networkUtils.toError,
// this should be renamed
streamKeepAliveTimeoutTime: optionsDefaulted.rpcCallTimeoutTime,
logger: logger.getChild(RPCClient.name),
}); |
|
The ops limits numbers are: So actually instead of loading |
|
Actually we can just the strings: Can derive them from the password option keys. |
|
Ok time to fix up the tests. In one of the tests Here it is written: This says it is verifying the server certificate of the client service. I'd expect that this would be done automatically, since the Right now because the So shouldn't this |
|
The I've also recovered the |
|
The One thing I noticed was that it is possible to establish a connection to an agent without application-level authentication, because authentication occurs at the RPC level. This is similar to being able to establish a TCP connection without authentication. Which is sort of what is happening here in this case. It's interesting that agent to agent connections P2P wise, this is not possible, we do mutual TLS, thus ensuring the connection is authenticated from the beginning. But for the client service, we cannot use MTLS because clients may have access to the root certificate of the agent. Thus they are using the root password or recovery code or a session token in their RPC requests. I'm just wondering if we can move down this auth to the connection layer itself, to avoid malicious connections just hanging on to the client service. One quick way to deal with this is with a timeout on the PK client that has no RPC request activity, but this can be complicated because who knows, maybe I do want a long-running PK client connection. If the auth were to be pushed down to the connection, this could work since websocket connections are just HTTP 1.1 connections atm. However transport layer wise, we don't have a defined interface for such an interaction. It would fit into the HTTP 1.1 basic authentication since you could set a basic token as part of the initial HTTP request. @amydevs can you track this to create an issue. |
* only `PolykeyAgentOptions` and `PolykeyClientOptions` should be used, all other subdomains should use flattened options * updated all uses of subdomains to stop using `options` * `PolykeyClient` encapsulates certificate verification behaviour * `PolykeyClient` creation is now timed cancellable * new utilities to deal with `NodeId` - `isNodeId`, `assertNodeId`, `generateNodeId`
CMCDragonkai
force-pushed
the
feature-websocket_client-encapsulation
branch
from
abaeb82 to
e57c36e
Compare
|
@addievo make sure to review this PR, so you can see the real scope of this particular issue, it was definitely more complex than originally thought. Because this is a the root of the entire project, you now have to consider global integrity of the entire application, which means you have to consider all sorts of trade offs here. So this is a good learning exercise. |
| @@ -122,11 +240,14 @@ class PolykeyClient { | |||
| this.logger.info(`Stopped ${this.constructor.name}`); | |||
| } | |||
|
|
|||
| public async destroy() { | |||
| public async destroy({ force = false }: { force?: boolean }) { | |||
There was a problem hiding this comment.
I decided to make this default to false. I don't actually think force should be true by default. There is a special case in our transports that does this, but at an application level we should know why we are forcing it.
There was a problem hiding this comment.
@tegefaulkes @amydevs @addievo Still need to consider what happens for PolykeyAgent.
i feel like verifyCallback should be changed to also pass through the entire request if HTTP headers are going to be used for auth rather than MTLS @CMCDragonkai |
|
So theres a prob with CommandStop and CommandStatus, if status is "NULL", then nodeID/Host/Port can be undefined, but PKC expects them regardless, causing issues. What do I do here? async function processClientStatus(
nodePath: string,
nodeId?: NodeId,
clientHost?: string,
clientPort?: number,
fs = require('fs'),
logger = new Logger(processClientStatus.name),
): Promise<
| {
statusInfo: StatusStarting | StatusStopping | StatusDead;
status: Status;
nodeId: NodeId | undefined;
clientHost: string | undefined;
clientPort: number | undefined;
}
| {
statusInfo: StatusLive;
status: Status;
nodeId: NodeId;
clientHost: string;
clientPort: number;
}
| {
statusInfo: undefined;
status: undefined;
nodeId: NodeId;
clientHost: string;
clientPort: number;
}
> {
if (nodeId != null && clientHost != null && clientPort != null) {
return {
statusInfo: undefined,
status: undefined,
nodeId,
clientHost,
clientPort,
};
} else if (nodeId == null && clientHost == null && clientPort == null) {
const statusPath = path.join(nodePath, config.paths.statusBase);
const statusLockPath = path.join(nodePath, config.paths.statusLockBase);
const status = new Status({
statusPath,
statusLockPath,
fs,
logger: logger.getChild(Status.name),
});
const statusInfo = await status.readStatus();
if (statusInfo == null) {
return {
statusInfo: { status: 'DEAD', data: {} },
status,
nodeId: undefined,
clientHost: undefined,
clientPort: undefined,
};
} else if (statusInfo.status === 'LIVE') {
nodeId = statusInfo.data.nodeId;
clientHost = statusInfo.data.clientHost;
clientPort = statusInfo.data.clientPort;
return {
statusInfo,
status,
nodeId,
clientHost,
clientPort,
};
} else {
return {
statusInfo,
status,
nodeId: undefined,
clientHost: undefined,
clientPort: undefined,
};
}
} else {
const errorMsg = arrayZip(
[nodeId, clientHost, clientPort],
[
'missing node ID, provide it with --node-id or PK_NODE_ID',
'missing client host, provide it with --client-host or PK_CLIENT_HOST',
'missing client port, provide it with --client-port or PK_CLIENT_PORT',
],
)
.flatMap(([option, msg]) => {
if (option == null) {
return [msg];
} else {
return [];
}
})
.join('; ');
throw new errors.ErrorPolykeyCLIClientOptions(errorMsg);
}
}Problem domain In CommandStop try {
pkClient = await PolykeyClient.createPolykeyClient({
nodeId: clientStatus.nodeId,
host: clientStatus.clientHost,
port: clientStatus.clientPort,
options: {
nodePath: options.nodePath,
},
logger: this.logger.getChild(PolykeyClient.name),
});x | undefined is not assignable to x |
Description
This PR aims to encapsulate WebSocketClient in PolykeyClient.
At the same time
Optionsusage has to be limited to onlyPolykeyAgentandPolykeyClient.Because:
options. Why? Because you can just doX.createX({ param1, param2 });, no need to doX.createX({ options: { param1, param2 });as theoptionsis superfluous.PolykeyAgentandPolykeyClienthaveoptions? The reason is simply because there is so many parameters, that it is useful to have a separate type to keep track of them all. But this is exceptional toPolykeyAgentandPolykeyClient, it should not be considered the default way of creating classes in our codebase. And this typePolykeyAgentOptionsandPolykeyClientOptionswill be used in various places, in particular tests that need to run many different variants of the Polykey program.PolykeyAgentandPolykeyClient. We want to retain some flexibility here.Thus we will design which will be put into
src/types.ts:But all other domains will not do such a thing.
Tasks
PolykeyClientOptionswithPolykeyAgentOptionsoptionswhich is unique only toPolykeyClientandPolykeyAgent.WebSocketClientintoPolykeyClientand brought backnodeIdas a necessary parameter, verification of the client service certificate is now done withinPolykeyClient.PolykeyClienta@timedCancellableoperation with the timeout defaulted toconfig.defaultsSystem.clientConnectTimeoutTime. This propagates down to theawait WebSocketClient.createWebSocketClient.PolykeyAgentandPolykeyClientand subdomains which was relying uponoptions, these call signatures are all changed.PolykeyClient.Final checklist