Add different run modes for the TUF server, allow saving TUF keys as a secret #1214

bkabrda · 2024-08-08T09:02:20Z

Summary

Addresses part of #1182.

This commit implements features necessary to run/operate the TUF server in production much better:

The TUF server can now be run in 4 different modes:
- init - only init the TUF repository and exit
- init-no-overwrite - same as init, but won't overwrite the TUF repository if it already exists
- serve - only serve an existing TUF repository (no init)
- init-and-serve - init and then serve (this is default to preserve backwards compatibility)
The TUF repository can now be initialized/served from a given path, as opposed to always living in a /tmp directory.
The TUF keys can optionally be exported as an individual k8s secret.

Some notes:

The idea behind this PR is that:
- There is a k8s Job/initContainer that runs the server with either init or init-no-overwrite to create the TUF repository at a mounted volume.
- TUF server Deployment starts using only serve to serve the TUF repository from the mounted volume.
- TUF keys are saved in the secret, so the admin of the system can download them and keep operating the TUF repository - they do so by updating TUF files localy and placing them in the volume.
The PR could optionally be split into multiple PRs. I think this functionality is interconnected and hence makes sense to submit as a single PR, but let me know if you want me to split it into multiple ones.
The choice to save the TUF keys into an individual secret (as opposed to adding them to the one that is already created) is kind of arbitrary. We could very well put everything in one secret, however I think having the option to delete the secret with keys (after it's downloaded by the admin of the system) and keeping the one with the repository is good to have.

Release Note

The TUF server was modified to be run in 4 different modes:
- init - only init the TUF repository and exit
- init-no-overwrite - same as init, but won't overwrite the TUF repository if it already exists
- serve - only serve an existing TUF repository (no init)
- init-and-serve - init and then serve (this is default to preserve backwards compatibility)
The TUF repository was made to be initialized/served from a given path, as opposed to always living in a /tmp directory.
Option to export the TUF keys an individual k8s secret was added.

Documentation

It feels like this change would deserve some docs, but I don't know if there even are docs for the TUF server. I can certainly write something up if you can point me to the right place.

…a secret This commit implements features necessary to run/operate the TUF server in production much better: * The TUF server can now be run in 4 different modes: * `init` - only init the TUF repository and exit * `init-no-overwrite` - same as `init`, but won't overwrite the TUF repository if it already exists * `serve` - only serve an existing TUF repository (no `init`) * `init-and-serve` - `init` and then `serve` * The TUF repository can now be initialized/served from a given path, as opposed to always living in a `/tmp` directory. * The TUF keys can optionally be exported as an individual k8s secret. Signed-off-by: Slavek Kabrda <[email protected]>

Signed-off-by: Slavek Kabrda <[email protected]>

jku

I think this makes sense: it makes it clearer that this is really just file serving code with a single function to (now optionally) initialize that file server with some generated TUF content.

I don't comment on the secrets management: I don't know this technology at all

evankanderson

@haydentherapper asked me to look at this. The k8s part makes sense, but I had a few questions about the general patterns since I'm not as familiar with the codebase.

Splitting init and serve makes a lot of sense to me. I am wondering about the "saving the keys" vs simply pulling the keys out of the existing saved secret.

evankanderson · 2024-08-12T21:28:53Z

cmd/tuf/server/main.go

+const (
+	modeInit            = "init"
+	modeInitAndServe    = "init-and-serve"
+	modeServe           = "serve"
+	modeInitNoOverwrite = "init-no-overwrite"
+)


If we were using Cobra, I'd suggest using subcommands, but I don't want to pull in that dependency randomly.

evankanderson · 2024-08-12T21:30:03Z

cmd/tuf/server/main.go

 	// Name of the "secret" where we create two entries, one for:
 	// root = Which holds 1.root.json
 	// repository - Compressed repo, which has been tar/gzipped.
 	secretName = flag.String("rootsecret", "tuf-root", "Name of the secret to create for the initial root file")
-	noK8s = flag.Bool("no-k8s", false, "Run in a non-k8s environment")
+	// Name of the "secret" where we create one entry "keys" which is a compressed directory of private keys generated by TUF


Suggested change

// Name of the "secret" where we create one entry "keys" which is a compressed directory of private keys generated by TUF

// Name of the "secret" where we create one entry "keys" which is a tar/gzipped directory of private keys generated by TUF

(We should be clear about the format)

Oh, this comment is actually no longer true. It was true in the first version of my code, but now it is not tar/gzipped, but it's literally a secret with key/value pairs for the 4 individual keys. I will update the comment.

evankanderson · 2024-08-12T21:42:19Z

cmd/tuf/server/main.go

+	case modeInitAndServe:
+		init = true
+		serve = true


I'm assuming there's no use case for InitNoOverwriteAndServe?

I personally don't have such a usecase right now and I always try to keep new stuff at minimum. If we ever see a need for it in the future, it will be easy to add it in a fully backwards compatible way.

evankanderson · 2024-08-12T21:49:01Z

cmd/tuf/server/main.go

-
-	ctx := signals.NewContext()
-
+func initTUFRepo(ctx context.Context, certsDir, targetDir, repoSecretName, keysSecretName string) error {


Extracting this is a nice win anyway, though it's unfortunate that we have 4 string-type arguments next to each other. Oh well.

evankanderson · 2024-08-12T21:52:40Z

cmd/tuf/server/main.go

-	trimDir := strings.TrimSuffix(*dir, "/")
+	trimDir := strings.TrimSuffix(certsDir, "/")


Not your fault, but it feels like we should move trimDir above line 88 (os.ReadDir) and use it consistently from that point onwards.

Sounds good, I can do that.

evankanderson · 2024-08-12T21:53:45Z

cmd/tuf/server/main.go

@@ -95,7 +103,7 @@ func main() {
 			fileName := fmt.Sprintf("%s/%s", trimDir, file.Name())
 			fileBytes, err := os.ReadFile(fileName)
 			if err != nil {
-				logging.FromContext(ctx).Fatalf("failed to read file %s/%s: %v", fileName, err)
+				return fmt.Errorf("failed to read file %s/%s: %v", trimDir, fileName, err)


fileName already contains trimDir -- this will look like:

failed to read file /tmp/tmp/foo: bad link

or what-not.

evankanderson · 2024-08-12T21:53:56Z

cmd/tuf/server/main.go

@@ -104,7 +112,7 @@ func main() {

 				certFiles, err := certs.SplitCertChain(fileBytes, "tsa")
 				if err != nil {
-					logging.FromContext(ctx).Fatalf("failed to parse  %s/%s: %v", fileName, err)
+					return fmt.Errorf("failed to parse  %s/%s: %v", trimDir, fileName, err)


evankanderson · 2024-08-12T22:35:23Z

cmd/tuf/server/main.go

+
+		// If we should also store created keys in a secret, read all their files and save them in the secret
+		if keysSecretName != "" {
+			keyFiles, err := os.ReadDir(filepath.Join(dir, "keys"))


Didn't we pack the keys directory into data["repository"] already, along with the rest of dir?

We didn't. If you look at how this is done:

if err := repo.CompressFS(os.DirFS(dir), &compressed, map[string]bool{"keys": true, "staged": true}); err != nil

and how the CompressFS function works, it will actually intentionally skip the keys and staged directories because of how it is called.

evankanderson · 2024-08-12T22:42:04Z

cmd/tuf/server/main.go

+
+	// Copy repository to the targetDir - until Go 1.23 which has os.CopyFS, we use
+	// a quick hack where we uncompress the compressed repository to the targetDir
+	repo.Uncompress(bytes.NewReader(data["repository"]), targetDir)


Question: Right now, it seems like we're creating in a temp directory and then moving to the final destination. What about having the repo.CreateRepo take targetDir as an argument, and not needing to do this unpack?

At that point, the code could also look like:

if noK8s { // We're done return nil }

I suppose one advantage of the copy approach is that you won't end up with a half-initialized directory if you crash partway through....

There were two reasons why I didn't add the extra argument, even though we considered it:

Exactly as you said, I think it's good to not end up with a potentially half-initialized directory.

Adding the new argument would change the public API of repo.CreateRepo. While I think that the modules from this repo might not be depended on very much, I wanted to prevent API breakage (but actually talking about this right now, I'm curious if that's ok - for future contributions).

Signed-off-by: Slavek Kabrda <[email protected]>

bkabrda · 2024-08-13T07:42:30Z

@evankanderson thanks a lot for your review! I think I addressed all the points. As noted in one of the inline comments, the currently saved secret intentionally explicitly excludes the keys directory from being saved. I'm not sure why it was coded this way, but I think it makes sense that these two are independent and the keys secret can be downloaded and deleted altogether. (I don't think this is a huge benefit, but it felt to me that it might be nice, YMMV of course).

…a secret (sigstore#1214) * Add different run modes for the TUF server, allow saving TUF keys as a secret This commit implements features necessary to run/operate the TUF server in production much better: * The TUF server can now be run in 4 different modes: * `init` - only init the TUF repository and exit * `init-no-overwrite` - same as `init`, but won't overwrite the TUF repository if it already exists * `serve` - only serve an existing TUF repository (no `init`) * `init-and-serve` - `init` and then `serve` * The TUF repository can now be initialized/served from a given path, as opposed to always living in a `/tmp` directory. * The TUF keys can optionally be exported as an individual k8s secret. Signed-off-by: Slavek Kabrda <[email protected]>

haydentherapper requested a review from jku August 8, 2024 21:34

bkabrda added 2 commits August 9, 2024 11:00

Provide TUF keys as individual values in the secret

7553ef3

Signed-off-by: Slavek Kabrda <[email protected]>

Fix comment

f5e033c

Signed-off-by: Slavek Kabrda <[email protected]>

bkabrda force-pushed the tuf-server-modes branch from 637879b to f5e033c Compare August 9, 2024 09:00

jku approved these changes Aug 12, 2024

View reviewed changes

evankanderson reviewed Aug 12, 2024

View reviewed changes

Address review

88ac0e1

Signed-off-by: Slavek Kabrda <[email protected]>

bkabrda force-pushed the tuf-server-modes branch from e5da94f to 88ac0e1 Compare August 13, 2024 07:40

evankanderson approved these changes Aug 19, 2024

View reviewed changes

evankanderson merged commit 3dcbf17 into sigstore:main Aug 19, 2024
22 checks passed

bouskaJ mentioned this pull request Aug 19, 2024

Add different run modes for the TUF server, allow saving TUF keys as … securesign/scaffolding#283

Merged

This was referenced Aug 22, 2024

Question: What would we expect whenever the server restarts ? It is okay to recreate the repository again and again. #265

Closed

Make the TUF server init a consistent_snapshot repository by default #1236

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add different run modes for the TUF server, allow saving TUF keys as a secret #1214

Add different run modes for the TUF server, allow saving TUF keys as a secret #1214

bkabrda commented Aug 8, 2024

jku left a comment

evankanderson left a comment

evankanderson Aug 12, 2024

evankanderson Aug 12, 2024

evankanderson Aug 12, 2024

bkabrda Aug 13, 2024

evankanderson Aug 12, 2024

bkabrda Aug 13, 2024

evankanderson Aug 12, 2024

evankanderson Aug 12, 2024

bkabrda Aug 13, 2024

evankanderson Aug 12, 2024

evankanderson Aug 12, 2024

evankanderson Aug 12, 2024

bkabrda Aug 13, 2024

evankanderson Aug 12, 2024

bkabrda Aug 13, 2024

bkabrda commented Aug 13, 2024

	// Name of the "secret" where we create one entry "keys" which is a compressed directory of private keys generated by TUF
	// Name of the "secret" where we create one entry "keys" which is a tar/gzipped directory of private keys generated by TUF


		ctx := signals.NewContext()

		func initTUFRepo(ctx context.Context, certsDir, targetDir, repoSecretName, keysSecretName string) error {

		trimDir := strings.TrimSuffix(*dir, "/")
		trimDir := strings.TrimSuffix(certsDir, "/")

Add different run modes for the TUF server, allow saving TUF keys as a secret #1214

Add different run modes for the TUF server, allow saving TUF keys as a secret #1214

Conversation

bkabrda commented Aug 8, 2024

Summary

Release Note

Documentation

jku left a comment

Choose a reason for hiding this comment

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkabrda commented Aug 13, 2024