Implementing TLS for ClientService

[tegefaulkes]

Specification

CLI GRPC Client should be using direct TLS to the GRPC Client Service.

Additional context

reference this comment: https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/207/diffs#note_651473935.

An MR has been created for this issue.
https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/210

Tasks

1. enable TLS connection as default for the PolykeyClient.
2. split the PolykeyAgent GRPCServer into two servers, enable TLS for the server serving the client service. unsecured for server serving the AgentService.
3. write a test and confirm that the client traffic is over TLS now.
4. confirm that the agent-agent traffic still functions.

[CMCDragonkai]

• CLI communication goes directly to the GRPC server, not through the forward and reverse proxies. This avoids creating unnecessarily expensive proxy objects just to perform a single CLI call.
• This means client service should only be exposed over TLS.
• There should be stub code or pre-existing code that allows the GRPC server to serve TLS.
• CLI and GUI can use TLS but not MTLS. That is client side shouldn't need to provide certificates... they can just accept and validate the server certificate. See the certification verification utility function inside network/utils.ts.
• Server side validates by the usage of the session token.

@tegefaulkes please spend time to clarify the task list (and/or prototyping) before starting on it. Similar to how #213 was addressed and the task list on the associated MR.

[tegefaulkes]

After digging through the code, I think TLS is already supported by GRPCServer, it just needs to be passed a TLSConfig when starting. so we should be able to enable TLS by having PolykeyAgent start the GRPCServer with a TLSConfig at src/PolykeyAgent.ts:324

await this.grpcServer.start({
      host: this.grpcHost as Host,
      port: this.grpcPort as Port,
    });

This may enable TLS for both the client service and the agent service though.
If we just want TLS for the client connections then some small changes to GRPCServer are needed.

[CMCDragonkai]

Yep I left that code there precisely for this possibility.

…

On 8/24/21 4:01 PM, Brian Botha wrote: After digging through the code, I think TLS is already supported by |GRPCServer|, it just needs to be passed a TLSConfig when starting. so we /*should*/ be able to enable TLS by having |PolykeyAgent| start the |GRPCServer| with a |TLSConfig| at |src/PolykeyAgent.ts:304| await this.grpcServer.start({ host:this.grpcHost as Host, port:this.grpcPort as Port, }); — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#229 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE4OHOZEO4BOHPS4TQOAMTT6MYVJANCNFSM5CLFCFFQ>.

[tegefaulkes]

I have gotten a test case to work with TLS doing an echo command as a test. The client doesn't have to provide any certs for this to work but the code is written to expect them so some modifications need to be made. I've pushed an example of this to https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/210

[CMCDragonkai]

That's right the client shouldn't need to provide a TLS credentials. It only has to verify that who it's connecting to, when it presents the certificate, that the certificate is valid for the expected Node ID which itself is acquired from the node path, or potentially manually specified.

…

On 8/24/21 6:35 PM, Brian Botha wrote: I have gotten a test case to work with TLS doing an echo command as a test. The client doesn't have to provide any certs for this to work but the code is written to expect them so some modifications need to be made. I've pushed an example of this to https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/210 <https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/210> — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#229 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE4OHKDVVYZUO7XJWMR7HLT6NKUVANCNFSM5CLFCFFQ>.

[tegefaulkes]

Looks like the TLS credentials is for the whole server. I don't think I can enable TLS for just the clientService and not the AgentService unless we run two seperate servers for them.

[CMCDragonkai]

Try and see if you can call bindAsync twice. Once for unsecured port, another for a secure port? But if not then I think having 2 server objects can work as well. One to serve the client server another to serve the agent server. That way 2 different ports. You'll need to update the ports being shown on the session lockfile or whatever is communicating to the CLI what port to use to connect to.

On 25 August 2021 11:42:59 am AEST, Brian Botha ***@***.***> wrote: Looks like the TLS credentials is for the whole server. I don't think I can enable TLS for just the clientService and not the AgentService unless we run two seperate servers for them. -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #229 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[tegefaulkes]

I will create separate servers for client and agent services.
I have updated the task list.

[tegefaulkes]

Re-based on client-refactoring.
So far I've split the ClientService and AgentService into two servers, the clientService has TLS enabled while the agentservice does not. I've split host and port information in PolykeyAgent for each server.

what is left is to confirm that the client communication is over TLS and that the agent communication still works. and maybe anything else I didn't think of.

[tegefaulkes]

Just tested and confirmed that the clientService is working over TLS and the agentService is insecure.
I tested both of them with a simple echo test and checking if the client was secure.

The tests are located at tests/client/PolykeyClient.test.ts:142 Can connect and echo over TLS and tests/agent/GRPCClientAgent.test.ts:204 Can connect over insecure connection.

If no other issues pop up with this. this should be fine to close when MR !210 Post session-and-misc fixes is merged.

[CMCDragonkai]

You are catching those exceptions as part of your test expectations right?

…

On 8/30/21 2:07 PM, Brian Botha wrote: Just tested and confirmed that the |clientService| is working over TLS and the |agentService| is insecure. I tested both of them with a simple echo test and checking if the client was secure. The tests are located at |tests/client/PolykeyClient.test.ts:142| |Can connect and echo over TLS| and |tests/agent/GRPCClientAgent.test.ts:204| |Can connect over insecure connection.| If no other issues pop up with this. this should be fine to close when MR !210 |Post session-and-misc fixes| is merged. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#229 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE4OHPNAKCB3YBFX7K7UFTT7L7ZZANCNFSM5CLFCFFQ>.

[tegefaulkes]

Yes, both tests check if the echo command worked and if the connection was secure with expect(pkClient.grpcClient.secured).toBeTruthy()

[tegefaulkes]

I'm moving this over to QA since the implementation is done.

[CMCDragonkai]

Done now. But the session token stuff requires a wiki diagrams and us to work out the exact behaviour.