cloudfoundry-community / cloudfoundry-community/safe

Strongbox opt-in migration cannot see targets that had it enabled

Open
#66 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Go
Stars
5
Forks
2
PR merge metrics
No merged PRs in 30d

Description

Upgrading to v1.20.0 silently turns Strongbox off for every target that had it on, because the legacy-key migration in cd043be keys off a field that f6509b8 made structurally impossible to write seven years ago.

The commit that broke it

f6509b8c34786964023b73df5fd512ce3c60fe34"don't write nostrongbox as strongbox in config", 2019-11-04, first released in v1.4.0.

 type Vault struct {
-	URL        string   `yaml:"url"`
-	Token      string   `yaml:"token"`
-	CACerts    []string `yaml:"ca_certs,omitempty"`
-	SkipVerify bool     `yaml:"skip_verify"`
-	//FIXME: NoStrongbox should not go into the config as "strongbox"
-	NoStrongbox bool `yaml:"strongbox"`
+	URL         string   `yaml:"url"`
+	Token       string   `yaml:"token"`
+	CACerts     []string `yaml:"ca_certs,omitempty"`
+	SkipVerify  bool     `yaml:"skip_verify,omitempty"`
+	NoStrongbox bool     `yaml:"no_strongbox,omitempty"`

Renaming the key was correct — the old one was inverted, as the FIXME said. Adding omitempty is the problem. Strongbox-enabled means NoStrongbox == false, and omitempty drops false, so from v1.4.0 onward safe writes nothing at all for a Strongbox-enabled target. The enabled state has had no on-disk representation for seven years; it was encoded purely as the absence of a key.

That was self-consistent while absence meant "on". It stopped being self-consistent when the default flipped.

What surfaced it

  • c62a87e"Make Strongbox opt-in per target" (2026-07-31, first in v1.20.0) changed the field to Strongbox bool yaml:"strongbox,omitempty" and HasStrongbox() to return v.Strongbox directly. Absence now means off.
  • cd043be"Honor legacy no_strongbox key when present" (2026-08-01, also v1.20.0) added the UnmarshalYAML shim intended to make that upgrade lossless.

The shim's own comment states the goal:

// UnmarshalYAML translates the legacy no_strongbox key into Strongbox when
// the key is present in the document [...] so an upgrade does not silently
// disarm Strongbox for every pre-existing target

But it is gated on presence:

if legacy.NoStrongbox != nil {
	v.Strongbox = !*legacy.NoStrongbox
}

Because of f6509b8, no_strongbox is present only when Strongbox was disabled. So the shim fires exactly for targets that were already off — the state the new default produces anyway — and never for the targets it was written to protect. It is a no-op with respect to its stated purpose.

Reproduction

Two rc files, one per legacy state, read by safe vdev/develop/0f50bec:

# Strongbox was DISABLED under <= v1.10.0
vaults:
  t: {url: https://vault.example.com, token: "", no_strongbox: true}
# Strongbox was ENABLED under <= v1.10.0 (the only on-disk form that exists)
vaults:
  t: {url: https://vault.example.com, token: ""}
$ safe target --json   # legacy-disabled
{ "name": "t", ..., "strongbox": false }     # correct

$ safe target --json   # legacy-enabled
{ "name": "t", ..., "strongbox": false }     # WRONG — was on, now off, no warning

On a real ~/.saferc with 12 targets, none of which had ever been given --no-strongbox, all 12 now report strongbox: false.

Write behaviour, both eras

Each era uses omitempty and so can persist only its own non-default case. They are exact mirrors:

safe target … v1.4.0 – v1.10.0 v1.20.0+
(no flag) writes nothing writes nothing
--strongbox writes nothing strongbox: true
--no-strongbox no_strongbox: true writes nothing

Secondary problem: older safe strips the new key

strongbox is unknown to the v1.10.0-and-earlier struct, and gopkg.in/yaml.v2 is non-strict, so it is dropped on read and omitted on write. Any rc write by an older binary — including targeting an unrelated alias — silently deletes it:

$ safe-dev  target --strongbox https://vault.example.com A   # strongbox: true
$ safe-1.9.0 target https://vault.example.com B              # unrelated alias
$ grep -A3 '  A:' ~/.saferc                                  # strongbox: true is gone
$ safe-dev  target --json                                    # "strongbox": false

So on any machine where both versions are reachable, --strongbox cannot be made to stick. This makes "just re-run safe target --strongbox" an unreliable workaround as well as a manual one.

Downstream impact

Genesis reads strongbox from safe targets --json and compares it against the value recorded in a deployment repository's .genesis/config when it was first configured. Repos set up under <= v1.10.0 recorded true; the same target now reports false, and Genesis refuses to resolve the vault at all:

[FATAL] Could not find matching safe target, but the following are similar:
         Alias:     'ops'
         Strongbox: '0' (expected '1')

Ten of eleven deployment repos pointing at one vault became unusable on upgrade. (Genesis is over-strict here — a seal-status sidecar should not gate vault resolution — and that is being fixed separately. But the trigger is the flip.)

Suggested fix

The shim cannot work as written, because on-disk absence is ambiguous between "pre-flip default: on" and "post-flip default: off" and nothing in the file distinguishes them. The rc already carries the discriminator:

version: 1
current: ...

Proposal: on reading version: 1, treat an absent key as on (the pre-flip default) and honour no_strongbox when present; then write version: 2 with strongbox always explicit — no omitempty — so the state is unambiguous from then on. A version: 2 file is read literally.

That also fixes the stripping problem for anyone who upgrades once and stays upgraded, since the key is then always present.

Whatever the mechanism, it seems worth (a) not changing a security-relevant default silently on upgrade, and (b) emitting a one-time notice when a target's effective Strongbox setting changes as a result of the migration.

Versions

  • Broken by: f6509b8 (v1.4.0, 2019-11-04) — made the enabled state unrepresentable
  • Surfaced by: c62a87e + cd043be (both v1.20.0, 2026-07-31/08-01)
  • Observed on: safe vdev/develop/0f50bec, compared against homebrew safe v1.9.0
  • safe targets --json first reported strongbox in 4c68409 (v1.5.8)

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start at the Go Vault struct and its UnmarshalYAML method, then trace version handling and the configuration write path used by safe target. Reproduce both legacy YAML states with safe target --json; done means upgrades preserve legacy-enabled Strongbox targets and subsequent writes make the setting unambiguous.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
cli
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
50/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.