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

mgr: remove default cert; disable both restful and dashboard by default #15601

Merged
merged 9 commits into from Jun 22, 2017

Conversation

Projects
None yet
3 participants
@liewegas
Member

liewegas commented Jun 9, 2017

  • no auto-cert creation by package
  • both restful and dashboard lose the default addr, so they will not start if the admin hasn't explicitly set server_addr

I don't think we want to enable either of these by default. And once the admin has to type in or cut&paste a command to turn on restful, they can also paste another 3 lines to generate and insert a self-signed cert (if they choose).

@liewegas liewegas requested review from b-ranto and jcsp Jun 9, 2017

@liewegas liewegas added the mgr label Jun 9, 2017

@liewegas liewegas requested a review from dmick Jun 9, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented Jun 9, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented Jun 9, 2017

retest this please

@b-ranto

This comment has been minimized.

Contributor

b-ranto commented Jun 12, 2017

a couple of things:

  • won't this break the rados test suite now that the restful test case is part of it?
  • if we are going to expect admins to do most of this on their own can we make it part of the ~same interface and use something like ceph tell mgr restful_generate_cert/restful_set_cert or even a do-it-all ceph tell mgr restful_init
  • why are we removing the default server address? localhost does seem like a good default value
@liewegas

This comment has been minimized.

Member

liewegas commented Jun 12, 2017

Sigh, yeah, this will break the test. It needs to generate the cert (or we need to do that in ceph.py).

I like 'ceph tell mgr restful generate-cert'. Did we come to a conclusion about whether we can use the internal helper function to generate an anon cert? IIRC there was a question about whether the dependencies were present..

There is no auth on dashboard so we can't enable it by default. And the thinking with restful is that in order to use it properly the admin should make some conscious decision about the cert and bind address, which means there is some manual setup anyway and no need to auto-gen a cert that is probably wrong anyway. The 'generate-cert' command is nice because it's easy and an active step taken by the admin here.

@liewegas

This comment has been minimized.

Member

liewegas commented Jun 15, 2017

@b-ranto did we come to a conclusion about the implicitly python openssl dependency?

@b-ranto

This comment has been minimized.

Contributor

b-ranto commented Jun 15, 2017

@liewegas It should be ok to use it for certificate generation (both implicitly and directly). AFAIK, only the certificate verification part of the library was deprecated in favour of the standard ssl python library.

@liewegas

This comment has been minimized.

Member

liewegas commented Jun 15, 2017

@b-ranto in that case, what do you think of a 'ceph tell mgr restful create-self-signed-cert' command on top of these patches?

@b-ranto

This comment has been minimized.

Contributor

b-ranto commented Jun 15, 2017

@liewegas yep, something like that would make sense, we could use the create_cert function as removed by commit f59e9bf#diff-1f1c563ffb053d5d63ef3c18a00af2a9L346 for that (we can just remove the C, ST, L metadata to create a more generic certificate).

@liewegas

This comment has been minimized.

Member

liewegas commented Jun 15, 2017

@jcsp suggestions on how to make the module be able to restart itself?

@jcsp

This comment has been minimized.

Contributor

jcsp commented Jun 15, 2017

@liewegas module should block itself in serve() if it's waiting to be configured, e.g. with a python threading.event() object. If it needs to restart after starting that's (hopefully) something that web framework of choice should handle.

@b-ranto

This comment has been minimized.

Contributor

b-ranto commented Jun 15, 2017

@liewegas I am thinking we can make _serve method reloadable in serve method via something like this (not tested)

def serve(self):
  while True:
    try:
      self._serve()
      break
    except:
      self.log.error(str(traceback.format_exc()))

With that we should be able to reload the server by throwing an exception while we would still be able to finish cleanly on a regular shutdown.

EDIT: Not sure how this would play along with all the threads, etc though. This might require setting passthrough_errors=True in make_server for it to work.

@jcsp

This comment has been minimized.

Contributor

jcsp commented Jun 19, 2017

If you wanted to use an exception for restarting (I don't know if this web framework passes exceptions from workers up to the original caller?) then it should definitely be a specific RestartMePlease class, rather than restarting the server on any exception. If we restarted on any exception then something that e.g. couldn't bind a network port would sit there spinning and spamming the log with the same exception.

@jcsp

This comment has been minimized.

Contributor

jcsp commented Jun 19, 2017

I also don't know where the exception would come from if we were e.g. restarting the server because of a config change -- surely it wouldn't come from inside the _serve method?

@b-ranto

This comment has been minimized.

Contributor

b-ranto commented Jun 19, 2017

I was playing with it a bit and I was able to implement a restart method that is fairly safe in wip-commit b-ranto@2c8727f

The thing is it won't start the daemon if it was not running (or failed) before. That would require some more changes to the mechanism.

@b-ranto

This comment has been minimized.

Contributor

b-ranto commented Jun 19, 2017

This wip-commit should cover even the case when it failed to start b-ranto@d9fcc5f

The module keeps running in polling mode (once a second) until something tells it to try to start again. create-self-signed cert sends a signal to try to boot again. The shutdown method sends a signal to go down.

@jcsp

This comment has been minimized.

Contributor

jcsp commented Jun 19, 2017

I've added comments to b-ranto@d9fcc5f

@b-ranto

This comment has been minimized.

Contributor

b-ranto commented Jun 19, 2017

This should cover the comments b-ranto@81e05df

liewegas added some commits Jun 9, 2017

pybind/mgr/dashboard: remove default of 127.0.0.1
i.e., disabled until explicitly enabled by an admin.

Signed-off-by: Sage Weil <sage@redhat.com>
pybind/mgr/restful: remove default of 127.0.0.1
i.e., disabled until explicitly enabled by an admin

Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas

This comment has been minimized.

Member

liewegas commented Jun 20, 2017

@b-ranto ok, closer, but now since the cert create happens quickly i get a 'Address already in use' error. Need to add the OS_REUSEADDR option to the socket, but I'm having a hard time seeing how that should be done with make_server()....

@b-ranto

This comment has been minimized.

Contributor

b-ranto commented Jun 20, 2017

@liewegas I was hitting the same error until I added the self.server.socket.close() call.

Did you by any chance hit this when you hit an exception in the _serve method? That would explain it and in that case we should move the self.server.socket.close() call outside the try: statement in the serve method.

@liewegas

This comment has been minimized.

Member

liewegas commented Jun 21, 2017

didn't work :/

@b-ranto

This comment has been minimized.

Contributor

b-ranto commented Jun 21, 2017

Hmm, I am reading

https://engineering.imvu.com/2014/03/06/charming-python-how-to-actually-close-a-socket-by-calling-shutdown-before-calling-close/

and it looks like we should probably call server.socket.shutdown() instead of server.socket.close(). Any help when doing that?

EDIT: It looks like it should be something like server.socket.shutdown(socket.SHUT_RDWR). I'll update the patch myself.

@b-ranto

This comment has been minimized.

Contributor

b-ranto commented Jun 21, 2017

This seemed to work best for me b-ranto@c8b65ff

Can you retry?

@liewegas

This comment has been minimized.

Member

liewegas commented Jun 21, 2017

Okay, fixed. I worked around it by not trying to make_server when there is no cert. The root of the problem seems to be that make_server binds to the socket before setting up ssl, and then bails out. But at that point it's too late... it is either not tearing down the socket properly or we just have to use SO_REUSEADDR to rebind to it, and that isn't that easy.

Anyway, this is more graceful anyway!

@liewegas liewegas added the needs-qa label Jun 21, 2017

b-ranto and others added some commits Jun 20, 2017

restful: Introduce create-self-signed-cert command
This implements a create-self-signed-cert command for the ceph-mgr
restful interface.

It is designed so that it will try to restart the module once the cert
is created.

Signed-off-by: Boris Ranto <branto@redhat.com>
pybind/mgr/restful: do not start if no certificate is configured
This removes the default filename, by the way.  We also work around a
problem with make_server where it sets up the socket to listen before
checking for the cert, thereby making it problematic to rebind to the
port shortly thereafter when we do have a socket.  (SO_REUSEADDR would
be appropriate but there doesn't seem to be an easy way to make
make_server use it.)

Signed-off-by: Sage Weil <sage@redhat.com>
pybind/mgr/restful: remove default path names for the cert
I don't think these are useful.

Signed-off-by: Sage Weil <sage@redhat.com>

liewegas added some commits Jun 22, 2017

pybind/mgr/restful: add 'restful restart' command
Signed-off-by: Sage Weil <sage@redhat.com>
qa/suites/rados/rest/mgr-restful: configure cert
Signed-off-by: Sage Weil <sage@redhat.com>
qa/workunits/rest/test_mgr_rest_api: remove ruleset
Signed-off-by: Sage Weil <sage@redhat.com>

@liewegas liewegas merged commit 8d8f2c3 into ceph:master Jun 22, 2017

2 of 4 checks passed

arm64 make check arm64 make check started
Details
make check running make check
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details

@liewegas liewegas deleted the liewegas:wip-no-restful-cert branch Jun 22, 2017

xiexingguo added a commit to xiexingguo/ceph that referenced this pull request Jun 22, 2017

src/vstart.sh: start mgr correctly
This fix the regression introduced by
ceph#15601
and brings ut back to normal.

Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>

xiexingguo added a commit to xiexingguo/ceph that referenced this pull request Jun 22, 2017

src/vstart.sh: start mgr correctly
This fixes the regression introduced by
ceph#15601
and brings ut back to normal.

Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment