Connect: Accommodate for making gRPC client creds from tshd key pair by ravicious · Pull Request #16782 · gravitational/teleport

ravicious · 2022-09-28T10:35:22Z

Equivalent webapps PR: gravitational/webapps#1220.

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.

The previous code assumes that tshd is only going to need server credentials. This commit makes it possible to create client credentials from the same key pair.

ravicious · 2022-09-28T10:56:36Z

This function used to do two things. I extracted them into separate functions.

Create a key pair for the tshd process and save the public key on disk.

This is now generateAndSaveCert.

Create gRPC server credentials.

This is now createServerCredentials.

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. On Windows, we use gRPC over TCP with mTLS. Each process creates its own keypair and saves the public key to a predetermined location. The previous code assumes that tshd is only going to need server credentials. This commit makes it possible to create client credentials from the same key pair.

ravicious · 2022-10-03T15:55:08Z

@nklaassen @xacrimon ping

avatus · 2022-10-03T19:51:19Z

+
+	certificate, err := keys.X509KeyPair(cert.Cert, cert.PrivateKey)
+	if err != nil {
+		return tls.Certificate{}, trace.Wrap(err)


I know this tls.Certificate{} is thrown away anyway if an error is returned but is there a reason to create the new struct on error? Or is this just to satisfy return types?

I know this tls.Certificate{} is thrown away anyway if an error is returned but is there a reason to create the new struct on error? Or is this just to satisfy return types?

Yes, that is to satisfy return types.

xacrimon · 2022-10-06T15:40:46Z

+	// File is first saved using under tempPath and then renamed to fullPath.
+	// This prevents other processes from reading a half written file.
+	fullPath := filepath.Join(path)
+	tempPath := fullPath + ".tmp"


Could there possibly be concurrent calls across different processes that could race here?

Multiple calls to generateAndSaveCert? No, this is called when we run tsh daemon start and we create only one such process.

still, os.CreateTemp might be appropriate and would avoid that problem altogether

e019eb2 makes it so that os.CreateTemp is used instead of manually creating a "temp" file.

xacrimon

lgtm

nklaassen · 2022-10-06T17:28:46Z

+	grpcServer := grpc.NewServer(tshdCreds, grpc.ChainUnaryInterceptor(
 		withErrorHandling(cfg.Log),
 	))


Suggested change

grpcServer := grpc.NewServer(tshdCreds, grpc.ChainUnaryInterceptor(

withErrorHandling(cfg.Log),

))

grpcServer := grpc.NewServer(serverOptions...)

nklaassen · 2022-10-06T17:32:08Z

+// createServerCredentials creates mTLS credentials for a gRPC server. The client cert file is read
+// only on an incoming connection, not upfront.


I can see that the client cert file is read on every connection, the comment should probably explain why

I can see that the client cert file is read on every connection, the comment should probably explain why

Comment added:

teleport/lib/teleterm/apiserver/grpccredentials.go

Lines 40 to 43 in fddbb94

// createServerCredentials creates mTLS credentials for a gRPC server. The client cert file is read

// only on an incoming connection, not upfront. Otherwise we'd need to wait for the client cert file

// to exist before booting up the server.

func createServerCredentials(serverKeyPair tls.Certificate, clientCertPath string) (grpc.ServerOption, error) {

nklaassen · 2022-10-06T17:37:15Z

+func generateAndSaveCert(path string) (tls.Certificate, error) {
+	// File is first saved using under tempPath and then renamed to fullPath.
+	// This prevents other processes from reading a half written file.
+	fullPath := filepath.Join(path)


is there a reason to call filepath.Join with a single arg?

Ah, good catch, thanks. That was a leftover, it used to look like this, then I refactored the function args but forgot to remove the call to Join.

teleport/lib/teleterm/apiserver/apiserver.go

Line 145 in 78cfbfd

tshServerCertFullPath := filepath.Join(certsDir, tshServerCertFileName)

nklaassen · 2022-10-06T17:38:37Z

+	// File is first saved using under tempPath and then renamed to fullPath.
+	// This prevents other processes from reading a half written file.
+	fullPath := filepath.Join(path)
+	tempPath := fullPath + ".tmp"


still, os.CreateTemp might be appropriate and would avoid that problem altogether

ravicious · 2022-10-19T09:46:15Z

+// createServerCredentials creates mTLS credentials for a gRPC server. The client cert file is read
+// only on an incoming connection, not upfront.


I can see that the client cert file is read on every connection, the comment should probably explain why

Comment added:

teleport/lib/teleterm/apiserver/grpccredentials.go

Lines 40 to 43 in fddbb94

// createServerCredentials creates mTLS credentials for a gRPC server. The client cert file is read

// only on an incoming connection, not upfront. Otherwise we'd need to wait for the client cert file

// to exist before booting up the server.

func createServerCredentials(serverKeyPair tls.Certificate, clientCertPath string) (grpc.ServerOption, error) {

ravicious · 2022-10-19T09:47:59Z

+func generateAndSaveCert(path string) (tls.Certificate, error) {
+	// File is first saved using under tempPath and then renamed to fullPath.
+	// This prevents other processes from reading a half written file.
+	fullPath := filepath.Join(path)


Ah, good catch, thanks. That was a leftover, it used to look like this, then I refactored the function args but forgot to remove the call to Join.

teleport/lib/teleterm/apiserver/apiserver.go

Line 145 in 78cfbfd

tshServerCertFullPath := filepath.Join(certsDir, tshServerCertFileName)

ravicious · 2022-10-19T09:49:05Z

+	// File is first saved using under tempPath and then renamed to fullPath.
+	// This prevents other processes from reading a half written file.
+	fullPath := filepath.Join(path)
+	tempPath := fullPath + ".tmp"


e019eb2 makes it so that os.CreateTemp is used instead of manually creating a "temp" file.

ravicious · 2022-10-19T09:52:39Z


-	err = os.WriteFile(tempPath, cert.Cert, 0600)
-	if err != nil {
+	if err = tempFile.Chmod(0600); err != nil {


I'm not sure if I need that extra Chmod here. AFAIK os.CreateTemp already creates a file with 0600 and I suppose we can expect a temp file to have the strictest permissions possible. But the docs for os.CreateTemp don't state any guarantees about it so I'd rather be explicit.

ravicious · 2022-10-20T10:39:14Z

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

github-actions · 2022-10-20T10:39:21Z

@ravicious See the table below for backport results.

Branch	Result
branch/v11	Create PR

ravicious mentioned this pull request Sep 28, 2022

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

Merged

ravicious commented Sep 28, 2022

View reviewed changes

ravicious force-pushed the ravicious/refactor-grpc-certs branch from 53ad1fe to 867f227 Compare September 28, 2022 10:59

ravicious marked this pull request as ready for review September 28, 2022 11:09

github-actions Bot requested review from nklaassen and xacrimon September 28, 2022 11:09

ravicious requested a review from avatus October 3, 2022 08:57

ravicious mentioned this pull request Oct 3, 2022

tshd-initiated communication for Connect #16043

Closed

ravicious added the backport/branch/v10 label Oct 3, 2022

avatus reviewed Oct 3, 2022

View reviewed changes

ravicious added backport/branch/v11 and removed backport/branch/v10 labels Oct 6, 2022

xacrimon reviewed Oct 6, 2022

View reviewed changes

xacrimon approved these changes Oct 6, 2022

View reviewed changes

nklaassen reviewed Oct 6, 2022

View reviewed changes

ravicious added 5 commits October 19, 2022 10:57

Refactor server options

ae2c05f

Expand the comment for createServerCredentials

fddbb94

Remove unnecessary filepath.Join

0aedbd3

generateAndSaveCert: Use os.CreateTemp

e019eb2

Merge branch 'master' into ravicious/refactor-grpc-certs

472d588

ravicious commented Oct 19, 2022

View reviewed changes

ravicious requested a review from nklaassen October 19, 2022 09:53

nklaassen approved these changes Oct 19, 2022

View reviewed changes

ravicious added 2 commits October 20, 2022 10:41

Merge branch 'master' into ravicious/refactor-grpc-certs

22c6608

Merge branch 'master' into ravicious/refactor-grpc-certs

bd5dd6b

ravicious merged commit 5adcebb into master Oct 20, 2022

ravicious deleted the ravicious/refactor-grpc-certs branch October 20, 2022 14:10

ravicious mentioned this pull request Oct 26, 2022

[v11] Connect: Accommodate for making gRPC client creds from tshd key pair #17841

Merged

		// createServerCredentials creates mTLS credentials for a gRPC server. The client cert file is read
		// only on an incoming connection, not upfront.

	// createServerCredentials creates mTLS credentials for a gRPC server. The client cert file is read
	// only on an incoming connection, not upfront. Otherwise we'd need to wait for the client cert file
	// to exist before booting up the server.
	func createServerCredentials(serverKeyPair tls.Certificate, clientCertPath string) (grpc.ServerOption, error) {

Conversation

ravicious commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravicious commented Oct 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xacrimon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravicious commented Oct 20, 2022

Uh oh!

github-actions Bot commented Oct 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ravicious commented Sep 28, 2022 •

edited

Loading