Merge remote-tracking branch 'gh/wip-5648-c'

Reviewed-by: Sage Weil <sage@inktank.com>

10 files changed, 533 insertions, 145 deletions

diff --git a/qa/workunits/mon/caps.py b/qa/workunits/mon/caps.py
new file mode 100644
index 00000000000..0a0828d053b
--- /dev/null
+++ b/qa/workunits/mon/caps.py
@@ -0,0 +1,370 @@
+#!/usr/bin/python
+
+import json
+import subprocess
+import shlex
+from StringIO import StringIO
+import errno
+import sys
+import os
+import io
+import re
+
+
+import rados
+from ceph_argparse import *
+
+keyring_base = '/tmp/cephtest-caps.keyring'
+
+class UnexpectedReturn(Exception):
+  def __init__(self, cmd, ret, expected, msg):
+    if isinstance(cmd, list):
+      self.cmd = ' '.join(cmd)
+    else:
+      assert isinstance(cmd, str) or isinstance(cmd, unicode), \
+          'cmd needs to be either a list or a str'
+      self.cmd = cmd
+    self.cmd = str(self.cmd)
+    self.ret = int(ret)
+    self.expected = int(expected)
+    self.msg = str(msg)
+
+  def __str__(self):
+    return repr('{c}: expected return {e}, got {r} ({o})'.format(
+        c=self.cmd, e=self.expected, r=self.ret, o=self.msg))
+
+def call(cmd):
+  if isinstance(cmd, list):
+    args = cmd
+  elif isinstance(cmd, str) or isinstance(cmd, unicode):
+    args = shlex.split(cmd)
+  else:
+    assert False, 'cmd is not a string/unicode nor a list!'
+
+  print 'call: {0}'.format(args)
+  proc = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+  ret = proc.wait()
+
+  return (ret, proc)
+
+def expect(cmd, expected_ret):
+
+  try:
+    (r, p) = call(cmd)
+  except ValueError as e:
+    print >> sys.stderr, \
+             'unable to run {c}: {err}'.format(c=repr(cmd), err=e.message)
+    return errno.EINVAL
+
+  assert r == p.returncode, \
+      'wth? r was supposed to match returncode!'
+
+  if r != expected_ret:
+    raise UnexpectedReturn(repr(cmd), r, expected_ret, str(p.stderr.read()))
+
+  return p
+
+def expect_to_file(cmd, expected_ret, out_file, mode='a'):
+
+  # Let the exception be propagated to the caller
+  p = expect(cmd, expected_ret)
+  assert p.returncode == expected_ret, \
+      'expected result doesn\'t match and no exception was thrown!'
+
+  with io.open(out_file, mode) as file:
+    file.write(unicode(p.stdout.read()))
+
+  return p
+
+class Command:
+  def __init__(self, cid, j):
+    self.cid = cid[3:]
+    self.perms = j['perm']
+    self.module = j['module']
+
+    self.sig = ''
+    self.args = []
+    for s in j['sig']:
+      if not isinstance(s, dict):
+        assert isinstance(s, str) or isinstance(s,unicode), \
+            'malformatted signature cid {0}: {1}\n{2}'.format(cid,s,j)
+        if len(self.sig) > 0:
+          self.sig += ' '
+        self.sig += s
+      else:
+        self.args.append(s)
+
+  def __str__(self):
+    return repr('command {0}: {1} (requires \'{2}\')'.format(self.cid,\
+          self.sig, self.perms))
+
+
+def destroy_keyring(path):
+  if not os.path.exists(path):
+    raise Exception('oops! cannot remove inexistent keyring {0}'.format(path))
+
+  # grab all client entities from the keyring
+  entities = [m.group(1) for m in [re.match(r'\[client\.(.*)\]', l)
+                for l in [str(line.strip())
+                  for line in io.open(path,'r')]] if m is not None]
+
+  # clean up and make sure each entity is gone
+  for e in entities:
+    expect('ceph auth del client.{0}'.format(e), 0)
+    expect('ceph auth get client.{0}'.format(e), errno.ENOENT)
+
+  # remove keyring
+  os.unlink(path)
+
+  return True
+
+def test_basic_auth():
+  # make sure we can successfully add/del entities, change their caps
+  # and import/export keyrings.
+
+  expect('ceph auth add client.basicauth', 0)
+  expect('ceph auth caps client.basicauth mon \'allow *\'', 0)
+  # entity exists and caps do not match
+  expect('ceph auth add client.basicauth', errno.EINVAL)
+  # this command attempts to change an existing state and will fail
+  expect('ceph auth add client.basicauth mon \'allow w\'', errno.EINVAL)
+  expect('ceph auth get-or-create client.basicauth', 0)
+  expect('ceph auth get-key client.basicauth', 0)
+  expect('ceph auth get-or-create client.basicauth2', 0)
+  # cleanup
+  expect('ceph auth del client.basicauth', 0)
+  expect('ceph auth del client.basicauth2', 0)
+
+  return True
+
+def gen_module_keyring(module):
+  module_caps = [
+      ('all', '{t} \'allow service {s} rwx\'', 0),
+      ('none', '', errno.EACCES),
+      ('wrong', '{t} \'allow service foobar rwx\'', errno.EACCES),
+      ('right', '{t} \'allow service {s} {p}\'', 0),
+      ('no-execute', '{t} \'allow service {s} x\'', errno.EACCES)
+      ]
+
+  keyring = '{0}.service-{1}'.format(keyring_base,module)
+  for perms in 'r rw x'.split():
+    for (n,p,r) in module_caps:
+      c = p.format(t='mon', s=module, p=perms)
+      expect_to_file(
+          'ceph auth get-or-create client.{cn}-{cp} {caps}'.format(
+          cn=n,cp=perms,caps=c), 0, keyring)
+
+  return keyring
+
+
+def test_all():
+
+
+  perms = {
+      'good': {
+        'broad':[
+          ('rwx', 'allow *'),
+          ('r', 'allow r'),
+          ('rw', 'allow rw'),
+          ('x', 'allow x'),
+          ],
+        'service':[
+          ('rwx', 'allow service {s} rwx'),
+          ('r', 'allow service {s} r'),
+          ('rw', 'allow service {s} rw'),
+          ('x', 'allow service {s} x'),
+          ],
+        'command':[
+          ('rwx', 'allow command "{c}"'),
+          ],
+        'command-with':[
+          ('rwx', 'allow command "{c}" with {kv}')
+          ],
+        'command-with-prefix':[
+          ('rwx', 'allow command "{c}" with {key} prefix {val}')
+          ]
+        },
+      'bad': {
+        'broad':[
+          ('none', ''),
+          ],
+        'service':[
+          ('none1', 'allow service foo rwx'),
+          ('none2', 'allow service foo r'),
+          ('none3', 'allow service foo rw'),
+          ('none4', 'allow service foo x'),
+          ],
+        'command':[
+          ('none', 'allow command foo'),
+          ],
+        'command-with':[
+          ('none', 'allow command "{c}" with foo=bar'),
+          ],
+        'command-with-prefix':[
+          ('none', 'allow command "{c}" with foo prefix bar'),
+          ],
+        }
+      }
+
+  cmds = {
+      '':[
+        {
+          'cmd':('status', '', 'r')
+          },
+        {
+          'pre':'heap start_profiler',
+          'cmd':('heap', 'heapcmd=stats', 'rw'),
+          'post':'heap stop_profiler'
+          }
+        ],
+      'auth':[
+        {
+          'pre':'',
+          'cmd':('auth list', '', 'r'),
+          'post':''
+          },
+        {
+          'pre':'auth get-or-create client.foo mon \'allow *\'',
+          'cmd':('auth caps', 'entity="client.foo"', 'rw'),
+          'post':'auth del client.foo'
+          }
+        ],
+      'pg':[
+        {
+          'cmd':('pg getmap', '', 'r'),
+          },
+        {
+          'cmd':('pg send_pg_creates', '', 'rw'),
+          },
+        ],
+      'mds':[
+        {
+          'cmd':('mds getmap', '', 'r'),
+          },
+        {
+          'cmd':('mds cluster_down', '', 'rw'),
+          'post':'mds cluster_up'
+          },
+        ],
+      'mon':[
+        {
+          'cmd':('mon getmap', '', 'r')
+          },
+        {
+          'cmd':('mon remove', 'name=a', 'rw')
+          }
+        ],
+      'osd':[
+        {
+          'cmd':('osd getmap', '', 'r'),
+          },
+        {
+          'cmd':('osd pause', '', 'rw'),
+          'post':'osd unpause'
+          },
+        {
+          'cmd':('osd crush dump', '', 'r')
+          },
+        ],
+      'config-key':[
+          {
+            'pre':'config-key put foo bar',
+            'cmd':('config-key get', 'key=foo', 'r')
+            },
+          {
+            'pre':'config-key put foo bar',
+            'cmd':('config-key del', 'key=foo', 'rw')
+            }
+          ]
+      }
+
+  for (module,cmd_lst) in cmds.iteritems():
+    k = keyring_base + '.' + module
+    for cmd in cmd_lst:
+
+      (cmd_cmd, cmd_args, cmd_perm) = cmd['cmd']
+      cmd_args_key = ''
+      cmd_args_val = ''
+      if len(cmd_args) > 0:
+        (cmd_args_key, cmd_args_val) = cmd_args.split('=')
+
+      print 'generating keyring for {m}/{c}'.format(m=module,c=cmd_cmd)
+      # gen keyring
+      for (good_or_bad,kind_map) in perms.iteritems():
+        for (kind,lst) in kind_map.iteritems():
+          for (perm, cap) in lst:
+            cap_formatted = cap.format(
+                s=module,
+                c=cmd_cmd,
+                kv=cmd_args,
+                key=cmd_args_key,
+                val=cmd_args_val)
+
+            if len(cap_formatted) == 0:
+              run_cap = ''
+            else:
+              run_cap = 'mon \'{fc}\''.format(fc=cap_formatted)
+
+            cname = 'client.{gb}-{kind}-{p}'.format(
+                gb=good_or_bad,kind=kind,p=perm)
+            expect_to_file(
+                'ceph auth get-or-create {n} {c}'.format(
+                  n=cname,c=run_cap), 0, k)
+      # keyring generated
+      print 'testing {m}/{c}'.format(m=module,c=cmd_cmd)
+
+      # test
+      for good_bad in perms.iterkeys():
+        for (kind,lst) in perms[good_bad].iteritems():
+          for (perm,_) in lst:
+            cname = 'client.{gb}-{k}-{p}'.format(gb=good_bad,k=kind,p=perm)
+
+          if good_bad == 'good':
+            expect_ret = 0
+          else:
+            expect_ret = errno.EACCES
+
+          if ( cmd_perm not in perm ):
+            expect_ret = errno.EACCES
+          if 'with' in kind and len(cmd_args) == 0:
+            expect_ret = errno.EACCES
+          if 'service' in kind and len(module) == 0:
+            expect_ret = errno.EACCES
+
+          if 'pre' in cmd and len(cmd['pre']) > 0:
+            expect('ceph {0}'.format(cmd['pre']), 0)
+          expect('ceph -n {cn} -k {k} {c} {arg_val}'.format(
+            cn=cname,k=k,c=cmd_cmd,arg_val=cmd_args_val), expect_ret)
+          if 'post' in cmd and len(cmd['post']) > 0:
+            expect('ceph {0}'.format(cmd['post']), 0)
+      # finish testing
+      destroy_keyring(k)
+
+
+  return True
+
+
+def test_misc():
+
+  k = keyring_base + '.misc'
+  expect_to_file(
+      'ceph auth get-or-create client.caps mon \'allow command "auth caps"' \
+          ' with entity="client.caps"\'', 0, k)
+  expect('ceph -n client.caps -k {kf} mon_status'.format(kf=k), errno.EACCES)
+  expect('ceph -n client.caps -k {kf} auth caps client.caps mon \'allow *\''.format(kf=k), 0)
+  expect('ceph -n client.caps -k {kf} mon_status'.format(kf=k), 0)
+  destroy_keyring(k)
+
+def main():
+
+  test_basic_auth()
+  test_all()
+  test_misc()
+
+  print 'OK'
+
+  return 0
+
+if __name__ == '__main__':
+  main()
+
diff --git a/src/mon/AuthMonitor.cc b/src/mon/AuthMonitor.cc
index f165b8c9fc7..4fb943e7939 100644
--- a/src/mon/AuthMonitor.cc
+++ b/src/mon/AuthMonitor.cc
@@ -546,8 +546,7 @@ bool AuthMonitor::preprocess_command(MMonCommand *m)
   }
 
   MonSession *session = m->get_session();
-  if (!session ||
-      (!mon->_allowed_command(session, cmdmap))) {
+  if (!session) {
     mon->reply_command(m, -EACCES, "access denied", rdata, get_last_committed());
     return true;
   }
@@ -695,8 +694,7 @@ bool AuthMonitor::prepare_command(MMonCommand *m)
   boost::scoped_ptr<Formatter> f(new_formatter(format));
 
   MonSession *session = m->get_session();
-  if (!session ||
-      (!mon->_allowed_command(session, cmdmap))) {
+  if (!session) {
     mon->reply_command(m, -EACCES, "access denied", rdata, get_last_committed());
     return true;
   }
@@ -739,37 +737,119 @@ bool AuthMonitor::prepare_command(MMonCommand *m)
     wait_for_finished_proposal(new Monitor::C_Command(mon, m, 0, rs, get_last_committed()));
     //paxos->wait_for_commit(new Monitor::C_Command(mon, m, 0, rs, get_last_committed()));
     return true;
-  } else if (prefix == "auth add") {
+  } else if (prefix == "auth add" && !entity_name.empty()) {
+    /* expected behavior:
+     *  - if command reproduces current state, return 0.
+     *  - if command adds brand new entity, handle it.
+     *  - if command adds new state to existing entity, return error.
+     */
     KeyServerData::Incremental auth_inc;
     auth_inc.name = entity;
     bufferlist bl = m->get_data();
-    dout(10) << "AuthMonitor::prepare_command bl.length()=" << bl.length() << dendl;
-    if (bl.length()) {
+    bool has_keyring = (bl.length() > 0);
+    map<string,bufferlist> new_caps;
+
+    KeyRing new_keyring;
+    if (has_keyring) {
       bufferlist::iterator iter = bl.begin();
-      KeyRing keyring;
       try {
-	::decode(keyring, iter);
+        ::decode(new_keyring, iter);
       } catch (const buffer::error &ex) {
-	ss << "error decoding keyring";
+        ss << "error decoding keyring";
+        err = -EINVAL;
+        goto done;
+      }
+    }
+
+    // are we about to have it?
+    for (vector<Incremental>::iterator p = pending_auth.begin();
+        p != pending_auth.end();
+        ++p) {
+      if (p->inc_type == AUTH_DATA) {
+        KeyServerData::Incremental inc;
+        bufferlist::iterator q = p->auth_data.begin();
+        ::decode(inc, q);
+        if (inc.op == KeyServerData::AUTH_INC_ADD &&
+            inc.name == entity) {
+          wait_for_finished_proposal(
+              new Monitor::C_Command(mon, m, 0, rs, get_last_committed()));
+          return true;
+        }
+      }
+    }
+
+    // build new caps from provided arguments (if available)
+    for (vector<string>::iterator it = caps_vec.begin();
+	 it != caps_vec.end() && (it + 1) != caps_vec.end();
+	 it += 2) {
+      string sys = *it;
+      bufferlist cap;
+      ::encode(*(it+1), cap);
+      new_caps[sys] = cap;
+    }
+
+    // pull info out of provided keyring
+    EntityAuth new_inc;
+    if (has_keyring) {
+      if (!new_keyring.get_auth(auth_inc.name, new_inc)) {
+	ss << "key for " << auth_inc.name
+	   << " not found in provided keyring";
 	err = -EINVAL;
 	goto done;
       }
-      if (!keyring.get_auth(auth_inc.name, auth_inc.auth)) {
-	ss << "key for " << auth_inc.name << " not found in provided keyring";
+      if (!new_caps.empty() && !new_inc.caps.empty()) {
+	ss << "caps cannot be specified both in keyring and in command";
 	err = -EINVAL;
 	goto done;
       }
-    } else {
-      // generate a new random key
-      dout(10) << "AuthMonitor::prepare_command generating random key for " << auth_inc.name << dendl;
-      auth_inc.auth.key.create(g_ceph_context, CEPH_CRYPTO_AES);
+      if (new_caps.empty()) {
+	new_caps = new_inc.caps;
+      }
     }
 
-    auth_inc.op = KeyServerData::AUTH_INC_ADD;
+    // does entry already exist?
+    if (mon->key_server.get_auth(auth_inc.name, auth_inc.auth)) {
+      // key match?
+      if (has_keyring) {
+        if (auth_inc.auth.key.get_secret().cmp(new_inc.key.get_secret())) {
+	  ss << "entity " << auth_inc.name << " exists but key does not match";
+	  err = -EINVAL;
+	  goto done;
+	}
+      }
 
-    for (vector<string>::iterator it = caps_vec.begin();
-	 it != caps_vec.end(); it += 2)
-      ::encode(*(it+1), auth_inc.auth.caps[*it]);
+      // caps match?
+      if (new_caps.size() != auth_inc.auth.caps.size()) {
+	ss << "entity " << auth_inc.name << " exists but caps do not match";
+	err = -EINVAL;
+	goto done;
+      }
+      for (map<string,bufferlist>::iterator it = new_caps.begin();
+	   it != new_caps.end(); ++it) {
+        if (auth_inc.auth.caps.count(it->first) == 0 ||
+            !auth_inc.auth.caps[it->first].contents_equal(it->second)) {
+          ss << "entity " << auth_inc.name << " exists but cap "
+	     << it->first << " does not match";
+          err = -EINVAL;
+          goto done;
+        }
+      }
+
+      // they match, no-op
+      err = 0;
+      goto done;
+    }
+
+    // okay, add it.
+    auth_inc.op = KeyServerData::AUTH_INC_ADD;
+    auth_inc.auth.caps = new_caps;
+    if (has_keyring) {
+      auth_inc.auth.key = new_inc.key;
+    } else {
+      dout(10) << "AuthMonitor::prepare_command generating random key for "
+               << auth_inc.name << dendl;
+      auth_inc.auth.key.create(g_ceph_context, CEPH_CRYPTO_AES);
+    }
 
     dout(10) << " importing " << auth_inc.name << dendl;
     dout(30) << "    " << auth_inc.auth << dendl;
diff --git a/src/mon/LogMonitor.cc b/src/mon/LogMonitor.cc
index cab49060082..47f56bebee4 100644
--- a/src/mon/LogMonitor.cc
+++ b/src/mon/LogMonitor.cc
@@ -362,9 +362,7 @@ bool LogMonitor::prepare_command(MMonCommand *m)
   cmd_getval(g_ceph_context, cmdmap, "prefix", prefix);
 
   MonSession *session = m->get_session();
-  if (!session ||
-      (!session->is_capable("log", MON_CAP_W) &&
-       !mon->_allowed_command(session, cmdmap))) {
+  if (!session) {
     mon->reply_command(m, -EACCES, "access denied", get_last_committed());
     return true;
   }
diff --git a/src/mon/MDSMonitor.cc b/src/mon/MDSMonitor.cc
index d89cc412912..9988d8c8402 100644
--- a/src/mon/MDSMonitor.cc
+++ b/src/mon/MDSMonitor.cc
@@ -554,9 +554,7 @@ bool MDSMonitor::preprocess_command(MMonCommand *m)
   boost::scoped_ptr<Formatter> f(new_formatter(format));
 
   MonSession *session = m->get_session();
-  if (!session ||
-      (!session->is_capable("mds", MON_CAP_R) &&
-       !mon->_allowed_command(session, cmdmap))) {
+  if (!session) {
     mon->reply_command(m, -EACCES, "access denied", rdata, get_last_committed());
     return true;
   }
@@ -768,9 +766,7 @@ bool MDSMonitor::prepare_command(MMonCommand *m)
   cmd_getval(g_ceph_context, cmdmap, "prefix", prefix);
 
   MonSession *session = m->get_session();
-  if (!session ||
-      (!session->is_capable("mds", MON_CAP_W) &&
-       !mon->_allowed_command(session, cmdmap))) {
+  if (!session) {
     mon->reply_command(m, -EACCES, "access denied", rdata, get_last_committed());
     return true;
   }
diff --git a/src/mon/Monitor.cc b/src/mon/Monitor.cc
index a9d3e48a3be..47488fc4f8b 100644
--- a/src/mon/Monitor.cc
+++ b/src/mon/Monitor.cc
@@ -1535,47 +1535,6 @@ void Monitor::finish_election()
 }
 
 
-bool Monitor::_allowed_command(MonSession *s, map<string, cmd_vartype>& cmd)
-{
-  bool retval = false;
-
-  if (s->caps.is_allow_all()) {
-    dout(10) << __func__ << " allow_all" << dendl;
-    return true;
-  }
-
-  string prefix;
-  cmd_getval(g_ceph_context, cmd, "prefix", prefix);
-
-  map<string,string> strmap;
-  for (map<string, cmd_vartype>::const_iterator p = cmd.begin();
-       p != cmd.end(); ++p) {
-    if (p->first == "prefix")
-      continue;
-    if (p->first == "caps") {
-      vector<string> cv;
-      if (cmd_getval(g_ceph_context, cmd, "caps", cv) &&
-	  cv.size() % 2 == 0) {
-	for (unsigned i = 0; i < cv.size(); i += 2) {
-	  string k = string("caps_") + cv[i];
-	  strmap[k] = cv[i + 1];
-	}
-	continue;
-      }
-    }
-    strmap[p->first] = cmd_vartype_stringify(p->second);
-  }
-
-  dout(20) << __func__ << " strmap " << strmap << dendl;
-  if (s->caps.is_capable(g_ceph_context, s->inst.name,
-			 "", prefix, strmap, false, false, true)) {
-    retval = true; 
-  }
-
-  dout(10) << __func__ << " = " << retval << dendl;
-  return retval;
-}
-
 void Monitor::sync_force(Formatter *f, ostream& ss)
 {
   bool free_formatter = false;
@@ -1893,6 +1852,51 @@ struct MonCommand {
 #include <mon/MonCommands.h>
 };
 
+bool Monitor::_allowed_command(MonSession *s, string &module, string &prefix,
+                               map<string,cmd_vartype>& cmdmap) {
+
+  map<string,string> strmap;
+  for (map<string,cmd_vartype>::const_iterator p = cmdmap.begin();
+       p != cmdmap.end(); ++p) {
+    if (p->first == "prefix")
+      continue;
+    if (p->first == "caps") {
+      vector<string> cv;
+      if (cmd_getval(g_ceph_context, cmdmap, "caps", cv) &&
+	  cv.size() % 2 == 0) {
+	for (unsigned i = 0; i < cv.size(); i += 2) {
+	  string k = string("caps_") + cv[i];
+	  strmap[k] = cv[i + 1];
+	}
+	continue;
+      }
+    }
+    strmap[p->first] = cmd_vartype_stringify(p->second);
+  }
+
+  MonCommand *this_cmd = NULL;
+  for (MonCommand *cp = mon_commands;
+       cp < &mon_commands[ARRAY_SIZE(mon_commands)]; cp++) {
+    dout(0) << __func__ << " CAPSBAR >> matching against " << cp->cmdstring << dendl;
+    if (cp->cmdstring.find(prefix) != string::npos) {
+      this_cmd = cp;
+      break;
+    }
+  }
+  assert(this_cmd != NULL);
+  bool cmd_r = (this_cmd->req_perms.find('r') != string::npos);
+  bool cmd_w = (this_cmd->req_perms.find('w') != string::npos);
+  bool cmd_x = (this_cmd->req_perms.find('x') != string::npos);
+
+  bool capable = s->caps.is_capable(g_ceph_context, s->inst.name,
+                                    module, prefix, strmap,
+                                    cmd_r, cmd_w, cmd_x);
+
+  dout(10) << __func__ << " " << (capable ? "" : "not ") << "capable" << dendl;
+  return capable;
+}
+
+
 void Monitor::handle_command(MMonCommand *m)
 {
   if (m->fsid != monmap->fsid) {
@@ -1958,10 +1962,6 @@ void Monitor::handle_command(MMonCommand *m)
     return;
   }
 
-  bool access_cmd;
-  bool access_r;
-  bool access_all;
-
   string module;
   string err;
 
@@ -1974,9 +1974,11 @@ void Monitor::handle_command(MMonCommand *m)
   get_str_vec(prefix, fullcmd);
   module = fullcmd[0];
 
-  access_cmd = _allowed_command(session, cmdmap);
-  access_r = (session->is_capable("mon", MON_CAP_R) || access_cmd);
-  access_all = (session->caps.is_allow_all() || access_cmd);
+  if (!_allowed_command(session, module, prefix, cmdmap)) {
+    dout(1) << __func__ << " access denied" << dendl;
+    reply_command(m, -EACCES, "access denied", 0);
+    return;
+  }
 
   if (module == "mds") {
     mdsmon()->dispatch(m);
@@ -2005,11 +2007,6 @@ void Monitor::handle_command(MMonCommand *m)
   }
 
   if (module == "config-key") {
-    if (!access_all) {
-      r = -EACCES;
-      rs = "access denied";
-      goto out;
-    }
     config_key_service->dispatch(m);
     return;
   }
@@ -2041,11 +2038,6 @@ void Monitor::handle_command(MMonCommand *m)
   }
 
   if (prefix == "compact") {
-    if (!access_all) {
-      r = -EACCES;
-      rs = "access denied";
-      goto out;
-    }
     dout(1) << "triggering manual compaction" << dendl;
     utime_t start = ceph_clock_now(g_ceph_context);
     store->compact();
@@ -2058,11 +2050,6 @@ void Monitor::handle_command(MMonCommand *m)
     r = 0;
   }
   else if (prefix == "injectargs") {
-    if (!access_all) {
-      r = -EACCES;
-      rs = "access denied";
-      goto out;
-    }
     vector<string> injected_args;
     cmd_getval(g_ceph_context, cmdmap, "injected_args", injected_args);
     if (!injected_args.empty()) {
@@ -2082,12 +2069,6 @@ void Monitor::handle_command(MMonCommand *m)
   } else if (prefix == "status" ||
 	     prefix == "health" ||
 	     prefix == "df") {
-    if (!access_r) {
-      r = -EACCES;
-      rs = "access denied";
-      goto out;
-    }
-
     string detail;
     cmd_getval(g_ceph_context, cmdmap, "detail", detail);
 
@@ -2138,11 +2119,6 @@ void Monitor::handle_command(MMonCommand *m)
     rs = "";
     r = 0;
   } else if (prefix == "report") {
-    if (!access_r) {
-      r = -EACCES;
-      rs = "access denied";
-      goto out;
-    }
 
     // this must be formatted, in its current form
     if (!f)
@@ -2181,11 +2157,6 @@ void Monitor::handle_command(MMonCommand *m)
     rs = ss2.str();
     r = 0;
   } else if (prefix == "quorum_status") {
-    if (!access_r) {
-      r = -EACCES;
-      rs = "access denied";
-      goto out;
-    }
     // make sure our map is readable and up to date
     if (!is_leader() && !is_peon()) {
       dout(10) << " waiting for quorum" << dendl;
@@ -2197,11 +2168,6 @@ void Monitor::handle_command(MMonCommand *m)
     rs = "";
     r = 0;
   } else if (prefix == "mon_status") {
-    if (!access_r) {
-      r = -EACCES;
-      rs = "access denied";
-      goto out;
-    }
     _mon_status(f.get(), ds);
     rdata.append(ds);
     rs = "";
@@ -2222,11 +2188,6 @@ void Monitor::handle_command(MMonCommand *m)
     rs = ds.str();
     r = 0;
   } else if (prefix == "heap") {
-    if (!access_all) {
-      r = -EACCES;
-      rs = "access denied";
-      goto out;
-    }
     if (!ceph_using_tcmalloc())
       rs = "tcmalloc not enabled, can't use heap profiler commands\n";
     else {
@@ -2241,11 +2202,6 @@ void Monitor::handle_command(MMonCommand *m)
       r = 0;
     }
   } else if (prefix == "quorum") {
-    if (!access_all) {
-      r = -EACCES;
-      rs = "access denied";
-      goto out;
-    }
     string quorumcmd;
     cmd_getval(g_ceph_context, cmdmap, "quorumcmd", quorumcmd);
     if (quorumcmd == "exit") {
@@ -2259,9 +2215,6 @@ void Monitor::handle_command(MMonCommand *m)
       rs = "started responding to quorum, initiated new election";
       r = 0;
     }
-  } else if (!access_cmd) {
-    r = -EACCES;
-    rs = "access denied";
   }
 
  out:
diff --git a/src/mon/Monitor.h b/src/mon/Monitor.h
index cb1f4138a25..e39ffc20378 100644
--- a/src/mon/Monitor.h
+++ b/src/mon/Monitor.h
@@ -583,7 +583,8 @@ public:
   void handle_get_version(MMonGetVersion *m);
   void handle_subscribe(MMonSubscribe *m);
   void handle_mon_get_map(MMonGetMap *m);
-  bool _allowed_command(MonSession *s, map<std::string, cmd_vartype>& cmd);
+  bool _allowed_command(MonSession *s, string &module, string& prefix,
+                        map<string,cmd_vartype>& cmdmap);
   void _mon_status(Formatter *f, ostream& ss);
   void _quorum_status(Formatter *f, ostream& ss);
   void _add_bootstrap_peer_hint(string cmd, cmdmap_t& cmdmap, ostream& ss);
diff --git a/src/mon/MonmapMonitor.cc b/src/mon/MonmapMonitor.cc
index 5ec1583b82f..799f19df154 100644
--- a/src/mon/MonmapMonitor.cc
+++ b/src/mon/MonmapMonitor.cc
@@ -164,9 +164,7 @@ bool MonmapMonitor::preprocess_command(MMonCommand *m)
   cmd_getval(g_ceph_context, cmdmap, "prefix", prefix);
 
   MonSession *session = m->get_session();
-  if (!session ||
-      (!session->is_capable("mon", MON_CAP_R) &&
-       !mon->_allowed_command(session, cmdmap))) {
+  if (!session) {
     mon->reply_command(m, -EACCES, "access denied", get_last_committed());
     return true;
   }
@@ -276,9 +274,7 @@ bool MonmapMonitor::prepare_command(MMonCommand *m)
   cmd_getval(g_ceph_context, cmdmap, "prefix", prefix);
 
   MonSession *session = m->get_session();
-  if (!session ||
-      (!session->is_capable("mon", MON_CAP_R) &&
-       !mon->_allowed_command(session, cmdmap))) {
+  if (!session) {
     mon->reply_command(m, -EACCES, "access denied", get_last_committed());
     return true;
   }
diff --git a/src/mon/OSDMonitor.cc b/src/mon/OSDMonitor.cc
index 07022aec73b..bad405fa6f3 100644
--- a/src/mon/OSDMonitor.cc
+++ b/src/mon/OSDMonitor.cc
@@ -1949,9 +1949,7 @@ bool OSDMonitor::preprocess_command(MMonCommand *m)
   }
 
   MonSession *session = m->get_session();
-  if (!session ||
-      (!session->is_capable("osd", MON_CAP_R) &&
-       !mon->_allowed_command(session, cmdmap))) {
+  if (!session) {
     mon->reply_command(m, -EACCES, "access denied", rdata, get_last_committed());
     return true;
   }
@@ -2594,9 +2592,7 @@ bool OSDMonitor::prepare_command(MMonCommand *m)
   boost::scoped_ptr<Formatter> f(new_formatter(format));
 
   MonSession *session = m->get_session();
-  if (!session ||
-      (!session->is_capable("osd", MON_CAP_W) &&
-       !mon->_allowed_command(session, cmdmap))) {
+  if (!session) {
     mon->reply_command(m, -EACCES, "access denied", get_last_committed());
     return true;
   }
diff --git a/src/mon/PGMonitor.cc b/src/mon/PGMonitor.cc
index 29c77dbe0ed..bb5f447a4e3 100644
--- a/src/mon/PGMonitor.cc
+++ b/src/mon/PGMonitor.cc
@@ -1323,9 +1323,7 @@ bool PGMonitor::preprocess_command(MMonCommand *m)
   cmd_getval(g_ceph_context, cmdmap, "prefix", prefix);
 
   MonSession *session = m->get_session();
-  if (!session ||
-      (!session->is_capable("pg", MON_CAP_R) &&
-       !mon->_allowed_command(session, cmdmap))) {
+  if (!session) {
     mon->reply_command(m, -EACCES, "access denied", rdata, get_last_committed());
     return true;
   }
@@ -1573,9 +1571,7 @@ bool PGMonitor::prepare_command(MMonCommand *m)
   cmd_getval(g_ceph_context, cmdmap, "prefix", prefix);
 
   MonSession *session = m->get_session();
-  if (!session ||
-      (!session->is_capable("pg", MON_CAP_W) &&
-       !mon->_allowed_command(session, cmdmap))) {
+  if (!session) {
     mon->reply_command(m, -EACCES, "access denied", get_last_committed());
     return true;
   }
diff --git a/src/test/mon/moncap.cc b/src/test/mon/moncap.cc
index 92d5750c9a9..19f82f55ecf 100644
--- a/src/test/mon/moncap.cc
+++ b/src/test/mon/moncap.cc
@@ -51,6 +51,8 @@ const char *parse_good[] = {
   "allow service foo-foo r, allow service bar r",
   "allow service \" foo \" w, allow service bar r",
   "allow command abc with arg=foo arg2=bar, allow service foo r",
+  "allow command \"foo bar\" with arg=\"baz\"",
+  "allow command \"foo bar\" with arg=\"baz.xx\"",
   0
 };