rgw: mfa support #19283

yehudasa · 2017-12-01T15:26:10Z

otp object class implementation
rgw support for S3 mfa api
rgw otp metadata provider to allow sync of otp data across zones
sync token due to time skew
support hex and base32 seeds

cbodley

initial review of the cls bits

cbodley · 2017-12-01T15:46:20Z

src/cls/otp/cls_otp.cc

+  int result = oath_hex2bin(otp.seed.c_str(), secret, &slen);
+  if (result != OATH_OK) {
+    CLS_LOG(20, "failed to parse seed");
+    return result;


return false here - the conversion from int->bool would return true when result != OATH_OK

cbodley · 2017-12-01T15:55:44Z

src/cls/otp/cls_otp.cc

+  ceph::real_time now = real_clock::now();
+  trim_expired(now);
+
+  for (auto entry : boost::adaptors::reverse(last_checks)) {


prefer auto& in ranged-for loops to avoid copying each element

cbodley · 2017-12-01T15:59:31Z

src/cls/otp/cls_otp_ops.h

+
+struct cls_otp_set_otp_op
+{
+  list<rados::cls::otp::otp_info_t> entries;


is there any reason to choose list over vector for any of these?

@cbodley only iterating over it, never access entries via index. list.size() should be O(1) in c++11 (and it is in all relevant implementations).

i worry about the complexity of allocations though, where list is linear and vector is logarithmic

@cbodley it's pretty moot as the number of entries expected to be here are somewhere between zero and one, but I can change it to vector if you really insist.

cbodley · 2017-12-01T16:04:18Z

src/cls/otp/cls_otp.cc

+  bufferlist bl;
+  string key = otp_key_prefix + id;
+
+  int r = cls_cxx_map_get_val(hctx, key.c_str(), &bl);


the cls_cxx_ apis all take std::string - so these are converting to const char* and back again

cbodley · 2017-12-01T16:09:34Z

src/cls/otp/cls_otp_ops.h

+{
+  list<rados::cls::otp::otp_info_t> entries;
+
+  cls_otp_set_otp_op() = default;


the compiler will generate this for you "if no other constructor is explicitly declared" - https://en.wikipedia.org/wiki/Special_member_functions

cbodley · 2017-12-01T16:15:12Z

src/cls/otp/cls_otp.cc

+
+void otp_instance::trim_expired(const ceph::real_time& now)
+{
+  ceph::real_time window_start = now - make_timespan(otp.step_size);


make_timespan() takes a double, so this does an extra conversion. std::chrono::seconds(otp.step_size) may be more concise here?

cbodley · 2017-12-20T16:44:41Z

looking good 👍 do you have plans to add some test coverage?

yehudasa · 2017-12-22T21:06:10Z

@cbodley yes, although it's not clear to me which tool is the best one. Requires functional tests intermixed with admin stuff.

cbodley · 2018-01-02T15:20:17Z

CMakeLists.txt

@@ -368,6 +368,8 @@ if(WITH_RADOSGW)
  endif()
 endif(WITH_RADOSGW)

+find_package(liboath REQUIRED)


i'm guessing that we'll also need to express this dependency in install-deps.sh and the rpm/deb builds as well?

yehudasa · 2018-03-24T19:14:53Z

@cbodley can this be merged?
http://pulpito.ceph.com/yehudasa-2018-03-23_17:18:06-rgw-wip-rgw-mfa-distro-basic-smithi/

cbodley · 2018-03-26T15:45:57Z

src/rgw/rgw_admin.cc

+                               1,
+                               nullptr,
+                               pins[1].c_str());
+      if (rc != OATH_INVALID_OTP) {


from the oath reference manual on oath_totp_validate2:

Returns absolute value of position in OTP window (zero is first position), or OATH_INVALID_OTP if no OTP was found in OTP window, or an error code.

shouldn't this check for other errors codes as well?

and is there something we can do with the position it returns on success to calculate a more accurate offset?

cbodley · 2018-03-26T15:47:22Z

src/rgw/rgw_admin.cc

+
+    if (totp_pin.size() != 2) {
+      cerr << "ERROR: missing two --totp-pin params (--totp-pin=<first> --totp-pin=<second>)" << std::endl;
+    }


missing a return EINVAL? this would hit an assert in scan_totp()

cbodley · 2018-03-26T15:52:56Z

src/rgw/rgw_admin.cc

@@ -6203,7 +6295,7 @@ int main(int argc, const char **argv)
      cerr << "ERROR: failed to read input: " << cpp_strerror(-ret) << std::endl;
      return -ret;
    }
-    ret = store->meta_mgr->put(metadata_key, bl, RGWMetadataHandler::APPLY_ALWAYS);
+    ret = store->meta_mgr->put(metadata_key, bl, RGWMetadataHandler::RGWMetadataHandler::APPLY_ALWAYS);


^ this doesn't look right

cbodley · 2018-03-26T15:58:09Z

@yehudasa can you please add some user docs to show how to set this up?