rgw: better error responses, logging for specific error cases

Also, adjusting return values

Signed-off-by: Yehuda Sadeh <yehuda@inktank.com>

4 files changed, 192 insertions, 45 deletions

diff --git a/src/rgw/rgw_policy_s3.cc b/src/rgw/rgw_policy_s3.cc
index c0ec993276b..91adbe85233 100644
--- a/src/rgw/rgw_policy_s3.cc
+++ b/src/rgw/rgw_policy_s3.cc
@@ -8,13 +8,12 @@
 
 #define dout_subsys ceph_subsys_rgw
 
-
 class RGWPolicyCondition {
 protected:
   string v1;
   string v2;
 
-  virtual bool check(const string& first, const string& second) = 0;
+  virtual bool check(const string& first, const string& second, string& err_msg) = 0;
 
 public:
   virtual ~RGWPolicyCondition() {}
@@ -24,13 +23,20 @@ public:
     v2 = _v2;
   }
 
-  bool check(RGWPolicyEnv *env, map<string, bool, ltstr_nocase>& checked_vars) {
+  bool check(RGWPolicyEnv *env, map<string, bool, ltstr_nocase>& checked_vars, string& err_msg) {
      string first, second;
      env->get_value(v1, first, checked_vars);
      env->get_value(v2, second, checked_vars);
 
      dout(1) << "policy condition check " << v1 << " [" << first << "] " << v2 << " [" << second << "]" << dendl;
-     return check(first, second);
+     bool ret = check(first, second, err_msg);
+     if (!ret) {
+       err_msg.append(": ");
+       err_msg.append(v1);
+       err_msg.append(", ");
+       err_msg.append(v2);
+     }
+     return ret;
   }
 
 };
@@ -38,15 +44,23 @@ public:
 
 class RGWPolicyCondition_StrEqual : public RGWPolicyCondition {
 protected:
-  bool check(const string& first, const string& second) {
-    return first.compare(second) == 0;
+  bool check(const string& first, const string& second, string& msg) {
+    bool ret = first.compare(second) == 0;
+    if (!ret) {
+      msg = "Policy condition failed: eq";
+    }
+    return ret;
   }
 };
 
 class RGWPolicyCondition_StrStartsWith : public RGWPolicyCondition {
 protected:
-  bool check(const string& first, const string& second) {
-    return first.compare(0, second.size(), second) == 0;
+  bool check(const string& first, const string& second, string& msg) {
+    bool ret = first.compare(0, second.size(), second) == 0;
+    if (!ret) {
+      msg = "Policy condition failed: starts-with";
+    }
+    return ret;
   }
 };
 
@@ -80,7 +94,7 @@ bool RGWPolicyEnv::get_value(const string& s, string& val, map<string, bool, lts
 }
 
 
-bool RGWPolicyEnv::match_policy_vars(map<string, bool, ltstr_nocase>& policy_vars)
+bool RGWPolicyEnv::match_policy_vars(map<string, bool, ltstr_nocase>& policy_vars, string& err_msg)
 {
   map<string, string, ltstr_nocase>::iterator iter;
   string ignore_prefix = "x-ignore-";
@@ -89,6 +103,8 @@ bool RGWPolicyEnv::match_policy_vars(map<string, bool, ltstr_nocase>& policy_var
     if (strncasecmp(ignore_prefix.c_str(), var.c_str(), ignore_prefix.size()) == 0)
       continue;
     if (policy_vars.count(var) == 0) {
+      err_msg = "Policy missing condition: ";
+      err_msg.append(iter->first);
       dout(1) << "env var missing in policy: " << iter->first << dendl;
       return false;
     }
@@ -116,7 +132,7 @@ int RGWPolicy::set_expires(const string& e)
   return 0;
 }
 
-int RGWPolicy::add_condition(const string& op, const string& first, const string& second)
+int RGWPolicy::add_condition(const string& op, const string& first, const string& second, string& err_msg)
 {
   RGWPolicyCondition *cond = NULL;
   if (stringcasecmp(op, "eq") == 0) {
@@ -126,12 +142,18 @@ int RGWPolicy::add_condition(const string& op, const string& first, const string
   } else if (stringcasecmp(op, "content-length-range") == 0) {
     off_t min, max;
     int r = stringtoll(first, &min);
-    if (r < 0)
+    if (r < 0) {
+      err_msg = "Bad content-length-range param";
+      dout(0) << "bad content-length-range param: " << first << dendl;
       return r;
+    }
 
     r = stringtoll(second, &max);
-    if (r < 0)
+    if (r < 0) {
+      err_msg = "Bad content-length-range param";
+      dout(0) << "bad content-length-range param: " << second << dendl;
       return r;
+    }
 
     if (min > min_length)
       min_length = min;
@@ -142,8 +164,12 @@ int RGWPolicy::add_condition(const string& op, const string& first, const string
     return 0;
   }
 
-  if (!cond)
+  if (!cond) {
+    err_msg = "Invalid condition: ";
+    err_msg.append(op);
+    dout(0) << "invalid condition: " << op << dendl;
     return -EINVAL;
+  }
 
   cond->set_vals(first, second);
   
@@ -152,11 +178,12 @@ int RGWPolicy::add_condition(const string& op, const string& first, const string
   return 0;
 }
 
-int RGWPolicy::check(RGWPolicyEnv *env)
+int RGWPolicy::check(RGWPolicyEnv *env, string& err_msg)
 {
   uint64_t now = ceph_clock_now(NULL).sec();
   if (expires <= now) {
     dout(0) << "NOTICE: policy calculated as expired: " << expiration_str << dendl;
+    err_msg = "Policy expired";
     return -EACCES; // change to condition about expired policy following S3
   }
 
@@ -166,55 +193,71 @@ int RGWPolicy::check(RGWPolicyEnv *env)
     const string& name = p.first;
     const string& check_val = p.second;
     string val;
-    if (!env->get_var(name, val))
-      return -EINVAL;
+    if (!env->get_var(name, val)) {
+      err_msg = "Policy check failed, variable not found: ";
+      err_msg.append(name);
+      return -EACCES;
+    }
 
     set_var_checked(name);
 
     dout(20) << "comparing " << name << " [" << val << "], " << check_val << dendl;
     if (val.compare(check_val) != 0) {
+      err_msg = "Policy check failed, variable not met condition: ";
+      err_msg.append(name);
       dout(1) << "policy check failed, val=" << val << " != " << check_val << dendl;
-      return -EINVAL;
+      return -EACCES;
     }
   }
 
   list<RGWPolicyCondition *>::iterator citer;
   for (citer = conditions.begin(); citer != conditions.end(); ++citer) {
     RGWPolicyCondition *cond = *citer;
-    if (!cond->check(env, checked_vars)) {
-      return -EINVAL;
+    if (!cond->check(env, checked_vars, err_msg)) {
+      return -EACCES;
     }
   }
 
-  if (!env->match_policy_vars(checked_vars)) {
+  if (!env->match_policy_vars(checked_vars, err_msg)) {
     dout(1) << "missing policy condition" << dendl;
-    return -EINVAL;
+    return -EACCES;
   }
   return 0;
 }
 
 
-int RGWPolicy::from_json(bufferlist& bl)
+int RGWPolicy::from_json(bufferlist& bl, string& err_msg)
 {
   RGWJSONParser parser;
 
-  if (!parser.parse(bl.c_str(), bl.length()))
+  if (!parser.parse(bl.c_str(), bl.length())) {
+    err_msg = "Malformed JSON";
+    dout(0) << "malformed json" << dendl;
     return -EINVAL;
+  }
 
   // as no time was included in the request, we hope that the user has included a short timeout
   JSONObjIter iter = parser.find_first("expiration");
-  if (iter.end())
+  if (iter.end()) {
+    err_msg = "Policy missing expiration";
+    dout(0) << "expiration not found" << dendl;
     return -EINVAL; // change to a "no expiration" error following S3
+  }
 
   JSONObj *obj = *iter;
   expiration_str = obj->get_data();
   int r = set_expires(expiration_str);
-  if (r < 0)
+  if (r < 0) {
+    err_msg = "Failed to parse policy expiration";
     return r;
+  }
 
   iter = parser.find_first("conditions");
-  if (iter.end())
+  if (iter.end()) {
+    err_msg = "Policy missing conditions";
+    dout(0) << "conditions not found" << dendl;
     return -EINVAL; // change to a "no conditions" error following S3
+  }
 
   obj = *iter;
 
@@ -231,10 +274,12 @@ int RGWPolicy::from_json(bufferlist& bl)
 	JSONObj *o = *aiter;
         v.push_back(o->get_data());
       }
-      if (i != 3 || !aiter.end())  /* we expect exactly 3 arguments here */
+      if (i != 3 || !aiter.end()) { /* we expect exactly 3 arguments here */
+        err_msg = "Bad condition array, expecting 3 arguments";
         return -EINVAL;
+      }
 
-      int r = add_condition(v[0], v[1], v[2]);
+      int r = add_condition(v[0], v[1], v[2], err_msg);
       if (r < 0)
         return r;
     } else {
diff --git a/src/rgw/rgw_policy_s3.h b/src/rgw/rgw_policy_s3.h
index d002bece8fb..84a2ee71751 100644
--- a/src/rgw/rgw_policy_s3.h
+++ b/src/rgw/rgw_policy_s3.h
@@ -19,7 +19,7 @@ public:
   void add_var(const string& name, const string& value);
   bool get_var(const string& name, string& val);
   bool get_value(const string& s, string& val, std::map<std::string, bool, ltstr_nocase>& checked_vars);
-  bool match_policy_vars(map<string, bool, ltstr_nocase>& policy_vars);
+  bool match_policy_vars(map<string, bool, ltstr_nocase>& policy_vars, string& err_msg);
 };
 
 class RGWPolicyCondition;
@@ -45,12 +45,12 @@ public:
     checked_vars[var] = true;
   }
 
-  int add_condition(const std::string& op, const std::string& first, const std::string& second);
+  int add_condition(const std::string& op, const std::string& first, const std::string& second, string& err_msg);
   void add_simple_check(const std::string& var, const std::string& value) {
     var_checks.push_back(pair<string, string>(var, value));
   }
 
-  int check(RGWPolicyEnv *env);
-  int from_json(bufferlist& bl);
+  int check(RGWPolicyEnv *env, string& err_msg);
+  int from_json(bufferlist& bl, string& err_msg);
 };
 #endif
diff --git a/src/rgw/rgw_rest_s3.cc b/src/rgw/rgw_rest_s3.cc
index 04ff70601a6..3f44700353d 100644
--- a/src/rgw/rgw_rest_s3.cc
+++ b/src/rgw/rgw_rest_s3.cc
@@ -705,8 +705,10 @@ int RGWPostObj_ObjStore_S3::get_params()
 
   parse_params(req_content_type_str, req_content_type, params);
 
-  if (req_content_type.compare("multipart/form-data") != 0)
+  if (req_content_type.compare("multipart/form-data") != 0) {
+    err_msg = "Request Content-Type is not multipart/form-data";
     return -EINVAL;
+  }
 
   if (s->cct->_conf->subsys.should_gather(ceph_subsys_rgw, 20)) {
     ldout(s->cct, 20) << "request content_type_str=" << req_content_type_str << dendl;
@@ -721,8 +723,10 @@ int RGWPostObj_ObjStore_S3::get_params()
   env.add_var("bucket", s->bucket.name);
 
   map<string, string>::iterator iter = params.find("boundary");
-  if (iter == params.end())
+  if (iter == params.end()) {
+    err_msg = "Missing multipart boundary specification";
     return -EINVAL;
+  }
 
   // create the boundary
   boundary = "--";
@@ -749,8 +753,10 @@ int RGWPostObj_ObjStore_S3::get_params()
       }
     }
 
-    if (done) /* unexpected here */
+    if (done) { /* unexpected here */
+      err_msg = "Malformed request";
       return -EINVAL;
+    }
 
     if (stringcasecmp(part.name, "file") == 0) { /* beginning of data transfer */
       struct post_part_field& field = part.fields["Content-Disposition"];
@@ -766,6 +772,7 @@ int RGWPostObj_ObjStore_S3::get_params()
     bool boundary;
     r = read_data(part.data, RGW_MAX_CHUNK_SIZE, &boundary, &done);
     if (!boundary) {
+      err_msg = "Couldn't find boundary";
       return -EINVAL;
     }
     parts[part.name] = part;
@@ -773,11 +780,15 @@ int RGWPostObj_ObjStore_S3::get_params()
     env.add_var(part.name, part_str);
   } while (!done);
 
-  if (!part_str("key", &s->object_str))
+  if (!part_str("key", &s->object_str)) {
+    err_msg = "Key not specified";
     return -EINVAL;
+  }
 
   rebuild_key(s->object_str);
 
+  env.add_var("key", s->object_str);
+
   part_str("Content-Type", &content_type);
   env.add_var("Content-Type", content_type);
 
@@ -821,11 +832,13 @@ int RGWPostObj_ObjStore_S3::get_policy()
     string s3_access_key;
     if (!part_str("AWSAccessKeyId", &s3_access_key)) {
       ldout(s->cct, 0) << "No S3 access key found!" << dendl;
+      err_msg = "Missing access key";
       return -EINVAL;
     }
     string signature_str;
     if (!part_str("signature", &signature_str)) {
       ldout(s->cct, 0) << "No signature found!" << dendl;
+      err_msg = "Missing signature";
       return -EINVAL;
     }
 
@@ -834,7 +847,8 @@ int RGWPostObj_ObjStore_S3::get_policy()
     ret = rgw_get_user_info_by_access_key(store, s3_access_key, user_info);
     if (ret < 0) {
       ldout(s->cct, 0) << "User lookup failed!" << dendl;
-      return -EINVAL;
+      err_msg = "Bad access key / signature";
+      return -EACCES;
     }
 
     map<string, RGWAccessKey> access_keys  = user_info.access_keys;
@@ -855,7 +869,8 @@ int RGWPostObj_ObjStore_S3::get_policy()
       ldout(s->cct, 0) << "Signature verification failed!" << dendl;
       ldout(s->cct, 0) << "expected: " << signature_str.c_str() << dendl;
       ldout(s->cct, 0) << "got: " << encoded_hmac.c_str() << dendl;
-      return -EINVAL;
+      err_msg = "Bad access key / signature";
+      return -EACCES;
     }
     ldout(s->cct, 0) << "Successful Signature Verification!" << dendl;
     bufferlist decoded_policy;
@@ -863,6 +878,7 @@ int RGWPostObj_ObjStore_S3::get_policy()
       decoded_policy.decode_base64(encoded_policy);
     } catch (buffer::error& err) {
       ldout(s->cct, 0) << "failed to decode_base64 policy" << dendl;
+      err_msg = "Could not decode policy";
       return -EINVAL;
     }
 
@@ -870,8 +886,11 @@ int RGWPostObj_ObjStore_S3::get_policy()
 
     ldout(s->cct, 0) << "POST policy: " << decoded_policy.c_str() << dendl;
 
-    int r = post_policy.from_json(decoded_policy);
+    int r = post_policy.from_json(decoded_policy, err_msg);
     if (r < 0) {
+      if (err_msg.empty()) {
+	err_msg = "Failed to parse policy";
+      }
       ldout(s->cct, 0) << "failed to parse policy" << dendl;
       return -EINVAL;
     }
@@ -880,8 +899,11 @@ int RGWPostObj_ObjStore_S3::get_policy()
     post_policy.set_var_checked("policy");
     post_policy.set_var_checked("signature");
 
-    r = post_policy.check(&env);
+    r = post_policy.check(&env, err_msg);
     if (r < 0) {
+      if (err_msg.empty()) {
+	err_msg = "Policy check failed";
+      }
       ldout(s->cct, 0) << "policy check failed" << dendl;
       return r;
     }
@@ -896,8 +918,10 @@ int RGWPostObj_ObjStore_S3::get_policy()
 
   RGWAccessControlPolicy_S3 s3policy(s->cct);
   ldout(s->cct, 20) << "canned_acl=" << canned_acl << dendl;
-  if (!s3policy.create_canned(s->user.user_id, "", canned_acl))
+  if (!s3policy.create_canned(s->user.user_id, "", canned_acl)) {
+    err_msg = "Bad canned ACLs";
     return -EINVAL;
+  }
 
   policy = s3policy;
 
@@ -948,19 +972,93 @@ int RGWPostObj_ObjStore_S3::get_data(bufferlist& bl)
   return bl.length();
 }
 
+static void escape_char(char c, string& dst)
+{
+  char buf[16];
+  snprintf(buf, sizeof(buf), "%%%.2X", (unsigned int)c);
+  dst.append(buf);
+}
+
+static bool char_needs_url_encoding(char c)
+{
+  if (c < 0x20 || c >= 0x7f)
+    return true;
+
+  switch (c) {
+    case 0x20:
+    case 0x22:
+    case 0x23:
+    case 0x25:
+    case 0x26:
+    case 0x2B:
+    case 0x2C:
+    case 0x2F:
+    case 0x3A:
+    case 0x3B:
+    case 0x3C:
+    case 0x3E:
+    case 0x3D:
+    case 0x3F:
+    case 0x40:
+    case 0x5B:
+    case 0x5D:
+    case 0x5C:
+    case 0x5E:
+    case 0x60:
+    case 0x7B:
+    case 0x7D:
+      return true;
+  }
+  return false;
+}
+
+static void url_escape(const string& src, string& dst)
+{
+  const char *p = src.c_str();
+  for (unsigned i = 0; i < src.size(); i++, p++) {
+    if (char_needs_url_encoding(*p)) {
+      escape_char(*p, dst);
+      continue;
+    }
+
+    dst.append(p, 1);
+  }
+}
+
 void RGWPostObj_ObjStore_S3::send_response()
 {
   if (ret == 0 && parts.count("success_action_redirect")) {
-    string success_action_redirect;
+    string redirect;
 
-    part_str("success_action_redirect", &success_action_redirect);
+    part_str("success_action_redirect", &redirect);
 
-    int r = check_utf8(success_action_redirect.c_str(), success_action_redirect.size());
+    string bucket;
+    string key;
+    string etag_str = "\"";
+
+    etag_str.append(etag);
+    etag_str.append("\"");
+
+    string etag_url;
+
+    url_escape(s->bucket_name, bucket);
+    url_escape(s->object_str, key);
+    url_escape(etag_str, etag_url);
+
+    
+    redirect.append("?bucket=");
+    redirect.append(bucket);
+    redirect.append("&key=");
+    redirect.append(key);
+    redirect.append("&etag=");
+    redirect.append(etag_url);
+
+    int r = check_utf8(redirect.c_str(), redirect.size());
     if (r < 0) {
       ret = r;
       goto done;
     }
-    dump_redirect(s, success_action_redirect);
+    dump_redirect(s, redirect);
     ret = STATUS_REDIRECT;
   } else if (ret == 0 && parts.count("success_action_status")) {
     string status_string;
@@ -984,6 +1082,8 @@ void RGWPostObj_ObjStore_S3::send_response()
 	ret = STATUS_NO_CONTENT;
 	break;
     }
+  } else if (!ret) {
+    ret = STATUS_NO_CONTENT;
   }
 
 done:
@@ -995,6 +1095,7 @@ done:
     s->formatter->dump_string("Key", s->object_str.c_str());
     s->formatter->close_section();
   }
+  s->err.message = err_msg;
   set_req_state_err(s, ret);
   dump_errno(s);
   dump_content_length(s, s->formatter->get_len());
diff --git a/src/rgw/rgw_rest_s3.h b/src/rgw/rgw_rest_s3.h
index 25e67bbf52d..05ca142f6bc 100644
--- a/src/rgw/rgw_rest_s3.h
+++ b/src/rgw/rgw_rest_s3.h
@@ -101,6 +101,7 @@ class RGWPostObj_ObjStore_S3 : public RGWPostObj_ObjStore {
   map<string, post_form_part, const ltstr_nocase> parts;  
   RGWPolicyEnv env;
   RGWPolicy post_policy;
+  string err_msg;
 
   int read_with_boundary(bufferlist& bl, uint64_t max, bool check_eol,
                          bool *reached_boundary,