rgw: swift: The http referer should be parsed to compare in swift API #13005

Jing-Scott · 2017-01-19T15:45:36Z

The http referer should be parsed to compare with the url set on the container read acl. If we set .r:www.example.com on container read acl, we should parse the hostname 'www.example.com' of the http referer like 'http://www.example.com' from the http request.

Signed-off-by: Jing Wenjun jingwenjun@cmss.chinamobile.com

rzarzynski

@Jing-Scott: Overall this PR looks good. Some minor changes are needed.

rzarzynski · 2017-01-24T16:30:56Z

src/rgw/rgw_acl.h

                                   url_spec.length(), url_spec);
    }

    return false;
  }

+  bool get_http_host(std::string url, std::string& http_host) const {


SUGGESTION: may we take the url parameter as boost::string_ref and return boost::optional<boost::string_ref>? Then method signature would look like following one:

boost::optional<boost::string_ref> get_http_host(const boost::string_ref url) const

rzarzynski · 2017-01-24T16:32:35Z

src/rgw/rgw_acl.h

                                   url_spec.length(), url_spec);
    }

    return false;
  }

+  bool get_http_host(std::string url, std::string& http_host) const {
+    int pos = url.find("://");


Could you please switch to size_t?

rzarzynski · 2017-01-24T16:34:23Z

src/rgw/rgw_acl.h

                                   url_spec.length(), url_spec);
    }

    return false;
  }

+  bool get_http_host(std::string url, std::string& http_host) const {
+    int pos = url.find("://");
+    if (pos < 0 || pos == (int)url.size() - 3)


Please compare with the npos member of a corresponding string class.

rzarzynski · 2017-01-24T16:39:01Z

src/rgw/rgw_acl.h

                                   url_spec.length(), url_spec);
    }

    return false;
  }

+  bool get_http_host(std::string url, std::string& http_host) const {
+    int pos = url.find("://");
+    if (pos < 0 || pos == (int)url.size() - 3)


No magics, please. Consider boost::string_ref::ends_with, boost::algorithm::ends_with or even strlen("://").

rzarzynski · 2017-01-24T16:41:37Z

src/rgw/rgw_acl.h

                                   url_spec.length(), url_spec);
    }

    return false;
  }

+  bool get_http_host(std::string url, std::string& http_host) const {


No need to make this method public.

rzarzynski · 2017-01-24T16:53:48Z

src/rgw/rgw_acl.h

+      return false;
+
+    string scheme = url.substr(0, pos);
+    if (scheme.empty())


Maybe starts_with("://") in the previous if?

rzarzynski · 2017-01-24T17:15:32Z

src/rgw/rgw_acl.h

+
+    string url_sub = url.substr(pos + 3);
+    pos = url_sub.find('@');
+    if (pos == (int)url_sub.size())


Hmm, I would say this condition is always false.

rzarzynski · 2017-01-24T17:24:25Z

src/rgw/rgw_acl.h

+    if (pos == (int)url_sub.size())
+      return false;
+
+    url_sub = url_sub.substr(pos + 1);


If url_sub doesn't contain @ then we're doing std::string::npos + 1 which results to 0 on most implementations. However, this isn't a way to go.

Jing-Scott · 2017-01-26T03:00:30Z

@rzarzynski
Your comments make sense to me. I've updated the commit according to your suggestion. please check it . Thanks!

The http referer should be parsed to compare with the url set on the container read acl. If we set .r:www.example.com on container read acl, we should parse the hostname 'www.example.com' of the http referer like 'http://www.example.com' from the http request. Fixes: http://tracker.ceph.com/issues/18685 Signed-off-by: Jing Wenjun <jingwenjun@cmss.chinamobile.com>

rzarzynski

Only cosmetic changes needed. We're getting close. :-)

rzarzynski · 2017-01-26T13:12:42Z

src/rgw/rgw_acl.h

-    if (!get_http_host(http_referer, http_host))
+  bool is_match(boost::string_ref http_referer) const {
+    boost::optional<boost::string_ref> http_host = get_http_host(http_referer);
+    if ((*http_host).empty()) {


In get_http_host you might want to return boost::none if parsing fails. Then this conditional would look like:

if (! http_host) { return false; }

rzarzynski · 2017-01-26T13:24:16Z

src/rgw/rgw_acl.h

      return true;
    }

-    if (http_host.length() < url_spec.length()) {
+    if ((*http_host).length() < host_spec.length()) {


boost::optional has overloads for the -> operator, so you may write here (and in others places) just:

if (http_host->length() < host_spec.length()) {

rzarzynski · 2017-01-26T13:36:54Z

src/rgw/rgw_acl.h

      /* Wildcard support: a referer matches the spec when its last char are
       * perfectly equal to spec. */
-      return !http_host.compare(http_host.length() - url_spec.length(),
-                                   url_spec.length(), url_spec);
+      return (*http_host).ends_with(host_spec);


Wow, now the code is really much easier to understand. I missed the opportunity for ends_with last time. Thanks for applying it here!

rzarzynski · 2017-01-26T13:38:29Z

src/rgw/rgw_acl.h

-    std::string http_host;
-    if (!get_http_host(http_referer, http_host))
+  bool is_match(boost::string_ref http_referer) const {
+    boost::optional<boost::string_ref> http_host = get_http_host(http_referer);


Hmm, maybe const here?
BTW: using auto is also perfectly fine in the project.

rzarzynski · 2017-01-26T13:40:08Z

src/rgw/rgw_acl.h

+    size_t pos = url.find("://");
+    if (pos == boost::string_ref::npos || url.starts_with("://") ||
+        url.ends_with("://") || url.ends_with('@')) {
+      return boost::string_ref(nullptr, 0);


I would propose returning boost::none (according to one of the comments above).

Jing-Scott · 2017-01-26T15:21:22Z

@rzarzynski
done. thanks for your patience. your comments help me to understand boost::string_ref and boost::optional deeply!

rzarzynski

Thanks for the changes! Could you please also prepend the link to tracker issue with the Fixes tag? That and squashing commits would help with backporting the fix to Kraken and Jewel.

rzarzynski · 2017-01-26T15:37:43Z

src/rgw/rgw_acl.h

      /* Wildcard support: a referer matches the spec when its last char are
       * perfectly equal to spec. */
-      return !http_referer.compare(http_referer.length() - url_spec.length(),
-                                   url_spec.length(), url_spec);
+      return (*http_host).ends_with(host_spec);


The -> operator can be applied here as well. I've missed that last time, sorry.

oh, it's my carelessness!

rzarzynski · 2017-01-26T15:51:32Z

src/rgw/rgw_acl.h

+      return false;
+    }
+
+    boost::string_ref host_spec(url_spec);


Hmm, I'm not sure whether we really need the host_spec temporary. IIUC boost::string_ref has the operator== overload for std::string so we could compare *http_host directly with url_spec.

if using operator== like *http_host == url_spec directly, it will return error. So here I use ->compare to compare http_host with url_spec insteaded.

Ouch, my fault. Sorry.

Jing-Scott · 2017-01-27T01:02:42Z

Hi, @rzarzynski
I've squashed them. the bug is created in tracker and it is included in commit message. Pls check it! Thanks a lot!

http://tracker.ceph.com/issues/18685

rzarzynski

Looks very good, thanks! I will schedule a Teuthology run soon.

rzarzynski · 2017-01-30T11:54:57Z

The Teuthology run looks good to me. There is one but unrelated failure. Merging.

liewegas added bug-fix rgw labels Jan 19, 2017

Jing-Scott mentioned this pull request Jan 24, 2017

rgw: swift: the http referer acl in swift API should be shown #13003

Merged

rzarzynski self-assigned this Jan 24, 2017

rzarzynski requested changes Jan 24, 2017

View reviewed changes

rzarzynski requested changes Jan 26, 2017

View reviewed changes

Jing-Scott force-pushed the dev/swift-http-url-parse branch from 8a3c09d to 4129291 Compare January 26, 2017 15:10

rzarzynski requested changes Jan 26, 2017

View reviewed changes

Jing-Scott force-pushed the dev/swift-http-url-parse branch from 4129291 to c262931 Compare January 27, 2017 00:49

Jing-Scott force-pushed the dev/swift-http-url-parse branch from c262931 to 941dfad Compare January 27, 2017 05:09

rzarzynski approved these changes Jan 27, 2017

View reviewed changes

rzarzynski merged commit fca5b3c into ceph:master Jan 30, 2017

Jing-Scott deleted the dev/swift-http-url-parse branch February 2, 2017 11:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rgw: swift: The http referer should be parsed to compare in swift API #13005

rgw: swift: The http referer should be parsed to compare in swift API #13005

Jing-Scott commented Jan 19, 2017

rzarzynski left a comment

rzarzynski Jan 24, 2017 •

edited

rzarzynski Jan 24, 2017

rzarzynski Jan 24, 2017

rzarzynski Jan 24, 2017

rzarzynski Jan 24, 2017

rzarzynski Jan 24, 2017

rzarzynski Jan 24, 2017

rzarzynski Jan 24, 2017

Jing-Scott commented Jan 26, 2017 •

edited

rzarzynski left a comment

rzarzynski Jan 26, 2017

rzarzynski Jan 26, 2017

rzarzynski Jan 26, 2017

rzarzynski Jan 26, 2017

rzarzynski Jan 26, 2017

Jing-Scott commented Jan 26, 2017

rzarzynski left a comment

rzarzynski Jan 26, 2017

Jing-Scott Jan 27, 2017

rzarzynski Jan 26, 2017

Jing-Scott Jan 27, 2017 •

edited

rzarzynski Jan 27, 2017

Jing-Scott commented Jan 27, 2017

rzarzynski left a comment

rzarzynski commented Jan 30, 2017

rgw: swift: The http referer should be parsed to compare in swift API #13005

rgw: swift: The http referer should be parsed to compare in swift API #13005

Conversation

Jing-Scott commented Jan 19, 2017

rzarzynski left a comment

Choose a reason for hiding this comment

rzarzynski Jan 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jing-Scott commented Jan 26, 2017 • edited

rzarzynski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jing-Scott commented Jan 26, 2017

rzarzynski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jing-Scott Jan 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jing-Scott commented Jan 27, 2017

rzarzynski left a comment

Choose a reason for hiding this comment

rzarzynski commented Jan 30, 2017

rzarzynski Jan 24, 2017 •

edited

Jing-Scott commented Jan 26, 2017 •

edited

Jing-Scott Jan 27, 2017 •

edited