Permit setting CKA_PRIVATE to CK_FALSE on PKCS#11 RSA public keys (#1019) (#1021)

* Add new PKCS#11 signer configuration setting 'anon_pubkey_access' (default false) (#1019).
* Use a type and allow the third possibility (token-default) also to be specified.
* Include advice to try changing pubkey_access when the signer registration underlying error is CKR_INCONSISTENT_TEMPLATE. Includes changes to gain access to the underlying error, and propagating the augmented error message (which was wrongly being dropped).
This commit is contained in:
Ximon Eighteen
2023-03-08 10:16:44 +01:00
committed by GitHub
parent 5379521523
commit b48ea6e287
7 changed files with 324 additions and 84 deletions
+53 -24
View File
@@ -183,33 +183,62 @@
# example when using SoftHSMv2 the library is commonly available at filesystem path
# /usr/lib/softhsm/libsofthsm2.so.
#
# Key Value Type Default Req'd Description
# Key Value Type Default Req'd Description
# ====================================================================================
# lib_path path string None Yes The path to the .so dynamic library
# file to load.
# slot integer or None Yes An integer PKCS#11 "slot" ID or a
# string string "slot" label. Can also be
# given in hexadecimal, e.g. 0x12AB.
# When a label is given Krill will
# inspect all available slots and use
# the first slot whose label matches.
# lib_path string None Yes The path to the .so dynamic library
# file to load.
# slot integer or None Yes An integer PKCS#11 "slot" ID or a
# string string "slot" label. Can also be
# given in hexadecimal, e.g. 0x12AB.
# When a label is given Krill will
# inspect all available slots and use
# the first slot whose label matches.
# ------------------------------------------------------------------------------------
# user_pin string None No The pin or password or secret value
# used to authenticate with the
# PKCS#11 provider. The format varies
# by provider, SoftHSMv2 uses numeric
# PINs such as "12345" while AWS
# CloudHSM expects this to be in the
# form "username:password".
# login boolean True No Whether the signer must be logged in
# to before performing other
# operations.
# user_pin string None No The pin or password or secret value
# used to authenticate with the
# PKCS#11 provider. The format varies
# by provider, SoftHSMv2 uses numeric
# PINs such as "12345" while AWS
# CloudHSM expects this to be in the
# form "username:password".
# login boolean true No Whether the signer must be logged
# in to before performing other
# operations.
# pubkey_access string *1 No Whether authentication should be
# required by HSM users to access
# RSA public keys generated by Krill.
#
# Can be one of the following:
# - "authenticated" (default *1)
# - "unauthenticated"
# - "token-default"
#
# Corresponds to setting the PKCS#11
# CKA_PRIVATE key attribute on the
# generated public key to CK_TRUE,
# CK_FALSE, or not specified (thus
# token specific behaviour).
#
# See section "4.4 Storage Objects"
# of the PKCS#11 v2.40 specification.
#
# Set this to "unauthenticated" or
# "token-default" if your HSM
# documentation states that the
# CKA_PRIVATE attribute on public
# keys may not be set to CK_TRUE, or
# if signer registration fails with
# error CKR_TEMPLATE_INCONSISTENT.
#
# Defaults to "authenticated" for
# backward compatibility with earlier
# versions of Krill.
# ------------------------------------------------------------------------------------
# retry_seconds integer 2 No Wait N seconds before retrying a
# failed request.
# backoff_multiplier float 1.5 No How much longer to wait before retry
# N+1 compared to retry N.
# max_retry_seconds integer 30 No Stop retrying after N seconds.
# retry_seconds integer 2 No Wait N seconds before retrying a
# failed request.
# backoff_multiplier float 1.5 No How much longer to wait before
# retry N+1 compared to retry N.
# max_retry_seconds integer 30 No Stop retrying after N seconds.
# KMIP signer configuration
+3
View File
@@ -9,3 +9,6 @@ pub use self::signing::*;
pub type SignerHandle = MyHandle;
pub type CryptoResult<T> = std::result::Result<T, self::error::Error>;
#[cfg(feature = "hsm")]
pub use self::signers::pkcs11::signer::PubKeyAccess;
@@ -242,7 +242,7 @@ enum IdentifyResult {
enum RegisterResult {
NotReady,
ReadyVerified(SignerHandle),
ReadyUnusable,
ReadyUnusable(String),
}
#[cfg(not(feature = "hsm"))]
@@ -337,16 +337,19 @@ impl SignerRouter {
// And remove it from the pending set
false
}
RegisterResult::ReadyUnusable => {
RegisterResult::ReadyUnusable(err) => {
// Signer registration failed, remove it from the pending set
warn!("Signer '{}' could not be registered: signer is not usable", signer_name);
error!(
"Signer '{}' could not be registered: signer is not usable: {}",
signer_name, err
);
false
}
})
}
IdentifyResult::Unusable => {
// Signer is ready and unusable, remove it from the pending set
warn!("Signer '{}' could not be identified: signer is not usable", signer_name);
error!("Signer '{}' could not be identified: signer is not usable", signer_name);
Ok(false)
}
IdentifyResult::Corrupt => {
@@ -544,20 +547,19 @@ impl SignerRouter {
let (public_key, signer_private_key_id) = match signer_provider.create_registration_key() {
Err(SignerError::TemporarilyUnavailable) => return Ok(RegisterResult::NotReady),
Err(_) => return Ok(RegisterResult::ReadyUnusable),
Err(err) => return Ok(RegisterResult::ReadyUnusable(err.to_string())),
Ok(res) => res,
};
let challenge = "Krill signer verification challenge".as_bytes();
let signature = match signer_provider.sign_registration_challenge(&signer_private_key_id, challenge) {
Err(SignerError::TemporarilyUnavailable) => return Ok(RegisterResult::NotReady),
Err(_) => return Ok(RegisterResult::ReadyUnusable),
Err(err) => return Ok(RegisterResult::ReadyUnusable(err.to_string())),
Ok(res) => res,
};
if public_key.verify(challenge, &signature).is_err() {
error!("Signer '{}' challenge signature is invalid", signer_name);
return Ok(RegisterResult::ReadyUnusable);
return Ok(RegisterResult::ReadyUnusable(format!("Challenge signature is invalid")));
}
debug!("Signer '{}' is ready and new, binding", signer_name);
+1 -1
View File
@@ -39,7 +39,7 @@ impl fmt::Display for SignerError {
SignerError::OpenSslError(e) => write!(f, "OpenSSL Error: {}", e),
SignerError::Other(e) => write!(f, "Signer error: {}", e),
SignerError::PermanentlyUnusable => write!(f, "Signer is unusable"),
SignerError::Pkcs11Error(e) => write!(f, "PKCS#11 Error: {}", e),
SignerError::Pkcs11Error(e) => write!(f, "{}", e), // Cryptoki prefixes e with "PKCS11 error"
SignerError::TemporarilyUnavailable => write!(f, "Signer is unavailable"),
SignerError::UnsupportedSigningAlg(key_format) => match key_format {
SigningAlgorithm::RsaSha256 => write!(f, "Signing with RSA not supported"),
@@ -11,7 +11,7 @@ use backoff::ExponentialBackoff;
use bytes::Bytes;
use cryptoki::{
context::Info,
error::Error as Pkcs11Error,
error::{Error as Pkcs11Error, RvError},
mechanism::Mechanism,
object::{Attribute, AttributeType, ObjectClass, ObjectHandle},
session::UserType,
@@ -42,6 +42,23 @@ use crate::commons::crypto::{
use serde::{de::Visitor, Deserialize};
/// How should public key access be controlled?
/// See: http://docs.oasis-open.org/pkcs11/pkcs11-base/v2.40/os/pkcs11-base-v2.40-os.html#_Toc416959705
#[derive(Clone, Copy, Debug, Deserialize, PartialEq)]
pub enum PubKeyAccess {
/// User may not access the object until the user has been authenticated to the token.
#[serde(alias = "authenticated")]
Authenticated,
/// User may access the object without having been authenticated to the token.
#[serde(alias = "unauthenticated")]
Unauthenticated,
/// Default value is token-specific, and may depend on the values of other attributes of the object.
#[serde(alias = "token-default")]
TokenDefault,
}
#[derive(Clone, Debug, Deserialize, PartialEq)]
pub struct Pkcs11SignerConfig {
pub lib_path: String,
@@ -54,6 +71,9 @@ pub struct Pkcs11SignerConfig {
#[serde(default = "Pkcs11SignerConfig::default_login")]
pub login: bool,
#[serde(default = "Pkcs11SignerConfig::default_pubkey_access")]
pub pubkey_access: PubKeyAccess,
#[serde(default = "Pkcs11SignerConfig::default_retry_seconds")]
pub retry_seconds: u64,
@@ -69,6 +89,10 @@ impl Pkcs11SignerConfig {
true
}
pub fn default_pubkey_access() -> PubKeyAccess {
PubKeyAccess::Authenticated
}
pub fn default_retry_seconds() -> u64 {
2
}
@@ -171,6 +195,8 @@ pub struct Pkcs11Signer {
/// A probe dependent interface to the PKCS#11 server.
server: Arc<StatefulProbe<ConnectionSettings, SignerError, UsableServerState>>,
pubkey_access: PubKeyAccess,
}
impl Pkcs11Signer {
@@ -209,6 +235,7 @@ impl Pkcs11Signer {
handle: RwLock::new(None),
mapper,
server,
pubkey_access: conf.pubkey_access,
};
Ok(s)
@@ -236,8 +263,22 @@ impl Pkcs11Signer {
}
pub fn create_registration_key(&self) -> Result<(PublicKey, String), SignerError> {
let (public_key, _, _, internal_key_id) = self.build_key(PublicKeyFormat::Rsa)?;
Ok((public_key, internal_key_id))
match self.build_key_internal(PublicKeyFormat::Rsa) {
Ok((public_key, _, _, internal_key_id)) => Ok((public_key, internal_key_id)),
Err(err @ InternalConnError::Pkcs11Error(Pkcs11Error::Pkcs11(RvError::TemplateInconsistent))) => {
// https://github.com/NLnetLabs/krill/issues/1019
let err_msg = format!(
"{} [Note: This error can occur if the signer does not support authenticated \
access to public keys. Setting `pubkey_access` in krill.conf to \"token-default\"` or \
`\"unauthenticated\"` may help]",
err
);
Err(SignerError::Pkcs11Error(err_msg))
}
Err(err) => Err(err.into()),
}
}
pub fn sign_registration_challenge<D: AsRef<[u8]> + ?Sized>(
@@ -584,7 +625,7 @@ impl Pkcs11Signer {
impl Pkcs11Signer {
/// Get a connection to the server, if the server is usable.
fn connect(&self) -> Result<Pkcs11Session, SignerError> {
fn connect(&self) -> Result<Pkcs11Session, InternalConnError> {
let conn = self.server.status(Self::probe_server)?.state()?.get_connection()?;
Ok(conn)
}
@@ -593,7 +634,7 @@ impl Pkcs11Signer {
///
/// Fails if the PKCS#11 server is not [Usable]. If the operation fails due to a transient connection error, retry
/// with backoff upto a defined retry limit.
fn with_conn<T, F>(&self, desc: &str, mut do_something_with_conn: F) -> Result<T, SignerError>
fn with_conn<T, F>(&self, desc: &str, mut do_something_with_conn: F) -> Result<T, InternalConnError>
where
F: FnMut(&Pkcs11Session) -> Result<T, Pkcs11Error>,
{
@@ -613,7 +654,7 @@ impl Pkcs11Signer {
// Define an operation to (re)try
let op = || {
// First get a (possibly already existing) connection from the pool
let conn = self.connect().map_err(retry_on_transient_signer_error)?;
let conn = self.connect().map_err(retry_on_transient_error)?;
// Next, try to execute the callers operation using the connection. If it fails, examine the cause of
// failure to determine if it should be a hard-fail (no more retries) or if we should try again.
@@ -676,6 +717,13 @@ impl Pkcs11Signer {
&self,
algorithm: PublicKeyFormat,
) -> Result<(PublicKey, ObjectHandle, ObjectHandle, String), SignerError> {
Ok(self.build_key_internal(algorithm)?)
}
fn build_key_internal(
&self,
algorithm: PublicKeyFormat,
) -> Result<(PublicKey, ObjectHandle, ObjectHandle, String), InternalConnError> {
// https://tools.ietf.org/html/rfc6485#section-3: Asymmetric Key Pair Formats
// "The RSA key pairs used to compute the signatures MUST have a 2048-bit
// modulus and a public exponent (e) of 65,537."
@@ -684,7 +732,7 @@ impl Pkcs11Signer {
return Err(SignerError::Pkcs11Error(format!(
"Algorithm {:?} not supported while creating key",
&algorithm
)));
)))?;
}
let mech = Mechanism::RsaPkcsKeyPairGen;
@@ -693,29 +741,7 @@ impl Pkcs11Signer {
openssl::rand::rand_bytes(&mut cka_id)
.map_err(|_| SignerError::Pkcs11Error("Internal error while generating a random number".to_string()))?;
let pub_template = vec![
Attribute::Id(cka_id.to_vec()),
Attribute::Verify(true),
Attribute::Encrypt(false),
Attribute::Wrap(false),
Attribute::Token(true),
Attribute::Private(true),
Attribute::ModulusBits(2048.into()),
Attribute::PublicExponent(vec![0x01, 0x00, 0x01]),
Attribute::Label("Krill".to_string().into_bytes()),
];
let priv_template = vec![
Attribute::Id(cka_id.to_vec()),
Attribute::Sign(true),
Attribute::Decrypt(false),
Attribute::Unwrap(false),
Attribute::Sensitive(true),
Attribute::Token(true),
Attribute::Private(true),
Attribute::Extractable(false),
Attribute::Label("Krill".to_string().into_bytes()),
];
let (pub_template, priv_template) = self.mk_keygen_templates(&cka_id);
let (pub_handle, priv_handle) = self.with_conn("generate key pair", |conn| {
// The Krill functional test once failed under GitHub Actions with error:
@@ -734,6 +760,47 @@ impl Pkcs11Signer {
Ok((public_key, pub_handle, priv_handle, hex::encode(cka_id)))
}
fn mk_keygen_templates(&self, cka_id: &[u8]) -> (Vec<Attribute>, Vec<Attribute>) {
let mut pub_template = vec![
Attribute::Id(cka_id.to_vec()),
Attribute::Verify(true),
Attribute::Encrypt(false),
Attribute::Wrap(false),
Attribute::Token(true),
Attribute::ModulusBits(2048.into()),
Attribute::PublicExponent(vec![0x01, 0x00, 0x01]),
Attribute::Label("Krill".to_string().into_bytes()),
];
// https://github.com/NLnetLabs/krill/issues/1019
match self.pubkey_access {
PubKeyAccess::Authenticated => {
pub_template.push(Attribute::Private(true));
}
PubKeyAccess::Unauthenticated => {
pub_template.push(Attribute::Private(false));
}
PubKeyAccess::TokenDefault => {
// Do not supply a value for the CKA_PRIVATE attribute.
}
}
let priv_template = vec![
Attribute::Id(cka_id.to_vec()),
Attribute::Sign(true),
Attribute::Decrypt(false),
Attribute::Unwrap(false),
Attribute::Sensitive(true),
Attribute::Token(true),
Attribute::Private(true),
Attribute::Extractable(false),
Attribute::Modifiable(false),
Attribute::Label("Krill".to_string().into_bytes()),
];
(pub_template, priv_template)
}
pub(super) fn get_public_key_from_handle(&self, pub_handle: ObjectHandle) -> Result<PublicKey, SignerError> {
let res = self.with_conn("get key pair parts", |conn| {
conn.get_attributes(pub_handle, &[AttributeType::Modulus, AttributeType::PublicExponent])
@@ -814,14 +881,16 @@ impl Pkcs11Signer {
let cka_id = hex::decode(cka_id_hex_str).map_err(|_| KeyError::Signer(SignerError::DecodeError))?;
let results = self.with_conn("find key", |conn| {
// Find at most one result that matches the given key class (public or private) and the given PKCS#11
// CKA_ID bytes.
let results = self
.with_conn("find key", |conn| {
// Find at most one result that matches the given key class (public or private) and the given PKCS#11
// CKA_ID bytes.
// A PKCS#11 session can have at most one active search operation at a time. A search must be initialized,
// results fetched, and then finalized, only then can the session perform another search.
conn.find_objects(&[Attribute::Class(key_class), Attribute::Id(cka_id.clone())])
})?;
// A PKCS#11 session can have at most one active search operation at a time. A search must be initialized,
// results fetched, and then finalized, only then can the session perform another search.
conn.find_objects(&[Attribute::Class(key_class), Attribute::Id(cka_id.clone())])
})
.map_err(SignerError::from)?;
match results.len() {
0 => Err(KeyError::KeyNotFound),
@@ -835,7 +904,7 @@ impl Pkcs11Signer {
pub(super) fn destroy_key_by_handle(&self, key_handle: ObjectHandle) -> Result<(), SignerError> {
trace!("[{}] Destroying key with PKCS#11 handle {}", self.name, key_handle);
self.with_conn("destroy", |conn| conn.destroy_object(key_handle))
Ok(self.with_conn("destroy", |conn| conn.destroy_object(key_handle))?)
}
}
@@ -957,7 +1026,52 @@ impl Pkcs11Signer {
// Retry with backoff related helper impls/fns:
// --------------------------------------------------------------------------------------------------------------------
fn retry_on_transient_pkcs11_error(err: Pkcs11Error) -> backoff::Error<SignerError> {
#[derive(Debug)]
enum InternalConnError {
Pkcs11Error(Pkcs11Error),
SignerError(SignerError),
}
impl std::fmt::Display for InternalConnError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
InternalConnError::Pkcs11Error(v) => v.fmt(f),
InternalConnError::SignerError(v) => v.fmt(f),
}
}
}
impl From<Pkcs11Error> for InternalConnError {
fn from(v: Pkcs11Error) -> Self {
InternalConnError::Pkcs11Error(v)
}
}
impl From<SignerError> for InternalConnError {
fn from(v: SignerError) -> Self {
InternalConnError::SignerError(v)
}
}
impl From<InternalConnError> for SignerError {
fn from(v: InternalConnError) -> Self {
match v {
InternalConnError::Pkcs11Error(v) => SignerError::Pkcs11Error(v.to_string()),
InternalConnError::SignerError(v) => v,
}
}
}
impl From<backoff::Error<InternalConnError>> for InternalConnError {
fn from(v: backoff::Error<InternalConnError>) -> Self {
match v {
backoff::Error::Permanent(err) => err,
backoff::Error::Transient(err) => err,
}
}
}
fn retry_on_transient_pkcs11_error(err: Pkcs11Error) -> backoff::Error<InternalConnError> {
if is_transient_error(&err) {
backoff::Error::Transient(err.into())
} else {
@@ -965,10 +1079,17 @@ fn retry_on_transient_pkcs11_error(err: Pkcs11Error) -> backoff::Error<SignerErr
}
}
fn retry_on_transient_signer_error(err: SignerError) -> backoff::Error<SignerError> {
fn retry_on_transient_signer_error(err: SignerError) -> backoff::Error<InternalConnError> {
match err {
SignerError::TemporarilyUnavailable => backoff::Error::Transient(err),
_ => backoff::Error::Permanent(err),
SignerError::TemporarilyUnavailable => backoff::Error::Transient(err.into()),
_ => backoff::Error::Permanent(err.into()),
}
}
fn retry_on_transient_error(err: InternalConnError) -> backoff::Error<InternalConnError> {
match err {
InternalConnError::Pkcs11Error(err) => retry_on_transient_pkcs11_error(err),
InternalConnError::SignerError(err) => retry_on_transient_signer_error(err),
}
}
@@ -1121,16 +1242,9 @@ impl From<Pkcs11Error> for SignerError {
}
}
impl From<ProbeError<SignerError>> for SignerError {
impl From<ProbeError<SignerError>> for InternalConnError {
fn from(err: ProbeError<SignerError>) -> Self {
match err {
ProbeError::WrongState => {
SignerError::Other("Internal error: probe is not in the expected state".to_string())
}
ProbeError::AwaitingNextProbe => SignerError::TemporarilyUnavailable,
ProbeError::CompletedUnusable => SignerError::PermanentlyUnusable,
ProbeError::CallbackFailed(err) => err,
}
err.into()
}
}
@@ -1189,6 +1303,8 @@ where
#[cfg(test)]
mod tests {
use crate::test;
use super::*;
#[test]
@@ -1224,4 +1340,77 @@ mod tests {
"not a valid PKCS#11 slot ID for key `slot` at line 3 column 20"
)
}
#[test]
fn test_pubkey_access() {
// Default behaviour for backward compatibility should be that the lack of the new config setting is equivalent
// to specifying that the public key generation template should have Attribute::Private(true).
with_pubkey_access(None, Some(true));
// Which should be the same as using the new config setting with value false.
with_pubkey_access(Some(PubKeyAccess::Authenticated), Some(true));
// Or we can explicity request unauthenticated access which should cause the public key generation template to
// contain a single occurence of Attribute::Private(false).
with_pubkey_access(Some(PubKeyAccess::Unauthenticated), Some(false));
// Or we can request the token default access control behaviour which should result in there NOT being any
// occurences of Attribute::Private(_) in the public key generation template.
with_pubkey_access(Some(PubKeyAccess::TokenDefault), None);
}
fn with_pubkey_access(flag: Option<PubKeyAccess>, expected_attr: Option<bool>) {
test::test_under_tmp(|d| {
let mut config_str = r#"
lib_path = "dummy path"
slot = 1234
"#
.to_string();
match flag {
Some(PubKeyAccess::Authenticated) => {
config_str.push_str("pubkey_access = \"authenticated\"");
}
Some(PubKeyAccess::Unauthenticated) => {
config_str.push_str("pubkey_access = \"unauthenticated\"");
}
Some(PubKeyAccess::TokenDefault) => {
config_str.push_str("pubkey_access = \"token-default\"");
}
None => {
// don't add any configuration setting to the config file
}
}
let config: Pkcs11SignerConfig = toml::from_str(&config_str).unwrap();
let mapper = Arc::new(SignerMapper::build(&d).unwrap());
let signer = Pkcs11Signer::build("dummy", &config, Duration::from_secs(600), mapper).unwrap();
let (pub_template, priv_template) = signer.mk_keygen_templates(&[0, 0, 0]);
assert_eq!(
1,
priv_template
.iter()
.filter(|attr| matches!(attr, Attribute::Private(true)))
.count()
);
match expected_attr {
Some(expected_v) => {
// The the set of public key template attributes should contain a single
// occurence of Attribute::Private with inner value expected_v.
let count = pub_template
.iter()
.filter(|attr| matches!(attr, Attribute::Private(v) if *v == expected_v))
.count();
assert_eq!(count, 1);
}
None => {
// The public key template attributes should NOT contain any occurences of
// Attribute::Private(_).
assert!(!pub_template.iter().any(|attr| matches!(attr, Attribute::Private(_))));
}
}
});
}
}
@@ -4,6 +4,8 @@ use std::{
time::{Duration, Instant},
};
use super::error::SignerError;
#[derive(Debug)]
pub enum ProbeError<E> {
WrongState,
@@ -229,6 +231,19 @@ impl<C, E, S> StatefulProbe<C, E, S> {
}
}
impl From<ProbeError<SignerError>> for SignerError {
fn from(err: ProbeError<SignerError>) -> Self {
match err {
ProbeError::WrongState => {
SignerError::Other("Internal error: probe is not in the expected state".to_string())
}
ProbeError::AwaitingNextProbe => SignerError::TemporarilyUnavailable,
ProbeError::CompletedUnusable => SignerError::PermanentlyUnusable,
ProbeError::CallbackFailed(err) => err,
}
}
}
#[cfg(test)]
pub mod tests {
use std::time::Duration;
+2
View File
@@ -290,12 +290,14 @@ impl ConfigDefaults {
#[cfg(all(feature = "hsm-tests-pkcs11", not(feature = "hsm-tests-kmip")))]
{
use crate::commons::crypto::PubKeyAccess;
use crate::commons::crypto::SlotIdOrLabel;
let signer_config = Pkcs11SignerConfig {
lib_path: "/usr/lib/softhsm/libsofthsm2.so".to_string(),
user_pin: Some("1234".to_string()),
slot: SlotIdOrLabel::Label("My token 1".to_string()),
login: true,
pubkey_access: PubKeyAccess::Authenticated,
retry_seconds: Pkcs11SignerConfig::default_retry_seconds(),
backoff_multiplier: Pkcs11SignerConfig::default_backoff_multiplier(),
max_retry_seconds: Pkcs11SignerConfig::default_max_retry_seconds(),