From 36622e19c7fb217f32f74aada0d224785344f565 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>
---
 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 c414e3627e419..a1d76c3aaf323 100644
--- a/src/rgw/rgw_auth_keystone.cc
+++ b/src/rgw/rgw_auth_keystone.cc
@@ -83,9 +83,6 @@ TokenEngine::get_from_keystone(const DoutPrefixProvider* dpp,
   validate.set_url(url);
 
   int ret = validate.process(y);
-  if (ret < 0) {
-    throw ret;
-  }
 
   /* NULL terminate for debug output. */
   token_body_bl.append(static_cast<char>(0));
@@ -104,6 +101,10 @@ TokenEngine::get_from_keystone(const DoutPrefixProvider* dpp,
                   << 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;
@@ -443,11 +444,6 @@ EC2Engine::get_from_keystone(const DoutPrefixProvider* dpp, const std::string_vi
 
   /* send request */
   ret = validate.process(y);
-  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() ==
@@ -457,6 +453,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;
@@ -521,18 +523,19 @@ auto EC2Engine::get_secret_from_keystone(const DoutPrefixProvider* dpp,
 
   /* send request */
   ret = secret.process(y);
+
+  /* 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 0249c8aebb04d..ea30ff868fd97 100644
--- a/src/rgw/rgw_kms.cc
+++ b/src/rgw/rgw_kms.cc
@@ -307,16 +307,17 @@ protected:
     }
 
     res = secret_req.process(y);
-    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;
@@ -937,13 +938,14 @@ static int request_key_from_barbican(const DoutPrefixProvider *dpp,
   secret_req.append_header("X-Auth-Token", barbican_token);
 
   res = secret_req.process(y);
-  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