From a51e78a75f85f094efcac074c0a85b7440fe6be0 Mon Sep 17 00:00:00 2001 From: Matt Benjamin Date: Sat, 23 Apr 2016 15:41:02 -0400 Subject: [PATCH] rgw: reduce string copies in tempURL processing Introduce a helper class encapsulating the temp URL HMAC computation, and also save it for later computation. Use the underlying HMACSHA1 implementation to compute the HMAC incrementally (saves a copy of all the parameters). Also avoid some other string copying in the logic prior to taking the HMAC, in one case by remembering to use auto& in a std::map range-for loop, and using boost::string_ref to share ReqState::request_uri and conditionally suffix of same. Signed-off-by: Matt Benjamin Signed-off-by: Radoslaw Zarzynski Conflicts: src/rgw/rgw_swift.cc --- src/rgw/rgw_swift_auth.cc | 82 ++++++++++++++++++++++++++------------- src/rgw/rgw_swift_auth.h | 5 +-- 2 files changed, 57 insertions(+), 30 deletions(-) diff --git a/src/rgw/rgw_swift_auth.cc b/src/rgw/rgw_swift_auth.cc index 43816b7a41a4a..190e616247574 100644 --- a/src/rgw/rgw_swift_auth.cc +++ b/src/rgw/rgw_swift_auth.cc @@ -1,6 +1,8 @@ // -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*- // vim: ts=8 sw=2 smarttab +#include + #include "rgw_swift_auth.h" #include "rgw_rest.h" @@ -136,29 +138,52 @@ std::string extract_swift_subuser(const std::string& swift_user_name) { } } -std::string RGWTempURLAuthEngine::generate_signature(const string& key, - const string& method, - const string& path, - const string& expires) const +class TempURLSig { - const string str = method + "\n" + expires + "\n" + path; - ldout(cct, 20) << "temp url signature (plain text): " << str << dendl; +private: + static constexpr uint32_t output_size = + CEPH_CRYPTO_HMACSHA1_DIGESTSIZE * 2 + 1; - /* unsigned */ char dest[CEPH_CRYPTO_HMACSHA1_DIGESTSIZE]; - calc_hmac_sha1(key.c_str(), key.size(), - str.c_str(), str.size(), - dest); + unsigned char dest[CEPH_CRYPTO_HMACSHA1_DIGESTSIZE]; // 20 + char dest_str[output_size]; - char dest_str[CEPH_CRYPTO_HMACSHA1_DIGESTSIZE * 2 + 1]; - buf_to_hex((const unsigned char *)dest, sizeof(dest), dest_str); +public: + TempURLSig() {} - return dest_str; -} + const char* calc(const string& key, + const string& method, + const boost::string_ref& path, + const string& expires) { + + using ceph::crypto::HMACSHA1; + using UCHARPTR = const unsigned char*; + + HMACSHA1 hmac((UCHARPTR) key.c_str(), key.size()); + hmac.Update((UCHARPTR) method.c_str(), method.size()); + hmac.Update((UCHARPTR) "\n", 1); + hmac.Update((UCHARPTR) expires.c_str(), expires.size()); + hmac.Update((UCHARPTR) "\n", 1); + hmac.Update((UCHARPTR) path.data(), path.size()); + hmac.Final(dest); + + buf_to_hex((UCHARPTR) dest, sizeof(dest), dest_str); + + return dest_str; + } + + int comp(const std::string& rhs) { + /* never allow out-of-range exception */ + if (rhs.size() < output_size) + return -1; + return rhs.compare(0 /* pos */, output_size, dest_str); + } + +}; /* TempURLSig */ RGWAuthApplier::aplptr_t RGWTempURLAuthEngine::authenticate() const { - const string temp_url_sig = s->info.args.get("temp_url_sig"); - const string temp_url_expires = s->info.args.get("temp_url_expires"); + const string& temp_url_sig = s->info.args.get("temp_url_sig"); + const string& temp_url_expires = s->info.args.get("temp_url_expires"); if (temp_url_sig.empty() || temp_url_expires.empty()) { return nullptr; } @@ -184,13 +209,18 @@ RGWAuthApplier::aplptr_t RGWTempURLAuthEngine::authenticate() const /* We need to verify two paths because of compliance with Swift, Tempest * and old versions of RadosGW. The second item will have the prefix * of Swift API entry point removed. */ + + /* XXX can we search this ONCE? */ const size_t pos = g_conf->rgw_swift_url_prefix.find_last_not_of('/') + 1; - const std::vector allowed_paths = { - s->info.request_uri, - s->info.request_uri.substr(pos + 1) + boost::string_ref ref_uri = s->info.request_uri; + const vector allowed_paths = { + ref_uri, + ref_uri.substr(pos + 1) }; /* Account owner calculates the signature also against a HTTP method. */ + /* XXX Boost 1.6 small_vector--or maybe, could we select from a few static + * vectors? */ std::vector allowed_methods; if (strcmp("HEAD", s->info.method) == 0) { /* HEAD requests are specially handled. */ @@ -200,7 +230,8 @@ RGWAuthApplier::aplptr_t RGWTempURLAuthEngine::authenticate() const } /* Need to try each combination of keys, allowed path and methods. */ - for (const auto kv : owner_info.temp_url_keys) { + TempURLSig sig_helper; + for (const auto& kv : owner_info.temp_url_keys) { const int temp_url_key_num = kv.first; const string& temp_url_key = kv.second; @@ -208,17 +239,16 @@ RGWAuthApplier::aplptr_t RGWTempURLAuthEngine::authenticate() const continue; } - for (const auto path : allowed_paths) { - for (const auto method : allowed_methods) { - const std::string local_sig = generate_signature(temp_url_key, - method, path, - temp_url_expires); + for (const auto& path : allowed_paths) { + for (const auto& method : allowed_methods) { + const char* local_sig = sig_helper.calc(temp_url_key, method, path, + temp_url_expires); ldout(s->cct, 20) << "temp url signature [" << temp_url_key_num << "] (calculated): " << local_sig << dendl; - if (local_sig != temp_url_sig) { + if (!!sig_helper.comp(temp_url_sig)) { ldout(s->cct, 5) << "temp url signature mismatch: " << local_sig << " != " << temp_url_sig << dendl; } else { diff --git a/src/rgw/rgw_swift_auth.h b/src/rgw/rgw_swift_auth.h index 1dd63fe756279..f60ffd8451df1 100644 --- a/src/rgw/rgw_swift_auth.h +++ b/src/rgw/rgw_swift_auth.h @@ -37,10 +37,7 @@ protected: /* Helper methods. */ void get_owner_info(RGWUserInfo& owner_info) const; bool is_expired(const std::string& expires) const; - std::string generate_signature(const string& key, - const string& method, - const string& path, - const string& expires) const; + public: RGWTempURLAuthEngine(const req_state * const s, /*const*/ RGWRados * const store, -- 2.39.5