Support for SecMod and RSA OAEP Wrapping - #53
Conversation
Adds bindings + Rust wrappers for two NSS features that downstream consumers (notably Mozilla's lockstore) need to talk to external PKCS#11 tokens like the YubiKey PIV applet: * Runtime module loader: a new `secmod` module exposes `SecmodModule::load_user` over `SECMOD_LoadUserModule` / `SECMOD_UnloadUserModule` / `SECMOD_DestroyModule`, so callers can register a vendor PKCS#11 .so / .dylib at runtime without going through the user's `secmod.db`. * RSA-OAEP-SHA256 wrap/unwrap: two free functions `rsa_oaep_sha256_encrypt` / `rsa_oaep_sha256_decrypt` wrap `PK11_PubEncrypt` / `PK11_PrivDecrypt` with MGF1-SHA256 and an empty label. Useful for wrapping a fresh AES KEK under an RSA public key held on a hardware token, then unwrapping later via the token's private key. Also exposes `Slot::find_cert_by_nickname`, `Slot::find_private_key_for_cert`, and `Certificate::public_key` to complete the YubiKey-style flow (find cert by label, extract pubkey, authenticate, decrypt with the matching privkey). No behaviour change for existing consumers.
Adds three optional fields to `Pkcs11Uri` so callers can locate
specific objects on a token by the standard RFC 7512 query
attributes:
* `object` - percent-decoded label (cert/key)
* `id` - percent-decoded raw CKA_ID bytes
* `type` - object-type discriminator (public, private, cert,
secret-key, data)
The existing `token`-only parse path is preserved; unknown
attributes continue to be ignored. Two new tests cover the
extended parse and the back-compat empty case.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| match key { | ||
| "token" => token = Some(percent_decode(value)), | ||
| "object" => object = Some(percent_decode(value)), | ||
| "id" => id = Some(percent_decode(value).into_bytes()), |
There was a problem hiding this comment.
Caution
percent_decode returns a String via from_utf8_lossy, which replaces non-UTF-8 bytes with U+FFFD. For the id attribute this silently corrupts binary CKA_ID values — e.g. id=%FF yields [0xEF, 0xBF, 0xBD] instead of [0xFF].
Per RFC 7512 §2.3, pk11-id is %-encoded arbitrary octets. You need a raw-bytes decode path here.
| "id" => id = Some(percent_decode(value).into_bytes()), | |
| "id" => id = Some(percent_decode_bytes(value)), |
With a percent_decode_bytes helper that returns Vec<u8> (same loop as percent_decode, but returns result directly without String::from_utf8_lossy). Then percent_decode itself can be built on top:
fn percent_decode(s: &str) -> String {
String::from_utf8_lossy(&percent_decode_bytes(s)).into_owned()
}
| /// Find a certificate on this slot by nickname. | ||
| #[must_use] | ||
| pub fn find_cert_by_nickname(&self, nickname: &str) -> Option<Certificate> { | ||
| let c_nickname = std::ffi::CString::new(nickname).ok()?; | ||
| let ptr = unsafe { | ||
| PK11_FindCertFromNickname(c_nickname.as_ptr().cast_mut(), null_mut()) | ||
| }; | ||
| if ptr.is_null() { | ||
| None | ||
| } else { | ||
| Certificate::from_ptr(ptr).ok() | ||
| } | ||
| } |
There was a problem hiding this comment.
Warning
PK11_FindCertFromNickname searches across all loaded slots — it does not constrain to self. Placing this on Slot is misleading.
Consider either:
- Making this a free function (or a
Certificateassociated fn), since the slot isn't used. - Documenting that the search is global and the nickname can be prefixed with
"tokenname:"to scope it.
| // `SECMODModule` is internally synchronised by NSS; the handle can move | ||
| // across threads safely once loaded. | ||
| unsafe impl Send for SecmodModule {} | ||
| unsafe impl Sync for SecmodModule {} |
There was a problem hiding this comment.
Warning
The Send impl is reasonable (ownership transfer). The Sync impl is a stronger claim — it asserts that &SecmodModule is safe to share across threads, meaning name() and slot_count() (reading raw struct fields via unsafe) must be data-race-free.
NSS holds locks for operations, but reading commonName / slotCount fields from Rust doesn't go through NSS locking. If another thread loads/unloads modules while these fields are read, that's a data race.
Unless you can confirm NSS guarantees these fields are immutable after SECMOD_LoadUserModule returns, consider dropping Sync or documenting the invariant.
| // unquoted in the module spec; bail early on the rare case. | ||
| if name.contains('"') || path_str.contains('"') { | ||
| return Err(Error::InvalidInput); | ||
| } |
There was a problem hiding this comment.
Note
The " rejection is good. Consider also rejecting backslashes — on Windows, library_path will naturally contain \, and NSS module-spec parsing may interpret \" as an escape.
| // unquoted in the module spec; bail early on the rare case. | |
| if name.contains('"') || path_str.contains('"') { | |
| return Err(Error::InvalidInput); | |
| } | |
| if name.contains('"') || name.contains('\\') || path_str.contains('"') || path_str.contains('\\') { | |
| return Err(Error::InvalidInput); | |
| } |
| pub fn rsa_oaep_sha256_encrypt(pubkey: &PublicKey, plaintext: &[u8]) -> Res<Vec<u8>> { | ||
| let params = CK_RSA_PKCS_OAEP_PARAMS { | ||
| hashAlg: CKM_SHA256.into(), | ||
| mgf: CKG_MGF1_SHA256.into(), | ||
| source: CKZ_DATA_SPECIFIED.into(), | ||
| pSourceData: null_mut(), | ||
| ulSourceDataLen: 0, | ||
| }; |
There was a problem hiding this comment.
Tip
The OAEP params construction is duplicated verbatim in rsa_oaep_sha256_decrypt. Consider extracting a small helper:
fn oaep_sha256_params() -> CK_RSA_PKCS_OAEP_PARAMS {
CK_RSA_PKCS_OAEP_PARAMS {
hashAlg: CKM_SHA256.into(),
mgf: CKG_MGF1_SHA256.into(),
source: CKZ_DATA_SPECIFIED.into(),
pSourceData: null_mut(),
ulSourceDataLen: 0,
}
}
| pub fn public_key(&self) -> Res<PublicKey> { | ||
| let ptr = unsafe { CERT_ExtractPublicKey(**self) }; | ||
| if ptr.is_null() { | ||
| Err(Error::Internal) |
There was a problem hiding this comment.
Note
CERT_ExtractPublicKey can return null when the SPKI uses an unsupported algorithm. Error::Internal loses the underlying NSS error code. Consider Error::last_nss_error() for consistency with secstatus_to_res / into_result:
| pub fn public_key(&self) -> Res<PublicKey> { | |
| let ptr = unsafe { CERT_ExtractPublicKey(**self) }; | |
| if ptr.is_null() { | |
| Err(Error::Internal) | |
| let ptr = unsafe { CERT_ExtractPublicKey(**self) }; | |
| if ptr.is_null() { | |
| Err(Error::last_nss_error()) | |
| } else { |
There was a problem hiding this comment.
Good additions — SecMod loading, RSA-OAEP wrapping, and the Pkcs11Uri extensions are well-structured and follow existing codebase patterns. The FFI boundary handling is careful (null checks, loaded field verification, spec-injection guard on ").
One correctness bug: the id attribute percent-decoding goes through String::from_utf8_lossy, which silently corrupts non-UTF-8 binary CKA_IDs. This needs a raw-bytes decode path per RFC 7512.
Also flagged: find_cert_by_nickname on Slot doesn't actually scope to the slot (PK11_FindCertFromNickname is global), and the unsafe impl Sync on SecmodModule warrants verification that the accessed struct fields are immutable post-load. Details in inline comments.
There was a problem hiding this comment.
Good additions — the SecMod wrapper, RSA-OAEP encrypt/decrypt, and extended Pkcs11Uri parsing are well-structured and follow existing codebase conventions (scoped_ptr, SECItemBorrowed, etc.). The FFI boundary handling is careful throughout.
Two items to address before merge:
-
Breaking API change on
Pkcs11Uri: Adding pub fields to a public struct without#[non_exhaustive]breaks downstream struct-literal construction. This is the right time to add the attribute. -
No test coverage for the core new functionality: The RSA-OAEP encrypt/decrypt round-trip,
SecmodModuleerror paths, and newSlot/Certificatemethods are all untested. Thepk11_utilsparser tests are good — the rest of the PR needs similar attention.
I've also noted that Certificate::public_key() can be simplified to a one-liner by leaning on the existing from_ptr → into_result null-handling path, which also addresses the Error::Internal concern from the prior review.
The prior review's points about percent_decode corrupting binary CKA_IDs and find_cert_by_nickname being misleadingly scoped to Slot remain open and valid — I won't repeat them here.
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct Pkcs11Uri { |
There was a problem hiding this comment.
Warning
Adding three pub fields to Pkcs11Uri is a breaking API change — any downstream code constructing the struct directly (e.g. Pkcs11Uri { token: Some(...) }) will fail to compile. Since this struct is already public without #[non_exhaustive], consider adding it now to prevent future breakage when more RFC 7512 attributes are added.
| #[derive(Debug, Clone, PartialEq, Eq)] | |
| pub struct Pkcs11Uri { | |
| #[derive(Debug, Clone, PartialEq, Eq)] | |
| #[non_exhaustive] | |
| pub struct Pkcs11Uri { |
| pub fn public_key(&self) -> Res<PublicKey> { | ||
| let ptr = unsafe { CERT_ExtractPublicKey(**self) }; | ||
| if ptr.is_null() { | ||
| Err(Error::Internal) | ||
| } else { | ||
| PublicKey::from_ptr(ptr) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Note
Expanding on the existing comment about Error::Internal: the manual null check is entirely redundant here. PublicKey::from_ptr already calls into_result, which returns Error::last_nss_error() on null. The whole method simplifies to:
| pub fn public_key(&self) -> Res<PublicKey> { | |
| let ptr = unsafe { CERT_ExtractPublicKey(**self) }; | |
| if ptr.is_null() { | |
| Err(Error::Internal) | |
| } else { | |
| PublicKey::from_ptr(ptr) | |
| } | |
| } | |
| } | |
| pub fn public_key(&self) -> Res<PublicKey> { | |
| unsafe { PublicKey::from_ptr(CERT_ExtractPublicKey(**self)) } | |
| } |
| /// | ||
| /// Returns an NSS error if the public key is not an RSA key, the | ||
| /// plaintext is too long, or the underlying token rejects the request. | ||
| pub fn rsa_oaep_sha256_encrypt(pubkey: &PublicKey, plaintext: &[u8]) -> Res<Vec<u8>> { |
There was a problem hiding this comment.
Warning
rsa_oaep_sha256_encrypt / rsa_oaep_sha256_decrypt have no tests. At minimum, add a round-trip test that generates an RSA key pair, encrypts a known plaintext, decrypts, and asserts equality. Edge cases worth covering: empty plaintext (valid for OAEP), plaintext at the maximum length boundary, and a mismatched-key failure path.
Certificate::public_key(), Slot::find_cert_by_nickname, Slot::find_private_key_for_cert are also untested, though those may be harder to test without a populated token. Even a #[cfg(test)] smoke test against the internal NSS softtoken would add confidence.
| } | ||
| return Err(Error::Internal); | ||
| } | ||
| Self::from_ptr(ptr) |
There was a problem hiding this comment.
Note
SecmodModule has no tests either. While load_user is difficult to test in CI without a real .so/.dylib, the failure paths are testable: a non-existent library path should return Err, and the name/path validation (embedded ") should reject bad input. For example:
#[test]
fn load_user_rejects_embedded_quote() {
fixture_init();
assert!(SecmodModule::load_user("bad\"name", Path::new("/nonexistent.so")).is_err());
}
#[test]
fn load_user_nonexistent_library() {
fixture_init();
assert!(SecmodModule::load_user("test", Path::new("/no/such/lib.so")).is_err());
}
There was a problem hiding this comment.
Good feature additions — the SecmodModule wrapper, RSA-OAEP encrypt/decrypt, Certificate::public_key(), and the Pkcs11Uri extensions are well-structured and consistent with existing crate patterns.
The prior two reviews raised several valid points that remain open (CKA_ID binary corruption via from_utf8_lossy, find_cert_by_nickname not scoping to the slot, #[non_exhaustive] on Pkcs11Uri, Certificate::public_key() simplification, unsafe impl Sync justification, missing tests). I won't repeat those — they should be addressed before merge.
Additional observations (new in this review):
- Recovered key material from
rsa_oaep_sha256_decryptis returned as a plainVec<u8>with no zeroization guidance. Since this is specifically a key-unwrapping API, callers need to know they're responsible for scrubbing the buffer. At minimum a doc note; ideally aZeroizing<Vec<u8>>return type. Pkcs11Uri::object_typestores a free-formStringbut RFC 7512 §2.3 defines a closed set (public,private,cert,secret-key,data). An enum or at least documented valid values would prevent silent misuse.- Minor doc inaccuracy in
SecmodModule::load_user: the comment says NSS builds the spec string, but this code builds it.
| /// | ||
| /// Returns the recovered plaintext; on a token-resident private key the | ||
| /// underlying `PK11_PrivDecrypt` performs the operation on the token, so | ||
| /// the private key material never leaves the device. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns an NSS error if the private key is not RSA, the ciphertext | ||
| /// length doesn't match the key's modulus, OAEP padding verification | ||
| /// fails, or the token rejects the request (e.g. not authenticated). | ||
| pub fn rsa_oaep_sha256_decrypt(privkey: &PrivateKey, ciphertext: &[u8]) -> Res<Vec<u8>> { |
There was a problem hiding this comment.
Warning
Since this is a key-wrapping API, rsa_oaep_sha256_decrypt will typically recover raw cryptographic key material. The returned Vec<u8> is not zeroized on drop — if the caller doesn't scrub it, the key material persists in freed heap memory.
The crate doesn't use zeroize today, so this isn't necessarily blocking, but it's worth either:
- Adding a
# Securitydoc section noting the caller's responsibility to zeroize, or - Returning a
zeroize::Zeroizing<Vec<u8>>(would add a dependency).
At minimum, document the expectation:
| /// | |
| /// Returns the recovered plaintext; on a token-resident private key the | |
| /// underlying `PK11_PrivDecrypt` performs the operation on the token, so | |
| /// the private key material never leaves the device. | |
| /// | |
| /// # Errors | |
| /// | |
| /// Returns an NSS error if the private key is not RSA, the ciphertext | |
| /// length doesn't match the key's modulus, OAEP padding verification | |
| /// fails, or the token rejects the request (e.g. not authenticated). | |
| pub fn rsa_oaep_sha256_decrypt(privkey: &PrivateKey, ciphertext: &[u8]) -> Res<Vec<u8>> { | |
| /// RSA-OAEP-SHA256 decryption symmetric to `rsa_oaep_sha256_encrypt`. | |
| /// | |
| /// Returns the recovered plaintext; on a token-resident private key the | |
| /// underlying `PK11_PrivDecrypt` performs the operation on the token, so | |
| /// the private key material never leaves the device. | |
| /// | |
| /// # Security | |
| /// | |
| /// The returned `Vec` may contain sensitive key material. Callers should | |
| /// zeroize the buffer when it is no longer needed. |
| /// `data`. Callers use this to pick the right NSS lookup (e.g. | ||
| /// `find_cert_by_nickname` vs `find_key_by_nickname`). | ||
| pub object_type: Option<String>, | ||
| } |
There was a problem hiding this comment.
Note
RFC 7512 §2.3 restricts type to a closed set: public, private, cert, secret-key, data. Storing it as a free-form String means callers can silently pass an invalid type through the parser.
Consider either an enum:
pub enum Pkcs11ObjectType {
Public,
Private,
Cert,
SecretKey,
Data,
}…or at least documenting the valid values in the field doc so callers know what to match on.
| /// module-spec string `library="<path>" name="<name>"` and dlopen's | ||
| /// the library. | ||
| /// | ||
| /// # Errors |
There was a problem hiding this comment.
Note
The doc comment says "NSS internally builds the module-spec string" — but in fact this code builds the spec string itself. Consider:
| /// module-spec string `library="<path>" name="<name>"` and dlopen's | |
| /// the library. | |
| /// | |
| /// # Errors | |
| /// `library_path` is the absolute path to the shared library (e.g. | |
| /// `/opt/homebrew/lib/libykcs11.dylib`). The wrapper builds the NSS | |
| /// module-spec string `library="<path>" name="<name>"` and passes it to | |
| /// `SECMOD_LoadUserModule`, which dlopen's the library. |
There was a problem hiding this comment.
Well-structured additions — the SecmodModule RAII wrapper, RSA-OAEP encrypt/decrypt, and the Pkcs11Uri extensions all follow existing crate conventions cleanly. The FFI boundary handling is careful throughout (null checks, loaded field verification, spec-injection guard).
The prior two reviews raised several valid points that remain unaddressed: binary CKA_ID corruption via percent_decode, find_cert_by_nickname not actually scoping to the slot, #[non_exhaustive] on Pkcs11Uri, Certificate::public_key() simplification, unsafe impl Sync justification, missing tests, zeroization guidance on decrypt output, and the doc inaccuracy in load_user. I won't repeat those — see the inline threads.
New observations in this review:
- RFC 7512 query component not stripped before parsing path attributes — a URI with
?pin-source=...will corrupt the last path attribute value. Pre-existing but exacerbated by the expanded parser. Inline suggestion provided. find_private_key_for_certauthentication prerequisite is undocumented — callers need to knowslot.authenticate()must precede this call.
| @@ -53,15 +63,27 @@ pub fn parse(uri: &str) -> Res<Pkcs11Uri> { | |||
| let path = uri.strip_prefix("pkcs11:").ok_or(Error::InvalidInput)?; | |||
There was a problem hiding this comment.
Warning
RFC 7512 §2 separates path attributes (;-delimited) from query attributes (&-delimited) with a ?. The parser treats everything after pkcs11: as path, so a URI like pkcs11:token=foo;object=bar?pin-source=... would pollute the last path attribute's value with the query string.
Pre-existing for token, but now that the parser handles more attributes this is more likely to bite callers.
| let path = uri.strip_prefix("pkcs11:").ok_or(Error::InvalidInput)?; | |
| let path = uri.strip_prefix("pkcs11:").ok_or(Error::InvalidInput)?; | |
| // RFC 7512 §2: query attributes (pin-source, etc.) follow `?`. | |
| let (path, _query) = path.split_once('?').unwrap_or((path, "")); |
| /// Find the private key associated with a certificate on this slot. | ||
| #[must_use] |
There was a problem hiding this comment.
Note
PK11_FindPrivateKeyFromCert receives null_mut() for wincx, so if the slot hasn't been authenticated yet this call will silently fail and return None. The commit message describes an "authenticate, decrypt" flow — worth a brief doc note so callers know to call slot.authenticate() first.
| /// Find the private key associated with a certificate on this slot. | |
| #[must_use] | |
| /// Find the private key associated with a certificate on this slot. | |
| /// | |
| /// The slot must already be authenticated (via [`Slot::authenticate`]) | |
| /// before calling this method; otherwise the lookup will fail. | |
| #[must_use] |
There was a problem hiding this comment.
Well-structured additions that follow existing crate conventions (scoped_ptr!, SECItemBorrowed, secstatus_to_res). The FFI boundary handling is careful — the SECMOD_LoadUserModule wrapper correctly checks both the pointer and the loaded field, and the OAEP functions properly size output buffers and propagate NSS errors.
Architectural note: SecmodModule exposes slot_count() but no way to obtain individual Slot handles from the loaded module. For the YubiKey workflow described in the commit message (load module → find slot → find cert → extract pubkey → encrypt), callers currently need to bridge the gap with p11::all_token_slots() and filter by token name. If that's the intended pattern, it works — but a SecmodModule::slots() iterator would make the API more self-contained.
Prior reviews identified several important issues — in particular the percent_decode / CKA_ID correctness bug (CAUTION), the #[non_exhaustive] concern on Pkcs11Uri (WARNING), and the RFC 7512 query-string stripping (WARNING) — that should be addressed before merge.
There was a problem hiding this comment.
Clean additions — the SecmodModule RAII wrapper, RSA-OAEP encrypt/decrypt, Certificate::public_key(), and the Pkcs11Uri parser extensions are well-structured and consistent with existing crate conventions.
Prior reviews raised several important issues that remain open and should be addressed before merge — in particular the binary CKA_ID corruption (CAUTION), #[non_exhaustive] on Pkcs11Uri (WARNING), RFC 7512 query-string stripping (WARNING), and missing test coverage for the core new functionality (WARNING). I won't repeat those.
Additional observation: pk11_utils::build() only emits token=, but Pkcs11Uri now carries object, id, and object_type. A parse→modify→build round-trip silently drops the new fields. Consider either updating build() to accept &Pkcs11Uri and emit all populated attributes, or documenting the limitation.
| err::{Error, IntoResult, PRErrorCode, Res, secstatus_to_res}, | ||
| ext::{ExtensionHandler, ExtensionHandlerResult, ExtensionWriterResult}, | ||
| p11::{PrivateKey, PublicKey, SymKey, random, randomize}, | ||
| p11::{Certificate, PrivateKey, PublicKey, SymKey, random, randomize}, |
There was a problem hiding this comment.
Note
Certificate and SecmodModule are added to the crate-root re-exports, but the two new public functions rsa_oaep_sha256_encrypt / rsa_oaep_sha256_decrypt are not. Callers must use the fully-qualified nss::p11::rsa_oaep_sha256_encrypt(…) path to reach them.
If these are intended as primary API (which the doc comments suggest), re-export them for consistency:
| p11::{Certificate, PrivateKey, PublicKey, SymKey, random, randomize}, | |
| p11::{Certificate, PrivateKey, PublicKey, SymKey, random, randomize, | |
| rsa_oaep_sha256_encrypt, rsa_oaep_sha256_decrypt}, |
1 participant
No description provided.