feat: --append flag for easy CLI mTLS key rotation by BrageHK · Pull Request #36942 · vespa-engine/vespa

BrageHK · 2026-05-19T11:49:01Z

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

This change makes it easier to rotate credentials with --append flag, as the user does not have to manually create a credential for it to work, instead they can use Vespa CLI. Will update the documentation as well.

The --append flag does overwrite the current private key, but this should not be an issue as the new certificate is used anyways.

esolitos

Nice rotation flow — append-on-rotation + prune-on-cleanup maps well onto how operators actually do this. 👍

Most comments below are about making the API a bit more honest (the appendKey param doesn't actually append) and not swallowing filesystem errors. Two worth a real round-trip: the WritePrivateKeyFile param naming, and the implicit clients.pem overwrite during --prune.

Friendly reminder from the PR description: docs update still pending. 🙂

AI-assisted review · Findings and comments are verified and approved by a human reviewer before submission

esolitos · 2026-05-19T13:28:21Z

 // WritePrivateKeyFile writes the private key contained in this key pair to privateKeyFile.
-func (kp *PemKeyPair) WritePrivateKeyFile(privateKeyFile string, overwrite bool) error {
-	if ioutil.Exists(privateKeyFile) && !overwrite {
+func (kp *PemKeyPair) WritePrivateKeyFile(privateKeyFile string, overwrite bool, appendKey bool) error {


🤔 I find the appendKey parameter a bit confusing here. The function name and the symmetry with WriteCertificateFile(... appendCredentials) suggest it appends, but in practice it always overwrites — it only uses the flag to gate the "file must exist" precondition. The PR description acknowledges this too.

Two possible cleanups:

Rename the param so callers can't be misled, e.g. requireExisting bool, or

Drop the param entirely and have the caller in cert.go do the existence check + call WritePrivateKeyFile(path, true /*overwrite*/).

I'd lean toward option 2 — it keeps the crypto helper honest (it writes a private key, full stop) and pushes the rotation-specific logic up into doCert where the reader already has the context. WDYT?

esolitos · 2026-05-19T13:28:21Z

+}
+
+// CleanCertificateFile keeps only the first (newest) PEM certificate in certificateFile, removing any older ones.
+func CleanCertificateFile(certificateFile string) error {


🙂 Tiny consistency thing: the user-facing flag is --prune, the cobra helper is doCleanCert, and the exported function is CleanCertificateFile. Could we settle on one term — I'd suggest Prune everywhere since that's what shows up in --help and what users will grep for? E.g. PruneCertificateFile, doPruneCert. Pure naming nit, no behavior change.

Agree. This was silly of me. I first used the name --clean for the flag, but then realized that both -c and -C are already existing flags. Forgot to update when i refactored, but fixed it now 😅

esolitos · 2026-05-19T13:28:21Z

+	// Use a fresh CLI with the same homeDir to avoid -A flag state bleeding into --prune
+	cli2, stdout2, _ := newTestCLI(t, "VESPA_CLI_HOME="+cli.config.homeDir)


This comment "avoid -A flag state bleeding into --prune" is interesting — would you mind expanding it just a little? My read is that cobra's BoolVarP binds to a closure-captured variable that persists across cli.Run calls within the same test process (so appendCertificate=true from the previous call is still set when --prune is parsed). Harmless in production (one process per CLI invocation), but a future maintainer is going to look at this and think "why the second CLI? this looks dumb, let me simplify" 😅 — a one-liner explaining why would save them.

esolitos · 2026-05-19T13:28:21Z

+	if !skipApplicationPackage {
+		return doCertAdd(cli, true, false, args)
 	}


🤔 We confirm with the user before pruning their home-dir cert file, which is great — but then we also implicitly overwrite security/clients.pem in whatever app package the user happens to be in (via doCertAdd(cli, true, false, args)). That second write is silent.

Two options I can see:

Extend the warning text on the cli.printWarning line above to mention that this will also rewrite security/clients.pem in the current application package, so the confirmation covers both.

Or make the package update opt-in (let --no-add/-N do its usual job; pruning a local cert is independent from re-publishing the package).

Not blocking, but the current behavior surprised me on first read.

Co-authored-by: Marlon (Esolitos) Saglia <marlon@vespa.ai>

…spa into bragehk/rotate-mtls-cert

esolitos

Nice round 2 — all prior comments addressed cleanly, and the MarkFlagsMutuallyExclusive move is honestly a better answer than what I'd suggested. 👍

One real concern left (the cert/key write ordering can leave a corrupted state if -A is used with a missing key file), plus a couple of tiny cleanups.

AI-assisted review · Findings and comments are verified and approved by a human reviewer before submission

esolitos · 2026-05-20T06:46:52Z

+	if err := keyPair.WriteCertificateFile(certificateFile.path, overwriteCertificate, appendCertificate); err != nil {
 		return fmt.Errorf("could not write certificate: %w", err)
 	}
-	if err := keyPair.WritePrivateKeyFile(privateKeyFile.path, overwriteCertificate); err != nil {
+	if appendCertificate {
+		if _, err := os.Stat(privateKeyFile.path); err != nil {
+			if os.IsNotExist(err) {
+				return fmt.Errorf("private key file does not exist: %s", privateKeyFile.path)
+			}
+			return fmt.Errorf("could not stat private key file %s: %w", privateKeyFile.path, err)
+		}
+	}
+	if err := keyPair.WritePrivateKeyFile(privateKeyFile.path, overwriteCertificate || appendCertificate); err != nil {
 		return fmt.Errorf("could not write private key: %w", err)
 	}


🤔 I think there's a small partial-failure window here when --append is used:

WriteCertificateFile runs first and (on -A) appends [newCert, oldCert] to the cert file.

Then we os.Stat the key file.

Then we write the private key.

If step 2 fails — e.g. the user has manually deleted data-plane-private-key.pem or hit a permission error — the cert file is already mutated, but the key file is untouched (so it still holds oldKey). tls.X509KeyPair will then fail on the next CLI call because newCert's public key won't match oldKey. Worse, re-running vespa auth cert -A to recover appends another cert on top, growing the file unboundedly.

The cert-existence precondition for -A is already checked up-front at line 146, so I think the cleanest fix is to move the key-existence check up there too and keep the writes back-to-back:

Suggested change

if err := keyPair.WriteCertificateFile(certificateFile.path, overwriteCertificate, appendCertificate); err != nil {

return fmt.Errorf("could not write certificate: %w", err)

}

if err := keyPair.WritePrivateKeyFile(privateKeyFile.path, overwriteCertificate); err != nil {

if appendCertificate {

if _, err := os.Stat(privateKeyFile.path); err != nil {

if os.IsNotExist(err) {

return fmt.Errorf("private key file does not exist: %s", privateKeyFile.path)

}

return fmt.Errorf("could not stat private key file %s: %w", privateKeyFile.path, err)

}

}

if err := keyPair.WritePrivateKeyFile(privateKeyFile.path, overwriteCertificate || appendCertificate); err != nil {

return fmt.Errorf("could not write private key: %w", err)

}

if appendCertificate && !ioutil.Exists(certificateFile.path) {

return errHint(fmt.Errorf("no certificate found at '%s'", color.CyanString(certificateFile.path)),

"Run 'vespa auth cert' first to create an initial certificate")

}

if appendCertificate {

if _, err := os.Stat(privateKeyFile.path); err != nil {

if os.IsNotExist(err) {

return fmt.Errorf("private key file does not exist: %s", privateKeyFile.path)

}

return fmt.Errorf("could not stat private key file %s: %w", privateKeyFile.path, err)

}

}

keyPair, err := vespa.CreateKeyPair()

if err != nil {

return err

}

if err := keyPair.WriteCertificateFile(certificateFile.path, overwriteCertificate, appendCertificate); err != nil {

return fmt.Errorf("could not write certificate: %w", err)

}

if err := keyPair.WritePrivateKeyFile(privateKeyFile.path, overwriteCertificate || appendCertificate); err != nil {

return fmt.Errorf("could not write private key: %w", err)

}

(suggestion is for the relative position — you'll want to delete the existing appendCertificate && !ioutil.Exists(...) block above to avoid the duplicate)

esolitos · 2026-05-20T06:46:52Z

+	cmd.MarkFlagsMutuallyExclusive("force", "append")
+	cmd.MarkFlagsMutuallyExclusive("prune", "force", "append")


nit: the 3-way exclusion below already means "at most one of {prune, force, append}", so the 2-way ("force", "append") call is redundant. 🙂

Suggested change

cmd.MarkFlagsMutuallyExclusive("force", "append")

cmd.MarkFlagsMutuallyExclusive("prune", "force", "append")

cmd.MarkFlagsMutuallyExclusive("prune", "force", "append")

esolitos · 2026-05-20T06:46:52Z

+	assert.Equal(t, 3, countPEMBlocks(t, certFile))
+}
+
+func TestCertCleanNoExisting(t *testing.T) {


nit: small consistency thing — the rename from Clean to Prune didn't reach the test names. TestCertClean, TestCertCleanNoExisting, TestCertCleanInvalidFlagForce and TestCertCleanInvalidFlagAppend would all read more clearly as TestCertPrune*. Not blocking. 🙂

BrageHK added 4 commits May 19, 2026 11:13

feat: --append flag for easy CLI mTLS key rotation

7a952f4

feat: clean old certificates

4c459c8

Add warning + better test

f89d1ed

test checks if cert is the newest

2cb9dac

BrageHK requested a review from esolitos May 19, 2026 11:49

BrageHK mentioned this pull request May 19, 2026

Bragehk/rotate mtls credentials vespa-engine/documentation#4714

Open

esolitos reviewed May 19, 2026

View reviewed changes

BrageHK and others added 9 commits May 19, 2026 15:52

error convension

661bad8

Co-authored-by: Marlon (Esolitos) Saglia <marlon@vespa.ai>

Better error handling

9d7616b

Co-authored-by: Marlon (Esolitos) Saglia <marlon@vespa.ai>

Improve error msg

b9de38f

Co-authored-by: Marlon (Esolitos) Saglia <marlon@vespa.ai>

Hint user to use -f or -A

fe0961d

Co-authored-by: Marlon (Esolitos) Saglia <marlon@vespa.ai>

better test workaround explanation

4de12d6

rename variables to prune and small code improvement

9b763a5

Merge branch 'bragehk/rotate-mtls-cert' of github.com:vespa-engine/ve…

632556d

…spa into bragehk/rotate-mtls-cert

Remove redundant check

1a98c5b

err msg and code improvement

42e5edb

esolitos reviewed May 20, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: --append flag for easy CLI mTLS key rotation#36942

feat: --append flag for easy CLI mTLS key rotation#36942
BrageHK wants to merge 13 commits into
masterfrom
bragehk/rotate-mtls-cert

BrageHK commented May 19, 2026

Uh oh!

esolitos left a comment

Uh oh!

Uh oh!

esolitos May 19, 2026

Uh oh!

Uh oh!

Uh oh!

esolitos May 19, 2026

Uh oh!

BrageHK May 19, 2026

Uh oh!

Uh oh!

esolitos May 19, 2026

Uh oh!

esolitos May 19, 2026

Uh oh!

esolitos left a comment

Uh oh!

esolitos May 20, 2026

Uh oh!

esolitos May 20, 2026

Uh oh!

esolitos May 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		// Use a fresh CLI with the same homeDir to avoid -A flag state bleeding into --prune
		cli2, stdout2, _ := newTestCLI(t, "VESPA_CLI_HOME="+cli.config.homeDir)

		cmd.MarkFlagsMutuallyExclusive("force", "append")
		cmd.MarkFlagsMutuallyExclusive("prune", "force", "append")

Conversation

BrageHK commented May 19, 2026

Uh oh!

esolitos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esolitos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants