From 80057f4343771efef9f8fc4fd0c86f4bf291e76a Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Wed, 19 Aug 2026 16:45:47 -0700 Subject: [PATCH 1/3] feat(sshcert): add per-environment SSH certificate store Foundation for certificate-based SSH auth. Generates fresh ed25519 keypairs per renewal, caches the (private key, certificate) pair on disk for the certificate's validity window, and writes atomically (0600 private key). Files live under ~/.brev/ssh-certs/{,-cert.pub} so a single IdentityFile directive loads both key and cert (OpenSSH -cert.pub convention). Independent of the IssueEnvironmentSSHCertificate RPC so the rest of the feature can build and test before the generated connect client is published. Includes EnvironmentCertEligible() mirroring dev-plane's label constants (sshprovider=certauth). --- go.mod | 10 +- go.sum | 16 +-- pkg/analytics/posthog.go | 23 ++++ pkg/cmd/cmd.go | 2 + pkg/cmd/mintcert/mintcert.go | 149 +++++++++++++++++++++++ pkg/cmd/mintcert/mintcert_test.go | 188 ++++++++++++++++++++++++++++ pkg/cmd/refresh/sshaccess.go | 3 + pkg/cmd/refresh/sshaccess_test.go | 35 ++++++ pkg/entity/entity.go | 18 ++- pkg/ssh/sshconfigurer.go | 141 ++++++++++++++++----- pkg/ssh/sshconfigurer_test.go | 195 +++++++++++++++++++++++++++++- pkg/sshcert/sshcert.go | 173 ++++++++++++++++++++++++++ pkg/sshcert/sshcert_test.go | 142 ++++++++++++++++++++++ 13 files changed, 1042 insertions(+), 53 deletions(-) create mode 100644 pkg/cmd/mintcert/mintcert.go create mode 100644 pkg/cmd/mintcert/mintcert_test.go create mode 100644 pkg/sshcert/sshcert.go create mode 100644 pkg/sshcert/sshcert_test.go diff --git a/go.mod b/go.mod index 1124a539..5855b110 100644 --- a/go.mod +++ b/go.mod @@ -3,8 +3,8 @@ module github.com/brevdev/brev-cli go 1.25.0 require ( - buf.build/gen/go/brevdev/devplane/connectrpc/go v1.20.0-20260708012811-ecba52f49600.1 - buf.build/gen/go/brevdev/devplane/protocolbuffers/go v1.36.11-20260708012811-ecba52f49600.1 + buf.build/gen/go/brevdev/devplane/connectrpc/go v1.20.0-20260820222245-1cfc91443320.1 + buf.build/gen/go/brevdev/devplane/protocolbuffers/go v1.36.12-20260820222245-1cfc91443320.1 connectrpc.com/connect v1.20.0 github.com/NVIDIA/go-nvml v0.13.0-1 github.com/alessio/shellescape v1.4.1 @@ -44,12 +44,13 @@ require ( github.com/tweekmonster/luser v0.0.0-20161003172636-3fa38070dbd7 github.com/wk8/go-ordered-map/v2 v2.0.0 github.com/writeas/go-strip-markdown v2.0.1+incompatible + golang.org/x/crypto v0.55.0 golang.org/x/text v0.41.0 k8s.io/cli-runtime v0.31.1 ) require ( - buf.build/gen/go/brevdev/protoc-gen-gotag/protocolbuffers/go v1.36.11-20220906235457-8b4922735da5.1 // indirect + buf.build/gen/go/brevdev/protoc-gen-gotag/protocolbuffers/go v1.36.12-20220906235457-8b4922735da5.1 // indirect dario.cat/mergo v1.0.0 // indirect github.com/Azure/go-ansiterm v0.0.0-20210617225240-d185dfc1b5a1 // indirect github.com/Microsoft/go-winio v0.6.2 // indirect @@ -100,7 +101,6 @@ require ( github.com/x448/float16 v0.8.4 // indirect github.com/xanzy/ssh-agent v0.3.3 // indirect golang.org/x/arch v0.8.0 // indirect - golang.org/x/crypto v0.55.0 // indirect golang.org/x/exp v0.0.0-20260410095643-746e56fc9e2f // indirect golang.org/x/sync v0.22.0 // indirect gopkg.in/evanphx/json-patch.v4 v4.12.0 // indirect @@ -152,7 +152,7 @@ require ( golang.org/x/sys v0.47.0 golang.org/x/term v0.45.0 // indirect golang.org/x/time v0.12.0 // indirect - google.golang.org/protobuf v1.36.11 + google.golang.org/protobuf v1.36.12 gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/yaml.v2 v2.4.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect diff --git a/go.sum b/go.sum index d6abb04d..4af12529 100644 --- a/go.sum +++ b/go.sum @@ -1,9 +1,9 @@ -buf.build/gen/go/brevdev/devplane/connectrpc/go v1.20.0-20260708012811-ecba52f49600.1 h1:xanul5g4JQ0OPAQ3tjN8bTznw+aA6B/oq3pzOy8kC8Q= -buf.build/gen/go/brevdev/devplane/connectrpc/go v1.20.0-20260708012811-ecba52f49600.1/go.mod h1:ZxWENaPM6882Wtl2z6rZYVpXoagSyF6DiY/6m4BjGMU= -buf.build/gen/go/brevdev/devplane/protocolbuffers/go v1.36.11-20260708012811-ecba52f49600.1 h1:KMs3AGf1zys1H8TnjBCorCd12zzWoUQae956KgsNfRM= -buf.build/gen/go/brevdev/devplane/protocolbuffers/go v1.36.11-20260708012811-ecba52f49600.1/go.mod h1:V/y7Wxg0QvU4XPVwqErF5NHLobUT1QEyfgrGuQIxdPo= -buf.build/gen/go/brevdev/protoc-gen-gotag/protocolbuffers/go v1.36.11-20220906235457-8b4922735da5.1 h1:6amhprQmCKJ4wgJ6ngkh32d9V+dQcOLUZ/SfHdOnYgo= -buf.build/gen/go/brevdev/protoc-gen-gotag/protocolbuffers/go v1.36.11-20220906235457-8b4922735da5.1/go.mod h1:O+pnSHMru/naTMrm4tmpBoH3wz6PHa+R75HR7Mv8X2g= +buf.build/gen/go/brevdev/devplane/connectrpc/go v1.20.0-20260820222245-1cfc91443320.1 h1:PKIsaGilewnQUSHNUn+Ir4sagWne713vJS3Ys7h9vAY= +buf.build/gen/go/brevdev/devplane/connectrpc/go v1.20.0-20260820222245-1cfc91443320.1/go.mod h1:r4xfuOy9bpAXm13ugDRO+JNmFVlXecGRuKtn1X7os/k= +buf.build/gen/go/brevdev/devplane/protocolbuffers/go v1.36.12-20260820222245-1cfc91443320.1 h1:gmAgE9NC+BAovZIs9CNmjgExqM+Gox8AZ6ud3eVMxfA= +buf.build/gen/go/brevdev/devplane/protocolbuffers/go v1.36.12-20260820222245-1cfc91443320.1/go.mod h1:N18pnR0HL6srurI7G19FpSEki71wA1u4e2c5zbfeTV8= +buf.build/gen/go/brevdev/protoc-gen-gotag/protocolbuffers/go v1.36.12-20220906235457-8b4922735da5.1 h1:Qk/4GJyWVWvWsfEFeX4T+k7KouZdRUxxUnIUwJ3hmZg= +buf.build/gen/go/brevdev/protoc-gen-gotag/protocolbuffers/go v1.36.12-20220906235457-8b4922735da5.1/go.mod h1:SacJAYqnICCQAsBA46cSA/hxhqhxYkiYzseucf6/fhQ= cloud.google.com/go v0.26.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMTw= cloud.google.com/go v0.34.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMTw= cloud.google.com/go v0.38.0/go.mod h1:990N+gfupTy94rShfmMCWGDn0LpTmnzTp2qbd1dvSRU= @@ -785,8 +785,8 @@ google.golang.org/protobuf v1.23.0/go.mod h1:EGpADcykh3NcUnDUJcl1+ZksZNG86OlYog2 google.golang.org/protobuf v1.23.1-0.20200526195155-81db48ad09cc/go.mod h1:EGpADcykh3NcUnDUJcl1+ZksZNG86OlYog2l/sGQquU= google.golang.org/protobuf v1.24.0/go.mod h1:r/3tXBNzIEhYS9I1OUVjXDlt8tc493IdKGjtUeSXeh4= google.golang.org/protobuf v1.25.0/go.mod h1:9JNX74DMeImyA3h4bdi1ymwjUzf21/xIlbajtzgsN7c= -google.golang.org/protobuf v1.36.11 h1:fV6ZwhNocDyBLK0dj+fg8ektcVegBBuEolpbTQyBNVE= -google.golang.org/protobuf v1.36.11/go.mod h1:HTf+CrKn2C3g5S8VImy6tdcUvCska2kB7j23XfzDpco= +google.golang.org/protobuf v1.36.12 h1:pJOKDDOyeXErUroCihFAd5LQuwXBSpVnKGrj5o/fwxc= +google.golang.org/protobuf v1.36.12/go.mod h1:HTf+CrKn2C3g5S8VImy6tdcUvCska2kB7j23XfzDpco= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= diff --git a/pkg/analytics/posthog.go b/pkg/analytics/posthog.go index 433328ab..65746be7 100644 --- a/pkg/analytics/posthog.go +++ b/pkg/analytics/posthog.go @@ -72,6 +72,29 @@ func IsAnalyticsFeatureEnabled() bool { return ok && enabled } +func IsSSHCertRequired() bool { + anonID := GetOrCreateAnalyticsID() + if anonID == "" { + return false + } + + c, err := getClient() + if err != nil { + return false + } + + result, err := c.IsFeatureEnabled(posthog.FeatureFlagPayload{ + Key: "ssh-cert-required-cli", + DistinctId: anonID, + }) + if err != nil { + return false + } + + enabled, ok := result.(bool) + return ok && enabled +} + // RecordCommandStart should be called from PersistentPreRunE to record the start time // and store the command context for potential error-path capture. func RecordCommandStart(cmd *cobra.Command, args []string) { diff --git a/pkg/cmd/cmd.go b/pkg/cmd/cmd.go index fb22eea3..8aa4c561 100644 --- a/pkg/cmd/cmd.go +++ b/pkg/cmd/cmd.go @@ -30,6 +30,7 @@ import ( "github.com/brevdev/brev-cli/pkg/cmd/login" "github.com/brevdev/brev-cli/pkg/cmd/logout" "github.com/brevdev/brev-cli/pkg/cmd/ls" + "github.com/brevdev/brev-cli/pkg/cmd/mintcert" "github.com/brevdev/brev-cli/pkg/cmd/notebook" "github.com/brevdev/brev-cli/pkg/cmd/ollama" "github.com/brevdev/brev-cli/pkg/cmd/open" @@ -303,6 +304,7 @@ func createCmdTree(cmd *cobra.Command, t *terminal.Terminal, loginCmdStore *stor cmd.AddCommand(configureenvvars.NewCmdConfigureEnvVars(t, loginCmdStore)) cmd.AddCommand(importideconfig.NewCmdImportIDEConfig(t, noLoginCmdStore)) cmd.AddCommand(shell.NewCmdShell(t, loginCmdStore, noLoginCmdStore)) + cmd.AddCommand(mintcert.NewCmdMintCert(noLoginCmdStore)) cmd.AddCommand(exec.NewCmdExec(t, loginCmdStore, noLoginCmdStore)) cmd.AddCommand(copy.NewCmdCopy(t, loginCmdStore, noLoginCmdStore)) cmd.AddCommand(open.NewCmdOpen(t, loginCmdStore, noLoginCmdStore)) diff --git a/pkg/cmd/mintcert/mintcert.go b/pkg/cmd/mintcert/mintcert.go new file mode 100644 index 00000000..2d62af00 --- /dev/null +++ b/pkg/cmd/mintcert/mintcert.go @@ -0,0 +1,149 @@ +// Package mintcert implements the `brev mint-cert` command, which mints a +// short-lived SSH certificate for an environment and writes it (with its +// backing ephemeral keypair) to disk. It is invoked by the ssh config's +// Match exec hook, generated by `brev refresh`. +package mintcert + +import ( + "context" + "fmt" + "os" + "time" + + devplanev1 "buf.build/gen/go/brevdev/devplane/protocolbuffers/go/devplaneapi/v1" + "connectrpc.com/connect" + "github.com/spf13/afero" + "github.com/spf13/cobra" + + "github.com/brevdev/brev-cli/pkg/cmd/register" + "github.com/brevdev/brev-cli/pkg/config" + breverrors "github.com/brevdev/brev-cli/pkg/errors" + "github.com/brevdev/brev-cli/pkg/externalnode" + "github.com/brevdev/brev-cli/pkg/sshcert" +) + +const timeout = 15 * time.Second + +type Store interface { + GetAccessToken() (string, error) +} + +type CertIssuer interface { + Issue(ctx context.Context, req certIssueRequest) (string, error) +} + +type certIssueRequest struct { + EnvironmentID string + PortID string + LinuxUser string + PublicKey string +} + +type environmentCertClient interface { + IssueEnvironmentSSHCertificate(ctx context.Context, req *connect.Request[devplanev1.IssueEnvironmentSSHCertificateRequest]) (*connect.Response[devplanev1.IssueEnvironmentSSHCertificateResponse], error) +} + +type rpcCertIssuer struct { + client environmentCertClient +} + +func (r rpcCertIssuer) Issue(ctx context.Context, req certIssueRequest) (string, error) { + res, err := r.client.IssueEnvironmentSSHCertificate(ctx, connect.NewRequest(&devplanev1.IssueEnvironmentSSHCertificateRequest{ + EnvironmentId: req.EnvironmentID, + LinuxUser: req.LinuxUser, + PortId: req.PortID, + PublicKey: req.PublicKey, + })) + if err != nil { + return "", breverrors.WrapAndTrace(err) + } + return res.Msg.GetCertificate(), nil +} + +func NewCmdMintCert(store Store) *cobra.Command { + var ( + env string + port string + user string + outKey string + ) + cmd := &cobra.Command{ + Use: "mint-cert", + Short: "Mint a short-lived SSH certificate for an environment", + Args: cobra.NoArgs, + Hidden: true, + RunE: func(cmd *cobra.Command, args []string) error { + return runMintCert(store, mintCertRequest{ + EnvironmentID: env, + PortID: port, + LinuxUser: user, + OutKey: outKey, + }) + }, + } + cmd.Flags().StringVar(&env, "env", "", "environment ID to mint a certificate for") + cmd.Flags().StringVar(&port, "port", "", "network-member port ID for the SSH access") + cmd.Flags().StringVar(&user, "linux-user", "", "Linux user for the certificate principal") + cmd.Flags().StringVar(&outKey, "out-key", "", "private-key path (certificate goes to -cert.pub)") + _ = cmd.MarkFlagRequired("env") + _ = cmd.MarkFlagRequired("port") + _ = cmd.MarkFlagRequired("linux-user") + _ = cmd.MarkFlagRequired("out-key") + return cmd +} + +type mintCertRequest struct { + EnvironmentID string + PortID string + LinuxUser string + OutKey string +} + +func runMintCert(store Store, req mintCertRequest) error { + return runMintCertWith(store, afero.NewOsFs(), newCertIssuer(store, config.GlobalConfig.GetBrevPublicAPIURL()), req) +} + +func runMintCertWith(store Store, fs afero.Fs, issuer CertIssuer, req mintCertRequest) error { + token, err := store.GetAccessToken() + if err != nil || token == "" { + _, _ = fmt.Fprintln(os.Stderr, "brev: not logged in. Run `brev login` and retry.") + if err != nil { + return breverrors.WrapAndTrace(err) + } + return fmt.Errorf("not logged in") + } + _ = token + certPath := req.OutKey + "-cert.pub" + if ok, err := sshcert.HasValidCertAt(fs, certPath, time.Now(), sshcert.DefaultRenewalMargin); err != nil { + _, _ = fmt.Fprintf(os.Stderr, "brev: failed to check cached cert: %v\n", err) + return breverrors.WrapAndTrace(err) + } else if ok { + return nil + } + privKeyPEM, pubKeyOpenSSH, err := sshcert.GenerateKeyPair() + if err != nil { + _, _ = fmt.Fprintf(os.Stderr, "brev: failed to generate keypair: %v\n", err) + return breverrors.WrapAndTrace(err) + } + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + cert, err := issuer.Issue(ctx, certIssueRequest{ + EnvironmentID: req.EnvironmentID, + PortID: req.PortID, + LinuxUser: req.LinuxUser, + PublicKey: pubKeyOpenSSH, + }) + if err != nil { + _, _ = fmt.Fprintf(os.Stderr, "brev: could not issue ssh certificate: %v\n", err) + return breverrors.WrapAndTrace(err) + } + if err := sshcert.WriteFiles(fs, req.OutKey, certPath, privKeyPEM, cert); err != nil { + _, _ = fmt.Fprintf(os.Stderr, "brev: failed to write cert files: %v\n", err) + return breverrors.WrapAndTrace(err) + } + return nil +} + +func newCertIssuer(provider externalnode.TokenProvider, baseURL string) CertIssuer { + return rpcCertIssuer{client: register.NewEnvironmentServiceClient(provider, baseURL)} +} diff --git a/pkg/cmd/mintcert/mintcert_test.go b/pkg/cmd/mintcert/mintcert_test.go new file mode 100644 index 00000000..9aaf5973 --- /dev/null +++ b/pkg/cmd/mintcert/mintcert_test.go @@ -0,0 +1,188 @@ +package mintcert + +import ( + "context" + "crypto/ed25519" + "crypto/rand" + "errors" + "strings" + "testing" + "time" + + devplanev1 "buf.build/gen/go/brevdev/devplane/protocolbuffers/go/devplaneapi/v1" + "connectrpc.com/connect" + "github.com/spf13/afero" + "golang.org/x/crypto/ssh" + + "github.com/brevdev/brev-cli/pkg/sshcert" +) + +type fakeStore struct { + token string + err error +} + +func (f fakeStore) GetAccessToken() (string, error) { + if f.err != nil { + return "", f.err + } + return f.token, nil +} + +type certIssuerFunc struct { + fn func(context.Context, certIssueRequest) (string, error) +} + +func (c *certIssuerFunc) Issue(ctx context.Context, req certIssueRequest) (string, error) { + return c.fn(ctx, req) +} + +type fakeEnvCertClient struct { + resp *devplanev1.IssueEnvironmentSSHCertificateResponse + err error + got *devplanev1.IssueEnvironmentSSHCertificateRequest +} + +func (f *fakeEnvCertClient) IssueEnvironmentSSHCertificate(_ context.Context, req *connect.Request[devplanev1.IssueEnvironmentSSHCertificateRequest]) (*connect.Response[devplanev1.IssueEnvironmentSSHCertificateResponse], error) { + f.got = req.Msg + if f.err != nil { + return nil, f.err + } + return connect.NewResponse(f.resp), nil +} + +func mintCertForTest(t *testing.T, pubKeyOpenSSH string) string { + t.Helper() + pubKey, _, _, _, err := ssh.ParseAuthorizedKey([]byte(pubKeyOpenSSH)) + if err != nil { + t.Fatalf("parse pub key: %v", err) + } + _, privCA, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("generate ca: %v", err) + } + signer, err := ssh.NewSignerFromKey(privCA) + if err != nil { + t.Fatalf("new signer: %v", err) + } + cert := &ssh.Certificate{ + Key: pubKey, + Serial: 42, + CertType: ssh.UserCert, + KeyId: "brev:v1:user:test", + ValidPrincipals: []string{"brev:v1:vm:test-env:login:ubuntu"}, + ValidAfter: uint64(1), + ValidBefore: uint64(1<<63 - 1), // far future for cache tests + Permissions: ssh.Permissions{Extensions: map[string]string{"permit-pty": ""}}, + } + if err := cert.SignCert(rand.Reader, signer); err != nil { + t.Fatalf("sign cert: %v", err) + } + return strings.TrimRight(string(ssh.MarshalAuthorizedKey(cert)), "\n") +} + +func TestRunMintCert_MintsAndWrites(t *testing.T) { + fs := afero.NewMemMapFs() + outKey := "/home/u/.brev/ssh-certs/env-1" + issuer := &certIssuerFunc{fn: func(_ context.Context, req certIssueRequest) (string, error) { + return mintCertForTest(t, req.PublicKey), nil + }} + if err := runMintCertWith(fakeStore{token: "tok"}, fs, issuer, mintCertRequest{ + EnvironmentID: "env-1", PortID: "port-1", LinuxUser: "ubuntu", OutKey: outKey, + }); err != nil { + t.Fatalf("runMintCertWith: %v", err) + } + for _, p := range []string{outKey, outKey + "-cert.pub"} { + if ok, _ := afero.Exists(fs, p); !ok { + t.Errorf("not written: %s", p) + } + } + if ok, err := sshcert.HasValidCertAt(fs, outKey+"-cert.pub", time.Now(), 0); err != nil || !ok { + t.Errorf("written cert not valid: ok=%v err=%v", ok, err) + } + privBytes, err := afero.ReadFile(fs, outKey) + if err != nil { + t.Fatalf("read private key: %v", err) + } + _, err = ssh.ParsePrivateKey(privBytes) + if err != nil { + t.Fatalf("written private key not ssh-loadable: %v", err) + } +} + +func TestRunMintCert_ReusesCachedCert(t *testing.T) { + fs := afero.NewMemMapFs() + outKey := "/home/u/.brev/ssh-certs/env-1" + _, pub, _ := sshcert.GenerateKeyPair() + if err := sshcert.WriteFiles(fs, outKey, outKey+"-cert.pub", []byte("priv"), mintCertForTest(t, pub)); err != nil { + t.Fatalf("seed: %v", err) + } + issuer := &certIssuerFunc{fn: func(_ context.Context, _ certIssueRequest) (string, error) { + t.Error("issuer should not be called when cache is valid") + return "", nil + }} + if err := runMintCertWith(fakeStore{token: "tok"}, fs, issuer, mintCertRequest{ + EnvironmentID: "env-1", PortID: "port-1", LinuxUser: "ubuntu", OutKey: outKey, + }); err != nil { + t.Fatalf("expected reuse, got err: %v", err) + } +} + +func TestRunMintCert_FallsBackOnIssueError(t *testing.T) { + fs := afero.NewMemMapFs() + issuer := &certIssuerFunc{fn: func(_ context.Context, _ certIssueRequest) (string, error) { + return "", errors.New("CA unavailable") + }} + err := runMintCertWith(fakeStore{token: "tok"}, fs, issuer, mintCertRequest{ + EnvironmentID: "env-1", PortID: "port-1", LinuxUser: "ubuntu", + OutKey: "/home/u/.brev/ssh-certs/env-1", + }) + if err == nil { + t.Fatal("expected error on issue failure") + } + if ok, _ := afero.Exists(fs, "/home/u/.brev/ssh-certs/env-1"); ok { + t.Error("private key should not be written on issue failure") + } +} + +func TestRunMintCert_FallsBackOnAuthError(t *testing.T) { + fs := afero.NewMemMapFs() + issuer := &certIssuerFunc{fn: func(_ context.Context, _ certIssueRequest) (string, error) { + t.Error("issuer should not be called when not authenticated") + return "", nil + }} + // GetAccessToken error -> auth failure (no prompt, fall back to brev.pem). + if err := runMintCertWith(fakeStore{err: errors.New("no token")}, fs, issuer, mintCertRequest{ + EnvironmentID: "env-1", PortID: "port-1", LinuxUser: "ubuntu", + OutKey: "/home/u/.brev/ssh-certs/env-1", + }); err == nil { + t.Fatal("expected error on auth failure") + } + // Empty token (noLoginCmdStore returns "") -> auth failure, NOT a prompt. + if err := runMintCertWith(fakeStore{token: ""}, fs, issuer, mintCertRequest{ + EnvironmentID: "env-1", PortID: "port-1", LinuxUser: "ubuntu", + OutKey: "/home/u/.brev/ssh-certs/env-1", + }); err == nil { + t.Fatal("expected error on empty token (must not prompt)") + } +} + +func TestRpcCertIssuer_MapsRequestAndResponse(t *testing.T) { + client := &fakeEnvCertClient{resp: &devplanev1.IssueEnvironmentSSHCertificateResponse{ + Certificate: "ssh-ed25519-cert-v01@openssh.com AAAA cert", + Principal: "brev:v1:vm:env-1:login:ubuntu", + }} + issuer := rpcCertIssuer{client: client} + cert, err := issuer.Issue(context.Background(), certIssueRequest{ + EnvironmentID: "env-1", PortID: "port-1", LinuxUser: "ubuntu", PublicKey: "ssh-ed25519 AAAA pub", + }) + if err != nil { + t.Fatalf("Issue: %v", err) + } + if cert != "ssh-ed25519-cert-v01@openssh.com AAAA cert" { + t.Errorf("unexpected certificate: %s", cert) + } + if client.got.GetEnvironmentId() != "env-1" || client.got.GetPortId() != "port-1" || client.got.GetLinuxUser() != "ubuntu" || client.got.GetPublicKey() != "ssh-ed25519 AAAA pub" { + t.Errorf("request fields wrong: %+v", client.got) + } +} diff --git a/pkg/cmd/refresh/sshaccess.go b/pkg/cmd/refresh/sshaccess.go index aad5c3e9..2d2a0737 100644 --- a/pkg/cmd/refresh/sshaccess.go +++ b/pkg/cmd/refresh/sshaccess.go @@ -13,6 +13,7 @@ import ( "github.com/brevdev/brev-cli/pkg/config" "github.com/brevdev/brev-cli/pkg/entity" breverrors "github.com/brevdev/brev-cli/pkg/errors" + "github.com/brevdev/brev-cli/pkg/sshcert" ) const sshAccessLookupTimeout = 10 * time.Second @@ -116,6 +117,8 @@ func resolveWorkspaceSSH( workspace.SSHPort = int(port.GetPortNumber()) workspace.SSHUser = access.GetLinuxUser() workspace.SSHProxyHostname = "" + workspace.PortID = access.GetPortId() + workspace.SSHCertEligible = sshcert.EnvironmentCertEligible(environment.GetLabels()) // To support the "--host" fallback, preserve the legacy hostname information returned by the initial workspace query. if providerHostname := providerSSHHostname(environment.GetInstance(), port.GetHostname()); providerHostname != "" { diff --git a/pkg/cmd/refresh/sshaccess_test.go b/pkg/cmd/refresh/sshaccess_test.go index 0695be95..91bed28d 100644 --- a/pkg/cmd/refresh/sshaccess_test.go +++ b/pkg/cmd/refresh/sshaccess_test.go @@ -82,6 +82,8 @@ func TestEnrichWorkspacesWithSSHAccess_UsesCurrentUsersPort(t *testing.T) { want.SSHProxyHostname = "" want.HostSSHHostname = "203.0.113.10" want.HostSSHProxyHostname = "" + want.PortID = "ssh-port" + want.SSHCertEligible = false // mock environment has no certauth label if diff := cmp.Diff([]entity.Workspace{want}, got); diff != "" { t.Fatalf("unexpected workspace (-want +got): %s", diff) @@ -138,3 +140,36 @@ func TestEnrichWorkspacesWithSSHAccess_FallsBackWithoutPortBackedAccess(t *testi t.Fatal("network info should not be fetched without port-backed access") } } + +func TestEnrichWorkspacesWithSSHAccess_MarksCertEligibleFromLabels(t *testing.T) { + workspace := entity.Workspace{ + ID: "env-1", + Name: "cert-env", + Status: entity.Running, + } + client := &stubEnvironmentSSHClient{ + environment: &devplanev1.Environment{ + Labels: map[string]string{"sshprovider": "certauth"}, + Instance: &devplanev1.Instance{SshHostname: "203.0.113.10", SshPort: 22, PublicIp: "203.0.113.10"}, + SshAccess: []*devplanev1.SSHAccess{ + {UserId: "user-1", LinuxUser: "ubuntu", PortId: "ssh-port"}, + }, + }, + networkInfo: &devplanev1.EnvironmentNetworkInfo{ + Ports: []*devplanev1.Port{ + {PortId: "ssh-port", Hostname: strPtr("skybridge.example.com"), PortNumber: 41234, ServerPort: 22}, + }, + }, + } + + got := enrichWorkspacesWithSSHAccess(context.Background(), client, "user-1", []entity.Workspace{workspace}) + if len(got) != 1 { + t.Fatalf("expected 1 workspace, got %d", len(got)) + } + if got[0].PortID != "ssh-port" { + t.Errorf("PortID = %q, want %q", got[0].PortID, "ssh-port") + } + if !got[0].SSHCertEligible { + t.Errorf("SSHCertEligible = false, want true (labels have sshprovider=certauth)") + } +} diff --git a/pkg/entity/entity.go b/pkg/entity/entity.go index 1efe288c..c22e1ee4 100644 --- a/pkg/entity/entity.go +++ b/pkg/entity/entity.go @@ -294,16 +294,14 @@ type Workspace struct { HostSSHProxyHostname string `json:"hostSshProxyHostname"` VerbBuildStatus VerbBuildStatus `json:"verbBuildStatus"` VerbYaml string `json:"verbYaml"` - // PrimaryApplicationId string `json:"primaryApplicationId,omitempty"` - // LastOnlineAt string `json:"lastOnlineAt,omitempty"` - // CreatedAt string `json:"createdAt,omitempty"` - // UpdatedAt string `json:"updatedAt,omitempty"` - HealthStatus string `json:"healthStatus"` - IsStoppable bool `json:"isStoppable"` // used for autopstop only - StatusMessage string `json:"statusMessage"` - StopTimeout time.Duration `json:"stopTimeout"` - AdditionalUsers []string `json:"additionalUsers"` - Tunnel Tunnel `json:"tunnel"` + PortID string `json:"portId,omitempty"` + SSHCertEligible bool `json:"sshCertEligible,omitempty"` + HealthStatus string `json:"healthStatus"` + IsStoppable bool `json:"isStoppable"` // used for autopstop only + StatusMessage string `json:"statusMessage"` + StopTimeout time.Duration `json:"stopTimeout"` + AdditionalUsers []string `json:"additionalUsers"` + Tunnel Tunnel `json:"tunnel"` } type APIKey struct { diff --git a/pkg/ssh/sshconfigurer.go b/pkg/ssh/sshconfigurer.go index 5d785625..f0367c48 100644 --- a/pkg/ssh/sshconfigurer.go +++ b/pkg/ssh/sshconfigurer.go @@ -5,14 +5,19 @@ import ( "encoding/xml" "fmt" "log" + "os" + "path/filepath" "regexp" "strings" "text/template" + "github.com/alessio/shellescape" + "github.com/brevdev/brev-cli/pkg/analytics" "github.com/brevdev/brev-cli/pkg/autostartconf" "github.com/brevdev/brev-cli/pkg/entity" breverrors "github.com/brevdev/brev-cli/pkg/errors" "github.com/brevdev/brev-cli/pkg/files" + "github.com/brevdev/brev-cli/pkg/sshcert" "github.com/brevdev/brev-cli/pkg/tasks" "github.com/hashicorp/go-multierror" ) @@ -201,6 +206,12 @@ func (s SSHConfigurerV2) Update(workspaces []entity.Workspace, nodes []ExternalN return breverrors.WrapAndTrace(err) } + // WSL/Windows config can't use certs because Match exec needs Linux Brev + // binary. Windows-native ssh will require Users manage keys + if isSSHCertRequired() { + return nil + } + // try to write wsl config wslConfig, err := s.CreateWSLConfig(workspaces) if err != nil { @@ -211,7 +222,6 @@ func (s SSHConfigurerV2) Update(workspaces []entity.Workspace, nodes []ExternalN return nil // not a fatal error // todo update sentry } - // todo ensure has include err = s.EnsureWSLConfigHasInclude() if err != nil { return breverrors.WrapAndTrace(err) @@ -238,7 +248,7 @@ func (s SSHConfigurerV2) CreateWSLConfig(workspaces []entity.Workspace) (string, return "", breverrors.WrapAndTrace(err) } - sshConfig, err := makeNewSSHConfig(toWindowsPath(configPath), workspaces, toWindowsPath(pkpath), toWindowsPath(cloudflaredBinaryPath)) + sshConfig, err := makeNewSSHConfig(toWindowsPath(configPath), workspaces, toWindowsPath(pkpath), toWindowsPath(cloudflaredBinaryPath), false) if err != nil { return "", breverrors.WrapAndTrace(err) } @@ -261,7 +271,7 @@ func (s SSHConfigurerV2) CreateNewSSHConfig(workspaces []entity.Workspace, nodes return "", breverrors.WrapAndTrace(err) } - sshConfig, err := makeNewSSHConfig(configPath, workspaces, pkPath, cloudflaredBinaryPath) + sshConfig, err := makeNewSSHConfig(configPath, workspaces, pkPath, cloudflaredBinaryPath, true) if err != nil { return "", breverrors.WrapAndTrace(err) } @@ -277,11 +287,11 @@ func (s SSHConfigurerV2) CreateNewSSHConfig(workspaces []entity.Workspace, nodes return sshConfig, nil } -func makeNewSSHConfig(configPath string, workspaces []entity.Workspace, pkpath string, cloudflaredBinaryPath string) (string, error) { +func makeNewSSHConfig(configPath string, workspaces []entity.Workspace, pkpath string, cloudflaredBinaryPath string, supportsCertHook bool) (string, error) { sshConfig := fmt.Sprintf("# included in %s\n", configPath) for _, w := range workspaces { - entry, err := makeSSHConfigEntryV2(w, pkpath, cloudflaredBinaryPath) + entry, err := makeSSHConfigEntryV2(w, pkpath, cloudflaredBinaryPath, supportsCertHook) if err != nil { return "", breverrors.WrapAndTrace(err) } @@ -312,6 +322,26 @@ const SSHConfigEntryTemplateV2 = `Host {{ .Alias }} {{ end }} ` +const SSHCertRequiredTemplateV2 = `Match host {{ .Alias }} exec {{ .ExecCommand }} + IdentityFile {{ .IdentityFile }} + User {{ .User }} + ProxyCommand {{ .ProxyCommand }} + ServerAliveInterval 30 + UserKnownHostsFile /dev/null + IdentitiesOnly yes + StrictHostKeyChecking no + PasswordAuthentication no + AddKeysToAgent yes + ForwardAgent yes + RequestTTY yes + ControlMaster auto + ControlPath ~/.ssh/brev-control-%C + ControlPersist 10m +{{ if .RunRemoteCMD }} + RemoteCommand cd {{ .Dir }}; $SHELL +{{ end }} +` + const SSHConfigEntryTemplateV3 = `Host {{ .Alias }} Hostname {{ .HostName }} IdentityFile {{ .IdentityFile }} @@ -333,6 +363,27 @@ const SSHConfigEntryTemplateV3 = `Host {{ .Alias }} {{ end }} ` +const SSHCertRequiredTemplateV3 = `Match host {{ .Alias }} exec {{ .ExecCommand }} + Hostname {{ .HostName }} + IdentityFile {{ .IdentityFile }} + User {{ .User }} + ServerAliveInterval 30 + UserKnownHostsFile /dev/null + IdentitiesOnly yes + StrictHostKeyChecking no + PasswordAuthentication no + AddKeysToAgent yes + ForwardAgent yes + RequestTTY yes + ControlMaster auto + ControlPath ~/.ssh/brev-control-%C + ControlPersist 10m + Port {{ .Port }} +{{ if .RunRemoteCMD }} + RemoteCommand cd {{ .Dir }}; $SHELL +{{ end }} +` + type SSHConfigEntryV2 struct { Alias string IdentityFile string @@ -342,6 +393,7 @@ type SSHConfigEntryV2 struct { RunRemoteCMD bool HostName string Port int + ExecCommand string // cert-required Match exec line (templates only) } func tmplAndValToString(tmpl *template.Template, val interface{}) (string, error) { @@ -353,12 +405,21 @@ func tmplAndValToString(tmpl *template.Template, val interface{}) (string, error return buf.String(), nil } -func makeSSHConfigEntryV2(workspace entity.Workspace, privateKeyPath string, cloudflaredBinaryPath string) (string, error) { //nolint:funlen,gocyclo // ok +var isSSHCertRequired = analytics.IsSSHCertRequired + +func makeSSHConfigEntryV2(workspace entity.Workspace, privateKeyPath string, cloudflaredBinaryPath string, supportsCertHook bool) (string, error) { //nolint:funlen,gocyclo // ok alias := string(workspace.GetLocalIdentifier()) + brevDir := filepath.Dir(privateKeyPath) privateKeyPath = "\"" + privateKeyPath + "\"" var sshVal string user := workspace.GetSSHUser() hostname := workspace.GetSSHHostname() + + certEligible := supportsCertHook && workspace.SSHCertEligible && workspace.PortID != "" + certRequired := certEligible && isSSHCertRequired() + certKeyPath := "\"" + sshcert.KeyPath(brevDir, workspace.ID) + "\"" + certExec := makeMintCertExecCommand(workspace.ID, workspace.PortID, user, sshcert.KeyPath(brevDir, workspace.ID)) + if workspace.SSHProxyHostname == "" { port := workspace.GetSSHPort() projPath, err := workspace.GetProjectFolderPath() @@ -373,7 +434,13 @@ func makeSSHConfigEntryV2(workspace entity.Workspace, privateKeyPath string, clo HostName: hostname, Port: port, } - tmpl, err := template.New(alias).Parse(SSHConfigEntryTemplateV3) + tmplStr := SSHConfigEntryTemplateV3 + if certRequired { + entry.IdentityFile = certKeyPath + entry.ExecCommand = certExec + tmplStr = SSHCertRequiredTemplateV3 + } + tmpl, err := template.New(alias).Parse(tmplStr) if err != nil { return "", breverrors.WrapAndTrace(err) } @@ -394,7 +461,13 @@ func makeSSHConfigEntryV2(workspace entity.Workspace, privateKeyPath string, clo ProxyCommand: proxyCommand, Dir: projPath, } - tmpl, err := template.New(alias).Parse(SSHConfigEntryTemplateV2) + tmplStr := SSHConfigEntryTemplateV2 + if certRequired { + entry.IdentityFile = certKeyPath + entry.ExecCommand = certExec + tmplStr = SSHCertRequiredTemplateV2 + } + tmpl, err := template.New(alias).Parse(tmplStr) if err != nil { return "", breverrors.WrapAndTrace(err) } @@ -454,6 +527,13 @@ func makeSSHConfigEntryV2(workspace entity.Workspace, privateKeyPath string, clo } val := fmt.Sprintf("%s%s", sshVal, hostSSHVal) + + if !certRequired { + // use cert but have static key as fallback + if certMatch := makeCertMatchEntry(workspace, brevDir, supportsCertHook); certMatch != "" { + val = certMatch + val + } + } return val, nil } @@ -461,6 +541,30 @@ func makeCloudflareSSHProxyCommand(cloudflaredBinaryPath string, hostname string return fmt.Sprintf("%s access ssh --hostname %s", cloudflaredBinaryPath, hostname) } +func makeCertMatchEntry(workspace entity.Workspace, brevDir string, supportsCertHook bool) string { + if !supportsCertHook || brevDir == "" || !workspace.SSHCertEligible || workspace.PortID == "" { + return "" + } + alias := string(workspace.GetLocalIdentifier()) + certKeyPath := sshcert.KeyPath(brevDir, workspace.ID) + exec := makeMintCertExecCommand(workspace.ID, workspace.PortID, workspace.GetSSHUser(), certKeyPath) + return fmt.Sprintf("Match host %s exec %q\n IdentityFile %q\n", alias, exec, certKeyPath) +} + +func makeMintCertExecCommand(envID, portID, linuxUser, outKey string) string { + brevBin, err := os.Executable() + if err != nil { + brevBin = "brev" + } + return shellescape.QuoteCommand([]string{ + brevBin, "mint-cert", + "--env", envID, + "--port", portID, + "--linux-user", linuxUser, + "--out-key", outKey, + }) +} + func (s SSHConfigurerV2) EnsureWSLConfigHasInclude() error { // openssh-7.3 @@ -537,27 +641,6 @@ func doesUserSSHConfigIncludeBrevConfig(conf string, brevConfigPath string) bool return false } -// Deprecated: var _ Config = SSHConfigurerServiceMesh{} - -// openssh-7.3 - -const SSHConfigEntryTemplateServiceMesh = `Host {{ .Alias }} - HostName {{ .Host }} - IdentityFile {{ .IdentityFile }} - User {{ .User }} - Port {{ .Port }} - ServerAliveInterval 30 - -` - -type SSHConfigEntryServiceMesh struct { - Alias string - Host string - IdentityFile string - User string - Port string -} - type SSHConfigurerJetBrains struct { store SSHConfigurerV2Store } diff --git a/pkg/ssh/sshconfigurer_test.go b/pkg/ssh/sshconfigurer_test.go index 4acb67f8..eb441fe9 100644 --- a/pkg/ssh/sshconfigurer_test.go +++ b/pkg/ssh/sshconfigurer_test.go @@ -1,8 +1,12 @@ package ssh import ( + "errors" "fmt" + "os" + "strings" "testing" + "text/template" "github.com/brevdev/brev-cli/pkg/entity" "github.com/brevdev/brev-cli/pkg/store" @@ -42,6 +46,18 @@ type DummyStore struct{} type DummySSHConfigurerV2Store struct{} +type noHomeSSHConfigurerV2Store struct { + DummySSHConfigurerV2Store +} + +func (noHomeSSHConfigurerV2Store) GetPrivateKeyPath() (string, error) { + return "/custom/brev-home/brev.pem", nil +} + +func (noHomeSSHConfigurerV2Store) UserHomeDir() (string, error) { + return "", errors.New("UserHomeDir should not be needed to locate Brev certificate files") +} + func (d DummySSHConfigurerV2Store) GetWSLHostUserSSHConfigPath() (string, error) { return "", nil } @@ -125,6 +141,10 @@ func (d DummySSHConfigurerV2Store) GetBrevCloudflaredBinaryPath() (string, error return "", nil } +func (d DummySSHConfigurerV2Store) UserHomeDir() (string, error) { + return "/home/test-user", nil +} + func TestCreateNewSSHConfig(t *testing.T) { c := NewSSHConfigurerV2(DummySSHConfigurerV2Store{}) cStr, err := c.CreateNewSSHConfig(somePlainWorkspaces, nil) @@ -216,6 +236,28 @@ Host %s-host assert.Equal(t, correct, cStr) } +func TestCreateNewSSHConfig_DerivesCertPathFromBrevDirectory(t *testing.T) { + w := entity.Workspace{ + ID: "env-cert", + Name: "cert-env", + Status: entity.Running, + SSHUser: "ubuntu", + SSHPort: 22, + SSHHostname: "10.0.0.1", + SSHCertEligible: true, + PortID: "port-1", + } + + c := NewSSHConfigurerV2(noHomeSSHConfigurerV2Store{}) + got, err := c.CreateNewSSHConfig([]entity.Workspace{w}, nil) + if err != nil { + t.Fatalf("CreateNewSSHConfig should not need UserHomeDir: %v", err) + } + if !strings.Contains(got, "/custom/brev-home/ssh-certs/env-cert") { + t.Fatalf("certificate path should be rooted in the Brev directory: %s", got) + } +} + func TestEnsureConfigHasInclude(t *testing.T) { c := NewSSHConfigurerV2(DummySSHConfigurerV2Store{}) @@ -512,7 +554,7 @@ Host testName2-host } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := makeSSHConfigEntryV2(tt.args.workspace, tt.args.privateKeyPath, tt.args.cloudflaredBinaryPath) + got, err := makeSSHConfigEntryV2(tt.args.workspace, tt.args.privateKeyPath, tt.args.cloudflaredBinaryPath, true) if (err != nil) != tt.wantErr { t.Errorf("makeSSHConfigEntryV2() error = %v, wantErr %v", err, tt.wantErr) return @@ -924,3 +966,154 @@ Host testName1-host }) } } + +func TestMakeCertMatchEntry_IneligibleWorkspace(t *testing.T) { + // No SSHCertEligible flag -> no Match block. + w := entity.Workspace{ID: "env-1", Name: "n", SSHUser: "u", PortID: "p"} + if got := makeCertMatchEntry(w, "/home/u/.brev", true); got != "" { + t.Errorf("ineligible workspace should produce no Match block, got: %s", got) + } + // Eligible but no PortID -> no Match block (can't mint without port_id). + w2 := entity.Workspace{ID: "env-1", Name: "n", SSHUser: "u", SSHCertEligible: true} + if got := makeCertMatchEntry(w2, "/home/u/.brev", true); got != "" { + t.Errorf("eligible without PortID should produce no Match block, got: %s", got) + } + // supportsCertHook=false (WSL/Windows config) -> no Match block even when eligible. + // The Windows config is consumed by Windows ssh, but the Match exec would + // invoke the WSL-Linux brev binary that Windows can't run; certs are deferred + // there and work via the POSIX config path inside WSL instead. + w3 := entity.Workspace{ID: "env-1", Name: "n", SSHUser: "u", SSHCertEligible: true, PortID: "p"} + if got := makeCertMatchEntry(w3, "/home/u/.brev", false); got != "" { + t.Errorf("WSL/Windows config should produce no Match block, got: %s", got) + } +} + +func TestMakeSSHConfigEntryV2_EligibleWorkspaceIncludesCertMatch(t *testing.T) { + w := entity.Workspace{ + ID: "env-cert", + Name: "cert-env", + Status: entity.Running, + SSHUser: "ubuntu", + SSHPort: 22, + SSHHostname: "10.0.0.1", + SSHCertEligible: true, + PortID: "port-1", + } + got, err := makeSSHConfigEntryV2(w, "/home/u/.brev/brev.pem", "/tmp/cf", true) + if err != nil { + t.Fatalf("makeSSHConfigEntryV2: %v", err) + } + // The Match block must precede the Host block. + matchIdx := strings.Index(got, "Match host cert-env exec") + hostIdx := strings.Index(got, "Host cert-env") + if matchIdx < 0 { + t.Fatal("expected Match block for cert-eligible workspace") + } + if hostIdx < 0 { + t.Fatal("expected Host block") + } + if matchIdx >= hostIdx { + t.Errorf("Match block must precede Host block (match=%d host=%d)", matchIdx, hostIdx) + } + if !strings.Contains(got, "/home/u/.brev/ssh-certs/env-cert") { + t.Error("missing cert key path in Match block") + } + if !strings.Contains(got, "/home/u/.brev/brev.pem") { + t.Error("missing static key path in Host block") + } +} + +func TestMakeCertMatchEntry_UsesAbsoluteBrevPath(t *testing.T) { + // The Match exec must invoke the absolute path to the running brev binary, + // not a bare `brev` that could resolve to a stale PATH binary. + w := entity.Workspace{ + ID: "env-abc", Name: "n", SSHUser: "ubuntu", + SSHCertEligible: true, PortID: "port-1", + } + got := makeCertMatchEntry(w, "/home/u/.brev", true) + exe, err := os.Executable() + if err != nil { + t.Skip("os.Executable unavailable; cannot assert path") + } + want := fmt.Sprintf("%s mint-cert --env env-abc", exe) + if !strings.Contains(got, want) { + t.Errorf("expected Match exec to use absolute brev path %q; got: %s", want, got) + } + if strings.Contains(got, " exec \"brev mint-cert") { + t.Errorf("Match exec must not use bare `brev`: %s", got) + } +} + +func TestSSHCertRequiredTemplateV3_MergesCertIntoMatchBlock(t *testing.T) { + entry := SSHConfigEntryV2{ + Alias: "my-env", + IdentityFile: `"/home/u/.brev/ssh-certs/env-1"`, + User: "ubuntu", + HostName: "10.0.0.5", + Port: 34828, + ExecCommand: `'/path/to/brev' mint-cert --env env-1 --port p --linux-user ubuntu --out-key /home/u/.brev/ssh-certs/env-1`, + } + tmpl, err := template.New("m").Parse(SSHCertRequiredTemplateV3) + if err != nil { + t.Fatal(err) + } + got, err := tmplAndValToString(tmpl, entry) + if err != nil { + t.Fatal(err) + } + if !strings.HasPrefix(got, "Match host my-env exec ") { + t.Errorf("expected Match block, got: %s", got) + } + if strings.Contains(got, "Host my-env\n") { + t.Errorf("cert-required mode must not emit a Host block: %s", got) + } + if !strings.Contains(got, "10.0.0.5") || !strings.Contains(got, "34828") { + t.Errorf("host settings must be in the Match block: %s", got) + } + if !strings.Contains(got, "/home/u/.brev/ssh-certs/env-1") { + t.Errorf("cert IdentityFile must be present: %s", got) + } + if strings.Contains(got, "brev.pem") { + t.Errorf("static key must be absent in cert-required mode: %s", got) + } +} + +type recordingWSLStore struct { + DummySSHConfigurerV2Store + wslWritten bool +} + +func (r *recordingWSLStore) WriteBrevSSHConfigWSL(_ string) error { + r.wslWritten = true + return nil +} + +func TestUpdate_SkipsWSLConfigWhenCertRequired(t *testing.T) { + orig := isSSHCertRequired + isSSHCertRequired = func() bool { return true } + t.Cleanup(func() { isSSHCertRequired = orig }) + + store := &recordingWSLStore{} + s := SSHConfigurerV2{store: store} + if err := s.Update(nil, nil); err != nil { + t.Fatalf("Update: %v", err) + } + if store.wslWritten { + t.Error("WriteBrevSSHConfigWSL must not be called when certs are required") + } +} + +func TestUpdate_WritesWSLConfigWhenCertNotRequired(t *testing.T) { + orig := isSSHCertRequired + isSSHCertRequired = func() bool { return false } + t.Cleanup(func() { isSSHCertRequired = orig }) + + store := &recordingWSLStore{} + s := SSHConfigurerV2{store: store} + if err := s.Update(nil, nil); err != nil { + t.Fatalf("Update: %v", err) + } + if !store.wslWritten { + t.Error("WriteBrevSSHConfigWSL should be called when certs are not required") + } +} diff --git a/pkg/sshcert/sshcert.go b/pkg/sshcert/sshcert.go new file mode 100644 index 00000000..f71573be --- /dev/null +++ b/pkg/sshcert/sshcert.go @@ -0,0 +1,173 @@ +// Package sshcert manages short-lived, per-environment SSH certificates and +// their backing ephemeral keypairs on disk for use by the OpenSSH client. +package sshcert + +import ( + "crypto/ed25519" + "crypto/rand" + "encoding/pem" + "fmt" + "os" + "path/filepath" + "strings" + "time" + + "github.com/spf13/afero" + "golang.org/x/crypto/ssh" + + breverrors "github.com/brevdev/brev-cli/pkg/errors" +) + +const certSubDir = "ssh-certs" + +// DefaultRenewalMargin is how long before expiry we renew, to avoid a race +// where the cert expires between mint and the subsequent ssh use. +const DefaultRenewalMargin = 60 * time.Second + +// Label constants mirroring dev-plane's internal/labels package +const ( + LabelKeySSHProvider = "sshprovider" + SSHProviderCertAuth = "certauth" +) + +func EnvironmentCertEligible(labels map[string]string) bool { + return labels[LabelKeySSHProvider] == SSHProviderCertAuth +} + +func Dir(brevDir string) string { + return filepath.Join(brevDir, certSubDir) +} + +func safeFilename(envID string) string { + s := strings.TrimSpace(envID) + if s == "" { + return "default" + } + var b strings.Builder + for _, r := range s { + switch { + case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9', r == '.', r == '_', r == '-': + b.WriteRune(r) + default: + b.WriteRune('-') + } + } + out := b.String() + if out == "" { + return "default" + } + return out +} + +func KeyPath(brevDir, envID string) string { + return filepath.Join(Dir(brevDir), safeFilename(envID)) +} + +// CertPath follows OpenSSH's -cert.pub convention, so a single +// IdentityFile directive loads both the key and the cert. +func CertPath(brevDir, envID string) string { + return KeyPath(brevDir, envID) + "-cert.pub" +} + +func GenerateKeyPair() (privKeyPEM []byte, pubKeyOpenSSH string, err error) { + pub, priv, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + return nil, "", breverrors.WrapAndTrace(err) + } + sshPubKey, err := ssh.NewPublicKey(pub) + if err != nil { + return nil, "", breverrors.WrapAndTrace(err) + } + pubKeyOpenSSH = string(ssh.MarshalAuthorizedKey(sshPubKey)) + block, err := ssh.MarshalPrivateKey(priv, "brev") + if err != nil { + return nil, "", breverrors.WrapAndTrace(err) + } + return pem.EncodeToMemory(block), pubKeyOpenSSH, nil +} + +func ParseCertificate(certOpenSSH string) (*ssh.Certificate, error) { + certOpenSSH = strings.TrimSpace(certOpenSSH) + if certOpenSSH == "" { + return nil, fmt.Errorf("certificate is empty") + } + pubKey, _, _, rest, err := ssh.ParseAuthorizedKey([]byte(certOpenSSH)) + if err != nil { + return nil, breverrors.WrapAndTrace(fmt.Errorf("parse certificate: %w", err)) + } + if len(strings.TrimSpace(string(rest))) != 0 { + return nil, fmt.Errorf("certificate has trailing data; expected exactly one key") + } + cert, ok := pubKey.(*ssh.Certificate) + if !ok { + return nil, fmt.Errorf("public key is not a certificate") + } + if cert.CertType != ssh.UserCert { + return nil, fmt.Errorf("certificate is not a user certificate (type=%d)", cert.CertType) + } + return cert, nil +} + +func CertValidAt(cert *ssh.Certificate, now time.Time, margin time.Duration) bool { + if cert == nil { + return false + } + notAfter := int64(cert.ValidBefore) + return now.Add(margin).Unix() < notAfter +} + +func HasValidCertAt(fs afero.Fs, certPath string, now time.Time, margin time.Duration) (bool, error) { + exists, err := afero.Exists(fs, certPath) + if err != nil { + return false, breverrors.WrapAndTrace(err) + } + if !exists { + return false, nil + } + certBytes, err := afero.ReadFile(fs, certPath) + if err != nil { + return false, breverrors.WrapAndTrace(err) + } + cert, err := ParseCertificate(string(certBytes)) + if err != nil { + return false, nil // corrupt cert -> mint fresh + } + return CertValidAt(cert, now, margin), nil +} + +func WriteFiles(fs afero.Fs, keyPath, certPath string, privKeyPEM []byte, certOpenSSH string) error { + if err := fs.MkdirAll(filepath.Dir(keyPath), 0o700); err != nil { + return breverrors.WrapAndTrace(err) + } + if err := writeAtomic(fs, keyPath, privKeyPEM, 0o600); err != nil { + return breverrors.WrapAndTrace(err) + } + if !strings.HasSuffix(certOpenSSH, "\n") { + certOpenSSH += "\n" + } + return writeAtomic(fs, certPath, []byte(certOpenSSH), 0o644) +} + +// writeAtomic renames a temp file in the same directory into place, so a +// reader never observes a partial write. +func writeAtomic(fs afero.Fs, path string, data []byte, mode os.FileMode) error { + dir := filepath.Dir(path) + tmp, err := afero.TempFile(fs, dir, ".brev-cert-*.tmp") + if err != nil { + return breverrors.WrapAndTrace(err) + } + tmpName := tmp.Name() + defer func() { _ = fs.Remove(tmpName) }() + + if _, err := tmp.Write(data); err != nil { + _ = tmp.Close() + return breverrors.WrapAndTrace(err) + } + if err := tmp.Close(); err != nil { + return breverrors.WrapAndTrace(err) + } + if err := fs.Chmod(tmpName, mode); err != nil { + return breverrors.WrapAndTrace(err) + } + return breverrors.WrapAndTrace(fs.Rename(tmpName, path)) +} diff --git a/pkg/sshcert/sshcert_test.go b/pkg/sshcert/sshcert_test.go new file mode 100644 index 00000000..beb8fb01 --- /dev/null +++ b/pkg/sshcert/sshcert_test.go @@ -0,0 +1,142 @@ +package sshcert + +import ( + "crypto/ed25519" + "crypto/rand" + "strings" + "testing" + "time" + + "github.com/spf13/afero" + "golang.org/x/crypto/ssh" +) + +func mintTestCert(t *testing.T, validBefore time.Time) string { + t.Helper() + _, privCA, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("generate ca: %v", err) + } + signer, err := ssh.NewSignerFromKey(privCA) + if err != nil { + t.Fatalf("new signer: %v", err) + } + pub, _, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("generate user key: %v", err) + } + sshPub, err := ssh.NewPublicKey(pub) + if err != nil { + t.Fatalf("new public key: %v", err) + } + cert := &ssh.Certificate{ + Key: sshPub, + Serial: 1, + CertType: ssh.UserCert, + KeyId: "test:user", + ValidPrincipals: []string{"brev:v1:vm:test-env:login:ubuntu"}, + ValidAfter: uint64(time.Now().Add(-time.Minute).Unix()), + ValidBefore: uint64(validBefore.Unix()), + Permissions: ssh.Permissions{Extensions: map[string]string{"permit-pty": ""}}, + } + if err := cert.SignCert(rand.Reader, signer); err != nil { + t.Fatalf("sign cert: %v", err) + } + return strings.TrimRight(string(ssh.MarshalAuthorizedKey(cert)), "\n") +} + +func TestParseCertificate(t *testing.T) { + cert, err := ParseCertificate(mintTestCert(t, time.Now().Add(10*time.Minute))) + if err != nil { + t.Fatalf("ParseCertificate: %v", err) + } + if cert.CertType != ssh.UserCert { + t.Errorf("expected user cert, got type %d", cert.CertType) + } + if len(cert.ValidPrincipals) != 1 || cert.ValidPrincipals[0] != "brev:v1:vm:test-env:login:ubuntu" { + t.Errorf("unexpected principals: %v", cert.ValidPrincipals) + } + for _, bad := range []string{"", "not a cert"} { + if _, err := ParseCertificate(bad); err == nil { + t.Errorf("expected error for %q", bad) + } + } +} + +func TestCertValidAt(t *testing.T) { + now := time.Now() + valid := &ssh.Certificate{ + ValidAfter: uint64(now.Add(-time.Hour).Unix()), + ValidBefore: uint64(now.Add(10 * time.Minute).Unix()), + } + if !CertValidAt(valid, now, time.Minute) { + t.Error("cert with 10m left should be valid with 1m margin") + } + if CertValidAt(valid, now, 10*time.Minute) { + t.Error("cert with 10m left should need renewal with 10m margin") + } + expired := &ssh.Certificate{ValidAfter: uint64(now.Add(-time.Hour).Unix()), ValidBefore: uint64(now.Add(-time.Minute).Unix())} + if CertValidAt(expired, now, time.Minute) { + t.Error("expired cert should not be valid") + } + zeroExpiry := &ssh.Certificate{ValidAfter: uint64(now.Add(-time.Hour).Unix()), ValidBefore: 0} + if CertValidAt(zeroExpiry, now, time.Minute) { + t.Error("zero-expiry cert should not be valid") + } + if CertValidAt(nil, now, time.Minute) { + t.Error("nil cert should not be valid") + } +} + +func TestHasValidCertAt(t *testing.T) { + fs := afero.NewMemMapFs() + certPath := CertPath("/home/u/.brev", "env-1") + + if ok, err := HasValidCertAt(fs, certPath, time.Now(), DefaultRenewalMargin); ok || err != nil { + t.Fatalf("missing cert: ok=%v err=%v", ok, err) + } + privPEM, _ := mustGen(t) + if err := WriteFiles(fs, KeyPath("/home/u/.brev", "env-1"), certPath, privPEM, mintTestCert(t, time.Now().Add(10*time.Minute))); err != nil { + t.Fatalf("WriteFiles: %v", err) + } + if ok, _ := HasValidCertAt(fs, certPath, time.Now(), DefaultRenewalMargin); !ok { + t.Error("expected valid after write") + } + if ok, _ := HasValidCertAt(fs, CertPath("/home/u/.brev", "env-2"), time.Now(), DefaultRenewalMargin); ok { + t.Error("env-2 should have no cert") + } + // Corrupt -> not valid, no error (mint fresh). + if err := afero.WriteFile(fs, certPath, []byte("garbage"), 0o644); err != nil { + t.Fatal(err) + } + if ok, err := HasValidCertAt(fs, certPath, time.Now(), DefaultRenewalMargin); ok || err != nil { + t.Errorf("corrupt cert: ok=%v err=%v (want false,nil)", ok, err) + } +} + +func TestWriteFiles_NoLeftoverTemp(t *testing.T) { + fs := afero.NewMemMapFs() + privPEM, _ := mustGen(t) + if err := WriteFiles(fs, KeyPath("/h/.brev", "x"), CertPath("/h/.brev", "x"), privPEM, mintTestCert(t, time.Now().Add(5*time.Minute))); err != nil { + t.Fatalf("WriteFiles: %v", err) + } + entries, _ := afero.ReadDir(fs, Dir("/h/.brev")) + for _, e := range entries { + if strings.HasPrefix(e.Name(), ".brev-cert-") { + t.Errorf("leftover temp file: %s", e.Name()) + } + } + b, _ := afero.ReadFile(fs, CertPath("/h/.brev", "x")) + if !strings.HasSuffix(string(b), "\n") { + t.Error("cert file should end with newline") + } +} + +func mustGen(t *testing.T) ([]byte, string) { + t.Helper() + priv, pub, err := GenerateKeyPair() + if err != nil { + t.Fatalf("GenerateKeyPair: %v", err) + } + return priv, pub +} From 1aff4f5acdf2d8d3f6c9cfd57bd462988cc01ac3 Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Tue, 25 Aug 2026 07:33:06 -0700 Subject: [PATCH 2/3] review feedback --- pkg/cmd/mintcert/mintcert.go | 36 +++++++++++++++------- pkg/cmd/mintcert/mintcert_test.go | 51 +++++++++++++++++++++++++------ pkg/ssh/sshconfigurer.go | 5 +-- pkg/ssh/sshconfigurer_test.go | 10 +++++- 4 files changed, 78 insertions(+), 24 deletions(-) diff --git a/pkg/cmd/mintcert/mintcert.go b/pkg/cmd/mintcert/mintcert.go index 2d62af00..dfde84d1 100644 --- a/pkg/cmd/mintcert/mintcert.go +++ b/pkg/cmd/mintcert/mintcert.go @@ -12,6 +12,7 @@ import ( devplanev1 "buf.build/gen/go/brevdev/devplane/protocolbuffers/go/devplaneapi/v1" "connectrpc.com/connect" + "github.com/brevdev/brev-cli/pkg/cmd/util" "github.com/spf13/afero" "github.com/spf13/cobra" @@ -25,6 +26,7 @@ import ( const timeout = 15 * time.Second type Store interface { + util.GetWorkspaceByNameOrIDErrStore GetAccessToken() (string, error) } @@ -74,14 +76,14 @@ func NewCmdMintCert(store Store) *cobra.Command { Hidden: true, RunE: func(cmd *cobra.Command, args []string) error { return runMintCert(store, mintCertRequest{ - EnvironmentID: env, - PortID: port, - LinuxUser: user, - OutKey: outKey, + NameOrID: env, + PortID: port, + LinuxUser: user, + OutKey: outKey, }) }, } - cmd.Flags().StringVar(&env, "env", "", "environment ID to mint a certificate for") + cmd.Flags().StringVar(&env, "env", "", "name or ID to mint a certificate for") cmd.Flags().StringVar(&port, "port", "", "network-member port ID for the SSH access") cmd.Flags().StringVar(&user, "linux-user", "", "Linux user for the certificate principal") cmd.Flags().StringVar(&outKey, "out-key", "", "private-key path (certificate goes to -cert.pub)") @@ -93,10 +95,10 @@ func NewCmdMintCert(store Store) *cobra.Command { } type mintCertRequest struct { - EnvironmentID string - PortID string - LinuxUser string - OutKey string + NameOrID string + PortID string + LinuxUser string + OutKey string } func runMintCert(store Store, req mintCertRequest) error { @@ -112,7 +114,19 @@ func runMintCertWith(store Store, fs afero.Fs, issuer CertIssuer, req mintCertRe } return fmt.Errorf("not logged in") } - _ = token + target, err := util.ResolveWorkspaceOrNode(store, req.NameOrID) + if err != nil { + return breverrors.WrapAndTrace(err) + } + var targetId string + if target.Workspace != nil { + targetId = target.Workspace.ID + } + if target.Node != nil { + // TODO uptake for external node + return breverrors.New("registered compute not yet supported for SSH Certs") + } + certPath := req.OutKey + "-cert.pub" if ok, err := sshcert.HasValidCertAt(fs, certPath, time.Now(), sshcert.DefaultRenewalMargin); err != nil { _, _ = fmt.Fprintf(os.Stderr, "brev: failed to check cached cert: %v\n", err) @@ -128,7 +142,7 @@ func runMintCertWith(store Store, fs afero.Fs, issuer CertIssuer, req mintCertRe ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() cert, err := issuer.Issue(ctx, certIssueRequest{ - EnvironmentID: req.EnvironmentID, + EnvironmentID: targetId, PortID: req.PortID, LinuxUser: req.LinuxUser, PublicKey: pubKeyOpenSSH, diff --git a/pkg/cmd/mintcert/mintcert_test.go b/pkg/cmd/mintcert/mintcert_test.go index 9aaf5973..78e301d0 100644 --- a/pkg/cmd/mintcert/mintcert_test.go +++ b/pkg/cmd/mintcert/mintcert_test.go @@ -11,6 +11,7 @@ import ( devplanev1 "buf.build/gen/go/brevdev/devplane/protocolbuffers/go/devplaneapi/v1" "connectrpc.com/connect" + "github.com/brevdev/brev-cli/pkg/entity" "github.com/spf13/afero" "golang.org/x/crypto/ssh" @@ -18,8 +19,11 @@ import ( ) type fakeStore struct { - token string - err error + token string + org *entity.Organization + err error + workspaces []entity.Workspace + user *entity.User } func (f fakeStore) GetAccessToken() (string, error) { @@ -29,6 +33,22 @@ func (f fakeStore) GetAccessToken() (string, error) { return f.token, nil } +func (f fakeStore) GetActiveOrganizationOrDefault() (*entity.Organization, error) { + return f.org, nil +} + +func (f fakeStore) GetAuthTokens() (*entity.AuthTokens, error) { + return nil, nil +} + +func (f fakeStore) GetCurrentUser() (*entity.User, error) { + return f.user, nil +} + +func (f fakeStore) GetWorkspaceByNameOrID(_ string, _ string) ([]entity.Workspace, error) { + return f.workspaces, nil +} + type certIssuerFunc struct { fn func(context.Context, certIssueRequest) (string, error) } @@ -81,14 +101,25 @@ func mintCertForTest(t *testing.T, pubKeyOpenSSH string) string { return strings.TrimRight(string(ssh.MarshalAuthorizedKey(cert)), "\n") } +func testStore(token string) fakeStore { + return fakeStore{ + token: token, + user: &entity.User{ID: "user-1"}, + org: &entity.Organization{ID: "org-1"}, + workspaces: []entity.Workspace{ + {ID: "env-1", CreatedByUserID: "user-1"}, + }, + } +} + func TestRunMintCert_MintsAndWrites(t *testing.T) { fs := afero.NewMemMapFs() outKey := "/home/u/.brev/ssh-certs/env-1" issuer := &certIssuerFunc{fn: func(_ context.Context, req certIssueRequest) (string, error) { return mintCertForTest(t, req.PublicKey), nil }} - if err := runMintCertWith(fakeStore{token: "tok"}, fs, issuer, mintCertRequest{ - EnvironmentID: "env-1", PortID: "port-1", LinuxUser: "ubuntu", OutKey: outKey, + if err := runMintCertWith(testStore("tok"), fs, issuer, mintCertRequest{ + NameOrID: "env-1", PortID: "port-1", LinuxUser: "ubuntu", OutKey: outKey, }); err != nil { t.Fatalf("runMintCertWith: %v", err) } @@ -121,8 +152,8 @@ func TestRunMintCert_ReusesCachedCert(t *testing.T) { t.Error("issuer should not be called when cache is valid") return "", nil }} - if err := runMintCertWith(fakeStore{token: "tok"}, fs, issuer, mintCertRequest{ - EnvironmentID: "env-1", PortID: "port-1", LinuxUser: "ubuntu", OutKey: outKey, + if err := runMintCertWith(testStore("tok"), fs, issuer, mintCertRequest{ + NameOrID: "env-1", PortID: "port-1", LinuxUser: "ubuntu", OutKey: outKey, }); err != nil { t.Fatalf("expected reuse, got err: %v", err) } @@ -133,8 +164,8 @@ func TestRunMintCert_FallsBackOnIssueError(t *testing.T) { issuer := &certIssuerFunc{fn: func(_ context.Context, _ certIssueRequest) (string, error) { return "", errors.New("CA unavailable") }} - err := runMintCertWith(fakeStore{token: "tok"}, fs, issuer, mintCertRequest{ - EnvironmentID: "env-1", PortID: "port-1", LinuxUser: "ubuntu", + err := runMintCertWith(testStore("tok"), fs, issuer, mintCertRequest{ + NameOrID: "env-1", PortID: "port-1", LinuxUser: "ubuntu", OutKey: "/home/u/.brev/ssh-certs/env-1", }) if err == nil { @@ -153,14 +184,14 @@ func TestRunMintCert_FallsBackOnAuthError(t *testing.T) { }} // GetAccessToken error -> auth failure (no prompt, fall back to brev.pem). if err := runMintCertWith(fakeStore{err: errors.New("no token")}, fs, issuer, mintCertRequest{ - EnvironmentID: "env-1", PortID: "port-1", LinuxUser: "ubuntu", + NameOrID: "env-1", PortID: "port-1", LinuxUser: "ubuntu", OutKey: "/home/u/.brev/ssh-certs/env-1", }); err == nil { t.Fatal("expected error on auth failure") } // Empty token (noLoginCmdStore returns "") -> auth failure, NOT a prompt. if err := runMintCertWith(fakeStore{token: ""}, fs, issuer, mintCertRequest{ - EnvironmentID: "env-1", PortID: "port-1", LinuxUser: "ubuntu", + NameOrID: "env-1", PortID: "port-1", LinuxUser: "ubuntu", OutKey: "/home/u/.brev/ssh-certs/env-1", }); err == nil { t.Fatal("expected error on empty token (must not prompt)") diff --git a/pkg/ssh/sshconfigurer.go b/pkg/ssh/sshconfigurer.go index f0367c48..e9ba5ec5 100644 --- a/pkg/ssh/sshconfigurer.go +++ b/pkg/ssh/sshconfigurer.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "regexp" + "strconv" "strings" "text/template" @@ -437,7 +438,7 @@ func makeSSHConfigEntryV2(workspace entity.Workspace, privateKeyPath string, clo tmplStr := SSHConfigEntryTemplateV3 if certRequired { entry.IdentityFile = certKeyPath - entry.ExecCommand = certExec + entry.ExecCommand = strconv.Quote(certExec) tmplStr = SSHCertRequiredTemplateV3 } tmpl, err := template.New(alias).Parse(tmplStr) @@ -464,7 +465,7 @@ func makeSSHConfigEntryV2(workspace entity.Workspace, privateKeyPath string, clo tmplStr := SSHConfigEntryTemplateV2 if certRequired { entry.IdentityFile = certKeyPath - entry.ExecCommand = certExec + entry.ExecCommand = strconv.Quote(certExec) tmplStr = SSHCertRequiredTemplateV2 } tmpl, err := template.New(alias).Parse(tmplStr) diff --git a/pkg/ssh/sshconfigurer_test.go b/pkg/ssh/sshconfigurer_test.go index eb441fe9..86f8add2 100644 --- a/pkg/ssh/sshconfigurer_test.go +++ b/pkg/ssh/sshconfigurer_test.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "os" + "strconv" "strings" "testing" "text/template" @@ -1045,13 +1046,14 @@ func TestMakeCertMatchEntry_UsesAbsoluteBrevPath(t *testing.T) { } func TestSSHCertRequiredTemplateV3_MergesCertIntoMatchBlock(t *testing.T) { + rawExec := `'/path/to/brev' mint-cert --env env-1 --port p --linux-user ubuntu --out-key /home/u/.brev/ssh-certs/env-1` entry := SSHConfigEntryV2{ Alias: "my-env", IdentityFile: `"/home/u/.brev/ssh-certs/env-1"`, User: "ubuntu", HostName: "10.0.0.5", Port: 34828, - ExecCommand: `'/path/to/brev' mint-cert --env env-1 --port p --linux-user ubuntu --out-key /home/u/.brev/ssh-certs/env-1`, + ExecCommand: strconv.Quote(rawExec), } tmpl, err := template.New("m").Parse(SSHCertRequiredTemplateV3) if err != nil { @@ -1064,6 +1066,12 @@ func TestSSHCertRequiredTemplateV3_MergesCertIntoMatchBlock(t *testing.T) { if !strings.HasPrefix(got, "Match host my-env exec ") { t.Errorf("expected Match block, got: %s", got) } + // The exec value must be a single quoted token; OpenSSH rejects bare + // whitespace-separated commands in Match exec. + execLine := "Match host my-env exec " + strconv.Quote(rawExec) + if !strings.Contains(got, execLine) { + t.Errorf("Match exec must quote the full command:\nwant: %s\ngot: %s", execLine, got) + } if strings.Contains(got, "Host my-env\n") { t.Errorf("cert-required mode must not emit a Host block: %s", got) } From 6c22364163c1895044b99eff7be568c7e7f9dd2f Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Tue, 25 Aug 2026 09:39:30 -0700 Subject: [PATCH 3/3] review feedback --- pkg/cmd/mintcert/mintcert.go | 4 +- pkg/cmd/mintcert/mintcert_test.go | 40 +++++++++-- pkg/ssh/sshconfigurer.go | 6 +- pkg/sshcert/sshcert.go | 34 +++++++-- pkg/sshcert/sshcert_test.go | 116 +++++++++++++++++++++++++++--- 5 files changed, 176 insertions(+), 24 deletions(-) diff --git a/pkg/cmd/mintcert/mintcert.go b/pkg/cmd/mintcert/mintcert.go index dfde84d1..926d0a55 100644 --- a/pkg/cmd/mintcert/mintcert.go +++ b/pkg/cmd/mintcert/mintcert.go @@ -128,12 +128,14 @@ func runMintCertWith(store Store, fs afero.Fs, issuer CertIssuer, req mintCertRe } certPath := req.OutKey + "-cert.pub" - if ok, err := sshcert.HasValidCertAt(fs, certPath, time.Now(), sshcert.DefaultRenewalMargin); err != nil { + + if ok, err := sshcert.HasValidCertAuth(fs, req.OutKey, certPath, time.Now(), sshcert.DefaultRenewalMargin); err != nil { _, _ = fmt.Fprintf(os.Stderr, "brev: failed to check cached cert: %v\n", err) return breverrors.WrapAndTrace(err) } else if ok { return nil } + privKeyPEM, pubKeyOpenSSH, err := sshcert.GenerateKeyPair() if err != nil { _, _ = fmt.Fprintf(os.Stderr, "brev: failed to generate keypair: %v\n", err) diff --git a/pkg/cmd/mintcert/mintcert_test.go b/pkg/cmd/mintcert/mintcert_test.go index 78e301d0..d8e04820 100644 --- a/pkg/cmd/mintcert/mintcert_test.go +++ b/pkg/cmd/mintcert/mintcert_test.go @@ -128,7 +128,7 @@ func TestRunMintCert_MintsAndWrites(t *testing.T) { t.Errorf("not written: %s", p) } } - if ok, err := sshcert.HasValidCertAt(fs, outKey+"-cert.pub", time.Now(), 0); err != nil || !ok { + if ok, err := sshcert.HasValidCertAuth(fs, outKey, outKey+"-cert.pub", time.Now(), 0); err != nil || !ok { t.Errorf("written cert not valid: ok=%v err=%v", ok, err) } privBytes, err := afero.ReadFile(fs, outKey) @@ -144,8 +144,13 @@ func TestRunMintCert_MintsAndWrites(t *testing.T) { func TestRunMintCert_ReusesCachedCert(t *testing.T) { fs := afero.NewMemMapFs() outKey := "/home/u/.brev/ssh-certs/env-1" - _, pub, _ := sshcert.GenerateKeyPair() - if err := sshcert.WriteFiles(fs, outKey, outKey+"-cert.pub", []byte("priv"), mintCertForTest(t, pub)); err != nil { + // Seed a matching keypair: the private key must correspond to the public + // key bound into the certificate. + privPEM, pub, err := sshcert.GenerateKeyPair() + if err != nil { + t.Fatalf("generate keypair: %v", err) + } + if err := sshcert.WriteFiles(fs, outKey, outKey+"-cert.pub", privPEM, mintCertForTest(t, pub)); err != nil { t.Fatalf("seed: %v", err) } issuer := &certIssuerFunc{fn: func(_ context.Context, _ certIssueRequest) (string, error) { @@ -159,6 +164,33 @@ func TestRunMintCert_ReusesCachedCert(t *testing.T) { } } +func TestRunMintCert_RemintsOnMismatchedKey(t *testing.T) { + // A cert paired with a wrong private key must be detected and re-minted, + // not silently reused. + fs := afero.NewMemMapFs() + outKey := "/home/u/.brev/ssh-certs/env-1" + _, pub, _ := sshcert.GenerateKeyPair() + if err := sshcert.WriteFiles(fs, outKey, outKey+"-cert.pub", []byte("priv"), mintCertForTest(t, pub)); err != nil { + t.Fatalf("seed: %v", err) + } + minted := false + issuer := &certIssuerFunc{fn: func(_ context.Context, req certIssueRequest) (string, error) { + minted = true + if req.EnvironmentID != "env-1" { + t.Errorf("unexpected EnvironmentID: %s", req.EnvironmentID) + } + return mintCertForTest(t, req.PublicKey), nil + }} + if err := runMintCertWith(testStore("tok"), fs, issuer, mintCertRequest{ + NameOrID: "env-1", PortID: "port-1", LinuxUser: "ubuntu", OutKey: outKey, + }); err != nil { + t.Fatalf("runMintCertWith: %v", err) + } + if !minted { + t.Fatal("expected re-mint when key/cert mismatch") + } +} + func TestRunMintCert_FallsBackOnIssueError(t *testing.T) { fs := afero.NewMemMapFs() issuer := &certIssuerFunc{fn: func(_ context.Context, _ certIssueRequest) (string, error) { @@ -190,7 +222,7 @@ func TestRunMintCert_FallsBackOnAuthError(t *testing.T) { t.Fatal("expected error on auth failure") } // Empty token (noLoginCmdStore returns "") -> auth failure, NOT a prompt. - if err := runMintCertWith(fakeStore{token: ""}, fs, issuer, mintCertRequest{ + if err := runMintCertWith(testStore(""), fs, issuer, mintCertRequest{ NameOrID: "env-1", PortID: "port-1", LinuxUser: "ubuntu", OutKey: "/home/u/.brev/ssh-certs/env-1", }); err == nil { diff --git a/pkg/ssh/sshconfigurer.go b/pkg/ssh/sshconfigurer.go index e9ba5ec5..a488938a 100644 --- a/pkg/ssh/sshconfigurer.go +++ b/pkg/ssh/sshconfigurer.go @@ -36,6 +36,10 @@ var ( collapseHyphensRe = regexp.MustCompile(`-{2,}`) ) +const ( + WindowsSupport = false // cannot execute mint-cert on Windows +) + // SanitizeNodeName converts a node display name into a valid SSH Host alias. func SanitizeNodeName(name string) string { s := strings.ToLower(name) @@ -249,7 +253,7 @@ func (s SSHConfigurerV2) CreateWSLConfig(workspaces []entity.Workspace) (string, return "", breverrors.WrapAndTrace(err) } - sshConfig, err := makeNewSSHConfig(toWindowsPath(configPath), workspaces, toWindowsPath(pkpath), toWindowsPath(cloudflaredBinaryPath), false) + sshConfig, err := makeNewSSHConfig(toWindowsPath(configPath), workspaces, toWindowsPath(pkpath), toWindowsPath(cloudflaredBinaryPath), WindowsSupport) if err != nil { return "", breverrors.WrapAndTrace(err) } diff --git a/pkg/sshcert/sshcert.go b/pkg/sshcert/sshcert.go index f71573be..84d8342b 100644 --- a/pkg/sshcert/sshcert.go +++ b/pkg/sshcert/sshcert.go @@ -3,6 +3,7 @@ package sshcert import ( + "bytes" "crypto/ed25519" "crypto/rand" "encoding/pem" @@ -116,23 +117,42 @@ func CertValidAt(cert *ssh.Certificate, now time.Time, margin time.Duration) boo return now.Add(margin).Unix() < notAfter } -func HasValidCertAt(fs afero.Fs, certPath string, now time.Time, margin time.Duration) (bool, error) { - exists, err := afero.Exists(fs, certPath) +func HasValidCertAuth(fs afero.Fs, keyPath, certPath string, now time.Time, margin time.Duration) (bool, error) { + certBytes, err := afero.ReadFile(fs, certPath) if err != nil { + if os.IsNotExist(err) { + return false, nil + } return false, breverrors.WrapAndTrace(err) } - if !exists { + cert, err := ParseCertificate(string(certBytes)) + if err != nil { + return false, nil + } + if !CertValidAt(cert, now, margin) { return false, nil } - certBytes, err := afero.ReadFile(fs, certPath) + + keyBytes, err := afero.ReadFile(fs, keyPath) if err != nil { + if os.IsNotExist(err) { + return false, nil + } return false, breverrors.WrapAndTrace(err) } - cert, err := ParseCertificate(string(certBytes)) + signer, err := ssh.ParsePrivateKey(keyBytes) if err != nil { - return false, nil // corrupt cert -> mint fresh + return false, nil + } + + if !PublicKeyMatches(signer.PublicKey(), cert.Key) { + return false, nil } - return CertValidAt(cert, now, margin), nil + return true, nil +} + +func PublicKeyMatches(a, b ssh.PublicKey) bool { + return bytes.Equal(a.Marshal(), b.Marshal()) } func WriteFiles(fs afero.Fs, keyPath, certPath string, privKeyPEM []byte, certOpenSSH string) error { diff --git a/pkg/sshcert/sshcert_test.go b/pkg/sshcert/sshcert_test.go index beb8fb01..5e7299b0 100644 --- a/pkg/sshcert/sshcert_test.go +++ b/pkg/sshcert/sshcert_test.go @@ -3,6 +3,7 @@ package sshcert import ( "crypto/ed25519" "crypto/rand" + "encoding/pem" "strings" "testing" "time" @@ -45,6 +46,46 @@ func mintTestCert(t *testing.T, validBefore time.Time) string { return strings.TrimRight(string(ssh.MarshalAuthorizedKey(cert)), "\n") } +// mintTestCertWithKey generates a fresh keypair and returns the private key PEM +// alongside a certificate bound to that key's public key. +func mintTestCertWithKey(t *testing.T, validBefore time.Time) ([]byte, string) { + t.Helper() + _, privCA, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("generate ca: %v", err) + } + signer, err := ssh.NewSignerFromKey(privCA) + if err != nil { + t.Fatalf("new signer: %v", err) + } + pub, priv, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("generate user key: %v", err) + } + sshPub, err := ssh.NewPublicKey(pub) + if err != nil { + t.Fatalf("new public key: %v", err) + } + cert := &ssh.Certificate{ + Key: sshPub, + Serial: 1, + CertType: ssh.UserCert, + KeyId: "test:user", + ValidPrincipals: []string{"brev:v1:vm:test-env:login:ubuntu"}, + ValidAfter: uint64(time.Now().Add(-time.Minute).Unix()), + ValidBefore: uint64(validBefore.Unix()), + Permissions: ssh.Permissions{Extensions: map[string]string{"permit-pty": ""}}, + } + if err := cert.SignCert(rand.Reader, signer); err != nil { + t.Fatalf("sign cert: %v", err) + } + block, err := ssh.MarshalPrivateKey(priv, "brev") + if err != nil { + t.Fatalf("marshal private key: %v", err) + } + return pem.EncodeToMemory(block), strings.TrimRight(string(ssh.MarshalAuthorizedKey(cert)), "\n") +} + func TestParseCertificate(t *testing.T) { cert, err := ParseCertificate(mintTestCert(t, time.Now().Add(10*time.Minute))) if err != nil { @@ -88,30 +129,83 @@ func TestCertValidAt(t *testing.T) { } } -func TestHasValidCertAt(t *testing.T) { +func TestHasValidSSHAuth(t *testing.T) { //nolint:gocyclo // test fs := afero.NewMemMapFs() + keyPath := KeyPath("/home/u/.brev", "env-1") certPath := CertPath("/home/u/.brev", "env-1") - if ok, err := HasValidCertAt(fs, certPath, time.Now(), DefaultRenewalMargin); ok || err != nil { - t.Fatalf("missing cert: ok=%v err=%v", ok, err) + // No files -> not valid. + if ok, err := HasValidCertAuth(fs, keyPath, certPath, time.Now(), DefaultRenewalMargin); ok || err != nil { + t.Fatalf("empty: ok=%v err=%v", ok, err) } - privPEM, _ := mustGen(t) - if err := WriteFiles(fs, KeyPath("/home/u/.brev", "env-1"), certPath, privPEM, mintTestCert(t, time.Now().Add(10*time.Minute))); err != nil { + + // Valid matching keypair -> valid. + privPEM, cert := mintTestCertWithKey(t, time.Now().Add(10*time.Minute)) + if err := WriteFiles(fs, keyPath, certPath, privPEM, cert); err != nil { t.Fatalf("WriteFiles: %v", err) } - if ok, _ := HasValidCertAt(fs, certPath, time.Now(), DefaultRenewalMargin); !ok { - t.Error("expected valid after write") + if ok, err := HasValidCertAuth(fs, keyPath, certPath, time.Now(), DefaultRenewalMargin); !ok || err != nil { + t.Fatalf("valid pair: ok=%v err=%v", ok, err) + } + + // Missing private key -> not valid (cert alone is insufficient). + _ = fs.Remove(keyPath) + if ok, err := HasValidCertAuth(fs, keyPath, certPath, time.Now(), DefaultRenewalMargin); ok || err != nil { + t.Errorf("missing key: ok=%v err=%v (want false,nil)", ok, err) + } + + // Restore key, corrupt it -> not valid. + if err := afero.WriteFile(fs, keyPath, []byte("garbage"), 0o600); err != nil { + t.Fatal(err) + } + if ok, err := HasValidCertAuth(fs, keyPath, certPath, time.Now(), DefaultRenewalMargin); ok || err != nil { + t.Errorf("corrupt key: ok=%v err=%v (want false,nil)", ok, err) + } + + // Mismatched key (valid but wrong key) -> not valid. + otherPriv, _ := mustGen(t) + if err := afero.WriteFile(fs, keyPath, otherPriv, 0o600); err != nil { + t.Fatal(err) } - if ok, _ := HasValidCertAt(fs, CertPath("/home/u/.brev", "env-2"), time.Now(), DefaultRenewalMargin); ok { - t.Error("env-2 should have no cert") + if ok, err := HasValidCertAuth(fs, keyPath, certPath, time.Now(), DefaultRenewalMargin); ok || err != nil { + t.Errorf("mismatched key: ok=%v err=%v (want false,nil)", ok, err) } - // Corrupt -> not valid, no error (mint fresh). + + // Corrupt cert -> not valid (mint fresh). if err := afero.WriteFile(fs, certPath, []byte("garbage"), 0o644); err != nil { t.Fatal(err) } - if ok, err := HasValidCertAt(fs, certPath, time.Now(), DefaultRenewalMargin); ok || err != nil { + if ok, err := HasValidCertAuth(fs, keyPath, certPath, time.Now(), DefaultRenewalMargin); ok || err != nil { t.Errorf("corrupt cert: ok=%v err=%v (want false,nil)", ok, err) } + + // Expired cert -> not valid. + expiredPriv, expiredCert := mintTestCertWithKey(t, time.Now().Add(-time.Minute)) + if err := WriteFiles(fs, keyPath, certPath, expiredPriv, expiredCert); err != nil { + t.Fatalf("WriteFiles expired: %v", err) + } + if ok, err := HasValidCertAuth(fs, keyPath, certPath, time.Now(), DefaultRenewalMargin); ok || err != nil { + t.Errorf("expired: ok=%v err=%v (want false,nil)", ok, err) + } +} + +func TestPublicKeyMatches(t *testing.T) { + _, pub1 := mustGen(t) + _, pub2 := mustGen(t) + k1, _, _, _, err := ssh.ParseAuthorizedKey([]byte(pub1)) + if err != nil { + t.Fatal(err) + } + k2, _, _, _, err := ssh.ParseAuthorizedKey([]byte(pub2)) + if err != nil { + t.Fatal(err) + } + if !PublicKeyMatches(k1, k1) { + t.Error("same key should match") + } + if PublicKeyMatches(k1, k2) { + t.Error("different keys should not match") + } } func TestWriteFiles_NoLeftoverTemp(t *testing.T) {