From c6a7ea0a2f20c8651168b8d3a33dfd49db3c665c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20Gr=C3=BCnbichler?= Date: Fri, 5 Feb 2021 16:35:31 +0100 Subject: [PATCH] client: refactor keyfile_parameters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit no semantic changes intended Signed-off-by: Fabian Grünbichler --- src/bin/proxmox-backup-client.rs | 147 +++++++++++----------- src/bin/proxmox_backup_client/catalog.rs | 10 +- src/bin/proxmox_backup_client/snapshot.rs | 8 +- 3 files changed, 86 insertions(+), 79 deletions(-) diff --git a/src/bin/proxmox-backup-client.rs b/src/bin/proxmox-backup-client.rs index 4a783abe..8268098e 100644 --- a/src/bin/proxmox-backup-client.rs +++ b/src/bin/proxmox-backup-client.rs @@ -597,7 +597,13 @@ fn spawn_catalog_upload( Ok(CatalogUploadResult { catalog_writer, result: catalog_result_rx }) } -fn keyfile_parameters(param: &Value) -> Result<(Option>, CryptMode), Error> { +#[derive(Debug, Eq, PartialEq)] +struct CryptoParams { + mode: CryptMode, + enc_key: Option>, +} + +fn crypto_parameters(param: &Value) -> Result { let keyfile = match param.get("keyfile") { Some(Value::String(keyfile)) => Some(keyfile), Some(_) => bail!("bad --keyfile parameter type"), @@ -616,7 +622,7 @@ fn keyfile_parameters(param: &Value) -> Result<(Option>, CryptMode), Err None => None, }; - let crypt_mode: Option = match param.get("crypt-mode") { + let mode: Option = match param.get("crypt-mode") { Some(mode) => Some(serde_json::from_value(mode.clone())?), None => None, }; @@ -640,30 +646,31 @@ fn keyfile_parameters(param: &Value) -> Result<(Option>, CryptMode), Err } }; - Ok(match (keydata, crypt_mode) { + Ok(match (keydata, mode) { // no parameters: (None, None) => match key::read_optional_default_encryption_key()? { - Some(key) => { + None => CryptoParams { enc_key: None, mode: CryptMode::None }, + enc_key => { eprintln!("Encrypting with default encryption key!"); - (Some(key), CryptMode::Encrypt) + CryptoParams { enc_key, mode: CryptMode::Encrypt } }, - None => (None, CryptMode::None), }, // just --crypt-mode=none - (None, Some(CryptMode::None)) => (None, CryptMode::None), + (None, Some(CryptMode::None)) => CryptoParams { enc_key: None, mode: CryptMode::None }, // just --crypt-mode other than none - (None, Some(crypt_mode)) => match key::read_optional_default_encryption_key()? { + (None, Some(mode)) => match key::read_optional_default_encryption_key()? { None => bail!("--crypt-mode without --keyfile and no default key file available"), - Some(key) => { + enc_key => { eprintln!("Encrypting with default encryption key!"); - (Some(key), crypt_mode) + + CryptoParams { enc_key, mode } }, } // just --keyfile - (Some(key), None) => (Some(key), CryptMode::Encrypt), + (enc_key, None) => CryptoParams { enc_key, mode: CryptMode::Encrypt }, // --keyfile and --crypt-mode=none (Some(_), Some(CryptMode::None)) => { @@ -671,57 +678,57 @@ fn keyfile_parameters(param: &Value) -> Result<(Option>, CryptMode), Err } // --keyfile and --crypt-mode other than none - (Some(key), Some(crypt_mode)) => (Some(key), crypt_mode), + (enc_key, Some(mode)) => CryptoParams { enc_key, mode }, }) } #[test] -// WARNING: there must only be one test for keyfile_parameters as the default key handling is not +// WARNING: there must only be one test for crypto_parameters as the default key handling is not // safe w.r.t. concurrency -fn test_keyfile_parameters_handling() -> Result<(), Error> { +fn test_crypto_parameters_handling() -> Result<(), Error> { let some_key = Some(vec![1;1]); let default_key = Some(vec![2;1]); - let no_key_res: (Option>, CryptMode) = (None, CryptMode::None); - let some_key_res = (some_key.clone(), CryptMode::Encrypt); - let some_key_sign_res = (some_key.clone(), CryptMode::SignOnly); - let default_key_res = (default_key.clone(), CryptMode::Encrypt); - let default_key_sign_res = (default_key.clone(), CryptMode::SignOnly); + let no_key_res = CryptoParams { enc_key: None, mode: CryptMode::None }; + let some_key_res = CryptoParams { enc_key: some_key.clone(), mode: CryptMode::Encrypt }; + let some_key_sign_res = CryptoParams { enc_key: some_key.clone(), mode: CryptMode::SignOnly }; + let default_key_res = CryptoParams { enc_key: default_key.clone(), mode: CryptMode::Encrypt }; + let default_key_sign_res = CryptoParams { enc_key: default_key.clone(), mode: CryptMode::SignOnly }; let keypath = "./tests/keyfile.test"; replace_file(&keypath, some_key.as_ref().unwrap(), CreateOptions::default())?; let invalid_keypath = "./tests/invalid_keyfile.test"; // no params, no default key == no key - let res = keyfile_parameters(&json!({})); + let res = crypto_parameters(&json!({})); assert_eq!(res.unwrap(), no_key_res); // keyfile param == key from keyfile - let res = keyfile_parameters(&json!({"keyfile": keypath})); + let res = crypto_parameters(&json!({"keyfile": keypath})); assert_eq!(res.unwrap(), some_key_res); // crypt mode none == no key - let res = keyfile_parameters(&json!({"crypt-mode": "none"})); + let res = crypto_parameters(&json!({"crypt-mode": "none"})); assert_eq!(res.unwrap(), no_key_res); // crypt mode encrypt/sign-only, no keyfile, no default key == Error - assert!(keyfile_parameters(&json!({"crypt-mode": "sign-only"})).is_err()); - assert!(keyfile_parameters(&json!({"crypt-mode": "encrypt"})).is_err()); + assert!(crypto_parameters(&json!({"crypt-mode": "sign-only"})).is_err()); + assert!(crypto_parameters(&json!({"crypt-mode": "encrypt"})).is_err()); // crypt mode none with explicit key == Error - assert!(keyfile_parameters(&json!({"crypt-mode": "none", "keyfile": keypath})).is_err()); + assert!(crypto_parameters(&json!({"crypt-mode": "none", "keyfile": keypath})).is_err()); // crypt mode sign-only/encrypt with keyfile == key from keyfile with correct mode - let res = keyfile_parameters(&json!({"crypt-mode": "sign-only", "keyfile": keypath})); + let res = crypto_parameters(&json!({"crypt-mode": "sign-only", "keyfile": keypath})); assert_eq!(res.unwrap(), some_key_sign_res); - let res = keyfile_parameters(&json!({"crypt-mode": "encrypt", "keyfile": keypath})); + let res = crypto_parameters(&json!({"crypt-mode": "encrypt", "keyfile": keypath})); assert_eq!(res.unwrap(), some_key_res); // invalid keyfile parameter always errors - assert!(keyfile_parameters(&json!({"keyfile": invalid_keypath})).is_err()); - assert!(keyfile_parameters(&json!({"keyfile": invalid_keypath, "crypt-mode": "none"})).is_err()); - assert!(keyfile_parameters(&json!({"keyfile": invalid_keypath, "crypt-mode": "sign-only"})).is_err()); - assert!(keyfile_parameters(&json!({"keyfile": invalid_keypath, "crypt-mode": "encrypt"})).is_err()); + assert!(crypto_parameters(&json!({"keyfile": invalid_keypath})).is_err()); + assert!(crypto_parameters(&json!({"keyfile": invalid_keypath, "crypt-mode": "none"})).is_err()); + assert!(crypto_parameters(&json!({"keyfile": invalid_keypath, "crypt-mode": "sign-only"})).is_err()); + assert!(crypto_parameters(&json!({"keyfile": invalid_keypath, "crypt-mode": "encrypt"})).is_err()); // now set a default key unsafe { key::set_test_encryption_key(Ok(default_key.clone())); } @@ -729,37 +736,37 @@ fn test_keyfile_parameters_handling() -> Result<(), Error> { // and repeat // no params but default key == default key - let res = keyfile_parameters(&json!({})); + let res = crypto_parameters(&json!({})); assert_eq!(res.unwrap(), default_key_res); // keyfile param == key from keyfile - let res = keyfile_parameters(&json!({"keyfile": keypath})); + let res = crypto_parameters(&json!({"keyfile": keypath})); assert_eq!(res.unwrap(), some_key_res); // crypt mode none == no key - let res = keyfile_parameters(&json!({"crypt-mode": "none"})); + let res = crypto_parameters(&json!({"crypt-mode": "none"})); assert_eq!(res.unwrap(), no_key_res); // crypt mode encrypt/sign-only, no keyfile, default key == default key with correct mode - let res = keyfile_parameters(&json!({"crypt-mode": "sign-only"})); + let res = crypto_parameters(&json!({"crypt-mode": "sign-only"})); assert_eq!(res.unwrap(), default_key_sign_res); - let res = keyfile_parameters(&json!({"crypt-mode": "encrypt"})); + let res = crypto_parameters(&json!({"crypt-mode": "encrypt"})); assert_eq!(res.unwrap(), default_key_res); // crypt mode none with explicit key == Error - assert!(keyfile_parameters(&json!({"crypt-mode": "none", "keyfile": keypath})).is_err()); + assert!(crypto_parameters(&json!({"crypt-mode": "none", "keyfile": keypath})).is_err()); // crypt mode sign-only/encrypt with keyfile == key from keyfile with correct mode - let res = keyfile_parameters(&json!({"crypt-mode": "sign-only", "keyfile": keypath})); + let res = crypto_parameters(&json!({"crypt-mode": "sign-only", "keyfile": keypath})); assert_eq!(res.unwrap(), some_key_sign_res); - let res = keyfile_parameters(&json!({"crypt-mode": "encrypt", "keyfile": keypath})); + let res = crypto_parameters(&json!({"crypt-mode": "encrypt", "keyfile": keypath})); assert_eq!(res.unwrap(), some_key_res); // invalid keyfile parameter always errors - assert!(keyfile_parameters(&json!({"keyfile": invalid_keypath})).is_err()); - assert!(keyfile_parameters(&json!({"keyfile": invalid_keypath, "crypt-mode": "none"})).is_err()); - assert!(keyfile_parameters(&json!({"keyfile": invalid_keypath, "crypt-mode": "sign-only"})).is_err()); - assert!(keyfile_parameters(&json!({"keyfile": invalid_keypath, "crypt-mode": "encrypt"})).is_err()); + assert!(crypto_parameters(&json!({"keyfile": invalid_keypath})).is_err()); + assert!(crypto_parameters(&json!({"keyfile": invalid_keypath, "crypt-mode": "none"})).is_err()); + assert!(crypto_parameters(&json!({"keyfile": invalid_keypath, "crypt-mode": "sign-only"})).is_err()); + assert!(crypto_parameters(&json!({"keyfile": invalid_keypath, "crypt-mode": "encrypt"})).is_err()); // now make default key retrieval error unsafe { key::set_test_encryption_key(Err(format_err!("test error"))); } @@ -767,34 +774,34 @@ fn test_keyfile_parameters_handling() -> Result<(), Error> { // and repeat // no params, default key retrieval errors == Error - assert!(keyfile_parameters(&json!({})).is_err()); + assert!(crypto_parameters(&json!({})).is_err()); // keyfile param == key from keyfile - let res = keyfile_parameters(&json!({"keyfile": keypath})); + let res = crypto_parameters(&json!({"keyfile": keypath})); assert_eq!(res.unwrap(), some_key_res); // crypt mode none == no key - let res = keyfile_parameters(&json!({"crypt-mode": "none"})); + let res = crypto_parameters(&json!({"crypt-mode": "none"})); assert_eq!(res.unwrap(), no_key_res); // crypt mode encrypt/sign-only, no keyfile, default key error == Error - assert!(keyfile_parameters(&json!({"crypt-mode": "sign-only"})).is_err()); - assert!(keyfile_parameters(&json!({"crypt-mode": "encrypt"})).is_err()); + assert!(crypto_parameters(&json!({"crypt-mode": "sign-only"})).is_err()); + assert!(crypto_parameters(&json!({"crypt-mode": "encrypt"})).is_err()); // crypt mode none with explicit key == Error - assert!(keyfile_parameters(&json!({"crypt-mode": "none", "keyfile": keypath})).is_err()); + assert!(crypto_parameters(&json!({"crypt-mode": "none", "keyfile": keypath})).is_err()); // crypt mode sign-only/encrypt with keyfile == key from keyfile with correct mode - let res = keyfile_parameters(&json!({"crypt-mode": "sign-only", "keyfile": keypath})); + let res = crypto_parameters(&json!({"crypt-mode": "sign-only", "keyfile": keypath})); assert_eq!(res.unwrap(), some_key_sign_res); - let res = keyfile_parameters(&json!({"crypt-mode": "encrypt", "keyfile": keypath})); + let res = crypto_parameters(&json!({"crypt-mode": "encrypt", "keyfile": keypath})); assert_eq!(res.unwrap(), some_key_res); // invalid keyfile parameter always errors - assert!(keyfile_parameters(&json!({"keyfile": invalid_keypath})).is_err()); - assert!(keyfile_parameters(&json!({"keyfile": invalid_keypath, "crypt-mode": "none"})).is_err()); - assert!(keyfile_parameters(&json!({"keyfile": invalid_keypath, "crypt-mode": "sign-only"})).is_err()); - assert!(keyfile_parameters(&json!({"keyfile": invalid_keypath, "crypt-mode": "encrypt"})).is_err()); + assert!(crypto_parameters(&json!({"keyfile": invalid_keypath})).is_err()); + assert!(crypto_parameters(&json!({"keyfile": invalid_keypath, "crypt-mode": "none"})).is_err()); + assert!(crypto_parameters(&json!({"keyfile": invalid_keypath, "crypt-mode": "sign-only"})).is_err()); + assert!(crypto_parameters(&json!({"keyfile": invalid_keypath, "crypt-mode": "encrypt"})).is_err()); Ok(()) } @@ -906,7 +913,7 @@ async fn create_backup( verify_chunk_size(size)?; } - let (keydata, crypt_mode) = keyfile_parameters(¶m)?; + let crypto = crypto_parameters(¶m)?; let backup_id = param["backup-id"].as_str().unwrap_or(&proxmox::tools::nodename()); @@ -1011,7 +1018,7 @@ async fn create_backup( println!("Starting backup protocol: {}", strftime_local("%c", epoch_i64())?); - let (crypt_config, rsa_encrypted_key) = match keydata { + let (crypt_config, rsa_encrypted_key) = match crypto.enc_key { None => (None, None), Some(key) => { let (key, created, fingerprint) = decrypt_key(&key, &key::get_encryption_key_password)?; @@ -1097,7 +1104,7 @@ async fn create_backup( BackupSpecificationType::CONFIG => { let upload_options = UploadOptions { compress: true, - encrypt: crypt_mode == CryptMode::Encrypt, + encrypt: crypto.mode == CryptMode::Encrypt, ..UploadOptions::default() }; @@ -1105,12 +1112,12 @@ async fn create_backup( let stats = client .upload_blob_from_file(&filename, &target, upload_options) .await?; - manifest.add_file(target, stats.size, stats.csum, crypt_mode)?; + manifest.add_file(target, stats.size, stats.csum, crypto.mode)?; } BackupSpecificationType::LOGFILE => { // fixme: remove - not needed anymore ? let upload_options = UploadOptions { compress: true, - encrypt: crypt_mode == CryptMode::Encrypt, + encrypt: crypto.mode == CryptMode::Encrypt, ..UploadOptions::default() }; @@ -1118,12 +1125,12 @@ async fn create_backup( let stats = client .upload_blob_from_file(&filename, &target, upload_options) .await?; - manifest.add_file(target, stats.size, stats.csum, crypt_mode)?; + manifest.add_file(target, stats.size, stats.csum, crypto.mode)?; } BackupSpecificationType::PXAR => { // start catalog upload on first use if catalog.is_none() { - let catalog_upload_res = spawn_catalog_upload(client.clone(), crypt_mode == CryptMode::Encrypt)?; + let catalog_upload_res = spawn_catalog_upload(client.clone(), crypto.mode == CryptMode::Encrypt)?; catalog = Some(catalog_upload_res.catalog_writer); catalog_result_rx = Some(catalog_upload_res.result); } @@ -1143,7 +1150,7 @@ async fn create_backup( let upload_options = UploadOptions { previous_manifest: previous_manifest.clone(), compress: true, - encrypt: crypt_mode == CryptMode::Encrypt, + encrypt: crypto.mode == CryptMode::Encrypt, ..UploadOptions::default() }; @@ -1156,7 +1163,7 @@ async fn create_backup( pxar_options, upload_options, ).await?; - manifest.add_file(target, stats.size, stats.csum, crypt_mode)?; + manifest.add_file(target, stats.size, stats.csum, crypto.mode)?; catalog.lock().unwrap().end_directory()?; } BackupSpecificationType::IMAGE => { @@ -1166,7 +1173,7 @@ async fn create_backup( previous_manifest: previous_manifest.clone(), fixed_size: Some(size), compress: true, - encrypt: crypt_mode == CryptMode::Encrypt, + encrypt: crypto.mode == CryptMode::Encrypt, }; let stats = backup_image( @@ -1176,7 +1183,7 @@ async fn create_backup( chunk_size_opt, upload_options, ).await?; - manifest.add_file(target, stats.size, stats.csum, crypt_mode)?; + manifest.add_file(target, stats.size, stats.csum, crypto.mode)?; } } } @@ -1193,7 +1200,7 @@ async fn create_backup( if let Some(catalog_result_rx) = catalog_result_rx { let stats = catalog_result_rx.await??; - manifest.add_file(CATALOG_NAME.to_owned(), stats.size, stats.csum, crypt_mode)?; + manifest.add_file(CATALOG_NAME.to_owned(), stats.size, stats.csum, crypto.mode)?; } } @@ -1204,7 +1211,7 @@ async fn create_backup( let stats = client .upload_blob_from_data(rsa_encrypted_key, target, options) .await?; - manifest.add_file(target.to_string(), stats.size, stats.csum, crypt_mode)?; + manifest.add_file(target.to_string(), stats.size, stats.csum, crypto.mode)?; } // create manifest (index.json) @@ -1379,9 +1386,9 @@ async fn restore(param: Value) -> Result { let target = tools::required_string_param(¶m, "target")?; let target = if target == "-" { None } else { Some(target) }; - let (keydata, _crypt_mode) = keyfile_parameters(¶m)?; + let crypto = crypto_parameters(¶m)?; - let crypt_config = match keydata { + let crypt_config = match crypto.enc_key { None => None, Some(key) => { let (key, _, fingerprint) = decrypt_key(&key, &key::get_encryption_key_password)?; diff --git a/src/bin/proxmox_backup_client/catalog.rs b/src/bin/proxmox_backup_client/catalog.rs index 61bcc57f..b69d9c37 100644 --- a/src/bin/proxmox_backup_client/catalog.rs +++ b/src/bin/proxmox_backup_client/catalog.rs @@ -16,7 +16,6 @@ use crate::{ KEYFD_SCHEMA, extract_repository_from_value, record_repository, - keyfile_parameters, key::get_encryption_key_password, decrypt_key, api_datastore_latest_snapshot, @@ -25,6 +24,7 @@ use crate::{ complete_group_or_snapshot, complete_pxar_archive_name, connect, + crypto_parameters, BackupDir, BackupGroup, BufferedDynamicReader, @@ -68,9 +68,9 @@ async fn dump_catalog(param: Value) -> Result { let path = tools::required_string_param(¶m, "snapshot")?; let snapshot: BackupDir = path.parse()?; - let (keydata, _) = keyfile_parameters(¶m)?; + let crypto = crypto_parameters(¶m)?; - let crypt_config = match keydata { + let crypt_config = match crypto.enc_key { None => None, Some(key) => { let (key, _created, _fingerprint) = decrypt_key(&key, &get_encryption_key_password)?; @@ -166,9 +166,9 @@ async fn catalog_shell(param: Value) -> Result<(), Error> { (snapshot.group().backup_type().to_owned(), snapshot.group().backup_id().to_owned(), snapshot.backup_time()) }; - let (keydata, _) = keyfile_parameters(¶m)?; + let crypto = crypto_parameters(¶m)?; - let crypt_config = match keydata { + let crypt_config = match crypto.enc_key { None => None, Some(key) => { let (key, _created, _fingerprint) = decrypt_key(&key, &get_encryption_key_password)?; diff --git a/src/bin/proxmox_backup_client/snapshot.rs b/src/bin/proxmox_backup_client/snapshot.rs index 3bdc5f33..0c694598 100644 --- a/src/bin/proxmox_backup_client/snapshot.rs +++ b/src/bin/proxmox_backup_client/snapshot.rs @@ -30,9 +30,9 @@ use crate::{ complete_backup_group, complete_repository, connect, + crypto_parameters, extract_repository_from_value, record_repository, - keyfile_parameters, }; #[api( @@ -234,9 +234,9 @@ async fn upload_log(param: Value) -> Result { let mut client = connect(&repo)?; - let (keydata, crypt_mode) = keyfile_parameters(¶m)?; + let crypto = crypto_parameters(¶m)?; - let crypt_config = match keydata { + let crypt_config = match crypto.enc_key { None => None, Some(key) => { let (key, _created, _) = decrypt_key(&key, &crate::key::get_encryption_key_password)?; @@ -248,7 +248,7 @@ async fn upload_log(param: Value) -> Result { let data = file_get_contents(logfile)?; // fixme: howto sign log? - let blob = match crypt_mode { + let blob = match crypto.mode { CryptMode::None | CryptMode::SignOnly => DataBlob::encode(&data, None, true)?, CryptMode::Encrypt => DataBlob::encode(&data, crypt_config.as_ref().map(Arc::as_ref), true)?, };