Feat/ssh certs - #452
Draft
patelspratik wants to merge 27 commits into
Draft
Conversation
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/<envID>{,-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).
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.
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.
For each cert-eligible workspace (sshprovider=certauth label + port_id), the SSH config generator now prepends a Match host <alias> 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.
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.
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.
patelspratik
force-pushed
the
feat/ssh-certs
branch
from
August 20, 2026 23:58
3d5b905 to
e719698
Compare
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.
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.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #452 +/- ##
==========================================
+ Coverage 24.67% 26.20% +1.52%
==========================================
Files 134 137 +3
Lines 18337 18858 +521
==========================================
+ Hits 4525 4942 +417
- Misses 13335 13400 +65
- Partials 477 516 +39
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
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.
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.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.