transitapi: add http handler enc/dec #1199
Conversation
jmxnzo
added
no changelog
jmxnzo
changed the title
| @@ -19,6 +19,11 @@ import ( | |||
| "golang.org/x/crypto/hkdf" | |||
| ) | |||
|
|
|||
| // GetterSeedEngine describes a getter function to return a SeedEngine. | |||
| type GetterSeedEngine interface { | |||
There was a problem hiding this comment.
Interfaces should be defined in the package they are used.
| symmetricOpts symmetricOpts | ||
| } | ||
|
|
||
| // Note: In case only err is returned the resulting httpError will fallbback to InternalServerError status code 500. |
There was a problem hiding this comment.
Could you elaborate a bit more? Since this is not a function documentation, this floats a bit between the functions/code? Is is related to the function below?
| var status int | ||
| var httpError interface { | ||
| error | ||
| HTTPStatus() int | ||
| } | ||
| if errors.As(err, &httpError) { | ||
| status = httpError.HTTPStatus() | ||
| if status == 0 { | ||
| status = http.StatusInternalServerError | ||
| } | ||
| } else { | ||
| status = http.StatusInternalServerError | ||
| } | ||
| http.Error(w, err.Error(), status) |
There was a problem hiding this comment.
Since we also have w in the cryptFunc we can write the status code there. Though I'd like to go in the direction that the cryptFunc is a pure function that doesn't know about being in a http request, i.e., remove w and r from the arguments. Though I haven't found a good solution for that.
|
|
||
| encryptionMapping.symmetricOpts = symmetricOpts{ | ||
| convergent: false, | ||
| additionalData: []byte("test"), |
| if err != nil { | ||
| return nil, err | ||
| } | ||
| gcm, err := cipher.NewGCM(aesCipher) |
There was a problem hiding this comment.
Using the raw Go crypto libs feels a bit sharp. I think that tink doesn't have good support for this use-case. Have we checked that there isn't a good library for this?
| if err != nil { | ||
| return httpError{error: err, status: http.StatusBadRequest} | ||
| } | ||
| encryptionMapping.symmetricOpts.nonce = ciphertextBytes[:12] |
There was a problem hiding this comment.
Those inline numbers should definitely be constants, since
- The name documents the usage
- this makes sure that we keep using the same number in multiple places.
| "bytes" | ||
| "encoding/json" | ||
| "io" | ||
| r "math/rand" |
There was a problem hiding this comment.
Why naming the import here? Also single letter variables are usually reserved for either function receivers are variables with very limited scope
| }) | ||
| t.Run("decryption request handling", func(t *testing.T) { | ||
| decryptReqBody, err := createReqBodyJSON("ciphertext", ciphertext) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Instead of using require.NoError(t, err) we call require := require.New(t) in the first lines of a new test and then call require.NoError(err).
|
|
||
| func TestCryptoAPICyclic(t *testing.T) { | ||
| t.Run("encrypt-decrypt handler", func(t *testing.T) { | ||
| for range 10 { |
There was a problem hiding this comment.
What is the idea of calling the test 10 times? You can also run go tests multiple times by specifying -count 1000. Generally, using randomness in tests is also an anti-pattern, instead create crypto related things (keys etc) beforehand and use them as constants.
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