Skip to content
This repository was archived by the owner on Feb 8, 2024. It is now read-only.

Connect: Accommodate for making gRPC server creds in shared process#1220

Merged
ravicious merged 9 commits into
masterfrom
ravicious/refactor-grpc-certs
Oct 20, 2022
Merged

Connect: Accommodate for making gRPC server creds in shared process#1220
ravicious merged 9 commits into
masterfrom
ravicious/refactor-grpc-certs

Conversation

@ravicious
Copy link
Copy Markdown
Member

@ravicious ravicious commented Sep 28, 2022

Equivalent Teleport PR: gravitational/teleport#16782.

On Windows, we use gRPC over TCP with mTLS since Node.js doesn't support UDS on Windows. Each process creates its own keypair and saves the public key to a predetermined location.

Up until now, these were the client-server pairs we had to support:

Server Client
tsh daemon renderer
shared (Electron process) renderer

For tshd-initiated communication, the tshd process will need to create a client that will connect to a gRPC server operated by the renderer process of the Electron app.

grpcCredentials refactor

packages/teleterm/src/services/grpcCredentials used to look like this:

packages/teleterm/src/services/grpcCredentials/
├── clientCredentials.ts
├── helpers.ts
├── index.ts
├── makeCert
├── serverCredentials.ts
└── types.ts

Now it looks like this:

packages/teleterm/src/services/grpcCredentials/
├── credentials.ts
├── files.ts
├── index.ts
├── makeCert
└── types.ts

clientCredentials.ts and serverCredentials.ts contained logic that was specific to a concrete process (renderer for clientCredentials). I extracted generic functions from those files. The orchestration of credentials for each process happens now in that process' main file.

Instead of having a catch-all helpers.ts or utils.ts file, I split the generic functions into functions that operate primarily on files (files.ts) and functions that work with credentials (credentials.ts). IMHO it's very important to do that – having a utils.ts file is almost always a bad choice as it signals that we didn't spend enough time thinking how the contents of that file fit into the general structure of the project.

Some of the timeouts in preload.ts, the shared process and tshd are set
to 10s. If the timeout to resolve the network address is lower or equal
to that, those timeout won't have the chance kick in.

Hence setting the timeout to a slightly larger value.
Without this, the cert generated by makeCert was failing when loaded
in Go as the gRPC server cert. The error message was:

    x509: certificate relies on legacy Common Name field, use SANs instead

See:

* https://stackoverflow.com/a/31143907/742872
* Search for "subjectAltName" https://github.com/digitalbazaar/forge/blob/2bb97afb5058285ef09bcf1d04d6bd6b87cffd58/README.md
//
// `Renderer` and `Tshd` file names are also used on the tshd side.
export enum GrpcCertName {
Client = 'client.crt',
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we're going to have both a gRPC client and a gRPC server in the renderer process, the name "client" stopped being sufficient. I refactored this so that it closer reflects what we want to achieve with gRPC certs – each process owns its own key pair, keeps the private key in memory and the public key is saved to the file.

Comment on lines +18 to +21
const runtimeSettings = getRuntimeSettings();
initializeLogger(runtimeSettings);
initializeServer(runtimeSettings);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just moved from the bottom of the file to the top, before function declarations so that you can immediately see what's going to be executed when this file is loaded.

Comment on lines +75 to +83
{
name: 'subjectAltName',
altNames: [
{
type: 2, // DNS type
value: commonName,
},
],
},
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To my surprise, Go didn't complain when a cert without this was used as a client cert, but it was a problem when I tried to use it as a server cert for the client credentials.

@ravicious ravicious marked this pull request as ready for review September 28, 2022 11:10
@ravicious ravicious requested review from avatus and gzdunek September 28, 2022 11:10
Copy link
Copy Markdown
Contributor

@avatus avatus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it and I'm excited for what it leads to. Also, side note: what a succinct and informative PR.

Comment thread packages/teleterm/src/services/grpcCredentials/credentials.ts Outdated
}

const { certsDir } = runtimeSettings;
const [rendererKeyPair, tshdCert, sharedCert] = await Promise.all([
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if any of these promises fail?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createGrpcCredentials will throw, then after it getElectronGlobals will throw. getElectronGlobals gets exposed through contextBridge as window.electron:

contextBridge.exposeInMainWorld('electron', getElectronGlobals());

It's used in boot.tsx:

try {
const globals = await getElectronGlobals();

getElectronGlobals in boot.tsx calls window.electron underneath, so it will fail as well, triggering the catch branch:

} catch (e) {
logger.error('Failed to boot the React app', e);
renderApp(<FailedApp message={e.toString()} />);

This will show the error to the user in the actual browser window.

Co-authored-by: Michael <michael.myers@goteleport.com>
Copy link
Copy Markdown
Contributor

@gzdunek gzdunek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done @ravicious! I don't have any comments to the code, I have to try it out (I'll do it on Monday).

Copy link
Copy Markdown
Contributor

@gzdunek gzdunek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works nicely!

@ravicious ravicious merged commit ebb2109 into master Oct 20, 2022
@ravicious ravicious deleted the ravicious/refactor-grpc-certs branch October 20, 2022 10:39
@ravicious
Copy link
Copy Markdown
Member Author

I’m going to backport this once v11.0.0 is released.

ravicious added a commit that referenced this pull request Oct 26, 2022
…1220)

* Add comment to resolveNetworkAddress

* Log errors during boot of React app

* Connect: Accommodate for making gRPC server creds in shared process

* Adjust timeout for resolveNetworkAddress

Some of the timeouts in preload.ts, the shared process and tshd are set
to 10s. If the timeout to resolve the network address is lower or equal
to that, those timeout won't have the chance kick in.

Hence setting the timeout to a slightly larger value.

* makeCert: Add subjectAltName to certs

Without this, the cert generated by makeCert was failing when loaded
in Go as the gRPC server cert. The error message was:

    x509: certificate relies on legacy Common Name field, use SANs instead

See:

* https://stackoverflow.com/a/31143907/742872
* Search for "subjectAltName" https://github.com/digitalbazaar/forge/blob/2bb97afb5058285ef09bcf1d04d6bd6b87cffd58/README.md

* Fix lint issues

* Reword comment

Co-authored-by: Michael <michael.myers@goteleport.com>

* Remove unused imports

Co-authored-by: Michael <michael.myers@goteleport.com>
ravicious added a commit that referenced this pull request Oct 27, 2022
…cess (#1220) (#1302)

* Add comment to resolveNetworkAddress

* Log errors during boot of React app

* Connect: Accommodate for making gRPC server creds in shared process

* Adjust timeout for resolveNetworkAddress

Some of the timeouts in preload.ts, the shared process and tshd are set
to 10s. If the timeout to resolve the network address is lower or equal
to that, those timeout won't have the chance kick in.

Hence setting the timeout to a slightly larger value.

* makeCert: Add subjectAltName to certs

Without this, the cert generated by makeCert was failing when loaded
in Go as the gRPC server cert. The error message was:

    x509: certificate relies on legacy Common Name field, use SANs instead

See:

* https://stackoverflow.com/a/31143907/742872
* Search for "subjectAltName" https://github.com/digitalbazaar/forge/blob/2bb97afb5058285ef09bcf1d04d6bd6b87cffd58/README.md

* Fix lint issues

* Reword comment

Co-authored-by: Michael <michael.myers@goteleport.com>

* Remove unused imports

Co-authored-by: Michael <michael.myers@goteleport.com>

Co-authored-by: Michael <michael.myers@goteleport.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants