bulk up unit test coverage

internal/crypto/algorithms/aes256cfb.go

@@ -18,6 +18,7 @@ import (
 	"crypto/aes"
 	"crypto/cipher"
 	"crypto/rand"
+	"fmt"
 	"io"
 
 	"golang.org/x/crypto/argon2"
@@ -58,6 +59,10 @@ func (a *AES256CFBAlgorithm) Decrypt(ciphertext []byte, key []byte, salt []byte)
 		return nil, status.Errorf(codes.Unknown, "failed to create cipher: %s", err)
 	}
 
+	if len(ciphertext) < aes.BlockSize {
+		return nil, fmt.Errorf("ciphertext too short to decrypt, length is: %d", len(ciphertext))
+	}
+
 	// The IV needs to be extracted from the ciphertext.
 	iv := ciphertext[:aes.BlockSize]
 	ciphertext = ciphertext[aes.BlockSize:]

internal/crypto/engine.go

@@ -190,6 +190,10 @@ func (e *engine) decrypt(data EncryptedData) ([]byte, error) {
 		return nil, errors.New("cannot decrypt empty data")
 	}
 
+	if len(data.Salt) == 0 {
+		return nil, errors.New("cannot decrypt data with empty salt")
+	}
+
 	algorithm, ok := e.supportedAlgorithms[data.Algorithm]
 	if !ok {
 		return nil, fmt.Errorf("%w: %s", algorithms.ErrUnknownAlgorithm, e.defaultAlgorithm)

internal/crypto/engine_test.go

@@ -25,6 +25,7 @@ import (
 	"google.golang.org/grpc/status"
 
 	"github.com/stacklok/minder/internal/config/server"
+	"github.com/stacklok/minder/internal/crypto/algorithms"
 )
 
 //Test both the algorithm and the engine in one test suite
@@ -79,6 +80,86 @@ func TestEncryptDecryptOAuthToken(t *testing.T) {
 	require.Equal(t, oauthToken, decrypted)
 }
 
+func TestDecryptEmpty(t *testing.T) {
+	t.Parallel()
+
+	engine, err := NewEngineFromConfig(config)
+	require.NoError(t, err)
+	encryptedToken := EncryptedData{
+		EncodedData: "",
+	}
+
+	_, err = engine.DecryptString(encryptedToken)
+	require.ErrorContains(t, err, "cannot decrypt empty data")
+}
+
+func TestDecryptEmptySalt(t *testing.T) {
+	t.Parallel()
+
+	engine, err := NewEngineFromConfig(config)
+	require.NoError(t, err)
+	encryptedToken := EncryptedData{
+		EncodedData: "abc",
+		Salt:        nil,
+	}
+
+	_, err = engine.DecryptString(encryptedToken)
+	require.ErrorContains(t, err, "cannot decrypt data with empty salt")
+}
+
+func TestDecryptBadAlgorithm(t *testing.T) {
+	t.Parallel()
+
+	engine, err := NewEngineFromConfig(config)
+	require.NoError(t, err)
+	encryptedToken := EncryptedData{
+		Algorithm:   "I'm a little teapot",
+		EncodedData: "abc",
+		Salt:        legacySalt,
+		KeyVersion:  "",
+	}
+	require.NoError(t, err)
+
+	_, err = engine.DecryptString(encryptedToken)
+	require.ErrorIs(t, err, algorithms.ErrUnknownAlgorithm)
+}
+
+func TestDecryptBadEncoding(t *testing.T) {
+	t.Parallel()
+
+	engine, err := NewEngineFromConfig(config)
+	require.NoError(t, err)
+	encryptedToken := EncryptedData{
+		Algorithm: algorithms.Aes256Cfb,
+		// Unicode snowman is _not_ a valid base64 character
+		EncodedData: "☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃",
+		Salt:        legacySalt,
+		KeyVersion:  "",
+	}
+	require.NoError(t, err)
+
+	_, err = engine.DecryptString(encryptedToken)
+	require.ErrorContains(t, err, "error decoding secret")
+}
+
+func TestDecryptFailedDecryption(t *testing.T) {
+	t.Parallel()
+
+	engine, err := NewEngineFromConfig(config)
+	require.NoError(t, err)
+	encryptedToken := EncryptedData{
+		Algorithm: algorithms.Aes256Cfb,
+		// too small of a value - will trigger the ciphertext length check
+		EncodedData: "abcdef0123456789",
+		Salt:        legacySalt,
+		KeyVersion:  "",
+	}
+	require.NoError(t, err)
+
+	_, err = engine.DecryptString(encryptedToken)
+	require.ErrorIs(t, err, ErrDecrypt)
+}
+
 var config = &server.Config{
 	Auth: server.AuthConfig{
 		TokenKey: "./testdata/test_encryption_key",