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

Adding QDR basic auth to smoke tests #492

Closed
wants to merge 11 commits into from
Closed

Conversation

csibbitt
Copy link
Collaborator

@csibbitt csibbitt commented Sep 28, 2023

  • Adds the user/pass to collectd and ceilometer config files
  • Builds new containers with cyrus-sasl-plain-2.1.27-5.el8.x86_64.rpm dependency
    • This is a temporary workaround until we can get that package into the tripleo containers

NOTE: This doesn't work

@csibbitt csibbitt added the do-not-merge Code is not ready to be merged label Sep 28, 2023
@csibbitt
Copy link
Collaborator Author

(Failed) TESTING

Collectd is failing to do the auth, but ceilometer doesn't have any problem.

$ oc logs default-interconnect-7c77794bf7-n48d5 | grep -B 1 auth=PLAIN
2023-09-28 15:35:23.432046 +0000 SERVER (info) [C501] Accepted connection to 0.0.0.0:5671 from 10.128.1.25:37210                                              
2023-09-28 15:35:23.436812 +0000 ROUTER (info) [C501] Connection Opened: dir=in host=10.128.1.25:37210 vhost= encrypted=no auth=PLAIN user=guest@default-interconnect container_id=openstack.org/om/container/stf-smoketest-smoke1-w2td4/ceilometer_publish.py/8/a7ed62e6dfa7462a999d222aaa33b309 props={:process="ceilometer_publish.py", :pid=8, :node="stf-smoketest-smoke1-w2td4"}

Collectd shows this

$ oc logs stf-smoketest-smoke1-gzqqc | grep amqp1

[...]
[2023-09-28 15:38:42] amqp1 plugin: PN_TRANSPORT_CLOSED: amqp:unauthorized-access: SASL(-4): no mechanism available: No worthy mechs found (Authentication failed [mech=none])

and the matching message from the QDR

2023-09-28 14:51:08.416965 +0000 SERVER (info) [C30] Accepted connection to 0.0.0.0:5671 from 10.128.1.2:60622
2023-09-28 14:51:08.417119 +0000 SERVER (info) [C30] Connection from 10.128.1.2:60622 (to 0.0.0.0:5671) failed: amqp:connection:policy-error Client skipped authentication - forbidden  

The "mech=none" and "Client skipped authentication" suggest it's not even trying to plaintext auth, but it has at least engaged the sasl layer. Ceilometer used to show a similar message, but installing the cyrus-sasl-plain package fixed it.

$ oc exec -it stf-smoketest-smoke1-dcs46 -c smoketest-collectd -- rpm -q cyrus-sasl-plain
Defaulted container "smoketest-collectd" out of: smoketest-collectd, smoketest-ceilometer
cyrus-sasl-plain-2.1.27-5.el8.x86_64

Here is the collectd config

$ oc exec -it stf-smoketest-smoke1-p8k2f -- cat /tmp/collectd.conf
[...]
LoadPlugin cpu
LoadPlugin amqp1
<Plugin "amqp1">
  <Transport "name">
    Host "default-interconnect"
    Port "5671"
    User "guest@default-interconnect"
    Password "H3YGz8PmX-jah8lX3cWY"
    Address "collectd"
    <Instance "cloud1-telemetry">
        Format JSON
        PreSettle false
    </Instance>
    <Instance "cloud1-notify">
        Format JSON
        PreSettle false
        Notify true
    </Instance>
  </Transport>
</Plugin>
[...]

The password appears to match the secret AND the file in the QDR container

$ oc get secret default-interconnect-users -ogo-template='{{ .data.guest | base64decode }}'
H3YGz8PmX-jah8lX3cWY

$ oc exec -it default-interconnect-7c77794bf7-n48d5 -- db_dump -p /tmp/qdrouterd.sasldb
VERSION=3
format=print
type=hash
db_pagesize=4096
HEADER=END
 guest\00default-interconnect\00userPassword
 H3YGz8PmX-jah8lX3cWY
DATA=END

Here is the code responsible for enabling auth: https://github.com/collectd/collectd/blob/ef1e157de1a4f2cff10f6f902002066d0998232c/src/amqp1.c#L265

And here is what the API docs have to say about it: https://qpid.apache.org/releases/qpid-proton-0.39.0/proton/c/api/group__connection.html#gafb84dd2ef7551ad864be08cb31010d19

Likely cause

There is a hint in the docs associated with pn_sasl_set_allow_insecure_mechs() "By default the SASL layer is configured not to allow mechanisms that disclose the clear text of the password over an unencrypted AMQP connection." https://qpid.apache.org/releases/qpid-proton-0.39.0/proton/c/api/group__sasl.html#gaf472325bc055bb18a5a6f5ca03eda315

If we look all the way back up at the QDR connection, we can see it says encrypted=no
(and also Accepted connection to 0.0.0.0:5671)

Here is the QDR config for the port 5671 listener:

$ oc exec -it default-interconnect-7c77794bf7-n48d5 -- grep -C 4 'port: 5671' /opt/interconnect/etc/qdrouterd.conf

listener {
    role: edge
    host: 0.0.0.0
    port: 5671
    saslMechanisms: PLAIN
    authenticatePeer: true
    sslProfile: openstack
}

and the associated sslProfile

$ oc exec -it default-interconnect-7c77794bf7-n48d5 -- grep -A 5 'sslProfile ' /opt/interconnect/etc/qdrouterd.conf
sslProfile {
   name: openstack
   certFile: /etc/qpid-dispatch-certs/openstack/default-interconnect-openstack-credentials/tls.crt
   privateKeyFile: /etc/qpid-dispatch-certs/openstack/default-interconnect-openstack-credentials/tls.key
   caCertFile: /etc/qpid-dispatch-certs/openstack/default-interconnect-openstack-ca/tls.crt
}

So AFAICT, despite the fact that the listener has an SSL profile, and that we use those certs in the OSP -> STF connection, both ceilometer and collectd are connecting without SSL enabled. My hunch is that the python proton library doesn't care and does the auth anyways, but the C proton library is more strict. It may be that a call to pn_transport_require_encryption() would be required in the amqp1 plugin in order to get this to work.

I tried to enable SSL on the ceilometer link by using driver=amqps (instead of "amqp") but that didn't work: `oslo_messaging.transport.DriverLoadFailure: Failed to load transport driver "amqps": No 'oslo.messaging.drivers' driver found, looking for 'amqps'

csibbitt added a commit that referenced this pull request Sep 28, 2023
Comment on lines +70 to +72
oc delete is openstack-collectd:latest
oc delete buildconfig openstack-ceilometer-notification
oc delete is openstack-ceilometer-notification
Copy link
Member

Choose a reason for hiding this comment

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

One of the oc delete is has a tag and the other does not. Missed tag, or superfluous tag?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The first one is probably superfluous/wrong

Comment on lines +74 to +75
oc new-build -D $'FROM quay.io/tripleomaster/openstack-collectd:current-tripleo\nUSER 0\nRUN rpm -i http://mirror.centos.org/centos/8-stream/BaseOS/x86_64/os/Packages/cyrus-sasl-plain-2.1.27-5.el8.x86_64.rpm'
oc new-build -D $'FROM quay.io/tripleomaster/openstack-ceilometer-notification:current-tripleo\nUSER 0\nRUN rpm -i http://mirror.centos.org/centos/8-stream/BaseOS/x86_64/os/Packages/cyrus-sasl-plain-2.1.27-5.el8.x86_64.rpm'
Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure this is fine in downstream testing as well... does CVP run this? Pretty sure the lack of pulling is simply a build system thing, so shouldn't be an issue. Just thinking out loud for possible gotchas.

Base automatically changed from csibbitt/STF-1502/qdr_auth to master September 29, 2023 12:45
leifmadsen added a commit that referenced this pull request Sep 29, 2023
* Initial changes for QDR basicAuth

* Update roles/servicetelemetry/tasks/pre.yml

Co-authored-by: Leif Madsen <lmadsen@redhat.com>

* correct API version on secret

* Touchups from fresh environment test

* swap ansible_date_time for a filter that doesnt required facts

...and adheres to the rules for label text

* Update CSV

* Disable qdr auth in smoketests

See: #492

---------

Co-authored-by: Leif Madsen <lmadsen@redhat.com>
@csibbitt
Copy link
Collaborator Author

This came up again today and I want to suggest a pretty easy solution - add a second QDR into the test so that it can be set up the same way the OSP QDR is (with tls+auth being use between the QDRs instead of from the collectors)

@csibbitt csibbitt closed this Nov 8, 2023
@leifmadsen
Copy link
Member

Can the branch associated with this closed issue be deleted?

@csibbitt csibbitt deleted the qdr_auth_in_smoketest branch December 4, 2023 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge Code is not ready to be merged
Development

Successfully merging this pull request may close these issues.

2 participants