Skip to content
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/dashboard: RGW proxy can't handle self-signed SSL certificates #22735

Merged
merged 1 commit into from Aug 6, 2018

Conversation

votdev
Copy link
Member

@votdev votdev commented Jun 27, 2018

Fixes https://tracker.ceph.com/issues/24677

The SSL certificate verification can be disabled via ceph dashboard set-rgw-api-no-ssl-verify True and the current setting can be checked via ceph dashboard get-rgw-api-no-ssl-verify.

Signed-off-by: Volker Theile vtheile@suse.com

@@ -175,7 +175,8 @@ def __init__(self, # pylint: disable-msg=R0913
self.admin_path = admin_path

s3auth = S3Auth(access_key, secret_key, service_url=self.service_url)
super(RgwClient, self).__init__(host, port, 'RGW', ssl, s3auth)
# Disable SSL verification to support self-signed certificates.
super(RgwClient, self).__init__(host, port, 'RGW', ssl, s3auth, ssl_verify=False)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disabling ssl_verify per default is not a good idea. An idea would be to make it configurable in a setting.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sebastian-philipp I agree, this should be configurable in some form.

@sebastian-philipp
Copy link
Contributor

@votdev votdev force-pushed the bug_24677 branch 4 times, most recently from 9417aa3 to d6943c1 Compare June 28, 2018 10:19
@votdev
Copy link
Member Author

votdev commented Jun 28, 2018

@sebastian-philipp The SSL verification can be configured now.

@@ -236,6 +236,11 @@ exist and you may find yourself in the situation that you have to use them::
$ ceph dashboard set-rgw-api-admin-resource <admin_resource>
$ ceph dashboard set-rgw-api-user-id <user_id>

If you are using a self-signed certificate in your Object Gateway setup, then
you should disable SSL verification in the dashboard::
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/SSL verification/certificate verification/ maybe?
And maybe explain why it should be disabled? To avoid errors?

LenzGr
LenzGr previously requested changes Jun 28, 2018
you should disable certificate verification in the dashboard to avoid refused
connections::

$ ceph dashboard set-rgw-api-no-ssl-verify True
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to rename this to rgw-api-cert-verification. You're verifying the certificate, not the SSL protocol per se :)

Also, I think we should avoid double negation to avoid confusion: instead of setting "no-ssl-verify" to "True", I suggest to use set the verification to "False" (disabled).

Copy link
Member Author

@votdev votdev Jun 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also prefer positive names for boolean variables and I think that it's a difference if a command line flag is called --no-ssl-verify which disables a features without having to provide a value than using a negated variable name and having to provide a value.

http://truelogic.org/wordpress/2009/02/27/how-not-to-name-a-boolean-variable/
https://stackoverflow.com/questions/1227998/naming-conventions-what-to-name-a-boolean-variable
http://docs.python-requests.org/en/master/user/advanced/#ssl-cert-verification

@votdev
Copy link
Member Author

votdev commented Jul 27, 2018

All comments have been adressed. Please re-review the PR.

@votdev votdev removed the DNM label Jul 30, 2018
Copy link
Contributor

@rjfd rjfd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@s0nea
Copy link
Member

s0nea commented Aug 3, 2018

Hey @votdev, I guess you didn't add src/seastar and src/rocksdb on purpose. Could you please remove it?

Fixes tracker.ceph.com/issues/24677

Signed-off-by: Volker Theile <vtheile@suse.com>
@votdev
Copy link
Member Author

votdev commented Aug 3, 2018

@s0nea Thx. Arghhh, seems to happen everytime when doing a rebase. This git submodules makes me crazy.

@votdev votdev dismissed LenzGr’s stale review August 6, 2018 08:54

The requested changes have been adressed.

@sebastian-philipp sebastian-philipp merged commit ba363a5 into ceph:master Aug 6, 2018
@votdev votdev deleted the bug_24677 branch August 6, 2018 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants