From eb1840a3a88e073ad2309ff2bd2e6d9383ec6305 Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Wed, 19 Aug 2026 16:45:47 -0700 Subject: [PATCH 01/27] 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). --- pkg/sshcert/sshcert.go | 269 ++++++++++++++++++++++++++++++++++++ pkg/sshcert/sshcert_test.go | 254 ++++++++++++++++++++++++++++++++++ 2 files changed, 523 insertions(+) create mode 100644 pkg/sshcert/sshcert.go create mode 100644 pkg/sshcert/sshcert_test.go diff --git a/pkg/sshcert/sshcert.go b/pkg/sshcert/sshcert.go new file mode 100644 index 00000000..70027dd2 --- /dev/null +++ b/pkg/sshcert/sshcert.go @@ -0,0 +1,269 @@ +// Package sshcert manages short-lived, per-environment SSH certificates and +// their backing ephemeral keypairs on disk for use by the OpenSSH client. +// +// Design notes (see the SSH-cert design discussion): +// +// - A fresh ed25519 keypair is generated each time a certificate is renewed. +// Keypair generation is effectively free, so reusing a key across renewals +// would save nothing while reintroducing a long-lived private key on disk +// (the exact property we are leaving static keys to escape). Instead, the +// private key's exposure window is bounded by the certificate's own validity +// window. The (private key, certificate) pair is cached on disk for the +// certificate's lifetime so repeated `ssh` invocations do not re-hit the CA. +// +// - Files live under ~/.brev/ssh-certs/{,-cert.pub}. OpenSSH +// auto-loads -cert.pub as the certificate, so a single +// IdentityFile directive in the ssh config covers both key and cert. +// +// - Writes are atomic (temp file in the same directory + rename) and the +// private key is written with mode 0600. +// +// This package is deliberately independent of the certificate-issuance RPC so +// the rest of the SSH-cert feature can build and be tested before the +// generated connect client for IssueEnvironmentSSHCertificate is published. +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" +) + +// Subdirectory under the brev home directory where per-environment keypairs and +// certificates are cached. +const certSubDir = "ssh-certs" + +// DefaultRenewalMargin is how long before a certificate's not-after time we +// consider it expired and in need of renewal. Renewing slightly early avoids a +// race where a certificate expires between the mint and the subsequent ssh use. +const DefaultRenewalMargin = 60 * time.Second + +// Label constants mirroring dev-plane's internal/labels package. They are +// duplicated here because that package is internal to dev-plane. Keep these in +// sync with dev-plane/internal/labels. +const ( + LabelKeySSHProvider = "sshprovider" + SSHProviderCertAuth = "certauth" +) + +// EnvironmentCertEligible reports whether an environment's labels opt it into +// certificate-based SSH auth. Non-eligible (e.g. older) environments fall back +// to the existing static key. +func EnvironmentCertEligible(labels map[string]string) bool { + return labels[LabelKeySSHProvider] == SSHProviderCertAuth +} + +// Dir returns the on-disk directory holding cached certificates for the given +// home directory. +func Dir(home string) string { + return filepath.Join(home, ".brev", certSubDir) +} + +// safeFilename reduces an environment ID to something safe to use as a file +// name. Environment IDs are UUIDs today, so this is mostly defensive. +func safeFilename(envID string) string { + s := strings.TrimSpace(envID) + if s == "" { + s = "default" + } + // Replace anything that isn't [A-Za-z0-9._-] with '-'. + 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 == "" { + out = "default" + } + return out +} + +// KeyPath returns the on-disk path of the private key for the given environment. +func KeyPath(home, envID string) string { + return filepath.Join(Dir(home), safeFilename(envID)) +} + +// CertPath returns the on-disk path of the certificate for the given environment. +// This follows OpenSSH's -cert.pub convention so a single +// IdentityFile directive loads both the key and the certificate. +func CertPath(home, envID string) string { + return KeyPath(home, envID) + "-cert.pub" +} + +// GenerateKeyPair generates a fresh ed25519 keypair suitable for certificate +// issuance. It returns the private key in OpenSSH PEM format (ready to write +// to disk and use as an IdentityFile) and the public key as a single-line +// OpenSSH authorized-key string (the format the certificate-issuance RPC +// expects as its public_key field). +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) + } + // MarshalAuthorizedKey produces "ssh-ed25519 AAAA... comment\n"; the CA + // requires exactly one line with no options, so trim the trailing newline. + pubKeyOpenSSH = strings.TrimRight(string(ssh.MarshalAuthorizedKey(sshPubKey)), "\n") + + block, err := ssh.MarshalPrivateKey(priv, "brev") + if err != nil { + return nil, "", breverrors.WrapAndTrace(err) + } + privKeyPEM = pem.EncodeToMemory(block) + return privKeyPEM, pubKeyOpenSSH, nil +} + +// ParseCertificate parses an OpenSSH authorized-key-formatted certificate +// string (the format returned by the issuance RPC) into an *ssh.Certificate. +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 +} + +// CertValidAt reports whether the certificate is valid at now+margin (i.e. not +// yet close enough to expiry to require renewal). ValidBefore is a unix +// timestamp; a value of 0 or ^uint64(0) means "forever" per the SSH spec. +func CertValidAt(cert *ssh.Certificate, now time.Time, margin time.Duration) bool { + if cert == nil { + return false + } + notBefore := int64(cert.ValidAfter) + notAfter := int64(cert.ValidBefore) + if notAfter == 0 || notAfter == -1 { + // "forever" — still bounded by ValidAfter. + return now.Add(margin).Unix() >= notBefore + } + return now.Add(margin).Unix() < notAfter +} + +// Store reads and writes cached certificates and their backing keypairs to a +// filesystem (afero is used so the logic is unit-testable). +type Store struct { + fs afero.Fs + home string +} + +// NewStore returns a Store rooted at home using the given filesystem. Use +// files.AppFs (the OS filesystem) in production and an afero.MemMapFs in tests. +func NewStore(fs afero.Fs, home string) *Store { + return &Store{fs: fs, home: home} +} + +// KeyPath returns the on-disk private-key path for the given environment. +func (s *Store) KeyPath(envID string) string { return KeyPath(s.home, envID) } + +// CertPath returns the on-disk certificate path for the given environment. +func (s *Store) CertPath(envID string) string { return CertPath(s.home, envID) } + +// HasValidCert reports whether a non-expired certificate (with margin) for the +// given environment is already present on disk. A missing file, unparseable +// certificate, or one within the renewal margin of expiry returns (false, nil). +// A genuine I/O error is returned. +func (s *Store) HasValidCert(envID string, now time.Time, margin time.Duration) (bool, error) { + exists, err := afero.Exists(s.fs, s.CertPath(envID)) + if err != nil { + return false, breverrors.WrapAndTrace(err) + } + if !exists { + return false, nil + } + certBytes, err := afero.ReadFile(s.fs, s.CertPath(envID)) + if err != nil { + return false, breverrors.WrapAndTrace(err) + } + cert, err := ParseCertificate(string(certBytes)) + if err != nil { + // A corrupt cert on disk is treated as "no valid cert" so the caller + // will mint a fresh one rather than failing the whole ssh attempt. + return false, nil + } + return CertValidAt(cert, now, margin), nil +} + +// Write writes the private key and certificate for the given environment to +// disk atomically (temp file + rename within the same directory). The private +// key is written with mode 0600; the certificate with 0644. +func (s *Store) Write(envID string, privKeyPEM []byte, certOpenSSH string) error { + dir := Dir(s.home) + if err := s.fs.MkdirAll(dir, 0o700); err != nil { + return breverrors.WrapAndTrace(err) + } + + if err := writeAtomic(s.fs, s.KeyPath(envID), privKeyPEM, 0o600); err != nil { + return breverrors.WrapAndTrace(err) + } + + // Ensure the certificate ends with a newline for OpenSSH's reader. + if !strings.HasSuffix(certOpenSSH, "\n") { + certOpenSSH += "\n" + } + if err := writeAtomic(s.fs, s.CertPath(envID), []byte(certOpenSSH), 0o644); err != nil { + return breverrors.WrapAndTrace(err) + } + return nil +} + +// writeAtomic writes data to path via a temp file in the same directory and +// renames it into place. Renaming within the same directory is atomic on POSIX +// filesystems, so a reader never observes a partially-written file. +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() + // Clean up the temp file if anything below fails. + 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) + } + if err := fs.Rename(tmpName, path); err != nil { + return breverrors.WrapAndTrace(err) + } + return nil +} diff --git a/pkg/sshcert/sshcert_test.go b/pkg/sshcert/sshcert_test.go new file mode 100644 index 00000000..b202ef6b --- /dev/null +++ b/pkg/sshcert/sshcert_test.go @@ -0,0 +1,254 @@ +package sshcert + +import ( + "bytes" + "crypto/ed25519" + "crypto/rand" + "strings" + "testing" + "time" + + "github.com/spf13/afero" + "golang.org/x/crypto/ssh" +) + +// mintTestCert mints a real user certificate signed by an in-memory CA, so the +// parse/cache logic is exercised against genuine ssh.Certificate objects. +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) + } + + // User keypair (the "client" key the cert is issued over). + 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 TestGenerateKeyPair_Format(t *testing.T) { + privPEM, pubOpenSSH, err := GenerateKeyPair() + if err != nil { + t.Fatalf("GenerateKeyPair: %v", err) + } + if !bytes.HasPrefix(privPEM, []byte("-----BEGIN OPENSSH PRIVATE KEY-----")) { + t.Errorf("private key is not OpenSSH PEM; got %q", privPEM[:40]) + } + if !strings.HasPrefix(pubOpenSSH, "ssh-ed25519 ") { + t.Errorf("public key not ssh-ed25519; got %q", pubOpenSSH) + } + if strings.ContainsAny(pubOpenSSH, "\r\n") { + t.Errorf("public key must be a single line; got %q", pubOpenSSH) + } + // The public key must be parseable as an authorized key with no options. + _, _, options, rest, err := ssh.ParseAuthorizedKey([]byte(pubOpenSSH)) + if err != nil { + t.Fatalf("ParseAuthorizedKey: %v", err) + } + if len(options) != 0 || len(bytes.TrimSpace(rest)) != 0 { + t.Errorf("public key has options or trailing data; options=%v rest=%q", options, rest) + } + // The private key must be parseable back. + signer, err := ssh.ParsePrivateKey(privPEM) + if err != nil { + t.Fatalf("ParsePrivateKey: %v", err) + } + if signer.PublicKey().Type() != ssh.KeyAlgoED25519 { + t.Errorf("expected ed25519 signer, got %s", signer.PublicKey().Type()) + } +} + +func TestParseCertificate(t *testing.T) { + certStr := mintTestCert(t, time.Now().Add(10*time.Minute)) + cert, err := ParseCertificate(certStr) + 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) + } + + if _, err := ParseCertificate(""); err == nil { + t.Error("expected error for empty cert") + } + if _, err := ParseCertificate("not a cert"); err == nil { + t.Error("expected error for garbage") + } +} + +func TestCertValidAt(t *testing.T) { + now := time.Now() + cert := &ssh.Certificate{ + ValidAfter: uint64(now.Add(-time.Hour).Unix()), + ValidBefore: uint64(now.Add(10 * time.Minute).Unix()), + } + if !CertValidAt(cert, now, time.Minute) { + t.Error("cert valid for 10 more minutes should be valid with 1m margin") + } + // 5 minutes left, 10 minute margin -> needs renewal. + if CertValidAt(cert, now, 10*time.Minute) { + t.Error("cert with 5m left should need renewal with 10m margin") + } + // Already expired. + cert.ValidBefore = uint64(now.Add(-time.Minute).Unix()) + if CertValidAt(cert, now, time.Minute) { + t.Error("expired cert should not be valid") + } + // Forever cert (ValidBefore == 0) bounded only by ValidAfter. + forever := &ssh.Certificate{ValidAfter: uint64(now.Add(-time.Hour).Unix()), ValidBefore: 0} + if !CertValidAt(forever, now, time.Minute) { + t.Error("forever cert within ValidAfter should be valid") + } + if CertValidAt(nil, now, time.Minute) { + t.Error("nil cert should not be valid") + } +} + +func TestStore_WriteAndHasValidCert(t *testing.T) { + fs := afero.NewMemMapFs() + store := NewStore(fs, "/home/user") + + // No cert yet. + ok, err := store.HasValidCert("env-1", time.Now(), DefaultRenewalMargin) + if err != nil { + t.Fatalf("HasValidCert on empty: %v", err) + } + if ok { + t.Error("expected no valid cert initially") + } + + // Generate a keypair and mint a cert valid for 10 minutes. + privPEM, _, err := GenerateKeyPair() + if err != nil { + t.Fatalf("GenerateKeyPair: %v", err) + } + certStr := mintTestCert(t, time.Now().Add(10*time.Minute)) + + if err := store.Write("env-1", privPEM, certStr); err != nil { + t.Fatalf("Write: %v", err) + } + + // Paths must follow the -cert.pub convention. + if got := store.KeyPath("env-1"); !strings.HasSuffix(got, "ssh-certs/env-1") { + t.Errorf("unexpected key path: %s", got) + } + if got := store.CertPath("env-1"); got != store.KeyPath("env-1")+"-cert.pub" { + t.Errorf("cert path must be key path + -cert.pub: %s", got) + } + + // Cert on disk is valid. + ok, err = store.HasValidCert("env-1", time.Now(), DefaultRenewalMargin) + if err != nil { + t.Fatalf("HasValidCert after write: %v", err) + } + if !ok { + t.Error("expected valid cert after write") + } + + // A different env has no cert. + ok, err = store.HasValidCert("env-2", time.Now(), DefaultRenewalMargin) + if err != nil { + t.Fatalf("HasValidCert env-2: %v", err) + } + if ok { + t.Error("env-2 should have no cert") + } + + // Corrupt cert on disk is treated as "no valid cert" (not an error). + if err := afero.WriteFile(fs, store.CertPath("env-1"), []byte("garbage"), 0o644); err != nil { + t.Fatalf("write corrupt: %v", err) + } + ok, err = store.HasValidCert("env-1", time.Now(), DefaultRenewalMargin) + if err != nil { + t.Fatalf("HasValidCert corrupt: %v", err) + } + if ok { + t.Error("corrupt cert should not be considered valid") + } +} + +func TestStore_WriteIsAtomic(t *testing.T) { + fs := afero.NewMemMapFs() + store := NewStore(fs, "/home/user") + privPEM, _, err := GenerateKeyPair() + if err != nil { + t.Fatalf("GenerateKeyPair: %v", err) + } + certStr := mintTestCert(t, time.Now().Add(5*time.Minute)) + if err := store.Write("env-x", privPEM, certStr); err != nil { + t.Fatalf("Write: %v", err) + } + // No leftover temp files in the cert dir. + entries, err := afero.ReadDir(fs, Dir("/home/user")) + if err != nil { + t.Fatalf("ReadDir: %v", err) + } + for _, e := range entries { + if strings.HasPrefix(e.Name(), ".brev-cert-") { + t.Errorf("leftover temp file: %s", e.Name()) + } + } + // Cert file ends with a newline. + b, err := afero.ReadFile(fs, store.CertPath("env-x")) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if !strings.HasSuffix(string(b), "\n") { + t.Error("cert file should end with newline") + } +} + +func TestEnvironmentCertEligible(t *testing.T) { + if !EnvironmentCertEligible(map[string]string{"sshprovider": "certauth"}) { + t.Error("certauth label should be eligible") + } + if EnvironmentCertEligible(map[string]string{"sshprovider": "other"}) { + t.Error("non-certauth label should not be eligible") + } + if EnvironmentCertEligible(map[string]string{}) { + t.Error("missing label should not be eligible") + } +} + +func TestSafeFilename(t *testing.T) { + if got := safeFilename("env_123"); got != "env_123" { + t.Errorf("safeFilename(env_123)=%s", got) + } + if got := safeFilename("env/evil"); got != "env-evil" { + t.Errorf("safeFilename(env/evil)=%s", got) + } + if got := safeFilename(""); got != "default" { + t.Errorf("safeFilename('')=%s", got) + } +} From 2095277f2377de46e0f3991f149877ec26cab4e0 Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Wed, 19 Aug 2026 16:46:42 -0700 Subject: [PATCH 02/27] feat(refresh): retain port_id and cert-eligibility on workspace resolveWorkspaceSSH already fetches the user's SSHAccess (carrying port_id + linux_user) and the environment labels from the Environment connect API during refresh, but discarded port_id after resolving the network port. Retain port_id and the sshprovider=certauth label on the workspace so the SSH config generator can emit a per-environment certificate-fetch entry. Fields stay zero-valued for environments that fall back to static-key auth. --- pkg/cmd/refresh/sshaccess.go | 9 ++++++++ pkg/cmd/refresh/sshaccess_test.go | 35 +++++++++++++++++++++++++++++++ pkg/entity/entity.go | 9 ++++++++ 3 files changed, 53 insertions(+) diff --git a/pkg/cmd/refresh/sshaccess.go b/pkg/cmd/refresh/sshaccess.go index aad5c3e9..fef248e5 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 @@ -117,6 +118,14 @@ func resolveWorkspaceSSH( workspace.SSHUser = access.GetLinuxUser() workspace.SSHProxyHostname = "" + // Retain the port ID and certificate-eligibility label so the SSH config + // generator can emit a per-environment certificate-fetch (Match exec) entry. + // These are only populated when SSH access was resolved via the Environment + // connect API; otherwise they stay zero-valued and the config falls back to + // the static key. + 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 != "" { workspace.HostSSHHostname = 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..e731cb62 100644 --- a/pkg/entity/entity.go +++ b/pkg/entity/entity.go @@ -294,6 +294,15 @@ type Workspace struct { HostSSHProxyHostname string `json:"hostSshProxyHostname"` VerbBuildStatus VerbBuildStatus `json:"verbBuildStatus"` VerbYaml string `json:"verbYaml"` + // PortID is the network-member port ID for this user's SSH access to the + // environment, resolved from the Environment connect API during refresh. + // It is required to issue an SSH certificate and is empty for environments + // that fall back to static-key auth (e.g. created before cert support). + PortID string `json:"portId,omitempty"` + // SSHCertEligible is true when the environment's labels opt it into + // certificate-based SSH auth (sshprovider=certauth). When false the SSH + // config falls back to the static brev.pem identity. + SSHCertEligible bool `json:"sshCertEligible,omitempty"` // PrimaryApplicationId string `json:"primaryApplicationId,omitempty"` // LastOnlineAt string `json:"lastOnlineAt,omitempty"` // CreatedAt string `json:"createdAt,omitempty"` From 4702aa909894197faa7f6ea62b5ae83f77ccc217 Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Wed, 19 Aug 2026 17:00:10 -0700 Subject: [PATCH 03/27] feat(shell): add brev shell --cert-only for Match exec cert minting Headless, non-interactive mode invoked by the ssh config's Match exec hook. Reuses the existing platform credential (no login prompt), checks the on-disk cert cache, mints a fresh ephemeral ed25519 keypair + short-lived certificate via a CertIssuer, and atomically writes them to the --out-key path (+ -cert.pub). On any failure it writes nothing and returns non-zero so ssh drops the Match IdentityFile and falls back to the static brev.pem. CertIssuer is an interface; today a stub returns ErrCertIssuanceUnavailable (the real connect-RPC issuer drops in once the buf module publishes IssueEnvironmentSSHCertificate). Flags are hidden since this is an implementation detail of the ssh config, not a user-facing mode. --- pkg/cmd/shell/certonly.go | 220 ++++++++++++++++++++++++++++++++ pkg/cmd/shell/certonly_test.go | 225 +++++++++++++++++++++++++++++++++ pkg/cmd/shell/shell.go | 13 ++ 3 files changed, 458 insertions(+) create mode 100644 pkg/cmd/shell/certonly.go create mode 100644 pkg/cmd/shell/certonly_test.go diff --git a/pkg/cmd/shell/certonly.go b/pkg/cmd/shell/certonly.go new file mode 100644 index 00000000..229637fd --- /dev/null +++ b/pkg/cmd/shell/certonly.go @@ -0,0 +1,220 @@ +package shell + +import ( + "context" + "fmt" + "os" + "strings" + "time" + + "github.com/spf13/afero" + "github.com/spf13/cobra" + + "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" +) + +// certOnlyTimeout bounds how long the --cert-only helper will wait for a single +// certificate issuance. The Match exec hook runs synchronously during ssh +// config evaluation, so this bounds the worst-case delay ssh sees before a +// login. Generous enough to survive a slow CA but short enough that ssh +// doesn't appear to hang. +const certOnlyTimeout = 15 * time.Second + +// certOnlyRequest carries the parameters the helper needs to mint a +// certificate for a specific environment. They are baked into the ssh config's +// Match exec line at config-generation time, so the helper never has to +// resolve %h back to an environment on its own. +type certOnlyRequest struct { + EnvironmentID string + PortID string + LinuxUser string + OutKey string // absolute path to write the private key (cert goes to -cert.pub) +} + +// CertIssuer mints a short-lived SSH certificate for a public key. +// +// The real implementation calls dev-plane's IssueEnvironmentSSHCertificate RPC. +// Until the generated connect client for that RPC is published in the buf +// module brev-cli depends on, a stub implementation returns ErrCertIssuanceUnavailable +// so the rest of the feature compiles and is unit-testable. When the stub is +// active, --cert-only writes no files and the ssh config falls back to the +// static brev.pem identity (see the Match exec design in sshconfigurer.go). +type CertIssuer interface { + Issue(ctx context.Context, req certIssueRequest) (certIssueResult, error) +} + +type certIssueRequest struct { + EnvironmentID string + PortID string + LinuxUser string + PublicKey string // single-line OpenSSH authorized key format +} + +type certIssueResult struct { + Certificate string // single-line OpenSSH authorized key format (the signed cert) +} + +// ErrCertIssuanceUnavailable is returned by the stub CertIssuer to signal that +// cert issuance is not yet wired in this build. The --cert-only helper treats +// this (and any error) as "mint failed; fall back to static key". +var ErrCertIssuanceUnavailable = fmt.Errorf("ssh certificate issuance is not available in this build") + +// stubCertIssuer is the placeholder CertIssuer used until the real connect-RPC +// client is wired. It always returns ErrCertIssuanceUnavailable. +type stubCertIssuer struct{} + +func (stubCertIssuer) Issue(_ context.Context, _ certIssueRequest) (certIssueResult, error) { + return certIssueResult{}, ErrCertIssuanceUnavailable +} + +// newCertIssuer constructs the CertIssuer appropriate for this build. Today +// that is the stub; once the buf module publishes IssueEnvironmentSSHCertificate, +// this returns a real connect-RPC-backed issuer. +func newCertIssuer(_ externalnode.TokenProvider, _ string) CertIssuer { + return stubCertIssuer{} +} + +// certOnlyStore is the minimal store dependency of runCertOnly: just the +// ability to read the existing platform credential. Keeping this narrow lets +// --cert-only be unit-tested with a trivial fake store. +type certOnlyStore interface { + GetAccessToken() (string, error) +} + +// runCertOnly implements `brev shell --cert-only`. It is a headless, non-interactive +// mint-and-write used by the ssh config's Match exec hook: it ensures a valid +// cached certificate exists at req.OutKey (+ "-cert.pub"), minting a fresh one +// if needed, and exits 0 on success. +// +// On any failure (auth unavailable, CA error, write error) it prints a short +// human-actionable message to stderr and exits non-zero WITHOUT writing files, +// so the Match block's IdentityFile is dropped and ssh falls back to the +// static brev.pem identity. It never prompts and never blocks on an interactive +// login — those would hang the ssh invocation. +func runCertOnly(store ShellStore, req certOnlyRequest) error { + return runCertOnlyWith(store, afero.NewOsFs(), newCertIssuer(store, config.GlobalConfig.GetBrevPublicAPIURL()), req) +} + +// runCertOnlyWith is the testable form of runCertOnly with injectable deps. +func runCertOnlyWith(store certOnlyStore, fs afero.Fs, issuer CertIssuer, req certOnlyRequest) error { + // Reuse the existing platform credential (API key / token) the CLI already + // stores. No new login here — this is the whole point of not prompting. + if _, err := store.GetAccessToken(); err != nil { + fmt.Fprintln(os.Stderr, "brev: not logged in. Run `brev login` and retry.") + return breverrors.WrapAndTrace(err) + } + + certPath := req.OutKey + "-cert.pub" + + // Cache check: if a non-expired cert is already on disk, reuse it and exit 0. + // This keeps repeated `ssh` from hitting the CA on every connection. + ok, err := sshcert.HasValidCertAt(fs, certPath, time.Now(), sshcert.DefaultRenewalMargin) + if err != nil { + fmt.Fprintf(os.Stderr, "brev: failed to check cached cert: %v\n", err) + return breverrors.WrapAndTrace(err) + } + if ok { + return nil + } + + // Generate a fresh ephemeral keypair. The private key never leaves this + // machine; only the public key is sent to the CA for signing. + 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(), certOnlyTimeout) + defer cancel() + + res, err := issuer.Issue(ctx, certIssueRequest{ + EnvironmentID: req.EnvironmentID, + PortID: req.PortID, + LinuxUser: req.LinuxUser, + PublicKey: pubKeyOpenSSH, + }) + if err != nil { + // This is the fallback path: print a concise message and return an + // error so the caller can exit non-zero. The ssh config's Match block + // then drops its IdentityFile and ssh uses the static brev.pem. + 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, res.Certificate); err != nil { + fmt.Fprintf(os.Stderr, "brev: failed to write cert files: %v\n", err) + return breverrors.WrapAndTrace(err) + } + return nil +} + +// certOnlyFlags holds the parsed --cert-only flag values. +type certOnlyFlags struct { + certOnly bool + env string + port string + user string + outKey string +} + +// addCertOnlyFlags registers the --cert-only family of flags on the shell +// command. They are hidden from help since they are an implementation detail of +// the ssh config's Match exec hook, not a user-facing mode. +func addCertOnlyFlags(cmd *cobra.Command, f *certOnlyFlags) { + cmd.Flags().BoolVar(&f.certOnly, "cert-only", false, "mint an SSH certificate and write it to disk, then exit (used by the ssh config Match exec hook)") + cmd.Flags().StringVar(&f.env, "env", "", "(--cert-only) environment ID to mint a certificate for") + cmd.Flags().StringVar(&f.port, "port", "", "(--cert-only) network-member port ID for the SSH access") + cmd.Flags().StringVar(&f.user, "user", "", "(--cert-only) linux user for the certificate principal") + cmd.Flags().StringVar(&f.outKey, "out-key", "", "(--cert-only) absolute path to write the private key (certificate goes to -cert.pub)") + + for _, name := range []string{"cert-only", "env", "port", "user", "out-key"} { + _ = cmd.Flags().MarkHidden(name) + } +} + +// validateCertOnly returns an error if the --cert-only flags are inconsistent +// or incomplete. When --cert-only is not set, all of its flags must be unset. +func validateCertOnly(f certOnlyFlags) error { + if !f.certOnly { + // cert-only flags must not be set without --cert-only. + var set []string + if f.env != "" { + set = append(set, "--env") + } + if f.port != "" { + set = append(set, "--port") + } + if f.user != "" { + set = append(set, "--user") + } + if f.outKey != "" { + set = append(set, "--out-key") + } + if len(set) > 0 { + return fmt.Errorf("flags %s require --cert-only", strings.Join(set, ", ")) + } + return nil + } + // --cert-only mode: all four parameters are required. + var missing []string + if f.env == "" { + missing = append(missing, "--env") + } + if f.port == "" { + missing = append(missing, "--port") + } + if f.user == "" { + missing = append(missing, "--user") + } + if f.outKey == "" { + missing = append(missing, "--out-key") + } + if len(missing) > 0 { + return fmt.Errorf("--cert-only requires %s", strings.Join(missing, ", ")) + } + return nil +} diff --git a/pkg/cmd/shell/certonly_test.go b/pkg/cmd/shell/certonly_test.go new file mode 100644 index 00000000..dfa6ce50 --- /dev/null +++ b/pkg/cmd/shell/certonly_test.go @@ -0,0 +1,225 @@ +package shell + +import ( + "context" + "crypto/ed25519" + "crypto/rand" + "errors" + "strings" + "testing" + "time" + + "github.com/spf13/afero" + "golang.org/x/crypto/ssh" + + "github.com/brevdev/brev-cli/pkg/sshcert" +) + +// fakeShellStore satisfies the GetAccessToken subset of ShellStore for --cert-only. +type fakeShellStore struct { + token string + err error +} + +func (f fakeShellStore) GetAccessToken() (string, error) { + if f.err != nil { + return "", f.err + } + return f.token, nil +} + +// fakeIssuer is a controllable CertIssuer for tests. +type fakeIssuer struct { + cert string + err error + got certIssueRequest +} + +func (f *fakeIssuer) Issue(_ context.Context, req certIssueRequest) (certIssueResult, error) { + f.got = req + if f.err != nil { + return certIssueResult{}, f.err + } + return certIssueResult{Certificate: f.cert}, nil +} + +// mintCertForTest mints a real, signed user certificate over an in-memory CA +// for the given public key, so the on-disk cert parses as a valid ssh.Certificate. +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 TestRunCertOnly_MintsAndWrites(t *testing.T) { + fs := afero.NewMemMapFs() + store := fakeShellStore{token: "tok"} + outKey := "/home/u/.brev/ssh-certs/env-1" + + // The fake issuer needs to return a cert that parses; mint one over the + // pub key the helper will generate. We capture the pub key by issuing once. + var issued bool + issuer := &fakeIssuer{} + issuer.err = nil + issuer.cert = "" // will be set on first call using the real pub key + + // Wrap so we can mint a cert over whatever pub key the helper generated. + wrapped := &certIssuerFunc{fn: func(_ context.Context, req certIssueRequest) (certIssueResult, error) { + issued = true + return certIssueResult{Certificate: mintCertForTest(t, req.PublicKey)}, nil + }} + + err := runCertOnlyWith(store, fs, wrapped, certOnlyRequest{ + EnvironmentID: "env-1", + PortID: "port-1", + LinuxUser: "ubuntu", + OutKey: outKey, + }) + if err != nil { + t.Fatalf("runCertOnlyWith: %v", err) + } + if !issued { + t.Error("expected issuer to be called") + } + + // Files must exist. + exists, _ := afero.Exists(fs, outKey) + if !exists { + t.Error("private key not written") + } + exists, _ = afero.Exists(fs, outKey+"-cert.pub") + if !exists { + t.Error("cert not written") + } + // Cert must parse. + ok, err := sshcert.HasValidCertAt(fs, outKey+"-cert.pub", time.Now(), 0) + if err != nil || !ok { + t.Errorf("written cert not valid: ok=%v err=%v", ok, err) + } +} + +func TestRunCertOnly_ReusesCachedCert(t *testing.T) { + fs := afero.NewMemMapFs() + store := fakeShellStore{token: "tok"} + outKey := "/home/u/.brev/ssh-certs/env-1" + + // Pre-write a valid cert. + _, pubKeyOpenSSH, _ := sshcert.GenerateKeyPair() + certStr := mintCertForTest(t, pubKeyOpenSSH) + if err := sshcert.WriteFiles(fs, outKey, outKey+"-cert.pub", []byte("priv"), certStr); err != nil { + t.Fatalf("seed: %v", err) + } + + issuer := &certIssuerFunc{fn: func(_ context.Context, _ certIssueRequest) (certIssueResult, error) { + t.Error("issuer should NOT be called when cache is valid") + return certIssueResult{}, nil + }} + + if err := runCertOnlyWith(store, fs, issuer, certOnlyRequest{ + EnvironmentID: "env-1", PortID: "port-1", LinuxUser: "ubuntu", OutKey: outKey, + }); err != nil { + t.Fatalf("expected reuse, got err: %v", err) + } +} + +func TestRunCertOnly_FallsBackOnIssueError(t *testing.T) { + fs := afero.NewMemMapFs() + store := fakeShellStore{token: "tok"} + issuer := &certIssuerFunc{fn: func(_ context.Context, _ certIssueRequest) (certIssueResult, error) { + return certIssueResult{}, errors.New("CA unavailable") + }} + + err := runCertOnlyWith(store, fs, issuer, certOnlyRequest{ + 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") + } + // No files must be written on fallback. + exists, _ := afero.Exists(fs, "/home/u/.brev/ssh-certs/env-1") + if exists { + t.Error("private key should not be written on issue failure") + } +} + +func TestRunCertOnly_FallsBackOnAuthError(t *testing.T) { + fs := afero.NewMemMapFs() + store := fakeShellStore{err: errors.New("no token")} + issuer := &certIssuerFunc{fn: func(_ context.Context, _ certIssueRequest) (certIssueResult, error) { + t.Error("issuer should not be called when not authenticated") + return certIssueResult{}, nil + }} + + err := runCertOnlyWith(store, fs, issuer, certOnlyRequest{ + EnvironmentID: "env-1", PortID: "port-1", LinuxUser: "ubuntu", + OutKey: "/home/u/.brev/ssh-certs/env-1", + }) + if err == nil { + t.Fatal("expected error on auth failure") + } +} + +func TestRunCertOnly_StubIssuerReturnsUnavailable(t *testing.T) { + // The stub issuer that ships today always returns ErrCertIssuanceUnavailable, + // so --cert-only falls back to the static key until the real issuer is wired. + _, err := stubCertIssuer{}.Issue(context.Background(), certIssueRequest{}) + if err == nil { + t.Fatal("expected ErrCertIssuanceUnavailable from stub") + } + if !errors.Is(err, ErrCertIssuanceUnavailable) { + t.Fatalf("expected ErrCertIssuanceUnavailable, got %v", err) + } +} + +func TestValidateCertOnly(t *testing.T) { + // Not cert-only, no flags -> ok. + if err := validateCertOnly(certOnlyFlags{}); err != nil { + t.Errorf("empty flags should be valid: %v", err) + } + // --cert-only requires all four. + if err := validateCertOnly(certOnlyFlags{certOnly: true}); err == nil { + t.Error("cert-only without params should error") + } + // All four set -> ok. + if err := validateCertOnly(certOnlyFlags{certOnly: true, env: "e", port: "p", user: "u", outKey: "/k"}); err != nil { + t.Errorf("complete cert-only should be valid: %v", err) + } + // Params without --cert-only -> error. + if err := validateCertOnly(certOnlyFlags{env: "e"}); err == nil { + t.Error("params without cert-only should error") + } +} + +// certIssuerFunc adapts a function into a CertIssuer for tests. +type certIssuerFunc struct { + fn func(context.Context, certIssueRequest) (certIssueResult, error) +} + +func (c *certIssuerFunc) Issue(ctx context.Context, req certIssueRequest) (certIssueResult, error) { + return c.fn(ctx, req) +} diff --git a/pkg/cmd/shell/shell.go b/pkg/cmd/shell/shell.go index a9760719..4f775fc6 100644 --- a/pkg/cmd/shell/shell.go +++ b/pkg/cmd/shell/shell.go @@ -53,6 +53,7 @@ type ShellStore interface { func NewCmdShell(t *terminal.Terminal, store ShellStore, noLoginStartStore ShellStore) *cobra.Command { var host bool + var certFlags certOnlyFlags cmd := &cobra.Command{ Annotations: map[string]string{"access": ""}, Use: "shell ", @@ -64,6 +65,17 @@ func NewCmdShell(t *terminal.Terminal, store ShellStore, noLoginStartStore Shell Args: cobra.ExactArgs(1), ValidArgsFunction: completions.GetAllWorkspaceNameCompletionHandler(noLoginStartStore, t), RunE: func(cmd *cobra.Command, args []string) error { + if err := validateCertOnly(certFlags); err != nil { + return breverrors.WrapAndTrace(err) + } + if certFlags.certOnly { + return runCertOnly(store, certOnlyRequest{ + EnvironmentID: certFlags.env, + PortID: certFlags.port, + LinuxUser: certFlags.user, + OutKey: certFlags.outKey, + }) + } instanceName := args[0] err := runShellCommand(t, store, instanceName, host) if err != nil { @@ -73,6 +85,7 @@ func NewCmdShell(t *terminal.Terminal, store ShellStore, noLoginStartStore Shell }, } cmd.Flags().BoolVarP(&host, "host", "", false, "ssh into the host machine instead of the container") + addCertOnlyFlags(cmd, &certFlags) return cmd } From bee70117e8d51b4588996c14cb968eaa8af0f1aa Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Wed, 19 Aug 2026 17:02:10 -0700 Subject: [PATCH 04/27] feat(ssh): emit Match exec cert block for cert-eligible workspaces For each cert-eligible workspace (sshprovider=certauth label + port_id), the SSH config generator now prepends a Match host exec block before the existing Host block. The Match block runs 'brev shell --cert-only' to mint a short-lived cert and carries the cert IdentityFile; the Host block keeps the static brev.pem IdentityFile. OpenSSH accumulates IdentityFile across Match and Host blocks when the exec succeeds (cert tried first, static as fallback), and drops the Match block's IdentityFile when the exec fails (mint error, CA down, or the not-yet-wired issuer stub), so ssh falls back to the static key. Verified empirically against the ssh binary. Ineligible workspaces (no certauth label, or missing port_id) and the WSL config (Windows paths/binary, deferred) get no Match block and keep the static key only. --- pkg/ssh/sshconfigurer.go | 80 ++++++++++++++++++++++-- pkg/ssh/sshconfigurer_test.go | 114 +++++++++++++++++++++++++++++++++- 2 files changed, 188 insertions(+), 6 deletions(-) diff --git a/pkg/ssh/sshconfigurer.go b/pkg/ssh/sshconfigurer.go index 5d785625..3c7380f2 100644 --- a/pkg/ssh/sshconfigurer.go +++ b/pkg/ssh/sshconfigurer.go @@ -13,6 +13,7 @@ import ( "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" ) @@ -173,6 +174,7 @@ type SSHConfigurerV2Store interface { GetWSLUserSSHConfig() (string, error) WriteWSLUserSSHConfig(config string) error GetBrevCloudflaredBinaryPath() (string, error) + UserHomeDir() (string, error) } var _ Config = SSHConfigurerV2{} @@ -238,7 +240,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), "") if err != nil { return "", breverrors.WrapAndTrace(err) } @@ -256,12 +258,17 @@ func (s SSHConfigurerV2) CreateNewSSHConfig(workspaces []entity.Workspace, nodes return "", breverrors.WrapAndTrace(err) } + home, err := s.store.UserHomeDir() + if err != nil { + return "", breverrors.WrapAndTrace(err) + } + cloudflaredBinaryPath, err := s.store.GetBrevCloudflaredBinaryPath() if err != nil { return "", breverrors.WrapAndTrace(err) } - sshConfig, err := makeNewSSHConfig(configPath, workspaces, pkPath, cloudflaredBinaryPath) + sshConfig, err := makeNewSSHConfig(configPath, workspaces, pkPath, cloudflaredBinaryPath, home) if err != nil { return "", breverrors.WrapAndTrace(err) } @@ -277,11 +284,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, home string) (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, home) if err != nil { return "", breverrors.WrapAndTrace(err) } @@ -353,7 +360,7 @@ 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 +func makeSSHConfigEntryV2(workspace entity.Workspace, privateKeyPath string, cloudflaredBinaryPath string, home string) (string, error) { //nolint:funlen,gocyclo // ok alias := string(workspace.GetLocalIdentifier()) privateKeyPath = "\"" + privateKeyPath + "\"" var sshVal string @@ -454,6 +461,17 @@ func makeSSHConfigEntryV2(workspace entity.Workspace, privateKeyPath string, clo } val := fmt.Sprintf("%s%s", sshVal, hostSSHVal) + + // For cert-eligible workspaces, prepend a Match exec block that mints a + // short-lived per-environment SSH certificate on connect. The block's + // IdentityFile accumulates with the Host block's static brev.pem identity + // when the exec succeeds (cert tried first, static as fallback); when the + // exec fails (mint error, CA down, not-yet-wired issuer) the Match block's + // IdentityFile is dropped and ssh falls back to the static key. + if certMatch := makeCertMatchEntry(workspace, home); certMatch != "" { + val = certMatch + val + } + return val, nil } @@ -461,6 +479,58 @@ func makeCloudflareSSHProxyCommand(cloudflaredBinaryPath string, hostname string return fmt.Sprintf("%s access ssh --hostname %s", cloudflaredBinaryPath, hostname) } +// SSHCertMatchTemplate is emitted before a workspace's Host block when the +// workspace is cert-eligible. The exec hook mints a short-lived certificate and +// writes it to CertKeyPath (+ "-cert.pub", which OpenSSH auto-loads). The +// IdentityFile here accumulates with the Host block's static brev.pem, so ssh +// tries the cert first and falls back to the static key if the exec fails. +const SSHCertMatchTemplate = `Match host {{ .Alias }} exec "{{ .ExecCommand }}" + IdentityFile {{ .CertKeyPath }} +` + +// sshCertMatchEntry holds the values for SSHCertMatchTemplate. +type sshCertMatchEntry struct { + Alias string + ExecCommand string + CertKeyPath string +} + +// makeCertMatchEntry returns the Match exec block for a cert-eligible workspace, +// or "" if the workspace is not eligible (e.g. created before cert support, or +// missing the port_id needed to mint) or when home is empty (WSL config, where +// cert support is not yet implemented and the static key is used). +func makeCertMatchEntry(workspace entity.Workspace, home string) string { + if home == "" || !workspace.SSHCertEligible || workspace.PortID == "" { + return "" + } + alias := string(workspace.GetLocalIdentifier()) + user := workspace.GetSSHUser() + certKeyPath := sshcert.KeyPath(home, workspace.ID) + entry := sshCertMatchEntry{ + Alias: alias, + ExecCommand: makeCertOnlyExecCommand(workspace.ID, workspace.PortID, user, certKeyPath), + CertKeyPath: "\"" + certKeyPath + "\"", + } + tmpl, err := template.New("certmatch-" + alias).Parse(SSHCertMatchTemplate) + if err != nil { + // A template parse error on a constant template is a programming bug; + // returning "" degrades to the static key fallback. + return "" + } + out, err := tmplAndValToString(tmpl, entry) + if err != nil { + return "" + } + return out +} + +// makeCertOnlyExecCommand builds the `brev shell --cert-only` invocation used by +// the Match exec hook. Paths are single-quoted for shell safety. +func makeCertOnlyExecCommand(envID, portID, linuxUser, outKey string) string { + return fmt.Sprintf("brev shell --cert-only --env %s --port %s --user %s --out-key '%s'", + envID, portID, linuxUser, outKey) +} + func (s SSHConfigurerV2) EnsureWSLConfigHasInclude() error { // openssh-7.3 diff --git a/pkg/ssh/sshconfigurer_test.go b/pkg/ssh/sshconfigurer_test.go index 4acb67f8..2a487e01 100644 --- a/pkg/ssh/sshconfigurer_test.go +++ b/pkg/ssh/sshconfigurer_test.go @@ -2,6 +2,7 @@ package ssh import ( "fmt" + "strings" "testing" "github.com/brevdev/brev-cli/pkg/entity" @@ -125,6 +126,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) @@ -512,7 +517,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, "/home/test-user") if (err != nil) != tt.wantErr { t.Errorf("makeSSHConfigEntryV2() error = %v, wantErr %v", err, tt.wantErr) return @@ -924,3 +929,110 @@ Host testName1-host }) } } + +func TestMakeCertMatchEntry_EligibleWorkspace(t *testing.T) { + w := entity.Workspace{ + ID: "env-abc", + Name: "my-env", + SSHUser: "ubuntu", + SSHCertEligible: true, + PortID: "port-1", + } + got := makeCertMatchEntry(w, "/home/u") + // Must be a Match block with the workspace alias and the brev --cert-only exec. + if !strings.HasPrefix(got, "Match host my-env exec \"") { + t.Errorf("expected Match host my-env exec block, got: %s", got) + } + if !strings.Contains(got, "--env env-abc") { + t.Errorf("missing --env env-abc: %s", got) + } + if !strings.Contains(got, "--port port-1") { + t.Errorf("missing --port port-1: %s", got) + } + if !strings.Contains(got, "--user ubuntu") { + t.Errorf("missing --user ubuntu: %s", got) + } + if !strings.Contains(got, "--out-key '/home/u/.brev/ssh-certs/env-abc'") { + t.Errorf("missing out-key path: %s", got) + } + // IdentityFile must point at the cert key path (quoted). + if !strings.Contains(got, "IdentityFile \"/home/u/.brev/ssh-certs/env-abc\"") { + t.Errorf("missing IdentityFile cert path: %s", got) + } +} + +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"); 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"); got != "" { + t.Errorf("eligible without PortID should produce no Match block, got: %s", got) + } + // Empty home (WSL) -> no Match block. + w3 := entity.Workspace{ID: "env-1", Name: "n", SSHUser: "u", SSHCertEligible: true, PortID: "p"} + if got := makeCertMatchEntry(w3, ""); got != "" { + t.Errorf("empty home 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", "/home/u") + 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) + } + // Both the cert IdentityFile (in Match) and static brev.pem (in Host) must be present. + 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 TestMakeSSHConfigEntryV2_IneligibleWorkspaceNoCertMatch(t *testing.T) { + w := entity.Workspace{ + ID: "env-old", + Name: "old-env", + Status: entity.Running, + SSHUser: "ubuntu", + SSHPort: 22, + SSHHostname: "10.0.0.1", + // SSHCertEligible false, PortID empty + } + got, err := makeSSHConfigEntryV2(w, "/home/u/.brev/brev.pem", "/tmp/cf", "/home/u") + if err != nil { + t.Fatalf("makeSSHConfigEntryV2: %v", err) + } + if strings.Contains(got, "Match host") { + t.Errorf("ineligible workspace should have no Match block: %s", got) + } + if !strings.Contains(got, "/home/u/.brev/brev.pem") { + t.Error("static key should still be present") + } +} From 5aaef0d5cf879382dd40ab16bcaef0e696423c66 Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Wed, 19 Aug 2026 17:08:56 -0700 Subject: [PATCH 05/27] refactor: trim over-defensiveness and test duplication Cut ~260 lines while preserving behavior and coverage: - Delete the Store type in pkg/sshcert (dead code: production uses the free functions HasValidCertAt/WriteFiles with explicit paths, since --cert-only receives --out-key). Removed its methods and tests. - Delete fakeIssuer from certonly_test.go (unused; tests use certIssuerFunc). - Replace makeCertMatchEntry's text/template with a direct Sprintf: the template was a constant string and the error branches guarded against impossible parse failures of that constant. - Drop validateCertOnly's 'flags set without --cert-only' branch: the flags are hidden and only set by the generated config, so the inverse scenario can't occur in practice. - Consolidate TestEnvironmentCertEligible and TestSafeFilename into table-driven form; merge redundant assertions. - Trim verbose per-function comments that restated the package doc. --- pkg/cmd/shell/certonly.go | 59 ++-------- pkg/cmd/shell/certonly_test.go | 116 +++++--------------- pkg/ssh/sshconfigurer.go | 50 +-------- pkg/sshcert/sshcert.go | 140 +++++++---------------- pkg/sshcert/sshcert_test.go | 195 +++++++++++++-------------------- 5 files changed, 152 insertions(+), 408 deletions(-) diff --git a/pkg/cmd/shell/certonly.go b/pkg/cmd/shell/certonly.go index 229637fd..fa0ee990 100644 --- a/pkg/cmd/shell/certonly.go +++ b/pkg/cmd/shell/certonly.go @@ -84,53 +84,33 @@ type certOnlyStore interface { GetAccessToken() (string, error) } -// runCertOnly implements `brev shell --cert-only`. It is a headless, non-interactive -// mint-and-write used by the ssh config's Match exec hook: it ensures a valid -// cached certificate exists at req.OutKey (+ "-cert.pub"), minting a fresh one -// if needed, and exits 0 on success. -// -// On any failure (auth unavailable, CA error, write error) it prints a short -// human-actionable message to stderr and exits non-zero WITHOUT writing files, -// so the Match block's IdentityFile is dropped and ssh falls back to the -// static brev.pem identity. It never prompts and never blocks on an interactive -// login — those would hang the ssh invocation. +// runCertOnly implements `brev shell --cert-only`: a headless mint-and-write +// used by the ssh config's Match exec hook. On any failure it writes nothing +// and returns non-zero so ssh drops the cert IdentityFile and falls back to +// the static brev.pem. It never prompts — that would hang the ssh invocation. func runCertOnly(store ShellStore, req certOnlyRequest) error { return runCertOnlyWith(store, afero.NewOsFs(), newCertIssuer(store, config.GlobalConfig.GetBrevPublicAPIURL()), req) } -// runCertOnlyWith is the testable form of runCertOnly with injectable deps. func runCertOnlyWith(store certOnlyStore, fs afero.Fs, issuer CertIssuer, req certOnlyRequest) error { - // Reuse the existing platform credential (API key / token) the CLI already - // stores. No new login here — this is the whole point of not prompting. if _, err := store.GetAccessToken(); err != nil { fmt.Fprintln(os.Stderr, "brev: not logged in. Run `brev login` and retry.") return breverrors.WrapAndTrace(err) } - certPath := req.OutKey + "-cert.pub" - - // Cache check: if a non-expired cert is already on disk, reuse it and exit 0. - // This keeps repeated `ssh` from hitting the CA on every connection. - ok, err := sshcert.HasValidCertAt(fs, certPath, time.Now(), sshcert.DefaultRenewalMargin) - if err != nil { + 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) - } - if ok { + } else if ok { return nil } - - // Generate a fresh ephemeral keypair. The private key never leaves this - // machine; only the public key is sent to the CA for signing. 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(), certOnlyTimeout) defer cancel() - res, err := issuer.Issue(ctx, certIssueRequest{ EnvironmentID: req.EnvironmentID, PortID: req.PortID, @@ -138,13 +118,9 @@ func runCertOnlyWith(store certOnlyStore, fs afero.Fs, issuer CertIssuer, req ce PublicKey: pubKeyOpenSSH, }) if err != nil { - // This is the fallback path: print a concise message and return an - // error so the caller can exit non-zero. The ssh config's Match block - // then drops its IdentityFile and ssh uses the static brev.pem. 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, res.Certificate); err != nil { fmt.Fprintf(os.Stderr, "brev: failed to write cert files: %v\n", err) return breverrors.WrapAndTrace(err) @@ -176,30 +152,13 @@ func addCertOnlyFlags(cmd *cobra.Command, f *certOnlyFlags) { } } -// validateCertOnly returns an error if the --cert-only flags are inconsistent -// or incomplete. When --cert-only is not set, all of its flags must be unset. +// validateCertOnly returns an error if --cert-only is set without all four +// required parameters. The flags are hidden and only set by the generated ssh +// config, so the inverse (params without --cert-only) is not a real scenario. func validateCertOnly(f certOnlyFlags) error { if !f.certOnly { - // cert-only flags must not be set without --cert-only. - var set []string - if f.env != "" { - set = append(set, "--env") - } - if f.port != "" { - set = append(set, "--port") - } - if f.user != "" { - set = append(set, "--user") - } - if f.outKey != "" { - set = append(set, "--out-key") - } - if len(set) > 0 { - return fmt.Errorf("flags %s require --cert-only", strings.Join(set, ", ")) - } return nil } - // --cert-only mode: all four parameters are required. var missing []string if f.env == "" { missing = append(missing, "--env") diff --git a/pkg/cmd/shell/certonly_test.go b/pkg/cmd/shell/certonly_test.go index dfa6ce50..61be0bb9 100644 --- a/pkg/cmd/shell/certonly_test.go +++ b/pkg/cmd/shell/certonly_test.go @@ -15,7 +15,7 @@ import ( "github.com/brevdev/brev-cli/pkg/sshcert" ) -// fakeShellStore satisfies the GetAccessToken subset of ShellStore for --cert-only. +// fakeShellStore satisfies the certOnlyStore interface (GetAccessToken only). type fakeShellStore struct { token string err error @@ -28,23 +28,17 @@ func (f fakeShellStore) GetAccessToken() (string, error) { return f.token, nil } -// fakeIssuer is a controllable CertIssuer for tests. -type fakeIssuer struct { - cert string - err error - got certIssueRequest +// certIssuerFunc adapts a closure into a CertIssuer for tests. +type certIssuerFunc struct { + fn func(context.Context, certIssueRequest) (certIssueResult, error) } -func (f *fakeIssuer) Issue(_ context.Context, req certIssueRequest) (certIssueResult, error) { - f.got = req - if f.err != nil { - return certIssueResult{}, f.err - } - return certIssueResult{Certificate: f.cert}, nil +func (c *certIssuerFunc) Issue(ctx context.Context, req certIssueRequest) (certIssueResult, error) { + return c.fn(ctx, req) } -// mintCertForTest mints a real, signed user certificate over an in-memory CA -// for the given public key, so the on-disk cert parses as a valid ssh.Certificate. +// mintCertForTest mints a real user certificate over an in-memory CA for the +// given public key, so the written cert parses as a valid ssh.Certificate. func mintCertForTest(t *testing.T, pubKeyOpenSSH string) string { t.Helper() pubKey, _, _, _, err := ssh.ParseAuthorizedKey([]byte(pubKeyOpenSSH)) @@ -77,69 +71,37 @@ func mintCertForTest(t *testing.T, pubKeyOpenSSH string) string { func TestRunCertOnly_MintsAndWrites(t *testing.T) { fs := afero.NewMemMapFs() - store := fakeShellStore{token: "tok"} outKey := "/home/u/.brev/ssh-certs/env-1" - - // The fake issuer needs to return a cert that parses; mint one over the - // pub key the helper will generate. We capture the pub key by issuing once. - var issued bool - issuer := &fakeIssuer{} - issuer.err = nil - issuer.cert = "" // will be set on first call using the real pub key - - // Wrap so we can mint a cert over whatever pub key the helper generated. - wrapped := &certIssuerFunc{fn: func(_ context.Context, req certIssueRequest) (certIssueResult, error) { - issued = true + issuer := &certIssuerFunc{fn: func(_ context.Context, req certIssueRequest) (certIssueResult, error) { return certIssueResult{Certificate: mintCertForTest(t, req.PublicKey)}, nil }} - - err := runCertOnlyWith(store, fs, wrapped, certOnlyRequest{ - EnvironmentID: "env-1", - PortID: "port-1", - LinuxUser: "ubuntu", - OutKey: outKey, - }) - if err != nil { + if err := runCertOnlyWith(fakeShellStore{token: "tok"}, fs, issuer, certOnlyRequest{ + EnvironmentID: "env-1", PortID: "port-1", LinuxUser: "ubuntu", OutKey: outKey, + }); err != nil { t.Fatalf("runCertOnlyWith: %v", err) } - if !issued { - t.Error("expected issuer to be called") + for _, p := range []string{outKey, outKey + "-cert.pub"} { + if ok, _ := afero.Exists(fs, p); !ok { + t.Errorf("not written: %s", p) + } } - - // Files must exist. - exists, _ := afero.Exists(fs, outKey) - if !exists { - t.Error("private key not written") - } - exists, _ = afero.Exists(fs, outKey+"-cert.pub") - if !exists { - t.Error("cert not written") - } - // Cert must parse. - ok, err := sshcert.HasValidCertAt(fs, outKey+"-cert.pub", time.Now(), 0) - if err != nil || !ok { + 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) } } func TestRunCertOnly_ReusesCachedCert(t *testing.T) { fs := afero.NewMemMapFs() - store := fakeShellStore{token: "tok"} outKey := "/home/u/.brev/ssh-certs/env-1" - - // Pre-write a valid cert. - _, pubKeyOpenSSH, _ := sshcert.GenerateKeyPair() - certStr := mintCertForTest(t, pubKeyOpenSSH) - if err := sshcert.WriteFiles(fs, outKey, outKey+"-cert.pub", []byte("priv"), certStr); err != nil { + _, 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) (certIssueResult, error) { - t.Error("issuer should NOT be called when cache is valid") + t.Error("issuer should not be called when cache is valid") return certIssueResult{}, nil }} - - if err := runCertOnlyWith(store, fs, issuer, certOnlyRequest{ + if err := runCertOnlyWith(fakeShellStore{token: "tok"}, fs, issuer, certOnlyRequest{ EnvironmentID: "env-1", PortID: "port-1", LinuxUser: "ubuntu", OutKey: outKey, }); err != nil { t.Fatalf("expected reuse, got err: %v", err) @@ -148,78 +110,50 @@ func TestRunCertOnly_ReusesCachedCert(t *testing.T) { func TestRunCertOnly_FallsBackOnIssueError(t *testing.T) { fs := afero.NewMemMapFs() - store := fakeShellStore{token: "tok"} issuer := &certIssuerFunc{fn: func(_ context.Context, _ certIssueRequest) (certIssueResult, error) { return certIssueResult{}, errors.New("CA unavailable") }} - - err := runCertOnlyWith(store, fs, issuer, certOnlyRequest{ + err := runCertOnlyWith(fakeShellStore{token: "tok"}, fs, issuer, certOnlyRequest{ 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") } - // No files must be written on fallback. - exists, _ := afero.Exists(fs, "/home/u/.brev/ssh-certs/env-1") - if exists { + 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 TestRunCertOnly_FallsBackOnAuthError(t *testing.T) { fs := afero.NewMemMapFs() - store := fakeShellStore{err: errors.New("no token")} issuer := &certIssuerFunc{fn: func(_ context.Context, _ certIssueRequest) (certIssueResult, error) { t.Error("issuer should not be called when not authenticated") return certIssueResult{}, nil }} - - err := runCertOnlyWith(store, fs, issuer, certOnlyRequest{ + if err := runCertOnlyWith(fakeShellStore{err: errors.New("no token")}, fs, issuer, certOnlyRequest{ EnvironmentID: "env-1", PortID: "port-1", LinuxUser: "ubuntu", OutKey: "/home/u/.brev/ssh-certs/env-1", - }) - if err == nil { + }); err == nil { t.Fatal("expected error on auth failure") } } func TestRunCertOnly_StubIssuerReturnsUnavailable(t *testing.T) { - // The stub issuer that ships today always returns ErrCertIssuanceUnavailable, - // so --cert-only falls back to the static key until the real issuer is wired. _, err := stubCertIssuer{}.Issue(context.Background(), certIssueRequest{}) - if err == nil { - t.Fatal("expected ErrCertIssuanceUnavailable from stub") - } if !errors.Is(err, ErrCertIssuanceUnavailable) { t.Fatalf("expected ErrCertIssuanceUnavailable, got %v", err) } } func TestValidateCertOnly(t *testing.T) { - // Not cert-only, no flags -> ok. if err := validateCertOnly(certOnlyFlags{}); err != nil { t.Errorf("empty flags should be valid: %v", err) } - // --cert-only requires all four. if err := validateCertOnly(certOnlyFlags{certOnly: true}); err == nil { t.Error("cert-only without params should error") } - // All four set -> ok. if err := validateCertOnly(certOnlyFlags{certOnly: true, env: "e", port: "p", user: "u", outKey: "/k"}); err != nil { t.Errorf("complete cert-only should be valid: %v", err) } - // Params without --cert-only -> error. - if err := validateCertOnly(certOnlyFlags{env: "e"}); err == nil { - t.Error("params without cert-only should error") - } -} - -// certIssuerFunc adapts a function into a CertIssuer for tests. -type certIssuerFunc struct { - fn func(context.Context, certIssueRequest) (certIssueResult, error) -} - -func (c *certIssuerFunc) Issue(ctx context.Context, req certIssueRequest) (certIssueResult, error) { - return c.fn(ctx, req) } diff --git a/pkg/ssh/sshconfigurer.go b/pkg/ssh/sshconfigurer.go index 3c7380f2..b7d01b5a 100644 --- a/pkg/ssh/sshconfigurer.go +++ b/pkg/ssh/sshconfigurer.go @@ -471,7 +471,6 @@ func makeSSHConfigEntryV2(workspace entity.Workspace, privateKeyPath string, clo if certMatch := makeCertMatchEntry(workspace, home); certMatch != "" { val = certMatch + val } - return val, nil } @@ -479,56 +478,17 @@ func makeCloudflareSSHProxyCommand(cloudflaredBinaryPath string, hostname string return fmt.Sprintf("%s access ssh --hostname %s", cloudflaredBinaryPath, hostname) } -// SSHCertMatchTemplate is emitted before a workspace's Host block when the -// workspace is cert-eligible. The exec hook mints a short-lived certificate and -// writes it to CertKeyPath (+ "-cert.pub", which OpenSSH auto-loads). The -// IdentityFile here accumulates with the Host block's static brev.pem, so ssh -// tries the cert first and falls back to the static key if the exec fails. -const SSHCertMatchTemplate = `Match host {{ .Alias }} exec "{{ .ExecCommand }}" - IdentityFile {{ .CertKeyPath }} -` - -// sshCertMatchEntry holds the values for SSHCertMatchTemplate. -type sshCertMatchEntry struct { - Alias string - ExecCommand string - CertKeyPath string -} - -// makeCertMatchEntry returns the Match exec block for a cert-eligible workspace, -// or "" if the workspace is not eligible (e.g. created before cert support, or -// missing the port_id needed to mint) or when home is empty (WSL config, where -// cert support is not yet implemented and the static key is used). +// makeCertMatchEntry returns the Match exec block for a cert-eligible +// workspace, or "" if the workspace is not eligible or home is empty (WSL). func makeCertMatchEntry(workspace entity.Workspace, home string) string { if home == "" || !workspace.SSHCertEligible || workspace.PortID == "" { return "" } alias := string(workspace.GetLocalIdentifier()) - user := workspace.GetSSHUser() certKeyPath := sshcert.KeyPath(home, workspace.ID) - entry := sshCertMatchEntry{ - Alias: alias, - ExecCommand: makeCertOnlyExecCommand(workspace.ID, workspace.PortID, user, certKeyPath), - CertKeyPath: "\"" + certKeyPath + "\"", - } - tmpl, err := template.New("certmatch-" + alias).Parse(SSHCertMatchTemplate) - if err != nil { - // A template parse error on a constant template is a programming bug; - // returning "" degrades to the static key fallback. - return "" - } - out, err := tmplAndValToString(tmpl, entry) - if err != nil { - return "" - } - return out -} - -// makeCertOnlyExecCommand builds the `brev shell --cert-only` invocation used by -// the Match exec hook. Paths are single-quoted for shell safety. -func makeCertOnlyExecCommand(envID, portID, linuxUser, outKey string) string { - return fmt.Sprintf("brev shell --cert-only --env %s --port %s --user %s --out-key '%s'", - envID, portID, linuxUser, outKey) + exec := fmt.Sprintf("brev shell --cert-only --env %s --port %s --user %s --out-key '%s'", + workspace.ID, workspace.PortID, workspace.GetSSHUser(), certKeyPath) + return fmt.Sprintf("Match host %s exec %q\n IdentityFile %q\n", alias, exec, certKeyPath) } func (s SSHConfigurerV2) EnsureWSLConfigHasInclude() error { diff --git a/pkg/sshcert/sshcert.go b/pkg/sshcert/sshcert.go index 70027dd2..1dcaeb1b 100644 --- a/pkg/sshcert/sshcert.go +++ b/pkg/sshcert/sshcert.go @@ -1,26 +1,12 @@ // Package sshcert manages short-lived, per-environment SSH certificates and // their backing ephemeral keypairs on disk for use by the OpenSSH client. // -// Design notes (see the SSH-cert design discussion): -// -// - A fresh ed25519 keypair is generated each time a certificate is renewed. -// Keypair generation is effectively free, so reusing a key across renewals -// would save nothing while reintroducing a long-lived private key on disk -// (the exact property we are leaving static keys to escape). Instead, the -// private key's exposure window is bounded by the certificate's own validity -// window. The (private key, certificate) pair is cached on disk for the -// certificate's lifetime so repeated `ssh` invocations do not re-hit the CA. -// -// - Files live under ~/.brev/ssh-certs/{,-cert.pub}. OpenSSH -// auto-loads -cert.pub as the certificate, so a single -// IdentityFile directive in the ssh config covers both key and cert. -// -// - Writes are atomic (temp file in the same directory + rename) and the -// private key is written with mode 0600. -// -// This package is deliberately independent of the certificate-issuance RPC so -// the rest of the SSH-cert feature can build and be tested before the -// generated connect client for IssueEnvironmentSSHCertificate is published. +// A fresh ed25519 keypair is generated each time a certificate is renewed; +// the (private key, certificate) pair is cached on disk for the certificate's +// validity window so repeated ssh invocations do not re-hit the CA. Files live +// under ~/.brev/ssh-certs/{,-cert.pub} (OpenSSH auto-loads +// -cert.pub). Writes are atomic (temp + rename) and the private +// key is written 0600. package sshcert import ( @@ -39,8 +25,6 @@ import ( breverrors "github.com/brevdev/brev-cli/pkg/errors" ) -// Subdirectory under the brev home directory where per-environment keypairs and -// certificates are cached. const certSubDir = "ssh-certs" // DefaultRenewalMargin is how long before a certificate's not-after time we @@ -48,35 +32,31 @@ const certSubDir = "ssh-certs" // race where a certificate expires between the mint and the subsequent ssh use. const DefaultRenewalMargin = 60 * time.Second -// Label constants mirroring dev-plane's internal/labels package. They are -// duplicated here because that package is internal to dev-plane. Keep these in -// sync with dev-plane/internal/labels. +// Label constants mirroring dev-plane's internal/labels package (kept in sync +// because that package is internal to dev-plane). const ( LabelKeySSHProvider = "sshprovider" SSHProviderCertAuth = "certauth" ) // EnvironmentCertEligible reports whether an environment's labels opt it into -// certificate-based SSH auth. Non-eligible (e.g. older) environments fall back -// to the existing static key. +// certificate-based SSH auth. func EnvironmentCertEligible(labels map[string]string) bool { return labels[LabelKeySSHProvider] == SSHProviderCertAuth } -// Dir returns the on-disk directory holding cached certificates for the given -// home directory. +// Dir returns the on-disk directory holding cached certificates for home. func Dir(home string) string { return filepath.Join(home, ".brev", certSubDir) } -// safeFilename reduces an environment ID to something safe to use as a file -// name. Environment IDs are UUIDs today, so this is mostly defensive. +// safeFilename reduces an environment ID to a safe file name. Environment IDs +// are UUIDs today, so this is mostly defensive. func safeFilename(envID string) string { s := strings.TrimSpace(envID) if s == "" { - s = "default" + return "default" } - // Replace anything that isn't [A-Za-z0-9._-] with '-'. var b strings.Builder for _, r := range s { switch { @@ -88,48 +68,41 @@ func safeFilename(envID string) string { } out := b.String() if out == "" { - out = "default" + return "default" } return out } -// KeyPath returns the on-disk path of the private key for the given environment. +// KeyPath returns the on-disk private-key path for the given environment. func KeyPath(home, envID string) string { return filepath.Join(Dir(home), safeFilename(envID)) } -// CertPath returns the on-disk path of the certificate for the given environment. -// This follows OpenSSH's -cert.pub convention so a single -// IdentityFile directive loads both the key and the certificate. +// CertPath returns the on-disk certificate path for the given environment, +// following OpenSSH's -cert.pub convention. func CertPath(home, envID string) string { return KeyPath(home, envID) + "-cert.pub" } -// GenerateKeyPair generates a fresh ed25519 keypair suitable for certificate -// issuance. It returns the private key in OpenSSH PEM format (ready to write -// to disk and use as an IdentityFile) and the public key as a single-line -// OpenSSH authorized-key string (the format the certificate-issuance RPC +// GenerateKeyPair generates a fresh ed25519 keypair. It returns the private +// key in OpenSSH PEM format (for IdentityFile) and the public key as a +// single-line OpenSSH authorized-key string (the format the issuance RPC // expects as its public_key field). 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) } - // MarshalAuthorizedKey produces "ssh-ed25519 AAAA... comment\n"; the CA - // requires exactly one line with no options, so trim the trailing newline. pubKeyOpenSSH = strings.TrimRight(string(ssh.MarshalAuthorizedKey(sshPubKey)), "\n") - block, err := ssh.MarshalPrivateKey(priv, "brev") if err != nil { return nil, "", breverrors.WrapAndTrace(err) } - privKeyPEM = pem.EncodeToMemory(block) - return privKeyPEM, pubKeyOpenSSH, nil + return pem.EncodeToMemory(block), pubKeyOpenSSH, nil } // ParseCertificate parses an OpenSSH authorized-key-formatted certificate @@ -156,9 +129,8 @@ func ParseCertificate(certOpenSSH string) (*ssh.Certificate, error) { return cert, nil } -// CertValidAt reports whether the certificate is valid at now+margin (i.e. not -// yet close enough to expiry to require renewal). ValidBefore is a unix -// timestamp; a value of 0 or ^uint64(0) means "forever" per the SSH spec. +// CertValidAt reports whether the certificate is valid at now+margin. A +// ValidBefore of 0 or ^uint64(0) means "forever" per the SSH spec. func CertValidAt(cert *ssh.Certificate, now time.Time, margin time.Duration) bool { if cert == nil { return false @@ -166,82 +138,50 @@ func CertValidAt(cert *ssh.Certificate, now time.Time, margin time.Duration) boo notBefore := int64(cert.ValidAfter) notAfter := int64(cert.ValidBefore) if notAfter == 0 || notAfter == -1 { - // "forever" — still bounded by ValidAfter. return now.Add(margin).Unix() >= notBefore } return now.Add(margin).Unix() < notAfter } -// Store reads and writes cached certificates and their backing keypairs to a -// filesystem (afero is used so the logic is unit-testable). -type Store struct { - fs afero.Fs - home string -} - -// NewStore returns a Store rooted at home using the given filesystem. Use -// files.AppFs (the OS filesystem) in production and an afero.MemMapFs in tests. -func NewStore(fs afero.Fs, home string) *Store { - return &Store{fs: fs, home: home} -} - -// KeyPath returns the on-disk private-key path for the given environment. -func (s *Store) KeyPath(envID string) string { return KeyPath(s.home, envID) } - -// CertPath returns the on-disk certificate path for the given environment. -func (s *Store) CertPath(envID string) string { return CertPath(s.home, envID) } - -// HasValidCert reports whether a non-expired certificate (with margin) for the -// given environment is already present on disk. A missing file, unparseable -// certificate, or one within the renewal margin of expiry returns (false, nil). -// A genuine I/O error is returned. -func (s *Store) HasValidCert(envID string, now time.Time, margin time.Duration) (bool, error) { - exists, err := afero.Exists(s.fs, s.CertPath(envID)) +// HasValidCertAt reports whether a non-expired certificate (with margin) is +// present at certPath. A missing file or unparseable certificate returns +// (false, nil) so the caller mints a fresh one rather than failing. +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(s.fs, s.CertPath(envID)) + certBytes, err := afero.ReadFile(fs, certPath) if err != nil { return false, breverrors.WrapAndTrace(err) } cert, err := ParseCertificate(string(certBytes)) if err != nil { - // A corrupt cert on disk is treated as "no valid cert" so the caller - // will mint a fresh one rather than failing the whole ssh attempt. - return false, nil + return false, nil // corrupt cert -> mint fresh } return CertValidAt(cert, now, margin), nil } -// Write writes the private key and certificate for the given environment to -// disk atomically (temp file + rename within the same directory). The private -// key is written with mode 0600; the certificate with 0644. -func (s *Store) Write(envID string, privKeyPEM []byte, certOpenSSH string) error { - dir := Dir(s.home) - if err := s.fs.MkdirAll(dir, 0o700); err != nil { +// WriteFiles writes the private key and certificate to the given paths +// atomically. The private key is 0600; the certificate is 0644. +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(s.fs, s.KeyPath(envID), privKeyPEM, 0o600); err != nil { + if err := writeAtomic(fs, keyPath, privKeyPEM, 0o600); err != nil { return breverrors.WrapAndTrace(err) } - - // Ensure the certificate ends with a newline for OpenSSH's reader. if !strings.HasSuffix(certOpenSSH, "\n") { certOpenSSH += "\n" } - if err := writeAtomic(s.fs, s.CertPath(envID), []byte(certOpenSSH), 0o644); err != nil { - return breverrors.WrapAndTrace(err) - } - return nil + return writeAtomic(fs, certPath, []byte(certOpenSSH), 0o644) } // writeAtomic writes data to path via a temp file in the same directory and -// renames it into place. Renaming within the same directory is atomic on POSIX -// filesystems, so a reader never observes a partially-written file. +// renames it 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") @@ -249,7 +189,6 @@ func writeAtomic(fs afero.Fs, path string, data []byte, mode os.FileMode) error return breverrors.WrapAndTrace(err) } tmpName := tmp.Name() - // Clean up the temp file if anything below fails. defer func() { _ = fs.Remove(tmpName) }() if _, err := tmp.Write(data); err != nil { @@ -262,8 +201,5 @@ func writeAtomic(fs afero.Fs, path string, data []byte, mode os.FileMode) error if err := fs.Chmod(tmpName, mode); err != nil { return breverrors.WrapAndTrace(err) } - if err := fs.Rename(tmpName, path); err != nil { - return breverrors.WrapAndTrace(err) - } - return nil + return fs.Rename(tmpName, path) } diff --git a/pkg/sshcert/sshcert_test.go b/pkg/sshcert/sshcert_test.go index b202ef6b..4d249a18 100644 --- a/pkg/sshcert/sshcert_test.go +++ b/pkg/sshcert/sshcert_test.go @@ -12,21 +12,18 @@ import ( "golang.org/x/crypto/ssh" ) -// mintTestCert mints a real user certificate signed by an in-memory CA, so the -// parse/cache logic is exercised against genuine ssh.Certificate objects. +// mintTestCert mints a real user certificate signed by an in-memory CA, +// exercising parse/cache logic against genuine ssh.Certificate objects. 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) } - - // User keypair (the "client" key the cert is issued over). pub, _, err := ed25519.GenerateKey(rand.Reader) if err != nil { t.Fatalf("generate user key: %v", err) @@ -35,7 +32,6 @@ func mintTestCert(t *testing.T, validBefore time.Time) string { if err != nil { t.Fatalf("new public key: %v", err) } - cert := &ssh.Certificate{ Key: sshPub, Serial: 1, @@ -44,9 +40,7 @@ func mintTestCert(t *testing.T, validBefore time.Time) string { 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": "", - }}, + Permissions: ssh.Permissions{Extensions: map[string]string{"permit-pty": ""}}, } if err := cert.SignCert(rand.Reader, signer); err != nil { t.Fatalf("sign cert: %v", err) @@ -60,35 +54,33 @@ func TestGenerateKeyPair_Format(t *testing.T) { t.Fatalf("GenerateKeyPair: %v", err) } if !bytes.HasPrefix(privPEM, []byte("-----BEGIN OPENSSH PRIVATE KEY-----")) { - t.Errorf("private key is not OpenSSH PEM; got %q", privPEM[:40]) + t.Errorf("private key not OpenSSH PEM: %q", privPEM[:40]) } if !strings.HasPrefix(pubOpenSSH, "ssh-ed25519 ") { - t.Errorf("public key not ssh-ed25519; got %q", pubOpenSSH) + t.Errorf("public key not ssh-ed25519: %q", pubOpenSSH) } if strings.ContainsAny(pubOpenSSH, "\r\n") { - t.Errorf("public key must be a single line; got %q", pubOpenSSH) + t.Errorf("public key must be a single line: %q", pubOpenSSH) } - // The public key must be parseable as an authorized key with no options. + // Must parse with no options and no trailing data (the CA requires this). _, _, options, rest, err := ssh.ParseAuthorizedKey([]byte(pubOpenSSH)) if err != nil { t.Fatalf("ParseAuthorizedKey: %v", err) } if len(options) != 0 || len(bytes.TrimSpace(rest)) != 0 { - t.Errorf("public key has options or trailing data; options=%v rest=%q", options, rest) + t.Errorf("pub key has options/trailing data: options=%v rest=%q", options, rest) } - // The private key must be parseable back. signer, err := ssh.ParsePrivateKey(privPEM) if err != nil { t.Fatalf("ParsePrivateKey: %v", err) } if signer.PublicKey().Type() != ssh.KeyAlgoED25519 { - t.Errorf("expected ed25519 signer, got %s", signer.PublicKey().Type()) + t.Errorf("expected ed25519, got %s", signer.PublicKey().Type()) } } func TestParseCertificate(t *testing.T) { - certStr := mintTestCert(t, time.Now().Add(10*time.Minute)) - cert, err := ParseCertificate(certStr) + cert, err := ParseCertificate(mintTestCert(t, time.Now().Add(10*time.Minute))) if err != nil { t.Fatalf("ParseCertificate: %v", err) } @@ -98,34 +90,29 @@ func TestParseCertificate(t *testing.T) { if len(cert.ValidPrincipals) != 1 || cert.ValidPrincipals[0] != "brev:v1:vm:test-env:login:ubuntu" { t.Errorf("unexpected principals: %v", cert.ValidPrincipals) } - - if _, err := ParseCertificate(""); err == nil { - t.Error("expected error for empty cert") - } - if _, err := ParseCertificate("not a cert"); err == nil { - t.Error("expected error for garbage") + 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() - cert := &ssh.Certificate{ + valid := &ssh.Certificate{ ValidAfter: uint64(now.Add(-time.Hour).Unix()), ValidBefore: uint64(now.Add(10 * time.Minute).Unix()), } - if !CertValidAt(cert, now, time.Minute) { - t.Error("cert valid for 10 more minutes should be valid with 1m margin") + if !CertValidAt(valid, now, time.Minute) { + t.Error("cert with 10m left should be valid with 1m margin") } - // 5 minutes left, 10 minute margin -> needs renewal. - if CertValidAt(cert, now, 10*time.Minute) { - t.Error("cert with 5m left should need renewal with 10m margin") + if CertValidAt(valid, now, 10*time.Minute) { + t.Error("cert with 10m left should need renewal with 10m margin") } - // Already expired. - cert.ValidBefore = uint64(now.Add(-time.Minute).Unix()) - if CertValidAt(cert, now, time.Minute) { + 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") } - // Forever cert (ValidBefore == 0) bounded only by ValidAfter. forever := &ssh.Certificate{ValidAfter: uint64(now.Add(-time.Hour).Unix()), ValidBefore: 0} if !CertValidAt(forever, now, time.Minute) { t.Error("forever cert within ValidAfter should be valid") @@ -135,120 +122,88 @@ func TestCertValidAt(t *testing.T) { } } -func TestStore_WriteAndHasValidCert(t *testing.T) { +func TestHasValidCertAt(t *testing.T) { fs := afero.NewMemMapFs() - store := NewStore(fs, "/home/user") - - // No cert yet. - ok, err := store.HasValidCert("env-1", time.Now(), DefaultRenewalMargin) - if err != nil { - t.Fatalf("HasValidCert on empty: %v", err) - } - if ok { - t.Error("expected no valid cert initially") - } + certPath := CertPath("/home/u", "env-1") - // Generate a keypair and mint a cert valid for 10 minutes. - privPEM, _, err := GenerateKeyPair() - if err != nil { - t.Fatalf("GenerateKeyPair: %v", err) - } - certStr := mintTestCert(t, time.Now().Add(10*time.Minute)) - - if err := store.Write("env-1", privPEM, certStr); err != nil { - t.Fatalf("Write: %v", err) + // Missing -> not valid, no error. + if ok, err := HasValidCertAt(fs, certPath, time.Now(), DefaultRenewalMargin); ok || err != nil { + t.Fatalf("missing cert: ok=%v err=%v", ok, err) } - - // Paths must follow the -cert.pub convention. - if got := store.KeyPath("env-1"); !strings.HasSuffix(got, "ssh-certs/env-1") { - t.Errorf("unexpected key path: %s", got) + // Written -> valid. + privPEM, _ := mustGen(t) + if err := WriteFiles(fs, KeyPath("/home/u", "env-1"), certPath, privPEM, mintTestCert(t, time.Now().Add(10*time.Minute))); err != nil { + t.Fatalf("WriteFiles: %v", err) } - if got := store.CertPath("env-1"); got != store.KeyPath("env-1")+"-cert.pub" { - t.Errorf("cert path must be key path + -cert.pub: %s", got) + if ok, _ := HasValidCertAt(fs, certPath, time.Now(), DefaultRenewalMargin); !ok { + t.Error("expected valid after write") } - - // Cert on disk is valid. - ok, err = store.HasValidCert("env-1", time.Now(), DefaultRenewalMargin) - if err != nil { - t.Fatalf("HasValidCert after write: %v", err) - } - if !ok { - t.Error("expected valid cert after write") - } - - // A different env has no cert. - ok, err = store.HasValidCert("env-2", time.Now(), DefaultRenewalMargin) - if err != nil { - t.Fatalf("HasValidCert env-2: %v", err) - } - if ok { + // Different env -> not valid. + if ok, _ := HasValidCertAt(fs, CertPath("/home/u", "env-2"), time.Now(), DefaultRenewalMargin); ok { t.Error("env-2 should have no cert") } - - // Corrupt cert on disk is treated as "no valid cert" (not an error). - if err := afero.WriteFile(fs, store.CertPath("env-1"), []byte("garbage"), 0o644); err != nil { - t.Fatalf("write corrupt: %v", err) - } - ok, err = store.HasValidCert("env-1", time.Now(), DefaultRenewalMargin) - if err != nil { - t.Fatalf("HasValidCert corrupt: %v", err) + // Corrupt -> not valid, no error (mint fresh). + if err := afero.WriteFile(fs, certPath, []byte("garbage"), 0o644); err != nil { + t.Fatal(err) } - if ok { - t.Error("corrupt cert should not be considered valid") + 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 TestStore_WriteIsAtomic(t *testing.T) { +func TestWriteFiles_NoLeftoverTemp(t *testing.T) { fs := afero.NewMemMapFs() - store := NewStore(fs, "/home/user") - privPEM, _, err := GenerateKeyPair() - if err != nil { - t.Fatalf("GenerateKeyPair: %v", err) - } - certStr := mintTestCert(t, time.Now().Add(5*time.Minute)) - if err := store.Write("env-x", privPEM, certStr); err != nil { - t.Fatalf("Write: %v", err) - } - // No leftover temp files in the cert dir. - entries, err := afero.ReadDir(fs, Dir("/home/user")) - if err != nil { - t.Fatalf("ReadDir: %v", err) + privPEM, _ := mustGen(t) + if err := WriteFiles(fs, KeyPath("/h", "x"), CertPath("/h", "x"), privPEM, mintTestCert(t, time.Now().Add(5*time.Minute))); err != nil { + t.Fatalf("WriteFiles: %v", err) } + entries, _ := afero.ReadDir(fs, Dir("/h")) for _, e := range entries { if strings.HasPrefix(e.Name(), ".brev-cert-") { t.Errorf("leftover temp file: %s", e.Name()) } } - // Cert file ends with a newline. - b, err := afero.ReadFile(fs, store.CertPath("env-x")) - if err != nil { - t.Fatalf("ReadFile: %v", err) - } + b, _ := afero.ReadFile(fs, CertPath("/h", "x")) if !strings.HasSuffix(string(b), "\n") { t.Error("cert file should end with newline") } } func TestEnvironmentCertEligible(t *testing.T) { - if !EnvironmentCertEligible(map[string]string{"sshprovider": "certauth"}) { - t.Error("certauth label should be eligible") - } - if EnvironmentCertEligible(map[string]string{"sshprovider": "other"}) { - t.Error("non-certauth label should not be eligible") - } - if EnvironmentCertEligible(map[string]string{}) { - t.Error("missing label should not be eligible") + cases := []struct { + labels map[string]string + want bool + }{ + {map[string]string{"sshprovider": "certauth"}, true}, + {map[string]string{"sshprovider": "other"}, false}, + {map[string]string{}, false}, + } + for _, c := range cases { + if got := EnvironmentCertEligible(c.labels); got != c.want { + t.Errorf("EnvironmentCertEligible(%v)=%v, want %v", c.labels, got, c.want) + } } } func TestSafeFilename(t *testing.T) { - if got := safeFilename("env_123"); got != "env_123" { - t.Errorf("safeFilename(env_123)=%s", got) - } - if got := safeFilename("env/evil"); got != "env-evil" { - t.Errorf("safeFilename(env/evil)=%s", got) + cases := map[string]string{ + "env_123": "env_123", + "env/evil": "env-evil", + "": "default", + "../etc/pw": "..-etc-pw", + } + for in, want := range cases { + if got := safeFilename(in); got != want { + t.Errorf("safeFilename(%q)=%q, want %q", in, got, want) + } } - if got := safeFilename(""); got != "default" { - t.Errorf("safeFilename('')=%s", got) +} + +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 47a2a2015e0524dc15c5d5ef3ff9cc8b1341afdf Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Thu, 20 Aug 2026 12:48:33 -0700 Subject: [PATCH 06/27] remove some unncessary comments --- pkg/cmd/shell/certonly.go | 10 +--------- pkg/entity/entity.go | 27 ++++++++------------------- 2 files changed, 9 insertions(+), 28 deletions(-) diff --git a/pkg/cmd/shell/certonly.go b/pkg/cmd/shell/certonly.go index fa0ee990..411fa578 100644 --- a/pkg/cmd/shell/certonly.go +++ b/pkg/cmd/shell/certonly.go @@ -16,17 +16,9 @@ import ( "github.com/brevdev/brev-cli/pkg/sshcert" ) -// certOnlyTimeout bounds how long the --cert-only helper will wait for a single -// certificate issuance. The Match exec hook runs synchronously during ssh -// config evaluation, so this bounds the worst-case delay ssh sees before a -// login. Generous enough to survive a slow CA but short enough that ssh -// doesn't appear to hang. +// certOnlyTimeout bounds how long to wait for a single certificate issuance const certOnlyTimeout = 15 * time.Second -// certOnlyRequest carries the parameters the helper needs to mint a -// certificate for a specific environment. They are baked into the ssh config's -// Match exec line at config-generation time, so the helper never has to -// resolve %h back to an environment on its own. type certOnlyRequest struct { EnvironmentID string PortID string diff --git a/pkg/entity/entity.go b/pkg/entity/entity.go index e731cb62..c22e1ee4 100644 --- a/pkg/entity/entity.go +++ b/pkg/entity/entity.go @@ -294,25 +294,14 @@ type Workspace struct { HostSSHProxyHostname string `json:"hostSshProxyHostname"` VerbBuildStatus VerbBuildStatus `json:"verbBuildStatus"` VerbYaml string `json:"verbYaml"` - // PortID is the network-member port ID for this user's SSH access to the - // environment, resolved from the Environment connect API during refresh. - // It is required to issue an SSH certificate and is empty for environments - // that fall back to static-key auth (e.g. created before cert support). - PortID string `json:"portId,omitempty"` - // SSHCertEligible is true when the environment's labels opt it into - // certificate-based SSH auth (sshprovider=certauth). When false the SSH - // config falls back to the static brev.pem identity. - SSHCertEligible bool `json:"sshCertEligible,omitempty"` - // 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 { From 2d334a4365ecddebe01b03cdca15277f2f50348f Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Thu, 20 Aug 2026 16:40:54 -0700 Subject: [PATCH 07/27] feat(shell): wire IssueEnvironmentSSHCertificate gRPC, drop stub Replace the placeholder stubCertIssuer with rpcCertIssuer, which calls dev-plane's EnvironmentService.IssueEnvironmentSSHCertificate via the authenticated connect client (register.NewEnvironmentServiceClient). The store satisfies externalnode.TokenProvider via GetAccessToken, so the existing platform credential is reused with no new login. CertIssuer remains an interface so runCertOnly stays unit-testable; add tests for rpcCertIssuer verifying request field mapping and error propagation. Remove the now-unused stub + ErrCertIssuanceUnavailable. --- go.mod | 10 +++---- go.sum | 16 +++++----- pkg/cmd/shell/certonly.go | 53 ++++++++++++++++++++-------------- pkg/cmd/shell/certonly_test.go | 46 ++++++++++++++++++++++++++--- 4 files changed, 86 insertions(+), 39 deletions(-) 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/cmd/shell/certonly.go b/pkg/cmd/shell/certonly.go index 411fa578..a9888670 100644 --- a/pkg/cmd/shell/certonly.go +++ b/pkg/cmd/shell/certonly.go @@ -7,9 +7,12 @@ import ( "strings" "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" @@ -26,14 +29,9 @@ type certOnlyRequest struct { OutKey string // absolute path to write the private key (cert goes to -cert.pub) } -// CertIssuer mints a short-lived SSH certificate for a public key. -// -// The real implementation calls dev-plane's IssueEnvironmentSSHCertificate RPC. -// Until the generated connect client for that RPC is published in the buf -// module brev-cli depends on, a stub implementation returns ErrCertIssuanceUnavailable -// so the rest of the feature compiles and is unit-testable. When the stub is -// active, --cert-only writes no files and the ssh config falls back to the -// static brev.pem identity (see the Match exec design in sshconfigurer.go). +// CertIssuer mints a short-lived SSH certificate for a public key. The +// production implementation calls dev-plane's IssueEnvironmentSSHCertificate RPC; +// the interface lets runCertOnly be unit-tested with a fake issuer. type CertIssuer interface { Issue(ctx context.Context, req certIssueRequest) (certIssueResult, error) } @@ -49,24 +47,35 @@ type certIssueResult struct { Certificate string // single-line OpenSSH authorized key format (the signed cert) } -// ErrCertIssuanceUnavailable is returned by the stub CertIssuer to signal that -// cert issuance is not yet wired in this build. The --cert-only helper treats -// this (and any error) as "mint failed; fall back to static key". -var ErrCertIssuanceUnavailable = fmt.Errorf("ssh certificate issuance is not available in this build") +// environmentCertClient is the subset of the connect EnvironmentServiceClient +// that cert issuance needs. The generated client satisfies this. +type environmentCertClient interface { + IssueEnvironmentSSHCertificate(ctx context.Context, req *connect.Request[devplanev1.IssueEnvironmentSSHCertificateRequest]) (*connect.Response[devplanev1.IssueEnvironmentSSHCertificateResponse], error) +} -// stubCertIssuer is the placeholder CertIssuer used until the real connect-RPC -// client is wired. It always returns ErrCertIssuanceUnavailable. -type stubCertIssuer struct{} +// rpcCertIssuer issues certificates via dev-plane's EnvironmentService. +type rpcCertIssuer struct { + client environmentCertClient +} -func (stubCertIssuer) Issue(_ context.Context, _ certIssueRequest) (certIssueResult, error) { - return certIssueResult{}, ErrCertIssuanceUnavailable +func (r rpcCertIssuer) Issue(ctx context.Context, req certIssueRequest) (certIssueResult, 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 certIssueResult{}, breverrors.WrapAndTrace(err) + } + return certIssueResult{Certificate: res.Msg.GetCertificate()}, nil } -// newCertIssuer constructs the CertIssuer appropriate for this build. Today -// that is the stub; once the buf module publishes IssueEnvironmentSSHCertificate, -// this returns a real connect-RPC-backed issuer. -func newCertIssuer(_ externalnode.TokenProvider, _ string) CertIssuer { - return stubCertIssuer{} +// newCertIssuer constructs an rpcCertIssuer backed by an authenticated +// EnvironmentService connect client. The token provider is the store itself +// (ShellStore satisfies externalnode.TokenProvider via GetAccessToken). +func newCertIssuer(provider externalnode.TokenProvider, baseURL string) CertIssuer { + return rpcCertIssuer{client: register.NewEnvironmentServiceClient(provider, baseURL)} } // certOnlyStore is the minimal store dependency of runCertOnly: just the diff --git a/pkg/cmd/shell/certonly_test.go b/pkg/cmd/shell/certonly_test.go index 61be0bb9..15373895 100644 --- a/pkg/cmd/shell/certonly_test.go +++ b/pkg/cmd/shell/certonly_test.go @@ -9,6 +9,8 @@ import ( "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" @@ -139,10 +141,46 @@ func TestRunCertOnly_FallsBackOnAuthError(t *testing.T) { } } -func TestRunCertOnly_StubIssuerReturnsUnavailable(t *testing.T) { - _, err := stubCertIssuer{}.Issue(context.Background(), certIssueRequest{}) - if !errors.Is(err, ErrCertIssuanceUnavailable) { - t.Fatalf("expected ErrCertIssuanceUnavailable, got %v", err) +// fakeEnvCertClient is a controllable environmentCertClient for testing rpcCertIssuer. +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 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} + res, 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 res.Certificate != "ssh-ed25519-cert-v01@openssh.com AAAA cert" { + t.Errorf("unexpected certificate: %s", res.Certificate) + } + 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) + } +} + +func TestRpcCertIssuer_PropagatesError(t *testing.T) { + client := &fakeEnvCertClient{err: errors.New("permission denied")} + issuer := rpcCertIssuer{client: client} + if _, err := issuer.Issue(context.Background(), certIssueRequest{}); err == nil { + t.Fatal("expected error to propagate") } } From e71969852f435df3dd25f976d656f524a64b2cab Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Thu, 20 Aug 2026 16:53:44 -0700 Subject: [PATCH 08/27] refactor: trim comments to non-obvious gotchas and motivation only --- pkg/cmd/refresh/sshaccess.go | 7 ++--- pkg/cmd/shell/certonly.go | 42 ++++++++++++++--------------- pkg/ssh/sshconfigurer.go | 14 +++++----- pkg/sshcert/sshcert.go | 51 +++++++++++------------------------- 4 files changed, 42 insertions(+), 72 deletions(-) diff --git a/pkg/cmd/refresh/sshaccess.go b/pkg/cmd/refresh/sshaccess.go index fef248e5..1ee4dbc2 100644 --- a/pkg/cmd/refresh/sshaccess.go +++ b/pkg/cmd/refresh/sshaccess.go @@ -118,11 +118,8 @@ func resolveWorkspaceSSH( workspace.SSHUser = access.GetLinuxUser() workspace.SSHProxyHostname = "" - // Retain the port ID and certificate-eligibility label so the SSH config - // generator can emit a per-environment certificate-fetch (Match exec) entry. - // These are only populated when SSH access was resolved via the Environment - // connect API; otherwise they stay zero-valued and the config falls back to - // the static key. + // Retain port_id and cert-eligibility for the SSH config generator's Match + // exec block. Empty when SSH access wasn't resolved via the Environment API. workspace.PortID = access.GetPortId() workspace.SSHCertEligible = sshcert.EnvironmentCertEligible(environment.GetLabels()) diff --git a/pkg/cmd/shell/certonly.go b/pkg/cmd/shell/certonly.go index a9888670..27bbe4fb 100644 --- a/pkg/cmd/shell/certonly.go +++ b/pkg/cmd/shell/certonly.go @@ -19,7 +19,9 @@ import ( "github.com/brevdev/brev-cli/pkg/sshcert" ) -// certOnlyTimeout bounds how long to wait for a single certificate issuance +// certOnlyTimeout bounds the wait for one issuance. The Match exec hook runs +// synchronously during ssh config evaluation, so this bounds the worst-case +// delay ssh sees before a login. const certOnlyTimeout = 15 * time.Second type certOnlyRequest struct { @@ -29,9 +31,8 @@ type certOnlyRequest struct { OutKey string // absolute path to write the private key (cert goes to -cert.pub) } -// CertIssuer mints a short-lived SSH certificate for a public key. The -// production implementation calls dev-plane's IssueEnvironmentSSHCertificate RPC; -// the interface lets runCertOnly be unit-tested with a fake issuer. +// CertIssuer mints a short-lived SSH certificate. The interface lets runCertOnly +// be unit-tested with a fake issuer. type CertIssuer interface { Issue(ctx context.Context, req certIssueRequest) (certIssueResult, error) } @@ -48,12 +49,11 @@ type certIssueResult struct { } // environmentCertClient is the subset of the connect EnvironmentServiceClient -// that cert issuance needs. The generated client satisfies this. +// that cert issuance needs. type environmentCertClient interface { IssueEnvironmentSSHCertificate(ctx context.Context, req *connect.Request[devplanev1.IssueEnvironmentSSHCertificateRequest]) (*connect.Response[devplanev1.IssueEnvironmentSSHCertificateResponse], error) } -// rpcCertIssuer issues certificates via dev-plane's EnvironmentService. type rpcCertIssuer struct { client environmentCertClient } @@ -71,24 +71,22 @@ func (r rpcCertIssuer) Issue(ctx context.Context, req certIssueRequest) (certIss return certIssueResult{Certificate: res.Msg.GetCertificate()}, nil } -// newCertIssuer constructs an rpcCertIssuer backed by an authenticated -// EnvironmentService connect client. The token provider is the store itself -// (ShellStore satisfies externalnode.TokenProvider via GetAccessToken). +// newCertIssuer returns an rpcCertIssuer. The token provider is the store +// itself — ShellStore satisfies externalnode.TokenProvider via GetAccessToken, +// so the existing platform credential is reused with no new login. func newCertIssuer(provider externalnode.TokenProvider, baseURL string) CertIssuer { return rpcCertIssuer{client: register.NewEnvironmentServiceClient(provider, baseURL)} } -// certOnlyStore is the minimal store dependency of runCertOnly: just the -// ability to read the existing platform credential. Keeping this narrow lets -// --cert-only be unit-tested with a trivial fake store. +// certOnlyStore is the minimal store dependency (GetAccessToken only), kept +// narrow so --cert-only is unit-testable with a trivial fake store. type certOnlyStore interface { GetAccessToken() (string, error) } -// runCertOnly implements `brev shell --cert-only`: a headless mint-and-write -// used by the ssh config's Match exec hook. On any failure it writes nothing -// and returns non-zero so ssh drops the cert IdentityFile and falls back to -// the static brev.pem. It never prompts — that would hang the ssh invocation. +// runCertOnly is invoked by the ssh config's Match exec hook. On any failure it +// writes nothing and returns non-zero so ssh drops the cert IdentityFile and +// falls back to the static brev.pem. It never prompts — that would hang ssh. func runCertOnly(store ShellStore, req certOnlyRequest) error { return runCertOnlyWith(store, afero.NewOsFs(), newCertIssuer(store, config.GlobalConfig.GetBrevPublicAPIURL()), req) } @@ -129,7 +127,6 @@ func runCertOnlyWith(store certOnlyStore, fs afero.Fs, issuer CertIssuer, req ce return nil } -// certOnlyFlags holds the parsed --cert-only flag values. type certOnlyFlags struct { certOnly bool env string @@ -138,9 +135,8 @@ type certOnlyFlags struct { outKey string } -// addCertOnlyFlags registers the --cert-only family of flags on the shell -// command. They are hidden from help since they are an implementation detail of -// the ssh config's Match exec hook, not a user-facing mode. +// addCertOnlyFlags registers the hidden --cert-only flags. They are an +// implementation detail of the ssh config's Match exec hook, not user-facing. func addCertOnlyFlags(cmd *cobra.Command, f *certOnlyFlags) { cmd.Flags().BoolVar(&f.certOnly, "cert-only", false, "mint an SSH certificate and write it to disk, then exit (used by the ssh config Match exec hook)") cmd.Flags().StringVar(&f.env, "env", "", "(--cert-only) environment ID to mint a certificate for") @@ -153,9 +149,9 @@ func addCertOnlyFlags(cmd *cobra.Command, f *certOnlyFlags) { } } -// validateCertOnly returns an error if --cert-only is set without all four -// required parameters. The flags are hidden and only set by the generated ssh -// config, so the inverse (params without --cert-only) is not a real scenario. +// validateCertOnly errors if --cert-only is set without all four required +// params. The flags are hidden and only set by the generated ssh config, so the +// inverse (params without --cert-only) isn't a real scenario. func validateCertOnly(f certOnlyFlags) error { if !f.certOnly { return nil diff --git a/pkg/ssh/sshconfigurer.go b/pkg/ssh/sshconfigurer.go index b7d01b5a..946548fa 100644 --- a/pkg/ssh/sshconfigurer.go +++ b/pkg/ssh/sshconfigurer.go @@ -462,12 +462,10 @@ func makeSSHConfigEntryV2(workspace entity.Workspace, privateKeyPath string, clo val := fmt.Sprintf("%s%s", sshVal, hostSSHVal) - // For cert-eligible workspaces, prepend a Match exec block that mints a - // short-lived per-environment SSH certificate on connect. The block's - // IdentityFile accumulates with the Host block's static brev.pem identity - // when the exec succeeds (cert tried first, static as fallback); when the - // exec fails (mint error, CA down, not-yet-wired issuer) the Match block's - // IdentityFile is dropped and ssh falls back to the static key. + // ssh accumulates IdentityFile across the Match block and the Host block + // below when the exec succeeds (cert first, static brev.pem as fallback); + // when the exec fails the Match block's IdentityFile is dropped, so ssh + // falls back to the static key. if certMatch := makeCertMatchEntry(workspace, home); certMatch != "" { val = certMatch + val } @@ -478,8 +476,8 @@ func makeCloudflareSSHProxyCommand(cloudflaredBinaryPath string, hostname string return fmt.Sprintf("%s access ssh --hostname %s", cloudflaredBinaryPath, hostname) } -// makeCertMatchEntry returns the Match exec block for a cert-eligible -// workspace, or "" if the workspace is not eligible or home is empty (WSL). +// makeCertMatchEntry returns the Match exec block for a cert-eligible workspace, +// or "" if not eligible or home is empty (WSL, deferred). func makeCertMatchEntry(workspace entity.Workspace, home string) string { if home == "" || !workspace.SSHCertEligible || workspace.PortID == "" { return "" diff --git a/pkg/sshcert/sshcert.go b/pkg/sshcert/sshcert.go index 1dcaeb1b..0d2fd790 100644 --- a/pkg/sshcert/sshcert.go +++ b/pkg/sshcert/sshcert.go @@ -1,12 +1,5 @@ // Package sshcert manages short-lived, per-environment SSH certificates and // their backing ephemeral keypairs on disk for use by the OpenSSH client. -// -// A fresh ed25519 keypair is generated each time a certificate is renewed; -// the (private key, certificate) pair is cached on disk for the certificate's -// validity window so repeated ssh invocations do not re-hit the CA. Files live -// under ~/.brev/ssh-certs/{,-cert.pub} (OpenSSH auto-loads -// -cert.pub). Writes are atomic (temp + rename) and the private -// key is written 0600. package sshcert import ( @@ -27,31 +20,25 @@ import ( const certSubDir = "ssh-certs" -// DefaultRenewalMargin is how long before a certificate's not-after time we -// consider it expired and in need of renewal. Renewing slightly early avoids a -// race where a certificate expires between the mint and the subsequent ssh use. +// 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 (kept in sync -// because that package is internal to dev-plane). +// Label constants mirroring dev-plane's internal/labels package (internal to +// dev-plane, so duplicated here). const ( LabelKeySSHProvider = "sshprovider" SSHProviderCertAuth = "certauth" ) -// EnvironmentCertEligible reports whether an environment's labels opt it into -// certificate-based SSH auth. func EnvironmentCertEligible(labels map[string]string) bool { return labels[LabelKeySSHProvider] == SSHProviderCertAuth } -// Dir returns the on-disk directory holding cached certificates for home. func Dir(home string) string { return filepath.Join(home, ".brev", certSubDir) } -// safeFilename reduces an environment ID to a safe file name. Environment IDs -// are UUIDs today, so this is mostly defensive. func safeFilename(envID string) string { s := strings.TrimSpace(envID) if s == "" { @@ -73,21 +60,19 @@ func safeFilename(envID string) string { return out } -// KeyPath returns the on-disk private-key path for the given environment. func KeyPath(home, envID string) string { return filepath.Join(Dir(home), safeFilename(envID)) } -// CertPath returns the on-disk certificate path for the given environment, -// following OpenSSH's -cert.pub convention. +// CertPath follows OpenSSH's -cert.pub convention, so a single +// IdentityFile directive loads both the key and the cert. func CertPath(home, envID string) string { return KeyPath(home, envID) + "-cert.pub" } -// GenerateKeyPair generates a fresh ed25519 keypair. It returns the private -// key in OpenSSH PEM format (for IdentityFile) and the public key as a -// single-line OpenSSH authorized-key string (the format the issuance RPC -// expects as its public_key field). +// GenerateKeyPair returns the private key in OpenSSH PEM format (for +// IdentityFile) and the public key as a single-line authorized-key string (the +// format the issuance RPC expects as its public_key field). func GenerateKeyPair() (privKeyPEM []byte, pubKeyOpenSSH string, err error) { pub, priv, err := ed25519.GenerateKey(rand.Reader) if err != nil { @@ -105,8 +90,6 @@ func GenerateKeyPair() (privKeyPEM []byte, pubKeyOpenSSH string, err error) { return pem.EncodeToMemory(block), pubKeyOpenSSH, nil } -// ParseCertificate parses an OpenSSH authorized-key-formatted certificate -// string (the format returned by the issuance RPC) into an *ssh.Certificate. func ParseCertificate(certOpenSSH string) (*ssh.Certificate, error) { certOpenSSH = strings.TrimSpace(certOpenSSH) if certOpenSSH == "" { @@ -129,8 +112,7 @@ func ParseCertificate(certOpenSSH string) (*ssh.Certificate, error) { return cert, nil } -// CertValidAt reports whether the certificate is valid at now+margin. A -// ValidBefore of 0 or ^uint64(0) means "forever" per the SSH spec. +// CertValidAt: a ValidBefore of 0 or ^uint64(0) means "forever" per the SSH spec. func CertValidAt(cert *ssh.Certificate, now time.Time, margin time.Duration) bool { if cert == nil { return false @@ -143,9 +125,8 @@ func CertValidAt(cert *ssh.Certificate, now time.Time, margin time.Duration) boo return now.Add(margin).Unix() < notAfter } -// HasValidCertAt reports whether a non-expired certificate (with margin) is -// present at certPath. A missing file or unparseable certificate returns -// (false, nil) so the caller mints a fresh one rather than failing. +// HasValidCertAt returns (false, nil) for a missing or corrupt cert so the +// caller mints a fresh one rather than failing the whole ssh attempt. func HasValidCertAt(fs afero.Fs, certPath string, now time.Time, margin time.Duration) (bool, error) { exists, err := afero.Exists(fs, certPath) if err != nil { @@ -165,8 +146,6 @@ func HasValidCertAt(fs afero.Fs, certPath string, now time.Time, margin time.Dur return CertValidAt(cert, now, margin), nil } -// WriteFiles writes the private key and certificate to the given paths -// atomically. The private key is 0600; the certificate is 0644. 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) @@ -180,8 +159,8 @@ func WriteFiles(fs afero.Fs, keyPath, certPath string, privKeyPEM []byte, certOp return writeAtomic(fs, certPath, []byte(certOpenSSH), 0o644) } -// writeAtomic writes data to path via a temp file in the same directory and -// renames it into place, so a reader never observes a partial write. +// 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") @@ -201,5 +180,5 @@ func writeAtomic(fs afero.Fs, path string, data []byte, mode os.FileMode) error if err := fs.Chmod(tmpName, mode); err != nil { return breverrors.WrapAndTrace(err) } - return fs.Rename(tmpName, path) + return breverrors.WrapAndTrace(fs.Rename(tmpName, path)) } From fc9849de40d1579c3a2eebf33e12dc6b8feca904 Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Thu, 20 Aug 2026 17:40:21 -0700 Subject: [PATCH 09/27] remove more comments --- pkg/cmd/shell/certonly.go | 31 ++++++++++--------------------- 1 file changed, 10 insertions(+), 21 deletions(-) diff --git a/pkg/cmd/shell/certonly.go b/pkg/cmd/shell/certonly.go index 27bbe4fb..cf1bcfbc 100644 --- a/pkg/cmd/shell/certonly.go +++ b/pkg/cmd/shell/certonly.go @@ -19,8 +19,7 @@ import ( "github.com/brevdev/brev-cli/pkg/sshcert" ) -// certOnlyTimeout bounds the wait for one issuance. The Match exec hook runs -// synchronously during ssh config evaluation, so this bounds the worst-case +// certOnlyTimeout bounds the wait for one issuance; bounds the worst-case // delay ssh sees before a login. const certOnlyTimeout = 15 * time.Second @@ -31,8 +30,6 @@ type certOnlyRequest struct { OutKey string // absolute path to write the private key (cert goes to -cert.pub) } -// CertIssuer mints a short-lived SSH certificate. The interface lets runCertOnly -// be unit-tested with a fake issuer. type CertIssuer interface { Issue(ctx context.Context, req certIssueRequest) (certIssueResult, error) } @@ -41,15 +38,13 @@ type certIssueRequest struct { EnvironmentID string PortID string LinuxUser string - PublicKey string // single-line OpenSSH authorized key format + PublicKey string } type certIssueResult struct { - Certificate string // single-line OpenSSH authorized key format (the signed cert) + Certificate string } -// environmentCertClient is the subset of the connect EnvironmentServiceClient -// that cert issuance needs. type environmentCertClient interface { IssueEnvironmentSSHCertificate(ctx context.Context, req *connect.Request[devplanev1.IssueEnvironmentSSHCertificateRequest]) (*connect.Response[devplanev1.IssueEnvironmentSSHCertificateResponse], error) } @@ -71,41 +66,35 @@ func (r rpcCertIssuer) Issue(ctx context.Context, req certIssueRequest) (certIss return certIssueResult{Certificate: res.Msg.GetCertificate()}, nil } -// newCertIssuer returns an rpcCertIssuer. The token provider is the store -// itself — ShellStore satisfies externalnode.TokenProvider via GetAccessToken, -// so the existing platform credential is reused with no new login. func newCertIssuer(provider externalnode.TokenProvider, baseURL string) CertIssuer { return rpcCertIssuer{client: register.NewEnvironmentServiceClient(provider, baseURL)} } -// certOnlyStore is the minimal store dependency (GetAccessToken only), kept -// narrow so --cert-only is unit-testable with a trivial fake store. type certOnlyStore interface { GetAccessToken() (string, error) } -// runCertOnly is invoked by the ssh config's Match exec hook. On any failure it -// writes nothing and returns non-zero so ssh drops the cert IdentityFile and -// falls back to the static brev.pem. It never prompts — that would hang ssh. +// runCertOnly is invoked by the ssh config's Match exec hook. On any failure it returns non-zero +// so ssh falls back to the static brev.pem. Must not prompt, as that would hang ssh. func runCertOnly(store ShellStore, req certOnlyRequest) error { return runCertOnlyWith(store, afero.NewOsFs(), newCertIssuer(store, config.GlobalConfig.GetBrevPublicAPIURL()), req) } func runCertOnlyWith(store certOnlyStore, fs afero.Fs, issuer CertIssuer, req certOnlyRequest) error { if _, err := store.GetAccessToken(); err != nil { - fmt.Fprintln(os.Stderr, "brev: not logged in. Run `brev login` and retry.") + _, _ = fmt.Fprintln(os.Stderr, "brev: no auth method found. Run `brev login` and retry.") return breverrors.WrapAndTrace(err) } 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) + _, _ = 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) + _, _ = fmt.Fprintf(os.Stderr, "brev: failed to generate keypair: %v\n", err) return breverrors.WrapAndTrace(err) } ctx, cancel := context.WithTimeout(context.Background(), certOnlyTimeout) @@ -117,11 +106,11 @@ func runCertOnlyWith(store certOnlyStore, fs afero.Fs, issuer CertIssuer, req ce PublicKey: pubKeyOpenSSH, }) if err != nil { - fmt.Fprintf(os.Stderr, "brev: could not issue ssh certificate: %v\n", err) + _, _ = 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, res.Certificate); err != nil { - fmt.Fprintf(os.Stderr, "brev: failed to write cert files: %v\n", err) + _, _ = fmt.Fprintf(os.Stderr, "brev: failed to write cert files: %v\n", err) return breverrors.WrapAndTrace(err) } return nil From 62753e1f9ce27d42f3306a3795a00388d00d3e9e Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Fri, 21 Aug 2026 09:54:00 -0700 Subject: [PATCH 10/27] fix(ssh): use absolute brev path in Match exec, not bare 'brev' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Match exec line invoked bare 'brev', which resolves via PATH to whatever brev is installed — potentially an older binary lacking --cert-only. That binary errors with 'unknown flag: --cert-only', exits non-zero, and the Match block's IdentityFile is dropped, so ssh silently falls back to the static brev.pem and the cert path never works. Emit the absolute path to the running binary (os.Executable) instead, so the config invokes the same build that generated it. --- pkg/ssh/sshconfigurer.go | 13 +++++++++++-- pkg/ssh/sshconfigurer_test.go | 23 +++++++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/pkg/ssh/sshconfigurer.go b/pkg/ssh/sshconfigurer.go index 946548fa..83661a10 100644 --- a/pkg/ssh/sshconfigurer.go +++ b/pkg/ssh/sshconfigurer.go @@ -5,6 +5,7 @@ import ( "encoding/xml" "fmt" "log" + "os" "regexp" "strings" "text/template" @@ -478,14 +479,22 @@ func makeCloudflareSSHProxyCommand(cloudflaredBinaryPath string, hostname string // makeCertMatchEntry returns the Match exec block for a cert-eligible workspace, // or "" if not eligible or home is empty (WSL, deferred). +// +// The exec command uses the absolute path to the running brev binary so it +// resolves to the same build that generated this config (which has --cert-only), +// not whatever stale `brev` is first in PATH at ssh time. func makeCertMatchEntry(workspace entity.Workspace, home string) string { if home == "" || !workspace.SSHCertEligible || workspace.PortID == "" { return "" } + brevBin, err := os.Executable() + if err != nil { + brevBin = "brev" // fallback; degraded but no worse than the old bare-brev behavior + } alias := string(workspace.GetLocalIdentifier()) certKeyPath := sshcert.KeyPath(home, workspace.ID) - exec := fmt.Sprintf("brev shell --cert-only --env %s --port %s --user %s --out-key '%s'", - workspace.ID, workspace.PortID, workspace.GetSSHUser(), certKeyPath) + exec := fmt.Sprintf("'%s' shell --cert-only --env %s --port %s --user %s --out-key '%s'", + brevBin, workspace.ID, workspace.PortID, workspace.GetSSHUser(), certKeyPath) return fmt.Sprintf("Match host %s exec %q\n IdentityFile %q\n", alias, exec, certKeyPath) } diff --git a/pkg/ssh/sshconfigurer_test.go b/pkg/ssh/sshconfigurer_test.go index 2a487e01..a348ab8b 100644 --- a/pkg/ssh/sshconfigurer_test.go +++ b/pkg/ssh/sshconfigurer_test.go @@ -2,6 +2,7 @@ package ssh import ( "fmt" + "os" "strings" "testing" @@ -1036,3 +1037,25 @@ func TestMakeSSHConfigEntryV2_IneligibleWorkspaceNoCertMatch(t *testing.T) { t.Error("static key should still be present") } } + +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 lacking + // --cert-only (the bug where the cert path silently fell back to static). + w := entity.Workspace{ + ID: "env-abc", Name: "n", SSHUser: "ubuntu", + SSHCertEligible: true, PortID: "port-1", + } + got := makeCertMatchEntry(w, "/home/u") + exe, err := os.Executable() + if err != nil { + t.Skip("os.Executable unavailable; cannot assert path") + } + want := fmt.Sprintf("'%s' shell --cert-only", 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 shell") { + t.Errorf("Match exec must not use bare `brev`: %s", got) + } +} From 96c18b84ee5b43e8580dd1061e802e04f71dc6be Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Fri, 21 Aug 2026 11:14:41 -0700 Subject: [PATCH 11/27] docs: design SSH certificate Match exec compatibility --- ...ificate-match-exec-compatibility-design.md | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-21-ssh-certificate-match-exec-compatibility-design.md diff --git a/docs/superpowers/specs/2026-08-21-ssh-certificate-match-exec-compatibility-design.md b/docs/superpowers/specs/2026-08-21-ssh-certificate-match-exec-compatibility-design.md new file mode 100644 index 00000000..2c700f04 --- /dev/null +++ b/docs/superpowers/specs/2026-08-21-ssh-certificate-match-exec-compatibility-design.md @@ -0,0 +1,65 @@ +# SSH Certificate Match Exec Compatibility Design + +## Goal + +Make both direct `ssh ` and `brev shell ` mint and use a VM-scoped SSH certificate through the generated OpenSSH `Match exec` block, without weakening the normal `brev shell` positional-argument contract. + +## Verified Current State + +- `dev-plane` issues five-minute user certificates whose principal is scoped to the environment ID and Linux account. +- Certificate issuance requires the authenticated caller to have an active SSH-access record matching the requested environment, port, and Linux user. +- VM bootstrap installs a `cert-authority,principals="brev:v1:vm::login:"` entry. +- A live `ssh -vvv` probe against `automatic-lavender-swordfish` ran the generated `Match exec`, obtained a certificate, and authenticated with the ED25519 certificate rather than `brev.pem`. +- `brev shell ` ultimately invokes system `ssh` with the workspace alias, so it consumes the same generated SSH configuration as a manual SSH command. +- The committed CLI generator currently emits `brev shell --cert-only ...` without the positional workspace argument, while the committed command still requires exactly one argument. + +## Command Contract + +The generated conditional entry will include the workspace alias as the normal `brev shell` positional argument: + +```text +Match host automatic-lavender-swordfish exec "'' shell automatic-lavender-swordfish --env emyxcusgq --port nport-... --linux-user ubuntu --out-key '/Users/example/.brev/ssh-certs/emyxcusgq'" + IdentityFile "/Users/example/.brev/ssh-certs/emyxcusgq" +``` + +The hidden `--cert-only` boolean will be removed. Certificate mode will be inferred from the certificate-specific flags: + +- `--env` +- `--port` +- `--linux-user` +- `--out-key` + +If none of these flags is supplied, `brev shell ` follows the normal interactive shell path. If any is supplied, the command treats the invocation as an internal certificate request and requires all four. Partial input fails before any authentication, API, or file operation. + +The certificate Linux-account flag is named `--linux-user`, not `--user`, because the root Brev command already owns a persistent `--user` flag. This avoids shadowing existing per-user configuration behavior. + +`brev shell` continues to use `cobra.ExactArgs(1)` for every invocation. The workspace argument is required even though certificate issuance uses the explicit environment, port, and Linux-user fields. + +## Data Flow + +1. `brev refresh` resolves the current user's SSH-access record and network port from `dev-plane`. +2. A workspace labeled `sshprovider=certauth` produces a `Match host ... exec ...` block containing its alias, immutable environment ID, SSH-access port ID, Linux user, certificate-key path, and the absolute path of the CLI binary generating the configuration. +3. OpenSSH evaluates the block for either manual `ssh ` or the system SSH command launched by `brev shell `. +4. The hidden certificate flags select the certificate path in `RunE`. +5. The CLI reuses a sufficiently fresh cached certificate or creates an ephemeral Ed25519 keypair, requests a VM-scoped certificate from `IssueEnvironmentSSHCertificate`, and atomically writes the key and matching `-cert.pub` file. +6. A successful hook adds the certificate identity to the effective SSH configuration. The normal host block also retains `brev.pem` as a compatibility fallback. + +## Error And Compatibility Behavior + +- A missing or partial certificate-flag bundle exits nonzero and does not enter the normal interactive shell path. +- Authentication, certificate issuance, parsing, and file-write failures remain nonzero so OpenSSH excludes the conditional certificate identity and can use the existing static-key fallback. +- The absolute CLI path remains in the hook so configuration generated by a development or non-PATH binary invokes that same binary. +- `dev-plane` RPCs, protobufs, authorization policies, principal construction, and VM bootstrap remain unchanged. +- Existing unrelated worktrees and changes remain untouched. No branch is pushed. + +## Testing And Verification + +Implementation will follow test-first development: + +1. Add a generator regression test requiring the positional workspace alias, `--linux-user`, and the absence of `--cert-only`; verify it fails against the current generator. +2. Add flag-classification tests for no certificate flags, a complete certificate bundle, and each partial bundle; verify the new expectations fail before changing production behavior. +3. Implement the minimal generator and shell-routing changes, then run the focused `pkg/cmd/shell`, `pkg/ssh`, `pkg/cmd/refresh`, and `pkg/sshcert` tests. Host-specific unrelated JetBrains test failures will be reported separately if they recur. +4. Run `gofmt` on touched Go files, `git diff --check`, and build the CLI. +5. Regenerate the local SSH configuration with the built binary and inspect the effective entry. +6. Run a non-ControlMaster verbose SSH probe and require evidence that the server accepts the generated certificate. +7. Run `brev shell automatic-lavender-swordfish`, exit the remote shell cleanly, and confirm it follows the same `Match exec` certificate path. From 91ff6b461947b1f66a9156935fcbb6a835f06128 Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Fri, 21 Aug 2026 11:55:01 -0700 Subject: [PATCH 12/27] docs: plan SSH certificate Match exec compatibility --- ...sh-certificate-match-exec-compatibility.md | 360 ++++++++++++++++++ 1 file changed, 360 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-21-ssh-certificate-match-exec-compatibility.md diff --git a/docs/superpowers/plans/2026-08-21-ssh-certificate-match-exec-compatibility.md b/docs/superpowers/plans/2026-08-21-ssh-certificate-match-exec-compatibility.md new file mode 100644 index 00000000..81e68e01 --- /dev/null +++ b/docs/superpowers/plans/2026-08-21-ssh-certificate-match-exec-compatibility.md @@ -0,0 +1,360 @@ +# SSH Certificate Match Exec Compatibility Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make direct `ssh ` and `brev shell ` use the same VM-scoped certificate path while retaining `brev shell`'s exact-one-argument contract. + +**Architecture:** Keep `dev-plane`'s verified certificate issuance and VM authorization unchanged. Generate a host-inclusive `Match exec` command in `brev-cli`, infer certificate mode from a complete bundle of hidden certificate flags, and retain the static key as OpenSSH's fallback when the hook exits nonzero. + +**Tech Stack:** Go, Cobra/pflag, OpenSSH client configuration, ConnectRPC, `golang.org/x/crypto/ssh` + +**Spec:** `docs/superpowers/specs/2026-08-21-ssh-certificate-match-exec-compatibility-design.md` + +## Global Constraints + +- `brev shell` always requires exactly one positional workspace argument. +- Certificate mode has no `--cert-only` marker; any certificate-specific flag selects it and all four fields are required. +- The certificate Linux account uses hidden `--linux-user`, never the root command's persistent `--user` flag. +- The generated hook uses the absolute running Brev binary path and retains `brev.pem` as a fallback identity. +- `dev-plane` RPCs, protobufs, authorization, principal construction, and bootstrap remain unchanged. +- Preserve unrelated worktrees and unstaged edits; do not push. + +## File Structure + +- `pkg/ssh/sshconfigurer.go`: generate the host-inclusive certificate hook. +- `pkg/ssh/sshconfigurer_test.go`: protect the generated OpenSSH contract. +- `pkg/cmd/shell/certonly.go`: register and classify the hidden certificate fields. +- `pkg/cmd/shell/certonly_test.go`: protect complete, absent, and partial flag behavior. +- `pkg/cmd/shell/shell.go`: retain `ExactArgs(1)` and route complete certificate requests before interactive shell execution. +- No `dev-plane` source file changes. + +--- + +### Task 1: Generate A Host-Inclusive Certificate Hook + +**Files:** +- Modify: `pkg/ssh/sshconfigurer_test.go:934-1061` +- Modify: `pkg/ssh/sshconfigurer.go:480-499` + +**Interfaces:** +- Consumes: `entity.Workspace.GetLocalIdentifier()`, `entity.Workspace.GetSSHUser()`, `sshcert.KeyPath(home, environmentID)`, and `os.Executable()`. +- Produces: `makeCertMatchEntry(workspace entity.Workspace, home string) string` containing `shell my-env --env env-abc --port port-1 --linux-user ubuntu --out-key /home/u/.brev/ssh-certs/env-abc` for the test fixture. + +- [ ] **Step 1: Write the failing generator assertions** + +Update `TestMakeCertMatchEntry_EligibleWorkspace` so the expected command contract is literal and independently derived: + +```go +if !strings.Contains(got, " shell my-env --env env-abc") { + t.Errorf("missing positional workspace alias: %s", got) +} +if !strings.Contains(got, "--port port-1") { + t.Errorf("missing --port port-1: %s", got) +} +if !strings.Contains(got, "--linux-user ubuntu") { + t.Errorf("missing --linux-user ubuntu: %s", got) +} +if strings.Contains(got, "--cert-only") { + t.Errorf("Match exec must infer certificate mode from its hidden fields: %s", got) +} +if strings.Contains(got, " --user ") { + t.Errorf("Match exec must not shadow Brev's persistent --user flag: %s", got) +} +``` + +Update the absolute-binary assertion in `TestMakeCertMatchEntry_UsesAbsoluteBrevPath`: + +```go +want := fmt.Sprintf("'%s' shell n --env env-abc", exe) +if !strings.Contains(got, want) { + t.Errorf("expected Match exec to use absolute brev path %q; got: %s", want, got) +} +``` + +- [ ] **Step 2: Run the focused test to verify RED** + +Run: + +```bash +go test ./pkg/ssh -run 'TestMakeCertMatchEntry_(EligibleWorkspace|UsesAbsoluteBrevPath)$' -count=1 +``` + +Expected: FAIL because the current generator omits `my-env`, emits `--cert-only`, and emits `--user` instead of `--linux-user`. + +- [ ] **Step 3: Implement the minimal generator change** + +Replace the hook command and update its compatibility comment: + +```go +// The exec command uses the absolute path to the running brev binary so it +// resolves to the build that generated this config. The workspace alias keeps +// the normal `brev shell ` argument contract intact. +func makeCertMatchEntry(workspace entity.Workspace, home string) string { + if home == "" || !workspace.SSHCertEligible || workspace.PortID == "" { + return "" + } + brevBin, err := os.Executable() + if err != nil { + brevBin = "brev" + } + alias := string(workspace.GetLocalIdentifier()) + certKeyPath := sshcert.KeyPath(home, workspace.ID) + exec := fmt.Sprintf("'%s' shell %s --env %s --port %s --linux-user %s --out-key '%s'", + brevBin, alias, workspace.ID, workspace.PortID, workspace.GetSSHUser(), certKeyPath) + return fmt.Sprintf("Match host %s exec %q\n IdentityFile %q\n", alias, exec, certKeyPath) +} +``` + +- [ ] **Step 4: Verify GREEN** + +Run: + +```bash +gofmt -w pkg/ssh/sshconfigurer.go pkg/ssh/sshconfigurer_test.go +go test ./pkg/ssh -run 'TestMakeCertMatchEntry|TestMakeSSHConfigEntryV2_(EligibleWorkspaceIncludesCertMatch|IneligibleWorkspaceNoCertMatch)' -count=1 +``` + +Expected: PASS. + +- [ ] **Step 5: Commit the generator contract** + +```bash +git add pkg/ssh/sshconfigurer.go pkg/ssh/sshconfigurer_test.go +git diff --cached --check +git commit -m "fix(ssh): include workspace in certificate hook" +``` + +--- + +### Task 2: Infer Certificate Mode From Hidden Fields + +**Files:** +- Modify: `pkg/cmd/shell/certonly_test.go:187-217` +- Modify: `pkg/cmd/shell/certonly.go:119-165` +- Modify: `pkg/cmd/shell/shell.go:54-95` + +**Interfaces:** +- Consumes: Cobra's hidden string flags and the existing `runCertOnly(store ShellStore, req certOnlyRequest) error` path. +- Produces: `certOnlyFlags.requested() bool`; hidden `--env`, `--port`, `--linux-user`, and `--out-key`; exact-one-argument shell routing. + +- [ ] **Step 1: Replace the broken argument-bypass test with failing flag-contract tests** + +Add `github.com/spf13/cobra` to `certonly_test.go` imports. Replace `TestValidateCertOnly` and remove `TestShellCmdArgs_CertOnlyBypassesArgRequirement` with: + +```go +func TestValidateCertOnly(t *testing.T) { + tests := []struct { + name string + flags certOnlyFlags + wantErr bool + }{ + {name: "normal shell"}, + {name: "complete certificate request", flags: certOnlyFlags{env: "e", port: "p", user: "u", outKey: "/k"}}, + {name: "environment only", flags: certOnlyFlags{env: "e"}, wantErr: true}, + {name: "port only", flags: certOnlyFlags{port: "p"}, wantErr: true}, + {name: "linux user only", flags: certOnlyFlags{user: "u"}, wantErr: true}, + {name: "output key only", flags: certOnlyFlags{outKey: "/k"}, wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateCertOnly(tt.flags) + if (err != nil) != tt.wantErr { + t.Fatalf("validateCertOnly() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + +func TestAddCertOnlyFlagsUsesImplicitCertificateMode(t *testing.T) { + cmd := &cobra.Command{} + flags := certOnlyFlags{} + addCertOnlyFlags(cmd, &flags) + + if flag := cmd.Flags().Lookup("cert-only"); flag != nil { + t.Error("--cert-only should not be registered") + } + if flag := cmd.Flags().Lookup("user"); flag != nil { + t.Error("certificate flags must not shadow persistent --user") + } + for _, name := range []string{"env", "port", "linux-user", "out-key"} { + flag := cmd.Flags().Lookup(name) + if flag == nil { + t.Errorf("--%s is not registered", name) + continue + } + if !flag.Hidden { + t.Errorf("--%s must remain hidden", name) + } + } +} +``` + +- [ ] **Step 2: Run the focused tests to verify RED** + +Run: + +```bash +go test ./pkg/cmd/shell -run 'TestValidateCertOnly|TestAddCertOnlyFlagsUsesImplicitCertificateMode' -count=1 +``` + +Expected: FAIL because partial certificate fields currently pass validation, `--cert-only` still exists, `--linux-user` is absent, and local `--user` is registered. + +- [ ] **Step 3: Implement certificate-field classification** + +Replace the flag structure, registration, and validation with: + +```go +type certOnlyFlags struct { + env string + port string + user string + outKey string +} + +func (f certOnlyFlags) requested() bool { + return f.env != "" || f.port != "" || f.user != "" || f.outKey != "" +} + +func addCertOnlyFlags(cmd *cobra.Command, f *certOnlyFlags) { + cmd.Flags().StringVar(&f.env, "env", "", "environment ID for internal SSH certificate issuance") + cmd.Flags().StringVar(&f.port, "port", "", "network-member port ID for internal SSH certificate issuance") + cmd.Flags().StringVar(&f.user, "linux-user", "", "Linux user for internal SSH certificate issuance") + cmd.Flags().StringVar(&f.outKey, "out-key", "", "private-key path for internal SSH certificate issuance") + + for _, name := range []string{"env", "port", "linux-user", "out-key"} { + _ = cmd.Flags().MarkHidden(name) + } +} + +func validateCertOnly(f certOnlyFlags) error { + if !f.requested() { + return nil + } + var missing []string + if f.env == "" { + missing = append(missing, "--env") + } + if f.port == "" { + missing = append(missing, "--port") + } + if f.user == "" { + missing = append(missing, "--linux-user") + } + if f.outKey == "" { + missing = append(missing, "--out-key") + } + if len(missing) > 0 { + return fmt.Errorf("SSH certificate request requires %s", strings.Join(missing, ", ")) + } + return nil +} +``` + +Restore `NewCmdShell`'s argument contract and route inferred requests: + +```go +Args: cobra.ExactArgs(1), +``` + +```go +if err := validateCertOnly(certFlags); err != nil { + return breverrors.WrapAndTrace(err) +} +if certFlags.requested() { + return runCertOnly(store, certOnlyRequest{ + EnvironmentID: certFlags.env, + PortID: certFlags.port, + LinuxUser: certFlags.user, + OutKey: certFlags.outKey, + }) +} +``` + +- [ ] **Step 4: Verify GREEN** + +Run: + +```bash +gofmt -w pkg/cmd/shell/certonly.go pkg/cmd/shell/certonly_test.go pkg/cmd/shell/shell.go +go test ./pkg/cmd/shell -count=1 +``` + +Expected: PASS. + +- [ ] **Step 5: Commit the implicit certificate mode** + +```bash +git add pkg/cmd/shell/certonly.go pkg/cmd/shell/certonly_test.go pkg/cmd/shell/shell.go +git diff --cached --check +git commit -m "fix(shell): infer SSH certificate requests from fields" +``` + +--- + +### Task 3: Verify The CLI End To End + +**Files:** +- Verify only: all touched CLI files +- Generated local artifact: ignored `brev` binary +- External local state: `/Users/pratpatel/.brev/ssh_config` and `/Users/pratpatel/.brev/ssh-certs/emyxcusgq*` + +**Interfaces:** +- Consumes: the built CLI, authenticated Brev API access, generated OpenSSH config, and `automatic-lavender-swordfish`. +- Produces: fresh evidence that both manual SSH and `brev shell` reach the certificate-enabled target. + +- [ ] **Step 1: Run focused CLI regression suites** + +```bash +go test ./pkg/cmd/shell ./pkg/cmd/refresh ./pkg/sshcert -count=1 +go test ./pkg/ssh -run 'TestMakeCertMatchEntry|TestMakeSSHConfigEntryV2_(EligibleWorkspaceIncludesCertMatch|IneligibleWorkspaceNoCertMatch)' -count=1 +``` + +Expected: PASS. Also run `go test ./pkg/ssh -count=1`; if the known JetBrains Gateway path-dependent tests fail, record them separately and require every SSH-certificate-focused test above to remain green. + +- [ ] **Step 2: Format, build, and check the worktree** + +From `/Users/pratpatel/code/brev-cli-ssh-certs`, run: + +```bash +gofmt -w pkg/ssh/sshconfigurer.go pkg/ssh/sshconfigurer_test.go pkg/cmd/shell/certonly.go pkg/cmd/shell/certonly_test.go pkg/cmd/shell/shell.go +make fast-build +git diff --check +git status --short --branch +``` + +Expected: build exits 0; diff check is clean; only intended work remains. + +- [ ] **Step 3: Regenerate and inspect the local SSH configuration** + +```bash +./brev refresh +rg -n -A2 'Match host automatic-lavender-swordfish' /Users/pratpatel/.brev/ssh_config +``` + +Expected entry contains `shell automatic-lavender-swordfish`, all four hidden fields including `--linux-user ubuntu`, no `--cert-only`, and the certificate `IdentityFile`. + +- [ ] **Step 4: Prove manual SSH certificate authentication** + +```bash +ssh -vvv -o ControlMaster=no -o ControlPath=none -o BatchMode=yes automatic-lavender-swordfish 'printf cert-authenticated' +ssh-keygen -Lf /Users/pratpatel/.brev/ssh-certs/emyxcusgq-cert.pub +``` + +Expected: the hook exits 0; OpenSSH reports `Server accepts key` for the ED25519 certificate; output is `cert-authenticated`; the certificate principal is `brev:v1:vm:emyxcusgq:login:ubuntu`. + +- [ ] **Step 5: Prove `brev shell` uses the same alias path** + +Run `./brev shell automatic-lavender-swordfish` in a PTY, wait for the remote prompt, then send `exit`. + +Expected: the command resolves `automatic-lavender-swordfish`, opens the remote shell, and exits 0. The generated host entry and the preceding manual probe establish that this alias evaluates the certificate hook. + +- [ ] **Step 6: Final repository audit** + +```bash +git status --short --branch +git log -5 --oneline --decorate +git diff origin/feat/ssh-certs...HEAD --stat +``` + +Expected: no `dev-plane` changes, no push, and only the design, plan, generator, routing, and test commits in the CLI feature worktree. From a368eedbb3d38dd0e66f18269e34f72d7555b359 Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Fri, 21 Aug 2026 12:01:55 -0700 Subject: [PATCH 13/27] fix(ssh): include workspace in certificate hook --- pkg/ssh/sshconfigurer.go | 8 ++++---- pkg/ssh/sshconfigurer_test.go | 21 +++++++++++++-------- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/pkg/ssh/sshconfigurer.go b/pkg/ssh/sshconfigurer.go index 83661a10..ed523371 100644 --- a/pkg/ssh/sshconfigurer.go +++ b/pkg/ssh/sshconfigurer.go @@ -481,8 +481,8 @@ func makeCloudflareSSHProxyCommand(cloudflaredBinaryPath string, hostname string // or "" if not eligible or home is empty (WSL, deferred). // // The exec command uses the absolute path to the running brev binary so it -// resolves to the same build that generated this config (which has --cert-only), -// not whatever stale `brev` is first in PATH at ssh time. +// resolves to the build that generated this config. The workspace alias keeps +// the normal `brev shell ` argument contract intact. func makeCertMatchEntry(workspace entity.Workspace, home string) string { if home == "" || !workspace.SSHCertEligible || workspace.PortID == "" { return "" @@ -493,8 +493,8 @@ func makeCertMatchEntry(workspace entity.Workspace, home string) string { } alias := string(workspace.GetLocalIdentifier()) certKeyPath := sshcert.KeyPath(home, workspace.ID) - exec := fmt.Sprintf("'%s' shell --cert-only --env %s --port %s --user %s --out-key '%s'", - brevBin, workspace.ID, workspace.PortID, workspace.GetSSHUser(), certKeyPath) + exec := fmt.Sprintf("'%s' shell %s --env %s --port %s --linux-user %s --out-key '%s'", + brevBin, alias, workspace.ID, workspace.PortID, workspace.GetSSHUser(), certKeyPath) return fmt.Sprintf("Match host %s exec %q\n IdentityFile %q\n", alias, exec, certKeyPath) } diff --git a/pkg/ssh/sshconfigurer_test.go b/pkg/ssh/sshconfigurer_test.go index a348ab8b..325180a6 100644 --- a/pkg/ssh/sshconfigurer_test.go +++ b/pkg/ssh/sshconfigurer_test.go @@ -940,18 +940,24 @@ func TestMakeCertMatchEntry_EligibleWorkspace(t *testing.T) { PortID: "port-1", } got := makeCertMatchEntry(w, "/home/u") - // Must be a Match block with the workspace alias and the brev --cert-only exec. + // Must be a Match block with the workspace alias and the brev certificate exec. if !strings.HasPrefix(got, "Match host my-env exec \"") { t.Errorf("expected Match host my-env exec block, got: %s", got) } - if !strings.Contains(got, "--env env-abc") { - t.Errorf("missing --env env-abc: %s", got) + if !strings.Contains(got, " shell my-env --env env-abc") { + t.Errorf("missing positional workspace alias: %s", got) } if !strings.Contains(got, "--port port-1") { t.Errorf("missing --port port-1: %s", got) } - if !strings.Contains(got, "--user ubuntu") { - t.Errorf("missing --user ubuntu: %s", got) + if !strings.Contains(got, "--linux-user ubuntu") { + t.Errorf("missing --linux-user ubuntu: %s", got) + } + if strings.Contains(got, "--cert-only") { + t.Errorf("Match exec must infer certificate mode from its hidden fields: %s", got) + } + if strings.Contains(got, " --user ") { + t.Errorf("Match exec must not shadow Brev's persistent --user flag: %s", got) } if !strings.Contains(got, "--out-key '/home/u/.brev/ssh-certs/env-abc'") { t.Errorf("missing out-key path: %s", got) @@ -1040,8 +1046,7 @@ func TestMakeSSHConfigEntryV2_IneligibleWorkspaceNoCertMatch(t *testing.T) { 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 lacking - // --cert-only (the bug where the cert path silently fell back to static). + // 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", @@ -1051,7 +1056,7 @@ func TestMakeCertMatchEntry_UsesAbsoluteBrevPath(t *testing.T) { if err != nil { t.Skip("os.Executable unavailable; cannot assert path") } - want := fmt.Sprintf("'%s' shell --cert-only", exe) + want := fmt.Sprintf("'%s' shell n --env env-abc", exe) if !strings.Contains(got, want) { t.Errorf("expected Match exec to use absolute brev path %q; got: %s", want, got) } From c4184eeec0fcc36db7fff0104d02bc38e52f17e3 Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Fri, 21 Aug 2026 12:03:13 -0700 Subject: [PATCH 14/27] fix(shell): infer SSH certificate requests from fields --- pkg/cmd/shell/certonly.go | 37 +++++++++++++------------- pkg/cmd/shell/certonly_test.go | 48 +++++++++++++++++++++++++++++----- pkg/cmd/shell/shell.go | 2 +- 3 files changed, 62 insertions(+), 25 deletions(-) diff --git a/pkg/cmd/shell/certonly.go b/pkg/cmd/shell/certonly.go index cf1bcfbc..8278e786 100644 --- a/pkg/cmd/shell/certonly.go +++ b/pkg/cmd/shell/certonly.go @@ -117,32 +117,33 @@ func runCertOnlyWith(store certOnlyStore, fs afero.Fs, issuer CertIssuer, req ce } type certOnlyFlags struct { - certOnly bool - env string - port string - user string - outKey string + env string + port string + user string + outKey string } -// addCertOnlyFlags registers the hidden --cert-only flags. They are an +func (f certOnlyFlags) requested() bool { + return f.env != "" || f.port != "" || f.user != "" || f.outKey != "" +} + +// addCertOnlyFlags registers hidden certificate request fields. They are an // implementation detail of the ssh config's Match exec hook, not user-facing. func addCertOnlyFlags(cmd *cobra.Command, f *certOnlyFlags) { - cmd.Flags().BoolVar(&f.certOnly, "cert-only", false, "mint an SSH certificate and write it to disk, then exit (used by the ssh config Match exec hook)") - cmd.Flags().StringVar(&f.env, "env", "", "(--cert-only) environment ID to mint a certificate for") - cmd.Flags().StringVar(&f.port, "port", "", "(--cert-only) network-member port ID for the SSH access") - cmd.Flags().StringVar(&f.user, "user", "", "(--cert-only) linux user for the certificate principal") - cmd.Flags().StringVar(&f.outKey, "out-key", "", "(--cert-only) absolute path to write the private key (certificate goes to -cert.pub)") + cmd.Flags().StringVar(&f.env, "env", "", "environment ID for internal SSH certificate issuance") + cmd.Flags().StringVar(&f.port, "port", "", "network-member port ID for internal SSH certificate issuance") + cmd.Flags().StringVar(&f.user, "linux-user", "", "Linux user for internal SSH certificate issuance") + cmd.Flags().StringVar(&f.outKey, "out-key", "", "private-key path for internal SSH certificate issuance") - for _, name := range []string{"cert-only", "env", "port", "user", "out-key"} { + for _, name := range []string{"env", "port", "linux-user", "out-key"} { _ = cmd.Flags().MarkHidden(name) } } -// validateCertOnly errors if --cert-only is set without all four required -// params. The flags are hidden and only set by the generated ssh config, so the -// inverse (params without --cert-only) isn't a real scenario. +// validateCertOnly rejects partial certificate requests before they can fall +// through to the normal interactive shell path. func validateCertOnly(f certOnlyFlags) error { - if !f.certOnly { + if !f.requested() { return nil } var missing []string @@ -153,13 +154,13 @@ func validateCertOnly(f certOnlyFlags) error { missing = append(missing, "--port") } if f.user == "" { - missing = append(missing, "--user") + missing = append(missing, "--linux-user") } if f.outKey == "" { missing = append(missing, "--out-key") } if len(missing) > 0 { - return fmt.Errorf("--cert-only requires %s", strings.Join(missing, ", ")) + return fmt.Errorf("SSH certificate request requires %s", strings.Join(missing, ", ")) } return nil } diff --git a/pkg/cmd/shell/certonly_test.go b/pkg/cmd/shell/certonly_test.go index 15373895..fc2f505d 100644 --- a/pkg/cmd/shell/certonly_test.go +++ b/pkg/cmd/shell/certonly_test.go @@ -12,6 +12,7 @@ import ( devplanev1 "buf.build/gen/go/brevdev/devplane/protocolbuffers/go/devplaneapi/v1" "connectrpc.com/connect" "github.com/spf13/afero" + "github.com/spf13/cobra" "golang.org/x/crypto/ssh" "github.com/brevdev/brev-cli/pkg/sshcert" @@ -185,13 +186,48 @@ func TestRpcCertIssuer_PropagatesError(t *testing.T) { } func TestValidateCertOnly(t *testing.T) { - if err := validateCertOnly(certOnlyFlags{}); err != nil { - t.Errorf("empty flags should be valid: %v", err) + tests := []struct { + name string + flags certOnlyFlags + wantErr bool + }{ + {name: "normal shell"}, + {name: "complete certificate request", flags: certOnlyFlags{env: "e", port: "p", user: "u", outKey: "/k"}}, + {name: "environment only", flags: certOnlyFlags{env: "e"}, wantErr: true}, + {name: "port only", flags: certOnlyFlags{port: "p"}, wantErr: true}, + {name: "linux user only", flags: certOnlyFlags{user: "u"}, wantErr: true}, + {name: "output key only", flags: certOnlyFlags{outKey: "/k"}, wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateCertOnly(tt.flags) + if (err != nil) != tt.wantErr { + t.Fatalf("validateCertOnly() error = %v, wantErr %v", err, tt.wantErr) + } + }) } - if err := validateCertOnly(certOnlyFlags{certOnly: true}); err == nil { - t.Error("cert-only without params should error") +} + +func TestAddCertOnlyFlagsUsesImplicitCertificateMode(t *testing.T) { + cmd := &cobra.Command{} + flags := certOnlyFlags{} + addCertOnlyFlags(cmd, &flags) + + if flag := cmd.Flags().Lookup("cert-only"); flag != nil { + t.Error("--cert-only should not be registered") } - if err := validateCertOnly(certOnlyFlags{certOnly: true, env: "e", port: "p", user: "u", outKey: "/k"}); err != nil { - t.Errorf("complete cert-only should be valid: %v", err) + if flag := cmd.Flags().Lookup("user"); flag != nil { + t.Error("certificate flags must not shadow persistent --user") + } + for _, name := range []string{"env", "port", "linux-user", "out-key"} { + flag := cmd.Flags().Lookup(name) + if flag == nil { + t.Errorf("--%s is not registered", name) + continue + } + if !flag.Hidden { + t.Errorf("--%s must remain hidden", name) + } } } diff --git a/pkg/cmd/shell/shell.go b/pkg/cmd/shell/shell.go index 4f775fc6..d81f1f1c 100644 --- a/pkg/cmd/shell/shell.go +++ b/pkg/cmd/shell/shell.go @@ -68,7 +68,7 @@ func NewCmdShell(t *terminal.Terminal, store ShellStore, noLoginStartStore Shell if err := validateCertOnly(certFlags); err != nil { return breverrors.WrapAndTrace(err) } - if certFlags.certOnly { + if certFlags.requested() { return runCertOnly(store, certOnlyRequest{ EnvironmentID: certFlags.env, PortID: certFlags.port, From 6320bc8527361345114cb30ef260b9469455d56b Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Fri, 21 Aug 2026 12:07:36 -0700 Subject: [PATCH 15/27] fix(ssh): escape certificate hook arguments --- ...sh-certificate-match-exec-compatibility.md | 38 +++++++++++++++++-- ...ificate-match-exec-compatibility-design.md | 3 +- pkg/ssh/sshconfigurer.go | 16 +++++++- pkg/ssh/sshconfigurer_test.go | 17 ++++++++- 4 files changed, 65 insertions(+), 9 deletions(-) diff --git a/docs/superpowers/plans/2026-08-21-ssh-certificate-match-exec-compatibility.md b/docs/superpowers/plans/2026-08-21-ssh-certificate-match-exec-compatibility.md index 81e68e01..371cabaa 100644 --- a/docs/superpowers/plans/2026-08-21-ssh-certificate-match-exec-compatibility.md +++ b/docs/superpowers/plans/2026-08-21-ssh-certificate-match-exec-compatibility.md @@ -65,12 +65,29 @@ if strings.Contains(got, " --user ") { Update the absolute-binary assertion in `TestMakeCertMatchEntry_UsesAbsoluteBrevPath`: ```go -want := fmt.Sprintf("'%s' shell n --env env-abc", exe) +want := fmt.Sprintf("%s shell n --env env-abc", exe) if !strings.Contains(got, want) { t.Errorf("expected Match exec to use absolute brev path %q; got: %s", want, got) } ``` +Add a command-argument escaping case: + +```go +func TestMakeCertMatchEntry_ShellEscapesCommandArguments(t *testing.T) { + w := entity.Workspace{ + ID: "env;id", Name: "my-env;whoami", SSHUser: "user;id", + SSHCertEligible: true, PortID: "port;id", + } + + got := makeCertMatchEntry(w, "/home/user name") + want := "shell 'my-env;whoami' --env 'env;id' --port 'port;id' --linux-user 'user;id' --out-key '/home/user name/.brev/ssh-certs/env-id'" + if !strings.Contains(got, want) { + t.Errorf("expected shell-escaped Match exec %q; got: %s", want, got) + } +} +``` + - [ ] **Step 2: Run the focused test to verify RED** Run: @@ -79,7 +96,7 @@ Run: go test ./pkg/ssh -run 'TestMakeCertMatchEntry_(EligibleWorkspace|UsesAbsoluteBrevPath)$' -count=1 ``` -Expected: FAIL because the current generator omits `my-env`, emits `--cert-only`, and emits `--user` instead of `--linux-user`. +Expected: FAIL because the current generator omits `my-env`, emits `--cert-only`, emits `--user` instead of `--linux-user`, and interpolates command arguments without shell escaping. - [ ] **Step 3: Implement the minimal generator change** @@ -99,12 +116,25 @@ func makeCertMatchEntry(workspace entity.Workspace, home string) string { } alias := string(workspace.GetLocalIdentifier()) certKeyPath := sshcert.KeyPath(home, workspace.ID) - exec := fmt.Sprintf("'%s' shell %s --env %s --port %s --linux-user %s --out-key '%s'", - brevBin, alias, workspace.ID, workspace.PortID, workspace.GetSSHUser(), certKeyPath) + exec := shellescape.QuoteCommand([]string{ + brevBin, + "shell", + alias, + "--env", + workspace.ID, + "--port", + workspace.PortID, + "--linux-user", + workspace.GetSSHUser(), + "--out-key", + certKeyPath, + }) return fmt.Sprintf("Match host %s exec %q\n IdentityFile %q\n", alias, exec, certKeyPath) } ``` +Import the existing dependency as `github.com/alessio/shellescape`. + - [ ] **Step 4: Verify GREEN** Run: diff --git a/docs/superpowers/specs/2026-08-21-ssh-certificate-match-exec-compatibility-design.md b/docs/superpowers/specs/2026-08-21-ssh-certificate-match-exec-compatibility-design.md index 2c700f04..0c2f4fbb 100644 --- a/docs/superpowers/specs/2026-08-21-ssh-certificate-match-exec-compatibility-design.md +++ b/docs/superpowers/specs/2026-08-21-ssh-certificate-match-exec-compatibility-design.md @@ -18,7 +18,7 @@ Make both direct `ssh ` and `brev shell ` mint and use a V The generated conditional entry will include the workspace alias as the normal `brev shell` positional argument: ```text -Match host automatic-lavender-swordfish exec "'' shell automatic-lavender-swordfish --env emyxcusgq --port nport-... --linux-user ubuntu --out-key '/Users/example/.brev/ssh-certs/emyxcusgq'" +Match host automatic-lavender-swordfish exec "/absolute/brev shell automatic-lavender-swordfish --env emyxcusgq --port nport-123 --linux-user ubuntu --out-key /Users/example/.brev/ssh-certs/emyxcusgq" IdentityFile "/Users/example/.brev/ssh-certs/emyxcusgq" ``` @@ -49,6 +49,7 @@ The certificate Linux-account flag is named `--linux-user`, not `--user`, becaus - A missing or partial certificate-flag bundle exits nonzero and does not enter the normal interactive shell path. - Authentication, certificate issuance, parsing, and file-write failures remain nonzero so OpenSSH excludes the conditional certificate identity and can use the existing static-key fallback. - The absolute CLI path remains in the hook so configuration generated by a development or non-PATH binary invokes that same binary. +- Every hook argument is shell-escaped before OpenSSH executes the command. - `dev-plane` RPCs, protobufs, authorization policies, principal construction, and VM bootstrap remain unchanged. - Existing unrelated worktrees and changes remain untouched. No branch is pushed. diff --git a/pkg/ssh/sshconfigurer.go b/pkg/ssh/sshconfigurer.go index ed523371..8d1c8757 100644 --- a/pkg/ssh/sshconfigurer.go +++ b/pkg/ssh/sshconfigurer.go @@ -10,6 +10,7 @@ import ( "strings" "text/template" + "github.com/alessio/shellescape" "github.com/brevdev/brev-cli/pkg/autostartconf" "github.com/brevdev/brev-cli/pkg/entity" breverrors "github.com/brevdev/brev-cli/pkg/errors" @@ -493,8 +494,19 @@ func makeCertMatchEntry(workspace entity.Workspace, home string) string { } alias := string(workspace.GetLocalIdentifier()) certKeyPath := sshcert.KeyPath(home, workspace.ID) - exec := fmt.Sprintf("'%s' shell %s --env %s --port %s --linux-user %s --out-key '%s'", - brevBin, alias, workspace.ID, workspace.PortID, workspace.GetSSHUser(), certKeyPath) + exec := shellescape.QuoteCommand([]string{ + brevBin, + "shell", + alias, + "--env", + workspace.ID, + "--port", + workspace.PortID, + "--linux-user", + workspace.GetSSHUser(), + "--out-key", + certKeyPath, + }) return fmt.Sprintf("Match host %s exec %q\n IdentityFile %q\n", alias, exec, certKeyPath) } diff --git a/pkg/ssh/sshconfigurer_test.go b/pkg/ssh/sshconfigurer_test.go index 325180a6..9696a738 100644 --- a/pkg/ssh/sshconfigurer_test.go +++ b/pkg/ssh/sshconfigurer_test.go @@ -959,7 +959,7 @@ func TestMakeCertMatchEntry_EligibleWorkspace(t *testing.T) { if strings.Contains(got, " --user ") { t.Errorf("Match exec must not shadow Brev's persistent --user flag: %s", got) } - if !strings.Contains(got, "--out-key '/home/u/.brev/ssh-certs/env-abc'") { + if !strings.Contains(got, "--out-key /home/u/.brev/ssh-certs/env-abc") { t.Errorf("missing out-key path: %s", got) } // IdentityFile must point at the cert key path (quoted). @@ -1056,7 +1056,7 @@ func TestMakeCertMatchEntry_UsesAbsoluteBrevPath(t *testing.T) { if err != nil { t.Skip("os.Executable unavailable; cannot assert path") } - want := fmt.Sprintf("'%s' shell n --env env-abc", exe) + want := fmt.Sprintf("%s shell n --env env-abc", exe) if !strings.Contains(got, want) { t.Errorf("expected Match exec to use absolute brev path %q; got: %s", want, got) } @@ -1064,3 +1064,16 @@ func TestMakeCertMatchEntry_UsesAbsoluteBrevPath(t *testing.T) { t.Errorf("Match exec must not use bare `brev`: %s", got) } } + +func TestMakeCertMatchEntry_ShellEscapesCommandArguments(t *testing.T) { + w := entity.Workspace{ + ID: "env;id", Name: "my-env;whoami", SSHUser: "user;id", + SSHCertEligible: true, PortID: "port;id", + } + + got := makeCertMatchEntry(w, "/home/user name") + want := "shell 'my-env;whoami' --env 'env;id' --port 'port;id' --linux-user 'user;id' --out-key '/home/user name/.brev/ssh-certs/env-id'" + if !strings.Contains(got, want) { + t.Errorf("expected shell-escaped Match exec %q; got: %s", want, got) + } +} From 1eefd43197c21a6bbc82eacba3938245ada5a232 Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Fri, 21 Aug 2026 12:14:14 -0700 Subject: [PATCH 16/27] remove docs --- ...sh-certificate-match-exec-compatibility.md | 390 ------------------ ...ificate-match-exec-compatibility-design.md | 66 --- 2 files changed, 456 deletions(-) delete mode 100644 docs/superpowers/plans/2026-08-21-ssh-certificate-match-exec-compatibility.md delete mode 100644 docs/superpowers/specs/2026-08-21-ssh-certificate-match-exec-compatibility-design.md diff --git a/docs/superpowers/plans/2026-08-21-ssh-certificate-match-exec-compatibility.md b/docs/superpowers/plans/2026-08-21-ssh-certificate-match-exec-compatibility.md deleted file mode 100644 index 371cabaa..00000000 --- a/docs/superpowers/plans/2026-08-21-ssh-certificate-match-exec-compatibility.md +++ /dev/null @@ -1,390 +0,0 @@ -# SSH Certificate Match Exec Compatibility Implementation Plan - -> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. - -**Goal:** Make direct `ssh ` and `brev shell ` use the same VM-scoped certificate path while retaining `brev shell`'s exact-one-argument contract. - -**Architecture:** Keep `dev-plane`'s verified certificate issuance and VM authorization unchanged. Generate a host-inclusive `Match exec` command in `brev-cli`, infer certificate mode from a complete bundle of hidden certificate flags, and retain the static key as OpenSSH's fallback when the hook exits nonzero. - -**Tech Stack:** Go, Cobra/pflag, OpenSSH client configuration, ConnectRPC, `golang.org/x/crypto/ssh` - -**Spec:** `docs/superpowers/specs/2026-08-21-ssh-certificate-match-exec-compatibility-design.md` - -## Global Constraints - -- `brev shell` always requires exactly one positional workspace argument. -- Certificate mode has no `--cert-only` marker; any certificate-specific flag selects it and all four fields are required. -- The certificate Linux account uses hidden `--linux-user`, never the root command's persistent `--user` flag. -- The generated hook uses the absolute running Brev binary path and retains `brev.pem` as a fallback identity. -- `dev-plane` RPCs, protobufs, authorization, principal construction, and bootstrap remain unchanged. -- Preserve unrelated worktrees and unstaged edits; do not push. - -## File Structure - -- `pkg/ssh/sshconfigurer.go`: generate the host-inclusive certificate hook. -- `pkg/ssh/sshconfigurer_test.go`: protect the generated OpenSSH contract. -- `pkg/cmd/shell/certonly.go`: register and classify the hidden certificate fields. -- `pkg/cmd/shell/certonly_test.go`: protect complete, absent, and partial flag behavior. -- `pkg/cmd/shell/shell.go`: retain `ExactArgs(1)` and route complete certificate requests before interactive shell execution. -- No `dev-plane` source file changes. - ---- - -### Task 1: Generate A Host-Inclusive Certificate Hook - -**Files:** -- Modify: `pkg/ssh/sshconfigurer_test.go:934-1061` -- Modify: `pkg/ssh/sshconfigurer.go:480-499` - -**Interfaces:** -- Consumes: `entity.Workspace.GetLocalIdentifier()`, `entity.Workspace.GetSSHUser()`, `sshcert.KeyPath(home, environmentID)`, and `os.Executable()`. -- Produces: `makeCertMatchEntry(workspace entity.Workspace, home string) string` containing `shell my-env --env env-abc --port port-1 --linux-user ubuntu --out-key /home/u/.brev/ssh-certs/env-abc` for the test fixture. - -- [ ] **Step 1: Write the failing generator assertions** - -Update `TestMakeCertMatchEntry_EligibleWorkspace` so the expected command contract is literal and independently derived: - -```go -if !strings.Contains(got, " shell my-env --env env-abc") { - t.Errorf("missing positional workspace alias: %s", got) -} -if !strings.Contains(got, "--port port-1") { - t.Errorf("missing --port port-1: %s", got) -} -if !strings.Contains(got, "--linux-user ubuntu") { - t.Errorf("missing --linux-user ubuntu: %s", got) -} -if strings.Contains(got, "--cert-only") { - t.Errorf("Match exec must infer certificate mode from its hidden fields: %s", got) -} -if strings.Contains(got, " --user ") { - t.Errorf("Match exec must not shadow Brev's persistent --user flag: %s", got) -} -``` - -Update the absolute-binary assertion in `TestMakeCertMatchEntry_UsesAbsoluteBrevPath`: - -```go -want := fmt.Sprintf("%s shell n --env env-abc", exe) -if !strings.Contains(got, want) { - t.Errorf("expected Match exec to use absolute brev path %q; got: %s", want, got) -} -``` - -Add a command-argument escaping case: - -```go -func TestMakeCertMatchEntry_ShellEscapesCommandArguments(t *testing.T) { - w := entity.Workspace{ - ID: "env;id", Name: "my-env;whoami", SSHUser: "user;id", - SSHCertEligible: true, PortID: "port;id", - } - - got := makeCertMatchEntry(w, "/home/user name") - want := "shell 'my-env;whoami' --env 'env;id' --port 'port;id' --linux-user 'user;id' --out-key '/home/user name/.brev/ssh-certs/env-id'" - if !strings.Contains(got, want) { - t.Errorf("expected shell-escaped Match exec %q; got: %s", want, got) - } -} -``` - -- [ ] **Step 2: Run the focused test to verify RED** - -Run: - -```bash -go test ./pkg/ssh -run 'TestMakeCertMatchEntry_(EligibleWorkspace|UsesAbsoluteBrevPath)$' -count=1 -``` - -Expected: FAIL because the current generator omits `my-env`, emits `--cert-only`, emits `--user` instead of `--linux-user`, and interpolates command arguments without shell escaping. - -- [ ] **Step 3: Implement the minimal generator change** - -Replace the hook command and update its compatibility comment: - -```go -// The exec command uses the absolute path to the running brev binary so it -// resolves to the build that generated this config. The workspace alias keeps -// the normal `brev shell ` argument contract intact. -func makeCertMatchEntry(workspace entity.Workspace, home string) string { - if home == "" || !workspace.SSHCertEligible || workspace.PortID == "" { - return "" - } - brevBin, err := os.Executable() - if err != nil { - brevBin = "brev" - } - alias := string(workspace.GetLocalIdentifier()) - certKeyPath := sshcert.KeyPath(home, workspace.ID) - exec := shellescape.QuoteCommand([]string{ - brevBin, - "shell", - alias, - "--env", - workspace.ID, - "--port", - workspace.PortID, - "--linux-user", - workspace.GetSSHUser(), - "--out-key", - certKeyPath, - }) - return fmt.Sprintf("Match host %s exec %q\n IdentityFile %q\n", alias, exec, certKeyPath) -} -``` - -Import the existing dependency as `github.com/alessio/shellescape`. - -- [ ] **Step 4: Verify GREEN** - -Run: - -```bash -gofmt -w pkg/ssh/sshconfigurer.go pkg/ssh/sshconfigurer_test.go -go test ./pkg/ssh -run 'TestMakeCertMatchEntry|TestMakeSSHConfigEntryV2_(EligibleWorkspaceIncludesCertMatch|IneligibleWorkspaceNoCertMatch)' -count=1 -``` - -Expected: PASS. - -- [ ] **Step 5: Commit the generator contract** - -```bash -git add pkg/ssh/sshconfigurer.go pkg/ssh/sshconfigurer_test.go -git diff --cached --check -git commit -m "fix(ssh): include workspace in certificate hook" -``` - ---- - -### Task 2: Infer Certificate Mode From Hidden Fields - -**Files:** -- Modify: `pkg/cmd/shell/certonly_test.go:187-217` -- Modify: `pkg/cmd/shell/certonly.go:119-165` -- Modify: `pkg/cmd/shell/shell.go:54-95` - -**Interfaces:** -- Consumes: Cobra's hidden string flags and the existing `runCertOnly(store ShellStore, req certOnlyRequest) error` path. -- Produces: `certOnlyFlags.requested() bool`; hidden `--env`, `--port`, `--linux-user`, and `--out-key`; exact-one-argument shell routing. - -- [ ] **Step 1: Replace the broken argument-bypass test with failing flag-contract tests** - -Add `github.com/spf13/cobra` to `certonly_test.go` imports. Replace `TestValidateCertOnly` and remove `TestShellCmdArgs_CertOnlyBypassesArgRequirement` with: - -```go -func TestValidateCertOnly(t *testing.T) { - tests := []struct { - name string - flags certOnlyFlags - wantErr bool - }{ - {name: "normal shell"}, - {name: "complete certificate request", flags: certOnlyFlags{env: "e", port: "p", user: "u", outKey: "/k"}}, - {name: "environment only", flags: certOnlyFlags{env: "e"}, wantErr: true}, - {name: "port only", flags: certOnlyFlags{port: "p"}, wantErr: true}, - {name: "linux user only", flags: certOnlyFlags{user: "u"}, wantErr: true}, - {name: "output key only", flags: certOnlyFlags{outKey: "/k"}, wantErr: true}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := validateCertOnly(tt.flags) - if (err != nil) != tt.wantErr { - t.Fatalf("validateCertOnly() error = %v, wantErr %v", err, tt.wantErr) - } - }) - } -} - -func TestAddCertOnlyFlagsUsesImplicitCertificateMode(t *testing.T) { - cmd := &cobra.Command{} - flags := certOnlyFlags{} - addCertOnlyFlags(cmd, &flags) - - if flag := cmd.Flags().Lookup("cert-only"); flag != nil { - t.Error("--cert-only should not be registered") - } - if flag := cmd.Flags().Lookup("user"); flag != nil { - t.Error("certificate flags must not shadow persistent --user") - } - for _, name := range []string{"env", "port", "linux-user", "out-key"} { - flag := cmd.Flags().Lookup(name) - if flag == nil { - t.Errorf("--%s is not registered", name) - continue - } - if !flag.Hidden { - t.Errorf("--%s must remain hidden", name) - } - } -} -``` - -- [ ] **Step 2: Run the focused tests to verify RED** - -Run: - -```bash -go test ./pkg/cmd/shell -run 'TestValidateCertOnly|TestAddCertOnlyFlagsUsesImplicitCertificateMode' -count=1 -``` - -Expected: FAIL because partial certificate fields currently pass validation, `--cert-only` still exists, `--linux-user` is absent, and local `--user` is registered. - -- [ ] **Step 3: Implement certificate-field classification** - -Replace the flag structure, registration, and validation with: - -```go -type certOnlyFlags struct { - env string - port string - user string - outKey string -} - -func (f certOnlyFlags) requested() bool { - return f.env != "" || f.port != "" || f.user != "" || f.outKey != "" -} - -func addCertOnlyFlags(cmd *cobra.Command, f *certOnlyFlags) { - cmd.Flags().StringVar(&f.env, "env", "", "environment ID for internal SSH certificate issuance") - cmd.Flags().StringVar(&f.port, "port", "", "network-member port ID for internal SSH certificate issuance") - cmd.Flags().StringVar(&f.user, "linux-user", "", "Linux user for internal SSH certificate issuance") - cmd.Flags().StringVar(&f.outKey, "out-key", "", "private-key path for internal SSH certificate issuance") - - for _, name := range []string{"env", "port", "linux-user", "out-key"} { - _ = cmd.Flags().MarkHidden(name) - } -} - -func validateCertOnly(f certOnlyFlags) error { - if !f.requested() { - return nil - } - var missing []string - if f.env == "" { - missing = append(missing, "--env") - } - if f.port == "" { - missing = append(missing, "--port") - } - if f.user == "" { - missing = append(missing, "--linux-user") - } - if f.outKey == "" { - missing = append(missing, "--out-key") - } - if len(missing) > 0 { - return fmt.Errorf("SSH certificate request requires %s", strings.Join(missing, ", ")) - } - return nil -} -``` - -Restore `NewCmdShell`'s argument contract and route inferred requests: - -```go -Args: cobra.ExactArgs(1), -``` - -```go -if err := validateCertOnly(certFlags); err != nil { - return breverrors.WrapAndTrace(err) -} -if certFlags.requested() { - return runCertOnly(store, certOnlyRequest{ - EnvironmentID: certFlags.env, - PortID: certFlags.port, - LinuxUser: certFlags.user, - OutKey: certFlags.outKey, - }) -} -``` - -- [ ] **Step 4: Verify GREEN** - -Run: - -```bash -gofmt -w pkg/cmd/shell/certonly.go pkg/cmd/shell/certonly_test.go pkg/cmd/shell/shell.go -go test ./pkg/cmd/shell -count=1 -``` - -Expected: PASS. - -- [ ] **Step 5: Commit the implicit certificate mode** - -```bash -git add pkg/cmd/shell/certonly.go pkg/cmd/shell/certonly_test.go pkg/cmd/shell/shell.go -git diff --cached --check -git commit -m "fix(shell): infer SSH certificate requests from fields" -``` - ---- - -### Task 3: Verify The CLI End To End - -**Files:** -- Verify only: all touched CLI files -- Generated local artifact: ignored `brev` binary -- External local state: `/Users/pratpatel/.brev/ssh_config` and `/Users/pratpatel/.brev/ssh-certs/emyxcusgq*` - -**Interfaces:** -- Consumes: the built CLI, authenticated Brev API access, generated OpenSSH config, and `automatic-lavender-swordfish`. -- Produces: fresh evidence that both manual SSH and `brev shell` reach the certificate-enabled target. - -- [ ] **Step 1: Run focused CLI regression suites** - -```bash -go test ./pkg/cmd/shell ./pkg/cmd/refresh ./pkg/sshcert -count=1 -go test ./pkg/ssh -run 'TestMakeCertMatchEntry|TestMakeSSHConfigEntryV2_(EligibleWorkspaceIncludesCertMatch|IneligibleWorkspaceNoCertMatch)' -count=1 -``` - -Expected: PASS. Also run `go test ./pkg/ssh -count=1`; if the known JetBrains Gateway path-dependent tests fail, record them separately and require every SSH-certificate-focused test above to remain green. - -- [ ] **Step 2: Format, build, and check the worktree** - -From `/Users/pratpatel/code/brev-cli-ssh-certs`, run: - -```bash -gofmt -w pkg/ssh/sshconfigurer.go pkg/ssh/sshconfigurer_test.go pkg/cmd/shell/certonly.go pkg/cmd/shell/certonly_test.go pkg/cmd/shell/shell.go -make fast-build -git diff --check -git status --short --branch -``` - -Expected: build exits 0; diff check is clean; only intended work remains. - -- [ ] **Step 3: Regenerate and inspect the local SSH configuration** - -```bash -./brev refresh -rg -n -A2 'Match host automatic-lavender-swordfish' /Users/pratpatel/.brev/ssh_config -``` - -Expected entry contains `shell automatic-lavender-swordfish`, all four hidden fields including `--linux-user ubuntu`, no `--cert-only`, and the certificate `IdentityFile`. - -- [ ] **Step 4: Prove manual SSH certificate authentication** - -```bash -ssh -vvv -o ControlMaster=no -o ControlPath=none -o BatchMode=yes automatic-lavender-swordfish 'printf cert-authenticated' -ssh-keygen -Lf /Users/pratpatel/.brev/ssh-certs/emyxcusgq-cert.pub -``` - -Expected: the hook exits 0; OpenSSH reports `Server accepts key` for the ED25519 certificate; output is `cert-authenticated`; the certificate principal is `brev:v1:vm:emyxcusgq:login:ubuntu`. - -- [ ] **Step 5: Prove `brev shell` uses the same alias path** - -Run `./brev shell automatic-lavender-swordfish` in a PTY, wait for the remote prompt, then send `exit`. - -Expected: the command resolves `automatic-lavender-swordfish`, opens the remote shell, and exits 0. The generated host entry and the preceding manual probe establish that this alias evaluates the certificate hook. - -- [ ] **Step 6: Final repository audit** - -```bash -git status --short --branch -git log -5 --oneline --decorate -git diff origin/feat/ssh-certs...HEAD --stat -``` - -Expected: no `dev-plane` changes, no push, and only the design, plan, generator, routing, and test commits in the CLI feature worktree. diff --git a/docs/superpowers/specs/2026-08-21-ssh-certificate-match-exec-compatibility-design.md b/docs/superpowers/specs/2026-08-21-ssh-certificate-match-exec-compatibility-design.md deleted file mode 100644 index 0c2f4fbb..00000000 --- a/docs/superpowers/specs/2026-08-21-ssh-certificate-match-exec-compatibility-design.md +++ /dev/null @@ -1,66 +0,0 @@ -# SSH Certificate Match Exec Compatibility Design - -## Goal - -Make both direct `ssh ` and `brev shell ` mint and use a VM-scoped SSH certificate through the generated OpenSSH `Match exec` block, without weakening the normal `brev shell` positional-argument contract. - -## Verified Current State - -- `dev-plane` issues five-minute user certificates whose principal is scoped to the environment ID and Linux account. -- Certificate issuance requires the authenticated caller to have an active SSH-access record matching the requested environment, port, and Linux user. -- VM bootstrap installs a `cert-authority,principals="brev:v1:vm::login:"` entry. -- A live `ssh -vvv` probe against `automatic-lavender-swordfish` ran the generated `Match exec`, obtained a certificate, and authenticated with the ED25519 certificate rather than `brev.pem`. -- `brev shell ` ultimately invokes system `ssh` with the workspace alias, so it consumes the same generated SSH configuration as a manual SSH command. -- The committed CLI generator currently emits `brev shell --cert-only ...` without the positional workspace argument, while the committed command still requires exactly one argument. - -## Command Contract - -The generated conditional entry will include the workspace alias as the normal `brev shell` positional argument: - -```text -Match host automatic-lavender-swordfish exec "/absolute/brev shell automatic-lavender-swordfish --env emyxcusgq --port nport-123 --linux-user ubuntu --out-key /Users/example/.brev/ssh-certs/emyxcusgq" - IdentityFile "/Users/example/.brev/ssh-certs/emyxcusgq" -``` - -The hidden `--cert-only` boolean will be removed. Certificate mode will be inferred from the certificate-specific flags: - -- `--env` -- `--port` -- `--linux-user` -- `--out-key` - -If none of these flags is supplied, `brev shell ` follows the normal interactive shell path. If any is supplied, the command treats the invocation as an internal certificate request and requires all four. Partial input fails before any authentication, API, or file operation. - -The certificate Linux-account flag is named `--linux-user`, not `--user`, because the root Brev command already owns a persistent `--user` flag. This avoids shadowing existing per-user configuration behavior. - -`brev shell` continues to use `cobra.ExactArgs(1)` for every invocation. The workspace argument is required even though certificate issuance uses the explicit environment, port, and Linux-user fields. - -## Data Flow - -1. `brev refresh` resolves the current user's SSH-access record and network port from `dev-plane`. -2. A workspace labeled `sshprovider=certauth` produces a `Match host ... exec ...` block containing its alias, immutable environment ID, SSH-access port ID, Linux user, certificate-key path, and the absolute path of the CLI binary generating the configuration. -3. OpenSSH evaluates the block for either manual `ssh ` or the system SSH command launched by `brev shell `. -4. The hidden certificate flags select the certificate path in `RunE`. -5. The CLI reuses a sufficiently fresh cached certificate or creates an ephemeral Ed25519 keypair, requests a VM-scoped certificate from `IssueEnvironmentSSHCertificate`, and atomically writes the key and matching `-cert.pub` file. -6. A successful hook adds the certificate identity to the effective SSH configuration. The normal host block also retains `brev.pem` as a compatibility fallback. - -## Error And Compatibility Behavior - -- A missing or partial certificate-flag bundle exits nonzero and does not enter the normal interactive shell path. -- Authentication, certificate issuance, parsing, and file-write failures remain nonzero so OpenSSH excludes the conditional certificate identity and can use the existing static-key fallback. -- The absolute CLI path remains in the hook so configuration generated by a development or non-PATH binary invokes that same binary. -- Every hook argument is shell-escaped before OpenSSH executes the command. -- `dev-plane` RPCs, protobufs, authorization policies, principal construction, and VM bootstrap remain unchanged. -- Existing unrelated worktrees and changes remain untouched. No branch is pushed. - -## Testing And Verification - -Implementation will follow test-first development: - -1. Add a generator regression test requiring the positional workspace alias, `--linux-user`, and the absence of `--cert-only`; verify it fails against the current generator. -2. Add flag-classification tests for no certificate flags, a complete certificate bundle, and each partial bundle; verify the new expectations fail before changing production behavior. -3. Implement the minimal generator and shell-routing changes, then run the focused `pkg/cmd/shell`, `pkg/ssh`, `pkg/cmd/refresh`, and `pkg/sshcert` tests. Host-specific unrelated JetBrains test failures will be reported separately if they recur. -4. Run `gofmt` on touched Go files, `git diff --check`, and build the CLI. -5. Regenerate the local SSH configuration with the built binary and inspect the effective entry. -6. Run a non-ControlMaster verbose SSH probe and require evidence that the server accepts the generated certificate. -7. Run `brev shell automatic-lavender-swordfish`, exit the remote shell cleanly, and confirm it follows the same `Match exec` certificate path. From 172cbc429ee0735964c26a59c82bb0f3ed708707 Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Fri, 21 Aug 2026 12:53:43 -0700 Subject: [PATCH 17/27] clean --- pkg/cmd/refresh/sshaccess.go | 3 --- pkg/cmd/shell/certonly.go | 6 ------ 2 files changed, 9 deletions(-) diff --git a/pkg/cmd/refresh/sshaccess.go b/pkg/cmd/refresh/sshaccess.go index 1ee4dbc2..2d2a0737 100644 --- a/pkg/cmd/refresh/sshaccess.go +++ b/pkg/cmd/refresh/sshaccess.go @@ -117,9 +117,6 @@ func resolveWorkspaceSSH( workspace.SSHPort = int(port.GetPortNumber()) workspace.SSHUser = access.GetLinuxUser() workspace.SSHProxyHostname = "" - - // Retain port_id and cert-eligibility for the SSH config generator's Match - // exec block. Empty when SSH access wasn't resolved via the Environment API. workspace.PortID = access.GetPortId() workspace.SSHCertEligible = sshcert.EnvironmentCertEligible(environment.GetLabels()) diff --git a/pkg/cmd/shell/certonly.go b/pkg/cmd/shell/certonly.go index 8278e786..7383c0c1 100644 --- a/pkg/cmd/shell/certonly.go +++ b/pkg/cmd/shell/certonly.go @@ -74,8 +74,6 @@ type certOnlyStore interface { GetAccessToken() (string, error) } -// runCertOnly is invoked by the ssh config's Match exec hook. On any failure it returns non-zero -// so ssh falls back to the static brev.pem. Must not prompt, as that would hang ssh. func runCertOnly(store ShellStore, req certOnlyRequest) error { return runCertOnlyWith(store, afero.NewOsFs(), newCertIssuer(store, config.GlobalConfig.GetBrevPublicAPIURL()), req) } @@ -127,8 +125,6 @@ func (f certOnlyFlags) requested() bool { return f.env != "" || f.port != "" || f.user != "" || f.outKey != "" } -// addCertOnlyFlags registers hidden certificate request fields. They are an -// implementation detail of the ssh config's Match exec hook, not user-facing. func addCertOnlyFlags(cmd *cobra.Command, f *certOnlyFlags) { cmd.Flags().StringVar(&f.env, "env", "", "environment ID for internal SSH certificate issuance") cmd.Flags().StringVar(&f.port, "port", "", "network-member port ID for internal SSH certificate issuance") @@ -140,8 +136,6 @@ func addCertOnlyFlags(cmd *cobra.Command, f *certOnlyFlags) { } } -// validateCertOnly rejects partial certificate requests before they can fall -// through to the normal interactive shell path. func validateCertOnly(f certOnlyFlags) error { if !f.requested() { return nil From a12991d683f5da6863ca51b2b842590acbf3ab13 Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Fri, 21 Aug 2026 13:10:22 -0700 Subject: [PATCH 18/27] refactor: rename certOnly -> mintCert Name the cert-minting mode by what it does (mint a cert) rather than by what it isn't (not a shell). Renames certOnly* -> mintCert* across identifiers and the test file; renames certonly.go -> mintcert.go. --- pkg/cmd/shell/{certonly.go => mintcert.go} | 24 +++++----- .../{certonly_test.go => mintcert_test.go} | 44 +++++++++---------- pkg/cmd/shell/shell.go | 8 ++-- 3 files changed, 38 insertions(+), 38 deletions(-) rename pkg/cmd/shell/{certonly.go => mintcert.go} (87%) rename pkg/cmd/shell/{certonly_test.go => mintcert_test.go} (83%) diff --git a/pkg/cmd/shell/certonly.go b/pkg/cmd/shell/mintcert.go similarity index 87% rename from pkg/cmd/shell/certonly.go rename to pkg/cmd/shell/mintcert.go index 7383c0c1..7b4285fd 100644 --- a/pkg/cmd/shell/certonly.go +++ b/pkg/cmd/shell/mintcert.go @@ -19,11 +19,11 @@ import ( "github.com/brevdev/brev-cli/pkg/sshcert" ) -// certOnlyTimeout bounds the wait for one issuance; bounds the worst-case +// mintCertTimeout bounds the wait for one issuance; bounds the worst-case // delay ssh sees before a login. -const certOnlyTimeout = 15 * time.Second +const mintCertTimeout = 15 * time.Second -type certOnlyRequest struct { +type mintCertRequest struct { EnvironmentID string PortID string LinuxUser string @@ -70,15 +70,15 @@ func newCertIssuer(provider externalnode.TokenProvider, baseURL string) CertIssu return rpcCertIssuer{client: register.NewEnvironmentServiceClient(provider, baseURL)} } -type certOnlyStore interface { +type mintCertStore interface { GetAccessToken() (string, error) } -func runCertOnly(store ShellStore, req certOnlyRequest) error { - return runCertOnlyWith(store, afero.NewOsFs(), newCertIssuer(store, config.GlobalConfig.GetBrevPublicAPIURL()), req) +func runMintCert(store ShellStore, req mintCertRequest) error { + return runMintCertWith(store, afero.NewOsFs(), newCertIssuer(store, config.GlobalConfig.GetBrevPublicAPIURL()), req) } -func runCertOnlyWith(store certOnlyStore, fs afero.Fs, issuer CertIssuer, req certOnlyRequest) error { +func runMintCertWith(store mintCertStore, fs afero.Fs, issuer CertIssuer, req mintCertRequest) error { if _, err := store.GetAccessToken(); err != nil { _, _ = fmt.Fprintln(os.Stderr, "brev: no auth method found. Run `brev login` and retry.") return breverrors.WrapAndTrace(err) @@ -95,7 +95,7 @@ func runCertOnlyWith(store certOnlyStore, fs afero.Fs, issuer CertIssuer, req ce _, _ = fmt.Fprintf(os.Stderr, "brev: failed to generate keypair: %v\n", err) return breverrors.WrapAndTrace(err) } - ctx, cancel := context.WithTimeout(context.Background(), certOnlyTimeout) + ctx, cancel := context.WithTimeout(context.Background(), mintCertTimeout) defer cancel() res, err := issuer.Issue(ctx, certIssueRequest{ EnvironmentID: req.EnvironmentID, @@ -114,18 +114,18 @@ func runCertOnlyWith(store certOnlyStore, fs afero.Fs, issuer CertIssuer, req ce return nil } -type certOnlyFlags struct { +type mintCertFlags struct { env string port string user string outKey string } -func (f certOnlyFlags) requested() bool { +func (f mintCertFlags) requested() bool { return f.env != "" || f.port != "" || f.user != "" || f.outKey != "" } -func addCertOnlyFlags(cmd *cobra.Command, f *certOnlyFlags) { +func addMintCertFlags(cmd *cobra.Command, f *mintCertFlags) { cmd.Flags().StringVar(&f.env, "env", "", "environment ID for internal SSH certificate issuance") cmd.Flags().StringVar(&f.port, "port", "", "network-member port ID for internal SSH certificate issuance") cmd.Flags().StringVar(&f.user, "linux-user", "", "Linux user for internal SSH certificate issuance") @@ -136,7 +136,7 @@ func addCertOnlyFlags(cmd *cobra.Command, f *certOnlyFlags) { } } -func validateCertOnly(f certOnlyFlags) error { +func validateMintCert(f mintCertFlags) error { if !f.requested() { return nil } diff --git a/pkg/cmd/shell/certonly_test.go b/pkg/cmd/shell/mintcert_test.go similarity index 83% rename from pkg/cmd/shell/certonly_test.go rename to pkg/cmd/shell/mintcert_test.go index fc2f505d..dc109255 100644 --- a/pkg/cmd/shell/certonly_test.go +++ b/pkg/cmd/shell/mintcert_test.go @@ -18,7 +18,7 @@ import ( "github.com/brevdev/brev-cli/pkg/sshcert" ) -// fakeShellStore satisfies the certOnlyStore interface (GetAccessToken only). +// fakeShellStore satisfies the mintCertStore interface (GetAccessToken only). type fakeShellStore struct { token string err error @@ -72,16 +72,16 @@ func mintCertForTest(t *testing.T, pubKeyOpenSSH string) string { return strings.TrimRight(string(ssh.MarshalAuthorizedKey(cert)), "\n") } -func TestRunCertOnly_MintsAndWrites(t *testing.T) { +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) (certIssueResult, error) { return certIssueResult{Certificate: mintCertForTest(t, req.PublicKey)}, nil }} - if err := runCertOnlyWith(fakeShellStore{token: "tok"}, fs, issuer, certOnlyRequest{ + if err := runMintCertWith(fakeShellStore{token: "tok"}, fs, issuer, mintCertRequest{ EnvironmentID: "env-1", PortID: "port-1", LinuxUser: "ubuntu", OutKey: outKey, }); err != nil { - t.Fatalf("runCertOnlyWith: %v", err) + t.Fatalf("runMintCertWith: %v", err) } for _, p := range []string{outKey, outKey + "-cert.pub"} { if ok, _ := afero.Exists(fs, p); !ok { @@ -93,7 +93,7 @@ func TestRunCertOnly_MintsAndWrites(t *testing.T) { } } -func TestRunCertOnly_ReusesCachedCert(t *testing.T) { +func TestRunMintCert_ReusesCachedCert(t *testing.T) { fs := afero.NewMemMapFs() outKey := "/home/u/.brev/ssh-certs/env-1" _, pub, _ := sshcert.GenerateKeyPair() @@ -104,19 +104,19 @@ func TestRunCertOnly_ReusesCachedCert(t *testing.T) { t.Error("issuer should not be called when cache is valid") return certIssueResult{}, nil }} - if err := runCertOnlyWith(fakeShellStore{token: "tok"}, fs, issuer, certOnlyRequest{ + if err := runMintCertWith(fakeShellStore{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 TestRunCertOnly_FallsBackOnIssueError(t *testing.T) { +func TestRunMintCert_FallsBackOnIssueError(t *testing.T) { fs := afero.NewMemMapFs() issuer := &certIssuerFunc{fn: func(_ context.Context, _ certIssueRequest) (certIssueResult, error) { return certIssueResult{}, errors.New("CA unavailable") }} - err := runCertOnlyWith(fakeShellStore{token: "tok"}, fs, issuer, certOnlyRequest{ + err := runMintCertWith(fakeShellStore{token: "tok"}, fs, issuer, mintCertRequest{ EnvironmentID: "env-1", PortID: "port-1", LinuxUser: "ubuntu", OutKey: "/home/u/.brev/ssh-certs/env-1", }) @@ -128,13 +128,13 @@ func TestRunCertOnly_FallsBackOnIssueError(t *testing.T) { } } -func TestRunCertOnly_FallsBackOnAuthError(t *testing.T) { +func TestRunMintCert_FallsBackOnAuthError(t *testing.T) { fs := afero.NewMemMapFs() issuer := &certIssuerFunc{fn: func(_ context.Context, _ certIssueRequest) (certIssueResult, error) { t.Error("issuer should not be called when not authenticated") return certIssueResult{}, nil }} - if err := runCertOnlyWith(fakeShellStore{err: errors.New("no token")}, fs, issuer, certOnlyRequest{ + if err := runMintCertWith(fakeShellStore{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 { @@ -185,34 +185,34 @@ func TestRpcCertIssuer_PropagatesError(t *testing.T) { } } -func TestValidateCertOnly(t *testing.T) { +func TestValidateMintCert(t *testing.T) { tests := []struct { name string - flags certOnlyFlags + flags mintCertFlags wantErr bool }{ {name: "normal shell"}, - {name: "complete certificate request", flags: certOnlyFlags{env: "e", port: "p", user: "u", outKey: "/k"}}, - {name: "environment only", flags: certOnlyFlags{env: "e"}, wantErr: true}, - {name: "port only", flags: certOnlyFlags{port: "p"}, wantErr: true}, - {name: "linux user only", flags: certOnlyFlags{user: "u"}, wantErr: true}, - {name: "output key only", flags: certOnlyFlags{outKey: "/k"}, wantErr: true}, + {name: "complete certificate request", flags: mintCertFlags{env: "e", port: "p", user: "u", outKey: "/k"}}, + {name: "environment only", flags: mintCertFlags{env: "e"}, wantErr: true}, + {name: "port only", flags: mintCertFlags{port: "p"}, wantErr: true}, + {name: "linux user only", flags: mintCertFlags{user: "u"}, wantErr: true}, + {name: "output key only", flags: mintCertFlags{outKey: "/k"}, wantErr: true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := validateCertOnly(tt.flags) + err := validateMintCert(tt.flags) if (err != nil) != tt.wantErr { - t.Fatalf("validateCertOnly() error = %v, wantErr %v", err, tt.wantErr) + t.Fatalf("validateMintCert() error = %v, wantErr %v", err, tt.wantErr) } }) } } -func TestAddCertOnlyFlagsUsesImplicitCertificateMode(t *testing.T) { +func TestAddMintCertFlagsUsesImplicitCertificateMode(t *testing.T) { cmd := &cobra.Command{} - flags := certOnlyFlags{} - addCertOnlyFlags(cmd, &flags) + flags := mintCertFlags{} + addMintCertFlags(cmd, &flags) if flag := cmd.Flags().Lookup("cert-only"); flag != nil { t.Error("--cert-only should not be registered") diff --git a/pkg/cmd/shell/shell.go b/pkg/cmd/shell/shell.go index d81f1f1c..52de2857 100644 --- a/pkg/cmd/shell/shell.go +++ b/pkg/cmd/shell/shell.go @@ -53,7 +53,7 @@ type ShellStore interface { func NewCmdShell(t *terminal.Terminal, store ShellStore, noLoginStartStore ShellStore) *cobra.Command { var host bool - var certFlags certOnlyFlags + var certFlags mintCertFlags cmd := &cobra.Command{ Annotations: map[string]string{"access": ""}, Use: "shell ", @@ -65,11 +65,11 @@ func NewCmdShell(t *terminal.Terminal, store ShellStore, noLoginStartStore Shell Args: cobra.ExactArgs(1), ValidArgsFunction: completions.GetAllWorkspaceNameCompletionHandler(noLoginStartStore, t), RunE: func(cmd *cobra.Command, args []string) error { - if err := validateCertOnly(certFlags); err != nil { + if err := validateMintCert(certFlags); err != nil { return breverrors.WrapAndTrace(err) } if certFlags.requested() { - return runCertOnly(store, certOnlyRequest{ + return runMintCert(store, mintCertRequest{ EnvironmentID: certFlags.env, PortID: certFlags.port, LinuxUser: certFlags.user, @@ -85,7 +85,7 @@ func NewCmdShell(t *terminal.Terminal, store ShellStore, noLoginStartStore Shell }, } cmd.Flags().BoolVarP(&host, "host", "", false, "ssh into the host machine instead of the container") - addCertOnlyFlags(cmd, &certFlags) + addMintCertFlags(cmd, &certFlags) return cmd } From 61f618c7d3d087a97c82d0eb8be79611109a37be Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Fri, 21 Aug 2026 13:53:34 -0700 Subject: [PATCH 19/27] clean --- pkg/cmd/shell/mintcert_test.go | 5 ----- pkg/sshcert/sshcert.go | 7 ------- 2 files changed, 12 deletions(-) diff --git a/pkg/cmd/shell/mintcert_test.go b/pkg/cmd/shell/mintcert_test.go index dc109255..0ea602af 100644 --- a/pkg/cmd/shell/mintcert_test.go +++ b/pkg/cmd/shell/mintcert_test.go @@ -18,7 +18,6 @@ import ( "github.com/brevdev/brev-cli/pkg/sshcert" ) -// fakeShellStore satisfies the mintCertStore interface (GetAccessToken only). type fakeShellStore struct { token string err error @@ -31,7 +30,6 @@ func (f fakeShellStore) GetAccessToken() (string, error) { return f.token, nil } -// certIssuerFunc adapts a closure into a CertIssuer for tests. type certIssuerFunc struct { fn func(context.Context, certIssueRequest) (certIssueResult, error) } @@ -40,8 +38,6 @@ func (c *certIssuerFunc) Issue(ctx context.Context, req certIssueRequest) (certI return c.fn(ctx, req) } -// mintCertForTest mints a real user certificate over an in-memory CA for the -// given public key, so the written cert parses as a valid ssh.Certificate. func mintCertForTest(t *testing.T, pubKeyOpenSSH string) string { t.Helper() pubKey, _, _, _, err := ssh.ParseAuthorizedKey([]byte(pubKeyOpenSSH)) @@ -142,7 +138,6 @@ func TestRunMintCert_FallsBackOnAuthError(t *testing.T) { } } -// fakeEnvCertClient is a controllable environmentCertClient for testing rpcCertIssuer. type fakeEnvCertClient struct { resp *devplanev1.IssueEnvironmentSSHCertificateResponse err error diff --git a/pkg/sshcert/sshcert.go b/pkg/sshcert/sshcert.go index 0d2fd790..00830c94 100644 --- a/pkg/sshcert/sshcert.go +++ b/pkg/sshcert/sshcert.go @@ -112,21 +112,14 @@ func ParseCertificate(certOpenSSH string) (*ssh.Certificate, error) { return cert, nil } -// CertValidAt: a ValidBefore of 0 or ^uint64(0) means "forever" per the SSH spec. func CertValidAt(cert *ssh.Certificate, now time.Time, margin time.Duration) bool { if cert == nil { return false } - notBefore := int64(cert.ValidAfter) notAfter := int64(cert.ValidBefore) - if notAfter == 0 || notAfter == -1 { - return now.Add(margin).Unix() >= notBefore - } return now.Add(margin).Unix() < notAfter } -// HasValidCertAt returns (false, nil) for a missing or corrupt cert so the -// caller mints a fresh one rather than failing the whole ssh attempt. func HasValidCertAt(fs afero.Fs, certPath string, now time.Time, margin time.Duration) (bool, error) { exists, err := afero.Exists(fs, certPath) if err != nil { From f2445c1f685161822b126eb35008ce50ab40610a Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Fri, 21 Aug 2026 16:13:14 -0700 Subject: [PATCH 20/27] cleanup --- pkg/ssh/sshconfigurer.go | 26 +++++++---------- pkg/ssh/sshconfigurer_test.go | 55 ++++++++++++++++++++++++++++------- pkg/sshcert/sshcert.go | 12 ++++---- pkg/sshcert/sshcert_test.go | 18 ++++++------ 4 files changed, 71 insertions(+), 40 deletions(-) diff --git a/pkg/ssh/sshconfigurer.go b/pkg/ssh/sshconfigurer.go index 8d1c8757..eca85d35 100644 --- a/pkg/ssh/sshconfigurer.go +++ b/pkg/ssh/sshconfigurer.go @@ -6,6 +6,7 @@ import ( "fmt" "log" "os" + "path/filepath" "regexp" "strings" "text/template" @@ -176,7 +177,6 @@ type SSHConfigurerV2Store interface { GetWSLUserSSHConfig() (string, error) WriteWSLUserSSHConfig(config string) error GetBrevCloudflaredBinaryPath() (string, error) - UserHomeDir() (string, error) } var _ Config = SSHConfigurerV2{} @@ -260,17 +260,13 @@ func (s SSHConfigurerV2) CreateNewSSHConfig(workspaces []entity.Workspace, nodes return "", breverrors.WrapAndTrace(err) } - home, err := s.store.UserHomeDir() - if err != nil { - return "", breverrors.WrapAndTrace(err) - } - cloudflaredBinaryPath, err := s.store.GetBrevCloudflaredBinaryPath() if err != nil { return "", breverrors.WrapAndTrace(err) } - sshConfig, err := makeNewSSHConfig(configPath, workspaces, pkPath, cloudflaredBinaryPath, home) + brevDir := filepath.Dir(pkPath) + sshConfig, err := makeNewSSHConfig(configPath, workspaces, pkPath, cloudflaredBinaryPath, brevDir) if err != nil { return "", breverrors.WrapAndTrace(err) } @@ -286,11 +282,11 @@ func (s SSHConfigurerV2) CreateNewSSHConfig(workspaces []entity.Workspace, nodes return sshConfig, nil } -func makeNewSSHConfig(configPath string, workspaces []entity.Workspace, pkpath string, cloudflaredBinaryPath string, home string) (string, error) { +func makeNewSSHConfig(configPath string, workspaces []entity.Workspace, pkpath string, cloudflaredBinaryPath string, brevDir string) (string, error) { sshConfig := fmt.Sprintf("# included in %s\n", configPath) for _, w := range workspaces { - entry, err := makeSSHConfigEntryV2(w, pkpath, cloudflaredBinaryPath, home) + entry, err := makeSSHConfigEntryV2(w, pkpath, cloudflaredBinaryPath, brevDir) if err != nil { return "", breverrors.WrapAndTrace(err) } @@ -362,7 +358,7 @@ func tmplAndValToString(tmpl *template.Template, val interface{}) (string, error return buf.String(), nil } -func makeSSHConfigEntryV2(workspace entity.Workspace, privateKeyPath string, cloudflaredBinaryPath string, home string) (string, error) { //nolint:funlen,gocyclo // ok +func makeSSHConfigEntryV2(workspace entity.Workspace, privateKeyPath string, cloudflaredBinaryPath string, brevDir string) (string, error) { //nolint:funlen,gocyclo // ok alias := string(workspace.GetLocalIdentifier()) privateKeyPath = "\"" + privateKeyPath + "\"" var sshVal string @@ -468,7 +464,7 @@ func makeSSHConfigEntryV2(workspace entity.Workspace, privateKeyPath string, clo // below when the exec succeeds (cert first, static brev.pem as fallback); // when the exec fails the Match block's IdentityFile is dropped, so ssh // falls back to the static key. - if certMatch := makeCertMatchEntry(workspace, home); certMatch != "" { + if certMatch := makeCertMatchEntry(workspace, brevDir); certMatch != "" { val = certMatch + val } return val, nil @@ -479,13 +475,13 @@ func makeCloudflareSSHProxyCommand(cloudflaredBinaryPath string, hostname string } // makeCertMatchEntry returns the Match exec block for a cert-eligible workspace, -// or "" if not eligible or home is empty (WSL, deferred). +// or "" if not eligible or the Brev directory is empty (WSL, deferred). // // The exec command uses the absolute path to the running brev binary so it // resolves to the build that generated this config. The workspace alias keeps // the normal `brev shell ` argument contract intact. -func makeCertMatchEntry(workspace entity.Workspace, home string) string { - if home == "" || !workspace.SSHCertEligible || workspace.PortID == "" { +func makeCertMatchEntry(workspace entity.Workspace, brevDir string) string { + if brevDir == "" || !workspace.SSHCertEligible || workspace.PortID == "" { return "" } brevBin, err := os.Executable() @@ -493,7 +489,7 @@ func makeCertMatchEntry(workspace entity.Workspace, home string) string { brevBin = "brev" // fallback; degraded but no worse than the old bare-brev behavior } alias := string(workspace.GetLocalIdentifier()) - certKeyPath := sshcert.KeyPath(home, workspace.ID) + certKeyPath := sshcert.KeyPath(brevDir, workspace.ID) exec := shellescape.QuoteCommand([]string{ brevBin, "shell", diff --git a/pkg/ssh/sshconfigurer_test.go b/pkg/ssh/sshconfigurer_test.go index 9696a738..3ec23265 100644 --- a/pkg/ssh/sshconfigurer_test.go +++ b/pkg/ssh/sshconfigurer_test.go @@ -1,6 +1,7 @@ package ssh import ( + "errors" "fmt" "os" "strings" @@ -44,6 +45,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 } @@ -222,6 +235,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{}) @@ -518,7 +553,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, "/home/test-user") + got, err := makeSSHConfigEntryV2(tt.args.workspace, tt.args.privateKeyPath, tt.args.cloudflaredBinaryPath, "/home/test-user/.brev") if (err != nil) != tt.wantErr { t.Errorf("makeSSHConfigEntryV2() error = %v, wantErr %v", err, tt.wantErr) return @@ -939,7 +974,7 @@ func TestMakeCertMatchEntry_EligibleWorkspace(t *testing.T) { SSHCertEligible: true, PortID: "port-1", } - got := makeCertMatchEntry(w, "/home/u") + got := makeCertMatchEntry(w, "/home/u/.brev") // Must be a Match block with the workspace alias and the brev certificate exec. if !strings.HasPrefix(got, "Match host my-env exec \"") { t.Errorf("expected Match host my-env exec block, got: %s", got) @@ -971,18 +1006,18 @@ func TestMakeCertMatchEntry_EligibleWorkspace(t *testing.T) { 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"); got != "" { + if got := makeCertMatchEntry(w, "/home/u/.brev"); 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"); got != "" { + if got := makeCertMatchEntry(w2, "/home/u/.brev"); got != "" { t.Errorf("eligible without PortID should produce no Match block, got: %s", got) } - // Empty home (WSL) -> no Match block. + // Empty Brev directory (WSL) -> no Match block. w3 := entity.Workspace{ID: "env-1", Name: "n", SSHUser: "u", SSHCertEligible: true, PortID: "p"} if got := makeCertMatchEntry(w3, ""); got != "" { - t.Errorf("empty home should produce no Match block, got: %s", got) + t.Errorf("empty Brev directory should produce no Match block, got: %s", got) } } @@ -997,7 +1032,7 @@ func TestMakeSSHConfigEntryV2_EligibleWorkspaceIncludesCertMatch(t *testing.T) { SSHCertEligible: true, PortID: "port-1", } - got, err := makeSSHConfigEntryV2(w, "/home/u/.brev/brev.pem", "/tmp/cf", "/home/u") + got, err := makeSSHConfigEntryV2(w, "/home/u/.brev/brev.pem", "/tmp/cf", "/home/u/.brev") if err != nil { t.Fatalf("makeSSHConfigEntryV2: %v", err) } @@ -1032,7 +1067,7 @@ func TestMakeSSHConfigEntryV2_IneligibleWorkspaceNoCertMatch(t *testing.T) { SSHHostname: "10.0.0.1", // SSHCertEligible false, PortID empty } - got, err := makeSSHConfigEntryV2(w, "/home/u/.brev/brev.pem", "/tmp/cf", "/home/u") + got, err := makeSSHConfigEntryV2(w, "/home/u/.brev/brev.pem", "/tmp/cf", "/home/u/.brev") if err != nil { t.Fatalf("makeSSHConfigEntryV2: %v", err) } @@ -1051,7 +1086,7 @@ func TestMakeCertMatchEntry_UsesAbsoluteBrevPath(t *testing.T) { ID: "env-abc", Name: "n", SSHUser: "ubuntu", SSHCertEligible: true, PortID: "port-1", } - got := makeCertMatchEntry(w, "/home/u") + got := makeCertMatchEntry(w, "/home/u/.brev") exe, err := os.Executable() if err != nil { t.Skip("os.Executable unavailable; cannot assert path") @@ -1071,7 +1106,7 @@ func TestMakeCertMatchEntry_ShellEscapesCommandArguments(t *testing.T) { SSHCertEligible: true, PortID: "port;id", } - got := makeCertMatchEntry(w, "/home/user name") + got := makeCertMatchEntry(w, "/home/user name/.brev") want := "shell 'my-env;whoami' --env 'env;id' --port 'port;id' --linux-user 'user;id' --out-key '/home/user name/.brev/ssh-certs/env-id'" if !strings.Contains(got, want) { t.Errorf("expected shell-escaped Match exec %q; got: %s", want, got) diff --git a/pkg/sshcert/sshcert.go b/pkg/sshcert/sshcert.go index 00830c94..e4f338cb 100644 --- a/pkg/sshcert/sshcert.go +++ b/pkg/sshcert/sshcert.go @@ -35,8 +35,8 @@ func EnvironmentCertEligible(labels map[string]string) bool { return labels[LabelKeySSHProvider] == SSHProviderCertAuth } -func Dir(home string) string { - return filepath.Join(home, ".brev", certSubDir) +func Dir(brevDir string) string { + return filepath.Join(brevDir, certSubDir) } func safeFilename(envID string) string { @@ -60,14 +60,14 @@ func safeFilename(envID string) string { return out } -func KeyPath(home, envID string) string { - return filepath.Join(Dir(home), safeFilename(envID)) +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(home, envID string) string { - return KeyPath(home, envID) + "-cert.pub" +func CertPath(brevDir, envID string) string { + return KeyPath(brevDir, envID) + "-cert.pub" } // GenerateKeyPair returns the private key in OpenSSH PEM format (for diff --git a/pkg/sshcert/sshcert_test.go b/pkg/sshcert/sshcert_test.go index 4d249a18..1326c0bc 100644 --- a/pkg/sshcert/sshcert_test.go +++ b/pkg/sshcert/sshcert_test.go @@ -113,9 +113,9 @@ func TestCertValidAt(t *testing.T) { if CertValidAt(expired, now, time.Minute) { t.Error("expired cert should not be valid") } - forever := &ssh.Certificate{ValidAfter: uint64(now.Add(-time.Hour).Unix()), ValidBefore: 0} - if !CertValidAt(forever, now, time.Minute) { - t.Error("forever cert within ValidAfter should 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") @@ -124,7 +124,7 @@ func TestCertValidAt(t *testing.T) { func TestHasValidCertAt(t *testing.T) { fs := afero.NewMemMapFs() - certPath := CertPath("/home/u", "env-1") + certPath := CertPath("/home/u/.brev", "env-1") // Missing -> not valid, no error. if ok, err := HasValidCertAt(fs, certPath, time.Now(), DefaultRenewalMargin); ok || err != nil { @@ -132,14 +132,14 @@ func TestHasValidCertAt(t *testing.T) { } // Written -> valid. privPEM, _ := mustGen(t) - if err := WriteFiles(fs, KeyPath("/home/u", "env-1"), certPath, privPEM, mintTestCert(t, time.Now().Add(10*time.Minute))); err != nil { + 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") } // Different env -> not valid. - if ok, _ := HasValidCertAt(fs, CertPath("/home/u", "env-2"), time.Now(), DefaultRenewalMargin); ok { + 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). @@ -154,16 +154,16 @@ func TestHasValidCertAt(t *testing.T) { func TestWriteFiles_NoLeftoverTemp(t *testing.T) { fs := afero.NewMemMapFs() privPEM, _ := mustGen(t) - if err := WriteFiles(fs, KeyPath("/h", "x"), CertPath("/h", "x"), privPEM, mintTestCert(t, time.Now().Add(5*time.Minute))); err != nil { + 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")) + 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", "x")) + b, _ := afero.ReadFile(fs, CertPath("/h/.brev", "x")) if !strings.HasSuffix(string(b), "\n") { t.Error("cert file should end with newline") } From 28819b0899f7adc6c50f16a5dda7fa04f1b897ab Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Fri, 21 Aug 2026 16:26:11 -0700 Subject: [PATCH 21/27] refactor: extract mint-cert into its own hidden command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pull cert minting out of the shell command into a standalone hidden 'brev mint-cert' command. shell is now pure: 1 positional arg, always a shell — no mode split, no flag inference, no branch-on-cert. mint-cert owns its own required flags, its own NoArgs validation, and its own help, so the contract is clean: refresh writes the ssh config referencing mint-cert, and mint-cert owns the minting. Trivially exposeable later by un-hiding it. The Match exec line now invokes 'mint-cert' instead of 'shell --env ...' with a positional alias, so the ExactArgs(1) bug class can't recur and the mode-inference wart is gone. --- pkg/cmd/cmd.go | 2 + pkg/cmd/{shell => mintcert}/mintcert.go | 125 ++++++++++--------- pkg/cmd/{shell => mintcert}/mintcert_test.go | 95 ++++---------- pkg/cmd/shell/shell.go | 13 -- pkg/ssh/sshconfigurer.go | 3 +- pkg/ssh/sshconfigurer_test.go | 12 +- 6 files changed, 100 insertions(+), 150 deletions(-) rename pkg/cmd/{shell => mintcert}/mintcert.go (54%) rename pkg/cmd/{shell => mintcert}/mintcert_test.go (74%) diff --git a/pkg/cmd/cmd.go b/pkg/cmd/cmd.go index 831a2aa9..4c1e3d97 100644 --- a/pkg/cmd/cmd.go +++ b/pkg/cmd/cmd.go @@ -48,6 +48,7 @@ import ( "github.com/brevdev/brev-cli/pkg/cmd/set" "github.com/brevdev/brev-cli/pkg/cmd/setupworkspace" "github.com/brevdev/brev-cli/pkg/cmd/shell" + "github.com/brevdev/brev-cli/pkg/cmd/mintcert" "github.com/brevdev/brev-cli/pkg/cmd/sshkeys" "github.com/brevdev/brev-cli/pkg/cmd/start" "github.com/brevdev/brev-cli/pkg/cmd/status" @@ -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(loginCmdStore)) 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/shell/mintcert.go b/pkg/cmd/mintcert/mintcert.go similarity index 54% rename from pkg/cmd/shell/mintcert.go rename to pkg/cmd/mintcert/mintcert.go index 7b4285fd..07be69f5 100644 --- a/pkg/cmd/shell/mintcert.go +++ b/pkg/cmd/mintcert/mintcert.go @@ -1,10 +1,13 @@ -package shell +// 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" - "strings" "time" devplanev1 "buf.build/gen/go/brevdev/devplane/protocolbuffers/go/devplaneapi/v1" @@ -19,17 +22,19 @@ import ( "github.com/brevdev/brev-cli/pkg/sshcert" ) -// mintCertTimeout bounds the wait for one issuance; bounds the worst-case -// delay ssh sees before a login. -const mintCertTimeout = 15 * time.Second +// timeout bounds the wait for one issuance; bounds the worst-case delay ssh +// sees before a login (the Match exec hook runs synchronously during config +// evaluation). +const timeout = 15 * time.Second -type mintCertRequest struct { - EnvironmentID string - PortID string - LinuxUser string - OutKey string // absolute path to write the private key (cert goes to -cert.pub) +// Store is the minimal dependency: the existing platform credential, reused +// with no new login. *store.AuthHTTPStore satisfies this via GetAccessToken. +type Store interface { + GetAccessToken() (string, error) } +// CertIssuer mints a short-lived SSH certificate. The interface lets the +// command be unit-tested with a fake issuer. type CertIssuer interface { Issue(ctx context.Context, req certIssueRequest) (certIssueResult, error) } @@ -45,6 +50,8 @@ type certIssueResult struct { Certificate string } +// environmentCertClient is the subset of the connect EnvironmentServiceClient +// that cert issuance needs. type environmentCertClient interface { IssueEnvironmentSSHCertificate(ctx context.Context, req *connect.Request[devplanev1.IssueEnvironmentSSHCertificateRequest]) (*connect.Response[devplanev1.IssueEnvironmentSSHCertificateResponse], error) } @@ -66,19 +73,55 @@ func (r rpcCertIssuer) Issue(ctx context.Context, req certIssueRequest) (certIss return certIssueResult{Certificate: res.Msg.GetCertificate()}, nil } -func newCertIssuer(provider externalnode.TokenProvider, baseURL string) CertIssuer { - return rpcCertIssuer{client: register.NewEnvironmentServiceClient(provider, baseURL)} +// NewCmdMintCert creates the `brev mint-cert` command. hidden keeps it out of +// help until it's ready to be public. +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 mintCertStore interface { - GetAccessToken() (string, error) +type mintCertRequest struct { + EnvironmentID string + PortID string + LinuxUser string + OutKey string } -func runMintCert(store ShellStore, req mintCertRequest) error { +// runMintCert is invoked by the ssh config's Match exec hook. On any failure it +// writes nothing and returns non-zero so ssh drops the cert IdentityFile and +// falls back to the static brev.pem. It never prompts — that would hang ssh. +func runMintCert(store Store, req mintCertRequest) error { return runMintCertWith(store, afero.NewOsFs(), newCertIssuer(store, config.GlobalConfig.GetBrevPublicAPIURL()), req) } -func runMintCertWith(store mintCertStore, fs afero.Fs, issuer CertIssuer, req mintCertRequest) error { +func runMintCertWith(store Store, fs afero.Fs, issuer CertIssuer, req mintCertRequest) error { if _, err := store.GetAccessToken(); err != nil { _, _ = fmt.Fprintln(os.Stderr, "brev: no auth method found. Run `brev login` and retry.") return breverrors.WrapAndTrace(err) @@ -95,7 +138,7 @@ func runMintCertWith(store mintCertStore, fs afero.Fs, issuer CertIssuer, req mi _, _ = fmt.Fprintf(os.Stderr, "brev: failed to generate keypair: %v\n", err) return breverrors.WrapAndTrace(err) } - ctx, cancel := context.WithTimeout(context.Background(), mintCertTimeout) + ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() res, err := issuer.Issue(ctx, certIssueRequest{ EnvironmentID: req.EnvironmentID, @@ -114,47 +157,9 @@ func runMintCertWith(store mintCertStore, fs afero.Fs, issuer CertIssuer, req mi return nil } -type mintCertFlags struct { - env string - port string - user string - outKey string -} - -func (f mintCertFlags) requested() bool { - return f.env != "" || f.port != "" || f.user != "" || f.outKey != "" -} - -func addMintCertFlags(cmd *cobra.Command, f *mintCertFlags) { - cmd.Flags().StringVar(&f.env, "env", "", "environment ID for internal SSH certificate issuance") - cmd.Flags().StringVar(&f.port, "port", "", "network-member port ID for internal SSH certificate issuance") - cmd.Flags().StringVar(&f.user, "linux-user", "", "Linux user for internal SSH certificate issuance") - cmd.Flags().StringVar(&f.outKey, "out-key", "", "private-key path for internal SSH certificate issuance") - - for _, name := range []string{"env", "port", "linux-user", "out-key"} { - _ = cmd.Flags().MarkHidden(name) - } -} - -func validateMintCert(f mintCertFlags) error { - if !f.requested() { - return nil - } - var missing []string - if f.env == "" { - missing = append(missing, "--env") - } - if f.port == "" { - missing = append(missing, "--port") - } - if f.user == "" { - missing = append(missing, "--linux-user") - } - if f.outKey == "" { - missing = append(missing, "--out-key") - } - if len(missing) > 0 { - return fmt.Errorf("SSH certificate request requires %s", strings.Join(missing, ", ")) - } - return nil +// newCertIssuer returns an rpcCertIssuer. The token provider is the store +// itself — it satisfies externalnode.TokenProvider via GetAccessToken, so the +// existing platform credential is reused with no new login. +func newCertIssuer(provider externalnode.TokenProvider, baseURL string) CertIssuer { + return rpcCertIssuer{client: register.NewEnvironmentServiceClient(provider, baseURL)} } diff --git a/pkg/cmd/shell/mintcert_test.go b/pkg/cmd/mintcert/mintcert_test.go similarity index 74% rename from pkg/cmd/shell/mintcert_test.go rename to pkg/cmd/mintcert/mintcert_test.go index 0ea602af..85acdca3 100644 --- a/pkg/cmd/shell/mintcert_test.go +++ b/pkg/cmd/mintcert/mintcert_test.go @@ -1,4 +1,4 @@ -package shell +package mintcert import ( "context" @@ -12,24 +12,25 @@ import ( devplanev1 "buf.build/gen/go/brevdev/devplane/protocolbuffers/go/devplaneapi/v1" "connectrpc.com/connect" "github.com/spf13/afero" - "github.com/spf13/cobra" "golang.org/x/crypto/ssh" "github.com/brevdev/brev-cli/pkg/sshcert" ) -type fakeShellStore struct { +// fakeStore satisfies Store (GetAccessToken only). +type fakeStore struct { token string err error } -func (f fakeShellStore) GetAccessToken() (string, error) { +func (f fakeStore) GetAccessToken() (string, error) { if f.err != nil { return "", f.err } return f.token, nil } +// certIssuerFunc adapts a closure into a CertIssuer for tests. type certIssuerFunc struct { fn func(context.Context, certIssueRequest) (certIssueResult, error) } @@ -38,6 +39,23 @@ func (c *certIssuerFunc) Issue(ctx context.Context, req certIssueRequest) (certI return c.fn(ctx, req) } +// fakeEnvCertClient is a controllable environmentCertClient for testing rpcCertIssuer. +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 +} + +// mintCertForTest mints a real user certificate over an in-memory CA for the +// given public key, so the written cert parses as a valid ssh.Certificate. func mintCertForTest(t *testing.T, pubKeyOpenSSH string) string { t.Helper() pubKey, _, _, _, err := ssh.ParseAuthorizedKey([]byte(pubKeyOpenSSH)) @@ -74,7 +92,7 @@ func TestRunMintCert_MintsAndWrites(t *testing.T) { issuer := &certIssuerFunc{fn: func(_ context.Context, req certIssueRequest) (certIssueResult, error) { return certIssueResult{Certificate: mintCertForTest(t, req.PublicKey)}, nil }} - if err := runMintCertWith(fakeShellStore{token: "tok"}, fs, issuer, mintCertRequest{ + 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) @@ -100,7 +118,7 @@ func TestRunMintCert_ReusesCachedCert(t *testing.T) { t.Error("issuer should not be called when cache is valid") return certIssueResult{}, nil }} - if err := runMintCertWith(fakeShellStore{token: "tok"}, fs, issuer, mintCertRequest{ + 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) @@ -112,7 +130,7 @@ func TestRunMintCert_FallsBackOnIssueError(t *testing.T) { issuer := &certIssuerFunc{fn: func(_ context.Context, _ certIssueRequest) (certIssueResult, error) { return certIssueResult{}, errors.New("CA unavailable") }} - err := runMintCertWith(fakeShellStore{token: "tok"}, fs, issuer, mintCertRequest{ + err := runMintCertWith(fakeStore{token: "tok"}, fs, issuer, mintCertRequest{ EnvironmentID: "env-1", PortID: "port-1", LinuxUser: "ubuntu", OutKey: "/home/u/.brev/ssh-certs/env-1", }) @@ -130,7 +148,7 @@ func TestRunMintCert_FallsBackOnAuthError(t *testing.T) { t.Error("issuer should not be called when not authenticated") return certIssueResult{}, nil }} - if err := runMintCertWith(fakeShellStore{err: errors.New("no token")}, fs, issuer, mintCertRequest{ + 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 { @@ -138,20 +156,6 @@ func TestRunMintCert_FallsBackOnAuthError(t *testing.T) { } } -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 TestRpcCertIssuer_MapsRequestAndResponse(t *testing.T) { client := &fakeEnvCertClient{resp: &devplanev1.IssueEnvironmentSSHCertificateResponse{ Certificate: "ssh-ed25519-cert-v01@openssh.com AAAA cert", @@ -179,50 +183,3 @@ func TestRpcCertIssuer_PropagatesError(t *testing.T) { t.Fatal("expected error to propagate") } } - -func TestValidateMintCert(t *testing.T) { - tests := []struct { - name string - flags mintCertFlags - wantErr bool - }{ - {name: "normal shell"}, - {name: "complete certificate request", flags: mintCertFlags{env: "e", port: "p", user: "u", outKey: "/k"}}, - {name: "environment only", flags: mintCertFlags{env: "e"}, wantErr: true}, - {name: "port only", flags: mintCertFlags{port: "p"}, wantErr: true}, - {name: "linux user only", flags: mintCertFlags{user: "u"}, wantErr: true}, - {name: "output key only", flags: mintCertFlags{outKey: "/k"}, wantErr: true}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := validateMintCert(tt.flags) - if (err != nil) != tt.wantErr { - t.Fatalf("validateMintCert() error = %v, wantErr %v", err, tt.wantErr) - } - }) - } -} - -func TestAddMintCertFlagsUsesImplicitCertificateMode(t *testing.T) { - cmd := &cobra.Command{} - flags := mintCertFlags{} - addMintCertFlags(cmd, &flags) - - if flag := cmd.Flags().Lookup("cert-only"); flag != nil { - t.Error("--cert-only should not be registered") - } - if flag := cmd.Flags().Lookup("user"); flag != nil { - t.Error("certificate flags must not shadow persistent --user") - } - for _, name := range []string{"env", "port", "linux-user", "out-key"} { - flag := cmd.Flags().Lookup(name) - if flag == nil { - t.Errorf("--%s is not registered", name) - continue - } - if !flag.Hidden { - t.Errorf("--%s must remain hidden", name) - } - } -} diff --git a/pkg/cmd/shell/shell.go b/pkg/cmd/shell/shell.go index 52de2857..a9760719 100644 --- a/pkg/cmd/shell/shell.go +++ b/pkg/cmd/shell/shell.go @@ -53,7 +53,6 @@ type ShellStore interface { func NewCmdShell(t *terminal.Terminal, store ShellStore, noLoginStartStore ShellStore) *cobra.Command { var host bool - var certFlags mintCertFlags cmd := &cobra.Command{ Annotations: map[string]string{"access": ""}, Use: "shell ", @@ -65,17 +64,6 @@ func NewCmdShell(t *terminal.Terminal, store ShellStore, noLoginStartStore Shell Args: cobra.ExactArgs(1), ValidArgsFunction: completions.GetAllWorkspaceNameCompletionHandler(noLoginStartStore, t), RunE: func(cmd *cobra.Command, args []string) error { - if err := validateMintCert(certFlags); err != nil { - return breverrors.WrapAndTrace(err) - } - if certFlags.requested() { - return runMintCert(store, mintCertRequest{ - EnvironmentID: certFlags.env, - PortID: certFlags.port, - LinuxUser: certFlags.user, - OutKey: certFlags.outKey, - }) - } instanceName := args[0] err := runShellCommand(t, store, instanceName, host) if err != nil { @@ -85,7 +73,6 @@ func NewCmdShell(t *terminal.Terminal, store ShellStore, noLoginStartStore Shell }, } cmd.Flags().BoolVarP(&host, "host", "", false, "ssh into the host machine instead of the container") - addMintCertFlags(cmd, &certFlags) return cmd } diff --git a/pkg/ssh/sshconfigurer.go b/pkg/ssh/sshconfigurer.go index eca85d35..ea29fffa 100644 --- a/pkg/ssh/sshconfigurer.go +++ b/pkg/ssh/sshconfigurer.go @@ -492,8 +492,7 @@ func makeCertMatchEntry(workspace entity.Workspace, brevDir string) string { certKeyPath := sshcert.KeyPath(brevDir, workspace.ID) exec := shellescape.QuoteCommand([]string{ brevBin, - "shell", - alias, + "mint-cert", "--env", workspace.ID, "--port", diff --git a/pkg/ssh/sshconfigurer_test.go b/pkg/ssh/sshconfigurer_test.go index 3ec23265..929d97b6 100644 --- a/pkg/ssh/sshconfigurer_test.go +++ b/pkg/ssh/sshconfigurer_test.go @@ -979,8 +979,8 @@ func TestMakeCertMatchEntry_EligibleWorkspace(t *testing.T) { if !strings.HasPrefix(got, "Match host my-env exec \"") { t.Errorf("expected Match host my-env exec block, got: %s", got) } - if !strings.Contains(got, " shell my-env --env env-abc") { - t.Errorf("missing positional workspace alias: %s", got) + if !strings.Contains(got, " mint-cert --env env-abc") { + t.Errorf("missing mint-cert subcommand: %s", got) } if !strings.Contains(got, "--port port-1") { t.Errorf("missing --port port-1: %s", got) @@ -992,7 +992,7 @@ func TestMakeCertMatchEntry_EligibleWorkspace(t *testing.T) { t.Errorf("Match exec must infer certificate mode from its hidden fields: %s", got) } if strings.Contains(got, " --user ") { - t.Errorf("Match exec must not shadow Brev's persistent --user flag: %s", got) + t.Errorf("Match exec must not use --user (use --linux-user): %s", got) } if !strings.Contains(got, "--out-key /home/u/.brev/ssh-certs/env-abc") { t.Errorf("missing out-key path: %s", got) @@ -1091,11 +1091,11 @@ func TestMakeCertMatchEntry_UsesAbsoluteBrevPath(t *testing.T) { if err != nil { t.Skip("os.Executable unavailable; cannot assert path") } - want := fmt.Sprintf("%s shell n --env env-abc", exe) + 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 shell") { + if strings.Contains(got, " exec \"brev mint-cert") { t.Errorf("Match exec must not use bare `brev`: %s", got) } } @@ -1107,7 +1107,7 @@ func TestMakeCertMatchEntry_ShellEscapesCommandArguments(t *testing.T) { } got := makeCertMatchEntry(w, "/home/user name/.brev") - want := "shell 'my-env;whoami' --env 'env;id' --port 'port;id' --linux-user 'user;id' --out-key '/home/user name/.brev/ssh-certs/env-id'" + want := "mint-cert --env 'env;id' --port 'port;id' --linux-user 'user;id' --out-key '/home/user name/.brev/ssh-certs/env-id'" if !strings.Contains(got, want) { t.Errorf("expected shell-escaped Match exec %q; got: %s", want, got) } From 86cf3484f9ca2a8369903d955b298db073a6aec7 Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Fri, 21 Aug 2026 16:29:29 -0700 Subject: [PATCH 22/27] clean --- pkg/cmd/mintcert/mintcert.go | 18 +----------------- pkg/cmd/mintcert/mintcert_test.go | 5 ----- 2 files changed, 1 insertion(+), 22 deletions(-) diff --git a/pkg/cmd/mintcert/mintcert.go b/pkg/cmd/mintcert/mintcert.go index 07be69f5..8f2d5f4f 100644 --- a/pkg/cmd/mintcert/mintcert.go +++ b/pkg/cmd/mintcert/mintcert.go @@ -22,19 +22,13 @@ import ( "github.com/brevdev/brev-cli/pkg/sshcert" ) -// timeout bounds the wait for one issuance; bounds the worst-case delay ssh -// sees before a login (the Match exec hook runs synchronously during config -// evaluation). +// timeout bounds the worst-case ssh delay before login const timeout = 15 * time.Second -// Store is the minimal dependency: the existing platform credential, reused -// with no new login. *store.AuthHTTPStore satisfies this via GetAccessToken. type Store interface { GetAccessToken() (string, error) } -// CertIssuer mints a short-lived SSH certificate. The interface lets the -// command be unit-tested with a fake issuer. type CertIssuer interface { Issue(ctx context.Context, req certIssueRequest) (certIssueResult, error) } @@ -50,8 +44,6 @@ type certIssueResult struct { Certificate string } -// environmentCertClient is the subset of the connect EnvironmentServiceClient -// that cert issuance needs. type environmentCertClient interface { IssueEnvironmentSSHCertificate(ctx context.Context, req *connect.Request[devplanev1.IssueEnvironmentSSHCertificateRequest]) (*connect.Response[devplanev1.IssueEnvironmentSSHCertificateResponse], error) } @@ -73,8 +65,6 @@ func (r rpcCertIssuer) Issue(ctx context.Context, req certIssueRequest) (certIss return certIssueResult{Certificate: res.Msg.GetCertificate()}, nil } -// NewCmdMintCert creates the `brev mint-cert` command. hidden keeps it out of -// help until it's ready to be public. func NewCmdMintCert(store Store) *cobra.Command { var ( env string @@ -114,9 +104,6 @@ type mintCertRequest struct { OutKey string } -// runMintCert is invoked by the ssh config's Match exec hook. On any failure it -// writes nothing and returns non-zero so ssh drops the cert IdentityFile and -// falls back to the static brev.pem. It never prompts — that would hang ssh. func runMintCert(store Store, req mintCertRequest) error { return runMintCertWith(store, afero.NewOsFs(), newCertIssuer(store, config.GlobalConfig.GetBrevPublicAPIURL()), req) } @@ -157,9 +144,6 @@ func runMintCertWith(store Store, fs afero.Fs, issuer CertIssuer, req mintCertRe return nil } -// newCertIssuer returns an rpcCertIssuer. The token provider is the store -// itself — it satisfies externalnode.TokenProvider via GetAccessToken, so the -// existing platform credential is reused with no new login. 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 index 85acdca3..15672d62 100644 --- a/pkg/cmd/mintcert/mintcert_test.go +++ b/pkg/cmd/mintcert/mintcert_test.go @@ -17,7 +17,6 @@ import ( "github.com/brevdev/brev-cli/pkg/sshcert" ) -// fakeStore satisfies Store (GetAccessToken only). type fakeStore struct { token string err error @@ -30,7 +29,6 @@ func (f fakeStore) GetAccessToken() (string, error) { return f.token, nil } -// certIssuerFunc adapts a closure into a CertIssuer for tests. type certIssuerFunc struct { fn func(context.Context, certIssueRequest) (certIssueResult, error) } @@ -39,7 +37,6 @@ func (c *certIssuerFunc) Issue(ctx context.Context, req certIssueRequest) (certI return c.fn(ctx, req) } -// fakeEnvCertClient is a controllable environmentCertClient for testing rpcCertIssuer. type fakeEnvCertClient struct { resp *devplanev1.IssueEnvironmentSSHCertificateResponse err error @@ -54,8 +51,6 @@ func (f *fakeEnvCertClient) IssueEnvironmentSSHCertificate(_ context.Context, re return connect.NewResponse(f.resp), nil } -// mintCertForTest mints a real user certificate over an in-memory CA for the -// given public key, so the written cert parses as a valid ssh.Certificate. func mintCertForTest(t *testing.T, pubKeyOpenSSH string) string { t.Helper() pubKey, _, _, _, err := ssh.ParseAuthorizedKey([]byte(pubKeyOpenSSH)) From 10426dd481dafca16b86621f788fe4df87f91533 Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Fri, 21 Aug 2026 16:34:30 -0700 Subject: [PATCH 23/27] fix(mintcert): use noLoginCmdStore, treat empty token as auth failure mint-cert was wired with loginCmdStore, whose GetAccessToken prompts on stdin when logged out. The Match exec hook runs non-interactively during ssh config evaluation, so a prompt would hang the ssh invocation. Use noLoginCmdStore (GetFreshAccessTokenOrNil, returns "" with no prompt) and treat an empty token as a hard auth failure so ssh drops the cert IdentityFile and falls back to brev.pem immediately. --- pkg/cmd/cmd.go | 4 ++-- pkg/cmd/mintcert/mintcert.go | 14 +++++++++++--- pkg/cmd/mintcert/mintcert_test.go | 8 ++++++++ pkg/ssh/sshconfigurer.go | 12 ++++++------ 4 files changed, 27 insertions(+), 11 deletions(-) diff --git a/pkg/cmd/cmd.go b/pkg/cmd/cmd.go index 4c1e3d97..5065799b 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" @@ -48,7 +49,6 @@ import ( "github.com/brevdev/brev-cli/pkg/cmd/set" "github.com/brevdev/brev-cli/pkg/cmd/setupworkspace" "github.com/brevdev/brev-cli/pkg/cmd/shell" - "github.com/brevdev/brev-cli/pkg/cmd/mintcert" "github.com/brevdev/brev-cli/pkg/cmd/sshkeys" "github.com/brevdev/brev-cli/pkg/cmd/start" "github.com/brevdev/brev-cli/pkg/cmd/status" @@ -304,7 +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(loginCmdStore)) + 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 index 8f2d5f4f..f376ee22 100644 --- a/pkg/cmd/mintcert/mintcert.go +++ b/pkg/cmd/mintcert/mintcert.go @@ -109,10 +109,18 @@ func runMintCert(store Store, req mintCertRequest) error { } func runMintCertWith(store Store, fs afero.Fs, issuer CertIssuer, req mintCertRequest) error { - if _, err := store.GetAccessToken(); err != nil { - _, _ = fmt.Fprintln(os.Stderr, "brev: no auth method found. Run `brev login` and retry.") - return breverrors.WrapAndTrace(err) + token, err := store.GetAccessToken() + if err != nil || token == "" { + // Match exec must stay non-interactive: an empty/expired token is a hard + // failure so ssh drops the cert IdentityFile and falls back to brev.pem, + // rather than blocking on a login prompt that would hang the ssh invocation. + _, _ = 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) diff --git a/pkg/cmd/mintcert/mintcert_test.go b/pkg/cmd/mintcert/mintcert_test.go index 15672d62..3dc26e13 100644 --- a/pkg/cmd/mintcert/mintcert_test.go +++ b/pkg/cmd/mintcert/mintcert_test.go @@ -143,12 +143,20 @@ func TestRunMintCert_FallsBackOnAuthError(t *testing.T) { t.Error("issuer should not be called when not authenticated") return certIssueResult{}, 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) { diff --git a/pkg/ssh/sshconfigurer.go b/pkg/ssh/sshconfigurer.go index ea29fffa..4bf9b487 100644 --- a/pkg/ssh/sshconfigurer.go +++ b/pkg/ssh/sshconfigurer.go @@ -242,7 +242,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)) if err != nil { return "", breverrors.WrapAndTrace(err) } @@ -265,8 +265,7 @@ func (s SSHConfigurerV2) CreateNewSSHConfig(workspaces []entity.Workspace, nodes return "", breverrors.WrapAndTrace(err) } - brevDir := filepath.Dir(pkPath) - sshConfig, err := makeNewSSHConfig(configPath, workspaces, pkPath, cloudflaredBinaryPath, brevDir) + sshConfig, err := makeNewSSHConfig(configPath, workspaces, pkPath, cloudflaredBinaryPath) if err != nil { return "", breverrors.WrapAndTrace(err) } @@ -282,11 +281,11 @@ func (s SSHConfigurerV2) CreateNewSSHConfig(workspaces []entity.Workspace, nodes return sshConfig, nil } -func makeNewSSHConfig(configPath string, workspaces []entity.Workspace, pkpath string, cloudflaredBinaryPath string, brevDir string) (string, error) { +func makeNewSSHConfig(configPath string, workspaces []entity.Workspace, pkpath string, cloudflaredBinaryPath string) (string, error) { sshConfig := fmt.Sprintf("# included in %s\n", configPath) for _, w := range workspaces { - entry, err := makeSSHConfigEntryV2(w, pkpath, cloudflaredBinaryPath, brevDir) + entry, err := makeSSHConfigEntryV2(w, pkpath, cloudflaredBinaryPath) if err != nil { return "", breverrors.WrapAndTrace(err) } @@ -358,8 +357,9 @@ func tmplAndValToString(tmpl *template.Template, val interface{}) (string, error return buf.String(), nil } -func makeSSHConfigEntryV2(workspace entity.Workspace, privateKeyPath string, cloudflaredBinaryPath string, brevDir string) (string, error) { //nolint:funlen,gocyclo // ok +func makeSSHConfigEntryV2(workspace entity.Workspace, privateKeyPath string, cloudflaredBinaryPath string) (string, error) { //nolint:funlen,gocyclo // ok alias := string(workspace.GetLocalIdentifier()) + brevDir := filepath.Dir(privateKeyPath) privateKeyPath = "\"" + privateKeyPath + "\"" var sshVal string user := workspace.GetSSHUser() From b544ab0deb4a3745b95e6aab15e4304b10773ba3 Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Fri, 21 Aug 2026 16:34:52 -0700 Subject: [PATCH 24/27] docs: drop stale 'brev shell argument contract' comment Left over from when makeCertMatchEntry emitted 'shell --env ...' with a positional alias; mint-cert is its own command now, so the alias contract no longer applies. --- pkg/ssh/sshconfigurer.go | 3 +-- pkg/ssh/sshconfigurer_test.go | 6 +++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/pkg/ssh/sshconfigurer.go b/pkg/ssh/sshconfigurer.go index 4bf9b487..2d16f5d4 100644 --- a/pkg/ssh/sshconfigurer.go +++ b/pkg/ssh/sshconfigurer.go @@ -478,8 +478,7 @@ func makeCloudflareSSHProxyCommand(cloudflaredBinaryPath string, hostname string // or "" if not eligible or the Brev directory is empty (WSL, deferred). // // The exec command uses the absolute path to the running brev binary so it -// resolves to the build that generated this config. The workspace alias keeps -// the normal `brev shell ` argument contract intact. +// resolves to the build that generated this config. func makeCertMatchEntry(workspace entity.Workspace, brevDir string) string { if brevDir == "" || !workspace.SSHCertEligible || workspace.PortID == "" { return "" diff --git a/pkg/ssh/sshconfigurer_test.go b/pkg/ssh/sshconfigurer_test.go index 929d97b6..eea7bf60 100644 --- a/pkg/ssh/sshconfigurer_test.go +++ b/pkg/ssh/sshconfigurer_test.go @@ -553,7 +553,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, "/home/test-user/.brev") + got, err := makeSSHConfigEntryV2(tt.args.workspace, tt.args.privateKeyPath, tt.args.cloudflaredBinaryPath) if (err != nil) != tt.wantErr { t.Errorf("makeSSHConfigEntryV2() error = %v, wantErr %v", err, tt.wantErr) return @@ -1032,7 +1032,7 @@ func TestMakeSSHConfigEntryV2_EligibleWorkspaceIncludesCertMatch(t *testing.T) { SSHCertEligible: true, PortID: "port-1", } - got, err := makeSSHConfigEntryV2(w, "/home/u/.brev/brev.pem", "/tmp/cf", "/home/u/.brev") + got, err := makeSSHConfigEntryV2(w, "/home/u/.brev/brev.pem", "/tmp/cf") if err != nil { t.Fatalf("makeSSHConfigEntryV2: %v", err) } @@ -1067,7 +1067,7 @@ func TestMakeSSHConfigEntryV2_IneligibleWorkspaceNoCertMatch(t *testing.T) { SSHHostname: "10.0.0.1", // SSHCertEligible false, PortID empty } - got, err := makeSSHConfigEntryV2(w, "/home/u/.brev/brev.pem", "/tmp/cf", "/home/u/.brev") + got, err := makeSSHConfigEntryV2(w, "/home/u/.brev/brev.pem", "/tmp/cf") if err != nil { t.Fatalf("makeSSHConfigEntryV2: %v", err) } From e93eb6a1536d80919400c9650930c0f006adc299 Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Fri, 21 Aug 2026 16:39:55 -0700 Subject: [PATCH 25/27] test: remove 4 low-value/duplicative cert tests Cut: - TestEnvironmentCertEligible: tests a one-line map lookup, already covered by the refresh integration test. - TestRpcCertIssuer_PropagatesError: tests WrapAndTrace, nothing of ours to break. - TestMakeCertMatchEntry_EligibleWorkspace: overlaps with ShellEscapesCommandArguments (same fields) and UsesAbsoluteBrevPath (same path); its negative assertions guard against forms that no longer exist. - TestMakeSSHConfigEntryV2_IneligibleWorkspaceNoCertMatch: 'no Match block' covered by IneligibleWorkspace at the unit level; 'static key present' tests pre-existing unchanged behavior. 15 cert tests remain across the three files. --- pkg/cmd/mintcert/mintcert_test.go | 8 ----- pkg/ssh/sshconfigurer_test.go | 59 ------------------------------- pkg/sshcert/sshcert_test.go | 30 ---------------- 3 files changed, 97 deletions(-) diff --git a/pkg/cmd/mintcert/mintcert_test.go b/pkg/cmd/mintcert/mintcert_test.go index 3dc26e13..a071cb7e 100644 --- a/pkg/cmd/mintcert/mintcert_test.go +++ b/pkg/cmd/mintcert/mintcert_test.go @@ -178,11 +178,3 @@ func TestRpcCertIssuer_MapsRequestAndResponse(t *testing.T) { t.Errorf("request fields wrong: %+v", client.got) } } - -func TestRpcCertIssuer_PropagatesError(t *testing.T) { - client := &fakeEnvCertClient{err: errors.New("permission denied")} - issuer := rpcCertIssuer{client: client} - if _, err := issuer.Issue(context.Background(), certIssueRequest{}); err == nil { - t.Fatal("expected error to propagate") - } -} diff --git a/pkg/ssh/sshconfigurer_test.go b/pkg/ssh/sshconfigurer_test.go index eea7bf60..95131ee3 100644 --- a/pkg/ssh/sshconfigurer_test.go +++ b/pkg/ssh/sshconfigurer_test.go @@ -966,43 +966,6 @@ Host testName1-host } } -func TestMakeCertMatchEntry_EligibleWorkspace(t *testing.T) { - w := entity.Workspace{ - ID: "env-abc", - Name: "my-env", - SSHUser: "ubuntu", - SSHCertEligible: true, - PortID: "port-1", - } - got := makeCertMatchEntry(w, "/home/u/.brev") - // Must be a Match block with the workspace alias and the brev certificate exec. - if !strings.HasPrefix(got, "Match host my-env exec \"") { - t.Errorf("expected Match host my-env exec block, got: %s", got) - } - if !strings.Contains(got, " mint-cert --env env-abc") { - t.Errorf("missing mint-cert subcommand: %s", got) - } - if !strings.Contains(got, "--port port-1") { - t.Errorf("missing --port port-1: %s", got) - } - if !strings.Contains(got, "--linux-user ubuntu") { - t.Errorf("missing --linux-user ubuntu: %s", got) - } - if strings.Contains(got, "--cert-only") { - t.Errorf("Match exec must infer certificate mode from its hidden fields: %s", got) - } - if strings.Contains(got, " --user ") { - t.Errorf("Match exec must not use --user (use --linux-user): %s", got) - } - if !strings.Contains(got, "--out-key /home/u/.brev/ssh-certs/env-abc") { - t.Errorf("missing out-key path: %s", got) - } - // IdentityFile must point at the cert key path (quoted). - if !strings.Contains(got, "IdentityFile \"/home/u/.brev/ssh-certs/env-abc\"") { - t.Errorf("missing IdentityFile cert path: %s", got) - } -} - func TestMakeCertMatchEntry_IneligibleWorkspace(t *testing.T) { // No SSHCertEligible flag -> no Match block. w := entity.Workspace{ID: "env-1", Name: "n", SSHUser: "u", PortID: "p"} @@ -1057,28 +1020,6 @@ func TestMakeSSHConfigEntryV2_EligibleWorkspaceIncludesCertMatch(t *testing.T) { } } -func TestMakeSSHConfigEntryV2_IneligibleWorkspaceNoCertMatch(t *testing.T) { - w := entity.Workspace{ - ID: "env-old", - Name: "old-env", - Status: entity.Running, - SSHUser: "ubuntu", - SSHPort: 22, - SSHHostname: "10.0.0.1", - // SSHCertEligible false, PortID empty - } - got, err := makeSSHConfigEntryV2(w, "/home/u/.brev/brev.pem", "/tmp/cf") - if err != nil { - t.Fatalf("makeSSHConfigEntryV2: %v", err) - } - if strings.Contains(got, "Match host") { - t.Errorf("ineligible workspace should have no Match block: %s", got) - } - if !strings.Contains(got, "/home/u/.brev/brev.pem") { - t.Error("static key should still be present") - } -} - 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. diff --git a/pkg/sshcert/sshcert_test.go b/pkg/sshcert/sshcert_test.go index 1326c0bc..cd53a832 100644 --- a/pkg/sshcert/sshcert_test.go +++ b/pkg/sshcert/sshcert_test.go @@ -169,36 +169,6 @@ func TestWriteFiles_NoLeftoverTemp(t *testing.T) { } } -func TestEnvironmentCertEligible(t *testing.T) { - cases := []struct { - labels map[string]string - want bool - }{ - {map[string]string{"sshprovider": "certauth"}, true}, - {map[string]string{"sshprovider": "other"}, false}, - {map[string]string{}, false}, - } - for _, c := range cases { - if got := EnvironmentCertEligible(c.labels); got != c.want { - t.Errorf("EnvironmentCertEligible(%v)=%v, want %v", c.labels, got, c.want) - } - } -} - -func TestSafeFilename(t *testing.T) { - cases := map[string]string{ - "env_123": "env_123", - "env/evil": "env-evil", - "": "default", - "../etc/pw": "..-etc-pw", - } - for in, want := range cases { - if got := safeFilename(in); got != want { - t.Errorf("safeFilename(%q)=%q, want %q", in, got, want) - } - } -} - func mustGen(t *testing.T) ([]byte, string) { t.Helper() priv, pub, err := GenerateKeyPair() From 71e9818467082bfc67c15d01a2812b3206d42e64 Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Fri, 21 Aug 2026 16:48:04 -0700 Subject: [PATCH 26/27] clean --- pkg/ssh/sshconfigurer.go | 32 ++------------------------------ 1 file changed, 2 insertions(+), 30 deletions(-) diff --git a/pkg/ssh/sshconfigurer.go b/pkg/ssh/sshconfigurer.go index 2d16f5d4..152da627 100644 --- a/pkg/ssh/sshconfigurer.go +++ b/pkg/ssh/sshconfigurer.go @@ -460,10 +460,7 @@ func makeSSHConfigEntryV2(workspace entity.Workspace, privateKeyPath string, clo val := fmt.Sprintf("%s%s", sshVal, hostSSHVal) - // ssh accumulates IdentityFile across the Match block and the Host block - // below when the exec succeeds (cert first, static brev.pem as fallback); - // when the exec fails the Match block's IdentityFile is dropped, so ssh - // falls back to the static key. + // prepend the Match exec so that failure falls back to key based ssh if certMatch := makeCertMatchEntry(workspace, brevDir); certMatch != "" { val = certMatch + val } @@ -474,15 +471,11 @@ func makeCloudflareSSHProxyCommand(cloudflaredBinaryPath string, hostname string return fmt.Sprintf("%s access ssh --hostname %s", cloudflaredBinaryPath, hostname) } -// makeCertMatchEntry returns the Match exec block for a cert-eligible workspace, -// or "" if not eligible or the Brev directory is empty (WSL, deferred). -// -// The exec command uses the absolute path to the running brev binary so it -// resolves to the build that generated this config. func makeCertMatchEntry(workspace entity.Workspace, brevDir string) string { if brevDir == "" || !workspace.SSHCertEligible || workspace.PortID == "" { return "" } + // get the binary that generated this config, so that test builds work brevBin, err := os.Executable() if err != nil { brevBin = "brev" // fallback; degraded but no worse than the old bare-brev behavior @@ -580,27 +573,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 } From 844f4cf4a0a89185f237ba93a643b057de5ff554 Mon Sep 17 00:00:00 2001 From: Pratik Patel Date: Fri, 21 Aug 2026 16:49:07 -0700 Subject: [PATCH 27/27] docs: cut redundant comments that restate the code Removed: - GenerateKeyPair doc (restates the return types in its signature) - timeout const comment (restates the Match exec context in the package doc) - 'prepend the Match exec so that failure falls back' (the line below prepends it; the fallback is ssh's behavior, not ours) - 'get the binary that generated this config, so that test builds work' (inaccurate; the real reason is to avoid PATH-resolving a stale brev) - section-label comments in tests that restate the assertions below them Kept the comments that earn their place: the expiry-race why, the label-duplication why, the -cert.pub convention, the atomicity property, the corrupt-cert-mints-fresh contract, and the empty-token non-interactive gotcha. --- pkg/cmd/mintcert/mintcert.go | 1 - pkg/ssh/sshconfigurer.go | 2 -- pkg/ssh/sshconfigurer_test.go | 1 - pkg/sshcert/sshcert.go | 3 --- pkg/sshcert/sshcert_test.go | 3 --- 5 files changed, 10 deletions(-) diff --git a/pkg/cmd/mintcert/mintcert.go b/pkg/cmd/mintcert/mintcert.go index f376ee22..e738bb09 100644 --- a/pkg/cmd/mintcert/mintcert.go +++ b/pkg/cmd/mintcert/mintcert.go @@ -22,7 +22,6 @@ import ( "github.com/brevdev/brev-cli/pkg/sshcert" ) -// timeout bounds the worst-case ssh delay before login const timeout = 15 * time.Second type Store interface { diff --git a/pkg/ssh/sshconfigurer.go b/pkg/ssh/sshconfigurer.go index 152da627..9cc41bce 100644 --- a/pkg/ssh/sshconfigurer.go +++ b/pkg/ssh/sshconfigurer.go @@ -460,7 +460,6 @@ func makeSSHConfigEntryV2(workspace entity.Workspace, privateKeyPath string, clo val := fmt.Sprintf("%s%s", sshVal, hostSSHVal) - // prepend the Match exec so that failure falls back to key based ssh if certMatch := makeCertMatchEntry(workspace, brevDir); certMatch != "" { val = certMatch + val } @@ -475,7 +474,6 @@ func makeCertMatchEntry(workspace entity.Workspace, brevDir string) string { if brevDir == "" || !workspace.SSHCertEligible || workspace.PortID == "" { return "" } - // get the binary that generated this config, so that test builds work brevBin, err := os.Executable() if err != nil { brevBin = "brev" // fallback; degraded but no worse than the old bare-brev behavior diff --git a/pkg/ssh/sshconfigurer_test.go b/pkg/ssh/sshconfigurer_test.go index 95131ee3..bae81581 100644 --- a/pkg/ssh/sshconfigurer_test.go +++ b/pkg/ssh/sshconfigurer_test.go @@ -1011,7 +1011,6 @@ func TestMakeSSHConfigEntryV2_EligibleWorkspaceIncludesCertMatch(t *testing.T) { if matchIdx >= hostIdx { t.Errorf("Match block must precede Host block (match=%d host=%d)", matchIdx, hostIdx) } - // Both the cert IdentityFile (in Match) and static brev.pem (in Host) must be present. if !strings.Contains(got, "/home/u/.brev/ssh-certs/env-cert") { t.Error("missing cert key path in Match block") } diff --git a/pkg/sshcert/sshcert.go b/pkg/sshcert/sshcert.go index e4f338cb..cb623659 100644 --- a/pkg/sshcert/sshcert.go +++ b/pkg/sshcert/sshcert.go @@ -70,9 +70,6 @@ func CertPath(brevDir, envID string) string { return KeyPath(brevDir, envID) + "-cert.pub" } -// GenerateKeyPair returns the private key in OpenSSH PEM format (for -// IdentityFile) and the public key as a single-line authorized-key string (the -// format the issuance RPC expects as its public_key field). func GenerateKeyPair() (privKeyPEM []byte, pubKeyOpenSSH string, err error) { pub, priv, err := ed25519.GenerateKey(rand.Reader) if err != nil { diff --git a/pkg/sshcert/sshcert_test.go b/pkg/sshcert/sshcert_test.go index cd53a832..6f020f86 100644 --- a/pkg/sshcert/sshcert_test.go +++ b/pkg/sshcert/sshcert_test.go @@ -126,11 +126,9 @@ func TestHasValidCertAt(t *testing.T) { fs := afero.NewMemMapFs() certPath := CertPath("/home/u/.brev", "env-1") - // Missing -> not valid, no error. if ok, err := HasValidCertAt(fs, certPath, time.Now(), DefaultRenewalMargin); ok || err != nil { t.Fatalf("missing cert: ok=%v err=%v", ok, err) } - // Written -> valid. 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) @@ -138,7 +136,6 @@ func TestHasValidCertAt(t *testing.T) { if ok, _ := HasValidCertAt(fs, certPath, time.Now(), DefaultRenewalMargin); !ok { t.Error("expected valid after write") } - // Different env -> not valid. if ok, _ := HasValidCertAt(fs, CertPath("/home/u/.brev", "env-2"), time.Now(), DefaultRenewalMargin); ok { t.Error("env-2 should have no cert") }