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

qpidd: hostname for config cmds #216

Merged
merged 1 commit into from
Oct 13, 2017
Merged

Conversation

timogoebel
Copy link
Member

@timogoebel timogoebel commented Sep 24, 2017

This PR allows configuring the hostname that is used to connect to qpidd while setting up queues etc. This is necessary when a developer sets interface to something else than lo.

@timogoebel timogoebel force-pushed the qpid_hostname branch 3 times, most recently from a6f89f8 to 6fc92a8 Compare September 24, 2017 19:54
@@ -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,
Copy link
Member

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.

@@ -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,
Copy link
Member

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

@timogoebel
Copy link
Member Author

@ekohl: Thanks. Done.

@ehelms
Copy link
Member

ehelms commented Oct 4, 2017

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.

@timogoebel
Copy link
Member Author

IIRC, we originally locked this to localhost due to security concerns.

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.

@stbenjam
Copy link
Member

stbenjam commented Oct 4, 2017

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.

@timogoebel
Copy link
Member Author

@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.
Do you guys think, we should extend the parameter description with a warning that this could be an issue? Or can we merge this without?

@stbenjam
Copy link
Member

stbenjam commented Oct 4, 2017

I'm not sure, but I guess since it's already under advanced that is probably enough.

Copy link
Member

@stbenjam stbenjam left a 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

Copy link
Member

@ehelms ehelms left a 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+

@ehelms ehelms merged commit 8555bda into theforeman:master Oct 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants