From dc46e2778e3a47b038f7337d4e08d91451fcaa3e Mon Sep 17 00:00:00 2001 From: Matt Benjamin Date: Sat, 22 Jun 2024 16:12:54 -0400 Subject: [PATCH] rgw_cksum: address review comments * remove rgw_cksum_pipe state enum, not needed [Casey review] * remove a format that just took a single string substitution and passed it to an iostream [Casey review] * use boost::to_upper* [Casey review] * remove unused RGW_ATTR_CKSUM_ALGORITHM decl [Casey review] * negate error code values in two places [Casey review] * split cksum digests from base type decls * resolve comment when checksum requested but not available * remove redundant memset * remove junk from rgw_blake3_digest.h * s/ldpp_dout + fmt::format/ldpp_dout_fmt/g; * fix conditional return of parts_count from RGWRados::Object::prepare(). A value for parts_count should be returned iff a *multipart* object manifest exists. * remove /tmp output test * finish moving ceph_crypto headers out of rgw_cksum.h * consume the optional in multipart_parts_count * target_attrs can be a reference (but not const) Signed-off-by: Matt Benjamin --- src/rgw/driver/rados/rgw_rados.cc | 5 +- src/rgw/rgw_blake3_digest.h | 3 - src/rgw/rgw_cksum.h | 140 +++--------------------------- src/rgw/rgw_cksum_digest.h | 1 + src/rgw/rgw_cksum_pipe.cc | 3 +- src/rgw/rgw_cksum_pipe.h | 16 +--- src/rgw/rgw_common.h | 12 +-- src/rgw/rgw_op.cc | 104 +++++++++++----------- src/rgw/rgw_rest_s3.cc | 16 ++-- src/test/rgw/test_rgw_cksum.cc | 13 --- 10 files changed, 88 insertions(+), 225 deletions(-) diff --git a/src/rgw/driver/rados/rgw_rados.cc b/src/rgw/driver/rados/rgw_rados.cc index f394a3e6ec0ba..d511094174ee8 100644 --- a/src/rgw/driver/rados/rgw_rados.cc +++ b/src/rgw/driver/rados/rgw_rados.cc @@ -6750,7 +6750,10 @@ int RGWRados::Object::Read::prepare(optional_yield y, const DoutPrefixProvider * if (manifest /* params.parts_count */) { RGWObjManifest::obj_iterator end = manifest->obj_end(dpp); auto cur_part_id = end.get_cur_part_id(); - params.parts_count = (cur_part_id == 1) ? 1 : cur_part_id - 1;; + if (cur_part_id != 0 ) { + /* end.get_cur_part_id() returns 0 for non-multipart manifests */ + params.parts_count = (cur_part_id == 1) ? 1 : cur_part_id - 1; + } } if (!astate->exists) { diff --git a/src/rgw/rgw_blake3_digest.h b/src/rgw/rgw_blake3_digest.h index f8a9fd6ade0bb..9dc51596ee389 100644 --- a/src/rgw/rgw_blake3_digest.h +++ b/src/rgw/rgw_blake3_digest.h @@ -19,9 +19,6 @@ #include #include "BLAKE3/c/blake3.h" -#define XXH_INLINE_ALL 1 /* required for streaming variants */ -#include "xxhash.h" - namespace rgw { namespace digest { class Blake3 { diff --git a/src/rgw/rgw_cksum.h b/src/rgw/rgw_cksum.h index 82298979dbff3..955b553f27de9 100644 --- a/src/rgw/rgw_cksum.h +++ b/src/rgw/rgw_cksum.h @@ -13,6 +13,7 @@ #pragma once +#include #include #include #include @@ -22,15 +23,9 @@ #include #include #include -#include -#include #include #include "fmt/format.h" -#include "common/ceph_crypto.h" #include "common/armor.h" -#include "rgw_blake3_digest.h" -#include "rgw_crc_digest.h" -#include "rgw_xxh_digest.h" #include #include "rgw_hex.h" #include "rgw_b64.h" @@ -83,11 +78,6 @@ namespace rgw { namespace cksum { namespace ba = boost::algorithm; - static inline std::string safe_upcase_str(std::string s) { - std::transform(s.begin(), s.end(), s.begin(), ::toupper); - return s; - } - class Cksum { public: static constexpr std::array checksums = @@ -112,8 +102,10 @@ namespace rgw { namespace cksum { Cksum(Type _type, const char* _armored_text) : type(_type) { const auto& ckd = checksums[uint16_t(type)]; - (void) ceph_unarmor((char*) digest.begin(), (char*) digest.begin() + ckd.digest_size, - _armored_text, _armored_text + std::strlen(_armored_text)); + (void) ceph_unarmor((char*) digest.begin(), + (char*) digest.begin() + ckd.digest_size, + _armored_text, + _armored_text + std::strlen(_armored_text)); } const char* type_string() const { @@ -138,7 +130,7 @@ namespace rgw { namespace cksum { std::string element_name() const { std::string ts{type_string()}; - return fmt::format("Checksum{}", safe_upcase_str(ts)); + return fmt::format("Checksum{}", boost::to_upper_copy(ts)); } std::string_view raw() const { @@ -150,7 +142,6 @@ namespace rgw { namespace cksum { std::string hs; const auto& ckd = checksums[uint16_t(type)]; hs.resize(ckd.armored_size); - memset(hs.data(), 0, hs.length()); ceph_armor((char*) hs.data(), (char*) hs.data() + ckd.armored_size, (char*) digest.begin(), (char*) digest.begin() + ckd.digest_size); @@ -199,6 +190,12 @@ namespace rgw { namespace cksum { static inline const std::optional no_cksum{std::nullopt}; + static inline std::string to_string(const Type type) { + std::string hs; + const auto& ckd = Cksum::checksums[uint16_t(type)]; + return ckd.name; + } + static inline Type parse_cksum_type(const char* name) { for (const auto& ck : Cksum::checksums) { @@ -226,117 +223,4 @@ namespace rgw { namespace cksum { parse_cksum_type_hdr(hdr_name) != Type::none; } /* is_cksum_hdr */ - class Digest { - public: - virtual void Restart() = 0; - virtual void Update (const unsigned char *input, size_t length) = 0; - virtual void Update(const ceph::buffer::list& bl) = 0; - virtual void Final (unsigned char *digest) = 0; - virtual ~Digest() {} - }; - - template - class TDigest : public Digest - { - T d; - public: - TDigest() {} - TDigest(TDigest&& rhs) noexcept - : d(std::move(rhs.d)) - {} - void Restart() override { d.Restart(); } - void Update(const unsigned char* data, uint64_t len) override { - d.Update(data, len); - } - void Update(const ceph::buffer::list& bl) { - for (auto& p : bl.buffers()) { - d.Update((const unsigned char *)p.c_str(), p.length()); - } - } - void Final(unsigned char* digest) override { - d.Final(digest); - } - }; - - typedef TDigest Blake3; - typedef TDigest Crc32; - typedef TDigest Crc32c; - typedef TDigest XXH3; - typedef TDigest SHA1; - typedef TDigest SHA256; - typedef TDigest SHA512; - - typedef boost::variant DigestVariant; - - struct get_digest_ptr : public boost::static_visitor - { - get_digest_ptr() {}; - Digest* operator()(const boost::blank& b) const { return nullptr; } - Digest* operator()(Blake3& digest) const { return &digest; } - Digest* operator()(Crc32& digest) const { return &digest; } - Digest* operator()(Crc32c& digest) const { return &digest; } - Digest* operator()(XXH3& digest) const { return &digest; } - Digest* operator()(SHA1& digest) const { return &digest; } - Digest* operator()(SHA256& digest) const { return &digest; } - Digest* operator()(SHA512& digest) const { return &digest; } - }; - - static inline Digest* get_digest(DigestVariant& ev) - { - return boost::apply_visitor(get_digest_ptr{}, ev); - } - - static inline DigestVariant digest_factory(const Type cksum_type) - { - switch (cksum_type) { - case Type::blake3: - return Blake3(); - break; - case Type::sha256: - return SHA256(); - break; - case Type::crc32: - return Crc32(); - break; - case Type::crc32c: - return Crc32c(); - break; - case Type::xxh3: - return XXH3(); - break; - case Type::sha512: - return SHA512(); - break; - case Type::sha1: - return SHA1(); - break; - case Type::none: - break; - }; - return boost::blank(); - } /* digest_factory */ - - static inline Cksum finalize_digest(Digest* digest, Type type) - { - Cksum cksum(type); - if (digest) { - auto data = cksum.digest.data(); - digest->Final(data); - } - return cksum; - } - - static inline std::string to_string(const Type type) { - std::string hs; - const auto& ckd = Cksum::checksums[uint16_t(type)]; - return ckd.name; - } - }} /* namespace */ diff --git a/src/rgw/rgw_cksum_digest.h b/src/rgw/rgw_cksum_digest.h index a70c892cdbcbe..ba7e3bd58c6f8 100644 --- a/src/rgw/rgw_cksum_digest.h +++ b/src/rgw/rgw_cksum_digest.h @@ -15,6 +15,7 @@ #include #include +#include "common/ceph_crypto.h" #include "rgw_blake3_digest.h" #include "rgw_crc_digest.h" #include "rgw_xxh_digest.h" diff --git a/src/rgw/rgw_cksum_pipe.cc b/src/rgw/rgw_cksum_pipe.cc index f9250303bceea..e06957e2715d5 100644 --- a/src/rgw/rgw_cksum_pipe.cc +++ b/src/rgw/rgw_cksum_pipe.cc @@ -30,8 +30,7 @@ namespace rgw::putobj { : Pipe(next), _type(_typ), dv(rgw::cksum::digest_factory(_type)), - _digest(cksum::get_digest(dv)), cksum_hdr(_hdr), - _state(State::DIGEST) + _digest(cksum::get_digest(dv)), cksum_hdr(_hdr) {} std::unique_ptr RGWPutObj_Cksum::Factory( diff --git a/src/rgw/rgw_cksum_pipe.h b/src/rgw/rgw_cksum_pipe.h index dce389ec8ffd0..fddcd283c84bc 100644 --- a/src/rgw/rgw_cksum_pipe.h +++ b/src/rgw/rgw_cksum_pipe.h @@ -19,7 +19,8 @@ #include #include #include -#include "rgw_cksum.h" +#include +#include "rgw_cksum_digest.h" #include "rgw_common.h" #include "rgw_putobj.h" @@ -76,7 +77,7 @@ namespace rgw::putobj { ix <= uint16_t(cksum::Type::blake3); ++ix) { cksum_type = cksum::Type(ix); auto hk = fmt::format("HTTP_X_AMZ_CHECKSUM_{}", - safe_upcase_str(to_string(cksum_type))); + boost::to_upper_copy(to_string(cksum_type))); auto hv = env.get(hk.c_str()); if (hv) { return @@ -90,18 +91,11 @@ namespace rgw::putobj { // PutObj filter for streaming checksums class RGWPutObj_Cksum : public rgw::putobj::Pipe { - enum class State : uint16_t { - START, - DIGEST, - FINAL - }; - cksum::Type _type; cksum::DigestVariant dv; cksum::Digest* _digest; cksum::Cksum _cksum; cksum_hdr_t cksum_hdr; - State _state; public: @@ -118,7 +112,6 @@ namespace rgw::putobj { cksum::Type type() { return _type; } cksum::Digest* digest() const { return _digest; } const cksum::Cksum& cksum() { return _cksum; }; - State state() const { return _state; } const cksum_hdr_t& header() const { return cksum_hdr; @@ -126,7 +119,6 @@ namespace rgw::putobj { const cksum::Cksum& finalize() { _cksum = finalize_digest(_digest, _type); - _state = State::FINAL; return _cksum; } @@ -137,7 +129,7 @@ namespace rgw::putobj { } VerifyResult verify(const RGWEnv& env) { - if (_state == State::DIGEST) [[likely]] { + if (_cksum.type == cksum::Type::none) [[likely]] { (void) finalize(); } auto hv = expected(env); diff --git a/src/rgw/rgw_common.h b/src/rgw/rgw_common.h index da2617f383545..c4fdd83d7fa9e 100644 --- a/src/rgw/rgw_common.h +++ b/src/rgw/rgw_common.h @@ -26,6 +26,7 @@ #include #include +#include "common/dout_fmt.h" #include "common/ceph_crypto.h" #include "common/random_string.h" #include "common/tracer.h" @@ -85,7 +86,6 @@ using ceph::crypto::MD5; #define RGW_ATTR_CORS RGW_ATTR_PREFIX "cors" #define RGW_ATTR_ETAG RGW_ATTR_PREFIX "etag" #define RGW_ATTR_CKSUM RGW_ATTR_PREFIX "cksum" -#define RGW_ATTR_CKSUM_ALGORITHM RGW_ATTR_PREFIX "x-amz-checksum-algorithm" #define RGW_ATTR_BUCKETS RGW_ATTR_PREFIX "buckets" #define RGW_ATTR_META_PREFIX RGW_ATTR_PREFIX RGW_AMZ_META_PREFIX #define RGW_ATTR_CONTENT_TYPE RGW_ATTR_PREFIX "content_type" @@ -1992,16 +1992,6 @@ static inline std::string ys_header_mangle(std::string_view name) return out; } /* ys_header_mangle */ -static inline std::string& upcase_str(std::string& s) { - std::transform(s.begin(), s.end(), s.begin(), ::toupper); - return s; -} - -static inline std::string safe_upcase_str(std::string s) { - std::transform(s.begin(), s.end(), s.begin(), ::toupper); - return s; -} - extern int rgw_bucket_parse_bucket_instance(const std::string& bucket_instance, std::string *bucket_name, std::string *bucket_id, int *shard_id); boost::intrusive_ptr diff --git a/src/rgw/rgw_op.cc b/src/rgw/rgw_op.cc index 0ca9b7e44f899..31a74e183e63b 100644 --- a/src/rgw/rgw_op.cc +++ b/src/rgw/rgw_op.cc @@ -25,7 +25,7 @@ #include "common/ceph_json.h" #include "common/static_ptr.h" #include "common/perf_counters_key.h" -#include "rgw_cksum.h" +#include "rgw_cksum_digest.h" #include "rgw_common.h" #include "rgw_tracer.h" @@ -4374,7 +4374,7 @@ void RGWPutObj::execute(optional_yield y) cksum_filter = rgw::putobj::RGWPutObj_Cksum::Factory(filter, *s->info.env); } catch (const rgw::io::Exception& e) { - op_ret = e.code().value(); + op_ret = -e.code().value(); return; } if (cksum_filter) { @@ -4527,12 +4527,14 @@ void RGWPutObj::execute(optional_yield y) cksum = get<1>(cksum_verify); if (std::get<0>(cksum_verify)) { buffer::list cksum_bl; - ldpp_dout(this, 16) - << fmt::format("{} checksum verified ", hdr.second) - << fmt::format("\n\tcomputed={} == \n\texpected= {}", - cksum->to_armor(), - cksum_filter->expected(*s->info.env)) - << dendl; + + ldpp_dout_fmt(this, 16, + "{} checksum verified " + "\n\tcomputed={} == \n\texpected={}", + hdr.second, + cksum->to_armor(), + cksum_filter->expected(*s->info.env)); + cksum->encode(cksum_bl); emplace_attr(RGW_ATTR_CKSUM, std::move(cksum_bl)); } else { @@ -4540,12 +4542,13 @@ void RGWPutObj::execute(optional_yield y) auto computed_ck = cksum->to_armor(); auto expected_ck = cksum_filter->expected(*s->info.env); - ldpp_dout(this, 4) - << fmt::format("{} content checksum mismatch", hdr.second) - << fmt::format("\n\tcalculated={} != \n\texpected={}", - computed_ck, - (!!expected_ck) ? expected_ck : "(checksum unavailable)") - << dendl; + ldpp_dout_fmt(this, 4, + "{} content checksum mismatch" + "\n\tcalculated={} != \n\texpected={}", + hdr.second, + computed_ck, + (!!expected_ck) ? expected_ck : "(checksum unavailable)"); + op_ret = -ERR_INVALID_REQUEST; return; } @@ -4735,7 +4738,7 @@ void RGWPostObj::execute(optional_yield y) cksum_filter = rgw::putobj::RGWPutObj_Cksum::Factory(filter, *s->info.env); } catch (const rgw::io::Exception& e) { - op_ret = e.code().value(); + op_ret = -e.code().value(); return; } if (cksum_filter) { @@ -4839,13 +4842,14 @@ void RGWPostObj::execute(optional_yield y) } else { /* content checksum mismatch */ const auto &hdr = cksum_filter->header(); - ldpp_dout(this, 4) << fmt::format("{} content checksum mismatch", - hdr.second) - << fmt::format( - "\n\tcalculated={} != \n\texpected={}", - cksum->to_armor(), - cksum_filter->expected(*s->info.env)) - << dendl; + + ldpp_dout_fmt(this, 4, + "{} content checksum mismatch" + "\n\tcalculated={} != \n\texpected={}", + hdr.second, + cksum->to_armor(), + cksum_filter->expected(*s->info.env)); + op_ret = -ERR_INVALID_REQUEST; return; } @@ -6464,11 +6468,12 @@ try_sum_part_cksums(const DoutPrefixProvider *dpp, } if (truncated) { - ldpp_dout(dpp, 20) - << fmt::format( - "WARNING: {} upload->list_parts {} {} truncated, again_count={}!", - __func__, num_parts, marker, again_count) - << dendl; + + ldpp_dout_fmt(dpp, 20, + "WARNING: {} upload->list_parts {} {} truncated, " + "again_count={}!", + __func__, num_parts, marker, again_count); + truncated = false; ++again_count; goto again; @@ -6488,20 +6493,21 @@ try_sum_part_cksums(const DoutPrefixProvider *dpp, for (auto& part : parts_map) { ++parts_ix; auto& part_cksum = part.second->get_cksum(); - ldpp_dout(dpp, 16) - << fmt::format("INFO: {} iterate part: {} {} {}", - __func__, parts_ix, part_cksum->type_string(), - part_cksum->to_armor()) - << dendl; + + ldpp_dout_fmt(dpp, 16, + "INFO: {} iterate part: {} {} {}", + __func__, parts_ix, part_cksum->type_string(), + part_cksum->to_armor()); + if ((part_cksum->type != cksum_type)) { /* if parts have inconsistent checksum, fail now */ - ldpp_dout(dpp, 4) - << fmt::format( - "ERROR: multipart part checksum type mismatch\n\tcomplete " - "multipart header={} part={}", - to_string(part_cksum->type), to_string(cksum_type)) - << dendl; - op_ret = -ERR_INVALID_REQUEST; + + ldpp_dout_fmt(dpp, 14, + "ERROR: multipart part checksum type mismatch\n\tcomplete " + "multipart header={} part={}", + to_string(part_cksum->type), to_string(cksum_type)); + + op_ret = -ERR_INVALID_REQUEST; return op_ret; } @@ -6517,12 +6523,11 @@ try_sum_part_cksums(const DoutPrefixProvider *dpp, /* we cannot verify this checksum, only compute it */ out_cksum = rgw::cksum::finalize_digest(digest, cksum_type); - ldpp_dout(dpp, 16) - << fmt::format("INFO: {} combined checksum {} {}-{}", - __func__, - out_cksum->type_string(), - out_cksum->to_armor(), num_parts) - << dendl; + ldpp_dout_fmt(dpp, 16, + "INFO: {} combined checksum {} {}-{}", + __func__, + out_cksum->type_string(), + out_cksum->to_armor(), num_parts); return op_ret; } /* try_sum_part_chksums */ @@ -6654,7 +6659,7 @@ void RGWCompleteMultipart::execute(optional_yield y) } } - auto target_attrs = meta_obj->get_attrs(); + auto& target_attrs = meta_obj->get_attrs(); if (cksum) { armored_cksum = @@ -6663,9 +6668,10 @@ void RGWCompleteMultipart::execute(optional_yield y) /* validate computed checksum against supplied checksum, if present */ auto [hdr_cksum, supplied_cksum] = rgw::putobj::find_hdr_cksum(*(s->info.env)); - ldpp_dout(this, 10) << fmt::format("INFO: client supplied checksum {}: {}", - hdr_cksum.header_name(), supplied_cksum) - << dendl; + + ldpp_dout_fmt(this, 10, + "INFO: client supplied checksum {}: {}", + hdr_cksum.header_name(), supplied_cksum); if (! (supplied_cksum.empty()) && (supplied_cksum != armored_cksum)) { diff --git a/src/rgw/rgw_rest_s3.cc b/src/rgw/rgw_rest_s3.cc index 60774d551e9ef..baead29ebcfa1 100644 --- a/src/rgw/rgw_rest_s3.cc +++ b/src/rgw/rgw_rest_s3.cc @@ -1,6 +1,7 @@ // -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*- // vim: ts=8 sw=2 smarttab ft=cpp +#include #include #include #include @@ -481,7 +482,7 @@ int RGWGetObj_ObjStore_S3::send_response_data(bufferlist& bl, off_t bl_ofs, } catch (const buffer::error&) {} } - if (multipart_parts_count && multipart_parts_count > 0) { + if (multipart_parts_count && *multipart_parts_count > 0) { dump_header(s, "x-amz-mp-parts-count", *multipart_parts_count); } @@ -513,7 +514,8 @@ int RGWGetObj_ObjStore_S3::send_response_data(bufferlist& bl, off_t bl_ofs, } catch (buffer::error& err) { ldpp_dout(this, 0) << "ERROR: failed to decode rgw::cksum::Cksum" << dendl; - /* XXX return error here? the user might prefer data we have */ + /* XXX agreed to handle this case as if there is no checksum + * to avoid data unavailable */ } } } /* checksum_mode */ @@ -2972,7 +2974,7 @@ int RGWPostObj_ObjStore_S3::get_params(optional_yield y) auto cksum_type = rgw::cksum::parse_cksum_type_hdr(k); if (cksum_type != rgw::cksum::Type::none) { put_prop("HTTP_X_AMZ_CHECKSUM_ALGORITHM", - safe_upcase_str(to_string(cksum_type))); + boost::to_upper_copy(to_string(cksum_type))); bufferlist& d = p.second.data; std::string v { rgw_trim_whitespace(std::string_view(d.c_str(), d.length()))}; @@ -4043,7 +4045,8 @@ void RGWInitMultipart_ObjStore_S3::send_response() dump_header_if_nonempty(s, "x-amz-abort-rule-id", rule_id); } if (cksum_algo != rgw::cksum::Type::none) { - dump_header(s, "x-amz-checksum-algorithm", safe_upcase_str(to_string(cksum_algo))); + dump_header(s, "x-amz-checksum-algorithm", + boost::to_upper_copy(to_string(cksum_algo))); } end_header(s, this, to_mime_type(s->format)); if (op_ret == 0) { @@ -4165,7 +4168,8 @@ void RGWListMultipart_ObjStore_S3::send_response() Container element that identifies who initiated the multipart upload. If the initiator is an AWS account, this element provides the same information as the Owner element. If the initiator is an IAM User, this element provides the user ARN and display name, see https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListParts.html */ if (cksum && cksum->aws()) { - s->formatter->dump_string("ChecksumAlgorithm", safe_upcase_str(cksum->type_string())); + s->formatter->dump_string("ChecksumAlgorithm", + boost::to_upper_copy(std::string(cksum->type_string()))); } for (; iter != upload->get_parts().end(); ++iter) { @@ -4180,7 +4184,7 @@ void RGWListMultipart_ObjStore_S3::send_response() auto& part_cksum = part->get_cksum(); if (part_cksum && part_cksum->aws()) { s->formatter->dump_string(part_cksum->element_name(), - fmt::format("{}", part_cksum->to_armor())); + part_cksum->to_armor()); } s->formatter->close_section(); } diff --git a/src/test/rgw/test_rgw_cksum.cc b/src/test/rgw/test_rgw_cksum.cc index d410ea2750635..3572f5994fa89 100644 --- a/src/test/rgw/test_rgw_cksum.cc +++ b/src/test/rgw/test_rgw_cksum.cc @@ -62,19 +62,6 @@ TEST(RGWCksum, Ctor) ASSERT_EQ(ck2.to_armor(), ck3.first.to_armor()); } -TEST(RGWCksum, Output) -{ - auto o_mode = std::ios::out|std::ios::trunc; - std::ofstream of; - of.open("/tmp/lorem", o_mode); - of << lorem; - of.close(); - - of.open("/tmp/dolor", o_mode); - of << dolor; - of.close(); -} - TEST(RGWCksum, DigestCRC32) { auto t = cksum::Type::crc32; -- 2.39.5