-
Notifications
You must be signed in to change notification settings - Fork 48
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
qpidd: hostname for config cmds #216
Conversation
a6f89f8
to
6fc92a8
Compare
manifests/init.pp
Outdated
@@ -79,6 +83,8 @@ | |||
|
|||
String $post_sync_token = $::katello::params::post_sync_token, | |||
Integer[0, 1000] $qpid_wcache_page_size = $::katello::params::qpid_wcache_page_size, | |||
Optional[String] $qpid_interface = $::katello::params::qpid_interface, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and qpid_hostname
can just be a String
since we have a default.
manifests/qpid.pp
Outdated
@@ -4,7 +4,8 @@ | |||
String $candlepin_event_queue = $::katello::candlepin_event_queue, | |||
String $candlepin_qpid_exchange = $::katello::candlepin_qpid_exchange, | |||
Integer[0, 5000] $wcache_page_size = $::katello::qpid_wcache_page_size, | |||
String $interface = 'lo', | |||
String $interface = $::katello::qpid_interface, | |||
Optional[String] $hostname = $::katello::qpid_hostname, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can also be a String
5da1a67
to
c819d29
Compare
@ekohl: Thanks. Done. |
IIRC, we originally locked this to localhost due to security concerns. I think those have since been fixed, but I would need to dig back in the past. Perhaps @stbenjam remembers. |
I think, this should not be a problem at all. The parameters default to localhost (which adds just another layer of security that relies heavily of the security of your host) and are listed as "advanced parameters". The main security should come through the TLS connection. I think, there is no need to sorry. |
Yea, in general we don't want to expose the broker to the outside world. Content hosts have valid client SSL certificates and can connect to the broker (which is fine, they used to do that before we had the dispatch router), but they can also see the pulp queues. Theoretically, if one found a bug in pulp it could be exploited by a compromised content host injecting a message into the queue. I don't remember if there was ever a specific proof of concept of that, you'd have to ask the pulp team. The dispatch router is more locked down as it only allows communication in certain directions, it's not possible for a client to inject a message into the various queues used by pulp. If qpidd is going to run on a different host than the Katello, ideally we'd need to allow access only to the Katello box by iptables. |
@stbenjam: Thanks for the clarification. That does make sense. For our use case, the connections are strictly limited by our SDN solution. So we're covered there. |
I'm not sure, but I guess since it's already under advanced that is probably enough. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed with Pulp that this is no longer an issue with version from 2.10+
This PR allows configuring the hostname that is used to connect to
qpidd
while setting up queues etc. This is necessary when a developer setsinterface
to something else thanlo
.