[Bug 1969000] Re: [SRU] bail from handle_command() if _generate_command_map() fails

Thu Oct 19 19:10:05 UTC 2023

Hello nikhil, or anyone else affected,

Accepted ceph into focal-proposed. The package will build now and be
available at
https://launchpad.net/ubuntu/+source/ceph/15.2.17-0ubuntu0.20.04.5 in a
few hours, and then in the -proposed repository.

Please help us by testing this new package.  See
https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how
to enable and use -proposed.  Your feedback will aid us getting this
update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug,
mentioning the version of the package you tested, what testing has been
performed on the package and change the tag from verification-needed-
focal to verification-done-focal. If it does not fix the bug for you,
please add a comment stating that, and change the tag to verification-
failed-focal. In either case, without details of your testing we will
not be able to proceed.

Further information regarding the verification process can be found at
https://wiki.ubuntu.com/QATeam/PerformingSRUVerification .  Thank you in
advance for helping!

N.B. The updated package will be released to -updates after the bug(s)
fixed by this package have been verified and the package has been in
-proposed for a minimum of 7 days.

** Changed in: ceph (Ubuntu Focal)
       Status: In Progress => Fix Committed

** Tags added: verification-needed verification-needed-focal

-- 
You received this bug notification because you are a member of Ubuntu
OpenStack, which is subscribed to Ubuntu Cloud Archive.
https://bugs.launchpad.net/bugs/1969000

Title:
  [SRU] bail from handle_command() if _generate_command_map() fails

Status in Ubuntu Cloud Archive:
  Invalid
Status in Ubuntu Cloud Archive ussuri series:
  In Progress
Status in ceph package in Ubuntu:
  Fix Released
Status in ceph source package in Focal:
  Fix Committed
Status in ceph source package in Impish:
  Won't Fix
Status in ceph source package in Jammy:
  Fix Released
Status in ceph source package in Kinetic:
  Fix Released
Status in ceph source package in Lunar:
  Fix Released
Status in ceph source package in Mantic:
  Fix Released

Bug description:
  [Impact]
  If improper json data is passed to rados using a manual curl command, or invalid json data through a script like the python eg. shown, it can end up crashing the mon. This is already fixed through https://github.com/ceph/ceph/pull/45891 and already in the Ubuntu octopus point release. This fix is a performance improvement that returns from the function immediately and does not continue executing code anymore in the handle_command() function, since we have caught the exception thrown by _generate_command_map() and dealt with it in Monitor::handle_command().

  [Test Plan]
  Setup a ceph octopus cluster. A manual run of curl with malformed request like this results in the exception being thrown.

  curl -k -H "Authorization: Basic $TOKEN"
  "https://juju-3b3d82-10-lxd-0:8003/request" -X POST -d
  '{"prefix":"auth add","entity":"client.testuser02","caps":"mon
  '\''allow r'\'' osd '\''allow rw pool=testpool01'\''"}'

  This reproduces without restful API too.

  This python script run on the mon node also will cause the exception
  to be thrown due to the particular json which is malformed,

  root at focal-testing:/home/nikhil/Downloads/ceph_upstream/ceph/build# cat test_ceph.sh 
  #!/usr/bin/env python3
  import json
  import rados
  c = rados.Rados(conffile='ceph.conf')
  c.connect()
  cmd = json.dumps({"prefix":"auth add","entity":"client.testuser02","caps":"mon '\''allow r'\'' osd '\''allow rw pool=testpool01'\''"})
  #cmd = json.dumps({"prefix":"auth add","entity":"client.testuser02","caps":["mon", "allow r", "osd", "allow rw pool=testpool01"]})
  print(c.mon_command(cmd, b''))
  root at focal-testing:/home/nikhil/Downloads/ceph_upstream/ceph/build# ./test_ceph.sh 
  (-22, b'', "bad or missing field 'caps'")

  Once this exception is caught correctly as above and the error message
  printed due to this code, and we bail out of the function due to this
  SRU, the following code
  https://github.com/ceph/ceph/blob/6a585618451421f0744745e4dd3636f10f678397/src/mon/Monitor.cc#L3349C1-L3358C6
  should never be then further executed because we return as soon as the
  exception is caught and handled.

  So therefore, setting debug level to 10 will validate that the message
  is never seen from _allowed_command(), i.e at the end of that
  function,

   dout(10) << __func__ << " " << (capable ? "" : "not ") << "capable"
  << dendl;

  Pasting the function code for reference,
  (https://github.com/ceph/ceph/blob/octopus/src/mon/Monitor.cc#L3061)

  bool Monitor::_allowed_command(MonSession *s, const string &module,
  			       const string &prefix, const cmdmap_t& cmdmap,
                                 const map<string,string>& param_str_map,
                                 const MonCommand *this_cmd) {

    bool cmd_r = this_cmd->requires_perm('r');
    bool cmd_w = this_cmd->requires_perm('w');
    bool cmd_x = this_cmd->requires_perm('x');

    bool capable = s->caps.is_capable(
      g_ceph_context,
      s->entity_name,
      module, prefix, param_str_map,
      cmd_r, cmd_w, cmd_x,
      s->get_peer_socket_addr());

    dout(10) << __func__ << " " << (capable ? "" : "not ") << "capable" << dendl;  
    return capable;
  }

  So it would be a reasonable to test the SRU and verify that at
  loglevel 10, we do not see the
  https://github.com/ceph/ceph/blob/octopus/src/mon/Monitor.cc#L3077
  debug message.

  [Where problems could occur]
  The only potential problem with this cleanup fix is if
  some additional code in the void Monitor::handle_command(MonOpRequestRef op) function is needed to run before exit()'ing out. I have looked for such potential conditions and not found any.

  [Other Info]
  While the fix to catch the exception is already part of the Octopus 15.2.17 point release, (PR https://github.com/ceph/ceph/pull/45891),
  we need this cleanup fix that has now been also merged to master upstream through https://github.com/ceph/ceph/pull/48044

  The cleanup fix bails out of the function if the exception is
  thrown, therefore avoiding continuing in the function
  void Monitor::handle_command(MonOpRequestRef op) in this
  error situation.

  Upstream tracker - https://tracker.ceph.com/issues/57859
  Fixed in ceph main through https://github.com/ceph/ceph/pull/48044

To manage notifications about this bug go to:
https://bugs.launchpad.net/cloud-archive/+bug/1969000/+subscriptions