Skip to content

Fix Pkcs11::new bug preventing PKCS#11 library loading and bump rust-version and update deps #289

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
426 changes: 275 additions & 151 deletions Cargo.lock

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions cryptoki-sys/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@ documentation = "https://docs.rs/crate/cryptoki-sys"
rust-version = "1.77"

[build-dependencies]
bindgen = { version = "0.70.1", optional = true }
bindgen = { version = "0.72.0", optional = true }

[dependencies]
libloading = "0.8.6"
libloading = "0.8.8"

[features]
generate-bindings = ["bindgen"]
11 changes: 5 additions & 6 deletions cryptoki/Cargo.toml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related with your PR and you don't need to change anything here but this made me thinking that we could add a dependabot check in some nightly CI which could check that we use the latest dependencies, and update if not!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. It took me quite a while to locate the actual change, even though I've seen it earlier 😅

Maybe it's best to split this into several smaller PRs and keep this one only about the fix? 🤔

(Sorry for putting you for even more work 🙇 )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mematthias made a good job of splitting between different commit and there seems to be one just to address the bug, would that be enough 🕵️?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That first commit is still kind-of packed: 860d945 and it includes stuff such as raising MRSV to 1.77 that has been done in another PR...

I just itches me to restructure these commits differently but... maybe it's just my OCD 😅

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be just rebased (instead of doing the merge commits on the way.

Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,16 @@ documentation = "https://docs.rs/crate/cryptoki"
rust-version = "1.77"

[dependencies]
bitflags = "1.3"
libloading = "0.8.6"
log = "0.4.14"
bitflags = "2.9.1"
libloading = "0.8.8"
log = "0.4.27"
cryptoki-sys = { path = "../cryptoki-sys", version = "0.4.0" }
paste = "1.0.6"
paste = "1.0.15"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for paste we have separate issue #279 where it is considered unmaintained (or feature complete -- depending on the point of view) and @hug-dev had a proposal to remove the dependency . Given that it is used only in one place and audit will show the issues with that, I would rather go with removing it than updating. @hug-dev can you open a PR with the proposed change in there to remove it?

secrecy = "0.10.3"

[dev-dependencies]
num-traits = "0.2.14"
hex = "0.4.3"
serial_test = "0.5.1"
serial_test = "3.2.0"
testresult = "0.4.1"

[features]
Expand Down
4 changes: 2 additions & 2 deletions cryptoki/src/context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,8 @@ impl Pkcs11Impl {

impl Drop for Pkcs11Impl {
fn drop(&mut self) {
if let Err(e) = self.finalize() {
error!("Failed to finalize: {}", e);
if let Err(err) = self.finalize() {
error!("Failed to finalize: {err}");
}
}
}
Expand Down
3 changes: 1 addition & 2 deletions cryptoki/src/error/rv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,7 @@ impl From<CK_RV> for Rv {
CKR_VENDOR_DEFINED => Rv::Error(RvError::VendorDefined),
other => {
error!(
"Can not find a corresponding error for {}, converting to GeneralError.",
other
"Can not find a corresponding error for {other}, converting to GeneralError."
);
Rv::Error(RvError::GeneralError)
}
Expand Down
34 changes: 26 additions & 8 deletions cryptoki/src/mechanism/mechanism_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use cryptoki_sys::*;
use std::fmt::{Debug, Formatter};

bitflags! {
#[derive(Debug, Clone, Copy)]
struct MechanismInfoFlags: CK_FLAGS {
const HW = CKF_HW;
const ENCRYPT = CKF_ENCRYPT;
Expand All @@ -25,7 +26,6 @@ bitflags! {
const EC_F_P = CKF_EC_F_P;
const EC_F_2M = CKF_EC_F_2M;
const EC_ECPARAMETERS = CKF_EC_ECPARAMETERS;
const EC_NAMEDCURVE = CKF_EC_NAMEDCURVE;
const EC_OID = CKF_EC_OID;
const EC_UNCOMPRESS = CKF_EC_UNCOMPRESS;
const EC_COMPRESS = CKF_EC_COMPRESS;
Expand All @@ -35,6 +35,12 @@ bitflags! {
}
}

impl MechanismInfoFlags {
/// `CKF_EC_NAMEDCURVE` is deprecated with `PKCS#11 3.00`. It is replaced by [`CKF_EC_OID`](MechanismInfoFlags::EC_OID).
#[deprecated = "use `EC_OID` instead"]
pub const EC_NAMEDCURVE: Self = Self::from_bits_retain(CKF_EC_NAMEDCURVE);
}

/// Information about a particular mechanism
#[derive(Debug, Clone, Copy)]
pub struct MechanismInfo {
Expand Down Expand Up @@ -199,6 +205,7 @@ impl MechanismInfo {
/// [`ec_from_named_curve`](Self::ec_from_named_curve) must be `true`
#[deprecated = "use `ec_from_oid` instead"]
pub fn ec_from_named_curve(&self) -> bool {
#[allow(deprecated)]
self.flags.contains(MechanismInfoFlags::EC_NAMEDCURVE)
}

Expand Down Expand Up @@ -286,15 +293,24 @@ impl From<CK_MECHANISM_INFO> for MechanismInfo {
#[cfg(test)]
mod test {
use super::{MechanismInfo, MechanismInfoFlags};
use cryptoki_sys::CK_FLAGS;

#[test]
fn deprecated_flags() {
let ec_oid_bits: CK_FLAGS = MechanismInfoFlags::EC_OID.bits();
#[allow(deprecated)]
let ec_namedcurve_bits: CK_FLAGS = MechanismInfoFlags::EC_NAMEDCURVE.bits();
assert_eq!(ec_oid_bits, ec_namedcurve_bits);
}

#[test]
fn debug_flags_all() {
let expected = "\
HW | ENCRYPT | DECRYPT | DIGEST | SIGN | SIGN_RECOVER | VERIFY | \
VERIFY_RECOVER | GENERATE | GENERATE_KEY_PAIR | WRAP | UNWRAP | DERIVE | \
EXTENSION | EC_F_P | EC_F_2M | EC_ECPARAMETERS | EC_NAMEDCURVE | \
EC_OID | EC_UNCOMPRESS | EC_COMPRESS | MESSAGE_ENCRYPT | MESSAGE_DECRYPT | \
MULTI_MESSAGE";
let expected = "MechanismInfoFlags(
HW | ENCRYPT | DECRYPT | DIGEST | SIGN | SIGN_RECOVER | VERIFY | \
VERIFY_RECOVER | GENERATE | GENERATE_KEY_PAIR | WRAP | UNWRAP | DERIVE | \
EXTENSION | EC_F_P | EC_F_2M | EC_ECPARAMETERS | EC_OID | EC_UNCOMPRESS | \
EC_COMPRESS | MESSAGE_ENCRYPT | MESSAGE_DECRYPT | MULTI_MESSAGE,
)";
let all = MechanismInfoFlags::all();
let observed = format!("{all:#?}");
println!("{observed}");
Expand All @@ -311,7 +327,9 @@ MULTI_MESSAGE";
let expected = r#"MechanismInfo {
min_key_size: 16,
max_key_size: 4096,
flags: (empty),
flags: MechanismInfoFlags(
0x0,
),
}"#;
let observed = format!("{info:#?}");
assert_eq!(observed, expected);
Expand Down
2 changes: 1 addition & 1 deletion cryptoki/src/mechanism/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -823,7 +823,7 @@ impl TryFrom<CK_MECHANISM_TYPE> for MechanismType {
CKM_SP800_108_FEEDBACK_KDF => Ok(MechanismType::SP800_108_FEEDBACK_KDF),
CKM_SP800_108_DOUBLE_PIPELINE_KDF => Ok(MechanismType::SP800_108_DOUBLE_PIPELINE_KDF),
other => {
error!("Mechanism type {} is not supported.", other);
error!("Mechanism type {other} is not supported.");
Err(Error::NotSupported)
}
}
Expand Down
5 changes: 1 addition & 4 deletions cryptoki/src/mechanism/rsa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,7 @@ impl TryFrom<CK_RSA_PKCS_MGF_TYPE> for PkcsMgfType {
CKG_MGF1_SHA384 => Ok(PkcsMgfType::MGF1_SHA384),
CKG_MGF1_SHA512 => Ok(PkcsMgfType::MGF1_SHA512),
other => {
error!(
"Mask Generation Function type {} is not one of the valid values.",
other
);
error!("Mask Generation Function type {other} is not one of the valid values.");
Err(Error::InvalidValue)
}
}
Expand Down
8 changes: 4 additions & 4 deletions cryptoki/src/object.rs
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,7 @@ impl TryFrom<CK_ATTRIBUTE_TYPE> for AttributeType {
CKA_WRAP_WITH_TRUSTED => Ok(AttributeType::WrapWithTrusted),
CKA_VENDOR_DEFINED..=MAX_CU_ULONG => Ok(AttributeType::VendorDefined(attribute_type)),
attr_type => {
error!("Attribute type {} not supported.", attr_type);
error!("Attribute type {attr_type} not supported.");
Err(Error::NotSupported)
}
}
Expand Down Expand Up @@ -1088,7 +1088,7 @@ impl TryFrom<CK_OBJECT_CLASS> for ObjectClass {
CKO_OTP_KEY => Ok(ObjectClass::OTP_KEY),

_ => {
error!("Object class {} is not supported.", object_class);
error!("Object class {object_class} is not supported.");
Err(Error::NotSupported)
}
}
Expand Down Expand Up @@ -1373,7 +1373,7 @@ impl TryFrom<CK_KEY_TYPE> for KeyType {
CKK_HKDF => Ok(KeyType::HKDF),
CKK_VENDOR_DEFINED..=MAX_CU_ULONG => KeyType::new_vendor_defined(key_type),
_ => {
error!("Key type {} is not supported.", key_type);
error!("Key type {key_type} is not supported.");
Err(Error::NotSupported)
}
}
Expand Down Expand Up @@ -1449,7 +1449,7 @@ impl TryFrom<CK_CERTIFICATE_TYPE> for CertificateType {
CKC_X_509_ATTR_CERT => Ok(CertificateType::X_509_ATTR),
CKC_WTLS => Ok(CertificateType::WTLS),
_ => {
error!("Certificate type {} is not supported.", certificate_type);
error!("Certificate type {certificate_type} is not supported.");
Err(Error::NotSupported)
}
}
Expand Down
4 changes: 2 additions & 2 deletions cryptoki/src/session/object_management.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,8 +197,8 @@ impl Drop for ObjectHandleIterator<'_> {
if let Some(f) = get_pkcs11_func!(self.session.client(), C_FindObjectsFinal) {
// swallow the return value, as we can't do anything about it,
// but log the error
if let Rv::Error(error) = Rv::from(unsafe { f(self.session.handle()) }) {
log::error!("C_FindObjectsFinal() failed with error: {:?}", error);
if let Rv::Error(err) = Rv::from(unsafe { f(self.session.handle()) }) {
log::error!("C_FindObjectsFinal() failed with error: {err:?}");
}
} else {
// bark but pass if C_FindObjectsFinal() is not implemented
Expand Down
9 changes: 7 additions & 2 deletions cryptoki/src/session/session_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use std::fmt::Debug;

bitflags! {
/// Collection of flags defined for [`CK_SESSION_INFO`]
#[derive(Debug, Clone, Copy)]
struct SessionInfoFlags: CK_FLAGS {
const RW_SESSION = CKF_RW_SESSION;
const SERIAL_SESSION = CKF_SERIAL_SESSION;
Expand Down Expand Up @@ -109,7 +110,9 @@ mod test {

#[test]
fn debug_flags_all() {
let expected = "RW_SESSION | SERIAL_SESSION";
let expected = "SessionInfoFlags(
RW_SESSION | SERIAL_SESSION,
)";
let all = SessionInfoFlags::all();
let observed = format!("{all:#?}");
assert_eq!(observed, expected);
Expand All @@ -128,7 +131,9 @@ mod test {
slot_id: 100,
},
state: RoPublic,
flags: (empty),
flags: SessionInfoFlags(
0x0,
),
device_error: 0,
}"#;
let observed = format!("{info:#?}");
Expand Down
4 changes: 2 additions & 2 deletions cryptoki/src/session/session_management.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ impl Drop for Session {
}
}

if let Err(e) = close(self) {
error!("Failed to close session: {}", e);
if let Err(err) = close(self) {
error!("Failed to close session: {err}");
}
}
}
Expand Down
9 changes: 7 additions & 2 deletions cryptoki/src/slot/slot_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use std::fmt::Debug;

bitflags! {
/// Collection of flags defined for [`CK_SLOT_INFO`]
#[derive(Debug, Clone, Copy)]
struct SlotInfoFlags: CK_FLAGS {
const TOKEN_PRESENT = CKF_TOKEN_PRESENT;
const REMOVABLE_DEVICE = CKF_REMOVABLE_DEVICE;
Expand Down Expand Up @@ -98,7 +99,9 @@ mod test {

#[test]
fn debug_flags_all() {
let expected = "TOKEN_PRESENT | REMOVABLE_DEVICE | HW_SLOT";
let expected = "SlotInfoFlags(
TOKEN_PRESENT | REMOVABLE_DEVICE | HW_SLOT,
)";
let all = SlotInfoFlags::all();
let observed = format!("{all:#?}");
assert_eq!(observed, expected);
Expand All @@ -116,7 +119,9 @@ mod test {
let expected = r#"SlotInfo {
slot_description: "Slot Description",
manufacturer_id: "Manufacturer ID",
flags: (empty),
flags: SlotInfoFlags(
0x0,
),
hardware_version: Version {
major: 0,
minor: 255,
Expand Down
20 changes: 12 additions & 8 deletions cryptoki/src/slot/token_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use std::fmt::Debug;

bitflags! {
/// Collection of flags defined for [`CK_TOKEN_INFO`]
#[derive(Debug, Clone, Copy)]
struct TokenInfoFlags: CK_FLAGS {
const RNG = CKF_RNG;
const WRITE_PROTECTED = CKF_WRITE_PROTECTED;
Expand Down Expand Up @@ -463,13 +464,14 @@ mod test {

#[test]
fn debug_flags_all() {
let expected = "\
RNG | WRITE_PROTECTED | LOGIN_REQUIRED | USER_PIN_INITIALIZED | \
RESTORE_KEY_NOT_NEEDED | CLOCK_ON_TOKEN | PROTECTED_AUTHENTICATION_PATH | \
DUAL_CRYPTO_OPERATIONS | TOKEN_INITIALIZED | SECONDARY_AUTHENTICATION | \
USER_PIN_COUNT_LOW | USER_PIN_FINAL_TRY | USER_PIN_LOCKED | \
USER_PIN_TO_BE_CHANGED | SO_PIN_COUNT_LOW | SO_PIN_FINAL_TRY | SO_PIN_LOCKED | \
SO_PIN_TO_BE_CHANGED | ERROR_STATE";
let expected = "TokenInfoFlags(
RNG | WRITE_PROTECTED | LOGIN_REQUIRED | USER_PIN_INITIALIZED | \
RESTORE_KEY_NOT_NEEDED | CLOCK_ON_TOKEN | PROTECTED_AUTHENTICATION_PATH | \
DUAL_CRYPTO_OPERATIONS | TOKEN_INITIALIZED | SECONDARY_AUTHENTICATION | \
USER_PIN_COUNT_LOW | USER_PIN_FINAL_TRY | USER_PIN_LOCKED | \
USER_PIN_TO_BE_CHANGED | SO_PIN_COUNT_LOW | SO_PIN_FINAL_TRY | SO_PIN_LOCKED | \
SO_PIN_TO_BE_CHANGED | ERROR_STATE,
)";
let all = TokenInfoFlags::all();
let observed = format!("{all:#?}");
assert_eq!(observed, expected);
Expand Down Expand Up @@ -509,7 +511,9 @@ SO_PIN_TO_BE_CHANGED | ERROR_STATE";
manufacturer_id: "Manufacturer ID",
model: "Token Model",
serial_number: "Serial Number",
flags: (empty),
flags: TokenInfoFlags(
0x0,
),
max_session_count: Max(
100,
),
Expand Down
2 changes: 1 addition & 1 deletion cryptoki/tests/basic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1190,7 +1190,7 @@ fn get_session_info_test() -> TestResult {
let session_info = session.get_session_info()?;
assert!(session_info.read_write());
assert_eq!(session_info.slot_id(), slot);
assert!(matches!(session_info.session_state(), SessionState::RwUser,));
assert!(matches!(session_info.session_state(), SessionState::RwUser));
session.logout()?;
session.login(UserType::So, Some(&AuthPin::new(SO_PIN.into())))?;
let session_info = session.get_session_info()?;
Expand Down
Loading