From b56760fa0b3bc0ae929abe4bed8189662b4ee8b0 Mon Sep 17 00:00:00 2001 From: AssemblyJohn Date: Wed, 1 May 2024 14:24:37 +0300 Subject: [PATCH] Interface updates Signed-off-by: AssemblyJohn --- .../crypto/interface/crypto_supplier.hpp | 3 +- .../crypto/interface/crypto_types.hpp | 14 +++ .../crypto/openssl/openssl_supplier.hpp | 3 +- include/evse_security/evse_security.hpp | 25 ++-- include/evse_security/evse_types.hpp | 13 ++ lib/evse_security/CMakeLists.txt | 1 + .../certificate/x509_wrapper.cpp | 4 +- .../crypto/interface/crypto_supplier.cpp | 5 +- .../crypto/interface/crypto_types.cpp | 32 +++++ .../crypto/openssl/openssl_supplier.cpp | 17 +-- lib/evse_security/evse_security.cpp | 117 +++++++++++++----- tests/openssl_supplier_test.cpp | 8 +- tests/tests.cpp | 11 +- 13 files changed, 194 insertions(+), 59 deletions(-) create mode 100644 lib/evse_security/crypto/interface/crypto_types.cpp diff --git a/include/evse_security/crypto/interface/crypto_supplier.hpp b/include/evse_security/crypto/interface/crypto_supplier.hpp index dc107fc..d73ddde 100644 --- a/include/evse_security/crypto/interface/crypto_supplier.hpp +++ b/include/evse_security/crypto/interface/crypto_supplier.hpp @@ -71,7 +71,8 @@ class AbstractCryptoSupplier { const std::vector& data); /// @brief Generates a certificate signing request with the provided parameters - static bool x509_generate_csr(const CertificateSigningRequestInfo& generation_info, std::string& out_csr); + static CertificateSignRequestResult x509_generate_csr(const CertificateSigningRequestInfo& generation_info, + std::string& out_csr); public: // Digesting/decoding utils static bool digest_file_sha256(const fs::path& path, std::vector& out_digest); diff --git a/include/evse_security/crypto/interface/crypto_types.hpp b/include/evse_security/crypto/interface/crypto_types.hpp index de90f99..604d67d 100644 --- a/include/evse_security/crypto/interface/crypto_types.hpp +++ b/include/evse_security/crypto/interface/crypto_types.hpp @@ -25,6 +25,16 @@ enum class KeyValidationResult { Unknown, // Unknown error, not related to provider validation }; +enum class CertificateSignRequestResult { + Valid, + KeyGenerationError, // Error when generating the key, maybe invalid key type + VersioningError, // The version could not be set + PubkeyError, // The public key could not be attached + ExtensionsError, // The extensions could not be appended + SigningError, // The CSR could not be signed, maybe key or signing algo invalid + Unknown, // Any other error +}; + struct KeyGenerationInfo { CryptoKeyType key_type; @@ -77,4 +87,8 @@ using KeyHandle_ptr = std::unique_ptr; // Transforms a duration of days into seconds using days_to_seconds = std::chrono::duration>; +namespace conversions { +std::string get_certificate_sign_request_result_to_string(CertificateSignRequestResult e); +} + } // namespace evse_security \ No newline at end of file diff --git a/include/evse_security/crypto/openssl/openssl_supplier.hpp b/include/evse_security/crypto/openssl/openssl_supplier.hpp index 0868bf4..b0fe169 100644 --- a/include/evse_security/crypto/openssl/openssl_supplier.hpp +++ b/include/evse_security/crypto/openssl/openssl_supplier.hpp @@ -40,7 +40,8 @@ class OpenSSLSupplier : public AbstractCryptoSupplier { static bool x509_verify_signature(X509Handle* handle, const std::vector& signature, const std::vector& data); - static bool x509_generate_csr(const CertificateSigningRequestInfo& csr_info, std::string& out_csr); + static CertificateSignRequestResult x509_generate_csr(const CertificateSigningRequestInfo& csr_info, + std::string& out_csr); public: static bool digest_file_sha256(const fs::path& path, std::vector& out_digest); diff --git a/include/evse_security/evse_security.hpp b/include/evse_security/evse_security.hpp index ad87902..db268a4 100644 --- a/include/evse_security/evse_security.hpp +++ b/include/evse_security/evse_security.hpp @@ -165,10 +165,11 @@ class EvseSecurity { /// @param organization /// @param common /// @param use_tpm If the TPM should be used for the CSR request - /// @return the PEM formatted certificate signing request - std::string generate_certificate_signing_request(LeafCertificateType certificate_type, const std::string& country, - const std::string& organization, const std::string& common, - bool use_tpm); + /// @return the status and an optional PEM formatted certificate signing request string + GetCertificateSignRequestResult generate_certificate_signing_request(LeafCertificateType certificate_type, + const std::string& country, + const std::string& organization, + const std::string& common, bool use_tpm); /// @brief Generates a certificate signing request for the given \p certificate_type , \p country , \p organization /// and \p common without using the TPM @@ -176,9 +177,11 @@ class EvseSecurity { /// @param country /// @param organization /// @param common - /// @return the PEM formatted certificate signing request - std::string generate_certificate_signing_request(LeafCertificateType certificate_type, const std::string& country, - const std::string& organization, const std::string& common); + /// @return the status and an optional PEM formatted certificate signing request string + GetCertificateSignRequestResult generate_certificate_signing_request(LeafCertificateType certificate_type, + const std::string& country, + const std::string& organization, + const std::string& common); /// @brief Searches the filesystem on the specified directories for the given \p certificate_type and retrieves the /// most recent certificate that is already valid and the respective key. If no certificate is present or no key is @@ -203,6 +206,9 @@ class EvseSecurity { /// @return CA certificate file std::string get_verify_file(CaCertificateType certificate_type); + /// @brief An extension of 'get_verify_file' with error handling included + GetCertificateInfoResult get_ca_certificate_info(CaCertificateType certificate_type); + /// @brief Gets the expiry day count for the leaf certificate of the given \p certificate_type /// @param certificate_type /// @return day count until the leaf certificate expires @@ -248,9 +254,14 @@ class EvseSecurity { LeafCertificateType certificate_type); GetCertificateInfoResult get_leaf_certificate_info_internal(LeafCertificateType certificate_type, EncodingFormat encoding, bool include_ocsp = false); + GetCertificateInfoResult get_ca_certificate_info_internal(CaCertificateType certificate_type); std::optional retrieve_ocsp_cache_internal(const CertificateHashData& certificate_hash_data); bool is_ca_certificate_installed_internal(CaCertificateType certificate_type); + GetCertificateSignRequestResult + generate_certificate_signing_request_internal(LeafCertificateType certificate_type, + const CertificateSigningRequestInfo& info); + /// @brief Determines if the total filesize of certificates is > than the max_filesystem_usage bytes bool is_filesystem_full(); diff --git a/include/evse_security/evse_types.hpp b/include/evse_security/evse_types.hpp index cb8ee86..804004d 100644 --- a/include/evse_security/evse_types.hpp +++ b/include/evse_security/evse_types.hpp @@ -90,6 +90,13 @@ enum class GetCertificateInfoStatus { PrivateKeyNotFound, }; +enum class GetCertificateSignRequestStatus { + Accepted, + InvalidRequestedType, ///< Requested a CSR for non CSMS/V2G leafs + KeyGenError, ///< The key could not be generated with the requested/default parameters + GenerationError, ///< Any other error when creating the CSR +}; + // types of evse_security struct CertificateHashData { @@ -148,6 +155,12 @@ struct GetCertificateInfoResult { GetCertificateInfoStatus status; std::optional info; }; + +struct GetCertificateSignRequestResult { + GetCertificateSignRequestStatus status; + std::optional csr; +}; + namespace conversions { std::string encoding_format_to_string(EncodingFormat e); std::string ca_certificate_type_to_string(CaCertificateType e); diff --git a/lib/evse_security/CMakeLists.txt b/lib/evse_security/CMakeLists.txt index 817b347..7a5515f 100644 --- a/lib/evse_security/CMakeLists.txt +++ b/lib/evse_security/CMakeLists.txt @@ -14,6 +14,7 @@ target_sources(evse_security utils/evse_filesystem.cpp crypto/interface/crypto_supplier.cpp + crypto/interface/crypto_types.cpp crypto/openssl/openssl_supplier.cpp crypto/openssl/openssl_tpm.cpp ) diff --git a/lib/evse_security/certificate/x509_wrapper.cpp b/lib/evse_security/certificate/x509_wrapper.cpp index 8d89ebc..de3e4c4 100644 --- a/lib/evse_security/certificate/x509_wrapper.cpp +++ b/lib/evse_security/certificate/x509_wrapper.cpp @@ -77,7 +77,9 @@ bool X509Wrapper::operator==(const X509Wrapper& other) const { } void X509Wrapper::update_validity() { - CryptoSupplier::x509_get_validity(get(), valid_in, valid_to); + if (false == CryptoSupplier::x509_get_validity(get(), valid_in, valid_to)) { + EVLOG_error << "Could not update validity for certificate: " << get_common_name(); + } } bool X509Wrapper::is_child(const X509Wrapper& parent) const { diff --git a/lib/evse_security/crypto/interface/crypto_supplier.cpp b/lib/evse_security/crypto/interface/crypto_supplier.cpp index 259fc38..ec983f5 100644 --- a/lib/evse_security/crypto/interface/crypto_supplier.cpp +++ b/lib/evse_security/crypto/interface/crypto_supplier.cpp @@ -91,8 +91,9 @@ bool AbstractCryptoSupplier::x509_verify_signature(X509Handle* handle, const std default_crypto_supplier_usage_error() return false; } -bool AbstractCryptoSupplier::x509_generate_csr(const CertificateSigningRequestInfo& csr_info, std::string& out_csr) { - default_crypto_supplier_usage_error() return false; +CertificateSignRequestResult AbstractCryptoSupplier::x509_generate_csr(const CertificateSigningRequestInfo& csr_info, + std::string& out_csr) { + default_crypto_supplier_usage_error() return CertificateSignRequestResult::Unknown; } bool AbstractCryptoSupplier::digest_file_sha256(const fs::path& path, std::vector& out_digest) { diff --git a/lib/evse_security/crypto/interface/crypto_types.cpp b/lib/evse_security/crypto/interface/crypto_types.cpp new file mode 100644 index 0000000..eba86fd --- /dev/null +++ b/lib/evse_security/crypto/interface/crypto_types.cpp @@ -0,0 +1,32 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Pionix GmbH and Contributors to EVerest + +#include + +namespace evse_security { + +namespace conversions { + +std::string get_certificate_sign_request_result_to_string(CertificateSignRequestResult e) { + + switch (e) { + case CertificateSignRequestResult::Valid: + return "Valid"; + case CertificateSignRequestResult::KeyGenerationError: + return "KeyGenerationError"; + case CertificateSignRequestResult::VersioningError: + return "VersioningError"; + case CertificateSignRequestResult::PubkeyError: + return "PubkeyError"; + case CertificateSignRequestResult::ExtensionsError: + return "ExtensionsError"; + case CertificateSignRequestResult::SigningError: + return "SigningError"; + } + + throw std::out_of_range("No known string conversion for provided enum of type CertificateSignRequestResult"); +} + +} // namespace conversions + +} // namespace evse_security \ No newline at end of file diff --git a/lib/evse_security/crypto/openssl/openssl_supplier.cpp b/lib/evse_security/crypto/openssl/openssl_supplier.cpp index 92671d8..3347b88 100644 --- a/lib/evse_security/crypto/openssl/openssl_supplier.cpp +++ b/lib/evse_security/crypto/openssl/openssl_supplier.cpp @@ -720,7 +720,8 @@ bool OpenSSLSupplier::x509_verify_signature(X509Handle* handle, const std::vecto } } -bool OpenSSLSupplier::x509_generate_csr(const CertificateSigningRequestInfo& csr_info, std::string& out_csr) { +CertificateSignRequestResult OpenSSLSupplier::x509_generate_csr(const CertificateSigningRequestInfo& csr_info, + std::string& out_csr) { KeyHandle_ptr gen_key; EVP_PKEY_CTX_ptr ctx; @@ -734,7 +735,7 @@ bool OpenSSLSupplier::x509_generate_csr(const CertificateSigningRequestInfo& csr } if (false == s_generate_key(csr_info.key_info, gen_key, ctx)) { - return false; + return CertificateSignRequestResult::KeyGenerationError; } EVP_PKEY* key = get(gen_key.get()); @@ -747,13 +748,13 @@ bool OpenSSLSupplier::x509_generate_csr(const CertificateSigningRequestInfo& csr if (false == X509_REQ_set_version(x509_req_ptr.get(), n_version)) { EVLOG_error << "Failed to set csr version!"; - return false; + return CertificateSignRequestResult::VersioningError; } // set public key of x509 req if (false == X509_REQ_set_pubkey(x509_req_ptr.get(), key)) { EVLOG_error << "Failed to set csr pubkey!"; - return false; + return CertificateSignRequestResult::PubkeyError; } X509_NAME* x509Name = X509_REQ_get_subject_name(x509_req_ptr.get()); @@ -794,9 +795,10 @@ bool OpenSSLSupplier::x509_generate_csr(const CertificateSigningRequestInfo& csr X509_EXTENSION_free(ext_basic_constraints); X509_EXTENSION_free(ext_san); sk_X509_EXTENSION_free(extensions); + if (!result) { EVLOG_error << "Failed to add csr extensions!"; - return false; + return CertificateSignRequestResult::ExtensionsError; } // sign the certificate with the private key @@ -806,7 +808,7 @@ bool OpenSSLSupplier::x509_generate_csr(const CertificateSigningRequestInfo& csr if (x509_signed == false) { EVLOG_error << "Failed to sign csr!"; - return false; + return CertificateSignRequestResult::SigningError; } // write csr @@ -817,7 +819,8 @@ bool OpenSSLSupplier::x509_generate_csr(const CertificateSigningRequestInfo& csr BIO_get_mem_ptr(bio.get(), &mem_csr); out_csr = std::string(mem_csr->data, mem_csr->length); - return true; + + return CertificateSignRequestResult::Valid; } bool OpenSSLSupplier::digest_file_sha256(const fs::path& path, std::vector& out_digest) { diff --git a/lib/evse_security/evse_security.cpp b/lib/evse_security/evse_security.cpp index 2d8d46b..8eb2227 100644 --- a/lib/evse_security/evse_security.cpp +++ b/lib/evse_security/evse_security.cpp @@ -962,28 +962,63 @@ void EvseSecurity::certificate_signing_request_failed(const std::string& csr, Le // TODO(ioan): delete the pairing key of the CSR } -std::string EvseSecurity::generate_certificate_signing_request(LeafCertificateType certificate_type, - const std::string& country, - const std::string& organization, - const std::string& common, bool use_tpm) { - std::lock_guard guard(EvseSecurity::security_mutex); +GetCertificateSignRequestResult +EvseSecurity::generate_certificate_signing_request_internal(LeafCertificateType certificate_type, + const CertificateSigningRequestInfo& info) { + GetCertificateSignRequestResult result{}; - fs::path key_path; + EVLOG_info << "Generating CSR for leaf: " << conversions::leaf_certificate_type_to_string(certificate_type); + + std::string csr; + CertificateSignRequestResult csr_result = CryptoSupplier::x509_generate_csr(info, csr); + + if (csr_result == CertificateSignRequestResult::Valid) { + result.status = GetCertificateSignRequestStatus::Accepted; + result.csr = std::move(csr); + + EVLOG_debug << "Generated CSR end. CSR: " << csr; + + // Add the key to the managed CRS that we will delete if we can't find a certificate pair within the time + if (info.key_info.private_key_file.has_value()) { + managed_csr.emplace(info.key_info.private_key_file.value(), std::chrono::steady_clock::now()); + } + } else { + EVLOG_error << "CSR leaf generation error: " + << conversions::get_certificate_sign_request_result_to_string(csr_result); + + if (csr_result == CertificateSignRequestResult::KeyGenerationError) { + result.status = GetCertificateSignRequestStatus::KeyGenError; + } else { + result.status = GetCertificateSignRequestStatus::GenerationError; + } + } + + return result; +} - EVLOG_info << "Generating CSR: " << conversions::leaf_certificate_type_to_string(certificate_type); +GetCertificateSignRequestResult EvseSecurity::generate_certificate_signing_request(LeafCertificateType certificate_type, + const std::string& country, + const std::string& organization, + const std::string& common, + bool use_tpm) { + std::lock_guard guard(EvseSecurity::security_mutex); // Make a difference between normal and tpm keys for identification const auto file_name = conversions::leaf_certificate_type_to_filename(certificate_type) + filesystem_utils::get_random_file_name(use_tpm ? TPM_KEY_EXTENSION.string() : KEY_EXTENSION.string()); + fs::path key_path; if (certificate_type == LeafCertificateType::CSMS) { key_path = this->directories.csms_leaf_key_directory / file_name; } else if (certificate_type == LeafCertificateType::V2G) { key_path = this->directories.secc_leaf_key_directory / file_name; } else { - EVLOG_error << "Generate CSR: create filename failed"; - throw std::runtime_error("Attempt to generate CSR for MF certificate"); + EVLOG_error << "Generate CSR for non CSMS/V2G leafs!"; + + GetCertificateSignRequestResult result{}; + result.status = GetCertificateSignRequestStatus::InvalidRequestedType; + return result; } std::string csr; @@ -1012,23 +1047,13 @@ std::string EvseSecurity::generate_certificate_signing_request(LeafCertificateTy info.key_info.private_key_pass = private_key_password; } - EVLOG_debug << "Generate CSR start"; - - if (false == CryptoSupplier::x509_generate_csr(info, csr)) { - EVLOG_error << "Failed to generate certificate signing request!"; - } else { - // Add the key to the managed CRS that we will delete if we can't find a certificate pair within the time - managed_csr.emplace(key_path, std::chrono::steady_clock::now()); - } - - EVLOG_debug << "Generated CSR end. CSR: " << csr; - return csr; + return generate_certificate_signing_request_internal(certificate_type, info); } -std::string EvseSecurity::generate_certificate_signing_request(LeafCertificateType certificate_type, - const std::string& country, - const std::string& organization, - const std::string& common) { +GetCertificateSignRequestResult EvseSecurity::generate_certificate_signing_request(LeafCertificateType certificate_type, + const std::string& country, + const std::string& organization, + const std::string& common) { return generate_certificate_signing_request(certificate_type, country, organization, common, false); } @@ -1304,8 +1329,8 @@ bool EvseSecurity::update_certificate_links(LeafCertificateType certificate_type return changed; } -std::string EvseSecurity::get_verify_file(CaCertificateType certificate_type) { - std::lock_guard guard(EvseSecurity::security_mutex); +GetCertificateInfoResult EvseSecurity::get_ca_certificate_info_internal(CaCertificateType certificate_type) { + GetCertificateInfoResult result{}; try { // Support bundle files, in case the certificates contain @@ -1321,12 +1346,23 @@ std::string EvseSecurity::get_verify_file(CaCertificateType certificate_type) { // Get all roots and search for a valid self-signed for (auto& root : hierarchy.get_hierarchy()) { - if (root.certificate.is_selfsigned() && root.certificate.is_valid()) - return root.certificate.get_file().value_or(""); + if (root.certificate.is_selfsigned() && root.certificate.is_valid()) { + CertificateInfo info; + result.info = info; + + result.info.value().certificate = root.certificate.get_file().value(); + result.status = GetCertificateInfoStatus::Accepted; + return result; + } } - } + } else { + CertificateInfo info; + result.info = info; - return verify_file.get_path().string(); + result.info.value().certificate = verify_file.get_path(); + result.status = GetCertificateInfoStatus::Accepted; + return result; + } } catch (const CertificateLoadException& e) { EVLOG_error << "Could not obtain verify file, wrong format for certificate: " << this->ca_bundle_path_map.at(certificate_type) << " with error: " << e.what(); @@ -1335,6 +1371,27 @@ std::string EvseSecurity::get_verify_file(CaCertificateType certificate_type) { EVLOG_error << "Could not find any CA certificate for: " << conversions::ca_certificate_type_to_string(certificate_type); + result.status = GetCertificateInfoStatus::NotFound; + return result; +} + +GetCertificateInfoResult EvseSecurity::get_ca_certificate_info(CaCertificateType certificate_type) { + std::lock_guard guard(EvseSecurity::security_mutex); + + return get_ca_certificate_info_internal(certificate_type); +} + +std::string EvseSecurity::get_verify_file(CaCertificateType certificate_type) { + std::lock_guard guard(EvseSecurity::security_mutex); + + auto result = get_ca_certificate_info_internal(certificate_type); + + if (result.status == GetCertificateInfoStatus::Accepted && result.info.has_value()) { + if (result.info.value().certificate.has_value()) { + result.info.value().certificate.value().string(); + } + } + return {}; } diff --git a/tests/openssl_supplier_test.cpp b/tests/openssl_supplier_test.cpp index 69ed7c9..d1106c3 100644 --- a/tests/openssl_supplier_test.cpp +++ b/tests/openssl_supplier_test.cpp @@ -104,7 +104,7 @@ TEST_F(OpenSSLSupplierTest, x509_generate_csr) { .ip_address = std::nullopt, {CryptoKeyType::EC_prime256v1, false, std::nullopt, "pki/csr_key.pem", std::nullopt}}; auto res = OpenSSLSupplier::x509_generate_csr(csr_info, csr); - ASSERT_TRUE(res); + ASSERT_EQ(res, CertificateSignRequestResult::Valid); std::ofstream out("csr.pem"); out << csr; @@ -124,7 +124,7 @@ TEST_F(OpenSSLSupplierTest, x509_generate_csr_dns) { .ip_address = std::nullopt, {CryptoKeyType::EC_prime256v1, false, std::nullopt, "pki/csr_key.pem", std::nullopt}}; auto res = OpenSSLSupplier::x509_generate_csr(csr_info, csr); - ASSERT_TRUE(res); + ASSERT_EQ(res, CertificateSignRequestResult::Valid); #ifdef OUTPUT_CSR std::ofstream out("csr_dns.pem"); @@ -146,7 +146,7 @@ TEST_F(OpenSSLSupplierTest, x509_generate_csr_ip) { .ip_address = "127.0.0.1", {CryptoKeyType::EC_prime256v1, false, std::nullopt, "pki/csr_key.pem", std::nullopt}}; auto res = OpenSSLSupplier::x509_generate_csr(csr_info, csr); - ASSERT_TRUE(res); + ASSERT_EQ(res, CertificateSignRequestResult::Valid); #ifdef OUTPUT_CSR std::ofstream out("csr_ip.pem"); @@ -168,7 +168,7 @@ TEST_F(OpenSSLSupplierTest, x509_generate_csr_dns_ip) { .ip_address = "127.0.0.1", {CryptoKeyType::EC_prime256v1, false, std::nullopt, "pki/csr_key.pem", std::nullopt}}; auto res = OpenSSLSupplier::x509_generate_csr(csr_info, csr); - ASSERT_TRUE(res); + ASSERT_EQ(res, CertificateSignRequestResult::Valid); #ifdef OUTPUT_CSR std::ofstream out("csr_dns_ip.pem"); diff --git a/tests/tests.cpp b/tests/tests.cpp index 264922c..c02ee18 100644 --- a/tests/tests.cpp +++ b/tests/tests.cpp @@ -396,8 +396,8 @@ TEST_F(EvseSecurityTests, verify_tpm_keygen_csr) { std::string csr; - gen = CryptoSupplier::x509_generate_csr(csr_info, csr); - ASSERT_TRUE(gen); + auto csr_gen = CryptoSupplier::x509_generate_csr(csr_info, csr); + ASSERT_EQ(csr_gen, CertificateSignRequestResult::Valid); std::cout << "TPM csr: " << std::endl << csr << std::endl; @@ -408,8 +408,8 @@ TEST_F(EvseSecurityTests, verify_tpm_keygen_csr) { csr_info.key_info = info; - gen = CryptoSupplier::x509_generate_csr(csr_info, csr); - ASSERT_TRUE(gen); + csr_gen = CryptoSupplier::x509_generate_csr(csr_info, csr); + ASSERT_EQ(csr_gen, CertificateSignRequestResult::Valid); std::cout << "normal csr: " << std::endl << csr << std::endl; } @@ -1051,8 +1051,7 @@ TEST_F(EvseSecurityTestsExpired, verify_expired_leaf_deletion) { TEST_F(EvseSecurityTests, verify_expired_csr_deletion) { // Generate a CSR - std::string csr = - evse_security->generate_certificate_signing_request(LeafCertificateType::CSMS, "DE", "Pionix", "NA"); + auto csr = evse_security->generate_certificate_signing_request(LeafCertificateType::CSMS, "DE", "Pionix", "NA"); fs::path csr_key_path = evse_security->managed_csr.begin()->first; // Simulate a full fs else no deletion will take place