feat: add AES protected key interface and implementation #2599

strantalis · 2025-07-29T20:22:00Z

Proposed Changes

Add ProtectedKey and Encapsulator interfaces to lib/ocrypto package, along with AESProtectedKey implementation. This exposes previously internal cryptographic functionality for external consumption.

Add interfaces.go with ProtectedKey and Encapsulator interfaces
Add protected_key.go with AESProtectedKey implementation
Add comprehensive test suite in protected_key_test.go
Use standard error types without logging dependencies

Checklist

I have added or updated unit tests
I have added or updated integration tests (if appropriate)
I have added or updated documentation

Testing Instructions

Add ProtectedKey and Encapsulator interfaces to lib/ocrypto package, along with AESProtectedKey implementation. This exposes previously internal cryptographic functionality for external consumption. - Add interfaces.go with ProtectedKey and Encapsulator interfaces - Add protected_key.go with AESProtectedKey implementation - Add comprehensive test suite in protected_key_test.go - Use standard error types without logging dependencies

gemini-code-assist

Summary of Changes

Hello @strantalis, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request aims to expose and standardize key management operations within the lib/ocrypto package by introducing new interfaces and a concrete AES-based implementation. This change modularizes cryptographic functionality, allowing for broader and more consistent consumption of protected keys across the system.

Highlights

New Interfaces: I've introduced ProtectedKey and Encapsulator interfaces in lib/ocrypto/interfaces.go. These interfaces define standard operations for managing and encapsulating cryptographic keys, making previously internal functionality accessible.
AESProtectedKey Implementation: A concrete implementation, AESProtectedKey, has been added in lib/ocrypto/protected_key.go. This struct implements the ProtectedKey interface, providing methods for AES-GCM decryption, secure key export (with optional encapsulation), and policy binding verification using HMAC-SHA256.
Comprehensive Testing: I've included a thorough test suite in lib/ocrypto/protected_key_test.go. These tests cover the AESProtectedKey's core functionalities, various error conditions, and ensure its compliance with the ProtectedKey interface, including the use of a mock encapsulator for isolated testing.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

New crypto flows, Keys protected, safe and sound, Code now open wide.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces a new ProtectedKey interface and its AESProtectedKey implementation for cryptographic operations. The changes are well-structured and include a comprehensive test suite.

My review focuses on improving security and code clarity. Key suggestions include:

Preventing slice aliasing in the constructor and Export method to enhance security by copying the key data.
Refactoring generateHMACDigest to remove an unused context parameter and a redundant error check.

lib/ocrypto/protected_key.go

- Add defensive copying in constructor to prevent external mutation - Add defensive copying in Export method for security - Simplify generateHMACDigest by removing unused context and error handling - Update tests to match simplified signature Addresses feedback from code review on PR opentdf#2599

strantalis · 2025-07-29T22:03:56Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces new interfaces for cryptographic operations and an AESProtectedKey implementation. The code is well-structured and includes a comprehensive test suite. The review focuses on improving performance by suggesting to initialize the AES cipher once in the constructor and to correct the AES key sizes in the tests.

lib/ocrypto/protected_key.go

lib/ocrypto/protected_key_test.go

lib/ocrypto/protected_key.go

strantalis · 2025-07-29T22:44:08Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces new interfaces (Encapsulator, ProtectedKey) and an implementation (AESProtectedKey) for cryptographic operations, exposing them for external use. The changes are well-structured and include a comprehensive test suite.

My review focuses on improving API clarity, consistency, and addressing potential security risks. Key points include:

Refining the new interfaces for better naming and documentation, following Go conventions.
Highlighting two significant security considerations in AESProtectedKey: the storage of raw keys in memory and the ability to export them unencrypted. These create potential "footgun" scenarios that could lead to accidental key exposure.
Suggesting improvements to the test suite for better maintainability and to test against the public API rather than internal state.

Overall, the PR is a good addition, but the security implications should be carefully considered and addressed.

gemini-code-assist · 2025-07-29T22:46:32Z

lib/ocrypto/protected_key.go

+type AESProtectedKey struct {
+	rawKey []byte
+	aesGcm AesGcm
+}


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.

gemini-code-assist · 2025-07-29T22:46:32Z

lib/ocrypto/protected_key.go

+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
+	}


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.

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

lib/ocrypto/interfaces.go

lib/ocrypto/protected_key.go

gemini-code-assist · 2025-07-29T22:46:33Z

lib/ocrypto/protected_key_test.go

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


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)

lib/ocrypto/protected_key_test.go

dmihalcik-virtru · 2025-07-30T15:06:21Z

lib/ocrypto/interfaces.go

+
+// Encapsulator interface for key encapsulation operations
+type Encapsulator interface {
+	// Encrypt wraps a secret key with the encapsulation key


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

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

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

lib/ocrypto/interfaces.go

dmihalcik-virtru · 2025-07-30T15:41:19Z

lib/ocrypto/interfaces.go

+	// For EC schemes, this method returns the public part of the ephemeral key.
+	// Otherwise, it returns nil.
+	EphemeralKey() []byte


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

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

dmihalcik-virtru · 2025-08-07T18:19:09Z

lib/ocrypto/protected_key.go

+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
+	}


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

strantalis requested a review from a team as a code owner July 29, 2025 20:22

github-actions bot added comp:lib:ocrypto size/m labels Jul 29, 2025

gemini-code-assist bot reviewed Jul 29, 2025

View reviewed changes

lib/ocrypto/protected_key.go Outdated Show resolved Hide resolved

lib/ocrypto/protected_key.go Outdated Show resolved Hide resolved

lib/ocrypto/protected_key.go Outdated Show resolved Hide resolved

strantalis changed the title ~~feat(ocrypto): add AES protected key interface and implementation~~ feat: add AES protected key interface and implementation Jul 29, 2025

strantalis added 2 commits July 29, 2025 17:41

cleanup lint issues

119dbc9

gemini-code-assist bot reviewed Jul 29, 2025

View reviewed changes

lib/ocrypto/protected_key.go Outdated Show resolved Hide resolved

lib/ocrypto/protected_key_test.go Outdated Show resolved Hide resolved

lib/ocrypto/protected_key.go Outdated Show resolved Hide resolved

gemini performance improvements recommendations

227c2aa

strantalis mentioned this pull request Jul 29, 2025

feat: use public AES protected key from lib/ocrypto #2600

Open

3 tasks

gemini-code-assist bot reviewed Jul 29, 2025

View reviewed changes

jentfoo approved these changes Jul 30, 2025

View reviewed changes

dmihalcik-virtru reviewed Jul 30, 2025

View reviewed changes

strantalis requested a review from dmihalcik-virtru July 30, 2025 16:25

strantalis enabled auto-merge July 31, 2025 14:31

dmihalcik-virtru requested changes Aug 7, 2025

View reviewed changes

-	// 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

feat: add AES protected key interface and implementation #2599

Are you sure you want to change the base?

feat: add AES protected key interface and implementation #2599

Uh oh!

Conversation

strantalis commented Jul 29, 2025

Proposed Changes

Checklist

Testing Instructions

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

strantalis commented Jul 29, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

strantalis commented Jul 29, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

dmihalcik-virtru Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist bot Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dmihalcik-virtru Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

strantalis Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

dmihalcik-virtru Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dmihalcik-virtru Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

strantalis Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

dmihalcik-virtru Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!