Skip to content

Support for EC keys, fix Attributes processing #369

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.

Sign up for GitHub

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
ya-mouse wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Support for EC keys, fix Attributes processing #369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions native-pkcs11-core/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ license.workspace = true
[dependencies]
der = "0.7.9"
native-pkcs11-traits = { version = "0.2.0", path = "../native-pkcs11-traits" }
spki = "0.7"
pkcs1 = { version = "0.7.5", default-features = false }
pkcs11-sys = { version = "0.2.24", path = "../pkcs11-sys" }
pkcs8 = "0.10.2"
Expand Down
128 changes: 88 additions & 40 deletions native-pkcs11-core/src/object.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@

use std::{ffi::CString, fmt::Debug, sync::Arc};

use der::{asn1::OctetString, Encode};
use der::{
asn1::{ObjectIdentifier, OctetString},
Encode,
};
use native_pkcs11_traits::{
backend,
Certificate,
Expand All @@ -35,13 +38,11 @@ use pkcs11_sys::{
CK_CERTIFICATE_CATEGORY_UNSPECIFIED,
CK_PROFILE_ID,
};
use spki::SubjectPublicKeyInfoRef;
use tracing::debug;

use crate::attribute::{Attribute, AttributeType, Attributes};

const P256_OID: pkcs8::ObjectIdentifier =
pkcs8::ObjectIdentifier::new_unwrap("1.2.840.10045.3.1.7");

#[derive(Debug)]
pub struct DataObject {
pub application: CString,
Expand Down Expand Up @@ -74,6 +75,33 @@ impl Clone for Object {
}
}

fn extract_ec_params(der_bytes: &[u8]) -> Option<(Vec<u8>, Vec<u8>)> {
let spki: SubjectPublicKeyInfoRef<'_> = SubjectPublicKeyInfoRef::try_from(der_bytes).unwrap();
// For EC keys, the algorithm parameters contain the curve OID
// For EC keys, the subject public key is the EC point
Some((
ObjectIdentifier::from_bytes(spki.algorithm.parameters.unwrap().value())
Copy link
Collaborator

@brandonweeks brandonweeks Feb 20, 2025

Choose a reason for hiding this comment

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

spki.algorithm.parameters_oid().unwrap().to_der().unwrap() seems to do the same thing.

.unwrap()
.to_der()
.unwrap(),
OctetString::new(spki.subject_public_key.raw_bytes()).unwrap().to_der().unwrap(),
Copy link
Collaborator

@brandonweeks brandonweeks Feb 20, 2025

Choose a reason for hiding this comment

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

spki.subject_public_key.to_der().unwrap() also seems like an easier way to get the key as DER encoded unless I'm missing something.

))
}

fn extract_rsa_params(der_bytes: &[u8]) -> Option<(Vec<u8>, Vec<u8>, u64)> {
Copy link
Collaborator

@brandonweeks brandonweeks Feb 20, 2025

Choose a reason for hiding this comment

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

Nit: return a usize from the method and do the type casting further down.

// Parse the DER-encoded SPKI
let spki: SubjectPublicKeyInfoRef<'_> = SubjectPublicKeyInfoRef::try_from(der_bytes).unwrap();
// Parse the RSA public key bytes from the SPKI
let rsa_pubkey = RsaPublicKey::from_der(spki.subject_public_key.raw_bytes()).ok()?;
Copy link
Collaborator

@brandonweeks brandonweeks Feb 20, 2025

Choose a reason for hiding this comment

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

RsaPublicKey::from_der(spki.subject_public_key.as_bytes()?).ok()? works as well.

let modulus = rsa_pubkey.modulus.as_bytes();
Copy link
Collaborator

@brandonweeks brandonweeks Feb 20, 2025

Choose a reason for hiding this comment

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

Nit: call rsa_pubkey.modulus.as_bytes().to_vec() in the struct constructor instead of assigning to a variable.


Some((
modulus.to_vec(),
rsa_pubkey.public_exponent.as_bytes().to_vec(),
(modulus.len() * 8) as u64,
))
}

impl Object {
pub fn attribute(&self, type_: AttributeType) -> Option<Attribute> {
match self {
Expand Down Expand Up @@ -102,7 +130,19 @@ impl Object {
AttributeType::Class => Some(Attribute::Class(CKO_PRIVATE_KEY)),
AttributeType::Decrypt => Some(Attribute::Decrypt(false)),
AttributeType::Derive => Some(Attribute::Derive(false)),
AttributeType::EcParams => Some(Attribute::EcParams(P256_OID.to_der().ok()?)),
AttributeType::EcParams | AttributeType::EcPoint => {
if private_key.algorithm() != KeyAlgorithm::Ecc {
return None;
}
private_key.find_public_key(backend()).ok().flatten().and_then(|public_key| {
let der_bytes = public_key.to_der();
extract_ec_params(&der_bytes).map(|(params, point)| match type_ {
AttributeType::EcParams => Attribute::EcParams(params),
AttributeType::EcPoint => Attribute::EcPoint(point),
_ => unreachable!(),
})
})
}
AttributeType::Extractable => Some(Attribute::Extractable(false)),
AttributeType::Id => Some(Attribute::Id(private_key.public_key_hash())),
AttributeType::KeyType => Some(Attribute::KeyType(match private_key.algorithm() {
Expand All @@ -111,31 +151,28 @@ impl Object {
})),
AttributeType::Label => Some(Attribute::Label(private_key.label())),
AttributeType::Local => Some(Attribute::Local(false)),
AttributeType::Modulus => {
let modulus = private_key.find_public_key(backend()).ok().flatten().and_then(
|public_key| {
let der = public_key.to_der();
RsaPublicKey::from_der(&der)
.map(|pk| pk.modulus.as_bytes().to_vec())
.ok()
},
);
modulus.map(Attribute::Modulus)
AttributeType::Modulus
| AttributeType::ModulusBits
| AttributeType::PublicExponent => {
if private_key.algorithm() != KeyAlgorithm::Rsa {
return None;
}
private_key.find_public_key(backend()).ok().flatten().and_then(|public_key| {
let der_bytes = public_key.to_der();
extract_rsa_params(&der_bytes).map(
|(modulus, exponent, bits)| match type_ {
AttributeType::Modulus => Attribute::Modulus(modulus),
AttributeType::ModulusBits => Attribute::ModulusBits(bits),
AttributeType::PublicExponent => {
Attribute::PublicExponent(exponent)
}
_ => unreachable!(),
},
)
})
}
AttributeType::NeverExtractable => Some(Attribute::NeverExtractable(true)),
AttributeType::Private => Some(Attribute::Private(true)),
AttributeType::PublicExponent => {
let public_exponent =
private_key.find_public_key(backend()).ok().flatten().and_then(
|public_key| {
let der = public_key.to_der();
RsaPublicKey::from_der(&der)
.map(|pk| pk.public_exponent.as_bytes().to_vec())
.ok()
},
);
public_exponent.map(Attribute::PublicExponent)
}
AttributeType::Sensitive => Some(Attribute::Sensitive(true)),
AttributeType::Sign => Some(Attribute::Sign(true)),
AttributeType::SignRecover => Some(Attribute::SignRecover(false)),
Expand All @@ -157,32 +194,43 @@ impl Object {
},
Object::PublicKey(pk) => match type_ {
AttributeType::Class => Some(Attribute::Class(CKO_PUBLIC_KEY)),
AttributeType::Verify => Some(Attribute::Verify(true)),
AttributeType::VerifyRecover => Some(Attribute::VerifyRecover(false)),
AttributeType::Wrap => Some(Attribute::Wrap(false)),
AttributeType::Encrypt => Some(Attribute::Encrypt(false)),
Copy link
Author

@ya-mouse ya-mouse Nov 22, 2024

Choose a reason for hiding this comment

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

Added some missed attributes with default values that pkcs11-tool/p11-kit tools complains about.

Copy link
Collaborator

@brandonweeks brandonweeks Feb 20, 2025

Choose a reason for hiding this comment

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

Apologies for the churn but can you also make this change in a separate PR? I'm working on adding integration tests for pkcs11-tool output I would like to use to validate this change.

AttributeType::Derive => Some(Attribute::Derive(false)),
AttributeType::Label => Some(Attribute::Label(pk.label())),
AttributeType::Local => Some(Attribute::Local(false)),
AttributeType::Modulus => {
let key = pk.to_der();
let key = RsaPublicKey::from_der(&key).unwrap();
Some(Attribute::Modulus(key.modulus.as_bytes().to_vec()))
}
AttributeType::PublicExponent => {
let key = pk.to_der();
let key = RsaPublicKey::from_der(&key).unwrap();
Some(Attribute::Modulus(key.public_exponent.as_bytes().to_vec()))
AttributeType::Modulus
| AttributeType::ModulusBits
| AttributeType::PublicExponent => {
if pk.algorithm() != KeyAlgorithm::Rsa {
return None;
}
let der_bytes = pk.to_der();
extract_rsa_params(&der_bytes).map(|(modulus, exponent, bits)| match type_ {
AttributeType::Modulus => Attribute::Modulus(modulus),
AttributeType::ModulusBits => Attribute::ModulusBits(bits),
AttributeType::PublicExponent => Attribute::PublicExponent(exponent),
_ => unreachable!(),
})
}
AttributeType::KeyType => Some(Attribute::KeyType(match pk.algorithm() {
native_pkcs11_traits::KeyAlgorithm::Rsa => CKK_RSA,
native_pkcs11_traits::KeyAlgorithm::Ecc => CKK_EC,
})),
AttributeType::Id => Some(Attribute::Id(pk.public_key_hash())),
AttributeType::EcPoint => {
AttributeType::EcParams | AttributeType::EcPoint => {
if pk.algorithm() != KeyAlgorithm::Ecc {
return None;
}
let wrapped = OctetString::new(pk.to_der()).ok()?;
Some(Attribute::EcPoint(wrapped.to_der().ok()?))
let der_bytes = pk.to_der();
extract_ec_params(&der_bytes).map(|(params, point)| match type_ {
AttributeType::EcParams => Attribute::EcParams(params),
AttributeType::EcPoint => Attribute::EcPoint(point),
_ => unreachable!(),
})
}
AttributeType::EcParams => Some(Attribute::EcParams(P256_OID.to_der().ok()?)),
_ => {
debug!("public_key: type_ unimplemented: {:?}", type_);
None
Expand Down
2 changes: 1 addition & 1 deletion native-pkcs11-traits/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use std::{
use x509_cert::der::Decode;

pub type Result<T> = std::result::Result<T, Box<dyn std::error::Error>>;
pub type Digest = [u8; 20];
pub type Digest = [u8; 64];
Copy link
Author

@ya-mouse ya-mouse Nov 22, 2024

Choose a reason for hiding this comment

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

was small to fit ECC P-256 digest value


// The Backend is first staged so it can be stored in a Box<dyn Backend>. This
// allows the Backend to be reference with `&'static`.
Expand Down
18 changes: 13 additions & 5 deletions native-pkcs11/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ use std::{
};

use native_pkcs11_core::{
attribute::{Attribute, Attributes},
attribute::{Attribute, AttributeType, Attributes},
mechanism::{parse_mechanism, SUPPORTED_SIGNATURE_MECHANISMS},
object::{self, Object},
};
Expand Down Expand Up @@ -589,10 +589,18 @@ cryptoki_fn!(
};
let mut result = Ok(());
for attribute in template.iter_mut() {
let type_ = attribute
.type_
.try_into()
.map_err(|_| Error::AttributeTypeInvalid(attribute.type_))?;
let type_ = match AttributeType::try_from(attribute.type_) {
Ok(t) => t,
Err(_) => {
tracing::debug!(
type_ = ?attribute.type_,
"Unsupported attribute type - marking as unavailable"
);
attribute.ulValueLen = CK_UNAVAILABLE_INFORMATION;
// result = Err(Error::AttributeTypeInvalid(attribute.type_));
Copy link
Author

@ya-mouse ya-mouse Nov 22, 2024

Choose a reason for hiding this comment

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

we should skip unknown attributes otherwise a tool that handles pkcs11 calls silently tries to interpret random values from the template: pkcs11-tool and p11-kit produces weird values for attributes when inspecting with pkcs11-spy.

Copy link
Collaborator

@brandonweeks brandonweeks Jan 7, 2025

Choose a reason for hiding this comment

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

Can you please split the bug fix for attribute parsing into a separate PR?

Copy link
Author

@ya-mouse ya-mouse Jan 14, 2025

Choose a reason for hiding this comment

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

#389
#390

Copy link
Author

@ya-mouse ya-mouse Feb 13, 2025

Choose a reason for hiding this comment

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

Gently ping :)

continue;
}
};
if let Some(value) = object.attribute(type_) {
let value = value.as_raw_value();
attribute.ulValueLen = value.len() as CK_ULONG;
Expand Down
4 changes: 2 additions & 2 deletions native-pkcs11/src/object_store.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,8 @@ impl ObjectStore {
}

fn find_with_backend_all_public_keys(&mut self) -> Result<()> {
for private_key in backend().find_all_private_keys()? {
self.insert(Object::PrivateKey(private_key));
for public_key in backend().find_all_public_keys()? {
self.insert(Object::PublicKey(public_key));
}
Ok(())
}
Expand Down
Loading