Skip to content

feat: add AES protected key interface and implementation #2599

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions lib/ocrypto/interfaces.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package ocrypto

import (
"context"
)

// Encapsulator interface for key encapsulation operations
type Encapsulator interface {
// Encrypt wraps a secret key with the encapsulation key
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow we don't have an Encapsulate

Suggested change
// Encrypt wraps a secret key with the encapsulation key
// Encapsulate wraps a secret key with the encapsulation key
Encapsulate(dek ProtectedKey) ([]byte, error)
// Encrypt encrypts extra data with the encapsulation key

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmihalcik-virtru Do we have a use for this today? I can add it and just leave it unimplemented for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather expose just Encapsulate than Encrypt if I had to choose

Encrypt(data []byte) ([]byte, error)

// PublicKeyInPemFormat Returns public key in pem format, or the empty string if not present
PublicKeyInPemFormat() (string, error)

// For EC schemes, this method returns the public part of the ephemeral key.
// Otherwise, it returns nil.
EphemeralKey() []byte
Comment on lines +15 to +17
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be replaced by an additional return value from encapsulate/encrypt perhaps? I really think we need to move the ek into the body of the wrappedKey array, since ML-KEM will generate a different value for each encapsulate operation IIRC. It is confusing that right now this and the PEM export method have the same implementation for EC operations, as well

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can leave this for now then remove / update in a future version of ocrypto

}

// ProtectedKey represents a decrypted key with operations that can be performed on it
type ProtectedKey interface {
// VerifyBinding checks if the policy binding matches the given policy data
VerifyBinding(ctx context.Context, policy, binding []byte) error

// Export returns the raw key data, optionally encrypting it with the provided encryptor
Export(encryptor Encapsulator) ([]byte, error)

// Used to decrypt encrypted policies and metadata
DecryptAESGCM(iv []byte, body []byte, tagSize int) ([]byte, error)
}
89 changes: 89 additions & 0 deletions lib/ocrypto/protected_key.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
package ocrypto

import (
"context"
"crypto/hmac"
"crypto/sha256"
"errors"
"fmt"
)

var (
// ErrEmptyKeyData is returned when the key data is empty
ErrEmptyKeyData = errors.New("key data is empty")
// ErrPolicyHMACMismatch is returned when policy binding verification fails
ErrPolicyHMACMismatch = errors.New("policy hmac mismatch")
)

// AESProtectedKey implements the ProtectedKey interface with an in-memory secret key
type AESProtectedKey struct {
rawKey []byte
aesGcm AesGcm
}
Comment on lines +19 to +22
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Storing the raw cryptographic key in a []byte field poses a security risk. The key will reside in plaintext in the application's memory, making it potentially vulnerable to memory-dumping attacks or other memory inspection vulnerabilities (e.g., side-channel attacks like Spectre/Meltdown).

While this may be an intentional design choice for performance, it's a significant security trade-off. For a library providing cryptographic primitives, it's crucial to be explicit about these risks.

Consider adding a prominent warning in the documentation for AESProtectedKey and NewAESProtectedKey about this behavior and the risks involved.


var _ ProtectedKey = (*AESProtectedKey)(nil)

// NewAESProtectedKey creates a new instance of AESProtectedKey
func NewAESProtectedKey(rawKey []byte) (*AESProtectedKey, error) {
if len(rawKey) == 0 {
return nil, ErrEmptyKeyData
}
// Create a defensive copy of the key
keyCopy := append([]byte{}, rawKey...)

// Pre-initialize the AES-GCM cipher for performance
aesGcm, err := NewAESGcm(keyCopy)
if err != nil {
return nil, fmt.Errorf("failed to initialize AES-GCM cipher: %w", err)
}

return &AESProtectedKey{
rawKey: keyCopy,
aesGcm: aesGcm,
}, nil
}

// DecryptAESGCM decrypts data using AES-GCM with the protected key
func (k *AESProtectedKey) DecryptAESGCM(iv []byte, body []byte, tagSize int) ([]byte, error) {
// Use the pre-initialized AES-GCM cipher for better performance
decryptedData, err := k.aesGcm.DecryptWithIVAndTagSize(iv, body, tagSize)
if err != nil {
return nil, fmt.Errorf("AES-GCM decryption failed: %w", err)
}

return decryptedData, nil
}

// Export returns the raw key data, optionally encrypting it with the provided Encapsulator
func (k *AESProtectedKey) Export(encapsulator Encapsulator) ([]byte, error) {
if encapsulator == nil {
// Return raw key data without encryption - caller should be aware of this
return append([]byte{}, k.rawKey...), nil
}
Comment on lines +58 to +62
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Allowing a nil encapsulator in the Export method, which then returns the raw, unencrypted key, creates a potential security vulnerability. It's easy for a developer to misuse this and accidentally expose the key. This is often referred to as a "footgun" API.

To make the API safer, I'd recommend disallowing a nil encapsulator. If the caller needs the raw key, they should be explicit about it, perhaps through a separate, clearly named method like Raw() []byte. This would make the intention much clearer and reduce the risk of accidental exposure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this version? My suggestion for exporting the key is either to pass in a 'no-op' encapsulator, or add an explicit Raw() method, requiring a down-cast to AESProtectedKey to use


// Encrypt the key data before returning
encryptedKey, err := encapsulator.Encrypt(k.rawKey)
if err != nil {
return nil, fmt.Errorf("failed to encrypt key data for export: %w", err)
}

return encryptedKey, nil
}

// VerifyBinding checks if the policy binding matches the given policy data
func (k *AESProtectedKey) VerifyBinding(_ context.Context, policy, policyBinding []byte) error {
actualHMAC := k.generateHMACDigest(policy)

if !hmac.Equal(actualHMAC, policyBinding) {
return ErrPolicyHMACMismatch
}

return nil
}

// generateHMACDigest is a helper to generate an HMAC digest from a message using the key
func (k *AESProtectedKey) generateHMACDigest(msg []byte) []byte {
mac := hmac.New(sha256.New, k.rawKey)
mac.Write(msg)
return mac.Sum(nil)
}
198 changes: 198 additions & 0 deletions lib/ocrypto/protected_key_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,198 @@
package ocrypto

import (
"context"
"crypto/rand"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestNewAESProtectedKey(t *testing.T) {
key := make([]byte, 32)
_, err := rand.Read(key)
require.NoError(t, err)

protectedKey, err := NewAESProtectedKey(key)
require.NoError(t, err)
assert.NotNil(t, protectedKey)
assert.Equal(t, key, protectedKey.rawKey)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Accessing the unexported field rawKey makes this test brittle to implementation changes. It's better to test the public behavior of the object. You can use Export(nil) to get the key and verify it's equal to the original key. This tests the public API rather than internal state.

exported, err := protectedKey.Export(nil)
require.NoError(t, err)
assert.Equal(t, key, exported)

}

func TestAESProtectedKey_DecryptAESGCM(t *testing.T) {
// Generate a random 256-bit key
key := make([]byte, 32)
_, err := rand.Read(key)
require.NoError(t, err)

protectedKey, err := NewAESProtectedKey(key)
require.NoError(t, err)

// Test data
plaintext := []byte("Hello, World!")

// Encrypt the data first using the same key
aesGcm, err := NewAESGcm(key)
require.NoError(t, err)

encrypted, err := aesGcm.Encrypt(plaintext)
require.NoError(t, err)

// Extract IV and ciphertext (first 12 bytes are IV for GCM standard nonce size)
iv := encrypted[:GcmStandardNonceSize]
ciphertext := encrypted[GcmStandardNonceSize:]

// Test decryption
decrypted, err := protectedKey.DecryptAESGCM(iv, ciphertext, 16) // 16 is standard GCM tag size
require.NoError(t, err)
assert.Equal(t, plaintext, decrypted)
}

func TestAESProtectedKey_DecryptAESGCM_InvalidKey(t *testing.T) {
// Empty key should fail
_, err := NewAESProtectedKey([]byte{})
require.Error(t, err)
assert.ErrorIs(t, err, ErrEmptyKeyData)
}

func TestAESProtectedKey_Export_NoEncapsulator(t *testing.T) {
key := []byte("test-key-12345678901234567890123") // 32 bytes
protectedKey, err := NewAESProtectedKey(key)
require.NoError(t, err)

exported, err := protectedKey.Export(nil)
require.NoError(t, err)
assert.Equal(t, key, exported)
}

func TestAESProtectedKey_Export_WithEncapsulator(t *testing.T) {
key := []byte("test-key-12345678901234567890123") // 32 bytes
protectedKey, err := NewAESProtectedKey(key)
require.NoError(t, err)

// Mock encapsulator
mockEncapsulator := &mockEncapsulator{
encryptFunc: func(data []byte) ([]byte, error) {
// Simple XOR encryption for testing
result := make([]byte, len(data))
for i, b := range data {
result[i] = b ^ 0xFF
}
return result, nil
},
}

exported, err := protectedKey.Export(mockEncapsulator)
require.NoError(t, err)

// Verify it was encrypted (should be different from original)
assert.NotEqual(t, key, exported)
assert.Len(t, exported, len(key))

// Verify we can decrypt it back
for i, b := range exported {
assert.Equal(t, key[i], b^0xFF)
}
}

func TestAESProtectedKey_Export_EncapsulatorError(t *testing.T) {
key := []byte("test-key-12345678901234567890123") // 32 bytes
protectedKey, err := NewAESProtectedKey(key)
require.NoError(t, err)

mockEncapsulator := &mockEncapsulator{
encryptFunc: func(_ []byte) ([]byte, error) {
return nil, assert.AnError
},
}

_, err = protectedKey.Export(mockEncapsulator)
require.Error(t, err)
assert.Contains(t, err.Error(), "failed to encrypt key data for export")
}

func TestAESProtectedKey_VerifyBinding(t *testing.T) {
key := []byte("test-key-12345678901234567890123") // 32 bytes
protectedKey, err := NewAESProtectedKey(key)
require.NoError(t, err)

policy := []byte("test-policy-data")
ctx := context.Background()

// Generate the expected HMAC
expectedHMAC := protectedKey.generateHMACDigest(policy)

// Verify binding should succeed with correct HMAC
err = protectedKey.VerifyBinding(ctx, policy, expectedHMAC)
assert.NoError(t, err)
}

func TestAESProtectedKey_VerifyBinding_Mismatch(t *testing.T) {
key := []byte("test-key-12345678901234567890123") // 32 bytes
protectedKey, err := NewAESProtectedKey(key)
require.NoError(t, err)

policy := []byte("test-policy-data")
wrongBinding := []byte("wrong-binding-data")
ctx := context.Background()

err = protectedKey.VerifyBinding(ctx, policy, wrongBinding)
require.Error(t, err)
assert.Equal(t, ErrPolicyHMACMismatch, err)
}

func TestAESProtectedKey_VerifyBinding_DifferentPolicyData(t *testing.T) {
key := []byte("test-key-12345678901234567890123") // 32 bytes
protectedKey, err := NewAESProtectedKey(key)
require.NoError(t, err)

ctx := context.Background()

// Generate HMAC for first policy
policy1 := []byte("policy-data-1")
hmac1 := protectedKey.generateHMACDigest(policy1)

// Try to verify with different policy data
policy2 := []byte("policy-data-2")
err = protectedKey.VerifyBinding(ctx, policy2, hmac1)
require.Error(t, err)
assert.Equal(t, ErrPolicyHMACMismatch, err)
}

func TestAESProtectedKey_InterfaceCompliance(t *testing.T) {
key := make([]byte, 32)
protectedKey, err := NewAESProtectedKey(key)
require.NoError(t, err)

// Ensure it implements the ProtectedKey interface
assert.Implements(t, (*ProtectedKey)(nil), protectedKey)
}

// Mock encapsulator for testing
type mockEncapsulator struct {
encryptFunc func([]byte) ([]byte, error)
publicKeyPEMFunc func() (string, error)
ephemeralKeyFunc func() []byte
}

func (m *mockEncapsulator) Encrypt(data []byte) ([]byte, error) {
if m.encryptFunc != nil {
return m.encryptFunc(data)
}
return data, nil
}

func (m *mockEncapsulator) PublicKeyInPemFormat() (string, error) {
if m.publicKeyPEMFunc != nil {
return m.publicKeyPEMFunc()
}
return "", nil
}

func (m *mockEncapsulator) EphemeralKey() []byte {
if m.ephemeralKeyFunc != nil {
return m.ephemeralKeyFunc()
}
return nil
}
Loading