rgw: swift: tempurl fixes for ceph #47723

mdw-at-linuxbox · 2022-08-22T07:48:29Z

rgw: swift: tempurl fixes for ceph

This is a set of 3 commits to update the swift tempurl (and formpost) logic to support sha256 and sha512 signatures. These fixes should make it possible for the tempest tests to pass.

This is intended to fix: https://tracker.ceph.com/issues/56564

mattbenjamin · 2022-08-22T11:04:21Z

src/rgw/rgw_rest_swift.cc

@@ -41,6 +41,110 @@

 using namespace std;

+template <class H, bool O>


a comment clarifying what the H and O template parameters mean would be helpful--I mean obviously H is for HMAC. It took me a long time to find the H actually used at the bottom of the stack of templates. But it seems reasonable. I speculate below that it would be clearer if we were able to somehow remove the untemplated versions, that support multiple digest sizes in all their methods...

mattbenjamin · 2022-08-22T11:15:10Z

src/rgw/rgw_rest_swift.cc

+  size_t pos = x.find(':');
+  if (pos == x.npos || pos <= 0) {
+    switch(x.length()) {
+    case CEPH_CRYPTO_HMACSHA1_DIGESTSIZE*2:


why hasn't the template split removed the need for the switch statement? i.e., you've templated out the HMAC and (um, O) parameters in descendant template types. Could the ancestor types become abstract, maybe?

The switch statement here is part of the logic that decides at run-time which of the 6 different forms of template logic it should realize. All the template logic happens strictly at compile-time, it can't replace the ultimate need to do grody old-fashioned run-time string parsing, which is what's happening here.
There are however 2 things I didn't do in this direction: firrstly, there are 2 copies of get_sig_helper, one for tempurl and one for formpost. It could be trivially one templated function, or less trivially, integrated into some other base class. There's no runtime advantage to this, and it makes include files a bit more messy.
The other thing is the template classes RGWFormPost::SignatureHelper_x and TempURLSignature::SignatureHelper_x, these both mainly contain thunks for calc() and is_equal_to(). Slightly more clever class logic might eliminate the need for those, but I kept ending up with worse kludges chasing that down.

I like the use of templates, sorry I seemed to be over reading here; I would rather we'd landed this in 22, rather than risking it getting stuck

cbodley · 2022-08-22T16:01:01Z

src/rgw/rgw_swift_auth.cc

+}; /* TempURLEngine::SignatureHelper */
+class TempURLEngine::SignatureHelper {


}; /* TempURLSignatureT */

cbodley · 2022-08-22T16:01:04Z

src/rgw/rgw_swift_auth.cc

@@ -82,7 +82,7 @@ void TempURLEngine::get_owner_info(const DoutPrefixProvider* dpp, const req_stat
  const string& bucket_name = s->init_state.url_bucket;

  /* TempURL requires that bucket and object names are specified. */
-  if (bucket_name.empty() || s->object->empty()) {
+  if (bucket_name.empty() || !s->object || s->object->empty()) {


rgw::sal::Object::empty(s->object) seems to be the canonical way to check for both nullness and emptiness

that's a useful observation, but I find this logic clear--the big risk would be missing part of the logic that is being fixed

cbodley · 2022-08-22T16:20:03Z

src/rgw/rgw_swift_auth.cc

+  virtual bool is_equal_to(const std::string& rhs) {
+    return false;
+  };
+  static SignatureHelper* get_sig_helper(std::string_view x);


please use std::unique_ptr as the return type for factory functions. otherwise it's not immediately obvious whether the caller is responsible for cleanup, or whether to use delete vs. free()

unique_ptr also means that the caller doesn't need to sprinkle deletes in all of the destructors and error paths

cbodley · 2022-08-22T16:41:53Z

src/rgw/rgw_swift_auth.cc

+
+TempURLEngine::SignatureHelper* TempURLEngine::SignatureHelper::get_sig_helper(std::string_view x) {
+  size_t pos = x.find(':');
+  if (pos == x.npos || pos <= 0) {


pos is unsigned, but when pos == 0 that means x starts with a colon - is that ever a valid input?

maybe we should drop the || pos <= 0 part here, so that x.substr(0,pos) below gives us an empty string and guarantees we return BadSignatureHelper?

is there any actual bug here?

github-actions · 2023-01-23T17:12:06Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

Functest uses Ceph in gate which doesn't support sha256 as default. Please see: - https://review.opendev.org/c/openstack/tempest/+/832771 - https://tracker.ceph.com/issues/56564 - ceph/ceph#47723 Change-Id: I3e98301feca4b68b1f9ebb4a6b20feb505b2dd01 Signed-off-by: Cédric Ollivier <cedric.ollivier@orange.com>

Functest uses Ceph in gate which doesn't support sha256 as default. Please see: - https://review.opendev.org/c/openstack/tempest/+/832771 - https://tracker.ceph.com/issues/56564 - ceph/ceph#47723 Change-Id: I3e98301feca4b68b1f9ebb4a6b20feb505b2dd01 Signed-off-by: Cédric Ollivier <cedric.ollivier@orange.com> (cherry picked from commit 507cdba)

cbodley · 2023-03-31T12:45:11Z

bump

cbodley · 2023-04-20T14:11:55Z

bump

cbodley · 2023-06-15T14:08:45Z

bump

mattbenjamin · 2023-06-15T14:26:34Z

flagged for discussion in "marcus task backlog"

github-actions · 2023-08-14T15:01:33Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

mattbenjamin · 2023-08-14T15:38:01Z

@mdw-at-linuxbox what is the status of this work?

mdw-at-linuxbox · 2023-08-18T03:13:55Z

I just force-pushed an updated copy of this. Builds [in fc37], haven't done any other testing. Apparently isn't building in github. sigh...

mdw-at-linuxbox · 2023-09-13T20:47:29Z

I pushed out an updated version that was also buildable in el9. Sadly, github was mean and threw away my comment saying that I had done so. Or at least, I thought it was buildable, not sure why "make check" failed again.

cbodley · 2023-09-13T20:50:05Z

jenkins test make check

cbodley · 2023-09-13T21:40:11Z

src/test/rgw/test_rgw_iam_policy.cc
In file included from src/test/rgw/test_rgw_iam_policy.cc:27:
In file included from src/rgw/rgw_auth_registry.h:14:
src/rgw/rgw_swift_auth.h:310:20: error: default initialization of an object of const type 'const uint32_t' (aka 'const unsigned int')
constexpr uint32_t signature_hash_size;
                   ^
                                       = 0
src/rgw/rgw_swift_auth.h:332:16: error: default initialization of an object of const type 'const uint32_t' (aka 'const unsigned int')
const uint32_t signature_hash_name_size;
               ^
                                        = 0
2 errors generated.

cbodley · 2023-09-28T14:08:36Z

^ ping @mdw-at-linuxbox

cbodley · 2023-10-12T14:08:11Z

bump

cbodley · 2023-12-08T18:05:51Z

bump

mattbenjamin · 2024-01-10T12:26:45Z

@mdw-at-linuxbox you have been revisiting this, correct?

swift tempurl (and formpost) now suport sha256 and sha512. hmacsha256 was there, adding hmacsha512 to round out the collection. Fixes: https://tracker.ceph.com/issues/56564 Signed-off-by: Marcus Watts <mwatts@redhat.com>

In openstack swift, formpost and tempurl share the same signature logic, so both now support sha1 / sha256 / sha512. Here are changes to ceph's formpost logic to match what swift does. Most of the guts of the signature logic are moved from .h to .cc because this logic is only useful to the actual formpost code. Fixes: https://tracker.ceph.com/issues/56564 Signed-off-by: Marcus Watts <mwatts@redhat.com>

mdw-at-linuxbox · 2024-01-17T04:15:28Z

I've pushed a new version. I believe the problem Casey cited above was some unrelated problem in the base. The new version is simply the same commits rebased to the latest "main". It built for me on fc37 and el9, and also built without issue at ceph-ci https://shaman.ceph.com/builds/ceph/mdw-master-tempurl-6/ I have not done any other testing on this

cbodley · 2024-01-17T13:25:17Z

thanks @mdw-at-linuxbox

It built for me on fc37 and el9, and also built without issue at ceph-ci https://shaman.ceph.com/builds/ceph/mdw-master-tempurl-6/ I have not done any other testing on this

still hitting those errors in 'make check'. unlike shaman, these builds use clang

The latest openstack swift logic supports sha256 and sha512 checksums for tempurl and formpost, which can be either hexadecimal or hashname:base64. This adds logic to inspect the incoming signature for type and to internally construct the same hash to compare. Also:: fixes an incidental crash if a malformed swift path does not contain an object name. Fixes: https://tracker.ceph.com/issues/56564 Signed-off-by: Marcus Watts <mwatts@redhat.com>

mdw-at-linuxbox · 2024-01-19T05:51:08Z

I've pushed a version that builds correctly with clang. Also built on ceph-ci here, https://shaman.ceph.com/builds/ceph/mdw-master-tempurl-7/

cbodley · 2024-01-25T15:20:20Z

from qa/suites/rgw/tempest/tasks/tempest.yaml

        # TODO(tobias-urdin): Use sha256 when supported in RadosGW
        tempurl_digest_hashlib: sha1

we'll need to change this to get test coverage

Signed-off-by: Casey Bodley <cbodley@redhat.com>

cbodley · 2024-01-26T19:46:42Z

jenkins test make check

cbodley · 2024-01-26T19:46:47Z

jenkins test windows

cbodley · 2024-01-26T19:46:56Z

tempest tests passed in https://pulpito.ceph.com/cbodley-2024-01-25_21:58:41-rgw:tempest-wip-master-tempurl-distro-default-smithi/

mdw-at-linuxbox · 2024-01-30T09:12:56Z

I'm having trouble figuring out why "make check" fails here. From the leg messgaes linked above, it appears it couldn't load a cls library, which sugests some kind of link problem.

I haven't been able to reproduce that problem, mostly because I don't know how to get "make check" to run just the piece I care about, and the whole thing, as best I can tell, takes hours to run. So far, I think I understand the unrelated environment problems that made my setup give up after 90 minutes, well before the failure reported above.

So am I looking at an "expected" problem, that I should ignore, or is this behavior I've introduced and need to solve?

cbodley · 2024-01-31T20:17:10Z

jenkins test make check

mdw-at-linuxbox requested review from dang, tchaikov, mattbenjamin, cbodley and prateek2211 August 22, 2022 07:48

github-actions bot added common rgw labels Aug 22, 2022

mattbenjamin reviewed Aug 22, 2022

View reviewed changes

cbodley changed the title ~~Wip master tempurl~~ rgw: swift: tempurl fixes for ceph Aug 22, 2022

cbodley reviewed Aug 22, 2022

View reviewed changes

github-actions bot added the needs-rebase label Jan 23, 2023

cbodley mentioned this pull request Mar 31, 2023

qa/rgw: unpin distro in several subsuites #50754

Closed

github-actions bot added the stale label Aug 14, 2023

github-actions bot removed the stale label Aug 14, 2023

mdw-at-linuxbox force-pushed the wip-master-tempurl branch from 2d9d8f8 to 8c6c135 Compare August 17, 2023 23:26

mdw-at-linuxbox requested a review from a team as a code owner August 17, 2023 23:26

github-actions bot removed the needs-rebase label Aug 17, 2023

mdw-at-linuxbox force-pushed the wip-master-tempurl branch from 8c6c135 to 161d0f1 Compare August 19, 2023 06:03

mdw-at-linuxbox added 2 commits January 11, 2024 17:54

common: hmacsha512 for radosgw

c32a042

swift tempurl (and formpost) now suport sha256 and sha512. hmacsha256 was there, adding hmacsha512 to round out the collection. Fixes: https://tracker.ceph.com/issues/56564 Signed-off-by: Marcus Watts <mwatts@redhat.com>

mdw-at-linuxbox force-pushed the wip-master-tempurl branch from 161d0f1 to 1d838bb Compare January 17, 2024 04:01

mdw-at-linuxbox force-pushed the wip-master-tempurl branch from 1d838bb to e2023d2 Compare January 18, 2024 05:46

cbodley added the needs-qa label Jan 18, 2024

cbodley approved these changes Jan 18, 2024

View reviewed changes

qa/tempest: use default tempurl_digest_hashlib=sha256

5d0477e

Signed-off-by: Casey Bodley <cbodley@redhat.com>

cbodley added the wip-cbodley-testing label Jan 25, 2024

cbodley added ready-to-merge and removed needs-qa wip-cbodley-testing common labels Jan 26, 2024

cbodley merged commit 8e554bf into ceph:main Jan 31, 2024
10 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rgw: swift: tempurl fixes for ceph #47723

rgw: swift: tempurl fixes for ceph #47723

mdw-at-linuxbox commented Aug 22, 2022

mattbenjamin Aug 22, 2022

mattbenjamin Aug 22, 2022

mdw-at-linuxbox Aug 23, 2022

mattbenjamin Jan 4, 2024

cbodley Aug 22, 2022

cbodley Aug 22, 2022

mattbenjamin Jan 4, 2024

cbodley Aug 22, 2022

cbodley Aug 22, 2022

mattbenjamin Jan 4, 2024

github-actions bot commented Jan 23, 2023

cbodley commented Mar 31, 2023

cbodley commented Apr 20, 2023

cbodley commented Jun 15, 2023

mattbenjamin commented Jun 15, 2023

github-actions bot commented Aug 14, 2023

mattbenjamin commented Aug 14, 2023

mdw-at-linuxbox commented Aug 18, 2023

mdw-at-linuxbox commented Sep 13, 2023

cbodley commented Sep 13, 2023

cbodley commented Sep 13, 2023

cbodley commented Sep 28, 2023

cbodley commented Oct 12, 2023

cbodley commented Dec 8, 2023

mattbenjamin commented Jan 10, 2024

mdw-at-linuxbox commented Jan 17, 2024

cbodley commented Jan 17, 2024

mdw-at-linuxbox commented Jan 19, 2024

cbodley commented Jan 25, 2024

cbodley commented Jan 26, 2024

cbodley commented Jan 26, 2024

cbodley commented Jan 26, 2024

mdw-at-linuxbox commented Jan 30, 2024

cbodley commented Jan 31, 2024

		@@ -41,6 +41,110 @@

		using namespace std;

		template <class H, bool O>

		}; /* TempURLEngine::SignatureHelper */
		class TempURLEngine::SignatureHelper {

rgw: swift: tempurl fixes for ceph #47723

rgw: swift: tempurl fixes for ceph #47723

Conversation

mdw-at-linuxbox commented Aug 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 23, 2023

cbodley commented Mar 31, 2023

cbodley commented Apr 20, 2023

cbodley commented Jun 15, 2023

mattbenjamin commented Jun 15, 2023

github-actions bot commented Aug 14, 2023

mattbenjamin commented Aug 14, 2023

mdw-at-linuxbox commented Aug 18, 2023

mdw-at-linuxbox commented Sep 13, 2023

cbodley commented Sep 13, 2023

cbodley commented Sep 13, 2023

cbodley commented Sep 28, 2023

cbodley commented Oct 12, 2023

cbodley commented Dec 8, 2023

mattbenjamin commented Jan 10, 2024

mdw-at-linuxbox commented Jan 17, 2024

cbodley commented Jan 17, 2024

mdw-at-linuxbox commented Jan 19, 2024

cbodley commented Jan 25, 2024

cbodley commented Jan 26, 2024

cbodley commented Jan 26, 2024

cbodley commented Jan 26, 2024

mdw-at-linuxbox commented Jan 30, 2024

cbodley commented Jan 31, 2024