From 13e8507b73b3d16431c29fe427b094bbdf804d96 Mon Sep 17 00:00:00 2001 From: Colin Patrick McCabe Date: Sun, 2 Jan 2011 12:19:35 -0800 Subject: [PATCH] auth: make g_conf.keyring a plain old string Make g_conf.keyring a plain old string rather than an array of strings. Don't do substitution using the user's HOME variable-- this could lead to security holes for setuid processes. Get rid of AuthMonitor::read_keyfile because there is already a Keyring member function, Keyring::load, that does the same thing. qa/rbd/common.sh: we can now use cconf to figure out what the keyring is. Signed-off-by: Colin McCabe --- qa/rbd/common.sh | 2 +- src/auth/KeyRing.cc | 46 +++++++++--------------- src/auth/KeyRing.h | 2 +- src/common/common_init.cc | 75 ++++++++++++++++++++++----------------- src/config.cc | 2 +- src/mon/AuthMonitor.cc | 36 ++----------------- src/mon/AuthMonitor.h | 1 - 7 files changed, 65 insertions(+), 99 deletions(-) diff --git a/qa/rbd/common.sh b/qa/rbd/common.sh index aa6efe7de83e2..b070e7f1a2fec 100644 --- a/qa/rbd/common.sh +++ b/qa/rbd/common.sh @@ -33,7 +33,7 @@ set_variables() { [ -z "$imgsize" ] && imgsize=1024 [ -z "$user" ] && user=admin - #[ -z "$keyring" ] && keyring="`$CCONF keyring`" + [ -z "$keyring" ] && keyring="`$CCONF keyring`" [ -z "$secret" ] && secret="`cauthtool $keyring -n client.$user -p`" monip="`echo $monhost | sed 's/:/ /g' | awk '{print $1}'`" diff --git a/src/auth/KeyRing.cc b/src/auth/KeyRing.cc index 5d00faaf49afe..b4c48b069a209 100644 --- a/src/auth/KeyRing.cc +++ b/src/auth/KeyRing.cc @@ -30,41 +30,29 @@ using namespace std; KeyRing g_keyring; -bool KeyRing::load(const char *filename_list) +int KeyRing::load(const char *filename) { - string k = filename_list; - string filename; - list ls; - get_str_list(k, ls); + if (!filename) + return -EINVAL; + bufferlist bl; - bool loaded = false; - for (list::iterator p = ls.begin(); p != ls.end(); p++) { - // subst in home dir? - size_t pos = p->find("~/"); - if (pos != string::npos) { - const char *home = getenv("HOME"); - if (home) - p->replace(pos, 1, getenv("HOME")); - else - continue; // skip this item, we couldn't do the substitution - } + int ret = bl.read_file(filename); + if (ret) { + derr << "error reading file: " << filename << dendl; + return ret; + } - if (bl.read_file(p->c_str(), true) == 0) { - loaded = true; - filename = *p; - break; - } + try { + bufferlist::iterator iter = bl.begin(); + decode(iter); } - if (!loaded) { - dout(0) << "can't open key file(s) " << filename_list << dendl; - return false; + catch (const buffer::error &err) { + derr << "error parsing file " << filename << dendl; + return -EIO; } - bufferlist::iterator p = bl.begin(); - decode(p); - - dout(2) << "loaded key file " << filename << dendl; - return true; + dout(2) << "KeyRing::load: loaded key file " << filename << dendl; + return 0; } void KeyRing::print(ostream& out) diff --git a/src/auth/KeyRing.h b/src/auth/KeyRing.h index 0a442d99eca22..80231e2af0dc5 100644 --- a/src/auth/KeyRing.h +++ b/src/auth/KeyRing.h @@ -26,7 +26,7 @@ class KeyRing { public: map& get_keys() { return keys; } // yuck - bool load(const char *filename); + int load(const char *filename); void print(ostream& out); // accessors diff --git a/src/common/common_init.cc b/src/common/common_init.cc index 5c877077a272f..69dd5c75f7000 100644 --- a/src/common/common_init.cc +++ b/src/common/common_init.cc @@ -32,6 +32,45 @@ void common_set_defaults(bool daemon) } } +static void keyring_init(const char *filename) +{ + int ret = g_keyring.load(filename); + if (ret) { + derr << "keyring_init: failed to load " << filename << dendl; + return; + } + + if (g_conf.key && g_conf.key[0]) { + string k = g_conf.key; + EntityAuth ea; + ea.key.decode_base64(k); + g_keyring.add(*g_conf.entity_name, ea); + } else if (g_conf.keyfile && g_conf.keyfile[0]) { + char buf[100]; + int fd = ::open(g_conf.keyfile, O_RDONLY); + if (fd < 0) { + int err = errno; + derr << "unable to open " << g_conf.keyfile << ": " + << cpp_strerror(err) << dendl; + exit(1); + } + int len = ::read(fd, buf, sizeof(buf)); + if (len < 0) { + int err = errno; + derr << "unable to read key from " << g_conf.keyfile << ": " + << cpp_strerror(err) << dendl; + exit(1); + } + ::close(fd); + + buf[len] = 0; + string k = buf; + EntityAuth ea; + ea.key.decode_base64(k); + g_keyring.add(*g_conf.entity_name, ea); + } +} + void common_init(std::vector& args, const char *module_type, bool init_keys) { tls_init(); @@ -55,37 +94,7 @@ void common_init(std::vector& args, const char *module_type, bool i } #endif //HAVE_LIBTCMALLOC - if (init_keys && is_supported_auth(CEPH_AUTH_CEPHX)) { - g_keyring.load(g_conf.keyring); - - if (strlen(g_conf.key)) { - string k = g_conf.key; - EntityAuth ea; - ea.key.decode_base64(k); - g_keyring.add(*g_conf.entity_name, ea); - } else if (strlen(g_conf.keyfile)) { - char buf[100]; - int fd = ::open(g_conf.keyfile, O_RDONLY); - if (fd < 0) { - int err = errno; - derr << "unable to open " << g_conf.keyfile << ": " - << cpp_strerror(err) << dendl; - exit(1); - } - int len = ::read(fd, buf, sizeof(buf)); - if (len < 0) { - int err = errno; - derr << "unable to read key from " << g_conf.keyfile << ": " - << cpp_strerror(err) << dendl; - exit(1); - } - ::close(fd); - - buf[len] = 0; - string k = buf; - EntityAuth ea; - ea.key.decode_base64(k); - g_keyring.add(*g_conf.entity_name, ea); - } - } + if (init_keys && is_supported_auth(CEPH_AUTH_CEPHX)) + keyring_init(g_conf.keyring); } + diff --git a/src/config.cc b/src/config.cc index 83781f548ddb7..28adc6c4fdf1b 100644 --- a/src/config.cc +++ b/src/config.cc @@ -368,7 +368,7 @@ static struct config_option config_optionsp[] = { OPTION(debug_finisher, 0, OPT_INT, 1), OPTION(key, 0, OPT_STR, ""), OPTION(keyfile, 'K', OPT_STR, ""), - OPTION(keyring, 'k', OPT_STR, "~/.ceph/keyring.bin, /etc/ceph/keyring.bin, .ceph_keyring"), + OPTION(keyring, 'k', OPT_STR, "/etc/ceph/keyring.bin"), OPTION(clock_lock, 0, OPT_BOOL, false), OPTION(clock_tare, 0, OPT_BOOL, false), OPTION(ms_tcp_nodelay, 0, OPT_BOOL, true), diff --git a/src/mon/AuthMonitor.cc b/src/mon/AuthMonitor.cc index 1c43c99d56fd0..ba8812c360391 100644 --- a/src/mon/AuthMonitor.cc +++ b/src/mon/AuthMonitor.cc @@ -89,43 +89,13 @@ void AuthMonitor::on_active() */ } -int AuthMonitor::read_keyfile(bufferlist &bl, std::string &keyfile) -{ - keyfile.clear(); - bl.clear(); - - if (!g_conf.keyring) - return 2; - list ls; - get_str_list(string(g_conf.keyring), ls); - for (list::const_iterator p = ls.begin(); p != ls.end(); ++p) { - if (bl.read_file(p->c_str()) == 0) { - keyfile = *p; - return 0; - } - bl.clear(); - } - return 1; -} - void AuthMonitor::create_initial(bufferlist& bl) { dout(10) << "create_initial -- creating initial map" << dendl; - bufferlist kbl; - string keyfile; - if (read_keyfile(kbl, keyfile) == 0) { - KeyRing keyring; - bool read_ok = false; - try { - bufferlist::iterator iter = kbl.begin(); - ::decode(keyring, iter); - read_ok = true; - } catch (const buffer::error &err) { - cerr << "error reading file " << g_conf.keyring << std::endl; - } - if (read_ok) - import_keyring(keyring); + KeyRing keyring; + if (keyring.load(g_conf.keyring) == 0) { + import_keyring(keyring); } max_global_id = MIN_GLOBAL_ID; diff --git a/src/mon/AuthMonitor.h b/src/mon/AuthMonitor.h index e4bd85e25ef55..bd183b41388e7 100644 --- a/src/mon/AuthMonitor.h +++ b/src/mon/AuthMonitor.h @@ -93,7 +93,6 @@ private: void on_active(); void election_finished(); bool should_propose(double& delay); - static int read_keyfile(bufferlist &bl, std::string &keyfile); void create_initial(bufferlist& bl); bool update_from_paxos(); void create_pending(); // prepare a new pending -- 2.39.5