From ef0d9d1c43cf51eacdacf6c21bc1aea3e3471a96 Mon Sep 17 00:00:00 2001 From: Priya Sehgal Date: Fri, 30 Jul 2021 10:32:26 +0530 Subject: [PATCH] rgw: Address review comments, fix test cases for rgw_kms Addressed earlier review comments on having const class members. Resolved the make check problem. Signed-off-by: Priya Sehgal (cherry picked from commit 38e8f63baa73be04b0e844df2b33620ea7ee669f) [ merge conflict in src/rgw/rgw_crypt.cc -- will address separately... -mdw ] --- src/rgw/rgw_crypt.cc | 2 +- src/rgw/rgw_kms.cc | 68 ++++++++++++++++++------------------ src/test/rgw/test_rgw_kms.cc | 21 ++++++----- 3 files changed, 47 insertions(+), 44 deletions(-) diff --git a/src/rgw/rgw_crypt.cc b/src/rgw/rgw_crypt.cc index fb6ea9956b279..0d75749472fbb 100644 --- a/src/rgw/rgw_crypt.cc +++ b/src/rgw/rgw_crypt.cc @@ -1103,7 +1103,7 @@ int rgw_s3_prepare_encrypt(struct req_state* s, } /* Checking bucket attributes if SSE is enabled. Currently only supporting SSE-S3 */ - rgw::sal::Attrs buck_attrs(s->bucket_attrs); + const auto& buck_attrs = s->bucket_attrs; auto aiter = buck_attrs.find(RGW_ATTR_BUCKET_ENCRYPTION_POLICY); if (aiter != buck_attrs.end()) { ldpp_dout(s, 5) << "Found RGW_ATTR_BUCKET_ENCRYPTION_POLICY on " diff --git a/src/rgw/rgw_kms.cc b/src/rgw/rgw_kms.cc index 86975c7944941..53ea9c82c5b56 100644 --- a/src/rgw/rgw_kms.cc +++ b/src/rgw/rgw_kms.cc @@ -185,17 +185,17 @@ class SSEContext { protected: virtual ~SSEContext(){}; public: - virtual std::string & backend() = 0; - virtual std::string & addr() = 0; - virtual std::string & auth() = 0; - virtual std::string & k_namespace() = 0; - virtual std::string & prefix() = 0; + virtual const std::string & backend() = 0; + virtual const std::string & addr() = 0; + virtual const std::string & auth() = 0; + virtual const std::string & k_namespace() = 0; + virtual const std::string & prefix() = 0; virtual const std::string & secret_engine() = 0; - virtual std::string & ssl_cacert() = 0; - virtual std::string & ssl_clientcert() = 0; - virtual std::string & ssl_clientkey() = 0; - virtual std::string & token_file() = 0; - virtual bool verify_ssl() = 0; + virtual const std::string & ssl_cacert() = 0; + virtual const std::string & ssl_clientcert() = 0; + virtual const std::string & ssl_clientkey() = 0; + virtual const std::string & token_file() = 0; + virtual const bool verify_ssl() = 0; }; class VaultSecretEngine: public SecretEngine { @@ -1085,37 +1085,37 @@ class KMSContext : public SSEContext { public: KMSContext(CephContext*_cct) : cct{_cct} {}; ~KMSContext() override {}; - std::string & backend() override { + const std::string & backend() override { return cct->_conf->rgw_crypt_s3_kms_backend; }; - std::string & addr() override { + const std::string & addr() override { return cct->_conf->rgw_crypt_vault_addr; }; - std::string & auth() override { + const std::string & auth() override { return cct->_conf->rgw_crypt_vault_auth; }; - std::string & k_namespace() override { + const std::string & k_namespace() override { return cct->_conf->rgw_crypt_vault_namespace; }; - std::string & prefix() override { + const std::string & prefix() override { return cct->_conf->rgw_crypt_vault_prefix; }; const std::string & secret_engine() override { return cct->_conf->rgw_crypt_vault_secret_engine; }; - std::string & ssl_cacert() override { + const std::string & ssl_cacert() override { return cct->_conf->rgw_crypt_vault_ssl_cacert; }; - std::string & ssl_clientcert() override { + const std::string & ssl_clientcert() override { return cct->_conf->rgw_crypt_vault_ssl_clientcert; }; - std::string & ssl_clientkey() override { + const std::string & ssl_clientkey() override { return cct->_conf->rgw_crypt_vault_ssl_clientkey; }; - std::string & token_file() override { + const std::string & token_file() override { return cct->_conf->rgw_crypt_vault_token_file; }; - bool verify_ssl() override { + const bool verify_ssl() override { return cct->_conf->rgw_crypt_vault_verify_ssl; }; }; @@ -1126,37 +1126,37 @@ public: static const std::string sse_s3_secret_engine; SseS3Context(CephContext*_cct) : cct{_cct} {}; ~SseS3Context(){}; - std::string & backend() override { + const std::string & backend() override { return cct->_conf->rgw_crypt_sse_s3_backend; }; - std::string & addr() override { + const std::string & addr() override { return cct->_conf->rgw_crypt_sse_s3_vault_addr; }; - std::string & auth() override { + const std::string & auth() override { return cct->_conf->rgw_crypt_sse_s3_vault_auth; }; - std::string & k_namespace() override { + const std::string & k_namespace() override { return cct->_conf->rgw_crypt_sse_s3_vault_namespace; }; - std::string & prefix() override { + const std::string & prefix() override { return cct->_conf->rgw_crypt_sse_s3_vault_prefix; }; const std::string & secret_engine() override { return sse_s3_secret_engine; }; - std::string & ssl_cacert() override { + const std::string & ssl_cacert() override { return cct->_conf->rgw_crypt_sse_s3_vault_ssl_cacert; }; - std::string & ssl_clientcert() override { + const std::string & ssl_clientcert() override { return cct->_conf->rgw_crypt_sse_s3_vault_ssl_clientcert; }; - std::string & ssl_clientkey() override { + const std::string & ssl_clientkey() override { return cct->_conf->rgw_crypt_sse_s3_vault_ssl_clientkey; }; - std::string & token_file() override { + const std::string & token_file() override { return cct->_conf->rgw_crypt_sse_s3_vault_token_file; }; - bool verify_ssl() override { + const bool verify_ssl() override { return cct->_conf->rgw_crypt_sse_s3_vault_verify_ssl; }; }; @@ -1168,7 +1168,7 @@ int reconstitute_actual_key_from_kms(const DoutPrefixProvider *dpp, CephContext { std::string key_id = get_str_attribute(attrs, RGW_ATTR_CRYPT_KEYID); KMSContext kctx { cct }; - std::string &kms_backend { kctx.backend() }; + const std::string &kms_backend { kctx.backend() }; ldpp_dout(dpp, 20) << "Getting KMS encryption key for key " << key_id << dendl; ldpp_dout(dpp, 20) << "SSE-KMS backend is " << kms_backend << dendl; @@ -1199,7 +1199,7 @@ int make_actual_key_from_kms(const DoutPrefixProvider *dpp, CephContext *cct, std::string& actual_key) { KMSContext kctx { cct }; - std::string &kms_backend { kctx.backend() }; + const std::string &kms_backend { kctx.backend() }; if (RGW_SSE_KMS_BACKEND_VAULT == kms_backend) return make_actual_key_from_vault(dpp, cct, kctx, attrs, actual_key); return reconstitute_actual_key_from_kms(dpp, cct, attrs, actual_key); @@ -1212,7 +1212,7 @@ int reconstitute_actual_key_from_sse_s3(const DoutPrefixProvider *dpp, { std::string key_id = get_str_attribute(attrs, RGW_ATTR_CRYPT_KEYID); SseS3Context kctx { cct }; - std::string &kms_backend { kctx.backend() }; + const std::string &kms_backend { kctx.backend() }; ldpp_dout(dpp, 20) << "Getting SSE-S3 encryption key for key " << key_id << dendl; ldpp_dout(dpp, 20) << "SSE-KMS backend is " << kms_backend << dendl; @@ -1231,7 +1231,7 @@ int make_actual_key_from_sse_s3(const DoutPrefixProvider *dpp, std::string& actual_key) { SseS3Context kctx { cct }; - std::string kms_backend { kctx.backend() }; + const std::string kms_backend { kctx.backend() }; if (RGW_SSE_KMS_BACKEND_VAULT != kms_backend) { ldpp_dout(dpp, 0) << "ERROR: Unsupported rgw_crypt_sse_s3_backend: " << kms_backend << dendl; return -EINVAL; diff --git a/src/test/rgw/test_rgw_kms.cc b/src/test/rgw/test_rgw_kms.cc index 148f0bd32653d..14319d7396706 100644 --- a/src/test/rgw/test_rgw_kms.cc +++ b/src/test/rgw/test_rgw_kms.cc @@ -18,7 +18,7 @@ using ::testing::StrEq; class MockTransitSecretEngine : public TransitSecretEngine { public: - MockTransitSecretEngine(CephContext *cct, EngineParmMap parms) : TransitSecretEngine(cct, parms){} + MockTransitSecretEngine(CephContext *cct, SSEContext & kctx, EngineParmMap parms) : TransitSecretEngine(cct, kctx, parms){} MOCK_METHOD(int, send_request, (const DoutPrefixProvider *dpp, const char *method, std::string_view infix, std::string_view key_id, const std::string& postdata, bufferlist &bl), (override)); @@ -27,7 +27,7 @@ public: class MockKvSecretEngine : public KvSecretEngine { public: - MockKvSecretEngine(CephContext *cct, EngineParmMap parms) : KvSecretEngine(cct, parms){} + MockKvSecretEngine(CephContext *cct, SSEContext & kctx, EngineParmMap parms) : KvSecretEngine(cct, kctx, parms){} MOCK_METHOD(int, send_request, (const DoutPrefixProvider *dpp, const char *method, std::string_view infix, std::string_view key_id, const std::string& postdata, bufferlist &bl), (override)); @@ -44,11 +44,12 @@ protected: void SetUp() override { EngineParmMap old_parms, kv_parms, new_parms; cct = (new CephContext(CEPH_ENTITY_TYPE_ANY))->get(); + KMSContext kctx { cct }; old_parms["compat"] = "2"; - old_engine = new MockTransitSecretEngine(cct, std::move(old_parms)); - kv_engine = new MockKvSecretEngine(cct, std::move(kv_parms)); + old_engine = new MockTransitSecretEngine(cct, kctx, std::move(old_parms)); + kv_engine = new MockKvSecretEngine(cct, kctx, std::move(kv_parms)); new_parms["compat"] = "1"; - transit_engine = new MockTransitSecretEngine(cct, std::move(new_parms)); + transit_engine = new MockTransitSecretEngine(cct, kctx, std::move(new_parms)); } void TearDown() { @@ -64,8 +65,9 @@ TEST_F(TestSSEKMS, vault_token_file_unset) { cct->_conf.set_val("rgw_crypt_vault_auth", "token"); EngineParmMap old_parms, kv_parms; - TransitSecretEngine te(cct, std::move(old_parms)); - KvSecretEngine kv(cct, std::move(kv_parms)); + KMSContext kctx { cct }; + TransitSecretEngine te(cct, kctx, std::move(old_parms)); + KvSecretEngine kv(cct, kctx, std::move(kv_parms)); const NoDoutPrefix no_dpp(cct, 1); std::string_view key_id("my_key"); @@ -81,8 +83,9 @@ TEST_F(TestSSEKMS, non_existent_vault_token_file) cct->_conf.set_val("rgw_crypt_vault_auth", "token"); cct->_conf.set_val("rgw_crypt_vault_token_file", "/nonexistent/file"); EngineParmMap old_parms, kv_parms; - TransitSecretEngine te(cct, std::move(old_parms)); - KvSecretEngine kv(cct, std::move(kv_parms)); + KMSContext kctx { cct }; + TransitSecretEngine te(cct, kctx, std::move(old_parms)); + KvSecretEngine kv(cct, kctx, std::move(kv_parms)); const NoDoutPrefix no_dpp(cct, 1); std::string_view key_id("my_key/1"); -- 2.39.5