Passport Reader#178
Conversation
CodSpeed WallTime Performance ReportMerging #178 will improve performances by 33.33%Comparing
|
| Benchmark | BASE |
HEAD |
Change | |
|---|---|---|---|---|
| ⚡ | reduce_add_rc |
4 ns | 3 ns | +33.33% |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a comprehensive passport reader system that replaces the previous mock test data generation with real passport SOD parsing and certificate validation capabilities. The key changes include:
- Replace mock data generators with real passport SOD (Security Object Document) parsing
- Add DSC (Document Signer Certificate) and CSCA (Country Signing Certificate Authority) validation
- Implement circuit input generation for complete age verification workflows
Reviewed Changes
Copilot reviewed 17 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/lib.rs |
New main library interface implementing PassportReader with validation and circuit input generation |
src/parser/*.rs |
Parser modules for binary data, SOD structures, DSC certificates, and type definitions |
src/mock_generator.rs |
Mock data generation utilities for testing with synthetic passport data |
src/mock_keys.rs |
Mock RSA private keys for testing certificate validation |
csca_registry/csca_public_key.json |
USA CSCA public key registry for certificate chain validation |
Cargo.toml |
Updated dependencies for X.509 parsing, ASN.1 handling, and cryptographic operations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| thread_local! { | ||
| static CSCA_JSON_PATH: RefCell<Option<String>> = RefCell::new(None); | ||
| } |
There was a problem hiding this comment.
Thread-local storage with RefCell can cause issues in multi-threaded environments. Consider using a global static with Mutex or RwLock for thread-safe access, or document that this should only be used in single-threaded contexts.
| signing_time: signed_attrs.signing_time.map(|ut| { | ||
| let time_str = ut.to_string(); | ||
| chrono::DateTime::parse_from_rfc3339(&format!("{}T00:00:00Z", time_str)) | ||
| .unwrap_or_else(|_| chrono::Utc::now().into()) | ||
| .with_timezone(&chrono::Utc) |
There was a problem hiding this comment.
The signing time parsing assumes the UtcTime string can be directly formatted as a date, but this may not always be valid. The fallback to current time could mask parsing errors and return incorrect timestamps.
| signing_time: signed_attrs.signing_time.map(|ut| { | |
| let time_str = ut.to_string(); | |
| chrono::DateTime::parse_from_rfc3339(&format!("{}T00:00:00Z", time_str)) | |
| .unwrap_or_else(|_| chrono::Utc::now().into()) | |
| .with_timezone(&chrono::Utc) | |
| signing_time: signed_attrs.signing_time.and_then(|ut| { | |
| // Try to parse the UtcTime using chrono. If parsing fails, return None. | |
| let time_str = ut.to_string(); | |
| chrono::DateTime::parse_from_rfc3339(&format!("{}T00:00:00Z", time_str)) | |
| .ok() | |
| .map(|dt| dt.with_timezone(&chrono::Utc)) |
| if msg_digest.len() > 2 && msg_digest[0] == 0x04 { | ||
| msg_digest = msg_digest[2..].to_vec(); | ||
| } |
There was a problem hiding this comment.
The magic numbers 0x04 and offset 2 should be documented or defined as named constants to explain what ASN.1 structure is being parsed here.
| let hex_der = hex::decode(binary.to_hex().trim_start_matches("0x")).unwrap(); | ||
| let content_info: ContentInfo = der::decode(&hex_der).expect("CMS decode failed"); |
There was a problem hiding this comment.
Converting binary data to hex string and then back to bytes is inefficient. Consider working directly with the binary data without the intermediate hex conversion.
| let hex_der = hex::decode(binary.to_hex().trim_start_matches("0x")).unwrap(); | |
| let content_info: ContentInfo = der::decode(&hex_der).expect("CMS decode failed"); | |
| let content_info: ContentInfo = der::decode(binary.as_bytes()).expect("CMS decode failed"); |
| @@ -0,0 +1 @@ | |||
| <!DOCTYPE html> | |||
| utils::{get_oid_name, strip_length_prefix, OidEntry}, | ||
| }, | ||
| std::collections::HashMap, | ||
| x509_parser::prelude::*, |
There was a problem hiding this comment.
Can we import only the required items?
| pub serial_number: Binary, | ||
| pub signature_algorithm: SignatureAlgorithm, | ||
| pub issuer: String, | ||
| pub validity_not_before: String, |
There was a problem hiding this comment.
We should use some Date format instead of String
| let dsc_signature = self.sod.signer_info.signature.to_number_array(); | ||
| dsc_pubkey | ||
| .verify( | ||
| Pkcs1v15Sign::new::<Sha256>(), |
There was a problem hiding this comment.
Don't show US passports use RSASSA-PSS? Maybe we can change it to something like this
match sod.signer_info.signature_algorithm.name {
SignatureAlgorithmName::Sha256WithRsaEncryption => {
dsc_pubkey.verify(Pkcs1v15Sign::new::<Sha256>(), &signed_attr_hash, &dsc_signature)
}
SignatureAlgorithmName::RsassaPss => {
// Use PSS verification with proper parameters
dsc_pubkey.verify(Pss::new::<Sha256>(), &signed_attr_hash, &dsc_signature)
}
// ... other algorithms
}There was a problem hiding this comment.
yeah, changed that
| .signed_attrs | ||
| .message_digest | ||
| .to_number_array(); | ||
| if msg_digest.len() > 2 && msg_digest[0] == 0x04 { |
There was a problem hiding this comment.
Is it okay to hardcode DER header length?
There was a problem hiding this comment.
Yeah, it works but hardcoding assumes a 2-byte OCTET STRING header since its ANS.1 Encoded
|
|
||
| pub const MAX_SIGNED_ATTRIBUTES_SIZE: usize = 200; | ||
| pub const MAX_DG1_SIZE: usize = 95; | ||
| pub const SIG_BYTES: usize = 256; |
There was a problem hiding this comment.
Missing 4096-bit RSA support?
There was a problem hiding this comment.
wWe’re directly parsing the CSCA key using RsaPublicKey::from_public_key_der(), so we don’t need that.
For csc_pubkey: [u8; SIG_BYTES * 2], we’re basically defining a fixed buffer (around 512 bytes)
| } | ||
|
|
||
| /// Convert to circuit inputs for Noir Circuits | ||
| pub fn to_circuit_inputs( |
There was a problem hiding this comment.
- Inconsistent Error Handling
- Many internal parsing details are public
- No clear API boundary between internal parsing and external interface
|
|
||
| /// Converts a parsed `X509Certificate` into the internal `DSC` struct. | ||
| pub fn from_x509(cert: X509Certificate<'_>) -> DSC { | ||
| let registry = load_oids(); |
There was a problem hiding this comment.
Use lazy_static!, heavy allocation used in multiple places.
| } | ||
|
|
||
| pub fn to_number_array(&self) -> Vec<u8> { | ||
| self.data.clone() |
There was a problem hiding this comment.
Unnecessary clone, return and use &[u8] where possible.
| "filename": "cert-00267-pubkey.pem", | ||
| "public_key": "MIICIjANBgkqhkiG9w0BAQEFAAOCAg8AMIICCgKCAgEAw4VGHnr3jsI//bv4fKBmGYLcrdcABkQNmVr8xkmlS7xxkDEQCohnzKSk8v0T+L0yp3h+WctO1ZwsaJfU0HDdMogDmEv3SeoNImOuqiofzkDLdH6/IWA/JfEwBYDz+1cn7/rbHuFCwo/dkPNJkeE7OrLGd9g6tsj8YibMZrB8N7nIVgEa9QOEiqXmUUV4KNKKRZJLu/RaWzZGNq+JqlXgANKvSwKWVW+aINb84UfkBIdbPfIkEq6JMdok+YIQyHemEP6nxnHAVQwOYRkgHidw9YHXZZkHgh75efixMoAy3LQxvoknECrlpZgxqIpdup+AwsEW8hoJZbpd3Dxeb27AAF3Rl9LBGPi1thd1A4Qb/AjVDtIyQaU62cHo7fEGcW5kVg/BLGhKH+RJKOqEZoG8b80X0jv/Z2VNNtDU/8flcKG9DkD+d+kgjMQAyPCOjC2achDcK5Zc1Q8/tq0T25x6GoRjbxadqR6MKrAYaP8KOLCrL0MJsjvcfD3M+hdyJXJPN/7TeCyKbK+dBvVCk9BePi6lBKmp6lEHjyyydgNBBzZPrTNf5l60Oh9eWpaIoTHcmToS6zKVffFb/dWnkCTF1BBuy1VgJNeIspn8n1PhY5Q23qvV8q+Q1Ta1X3TFGkvAb1SXGiPQW4Hd2JrFJEBo8Ah8nn8rQKX999PLI+An1acCAwEAAQ==", | ||
| "subject": "C=US, O=U.S. Government, OU=Department of State, OU=MRTD, OU=Certification Authorities, OU=U.S. Department of State MRTD CA", | ||
| "notBefore": "Dec 18 16:21:01 2014 GMT", |
There was a problem hiding this comment.
We should use ISO 8601 or Unix timestamp for time format.
|
Please add a README as well. |

Uh oh!
There was an error while loading. Please reload this page.