transitapi: add http handler enc/dec #1199

jmxnzo · 2025-01-31T16:03:13Z

This PR adds basic http handling functionality for the encrypt and decrypt endpoints of the transit engine API, allowing the auto-unsealing process for user-managed Vaults. Additionally a cyclic cryptographic unit test, testing the implemented handler functions for encrypt and decrypt was added.

ToDo's

add logic to extract name parameter of URL, specifying the workloadSecretID
authorize transit engine requests to name parameter with client cert policy hash
update seedengine logic
Implement Vault json error bodies

coordinator/internal/seedengine/seedengine.go

coordinator/internal/transitengine/crypto.go

coordinator/internal/transitengine/transitengine.go

coordinator/internal/transitengine/crypto.go

coordinator/internal/transitengine/crypto_test.go

coordinator/internal/transitengine/transitengine.go

coordinator/internal/transitengine/crypto.go

coordinator/internal/transitengine/crypto_test.go

coordinator/internal/transitengine/crypto.go

coordinator/internal/transitengine/transitengine.go

burgerdev · 2025-02-06T17:01:24Z

coordinator/main.go

+	eg.Go(func() error {
+		mux := transitengine.NewTransitEngineAPI(meshAuth, logger)
+		logger.Info("Transit Engine API initialized")
+		port := 8200
+		fmt.Printf("Serving transit engine API on port %d\n", port)
+		if err := http.ListenAndServe(fmt.Sprintf(":%d", port), mux); err != nil {
+			logger.Error("Failed to start transit engine API", "err", err)
+		}
+		return nil
+	})
+


This should only be enabled after authorization.

Of course, it was intended for testing purposes only and i'll drop this when squashing everything!

Let's leave the comment open until it's actually resolved.

coordinator/internal/transitengine/crypto.go

burgerdev

Thanks for the thorough review, @3u13r!

coordinator/internal/transitengine/crypto.go

coordinator/internal/transitengine/transitengine.go

burgerdev · 2025-02-17T10:49:16Z

coordinator/internal/transitengine/transitengine.go

+			http.Error(w, fmt.Sprint("URL version: %w", err), http.StatusBadRequest)
+			return
+		}
+		key, err := deriveEncryptionKey(authority, workloadSecretID+strVersion)


This potentially allows access to secrets for other workloads, because we can't tell the boundary between workloadSecretId and strVersion. E.g.:

workloadSecretId := `pod1` strVersion := `2` // vs workloadSecretId := `pod` strVersion := `12`

I think we should either derive twice (first for the name, then for the version), or define a separation character, ensure that the version does not contain that character and then derive for strVersion + sepChar + id (because the id does not have any character set limitations).

Thanks for pointing out and the thorough review! I changed this to your suggestion. StrVersion implicitly always was ensured to match ^v(\d+)$, therefore i added '_' as seperator and changed the order to mitigate the described attack possibilities.

But as described in the other comments, i dropped the strVersion completely as it does not specify the key_version as expected and used the key_version value instead. In the decryptionRequest, the version is still matched against the regex ^v(\d+)$, because it is part of the ciphertext field. And for the encryptionRequest the key_version value is autoparsed during json unmarshalling to int.

Can i mark this as resolved this with the updated separation logic? @burgerdev

coordinator/internal/transitengine/transitengine.go

coordinator/internal/transitengine/transitengine_test.go

burgerdev · 2025-02-17T11:26:45Z

coordinator/main.go

+	eg.Go(func() error {
+		mux := transitengine.NewTransitEngineAPI(meshAuth, logger)
+		logger.Info("Transit Engine API initialized")
+		port := 8200
+		fmt.Printf("Serving transit engine API on port %d\n", port)
+		if err := http.ListenAndServe(fmt.Sprintf(":%d", port), mux); err != nil {
+			logger.Error("Failed to start transit engine API", "err", err)
+		}
+		return nil
+	})
+


Let's leave the comment open until it's actually resolved.

This commit drops the version parsing of the URL, as it was refering to the API endpoint and not the key_version which was a wrong assumption. It introduces the key_version field to the encryption request and adds the necessary logic to the derivation, to ensure inputs are synthesized and seperated by disallowed characters. It reduces the (un)marshalling functions to only implement ciphertext container marshalling and instead rely on the starndard go library.

burgerdev · 2025-02-18T09:15:01Z

coordinator/internal/transitengine/crypto.go

+type (
+	// ciphertextContainer describes a base64-encoded ciphertext prepended with the nonce and specified key version.
+	ciphertextContainer struct {
+		nonce      []byte
+		ciphertext []byte
+		keyVersion uint
+	}
+)


Needs gofmt.

Sorry, I was expecting gofmt to convert this to

type ciphertextContainer struct { }

Please do that manually.

burgerdev · 2025-02-18T09:20:03Z

coordinator/internal/transitengine/transitengine.go

+	encodedNonce := b64.StdEncoding.EncodeToString(c.nonce)
+	encodedCiphertext := b64.StdEncoding.EncodeToString(c.ciphertext)
+	// Convert to "vault:vX:base64" format
+	versioned := fmt.Sprintf("vault:v%d:%s%s", c.keyVersion, encodedNonce, encodedCiphertext)


What happens if keyVersion==0, considering that Vault key versions start with 1?

Regarding https://developer.hashicorp.com/vault/api-docs/secret/transit#encrypt-data default of key_version is 0, i looked it up to be compatible.

Are you referring to key_version (int: 0)? I took that to mean

The default for key_version is 0, which means "use latest key".

which would be consistent with the example responses in the docs.

coordinator/internal/transitengine/transitengine.go

coordinator/internal/transitengine/transitengine_test.go

burgerdev

We're closing in!

burgerdev · 2025-02-18T15:31:17Z

coordinator/internal/transitengine/crypto.go

+type (
+	// ciphertextContainer describes a base64-encoded ciphertext prepended with the nonce and specified key version.
+	ciphertextContainer struct {
+		nonce      []byte
+		ciphertext []byte
+		keyVersion uint
+	}
+)


Sorry, I was expecting gofmt to convert this to

type ciphertextContainer struct { }

Please do that manually.

burgerdev · 2025-02-18T16:00:37Z

coordinator/internal/transitengine/transitengine.go

+// parseRequest parses the given HTTP request body into the struct, which should represent to the
+// received request body.


Suggested change

// parseRequest parses the given HTTP request body into the struct, which should represent to the

// received request body.

// parseRequest parses the given HTTP request body into the struct

burgerdev · 2025-02-18T16:04:45Z

coordinator/internal/transitengine/transitengine_test.go

+					data, exist := decRespBody["data"].(map[string]any)
+					require.True(exist)
+					receivedPlaintext, exist = data["plaintext"].(string)
+					require.True(exist)


These are not existence checks, but type cast checks. If the expected fields are not found, this will panic. You could avoid the type cast by assigning the expected type to begin with:

var decRespBody map[string]map[string]string

jmxnzo added no changelog PRs not listed in the release notes do not merge This shouldn't be merged at this point labels Jan 31, 2025

jmxnzo marked this pull request as ready for review January 31, 2025 16:28

jmxnzo requested a review from burgerdev as a code owner January 31, 2025 16:28

jmxnzo changed the title ~~transitapi: add hhtp handler enc/dec~~ transitapi: add http handler enc/dec Jan 31, 2025

3u13r requested changes Feb 1, 2025

View reviewed changes

jmxnzo force-pushed the http-handler/transit-engine-api branch 4 times, most recently from 2b3b3ce to 9cd6b17 Compare February 6, 2025 15:11

burgerdev reviewed Feb 6, 2025

View reviewed changes

jmxnzo force-pushed the http-handler/transit-engine-api branch from 8c98f39 to 26f2591 Compare February 10, 2025 20:02

coordinator: add getSeedEngine() of authority

c98fa29

jmxnzo force-pushed the http-handler/transit-engine-api branch from d12a895 to 031533a Compare February 11, 2025 13:48

3u13r requested changes Feb 12, 2025

View reviewed changes

coordinator/internal/transitengine/crypto.go Show resolved Hide resolved

burgerdev reviewed Feb 12, 2025

View reviewed changes

coordinator/internal/transitengine/crypto.go Show resolved Hide resolved

burgerdev reviewed Feb 17, 2025

View reviewed changes

jmxnzo added 4 commits February 17, 2025 17:02

transitapi: add enc/dec http handler

ef6fdff

coordinator: transit engine API

c86a9fd

transitapi: support optional parameters

05e5475

transitapi: key version as int

496f48c

jmxnzo force-pushed the http-handler/transit-engine-api branch 2 times, most recently from 8b6dbdb to 047b6bd Compare February 17, 2025 16:07

jmxnzo force-pushed the http-handler/transit-engine-api branch from 047b6bd to 5879177 Compare February 17, 2025 16:09

jmxnzo added 2 commits February 17, 2025 17:42

transitapi: check derived key length

d4c190a

transitapi: make keyversion uint

0ce5740

jmxnzo force-pushed the http-handler/transit-engine-api branch from 55d25fa to f2c3a9f Compare February 18, 2025 07:00

transitapi: rework unit test

c046718

burgerdev reviewed Feb 18, 2025

View reviewed changes

transitapi: add low-level crypto test

58df550

jmxnzo force-pushed the http-handler/transit-engine-api branch from f2c3a9f to 9c72d62 Compare February 18, 2025 10:30

fixup! transitapi: drop URL 'version' and (un) marshall function

029fcec

jmxnzo force-pushed the http-handler/transit-engine-api branch from 9c72d62 to 029fcec Compare February 18, 2025 10:59

fixup! transitapi: rework unit test

2645460

jmxnzo requested a review from burgerdev February 18, 2025 11:16

burgerdev reviewed Feb 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

transitapi: add http handler enc/dec #1199

transitapi: add http handler enc/dec #1199

jmxnzo commented Jan 31, 2025 •

edited

Loading

burgerdev Feb 6, 2025

jmxnzo Feb 10, 2025

burgerdev Feb 17, 2025

burgerdev left a comment

burgerdev Feb 17, 2025

jmxnzo Feb 17, 2025

jmxnzo Feb 17, 2025 •

edited

Loading

jmxnzo Feb 18, 2025 •

edited

Loading

burgerdev Feb 17, 2025

burgerdev Feb 18, 2025

burgerdev Feb 18, 2025

burgerdev Feb 18, 2025

jmxnzo Feb 18, 2025

burgerdev Feb 18, 2025

burgerdev left a comment

burgerdev Feb 18, 2025

burgerdev Feb 18, 2025

burgerdev Feb 18, 2025

		// parseRequest parses the given HTTP request body into the struct, which should represent to the
		// received request body.

transitapi: add http handler enc/dec #1199

Are you sure you want to change the base?

transitapi: add http handler enc/dec #1199

Conversation

jmxnzo commented Jan 31, 2025 • edited Loading

ToDo's

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burgerdev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmxnzo Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

jmxnzo Feb 18, 2025 • edited Loading

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

burgerdev 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

jmxnzo commented Jan 31, 2025 •

edited

Loading

jmxnzo Feb 17, 2025 •

edited

Loading

jmxnzo Feb 18, 2025 •

edited

Loading