New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rgw: pass authentication domain to civetweb #12861

Merged
merged 2 commits into from May 22, 2017

Conversation

Projects
None yet
3 participants
@theanalyst
Member

theanalyst commented Jan 10, 2017

This is needed to support absolute uris, as civetweb further cross
checks whether the request is made to the same domain or not, otherwise
civetweb ends up sending NULL response back to the client. Also set the
REQUEST_URI to the full request uri, since the old uri parameter is
deprecated.

This needs ceph/civetweb#16 also merged for full support of subdomain style requests

Fixes: http://tracker.ceph.com/issues/17657
Signed-off-by: Abhishek Lekshmanan abhishek@suse.com

@theanalyst theanalyst added the rgw label Jan 10, 2017

@theanalyst

This comment has been minimized.

Member

theanalyst commented Jan 10, 2017

civetweb offers the choice of both local_uri and the request_uri (which is currently used), if we set the uri as local_uri we could avoid an additional parsing we do in RGW where we get the path from abs uri, but I guess it is better to pass on the full request unmodified by civetweb, so set the value to request_uri

@theanalyst

This comment has been minimized.

Member

theanalyst commented Jan 10, 2017

Tested initially with aws-sdk-go, which by default creates requests with abs. urls, eg. https://gist.github.com/theanalyst/45732de392237ac3e01c26bc4ff9d19f

though by default boto doesn't allow this sort of urls, we can override boto's parent libraries to make similar requests, which we can probably use to add to s3 tests, an example here https://gist.github.com/theanalyst/ac32ed95a96f36d5ee10efc2673c6b7d

@theanalyst theanalyst requested review from yehudasa, mattbenjamin and cbodley Jan 10, 2017

@@ -64,6 +64,9 @@ int RGWCivetWebFrontend::run()
conf_map["run_as_user"] = std::move(uid_string);
}
/* Set authentication domains, this is needed for handling absolute uris in
requests */
conf_map["authentication_domain"] = g_conf->rgw_dns_name;

This comment has been minimized.

@yehudasa

yehudasa Jan 10, 2017

Member

@theanalyst this is not necessarily the only domain that we support. We'd have a different domain for static websites, and nowadays we check every domain to match buckets (see code in rgw_rest.cc). Instead of having civetweb limit us to a specific domain, I'd rather have it send us everything and we can make the decision in rgw.

This comment has been minimized.

@theanalyst

theanalyst Jan 10, 2017

Member

currently the problem is that civetweb validates that the request is made to the same domain, or errors out there we could probably modify civetweb to not do any checking at all on domain and only rely on our checks,

This comment has been minimized.

@theanalyst

theanalyst Jan 10, 2017

Member

@yehudasa this is the current behaviour of civetweb (with the proposed cherry-pick) civetweb/civetweb#197 (comment)

This comment has been minimized.

@yehudasa

yehudasa Jan 10, 2017

Member

Yeah, so we should modify civetweb (add a new configurable to allow that so that it can be accepted upstream).

This comment has been minimized.

@theanalyst

theanalyst Jan 10, 2017

Member

ack, will add a configurable to bypass domain check

@theanalyst

This comment has been minimized.

Member

theanalyst commented Jan 11, 2017

@yehudasa updated the civetweb pr to use a new configurable to disable auth domain check, and updated our configuration of civetweb. I'll open a new pr against civetweb upstream for the configurable as well.

@theanalyst

This comment has been minimized.

Member

theanalyst commented Jan 13, 2017

@yehudasa updated the civetweb pr ceph/civetweb#16, also upstream merged the civetweb/civetweb#397

@theanalyst

This comment has been minimized.

Member

theanalyst commented Feb 2, 2017

@yehudasa can you take a look

@@ -111,7 +111,7 @@ void RGWCivetWeb::init_env(CephContext *cct)
}
env.set("REQUEST_METHOD", info->request_method);
env.set("REQUEST_URI", info->uri);
env.set("REQUEST_URI", info->request_uri); // get the full uri, we anyway handle abs uris later

This comment has been minimized.

@yehudasa

yehudasa Feb 2, 2017

Member

@theanalyst is this change needed? it's not clear to me what's the effect of this line

This comment has been minimized.

@theanalyst

theanalyst Feb 2, 2017

Member

https://github.com/civetweb/civetweb/blob/59fa55a87cdc8397f4f88e4b76881857f6e0622e/docs/api/mg_request_info.md civetweb marked their uri field as deprecated and introduced local_uri and request_uri, and uri field was set as local_uri for now, but maybe removed in future

@yehudasa

This comment has been minimized.

Member

yehudasa commented Feb 2, 2017

@theanalyst need to squash commits

rgw: pass authentication domain to civetweb
This is needed to support absolute uris, as civetweb further cross
checks whether the request is made to the same domain or not, otherwise
civetweb ends up sending NULL response back to the client. Also set the
REQUEST_URI to the full request uri, since the old uri parameter is
deprecated.

Fixes: http://tracker.ceph.com/issues/17657
Signed-off-by: Abhishek Lekshmanan <abhishek@suse.com>
@theanalyst

This comment has been minimized.

Member

theanalyst commented Feb 6, 2017

squashed the commits, we would want ceph/civetweb#16 merged and then updatin the submodule ref before the commit goes in right?

@yehudasa

This comment has been minimized.

Member

yehudasa commented Feb 6, 2017

@theanalyst yes, we need to have it merged. Submodule ref change needs to be part of this PR.

@theanalyst

This comment has been minimized.

Member

theanalyst commented Mar 7, 2017

@yehudasa is ceph/civetweb#16 ok for merging?

@theanalyst

This comment has been minimized.

Member

theanalyst commented Apr 26, 2017

@yehudasa ping

@yehudasa yehudasa self-requested a review Apr 26, 2017

@yehudasa

lgtm

@yehudasa

This comment has been minimized.

Member

yehudasa commented Apr 26, 2017

@theanalyst this is still missing the submodule update though

@theanalyst

This comment has been minimized.

Member

theanalyst commented Apr 26, 2017

@yehudasa adding, didn't know what hash to set it to before, setting to ceph/civetweb@de23828

rgw: update civetweb to support absolute urls
updating the submodule ref. to support absolute urls in civetweb

Signed-off-by: Abhishek Lekshmanan <alekshmanan@suse.com>
@theanalyst

This comment has been minimized.

Member

theanalyst commented May 17, 2017

scheduled a small subset of s3 suites for both before and after this PR (luminous branch and the current branch)
PASSED http://pulpito.ceph.com/abhi-2017-05-17_16:13:38-rgw-wip-absolute-urls---basic-smithi/
PASSED (except fro absolute urls which errors) http://pulpito.ceph.com/abhi-2017-05-17_16:12:50-rgw-wip-luminous---basic-smithi/

@theanalyst

This comment has been minimized.

Member

theanalyst commented May 18, 2017

@yehudasa I have a small QE run which is green for s3tests before this pr except for absolute urls, is this sufficient or do we need a full QE run with a subset or so?

@theanalyst

This comment has been minimized.

@theanalyst

This comment has been minimized.

Member

theanalyst commented May 22, 2017

there were failures in a few s3tests and swift tests, but these were headers/url encoding tests failing on apache fastcgi, maybe relevant to call teuthology with a --filter-out=apache

@cbodley

This comment has been minimized.

Contributor

cbodley commented May 22, 2017

there were failures in a few s3tests and swift tests

confirmed that the test failures were known issues with apache frontend 👍

@theanalyst

This comment has been minimized.

Member

theanalyst commented May 22, 2017

@yehudasa can you take a look again?

@yehudasa yehudasa merged commit 34a17d5 into ceph:master May 22, 2017

3 of 4 checks passed

Unmodifed Submodules Approval needed: modified submodules found
Details
Signed-off-by all commits in this PR are signed
Details
arm build successfully built on arm
Details
default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment