Skip to content

feat(server)!: delete the server-secret write path #1220

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rigel-mintaka wants to merge 2 commits into from
Open

feat(server)!: delete the server-secret write path #1220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8d6e1ef
feat(server)!: delete the server-secret write path
rigel-mintaka Sep 13, 2026
d7bf0e2
docs(server): correct comments the write-path deletion made stale
rigel-mintaka Sep 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions go/cmd/compass-stack/preflight.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,11 @@ import (
// later V5 wave. This command is a thin consumer of that shared core — no
// longer a placeholder to be replaced — and keeps only what is stack-specific:
// the podman rootless-capability check (postgres runs as a rootless container)
// and the print/exit surface. It also reports the secretspec CLI: the secrets
// WRITE path (internal/secrets SpecResolver.Set) spawns it by name, so it is an
// install-time dependency the operator must have even though boot, which reads
// through the SDK, never touches it — surfacing it here is what turns "the first
// admin write fails" into an install-time line. That is one more entry in this
// same list, not a new abstraction. Do not grow this into a capability
// framework.
// and the print/exit surface. It also reports the secretspec CLI: no Go code
// spawns it, but it is the operator's server-secret rotation path, so surfacing
// it here turns "the first rotation fails" into an install-time line. That is
// one more entry in this same list, not a new abstraction. Do not grow this
// into a capability framework.

// podmanBinary is the podman executable name, resolved on PATH. It is the check
// name and the LookPath target, so it is named once here (goconst).
Expand Down Expand Up @@ -103,7 +101,7 @@ func runPreflight(args []string) error {
checks = append(checks, checkBinaryVersion(f))
}
// After the trio so the microVM group stays contiguous and the output order
// is stable: secretspec is the secrets write path's dependency, not a microVM
// is stable: secretspec is the operator's rotation dependency, not a microVM
// userspace binary.
checks = append(checks, checkBinaryVersion(hostcheck.SecretSpecFloor))

Expand Down
7 changes: 3 additions & 4 deletions go/cmd/compass/secret.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,10 +130,9 @@ func newSecretDeleteCmd() *cobra.Command {
// required and it is read from stdin.
var errEmptySecretValue = errors.New("a secret value is required: pipe it on stdin (it is never taken from the command line)")

// readSecretValue reads a secret value from stdin, the ONE place both the user
// and server-secret set paths get it — so the size cap, the trailing-newline
// trim (a bare `echo` adds one and it is not part of the value), and the
// empty-value rejection cannot drift between the two verbs.
// readSecretValue reads a secret value from stdin: it applies the size cap, the
// trailing-newline trim (a bare `echo` adds one and it is not part of the
// value), and the empty-value rejection.
//
// stdin is the only source by design: a value on argv would be visible in the
// host process list.
Expand Down
91 changes: 5 additions & 86 deletions go/cmd/compass/server_secret.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,46 +17,18 @@ import (
)

// newServerSecretCmd builds the server-secret noun: the DEPLOYMENT-owned secret
// surface (set/list), disjoint from the fleet `secret` noun by reserved name
// prefix. It carries no logic of its own; each verb is a child that dials the
// Server and drives one SecretsService RPC. A server secret value is read from
// stdin, never argv, so it cannot leak into the process table.
// surface, disjoint from the fleet `secret` noun by reserved name prefix. It
// carries no logic of its own; its verb is a child that dials the Server and
// drives one SecretsService RPC.
func newServerSecretCmd() *cobra.Command {
cmd := &cobra.Command{
Use: "server-secret",
Short: "Manage deployment-owned server secrets (set / list)",
Short: "Manage deployment-owned server secrets (list)",
}
cmd.AddCommand(newServerSecretSetCmd(), newServerSecretListCmd())
cmd.AddCommand(newServerSecretListCmd())
return cmd
}

// newServerSecretSetCmd builds `server-secret set <NAME>`: write a server
// secret's value. The value is read from stdin, never a flag or positional, so
// it cannot leak into the process table (the load-bearing convention shared
// with the fleet `secret set` verb and the bearer token).
//
// The name is accepted with OR without the reserved prefix, because the two
// sides spell it differently: the deployment's config carries the BARE name
// (the operator writes `forge.appId`-style config, not a registry key) while
// the server-secret registry carries the PREFIXED one (serve.go's
// serverSecretName wraps every declared name). Accepting both and sending the
// prefixed form means the operator can paste either spelling and still write
// the row the Server reads.
func newServerSecretSetCmd() *cobra.Command {
return &cobra.Command{
Use: "set <NAME>",
Short: "Write a server secret's value (value read from stdin, admin)",
Args: cobra.ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
client, err := dialSecretsClient(cmd)
if err != nil {
return err
}
return runServerSecretSet(cmd.Context(), client, args[0], cmd.InOrStdin(), cmd.OutOrStdout())
},
}
}

// newServerSecretListCmd builds `server-secret list`: ListServerSecrets and
// render each declared server secret's name and set/unset state. It NEVER
// renders a value (there is none on the wire). An empty list renders a clear
Expand All @@ -76,59 +48,6 @@ func newServerSecretListCmd() *cobra.Command {
}
}

// runServerSecretSet reads the value from in (trimming a single trailing
// newline and rejecting an empty value) and calls SetServerSecret under the
// prefixed name. The value is never taken from argv, so it cannot leak into the
// process table.
func runServerSecretSet(ctx context.Context, client compassv1connect.SecretsServiceClient, name string, in io.Reader, out io.Writer) error {
value, err := readSecretValue(in)
if err != nil {
return err
}
wire, err := serverSecretWireName(name)
if err != nil {
return err
}

ctx, cancel := context.WithTimeout(ctx, rpcTimeout)
defer cancel()
if _, err := client.SetServerSecret(ctx, connect.NewRequest(&compassv1.SetServerSecretRequest{
Name: wire,
Value: value,
})); err != nil {
return fmt.Errorf("setting server secret %s: %w", wire, err)
}
_, err = fmt.Fprintf(out, "set server secret %s\n", wire)
return err
}

// serverSecretWireName maps the operator's spelling to the registry's. A name
// that already carries a reserved prefix is sent as-is (never double-prefixed);
// a bare one is wrapped, matching serve.go's serverSecretName. The store's
// HasServerSecretPrefix is the authority on what counts as prefixed, so the two
// doors cannot drift.
//
// A bare name that would SHADOW the master-key row is refused rather than
// wrapped. `list` strips any reserved prefix, so the master key prints as the
// bare `MASTER_KEY`; feeding that spelling back here would wrap it to
// `SERVER_MASTER_KEY`, which is a DIFFERENT secret. That name clears the
// server's master-key guard (it compares the exact COMPASS_MASTER_KEY name),
// so the write would silently mint a shadow row, leave the real key untouched,
// and make `list` print the same bare name twice. Refusing is the only safe
// answer: wrapping writes a different secret than the operator named, with no
// error at any layer.
func serverSecretWireName(name string) (string, error) {
if store.HasServerSecretPrefix(name) {
return name, nil
}
if store.CompassPrefix+name == store.MasterKeyName {
return "", fmt.Errorf(
"%s is the bare spelling of %s, which is provisioned and rotated by the server; pass the full name if you meant a different secret",
name, store.MasterKeyName)
}
return store.ServerSecretPrefix + name, nil
}

// runServerSecretList calls ListServerSecrets and renders each declared server
// secret. An empty list renders a clear message, not an error.
//
Expand Down
124 changes: 6 additions & 118 deletions go/cmd/compass/server_secret_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,14 @@ import (
"github.com/RigelBuild/compass/go/internal/store"
)

// fakeServerSecrets is a fake SecretsService handler recording the request each
// server-secret verb constructs and returning a canned ListServerSecrets
// response, so the subcommand RPC wiring is tested without a live Server or
// Postgres (mirroring fakeSecrets for the user-facing verbs).
// fakeServerSecrets is a fake SecretsService handler returning a canned
// ListServerSecrets response, so the list subcommand's RPC wiring is tested
// without a live Server or Postgres (mirroring fakeSecrets for the user-facing
// verbs).
type fakeServerSecrets struct {
compassv1connect.UnimplementedSecretsServiceHandler
gotSet *compassv1.SetServerSecretRequest
setCalls int
list *compassv1.ListServerSecretsResponse
gotAuth string
}

func (f *fakeServerSecrets) SetServerSecret(_ context.Context, req *connect.Request[compassv1.SetServerSecretRequest]) (*connect.Response[compassv1.SetServerSecretResponse], error) {
f.setCalls++
f.gotSet = req.Msg
f.gotAuth = req.Header().Get("Authorization")
return connect.NewResponse(&compassv1.SetServerSecretResponse{}), nil
list *compassv1.ListServerSecretsResponse
gotAuth string
}

func (f *fakeServerSecrets) ListServerSecrets(_ context.Context, req *connect.Request[compassv1.ListServerSecretsRequest]) (*connect.Response[compassv1.ListServerSecretsResponse], error) {
Expand Down Expand Up @@ -134,106 +125,3 @@ func TestRunServerSecretListEmpty(t *testing.T) {
t.Errorf("empty-list output %q does not report an empty registry", out.String())
}
}

// TestRunServerSecretSetPrefixesName asserts the value comes from stdin (never
// argv) and that a bare operator-facing name is sent PREFIXED on the wire, while
// an already-prefixed name is not double-prefixed.
func TestRunServerSecretSetPrefixesName(t *testing.T) {
tests := []struct {
name string
input string
want string
}{
{name: "bare name is prefixed", input: "FORGE_APP_PEM", want: "SERVER_FORGE_APP_PEM"},
{name: "prefixed name is unchanged", input: "SERVER_FORGE_APP_PEM", want: "SERVER_FORGE_APP_PEM"},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
fake := &fakeServerSecrets{}
client := startFakeServerSecretsServer(t, fake)

var out strings.Builder
in := strings.NewReader("s3cr3t\n")
if err := runServerSecretSet(context.Background(), client, tt.input, in, &out); err != nil {
t.Fatalf("runServerSecretSet: %v", err)
}
if fake.gotSet == nil {
t.Fatal("SetServerSecret was not called")
}
if fake.gotSet.GetName() != tt.want {
t.Errorf("name = %q, want %q", fake.gotSet.GetName(), tt.want)
}
if fake.gotSet.GetValue() != "s3cr3t" {
t.Errorf("value = %q, want s3cr3t (trailing newline trimmed, from stdin)", fake.gotSet.GetValue())
}
if fake.gotAuth != "Bearer test-token" {
t.Errorf("Authorization = %q, want Bearer test-token", fake.gotAuth)
}
})
}
}

// TestRunServerSecretSetRefusesBareMasterKey pins the round-trip hazard: `list`
// strips any reserved prefix, so the master key prints as bare MASTER_KEY.
// Wrapping that spelling would send SERVER_MASTER_KEY — a DIFFERENT secret that
// clears the server's exact-name master-key guard, minting a shadow row while
// the real key stays unprovisioned and `list` prints the same bare name twice.
// It must be refused before any RPC.
func TestRunServerSecretSetRefusesBareMasterKey(t *testing.T) {
fake := &fakeServerSecrets{}
client := startFakeServerSecretsServer(t, fake)

var out strings.Builder
in := strings.NewReader("s3cr3t\n")
err := runServerSecretSet(context.Background(), client, "MASTER_KEY", in, &out)
if err == nil {
t.Fatal("bare MASTER_KEY was accepted; it must be refused rather than re-prefixed to a different secret")
}
if fake.gotSet != nil {
t.Errorf("SetServerSecret was called with %q; the refusal must precede any RPC", fake.gotSet.GetName())
}
if !strings.Contains(err.Error(), store.MasterKeyName) {
t.Errorf("error %q does not name %s, so it is not actionable", err, store.MasterKeyName)
}
}

// TestRunServerSecretSetAcceptsFullMasterKeyName asserts the refusal is narrow:
// the FULL master-key name still reaches the server, which is what fail-closes
// on it (secrets_service.go's store.MasterKeyName guard). The CLI must not become a
// second, divergent authority on which names are writable.
func TestRunServerSecretSetAcceptsFullMasterKeyName(t *testing.T) {
fake := &fakeServerSecrets{}
client := startFakeServerSecretsServer(t, fake)

var out strings.Builder
in := strings.NewReader("s3cr3t\n")
if err := runServerSecretSet(context.Background(), client, store.MasterKeyName, in, &out); err != nil {
t.Fatalf("runServerSecretSet: %v", err)
}
if fake.gotSet == nil {
t.Fatal("SetServerSecret was not called; the server must be the authority on this refusal")
}
if fake.gotSet.GetName() != store.MasterKeyName {
t.Errorf("name = %q, want %q unchanged", fake.gotSet.GetName(), store.MasterKeyName)
}
}

// TestRunServerSecretSetEmptyStdin asserts an empty stdin value is rejected with
// the shared empty-value error BEFORE any RPC — a blank pipe must never clear a
// populated server secret.
func TestRunServerSecretSetEmptyStdin(t *testing.T) {
fake := &fakeServerSecrets{}
client := startFakeServerSecretsServer(t, fake)

var out strings.Builder
err := runServerSecretSet(context.Background(), client, "FORGE_APP_PEM", strings.NewReader("\n"), &out)
if err == nil {
t.Fatal("runServerSecretSet with empty stdin = nil error, want rejection")
}
if !strings.Contains(err.Error(), "value is required") {
t.Errorf("error %q does not mention the required value", err.Error())
}
if fake.setCalls != 0 {
t.Errorf("SetServerSecret called %d times despite an empty value", fake.setCalls)
}
}
Loading
Loading