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.

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

Better handle network errors #164

Merged
merged 8 commits into from
Jan 4, 2024
Merged
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
196 changes: 131 additions & 65 deletions Cargo.lock

Large diffs are not rendered by default.

5 changes: 4 additions & 1 deletion p11nethsm.conf
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,7 @@ slots:
danger_insecure_cert: true
# sha256_fingerprints:
# - "31:92:8E:A4:5E:16:5C:A7:33:44:E8:E9:8E:64:C4:AE:7B:2A:57:E5:77:43:49:F3:69:C9:8F:C4:2F:3A:3B:6E"

retries:
count: 10
delay_seconds: 1
timeout_seconds: 10
2 changes: 1 addition & 1 deletion pkcs11/src/api/generation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ pub extern "C" fn C_GenerateRandom(
let data = match session.login_ctx.try_(
|api_config| {
default_api::random_post(
&api_config,
api_config,
nethsm_sdk_rs::models::RandomRequestData {
length: ulRandomLen as i32,
},
Expand Down
11 changes: 10 additions & 1 deletion pkcs11/src/api/object.rs
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@ mod tests {
login::LoginCtx,
session::Session,
},
config::config_file::RetryConfig,
data::SESSION_MANAGER,
};

Expand Down Expand Up @@ -393,7 +394,15 @@ mod tests {
device_error: 0,
enum_ctx: None,
flags: 0,
login_ctx: LoginCtx::new(None, None, vec![]),
login_ctx: LoginCtx::new(
None,
None,
vec![],
Some(RetryConfig {
count: 2,
delay_seconds: 0,
}),
),
slot_id: 0,
};

Expand Down
20 changes: 11 additions & 9 deletions pkcs11/src/api/token.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,10 @@ pub extern "C" fn C_GetSlotInfo(

let mut flags = 0;

let mut login_ctx = LoginCtx::new(None, None, slot.instances.clone());
let mut login_ctx = LoginCtx::new(None, None, slot.instances.clone(), slot.retries);

let result = login_ctx.try_(
|conf| default_api::info_get(&conf),
default_api::info_get,
crate::backend::login::UserMode::Guest,
);

Expand All @@ -108,7 +108,7 @@ pub extern "C" fn C_GetSlotInfo(
};

let result = login_ctx.try_(
|conf| default_api::health_state_get(&conf),
default_api::health_state_get,
crate::backend::login::UserMode::Guest,
);

Expand Down Expand Up @@ -160,10 +160,15 @@ pub extern "C" fn C_GetTokenInfo(
return cryptoki_sys::CKR_ARGUMENTS_BAD;
}

let mut login_ctx = LoginCtx::new(None, slot.administrator.clone(), slot.instances.clone());
let mut login_ctx = LoginCtx::new(
None,
slot.administrator.clone(),
slot.instances.clone(),
slot.retries,
);

let result = login_ctx.try_(
|conf| default_api::info_get(&conf),
default_api::info_get,
crate::backend::login::UserMode::Guest,
);

Expand All @@ -184,10 +189,7 @@ pub extern "C" fn C_GetTokenInfo(
// Try to fech system info

if login_ctx.can_run_mode(crate::backend::login::UserMode::Administrator) {
match login_ctx.try_(
|conf| default_api::system_info_get(&conf),
UserMode::Administrator,
) {
match login_ctx.try_(default_api::system_info_get, UserMode::Administrator) {
Err(e) => {
warn!("Error getting system info: {:?}", e);
}
Expand Down
2 changes: 1 addition & 1 deletion pkcs11/src/backend/decrypt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ impl DecryptCtx {
let output = self.login_ctx.try_(
|api_config| {
default_api::keys_key_id_decrypt_post(
&api_config,
api_config,
key_id,
nethsm_sdk_rs::models::DecryptRequestData {
mode,
Expand Down
2 changes: 1 addition & 1 deletion pkcs11/src/backend/encrypt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ fn encrypt_data(
.try_(
|api_config| {
default_api::keys_key_id_encrypt_post(
&api_config,
api_config,
key_id,
nethsm_sdk_rs::models::EncryptRequestData {
mode,
Expand Down
7 changes: 2 additions & 5 deletions pkcs11/src/backend/events.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,9 @@ pub fn update_slot_state(slot_id: CK_SLOT_ID, present: bool) {

pub fn fetch_slots_state() {
for (index, slot) in DEVICE.slots.iter().enumerate() {
let mut login_ctx = LoginCtx::new(None, None, slot.instances.clone());
let mut login_ctx = LoginCtx::new(None, None, slot.instances.clone(), slot.retries);
let status = login_ctx
.try_(
|conf| default_api::health_state_get(&conf),
super::login::UserMode::Guest,
)
.try_(default_api::health_state_get, super::login::UserMode::Guest)
.map(|state| state.entity.state == SystemState::Operational)
.unwrap_or(false);

Expand Down
12 changes: 6 additions & 6 deletions pkcs11/src/backend/key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ fn upload_certificate(
let key_id = id.as_str();

login_ctx.try_(
|api_config| default_api::keys_key_id_cert_put(&api_config, key_id, body.into_bytes()),
|api_config| default_api::keys_key_id_cert_put(api_config, key_id, body.into_bytes()),
login::UserMode::Administrator,
)?;

Expand Down Expand Up @@ -324,7 +324,7 @@ pub fn create_key_from_template(
if let Err(err) = login_ctx.try_(
|api_config| {
default_api::keys_key_id_put(
&api_config,
api_config,
key_id,
default_api::KeysKeyIdPutBody::ApplicationJson(private_key),
)
Expand All @@ -339,7 +339,7 @@ pub fn create_key_from_template(
let resp = login_ctx.try_(
|api_config| {
default_api::keys_post(
&api_config,
api_config,
default_api::KeysPostBody::ApplicationJson(private_key),
)
},
Expand Down Expand Up @@ -439,7 +439,7 @@ pub fn generate_key_from_template(
let id = login_ctx.try_(
|api_config| {
default_api::keys_generate_post(
&api_config,
api_config,
KeyGenerateRequestData {
mechanisms: api_mechs,
r#type: key_type,
Expand Down Expand Up @@ -471,7 +471,7 @@ pub fn fetch_key(
}

let key_data = match login_ctx.try_(
|api_config| default_api::keys_key_id_get(&api_config, key_id),
|api_config| default_api::keys_key_id_get(api_config, key_id),
super::login::UserMode::OperatorOrAdministrator,
) {
Ok(key_data) => key_data.entity,
Expand Down Expand Up @@ -514,7 +514,7 @@ pub fn fetch_certificate(
}

let cert_data = login_ctx.try_(
|api_config| default_api::keys_key_id_cert_get(&api_config, key_id),
|api_config| default_api::keys_key_id_cert_get(api_config, key_id),
super::login::UserMode::OperatorOrAdministrator,
)?;

Expand Down
69 changes: 51 additions & 18 deletions pkcs11/src/backend/login.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@
CKR_USER_TYPE_INVALID, CKS_RO_PUBLIC_SESSION, CKS_RW_SO_FUNCTIONS, CKS_RW_USER_FUNCTIONS,
CKU_CONTEXT_SPECIFIC, CKU_SO, CKU_USER, CK_RV, CK_STATE, CK_USER_TYPE,
};
use log::{debug, error, trace};
use log::{debug, error, trace, warn};
use nethsm_sdk_rs::{
apis::{self, configuration::Configuration, default_api, ResponseContent},
models::UserRole,
ureq,
};
use std::fmt::Debug;
use std::{thread, time::Duration};

use crate::config::config_file::UserConfig;
use crate::config::config_file::{RetryConfig, UserConfig};

use super::{ApiError, Error};

Expand All @@ -21,6 +22,7 @@
instances: Vec<Configuration>,
index: usize,
ck_state: CK_STATE,
retries: Option<RetryConfig>,
}

#[derive(Debug, Clone)]
Expand Down Expand Up @@ -64,6 +66,7 @@
operator: Option<UserConfig>,
administrator: Option<UserConfig>,
instances: Vec<Configuration>,
retries: Option<RetryConfig>,
) -> Self {
let mut ck_state = CKS_RO_PUBLIC_SESSION;

Expand All @@ -81,6 +84,7 @@
operator,
administrator,
instances,
retries,
index: 0,
ck_state,
}
Expand Down Expand Up @@ -190,7 +194,7 @@
// Try to run the api call on each instance until one succeeds
pub fn try_<F, T, R>(&mut self, api_call: F, user_mode: UserMode) -> Result<R, ApiError>
where
F: FnOnce(Configuration) -> Result<R, apis::Error<T>> + Clone,
F: FnOnce(&Configuration) -> Result<R, apis::Error<T>> + Clone,
{
// we loop for a maximum of instances.len() times
for _ in 0..self.instances.len() {
Expand All @@ -199,19 +203,48 @@
None => continue,
};

let api_call_clone = api_call.clone();
match api_call_clone(conf) {
Ok(result) => return Ok(result),

// If the server is in an unusable state, try the next one
Err(apis::Error::ResponseError(ResponseContent { status: 500, .. }))
| Err(apis::Error::ResponseError(ResponseContent { status: 501, .. }))
| Err(apis::Error::ResponseError(ResponseContent { status: 502, .. }))
| Err(apis::Error::ResponseError(ResponseContent { status: 503, .. }))
| Err(apis::Error::ResponseError(ResponseContent { status: 412, .. })) => continue,

// Otherwise, return the error
Err(err) => return Err(err.into()),
let mut retry_count = 0;
let RetryConfig {
count: retry_limit,
delay_seconds,
} = self.retries.unwrap_or(RetryConfig {
count: 1,
delay_seconds: 0,
});

let delay = Duration::from_secs(delay_seconds);

loop {
retry_count += 1;
let api_call_clone = api_call.clone();
match api_call_clone(&conf) {
Ok(result) => return Ok(result),

// If the server is in an unusable state, skip retries and try the next one
Err(apis::Error::ResponseError(ResponseContent { status: 500, .. }))
| Err(apis::Error::ResponseError(ResponseContent { status: 501, .. }))
| Err(apis::Error::ResponseError(ResponseContent { status: 502, .. }))
| Err(apis::Error::ResponseError(ResponseContent { status: 503, .. }))
| Err(apis::Error::ResponseError(ResponseContent { status: 412, .. })) => break,

Check warning on line 228 in pkcs11/src/backend/login.rs

View check run for this annotation

Codecov / codecov/patch

pkcs11/src/backend/login.rs#L228

Added line #L228 was not covered by tests

// If the connection to the server failed with a network error, reconnecting might solve the issue
Err(apis::Error::Ureq(ureq::Error::Transport(err)))
if matches!(

Check warning on line 232 in pkcs11/src/backend/login.rs

View check run for this annotation

Codecov / codecov/patch

pkcs11/src/backend/login.rs#L232

Added line #L232 was not covered by tests
err.kind(),
ureq::ErrorKind::Io | ureq::ErrorKind::ConnectionFailed
) =>
{
if retry_count == retry_limit {
error!("Retry count exceeded, instance is unreachable: {err}");
return Err(ApiError::InstanceRemoved);
}

warn!("IO error connecting to the instance, {err}, retrying in {delay_seconds}s");
thread::sleep(delay);

Check warning on line 243 in pkcs11/src/backend/login.rs

View check run for this annotation

Codecov / codecov/patch

pkcs11/src/backend/login.rs#L240-L243

Added lines #L240 - L243 were not covered by tests
}
// Otherwise, return the error
Err(err) => return Err(err.into()),
}
}
}
Err(ApiError::NoInstance)
Expand Down Expand Up @@ -244,7 +277,7 @@
match self.try_(
|config| {
default_api::users_user_id_passphrase_post(
&config,
config,
&options.0,
nethsm_sdk_rs::models::UserPassphrasePostData { passphrase: pin },
)
Expand Down
12 changes: 8 additions & 4 deletions pkcs11/src/backend/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@
};
use cryptoki_sys::{
CKR_ARGUMENTS_BAD, CKR_ATTRIBUTE_VALUE_INVALID, CKR_DATA_INVALID, CKR_DATA_LEN_RANGE,
CKR_DEVICE_ERROR, CKR_DEVICE_MEMORY, CKR_ENCRYPTED_DATA_LEN_RANGE, CKR_KEY_HANDLE_INVALID,
CKR_MECHANISM_INVALID, CKR_OPERATION_ACTIVE, CKR_OPERATION_NOT_INITIALIZED,
CKR_TOKEN_NOT_PRESENT, CKR_USER_NOT_LOGGED_IN, CK_ATTRIBUTE_TYPE, CK_OBJECT_HANDLE, CK_RV,
CKR_DEVICE_ERROR, CKR_DEVICE_MEMORY, CKR_DEVICE_REMOVED, CKR_ENCRYPTED_DATA_LEN_RANGE,
CKR_KEY_HANDLE_INVALID, CKR_MECHANISM_INVALID, CKR_OPERATION_ACTIVE,
CKR_OPERATION_NOT_INITIALIZED, CKR_TOKEN_NOT_PRESENT, CKR_USER_NOT_LOGGED_IN,
CK_ATTRIBUTE_TYPE, CK_OBJECT_HANDLE, CK_RV,
};
use log::error;
use nethsm_sdk_rs::apis;
Expand Down Expand Up @@ -38,6 +39,7 @@
Serde(serde_json::Error),
Io(std::io::Error),
ResponseError(ResponseContent),
InstanceRemoved,
NoInstance,
StringParse(std::string::FromUtf8Error),
}
Expand All @@ -51,7 +53,7 @@
apis::Error::ResponseError(resp) => ApiError::ResponseError(ResponseContent {
status: resp.status,
content: String::from_utf8(resp.content).unwrap_or_else(|e| {
format!(
error!(

Check warning on line 56 in pkcs11/src/backend/mod.rs

View check run for this annotation

Codecov / codecov/patch

pkcs11/src/backend/mod.rs#L56

Added line #L56 was not covered by tests
"Unable to parse response content into string: {:?}",
e.as_bytes()
);
Expand Down Expand Up @@ -153,6 +155,7 @@
_ => CKR_DEVICE_ERROR,
},
ApiError::StringParse(_) => CKR_DEVICE_ERROR,
ApiError::InstanceRemoved => CKR_DEVICE_REMOVED,

Check warning on line 158 in pkcs11/src/backend/mod.rs

View check run for this annotation

Codecov / codecov/patch

pkcs11/src/backend/mod.rs#L158

Added line #L158 was not covered by tests
},
}
}
Expand Down Expand Up @@ -205,6 +208,7 @@
_ => format!("Api error: {:?}", resp),
},
ApiError::StringParse(err) => format!("String parse error: {:?}", err),
ApiError::InstanceRemoved => "Failed to connect to instance".to_string(),

Check warning on line 211 in pkcs11/src/backend/mod.rs

View check run for this annotation

Codecov / codecov/patch

pkcs11/src/backend/mod.rs#L211

Added line #L211 was not covered by tests
},
Error::Base64(err) => format!("Base64 Decode error: {:?}", err),
Error::StringParse(err) => format!("String parse error: {:?}", err),
Expand Down
8 changes: 5 additions & 3 deletions pkcs11/src/backend/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ impl SessionManager {
0,
Arc::new(Slot {
administrator: None,
retries: None,
db: Arc::new(Mutex::new(Db::new())),
description: None,
instances: vec![],
Expand Down Expand Up @@ -125,6 +126,7 @@ impl Session {
slot.operator.clone(),
slot.administrator.clone(),
slot.instances.clone(),
slot.retries,
);

Self {
Expand Down Expand Up @@ -513,7 +515,7 @@ impl Session {
let keys = self
.login_ctx
.try_(
|api_config| default_api::keys_get(&api_config, None),
|api_config| default_api::keys_get(api_config, None),
super::login::UserMode::OperatorOrAdministrator,
)?
.entity;
Expand Down Expand Up @@ -617,13 +619,13 @@ impl Session {
match key.kind {
ObjectKind::Certificate => {
self.login_ctx.try_(
|api_config| default_api::keys_key_id_cert_delete(&api_config, &key.id),
|api_config| default_api::keys_key_id_cert_delete(api_config, &key.id),
crate::backend::login::UserMode::Administrator,
)?;
}
ObjectKind::SecretKey | ObjectKind::PrivateKey => {
self.login_ctx.try_(
|api_config| default_api::keys_key_id_delete(&api_config, &key.id),
|api_config| default_api::keys_key_id_delete(api_config, &key.id),
crate::backend::login::UserMode::Administrator,
)?;
}
Expand Down
Loading
Loading