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

Fixes #31878: Turn on Qpid auth and set ACLs to the router certificat… #384

Merged
merged 1 commit into from Feb 19, 2021

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Feb 16, 2021

…es CN

Copy link
Contributor

@jturel jturel left a comment

Choose a reason for hiding this comment

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

ACK - tested working in conjunction w/ my Katello PR

acl allow katello_agent@QPID publish exchange name=qmf.default.direct
acl allow katello_agent@QPID access method name=create
# allow the actions needed by qpid_router_katello_agent
acl allow qpid_router_katello_agent@QPID create queue
Copy link
Member

Choose a reason for hiding this comment

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

It wasn't possible to use the same certificates with the right CN?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we use certificates with a CN of the FQDN for both katello -> qpid and qpid-dispatch -> qpid then we end up having to ensure the ACLs handle both ends of the connection and we potentially elevate qpid-dispatch. Further, when you do debugging you have a hard time deciphering which connection is which.

For example with this update:

 connection                   cproc      cpid   mech       auth                                                           connected  idle  msgIn  msgOut
=========================================================================================================================================================
 qpid.[::1]:5671-[::1]:35360  -                 EXTERNAL   qpid_router_katello_agent@QPID                                 6s         4s       0      0
 qpid.[::1]:5671-[::1]:35364  -                 EXTERNAL   pipe-katello-server-nightly-centos7.wareagle.example.com@QPID  2s         0s       0      0
 qpid.[::1]:5671-[::1]:35366  qpid-stat  26105  ANONYMOUS  anonymous@QPID                                                 0s         0s       1      0

Copy link
Member

Choose a reason for hiding this comment

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

I still think we should use a variable instead of hardcoding qpid_router_katello_agent. Even if we hardcode the value in Puppet code, it is useful to know that it's the common name on the cert. If you use $common_name, I think it's easier to understand.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well common_name is how we are using it in this instance. This could just as easily be a username how we previously were defining it. The point is more that if a user with this name connects, it needs to have these restricted ACLs. If we change this to common_name and switch back to username and password we are now having to re-update this code. The only reason we chose to update this from katello_agent to qpid_router_katello_agent was to draw the distinction that we expect the qpid router to connect and not katello-agent directly.

I think we should just leave this as is and do any potential clean up or specification when we tackle having qpid infra off by default because I think that will likely result in moving this qpid setup code closer to the dispatch code.

Copy link
Contributor

@jlsherrill jlsherrill left a comment

Choose a reason for hiding this comment

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

ACK'ing based on @jturel's approval and the need to unblock the pipeline

@ehelms ehelms merged commit d52a981 into theforeman:master Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants