Skip to content
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

Jump to bottom

Make sure Cargo features work #76

Merged
merged 1 commit into from
Dec 9, 2019
Merged

Make sure Cargo features work #76

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
4 changes: 2 additions & 2 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,6 @@ serde = { version = "1.0", features = ["derive"] }
mbed-crypto-version = "mbedcrypto-2.0.0"

[features]
default = ["mbed", "pkcs11-provider"]
mbed = []
default = ["mbed-crypto-provider", "pkcs11-provider"]
mbed-crypto-provider = []
pkcs11-provider = ["pkcs11", "serde_asn1_der"]
2 changes: 1 addition & 1 deletion build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ fn main() {
.expect("Cargo.toml does not contain package metadata.");
let parsec_config = get_value_from_table(&metadata, CONFIG_TABLE_NAME);

if cfg!(feature = "mbed") {
if cfg!(feature = "mbed-crypto-provider") {
let mbed_config = config.mbed_config.expect(&format!(
"Could not find mbed_config table in the {} file.",
BUILD_CONFIG_FILE_PATH
Expand Down
11 changes: 8 additions & 3 deletions src/bin/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
use log::info;
use parsec::utils::{ServiceBuilder, ServiceConfig};
use signal_hook::{flag, SIGHUP, SIGTERM};
use std::io::Error;
use std::io::{Error, ErrorKind};
use std::sync::{
atomic::{AtomicBool, Ordering},
Arc,
Expand All @@ -42,10 +42,12 @@ fn main() -> Result<(), Error> {

info!("Parsec started. Configuring the service...");

let front_end_handler = ServiceBuilder::build_service(&config)
.ok_or_else(|| Error::new(ErrorKind::Other, "Parsec can not be configured."))?;
// Multiple threads can not just have a reference of the front end handler because they could
// outlive the run function. It is needed to give them all ownership of the front end handler
// through an Arc.
let mut front_end_handler = Arc::from(ServiceBuilder::build_service(&config));
let mut front_end_handler = Arc::from(front_end_handler);
let mut listener = ServiceBuilder::start_listener(&config.listener);
let mut threadpool = ServiceBuilder::build_threadpool(config.core_settings.thread_pool_size);

Expand All @@ -71,7 +73,10 @@ fn main() -> Result<(), Error> {
config_file = ::std::fs::read_to_string(CONFIG_FILE_PATH)
.expect("Failed to read configuration file");
config = toml::from_str(&config_file).expect("Failed to parse service configuration");
front_end_handler = Arc::from(ServiceBuilder::build_service(&config));
front_end_handler =
Arc::from(ServiceBuilder::build_service(&config).ok_or_else(|| {
Error::new(ErrorKind::Other, "Parsec can not be configured.")
})?);
listener = ServiceBuilder::start_listener(&config.listener);
threadpool = ServiceBuilder::build_threadpool(config.core_settings.thread_pool_size);

Expand Down
2 changes: 1 addition & 1 deletion src/providers/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ pub mod core_provider;
#[cfg(feature = "pkcs11-provider")]
pub mod pkcs11_provider;

#[cfg(feature = "mbed")]
#[cfg(feature = "mbed-crypto-provider")]
pub mod mbed_provider;

#[derive(Deserialize, Debug)]
Expand Down
76 changes: 51 additions & 25 deletions src/utils/service_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,8 @@ use crate::front::{
};
use crate::key_id_managers::on_disk_manager::{OnDiskKeyIDManagerBuilder, DEFAULT_MAPPINGS_PATH};
use crate::key_id_managers::{KeyIdManagerConfig, KeyIdManagerType, ManageKeyIDs};
use crate::providers::{
core_provider::CoreProviderBuilder, mbed_provider::MbedProviderBuilder,
pkcs11_provider::Pkcs11ProviderBuilder, Provide, ProviderConfig, ProviderType,
};
use log::{info, LevelFilter};
use crate::providers::{core_provider::CoreProviderBuilder, Provide, ProviderConfig};
use log::{error, LevelFilter};
use parsec_interface::operations_protobuf::ProtobufConverter;
use parsec_interface::requests::AuthType;
use parsec_interface::requests::{BodyType, ProviderID};
Expand All @@ -40,6 +37,15 @@ use std::sync::RwLock;
use std::time::Duration;
use threadpool::{Builder as ThreadPoolBuilder, ThreadPool};

#[cfg(feature = "mbed-crypto-provider")]
use crate::providers::mbed_provider::MbedProviderBuilder;
#[cfg(feature = "pkcs11-provider")]
use crate::providers::pkcs11_provider::Pkcs11ProviderBuilder;
#[cfg(not(all(feature = "mbed-crypto-provider", feature = "pkcs11-provider")))]
use log::warn;
#[cfg(any(feature = "mbed-crypto-provider", feature = "pkcs11-provider"))]
use {crate::providers::ProviderType, log::info};

const VERSION_MINOR: u8 = 0;
const VERSION_MAJOR: u8 = 1;

Expand All @@ -65,11 +71,16 @@ pub struct ServiceConfig {
pub struct ServiceBuilder;

impl ServiceBuilder {
pub fn build_service(config: &ServiceConfig) -> FrontEndHandler {
pub fn build_service(config: &ServiceConfig) -> Option<FrontEndHandler> {
let key_id_managers = build_key_id_managers(&config.key_manager);

let providers = build_providers(&config.provider, key_id_managers);

if providers.is_empty() {
error!("Parsec needs at least one provider to start. No valid provider could be created from the configuration.");
return None;
}

let backend_handlers = build_backend_handlers(providers);

let dispatcher = DispatcherBuilder::new()
Expand All @@ -78,10 +89,12 @@ impl ServiceBuilder {

let simple_authenticator = Box::from(SimpleAuthenticator {});

FrontEndHandlerBuilder::new()
.with_dispatcher(dispatcher)
.with_authenticator(AuthType::Simple, simple_authenticator)
.build()
Some(
FrontEndHandlerBuilder::new()
.with_dispatcher(dispatcher)
.with_authenticator(AuthType::Simple, simple_authenticator)
.build(),
)
}

pub fn start_listener(config: &ListenerConfig) -> Box<dyn Listen> {
Expand Down Expand Up @@ -145,36 +158,41 @@ fn build_providers(
) -> HashMap<ProviderID, Provider> {
let mut map = HashMap::new();
for config in configs {
let key_id_manager = key_id_managers
.get(&config.key_id_manager)
.unwrap_or_else(|| {
panic!(
let key_id_manager = match key_id_managers.get(&config.key_id_manager) {
Some(key_id_manager) => key_id_manager,
None => {
error!(
"Key ID manager with specified name was not found ({})",
config.key_id_manager
)
});
map.insert(
config.provider_type.to_provider_id(),
get_provider(config, key_id_manager.clone()),
);
);
continue;
}
};
let provider = match get_provider(config, key_id_manager.clone()) {
Some(provider) => provider,
None => continue,
};
map.insert(config.provider_type.to_provider_id(), provider);
}

map
}

fn get_provider(config: &ProviderConfig, key_id_manager: KeyIdManager) -> Provider {
fn get_provider(config: &ProviderConfig, key_id_manager: KeyIdManager) -> Option<Provider> {
match config.provider_type {
#[cfg(feature = "mbed-crypto-provider")]
ProviderType::MbedProvider => {
info!("Creating a Mbed Crypto Provider.");
Box::from(
Some(Box::from(
MbedProviderBuilder::new()
.with_key_id_store(key_id_manager)
.build(),
)
))
}
#[cfg(feature = "pkcs11-provider")]
ProviderType::Pkcs11Provider => {
info!("Creating a PKCS 11 Provider.");
Box::from(
Some(Box::from(
Pkcs11ProviderBuilder::new()
.with_key_id_store(key_id_manager)
.with_pkcs11_library_path(config.library_path.clone().expect(
Expand All @@ -185,7 +203,15 @@ fn get_provider(config: &ProviderConfig, key_id_manager: KeyIdManager) -> Provid
))
.with_user_pin(config.user_pin.clone())
.build()
)
))
}
#[cfg(not(all(feature = "mbed-crypto-provider", feature = "pkcs11-provider")))]
_ => {
warn!(
Copy link
Member

Choose a reason for hiding this comment

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

I know we're not ready for a release yet, but this sounds like a case where we should panic instead of just printing a warning - if the config file defines a provider but that provider isn't enabled, the admin might be expecting some functionality to be available which will not be there

Copy link
Member Author

Choose a reason for hiding this comment

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

we should panic instead of just printing a warning

That is what I did first but having it return an Option allows us to do the following things:

  • even if one of the providers could not be instantiated, Parsec can still run with the other ones
  • an admin might reload the configuration with a bad one, adding a provider that is not in the binary: in that case, it will print a warning without the whole service shutting down
  • I added a feature in this commit that checks if no providers could be instantiated in which case the Parsec binary will exit with an error code: I think that is still a better way to exit than panicking which can be confusing.

IMO returning always seems cleaner than panicking when it is possible.

the admin might be expecting some functionality to be available which will not be there

You have a point there: I think it is better to solve this with the admin reading the logs and executing tests after Parsec has been installed than just panicking. I guess both options are possible and we will have to make a decision about this.

Copy link
Member

Choose a reason for hiding this comment

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

I'm ok with this going in like this, we'll have to see if we get any feedback about it

"Provider \"{:?}\" chosen in the configuration was not compiled in Parsec binary.",
config.provider_type
);
None
}
}
}
Expand Down