crimson/auth: refactor AuthClient interface

* we'd better return value by "return" not by passing output prameters by
  pointers.
* remove unused parameters from AuthClient::handle_auth_done(),
  `session_key` and `connection_secret` are returned by setting
  corresponding member variables of `auth_meta`.

Signed-off-by: Kefu Chai <kchai@redhat.com>

diff --git a/src/crimson/auth/AuthClient.h b/src/crimson/auth/AuthClient.h
index 236707b66ea81ff67ab24ba6ce1becdd201a0684..f8d0685673bcbcc4ab6b3ca7f205d146ba25cd0a 100644 (file)
--- a/src/crimson/auth/AuthClient.h
+++ b/src/crimson/auth/AuthClient.h
@@ -3,7 +3,11 @@
 
 #pragma once
 
+#include <cstdint>
+#include <string>
+#include <tuple>
 #include <vector>
+#include "include/buffer_fwd.h"
 #include "crimson/net/Fwd.h"
 
 class CryptoKey;
@@ -15,37 +19,44 @@ public:
   using std::logic_error::logic_error;
 };
 
+using method_t = uint32_t;
+
 // TODO: revisit interfaces for non-dummy implementations
 class AuthClient {
 public:
   virtual ~AuthClient() {}
 
-  // Build an authentication request to begin the handshake
-  virtual int get_auth_request(
-    ceph::net::ConnectionRef conn,
-    AuthConnectionMetaRef auth_meta,
-    uint32_t *method,
-    std::vector<uint32_t> *preferred_modes,
-    bufferlist *out) = 0;
+  /// Build an authentication request to begin the handshake
+  ///
+  /// @throw auth::error if unable to build the request
+  virtual std::tuple<method_t,              // auth method
+                    std::vector<uint32_t>,  // preferred modes
+                    ceph::bufferlist>       // auth bl
+  get_auth_request(ceph::net::ConnectionRef conn,
+                  AuthConnectionMetaRef auth_meta) = 0;
 
-  // Handle server's request to continue the handshake
-  virtual int handle_auth_reply_more(
+  /// Handle server's request to continue the handshake
+  ///
+  /// @throw auth::error if unable to build the request
+  virtual ceph::bufferlist handle_auth_reply_more(
     ceph::net::ConnectionRef conn,
     AuthConnectionMetaRef auth_meta,
-    const bufferlist& bl,
-    bufferlist *reply) = 0;
+    const ceph::bufferlist& bl) = 0;
 
-  // Handle server's indication that authentication succeeded
+  /// Handle server's indication that authentication succeeded
+  ///
+  /// @return 0 if authenticated, a negative number otherwise
   virtual int handle_auth_done(
     ceph::net::ConnectionRef conn,
     AuthConnectionMetaRef auth_meta,
     uint64_t global_id,
     uint32_t con_mode,
-    const bufferlist& bl,
-    CryptoKey *session_key,
-    std::string *connection_secret) = 0;
+    const bufferlist& bl) = 0;
 
-  // Handle server's indication that the previous auth attempt failed
+  /// Handle server's indication that the previous auth attempt failed
+  ///
+  /// @return 0 if will try next auth method, a negative number if we have no
+  ///         more options
   virtual int handle_auth_bad_method(
     ceph::net::ConnectionRef conn,
     AuthConnectionMetaRef auth_meta,

diff --git a/src/crimson/auth/DummyAuth.h b/src/crimson/auth/DummyAuth.h
index b3b2dc62089e6dad06c812206e2c96319ce8b993..1f51b1aa38c6bbda15dd2595548de9adcdd22548 100644 (file)
--- a/src/crimson/auth/DummyAuth.h
+++ b/src/crimson/auth/DummyAuth.h
@@ -31,22 +31,16 @@ public:
     return nullptr;
   }
 
-  int get_auth_request(
+  std::tuple<uint32_t, std::vector<uint32_t>, bufferlist> get_auth_request(
     ceph::net::ConnectionRef conn,
-    AuthConnectionMetaRef auth_meta,
-    uint32_t *method,
-    std::vector<uint32_t> *preferred_modes,
-    bufferlist *out) override {
-    *method = CEPH_AUTH_NONE;
-    *preferred_modes = { CEPH_CON_MODE_CRC };
-    return 0;
+    AuthConnectionMetaRef auth_meta) override {
+    return {CEPH_AUTH_NONE, {CEPH_CON_MODE_CRC}, {}};
   }
 
-  int handle_auth_reply_more(
+  ceph::bufferlist handle_auth_reply_more(
     ceph::net::ConnectionRef conn,
     AuthConnectionMetaRef auth_meta,
-    const bufferlist& bl,
-    bufferlist *reply) override {
+    const bufferlist& bl) override {
     ceph_abort();
   }
 
@@ -55,9 +49,7 @@ public:
     AuthConnectionMetaRef auth_meta,
     uint64_t global_id,
     uint32_t con_mode,
-    const bufferlist& bl,
-    CryptoKey *session_key,
-    std::string *connection_secret) {
+    const bufferlist& bl) override {
     return 0;
   }
 

diff --git a/src/crimson/net/ProtocolV2.cc b/src/crimson/net/ProtocolV2.cc
index 5d85349883c8bc0ab90ef4e71dfd05b00989a455..c76fb0c8dedf056780523247810ac3d1a899f514 100644 (file)
--- a/src/crimson/net/ProtocolV2.cc
+++ b/src/crimson/net/ProtocolV2.cc
@@ -481,14 +481,9 @@ seastar::future<> ProtocolV2::handle_auth_reply()
           logger().debug("{} auth reply more len={}",
                          conn, auth_more.auth_payload().length());
           ceph_assert(messenger.get_auth_client());
-          ceph::bufferlist reply;
-          int r = messenger.get_auth_client()->handle_auth_reply_more(
-               conn.shared_from_this(), auth_meta, auth_more.auth_payload(), &reply);
-          if (r < 0) {
-            logger().error("{} auth_client handle_auth_reply_more returned {}",
-                           conn, r);
-            abort_in_fault();
-          }
+          // let execute_connecting() take care of the thrown exception
+          auto reply = messenger.get_auth_client()->handle_auth_reply_more(
+            conn.shared_from_this(), auth_meta, auth_more.auth_payload());
           auto more_reply = AuthRequestMoreFrame::Encode(reply);
           return write_frame(more_reply);
         }).then([this] {
@@ -503,9 +498,7 @@ seastar::future<> ProtocolV2::handle_auth_reply()
               conn.shared_from_this(), auth_meta,
               auth_done.global_id(),
               auth_done.con_mode(),
-              auth_done.auth_payload(),
-              &auth_meta->session_key,
-              &auth_meta->connection_secret);
+              auth_done.auth_payload());
           if (r < 0) {
             logger().error("{} auth_client handle_auth_done returned {}", conn, r);
             abort_in_fault();
@@ -528,21 +521,20 @@ seastar::future<> ProtocolV2::client_auth(std::vector<uint32_t> &allowed_methods
   // send_auth_request() logic
   ceph_assert(messenger.get_auth_client());
 
-  bufferlist bl;
-  vector<uint32_t> preferred_modes;
-  int r = messenger.get_auth_client()->get_auth_request(
-      conn.shared_from_this(), auth_meta, &auth_meta->auth_method,
-      &preferred_modes, &bl);
-  if (r < 0) {
-    logger().error("{} get_initial_auth_request returned {}", conn, r);
+  try {
+    auto [auth_method, preferred_modes, bl] =
+      messenger.get_auth_client()->get_auth_request(conn.shared_from_this(), auth_meta);
+    auth_meta->auth_method = auth_method;
+    auto frame = AuthRequestFrame::Encode(auth_method, preferred_modes, bl);
+    return write_frame(frame).then([this] {
+      return handle_auth_reply();
+    });
+  } catch (const ceph::auth::error& e) {
+    logger().error("{} get_initial_auth_request returned {}", conn, e);
     dispatch_reset();
     abort_in_close();
+    return seastar::now();
   }
-
-  auto frame = AuthRequestFrame::Encode(auth_meta->auth_method, preferred_modes, bl);
-  return write_frame(frame).then([this] {
-    return handle_auth_reply();
-  });
 }
 
 seastar::future<bool> ProtocolV2::process_wait()