[KCC] Add generate_kem_keypair and generate_binding_keypair FFIs by meetrajvala · Pull Request #633 · google/go-tpm-tools

meetrajvala · 2026-01-31T09:36:10Z

No description provided.

atulpatildbz

On-going review. Will continue tomorrow. Meanwhile you can start working on these comments.

kcc/src/lib.rs

kcc/src/types.rs

atulpatildbz · 2026-02-02T17:27:32Z

kcc/src/vault.rs

+#[derive(ZeroizeOnDrop)]
+pub struct Vault {
+    mmap: MmapMut,
+}


MmapMut doesn't implement Zeroize it seems. https://docs.rs/memmap2/latest/memmap2/struct.MmapMut.html#trait-implementations

Implement Drop manually to zero the mmap contents via self.mmap[..].zeroize() before unmapping.

atulpatildbz · 2026-02-02T18:12:36Z

kcc/src/vault.rs

+        let name_cstr = CString::new(name)
+            .map_err(|_| IoError::new(std::io::ErrorKind::InvalidInput, "Invalid name"))?;
+        let fd = unsafe {
+            libc::syscall(libc::SYS_memfd_create, name_cstr.as_ptr(), MFD_CLOEXEC | MFD_SECRET)


MFD_SECRET isn't a valid flag for memfd_create. https://man7.org/linux/man-pages/man2/memfd_create.2.html

Call the actual memfd_secret syscall . (man page reference)

Use ftruncate to set the size.

Use mmap to write the data (since memfd_secret fds don't support write()).

Yeah I think I wrote MFD_SECRET in my doc. It should be MFD_NOEXEC_SEAL instead. We should add it to prevent this memory from ever being executable.

Regarding using ftruncate, while it is a valid method. A memory write() should have similar effects. As the man-page mentions:

The initial size of the file is set to 0. Following the call, the
file size should be set using ftruncate(2). (Alternatively, the
file may be populated by calls to write(2) or similar.)

So I think the file.write_all(data)?; below should be good enough to set the file size.

Regarding write() not being supported, I am not sure why that will be. Unless we add F_SEAL_WRITE we should be able to write to the fd location similar to memfd backed files.

atulpatildbz · 2026-02-02T18:14:28Z

kcc/src/vault.rs

+
+// From <linux/memfd.h>
+const MFD_CLOEXEC: u32 = 0x0001;
+const MFD_SECRET: u32 = 0x0008;


in linux/memfd.h 0x0008 points to MFD_NOEXEC_SEAL

LMK if i'm missing something

NilanjanDaw

Thanks for working on this, added some initial comments.

NilanjanDaw · 2026-02-02T19:08:50Z

kcc/src/lib.rs

+}
+
+pub struct KeyManager {
+    pub binding_keys: KeyRegistry,


As discussed today, we need to separate these out into individual Key Management Agents, one for the Workload daemon, and the other for the Key Protection Service.

NilanjanDaw · 2026-02-02T19:10:04Z

kcc/src/lib.rs

+            let meta = KeyMetadata {
+                id: key_id,
+                created_at: Instant::now(),
+                delete_after: Some(Instant::now() + Duration::from_secs(3600)),


The binding key lifespan is a parameter passed down from the orchestration layer.

NilanjanDaw · 2026-02-02T19:13:10Z

kcc/src/lib.rs

+    pub fn generate_binding_keypair(&self) -> (KeyHandle, Vec<u8>) {
+        clear_stack_on_return(2, || {
+            let key_id = Uuid::new_v4();
+            let (pub_key, mut priv_key) = hpke::Kem::X25519HkdfSha256.generate_keypair();


Even though for initial phases, we will exclusively support X25519, we need to make these configurable. The KemAlgorithm will be passed as parameter indicating the DHKEM algorithm to use for the request.

NilanjanDaw · 2026-02-02T19:16:25Z

kcc/src/lib.rs

+                    algo: HpkeAlgorithm {
+                        kem: KemAlgorithm::DhKemX25519HkdfSha256,
+                        kdf: KdfAlgorithm::HkdfSha256,
+                        aead: AeadAlgorithm::Aes256Gcm,
+                    },


Again the exact algorithm chain should be passed from the orchestration layer and not hardcoded here.

NilanjanDaw · 2026-02-02T19:19:50Z

kcc/src/types.rs

+    Binding { algo: HpkeAlgorithm, binding_public_key: Vec<u8> },
+    Sek { algo: SigningAlgorithm, verifying_key: Vec<u8> },
+}
+


I would prefer defining the Algorithms below as a common protobuf definition and not in rust. And then use Prost to have a Rust binding for those protos. That would allow us to have a common definition between both the orchestration and the custody core components.

NilanjanDaw · 2026-02-02T19:21:09Z

kcc/src/types.rs

+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+#[repr(u32)]
+pub enum KemAlgorithm {
+    DhKemX25519HkdfSha256 = 1,


For each of these, we should have a default unknown definition, like KEM_ALGORITHM_UNSPECIFIED to prevent accidental values from being set.

NilanjanDaw · 2026-02-02T19:29:25Z

kcc/src/vault.rs

+        let name_cstr = CString::new(name)
+            .map_err(|_| IoError::new(std::io::ErrorKind::InvalidInput, "Invalid name"))?;
+        let fd = unsafe {
+            libc::syscall(libc::SYS_memfd_create, name_cstr.as_ptr(), MFD_CLOEXEC | MFD_SECRET)


Yeah I think I wrote MFD_SECRET in my doc. It should be MFD_NOEXEC_SEAL instead. We should add it to prevent this memory from ever being executable.

NilanjanDaw · 2026-02-02T19:45:09Z

kcc/src/lib.rs

+
+    pub fn generate_kem_keypair(&self, binding_pub_key: &[u8]) -> (KeyHandle, Vec<u8>) {
+        clear_stack_on_return(2, || {
+            let key_id = Uuid::new_v4();


The KEM keys and the Binding keys will share the same UUID. When creating the binding keys, a new UUID will be generated and returned back to the orchestration layer. This same UUID will be passed back to the generate_kem_keypair FFI. So when the KEM keys are generated, these keys are mapped to the same UUID and not a new one. This way we can look up the correct KEM key <-> Binding Key pair during the "decap and seal" and subsequent "open" operation.

NilanjanDaw · 2026-02-02T19:55:03Z

kcc/src/error.rs

@@ -0,0 +1,14 @@
+
+use thiserror::Error;
+


I think we should move this to a protobuf file, to ensure a single definition across the orchestration and custody core layers.

NilanjanDaw · 2026-02-03T06:44:01Z

kcc/src/lib.rs

+
+        let reaper_handle = Some(thread::spawn(move || {
+            while !stop_clone.load(Ordering::Relaxed) {
+                thread::sleep(Duration::from_secs(10));


Let's make this configurable?

df1a7c4 google#633

add generate_kem_keypair and generate_binding_keypair ffis

df1a7c4

atulpatildbz reviewed Feb 2, 2026

View reviewed changes

NilanjanDaw reviewed Feb 2, 2026

View reviewed changes

NilanjanDaw reviewed Feb 3, 2026

View reviewed changes

atulpatildbz pushed a commit to atulpatildbz/go-tpm-tools that referenced this pull request Feb 5, 2026

cherry-pick: add generate_kem_keypair and generate_binding_keypair ffis

7a11f4b

df1a7c4 google#633

Conversation

meetrajvala commented Jan 31, 2026

Uh oh!

atulpatildbz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NilanjanDaw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants