From c70e76f3062c5dadbbe395afd957084057927669 Mon Sep 17 00:00:00 2001 From: Brian Bockelman Date: Sat, 6 Feb 2021 12:57:38 -0600 Subject: [PATCH] XrdMacaroons: Normalize slashes when issuing and authorizing macaroons. For authorization purposes, there have been many requests to be more lenient about mismatches slashes (that is, macaroon was requested for `/foo//bar` but the user tries to access `/foo/bar`). This normalizes the slashes when the macaroon is issued and when it is used for authorization. --- src/XrdMacaroons/XrdMacaroonsAuthz.cc | 13 ++++++------ src/XrdMacaroons/XrdMacaroonsHandler.cc | 27 ++++++++++++++++++++++++- src/XrdMacaroons/XrdMacaroonsHandler.hh | 15 ++++++++++++++ 3 files changed, 47 insertions(+), 8 deletions(-) diff --git a/src/XrdMacaroons/XrdMacaroonsAuthz.cc b/src/XrdMacaroons/XrdMacaroonsAuthz.cc index 081a04ab876..0287d5499db 100644 --- a/src/XrdMacaroons/XrdMacaroonsAuthz.cc +++ b/src/XrdMacaroons/XrdMacaroonsAuthz.cc @@ -240,7 +240,7 @@ Authz::Access(const XrdSecEntity *Entity, const char *path, AuthzCheck::AuthzCheck(const char *req_path, const Access_Operation req_oper, ssize_t max_duration, XrdSysError &log) : m_max_duration(max_duration), m_log(log), - m_path(req_path), + m_path(NormalizeSlashes(req_path)), m_oper(req_oper), m_now(time(NULL)) { @@ -377,8 +377,9 @@ AuthzCheck::verify_activity(const unsigned char * pred, size_t pred_sz) int AuthzCheck::verify_path(const unsigned char * pred, size_t pred_sz) { - std::string pred_str(reinterpret_cast(pred), pred_sz); - if (strncmp("path:", pred_str.c_str(), 5)) {return 1;} + std::string pred_str_raw(reinterpret_cast(pred), pred_sz); + if (strncmp("path:", pred_str_raw.c_str(), 5)) {return 1;} + std::string pred_str = NormalizeSlashes(pred_str_raw.substr(5)); m_log.Log(LogMask::Debug, "AuthzCheck", "running verify path", pred_str.c_str()); if ((m_path.find("/./") != std::string::npos) || @@ -387,10 +388,8 @@ AuthzCheck::verify_path(const unsigned char * pred, size_t pred_sz) m_log.Log(LogMask::Info, "AuthzCheck", "invalid requested path", m_path.c_str()); return 1; } - size_t compare_chars = pred_str.size() - 5; - if (pred_str[compare_chars + 5 - 1] == '/') {compare_chars--;} - int result = strncmp(pred_str.c_str() + 5, m_path.c_str(), compare_chars); + int result = strncmp(pred_str.c_str(), m_path.c_str(), pred_str.size()); if (!result) { m_log.Log(LogMask::Debug, "AuthzCheck", "path request verified for", m_path.c_str()); @@ -399,7 +398,7 @@ AuthzCheck::verify_path(const unsigned char * pred, size_t pred_sz) // to READ_METADATA for /foo. else if (m_oper == AOP_Stat) { - result = strncmp(m_path.c_str(), pred_str.c_str() + 5, m_path.size()); + result = strncmp(m_path.c_str(), pred_str.c_str(), m_path.size()); if (!result) {m_log.Log(LogMask::Debug, "AuthzCheck", "READ_METADATA path request verified for", m_path.c_str());} else {m_log.Log(LogMask::Debug, "AuthzCheck", "READ_METADATA path request NOT allowed", m_path.c_str());} } diff --git a/src/XrdMacaroons/XrdMacaroonsHandler.cc b/src/XrdMacaroons/XrdMacaroonsHandler.cc index fab84e14db9..88fb2b2bcc5 100644 --- a/src/XrdMacaroons/XrdMacaroonsHandler.cc +++ b/src/XrdMacaroons/XrdMacaroonsHandler.cc @@ -52,6 +52,27 @@ char *unquote(const char *str) { } +std::string Macaroons::NormalizeSlashes(const std::string &input) +{ + std::string output; + // In most cases, the output should be "about as large" + // as the input + output.reserve(input.size()); + char prior_chr = '\0'; + size_t output_idx = 0; + for (size_t idx = 0; idx < input.size(); idx++) { + char chr = input[idx]; + if (prior_chr == '/' && chr == '/') { + output_idx++; + continue; + } + output += input[output_idx]; + prior_chr = chr; + output_idx++; + } + return output; +} + static ssize_t determine_validity(const std::string& input) { @@ -119,7 +140,7 @@ Handler::GenerateID(const std::string &resource, std::stringstream ss; ss << "ID=" << result << ", "; - ss << "resource=" << resource << ", "; + ss << "resource=" << NormalizeSlashes(resource) << ", "; if (entity.prot[0] != '\0') {ss << "protocol=" << entity.prot << ", ";} if (entity.name) {ss << "name=" << entity.name << ", ";} if (entity.host) {ss << "host=" << entity.host << ", ";} @@ -513,6 +534,10 @@ Handler::GenerateMacaroonResponse(XrdHttpExtReq &req, const std::string &resourc } } + // Note we don't call `NormalizeSlashes` here; for backward compatibility reasons, we ensure the + // token issued is identical to what was working with prior versions of XRootD. This allows for a + // mix of old/new versions in a single cluster to interoperate. In a few years, it might be reasonable + // to invoke it here as well. std::string path_caveat = "path:" + resource; struct macaroon *mac_with_path = macaroon_add_first_party_caveat(mac_with_activities, reinterpret_cast(path_caveat.c_str()), diff --git a/src/XrdMacaroons/XrdMacaroonsHandler.hh b/src/XrdMacaroons/XrdMacaroonsHandler.hh index 6a2c95ae87d..6c1a5d8df7a 100644 --- a/src/XrdMacaroons/XrdMacaroonsHandler.hh +++ b/src/XrdMacaroons/XrdMacaroonsHandler.hh @@ -21,6 +21,21 @@ enum LogMask { All = 0xff }; +// 'Normalize' the macaroon path. This only takes care of double slashes +// but, as is common in XRootD, it doesn't treat these as a hierarchy. +// For example, these result in the same path: +// +// /foo/bar -> /foo/bar +// //foo////bar -> /foo/bar +// +// These are all distinct: +// +// /foo/bar -> /foo/bar +// /foo/bar/ -> /foo/bar/ +// /foo/baz//../bar -> /foo/baz/../bar +// +std::string NormalizeSlashes(const std::string &); + class Handler : public XrdHttpExtHandler { public: Handler(XrdSysError *log, const char *config, XrdOucEnv *myEnv,