From 76b18706f1f0a8fcf12797150f5ec9f7c2cce652 Mon Sep 17 00:00:00 2001
From: Enginex0 <enginex0@users.noreply.github.com>
Date: Mon, 9 Mar 2026 16:08:07 +0100
Subject: [PATCH] fix(native-certgen): address Phase 3 validation findings

Remove ENCRYPT/VERIFY from KeyUsage mapping to match Kotlin behavior.
Document BasicConstraints and SKI suppression via rcgen NoCa default.
Fix rotating log off-by-one that kept one extra backup file. Handle
BMPString (UTF-16BE) and VisibleString in X.500 DN parser.
---
 native-certgen/src/certbuilder.rs      | 30 +++++++++++++++-----------
 native-certgen/src/logging/rotating.rs | 13 ++++++-----
 2 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/native-certgen/src/certbuilder.rs b/native-certgen/src/certbuilder.rs
index a3426ed..4027fec 100644
--- a/native-certgen/src/certbuilder.rs
+++ b/native-certgen/src/certbuilder.rs
@@ -87,8 +87,9 @@ fn build_leaf_params(
         timestamp_to_datetime(params.cert_not_after)?
     };
 
-    // Suppress AKI/SKI — Android attestation leaf certs don't include these
-    cp.key_identifier_method = rcgen::KeyIdMethod::PreSpecified(vec![]);
+    // rcgen 0.13.2: IsCa::NoCa (the default) emits neither BasicConstraints nor SKI
+    // extension. This matches real Android attestation leaf certs which include neither.
+    // No explicit suppression needed — NoCa is a no-op in the extension writer.
 
     // KeyUsage from purposes
     cp.key_usages = map_key_usages(&params.purposes);
@@ -101,22 +102,19 @@ fn build_leaf_params(
     Ok(cp)
 }
 
+/// Maps KeyPurpose values to X.509 KeyUsage bits per KeyCreationResult.aidl spec.
+/// Only SIGN, DECRYPT, WRAP_KEY, AGREE_KEY, and ATTEST_KEY produce KeyUsage bits.
+/// ENCRYPT and VERIFY are intentionally excluded (matches Kotlin CertificateGenerator).
 fn map_key_usages(purposes: &[i32]) -> Vec<KeyUsagePurpose> {
     let mut usages = Vec::new();
     for &purpose in purposes {
         match purpose {
-            2 | 3 => {
-                // SIGN or VERIFY -> digitalSignature
+            2 => {
+                // SIGN -> digitalSignature
                 if !usages.contains(&KeyUsagePurpose::DigitalSignature) {
                     usages.push(KeyUsagePurpose::DigitalSignature);
                 }
             }
-            0 => {
-                // ENCRYPT -> keyEncipherment
-                if !usages.contains(&KeyUsagePurpose::KeyEncipherment) {
-                    usages.push(KeyUsagePurpose::KeyEncipherment);
-                }
-            }
             1 => {
                 // DECRYPT -> dataEncipherment
                 if !usages.contains(&KeyUsagePurpose::DataEncipherment) {
@@ -224,10 +222,18 @@ fn extract_string_from_der_any(der: &[u8]) -> String {
     let content = &der[header_len..end];
 
     match tag {
-        0x0C | 0x13 | 0x16 => {
-            // UTF8String (0x0C), PrintableString (0x13), IA5String (0x16)
+        0x0C | 0x13 | 0x16 | 0x1A => {
+            // UTF8String (0x0C), PrintableString (0x13), IA5String (0x16), VisibleString (0x1A)
             String::from_utf8_lossy(content).into_owned()
         }
+        0x1E => {
+            // BMPString (UTF-16BE)
+            let chars: Vec<u16> = content
+                .chunks_exact(2)
+                .map(|c| u16::from_be_bytes([c[0], c[1]]))
+                .collect();
+            String::from_utf16_lossy(&chars)
+        }
         _ => String::from_utf8_lossy(content).into_owned(),
     }
 }
diff --git a/native-certgen/src/logging/rotating.rs b/native-certgen/src/logging/rotating.rs
index 743e544..e6e7096 100644
--- a/native-certgen/src/logging/rotating.rs
+++ b/native-certgen/src/logging/rotating.rs
@@ -53,7 +53,12 @@ fn rotate(state: &mut RotatingState) {
     state.current.take();
 
     let dir = &state.dir;
-    // Shift older files up: .{max-1} is deleted, .{N} -> .{N+1}
+    // Delete the oldest rotated file before shifting
+    let oldest = dir.join(format!("certgen.log.{}", state.max_files));
+    if oldest.exists() {
+        let _ = fs::remove_file(&oldest);
+    }
+    // Shift older files up: .{N} -> .{N+1}
     for i in (1..state.max_files).rev() {
         let from = dir.join(format!("certgen.log.{}", i));
         let to = dir.join(format!("certgen.log.{}", i + 1));
@@ -68,12 +73,6 @@ fn rotate(state: &mut RotatingState) {
         let _ = fs::rename(&current_path, &first_rotated);
     }
 
-    // Delete excess files beyond max_files
-    let excess = dir.join(format!("certgen.log.{}", state.max_files + 1));
-    if excess.exists() {
-        let _ = fs::remove_file(&excess);
-    }
-
     let (file, size) = open_current_log(dir);
     state.current = file;
     state.current_size = size;