rgw/notification: allow sending bucket notifications secrets in cleartext #43436

yuvalif · 2021-10-06T17:31:10Z

rgw_allow_secrets_in_cleartext conf parameter was added for that

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

Signed-off-by: Yuval Lifshitz ylifshit@redhat.com

Testing

run vstart cluster:

MON=1 OSD=1 MDS=0 MGR=0 RGW=1 ../src/vstart.sh -n -d

behavior should not change by default, so the following command should fail:

aws --endpoint-url http://localhost:8000 sns create-topic --name=fishtopic2 --attributes='{"push-endpoint": "amqp://hello:world@localhost:10900", "amqp-exchange": "ex1"}'

however, after setting the parameter to "true":

bin/ceph -c ceph.conf config set client.rgw.8000 rgw_allow_notification_secrets_in_cleartext true

topic creation with user and password over non-secure connection should be ok:

# aws --endpoint-url http://localhost:8000 sns create-topic --name=fishtopic2 --attributes='{"push-endpoint": "amqp://hello:world@localhost:10900", "amqp-exchange": "ex1"}'
{
    "TopicArn": "arn:aws:sns:default::fishtopic2"
}

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox

mattbenjamin · 2021-10-06T17:50:52Z

src/rgw/rgw_rest_pubsub_common.cc

@@ -24,7 +31,7 @@ bool validate_and_update_endpoint_secret(rgw_pubsub_sub_dest& dest, CephContext
  ceph_assert(user.empty() == password.empty());
  if (!user.empty()) {
      dest.stored_secret = true;
-      if (!rgw_transport_is_secure(cct, env)) {
+      if (!verify_security(cct, env)) {


should still indicate transport security, non?

verify_security() calls rgw_transport_is_secure() if the flag is not set.
if the flag is set there is no need to check for transport security, because we allow for user/password on non-secure transport

there are many aspects of security this won't disable, e.g., secure authentication

this is not disabling anything. only thing it allows is sending "secrets" (e.g. user/password) on non-secure transport

maybe i should name this function differently?

that's all I was saying

cbodley · 2021-10-06T19:25:49Z

this is useful fro the case that the RGW is behind a load balancer, when SSL is between client and balancer, and HTTP is used between balancer and RGW

rgw_transport_is_secure() has built-in support for this via the rgw_trust_forwarded_https config variable, can you try with that enabled?

cbodley · 2021-10-06T19:27:54Z

see #24700 for some more background on rgw_trust_forwarded_https

yuvalif · 2021-10-07T07:10:06Z

this is useful fro the case that the RGW is behind a load balancer, when SSL is between client and balancer, and HTTP is used between balancer and RGW

rgw_transport_is_secure() has built-in support for this via the rgw_trust_forwarded_https config variable, can you try with that enabled?

i don't think that would work. in rgw_transport_is_secure():

if (!cct->_conf->rgw_trust_forwarded_https) {                                                                                                                                                                                             
  return false;
}

so, if the conf parameter is set to "true" we won't enter the "if" statement.
if the conf parameter is set to "false" we would return "false" - marking the transport as insecure (which is not what we want)

cbodley · 2021-10-07T12:59:22Z

so, if the conf parameter is set to "true" we won't enter the "if" statement.

which if statement? setting it to true makes rgw_transport_is_secure() check whether we got a secure Forwarded or X-Forwarded-Proto header from the load balancer, and if we did it considers the transport secure. that's what you want right?

yuvalif · 2021-10-07T13:02:10Z

so, if the conf parameter is set to "true" we won't enter the "if" statement.

which if statement? setting it to true makes rgw_transport_is_secure() check whether we got a secure Forwarded or X-Forwarded-Proto header from the load balancer, and if we did it considers the transport secure. that's what you want right?

this is done right :-) but it does not help in the case of this PR
in this PR, we want to say that a transport is secure, even if it isn't, for the only purpose of allowing user/password to be sent over non-secure transport

cbodley · 2021-10-07T13:11:00Z

ok i was going by what the tracker issue said. but do we really want to be insecure here? i still think it should be a requirement

yuvalif · 2021-10-07T13:17:51Z

ok i was going by what the tracker issue said. but do we really want to be insecure here? i still think it should be a requirement

this is why it is only disabled by a conf parameter (which is "false" by default).
this "backdoor" was requested by an upstream users that didn't want to add TLS to the RGW only for the purpose of AMQP integration.
I ran into a similar issue trying to configure bucket notifications with AMQP in Rook. The AMQP k8s operator force you to use the non-default user/password (which is not the case in out teuthology tests), and the Rook environment makes it hard to setup TLS for the RGW.

cbodley · 2021-10-07T13:26:23Z

the Rook environment makes it hard to setup TLS for the RGW

i had assumed that kubernetes made this easy with its secrets; is there something we could improve on here? cc @travisn

i really don't think "security is hard" is a good reason not to do it. it'll also require some effort for users to change this config variable to open the back door, but i'd much rather see that effort spent on enabling TLS

cbodley · 2021-10-07T13:28:07Z

this is useful fro the case that the RGW is behind a load balancer, when SSL is between client and balancer, and HTTP is used between balancer and RGW

what was the context for this tracker issue? is this proxy configuration common in rook?

yuvalif · 2021-10-07T13:37:42Z

the Rook environment makes it hard to setup TLS for the RGW

i had assumed that kubernetes made this easy with its secrets; is there something we could improve on here? cc @travisn

i really don't think "security is hard" is a good reason not to do it. it'll also require some effort for users to change this config variable to open the back door, but i'd much rather see that effort spent on enabling TLS

its is not exactly the case of "security is hard". without this fix, in an environment where RGW do not use TLS, we are forcing the use of TLS in RGW because we want to send notifications to AMQP.
this would be a major change in most environment (k8s or not). in this PR, I want to allow for such integration (with AMQP) without forcing the RGW to be secure

yuvalif · 2021-10-07T16:08:16Z

as discussed in the standup, adding a "backdoor" is usually a bad idea :-)
hence, this PR will be put on hold until we figure out the limitations we have in using secure RGW in Rook.

yuvalif · 2021-10-07T16:22:54Z

teuthology run: http://pulpito.front.sepia.ceph.com/yuvalif-2021-10-07_14:56:45-rgw:notifications-wip-yuval-fix-50611-distro-basic-smithi/

stale · 2022-01-09T00:27:14Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

yuvalif · 2022-01-12T13:50:24Z

the reason TLS was not working properly in rook was just a bug (being fixed here: rook/rook#9565)
once the rook issue is fixed, there is no reason to send secrets in cleartext

yuvalif · 2022-07-20T15:15:46Z

going to reopen this, as this was specifically requested by users.
there are both kafka and amqp deployments that want to use user/password but does not want to use TLS (usually to avoid self signed certificates issues).
this is even more problematic in kafka, when our version of librdkafka does not support the "verify_sll=false" option

github-actions · 2022-07-20T15:16:08Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

cbodley · 2022-07-25T16:12:02Z

src/common/options/rgw.yaml.in

+- name: rgw_allow_secrets_in_cleartext
+  type: bool
+  level: advanced
+  desc: Allows sending secrets (e.g. passwords) over non encrypted HTTP messages


please use an option name that's specific to pubsub or notifications

in the description, please add some strong wording that this is a security vulnerability and should never be used in production

this is related to rgw_trust_forwarded_https, so a see_also: would be helpful

@cbodley please see: https://github.com/ceph/ceph/pull/43436/files#diff-32e4985985c3a5adaddfe7a70c31a1112eb5887cd1149a967dac5bc303487903R3566-R3569

let me know if this is enough.

cbodley · 2022-07-25T16:14:02Z

src/rgw/rgw_rest_pubsub_common.cc

+bool verify_transport_security(CephContext *cct, const RGWEnv& env) {
+  if (g_conf().get_val<bool>("rgw_allow_secrets_in_cleartext")) {
+    return true;
+  }
+  return rgw_transport_is_secure(cct, env);


can we please add a scary warning message at log level 0 whenever this override is triggered?

added a warning here: https://github.com/ceph/ceph/pull/43436/files#diff-7dd5f6f1e1de636d2699c66e02f38ca4e255a48f14304f8ad54b6c68e9248e94R15

yuvalif · 2022-07-26T07:13:14Z

teuthology passing after the latest changes: http://pulpito.front.sepia.ceph.com/yuvalif-2022-07-26_04:07:30-rgw:notifications-wip-yuval-fix-50611-distro-default-smithi/

yuvalif · 2022-08-04T16:00:12Z

@cbodley can you please review?

cbodley · 2022-08-11T15:04:00Z

@yuvalif i'm still against adding an intentional security hole here. if we know that we're leaking credentials, but it's okay because we're on a private network, then why do we need password authentication for the message broker?

would it be an acceptable workaround to configure the notification endpoints without passwords instead?

yuvalif · 2022-08-11T15:15:14Z

@yuvalif i'm still against adding an intentional security hole here. if we know that we're leaking credentials, but it's okay because we're on a private network, then why do we need password authentication for the message broker?

would it be an acceptable workaround to configure the notification endpoints without passwords instead?

not really. the configuration of the brokers is often managed differently than ceph.
in addition, kafka allows for "sasl plaintext" mode - meaning that it gets the user/password over non-encrypted connection:
https://docs.confluent.io/platform/current/kafka/authentication_sasl/authentication_sasl_plain.html

cbodley · 2022-08-11T15:25:11Z

i still don't understand what use case we're targeting here

if this is just for demos/testing, you can set up the notification endpoint without passwords

if this is for production use, then we should enforce real security. let's encrypt is an easy alternative to self-signed certificates. a google search for "kubernetes let's encrypt" comes up with a bunch of hits

yuvalif · 2022-08-11T15:29:14Z

i still don't understand what use case we're targeting here

if this is just for demos/testing, you can set up the notification endpoint without passwords

if this is for production use, then we should enforce real security. let's encrypt is an easy alternative to self-signed certificates. a google search for "kubernetes let's encrypt" comes up with a bunch of hits

these are requirements from users.
if the kafka system allows that (in "sasl plaintext" mode) it is probably ok for us to allow that as well

cbodley

please add some strong wording that this is a security vulnerability and should never be used in production

i don't think the language was strong enough in this regard

src/rgw/rgw_rest_pubsub_common.cc

cbodley · 2022-08-11T14:49:12Z

src/common/options/rgw.yaml.in

+  type: bool
+  level: advanced
+  desc: Allows sending secrets (e.g. passwords) over non encrypted HTTP messages.
+  long_desc: When bucket notification ednpoints require secrets (e.g. passwords),


cbodley · 2022-08-11T14:49:22Z

src/common/options/rgw.yaml.in

+  level: advanced
+  desc: Allows sending secrets (e.g. passwords) over non encrypted HTTP messages.
+  long_desc: When bucket notification ednpoints require secrets (e.g. passwords),
+    we allow the topic creation only over encrypted HTTP messages. 


cbodley · 2022-08-11T14:53:16Z

src/common/options/rgw.yaml.in

+  long_desc: When bucket notification ednpoints require secrets (e.g. passwords),
+    we allow the topic creation only over encrypted HTTP messages. 
+    This parameter can be set to "true" to bypass this check.
+    Use this only if secure connection cannot be established with the radosgw,


please clarify what it means when security 'cannot be established'

cbodley · 2022-08-11T15:00:15Z

src/common/options/rgw.yaml.in

+    these messages could be intercepted and or else malicious users will be able to
+    access the notification endpoints, to write and read messages from them.


Otherwise, this will leak the credentials of your message broker and compromise its security.

yuvalif · 2022-08-14T13:59:05Z

jenkins test make check

cbodley · 2022-08-15T17:09:35Z

src/common/options/rgw.yaml.in

+  type: bool
+  level: advanced
+  desc: Allows sending secrets (e.g. passwords) over non encrypted HTTP messages.
+  long_desc: When bucket notification ednpoint require secrets (e.g. passwords),


cbodley · 2022-08-15T17:10:27Z

src/common/options/rgw.yaml.in

+    leak the credentials of your message broker and compromise its security.
+  default: false
+  services:
+  - rgw


see_also: - rgw_trust_forwarded_https

cbodley · 2022-08-15T17:15:21Z

src/common/options/rgw.yaml.in

+    Use this only if secure connection cannot be established with the radosgw
+    (e.g. a proxy in front of radosgw is used for ssl termination), and if the 
+    network through which these requests are sent is trusted. Otherwise, this will 


rgw_trust_forwarded_https already covers the case where a proxy is terminating ssl. how about this wording?

Use this only if radosgw is on a trusted private network, and the message broker cannot be configured without password authentication.

yuvalif · 2022-08-15T17:43:37Z

jenkins test make check

yuvalif · 2022-08-21T11:45:53Z

jenkins test make check

yuvalif · 2022-08-21T11:46:13Z

jenkins test api

…text rgw_allow_notification_secrets_in_cleartext conf parameter was added for that Fixes: https://tracker.ceph.com/issues/50611 Signed-off-by: Yuval Lifshitz <ylifshit@redhat.com>

yuvalif · 2022-08-24T18:56:44Z

RGW crash seen here: http://qa-proxy.ceph.com/teuthology/yuvalif-2022-08-23_09:52:18-rgw-wip-yuval-fix-50611-distro-default-smithi/6987728/teuthology.log

std::__throw_bad_variant_access(bool)

seems like this is happening due to this line: https://github.com/ceph/ceph/pull/43436/files#diff-7dd5f6f1e1de636d2699c66e02f38ca4e255a48f14304f8ad54b6c68e9248e94R15
when compiled for Centos Streams 8.

this may also be what causes the API tests to fail: https://jenkins.ceph.com/job/ceph-api/43162/

yuvalif · 2022-08-25T04:33:52Z

RGW crash seen here: http://qa-proxy.ceph.com/teuthology/yuvalif-2022-08-23_09:52:18-rgw-wip-yuval-fix-50611-distro-default-smithi/6987728/teuthology.log
std::__throw_bad_variant_access(bool)
seems like this is happening due to this line: https://github.com/ceph/ceph/pull/43436/files#diff-7dd5f6f1e1de636d2699c66e02f38ca4e255a48f14304f8ad54b6c68e9248e94R15 when compiled for Centos Streams 8.

this may also be what causes the API tests to fail: https://jenkins.ceph.com/job/ceph-api/43162/

probably not related to this PR. same crashes are also seen here: #47507
(see: http://pulpito.front.sepia.ceph.com/ozeneva-2022-08-22_06:41:17-rgw-wip-omri-fix-tracer-provider-distro-basic-smithi/)

tracker already exists: https://tracker.ceph.com/issues/57195

yuvalif · 2022-08-25T14:21:57Z

jenkins test api

yuvalif · 2022-08-25T16:34:20Z

teuthology: http://pulpito.front.sepia.ceph.com/yuvalif-2022-08-23_09:52:18-rgw-wip-yuval-fix-50611-distro-default-smithi/

"Cannot create secret" - know issue
"[WRN] Health check failed" - known issue
tempest failure - known issue
multisite failure - known issues
"std::__throw_bad_variant_access(bool)" - known issue

cbodley · 2022-08-25T17:11:36Z

thanks @yuvalif, sorry i was a pain about this one! i'm happy with the result here, nice work 👍

yuvalif requested a review from cbodley October 6, 2021 17:31

github-actions bot added common rgw labels Oct 6, 2021

mattbenjamin reviewed Oct 6, 2021

View reviewed changes

yuvalif force-pushed the wip-yuval-fix-50611 branch from e37b695 to bb13593 Compare October 6, 2021 18:38

yuvalif added the DNM label Oct 7, 2021

yuvalif requested a review from mdw-at-linuxbox October 18, 2021 16:34

stale bot added the stale label Jan 9, 2022

stale bot removed the stale label Jan 12, 2022

yuvalif closed this Jan 12, 2022

yuvalif reopened this Jul 20, 2022

github-actions bot added the needs-rebase label Jul 20, 2022

djgalloway changed the base branch from master to main July 21, 2022 00:00

yuvalif force-pushed the wip-yuval-fix-50611 branch from bb13593 to 6a5d175 Compare July 21, 2022 09:02

yuvalif added the needs-review label Jul 21, 2022

cbodley reviewed Jul 25, 2022

View reviewed changes

yuvalif force-pushed the wip-yuval-fix-50611 branch from 6a5d175 to 3e46144 Compare July 25, 2022 19:16

yuvalif removed the needs-review label Jul 26, 2022

yuvalif requested a review from cbodley July 27, 2022 18:41

cbodley reviewed Aug 11, 2022

View reviewed changes

yuvalif force-pushed the wip-yuval-fix-50611 branch from 3e46144 to 7407f25 Compare August 14, 2022 09:47

cbodley reviewed Aug 15, 2022

View reviewed changes

yuvalif force-pushed the wip-yuval-fix-50611 branch from 7407f25 to 63b3a24 Compare August 15, 2022 17:32

cbodley approved these changes Aug 15, 2022

View reviewed changes

yuvalif force-pushed the wip-yuval-fix-50611 branch from 63b3a24 to aba13d8 Compare August 16, 2022 12:44

rgw/notification: allow sending bucket notifications secrets in clear…

b064c2a

…text rgw_allow_notification_secrets_in_cleartext conf parameter was added for that Fixes: https://tracker.ceph.com/issues/50611 Signed-off-by: Yuval Lifshitz <ylifshit@redhat.com>

yuvalif force-pushed the wip-yuval-fix-50611 branch from aba13d8 to b064c2a Compare August 22, 2022 16:44

yuvalif merged commit 1148464 into ceph:main Aug 25, 2022

yuvalif mentioned this pull request Oct 12, 2022

src/rgw: SASL mechanism implementation #48181

Merged

10 tasks

		these messages could be intercepted and or else malicious users will be able to
		access the notification endpoints, to write and read messages from them.

rgw/notification: allow sending bucket notifications secrets in cleartext #43436

rgw/notification: allow sending bucket notifications secrets in cleartext #43436

Conversation

yuvalif commented Oct 6, 2021 • edited

Testing

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

cbodley commented Oct 6, 2021

cbodley commented Oct 6, 2021

yuvalif commented Oct 7, 2021 • edited

cbodley commented Oct 7, 2021

yuvalif commented Oct 7, 2021

cbodley commented Oct 7, 2021

yuvalif commented Oct 7, 2021 • edited

cbodley commented Oct 7, 2021 • edited

cbodley commented Oct 7, 2021

yuvalif commented Oct 7, 2021

yuvalif commented Oct 7, 2021

yuvalif commented Oct 7, 2021

stale bot commented Jan 9, 2022

yuvalif commented Jan 12, 2022

yuvalif commented Jul 20, 2022

github-actions bot commented Jul 20, 2022

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

yuvalif commented Jul 26, 2022

yuvalif commented Aug 4, 2022

cbodley commented Aug 11, 2022

yuvalif commented Aug 11, 2022

cbodley commented Aug 11, 2022

yuvalif commented Aug 11, 2022

cbodley 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

yuvalif commented Aug 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuvalif commented Aug 15, 2022

yuvalif commented Aug 21, 2022

yuvalif commented Aug 21, 2022

yuvalif commented Aug 24, 2022 • edited

yuvalif commented Aug 25, 2022 • edited

yuvalif commented Aug 25, 2022

yuvalif commented Aug 25, 2022

cbodley commented Aug 25, 2022

yuvalif commented Oct 6, 2021 •

edited

yuvalif commented Oct 7, 2021 •

edited

yuvalif commented Oct 7, 2021 •

edited

cbodley commented Oct 7, 2021 •

edited

yuvalif commented Aug 24, 2022 •

edited

yuvalif commented Aug 25, 2022 •

edited