rgw: openssl 3 #10335

mdw-at-linuxbox · 2016-07-18T22:44:38Z

[ this is an updated copy of PR #8339 ]
Augment configuration so it's easy to support http & https in one civetweb frontend.

Get rid of an incorrect warning when using openssl.

Document the thing so people can use it.

And load shared libraries by soname.

djzort · 2016-07-18T22:51:19Z

please pull this asap! :)

mdw-at-linuxbox · 2016-07-20T21:56:19Z

somebody rearranged all the cmake stuff. I've force pushed a fix for that.

alimaredia · 2016-07-21T16:48:32Z

@mdw-at-linuxbox the CMake aspect of this looks fine to me. @tchaikov anything look out of place to you?

yehudasa · 2016-07-21T20:47:44Z

src/rgw/rgw_main.cc

-      config->get_val("port", 80, &port);
-
-      RGWProcessEnv env = { store, &rest, olog, port };
+      RGWProcessEnv env = { store, &rest, olog, 0 };


@mdw-at-linuxbox you can't remove the port here without addressing its use at RGWMongoose::init_env()

Ok, that's related to : http://tracker.ceph.com/issues/16548 . I sent mail 20160628 about that where I went through the code that does this. The basic problem is that the current code path that uses that assumes that it can guess the server port based solely on whether the request came in with ssl or not. That means not more than 1 ssl and 1 non-ssl port. Do we really want to enforce that?

@mdw-at-linuxbox we need to have some solution to that, a 0 port number is not a proper one.

We could either

parse port=".." the way civetweb does, ie. allow ip:port combo, r, s params that civetweb allows etc.

alternatively, we can set override the env.port after initializing port from civetweb and getting the value of ports civetweb parses using the mg_get_ports api ?; ie something like this in rgw_civetweb_frontend:

(assume ports = new int [num ports] etc) size_t len = mg_get_ports(ctx, num_ports, ports, ssl_ports) if (len){ env.port = ports[0] ;}

etc
?

smithfarm · 2016-07-26T20:22:20Z

@mdw-at-linuxbox Does this PR fix http://tracker.ceph.com/issues/13718 ? If so, could you add

Fixes: http://tracker.ceph.com/issues/13718

immediately above the Signed-off-by: line of the main/most-relevant commit?

mdw-at-linuxbox · 2016-07-26T21:17:38Z

Yes, this pull request would address #13718 - excepting that the problem Yehuda raised above makes this more complicated and probably a separate patch.

The top level CMakeLists.txt sets "HAVE_OPENSSL". It should instead set USE_OPENSSL.

The "extra" blank line is because I had stuck a large glop of SSL obscurity in just before, and I didn't think the "add_subdirectory(rgw)" was obviously intentional without the extra blank lines.

Sure "if(LIBSSL_SONAME)" would work too - I take it we prefer this construction?

tchaikov · 2016-08-01T16:08:20Z

I take it we prefer this construction?

yeah. i think it's more concise.

@mdw-at-linuxbox ping

theanalyst · 2016-08-03T12:32:48Z

this should probably address http://tracker.ceph.com/issues/16581 as well

mdw-at-linuxbox · 2016-08-04T08:08:14Z

I've updated the branch wip-rgw-openssl-3. I've removed "Replace + with , in civetweb port string.". Whatever we do for that should be a separate pull request. I've revised the doc accordingly. I also spelled USE_OPENSSL right and removed DEFINED. Finally I rebased on the latest master. Passed my testing. Hopefully gitbuilder will still like it.

theanalyst · 2016-08-04T08:18:44Z

src/rgw/rgw_main.cc

+      if (portno <= 0 || portno > 65535) {
+	derr << "ERROR: port out of range " << port << dendl;
+	return -1;
+      }


what happens if we pass an ip:port combo which currently works, albeit setting SERVER_PORT to 0, here don't we end up not starting rgw at all ?

Well, actually, civetweb doesn't start up if you give it a bad port. It fails in a much more confusing way later on.

However, you are right, it's possible to pass an ip:port in. Or [ipv6]:port. And append one of [sr,]. And it's possible to pass in non-zero bits >65535 in portno and have them ignored. The ssl redir feature isn't useful to us currently, since we don't yet support listening for both ssl|non-ssl. And we'll never pass in a , because the config file parser eats those elsewhere. I don't mind breaking rgw for people who set high order bits in portno.

But being able to specify a specific ipaddr is useful, sigh.

This fixes a regression where USE_IPV6 is no longer defined. Formerly, this was set in a terribly confusing and weird way. The civetweb source was customized with a file "include/civetweb_conf.h" that defined USE_IPV6. This file isn't used or referenced anywhere in the civetweb source (or build). The ceph automake build has its own separate process to make civetweb.o, which doesn't use any part of the build logic in civetweb. Instead, it builds civetweb.o with its own rule passing in the compiler option "--include $(srcdir)/civetweb/include/civetweb_conf.h". This is both silly and confusing, plus it makes it harder to update the civetweb source. Instead of passing in --include X, we can just pass in -DUSE_IPV6. In cmake that gets set using COMPILE_DEFINITIONS. Cmake doesn't support doing that on a per-file basis, but it's perfectly happy doing that for an object library containing one object. This is not coincidentally just exactly how we're already building civetweb.c.o with cmake. Signed-off-by: Marcus Watts <mwatts@redhat.com>

This includes information on file format and configuration file syntax. Signed-off-by: Marcus Watts <mwatts@redhat.com>

If building with radosgw, always look for openssl library (even when building with nss). Then, use objdump to fetch SONAME from the copies of libssl and libcrypto that were found. When building civetweb; pass the library soname values in as the libraries to load with "dlopen". This is a problem that went away for a bit, but came back with some changes for http://tracker.ceph.com/issues/16535 Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1341775 Fixes: http://tracker.ceph.com/issues/11239 Signed-off-by: Marcus Watts <mwatts@redhat.com>

To do https: append an 's' to portno. The code that actually passes that to civetweb is elsewhere. But apparently we need to set "port" in the frontend here because logic elsewhere in radosgw needs to know on which port a connection was accepted, and there's no logic in civetweb to indicate that. Signed-off-by: Marcus Watts <mwatts@redhat.com>

mdw-at-linuxbox · 2016-08-09T05:30:15Z

I've force-pushed a new copy of this branch. Rebased on latest master, there are now 4 commits:
a43ebf3 set -DUSE_IPV6 (NEW)
fe08886 document ssl
4470877 load by soname (FIX TYPO)
a43ebf3 parse and accept portno (UPDATED).

yehudasa · 2016-08-09T05:40:58Z

src/rgw/rgw_main.cc

+static int parse_civetweb_portno(const string &port) {
+  char *portend;
+  const char *portstart = port.c_str();
+  const char *cp = strrchr(portstart, ':');


@mdw-at-linuxbox could just use port.find_last_of()

yehudasa · 2016-08-09T05:45:11Z

src/rgw/rgw_main.cc

+  int portno = strtol(cp, &portend, 10);
+  if (cp == portend) {
+    derr << "ERROR: cannot parse port " << port << dendl;
+    return -1;


return -EINVAL

also other occurances in this function

mdw-at-linuxbox · 2016-08-11T07:25:20Z

port=0s can't work - you already pointed out why. port=0 would result in a random port assignment. How is that useful?

yehudasa · 2016-08-23T15:08:32Z

@mdw-at-linuxbox see my comments on the last commit, other than that I think it's ok

djzort · 2016-09-14T11:22:21Z

this seems to have gotten stuck?

mdw-at-linuxbox · 2016-10-20T05:29:42Z

I've created a new PR that has just the soname fix and the documentation fix. That is now PR #11571.

liewegas · 2016-11-03T16:30:17Z

need rebase?

mdw-at-linuxbox · 2016-11-04T07:54:50Z

This PR is superseded by #11571 and #11776 so I'm closing this one.

cbodley added bug-fix rgw labels Jul 19, 2016

mdw-at-linuxbox assigned alimaredia and yehudasa Jul 20, 2016

mdw-at-linuxbox force-pushed the wip-rgw-openssl-3 branch from 1573596 to 8c573ee Compare July 20, 2016 21:55

yehudasa reviewed Jul 21, 2016
View reviewed changes

mdw-at-linuxbox force-pushed the wip-rgw-openssl-3 branch from 8c573ee to 8548d97 Compare August 4, 2016 08:00

theanalyst reviewed Aug 4, 2016
View reviewed changes

mdw-at-linuxbox added 2 commits August 9, 2016 01:17

Document that radosgw now supports SSL.

fe08886

This includes information on file format and configuration file syntax. Signed-off-by: Marcus Watts <mwatts@redhat.com>

mdw-at-linuxbox force-pushed the wip-rgw-openssl-3 branch from 8548d97 to 261b91c Compare August 9, 2016 05:20

mdw-at-linuxbox added 2 commits August 9, 2016 01:27

mdw-at-linuxbox force-pushed the wip-rgw-openssl-3 branch from 261b91c to 9244166 Compare August 9, 2016 05:28

yehudasa reviewed Aug 9, 2016
View reviewed changes

liewegas changed the title ~~Wip rgw openssl 3~~ rgw: openssl 3 Nov 3, 2016

mdw-at-linuxbox closed this Nov 4, 2016

mdw-at-linuxbox deleted the wip-rgw-openssl-3 branch November 4, 2016 07:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rgw: openssl 3 #10335

rgw: openssl 3 #10335

mdw-at-linuxbox commented Jul 18, 2016

djzort commented Jul 18, 2016

mdw-at-linuxbox commented Jul 20, 2016

alimaredia commented Jul 21, 2016

yehudasa Jul 21, 2016

mdw-at-linuxbox Jul 22, 2016

yehudasa Jul 22, 2016

theanalyst Aug 3, 2016 •

edited

smithfarm commented Jul 26, 2016

mdw-at-linuxbox commented Jul 26, 2016

tchaikov commented Aug 1, 2016

theanalyst commented Aug 3, 2016

mdw-at-linuxbox commented Aug 4, 2016

theanalyst Aug 4, 2016

mdw-at-linuxbox Aug 4, 2016

mdw-at-linuxbox commented Aug 9, 2016

yehudasa Aug 9, 2016

yehudasa Aug 9, 2016

yehudasa Aug 9, 2016

mdw-at-linuxbox commented Aug 11, 2016

yehudasa commented Aug 23, 2016

djzort commented Sep 14, 2016

mdw-at-linuxbox commented Oct 20, 2016 •

edited by tchaikov

liewegas commented Nov 3, 2016

mdw-at-linuxbox commented Nov 4, 2016

rgw: openssl 3 #10335

rgw: openssl 3 #10335

Conversation

mdw-at-linuxbox commented Jul 18, 2016

djzort commented Jul 18, 2016

mdw-at-linuxbox commented Jul 20, 2016

alimaredia commented Jul 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theanalyst Aug 3, 2016 • edited

Choose a reason for hiding this comment

smithfarm commented Jul 26, 2016

mdw-at-linuxbox commented Jul 26, 2016

tchaikov commented Aug 1, 2016

theanalyst commented Aug 3, 2016

mdw-at-linuxbox commented Aug 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdw-at-linuxbox commented Aug 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdw-at-linuxbox commented Aug 11, 2016

yehudasa commented Aug 23, 2016

djzort commented Sep 14, 2016

mdw-at-linuxbox commented Oct 20, 2016 • edited by tchaikov

liewegas commented Nov 3, 2016

mdw-at-linuxbox commented Nov 4, 2016

theanalyst Aug 3, 2016 •

edited

mdw-at-linuxbox commented Oct 20, 2016 •

edited by tchaikov