[vnet] mTLS for IPC on Windows #51856
Open
+407
−14
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nklaassen
added
no-changelog
codingllama
reviewed
Feb 5, 2025
Comment on lines
+53
to
+54
| // longer than any individual Teleport session. Going with a month for now. | ||
| certTTL = 30 * 12 * time.Hour |
There was a problem hiding this comment.
Going with a month for now.
certTTL = 30 * 12 * time.Hour
This is 15d, no? Also a minor nit, but I would say "30 days" or "15 days" instead of "a month" - a month is not an easily defined measure of time.
Comment on lines
+40
to
+41
| trustedCAPEM []byte | ||
| certPEM []byte |
There was a problem hiding this comment.
nit: Document what is inside the PEM.
Suggested change
| trustedCAPEM []byte | |
| certPEM []byte | |
| trustedCAPEM []byte // X.509 CA certificate | |
| certPEM []byte // X.509 certificate for the vnet proccess |
| // there's no way to fetch the cluster signature_algorithm_suite or unify | ||
| // suites across multiple root clusters, so just statically use ECDSA | ||
| // P-256 for these keys. | ||
| keyAlgo = cryptosuites.ECDSAP256 |
There was a problem hiding this comment.
Out of curiosity, would we ever have to customize these? These are only for IPC/local vnet connections, right?
Comment on lines
+57
to
+72
| serverCAKey, err := cryptosuites.GenerateKeyWithAlgorithm(keyAlgo) | ||
| if err != nil { | ||
| return nil, trace.Wrap(err, "generating key") | ||
| } | ||
| clientCAKey, err := cryptosuites.GenerateKeyWithAlgorithm(keyAlgo) | ||
| if err != nil { | ||
| return nil, trace.Wrap(err, "generating key") | ||
| } | ||
| serverSigner, err := cryptosuites.GenerateKeyWithAlgorithm(keyAlgo) | ||
| if err != nil { | ||
| return nil, trace.Wrap(err, "generating key") | ||
| } | ||
| clientSigner, err := cryptosuites.GenerateKeyWithAlgorithm(keyAlgo) | ||
| if err != nil { | ||
| return nil, trace.Wrap(err, "generating key") | ||
| } |
There was a problem hiding this comment.
nit: Add a little more detail?
Suggested change
| serverCAKey, err := cryptosuites.GenerateKeyWithAlgorithm(keyAlgo) | |
| if err != nil { | |
| return nil, trace.Wrap(err, "generating key") | |
| } | |
| clientCAKey, err := cryptosuites.GenerateKeyWithAlgorithm(keyAlgo) | |
| if err != nil { | |
| return nil, trace.Wrap(err, "generating key") | |
| } | |
| serverSigner, err := cryptosuites.GenerateKeyWithAlgorithm(keyAlgo) | |
| if err != nil { | |
| return nil, trace.Wrap(err, "generating key") | |
| } | |
| clientSigner, err := cryptosuites.GenerateKeyWithAlgorithm(keyAlgo) | |
| if err != nil { | |
| return nil, trace.Wrap(err, "generating key") | |
| } | |
| serverCAKey, err := cryptosuites.GenerateKeyWithAlgorithm(keyAlgo) | |
| if err != nil { | |
| return nil, trace.Wrap(err, "generating server CA key") | |
| } | |
| clientCAKey, err := cryptosuites.GenerateKeyWithAlgorithm(keyAlgo) | |
| if err != nil { | |
| return nil, trace.Wrap(err, "generating client CA key") | |
| } | |
| serverSigner, err := cryptosuites.GenerateKeyWithAlgorithm(keyAlgo) | |
| if err != nil { | |
| return nil, trace.Wrap(err, "generating server key") | |
| } | |
| clientSigner, err := cryptosuites.GenerateKeyWithAlgorithm(keyAlgo) | |
| if err != nil { | |
| return nil, trace.Wrap(err, "generating client key") | |
| } |
|
|
||
| serverCAPEM, err := tlsca.GenerateSelfSignedCAWithSigner(serverCAKey, pkix.Name{CommonName: "TeleportVNet Server CA"}, nil /*dnsNames*/, certTTL) | ||
| if err != nil { | ||
| return nil, trace.Wrap(err, "generating self-signed CA") |
There was a problem hiding this comment.
Suggested change
| return nil, trace.Wrap(err, "generating self-signed CA") | |
| return nil, trace.Wrap(err, "generating self-signed server CA") |
| // O: Set Owner as current user (Windows won't allow LocalSystem to be the owner) | ||
| // G: Set Primary Group as current user | ||
| // D: Discretionary ACL | ||
| // Grant GA (Generic All) to LocalSystam |
There was a problem hiding this comment.
Suggested change
| // Grant GA (Generic All) to LocalSystam | |
| // Grant GA (Generic All) to LocalSystem |
| // Grant GA (Generic All) to LocalSystam | ||
| // Grant GW (Generic Write) to the current user so we can add files | ||
| // Deny GR (Generic Read) for the current user so other processes can't read the credentials | ||
| // OICI (Object Inherit, Container Inherit) propagates permissions to files/folders |
There was a problem hiding this comment.
I appreciate the comment block above, this would be extremely opaque without it!
There was a problem hiding this comment.
TIL a _windows_test.go file will still auto-apply the windows build tag, I thought the file had to end with the build tag. This is nice.
| require.NoError(t, creds.client.write(credPath)) | ||
|
|
||
| _, err = readCredentials(credPath) | ||
| require.Error(t, err, "expected no access to creds") |
There was a problem hiding this comment.
Should we check for a particular error type? What do we get, fs.ErrPermission?
| for _, f := range files { | ||
| fp := filepath.Join(credPath, f) | ||
| _, err := os.ReadFile(fp) | ||
| require.Error(t, err, "expected no access to %s", fp) |
There was a problem hiding this comment.
Same for checking for a specific error. Also, Keep Going.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of RFD 195.
This PR sets up mTLS for the gRPC service provided by the VNet client application (tsh or Connect) to the Windows service.
Following the design in the RFD, the directory that the service credentials are written to is configured with Windows ACLs so that only the LocalSystem account can read the files. This way, the client can trust that any client connecting to it already has LocalSystem permissions and is probably the VNet Windows service.