From 44550e2f00587e09846089570fa0a03bfa743a9d Mon Sep 17 00:00:00 2001
From: Casey Bodley <cbodley@redhat.com>
Date: Thu, 5 Oct 2023 11:59:52 -0400
Subject: [PATCH] rgw: fix http error checks in keystone/barbican/vault clients

when RGWHTTPManager encounters an http error, it uses
rgw_http_error_to_errno() to map that to a negative posix error code.
RGWHTTPClient::process() returns that mapped error code, and exposes the
original http error via get_http_status()

the http client code for keystone, barbican, and vault were returning
early on the errors from process(), so weren't getting to the http error
checks

these clients now check for specific http errors before testing the
result of process()

Fixes: https://tracker.ceph.com/issues/62989

Signed-off-by: Casey Bodley <cbodley@redhat.com>
(cherry picked from commit 36622e19c7fb217f32f74aada0d224785344f565)

Conflicts:
	src/rgw/rgw_auth_keystone.cc null_yield
	src/rgw/rgw_kms.cc null_yield
---
 src/rgw/rgw_auth_keystone.cc | 31 +++++++++++++++++--------------
 src/rgw/rgw_kms.cc           | 16 +++++++++-------
 2 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/src/rgw/rgw_auth_keystone.cc b/src/rgw/rgw_auth_keystone.cc
index 035b4c3020411..81588d50c4fc9 100644
--- a/src/rgw/rgw_auth_keystone.cc
+++ b/src/rgw/rgw_auth_keystone.cc
@@ -80,9 +80,6 @@ TokenEngine::get_from_keystone(const DoutPrefixProvider* dpp, const std::string&
   validate.set_url(url);
 
   int ret = validate.process(null_yield);
-  if (ret < 0) {
-    throw ret;
-  }
 
   /* NULL terminate for debug output. */
   token_body_bl.append(static_cast<char>(0));
@@ -101,6 +98,10 @@ TokenEngine::get_from_keystone(const DoutPrefixProvider* dpp, const std::string&
                   << validate.get_http_status() << dendl;
     return boost::none;
   }
+  // throw any other http or connection errors
+  if (ret < 0) {
+    throw ret;
+  }
 
   ldpp_dout(dpp, 20) << "received response status=" << validate.get_http_status()
                  << ", body=" << token_body_bl.c_str() << dendl;
@@ -425,11 +426,6 @@ EC2Engine::get_from_keystone(const DoutPrefixProvider* dpp, const std::string_vi
 
   /* send request */
   ret = validate.process(null_yield);
-  if (ret < 0) {
-    ldpp_dout(dpp, 2) << "s3 keystone: token validation ERROR: "
-                  << token_body_bl.c_str() << dendl;
-    throw ret;
-  }
 
   /* if the supplied signature is wrong, we will get 401 from Keystone */
   if (validate.get_http_status() ==
@@ -439,6 +435,12 @@ EC2Engine::get_from_keystone(const DoutPrefixProvider* dpp, const std::string_vi
           decltype(validate)::HTTP_STATUS_NOTFOUND) {
     return std::make_pair(boost::none, -ERR_INVALID_ACCESS_KEY);
   }
+  // throw any other http or connection errors
+  if (ret < 0) {
+    ldpp_dout(dpp, 2) << "s3 keystone: token validation ERROR: "
+                  << token_body_bl.c_str() << dendl;
+    throw ret;
+  }
 
   /* now parse response */
   rgw::keystone::TokenEnvelope token_envelope;
@@ -501,18 +503,19 @@ std::pair<boost::optional<std::string>, int> EC2Engine::get_secret_from_keystone
 
   /* send request */
   ret = secret.process(null_yield);
+
+  /* if the supplied access key isn't found, we will get 404 from Keystone */
+  if (secret.get_http_status() ==
+          decltype(secret)::HTTP_STATUS_NOTFOUND) {
+    return make_pair(boost::none, -ERR_INVALID_ACCESS_KEY);
+  }
+  // return any other http or connection errors
   if (ret < 0) {
     ldpp_dout(dpp, 2) << "s3 keystone: secret fetching error: "
                   << token_body_bl.c_str() << dendl;
     return make_pair(boost::none, ret);
   }
 
-  /* if the supplied signature is wrong, we will get 401 from Keystone */
-  if (secret.get_http_status() ==
-          decltype(secret)::HTTP_STATUS_NOTFOUND) {
-    return make_pair(boost::none, -EINVAL);
-  }
-
   /* now parse response */
 
   JSONParser parser;
diff --git a/src/rgw/rgw_kms.cc b/src/rgw/rgw_kms.cc
index eec5d80daab13..936580276680d 100644
--- a/src/rgw/rgw_kms.cc
+++ b/src/rgw/rgw_kms.cc
@@ -306,16 +306,17 @@ protected:
     }
 
     res = secret_req.process(null_yield);
-    if (res < 0) {
-      ldpp_dout(dpp, 0) << "ERROR: Request to Vault failed with error " << res << dendl;
-      return res;
-    }
 
+    // map 401 to EACCES instead of EPERM
     if (secret_req.get_http_status() ==
         RGWHTTPTransceiver::HTTP_STATUS_UNAUTHORIZED) {
       ldpp_dout(dpp, 0) << "ERROR: Vault request failed authorization" << dendl;
       return -EACCES;
     }
+    if (res < 0) {
+      ldpp_dout(dpp, 0) << "ERROR: Request to Vault failed with error " << res << dendl;
+      return res;
+    }
 
     ldpp_dout(dpp, 20) << "Request to Vault returned " << res << " and HTTP status "
       << secret_req.get_http_status() << dendl;
@@ -925,13 +926,14 @@ static int request_key_from_barbican(const DoutPrefixProvider *dpp,
   secret_req.append_header("X-Auth-Token", barbican_token);
 
   res = secret_req.process(null_yield);
-  if (res < 0) {
-    return res;
-  }
+  // map 401 to EACCES instead of EPERM
   if (secret_req.get_http_status() ==
       RGWHTTPTransceiver::HTTP_STATUS_UNAUTHORIZED) {
     return -EACCES;
   }
+  if (res < 0) {
+    return res;
+  }
 
   if (secret_req.get_http_status() >=200 &&
       secret_req.get_http_status() < 300 &&
-- 
2.39.5