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 #27932 - Add REX Cockpit support #760

Merged
merged 4 commits into from
Oct 24, 2019
Merged

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Sep 24, 2019

This only works with REX 1.8.3 and newer on RPMs.

Replaces #748. It takes a different approach by making a separate class rather than a parameter.

This only works with REX 1.8.3 and newer on RPMs.
@ares
Copy link
Member

ares commented Sep 30, 2019

@ekohl is there something I can help with? I'm pretty sure this will get decent testing after it's in :-)

@ekohl
Copy link
Member Author

ekohl commented Sep 30, 2019

@ares this will also need an installer migration to expose the class. @iNecas confirmed in the other PR that it should indeed be the CA that's exposed by Foreman so I've pushed a change. Initially forgot to git add the acceptance test. It may also need SELinux changes but that our CI can't test that nor is it a blocker to merging.

@ares
Copy link
Member

ares commented Oct 1, 2019

Sounds good, I will ask someone (@lzap 😉) to take a look at selinux when this is in. Right now, tests seems to be broken by last change. Once fixed, I'm happy to merge.

@ehelms
Copy link
Member

ehelms commented Oct 1, 2019

What exactly is this deploying? A new service? A set of plugin configurations?

@ekohl
Copy link
Member Author

ekohl commented Oct 2, 2019

There are files in https://github.com/theforeman/foreman_remote_execution/tree/master/extra/cockpit which are packaged as a -cockpit subpackage. This contains a service that runs on port 9999 that allows tunneling SSH over a websockets connection. It is running a private cockpit instance as well.

It does connect back to Foreman to verify a token. The CA file is needed to verify the HTTPS connection to it. A cleaner approach would probably be to use JWT to sign it so it can be verified without a connection.

It needs the cert and key to set up an authenticated connection to a Smart Proxy. I can't see whether it verifies that connection since it doesn't set up a CA to verify it. It could be made more secure by providing the client CA.

This all depends on the REX plugin being deployed so it requires that.

@lzap
Copy link
Member

lzap commented Oct 3, 2019

Sounds good, I will ask someone (@lzap wink) to take a look at selinux when this is in. Right now, tests seems to be broken by last change. Once fixed, I'm happy to merge.

If I understand this correctly then it's the cockpit service which connect to port 9999 and not Foreman. Thus it will probably need a change in Cockpit policy rather than ours.

@ares
Copy link
Member

ares commented Oct 4, 2019

@lzap the service that runs on 9999 is foreman specific, it's used for tunelling the connection over REX SSH connection. So I think our policy needs to be updated.

mode => '0644',
content => template('foreman/remote_execution_cockpit_session.yml.erb'),
require => Foreman::Plugin['remote_execution-cockpit'],
notify => Service['foreman-cockpit'],
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering a bit if this service name should be more specific. As I think I understand the comments, this is dependent entirely upon remote execution and is not a generic service for Foreman and cockpit. Something like foreman-rex-cockpit or foreman-remote-execution-cockpit.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ehelms
Copy link
Member

ehelms commented Oct 8, 2019

Do I understand correctly that the service:

  1. Spins up an instance of cockpit
  2. Provides websocket support for tunneling SSH

Are we using cockpit only to get the SSH -> websocket functionality as a convenience? Is there other functionality of the running cockpit instance being used?

@ehelms
Copy link
Member

ehelms commented Oct 8, 2019

Is a description of all the connections laid out anywhere I can look at?

@ares
Copy link
Member

ares commented Oct 8, 2019

@mvollmer mind to link any docs or provide answers to @ehelms, cc @iNecas

@iNecas
Copy link
Member

iNecas commented Oct 10, 2019

This thread should have most of the context https://community.theforeman.org/t/seamless-cockpit-and-foreman/9846/23,

This service is mainly focused on performing authentication, the tunneling is achieved with this combined with smart-proxy capabilities.

Deferring to @mvollmer for more details.

@ekohl
Copy link
Member Author

ekohl commented Oct 14, 2019

@ehelms do you want to block on this? Given most of the packaging changes have already been merged for some time.

@lzap
Copy link
Member

lzap commented Oct 16, 2019

SELinux related question - where/how do we distribute file named /etc/systemd/system/foreman-cockpit.service described in the README of https://github.com/mvollmer/foreman-cockpit/ ?

It simply spawns /usr/libexec/cockpit-ws --no-tls --address 127.0.0.1 --port 9999 which is a problem for SELinux - we need to create our own wrapper script which can be labelled with foreman_cockpit_bin_t to do the initial transition. Also a sidenote - the port seems to be hardcoded there, we probably want to extract it into an ENV file.

Another concern is about the default port (9999), I need to check (I am in a train currently) however if it is already taken by existing policy we want probably to pick a different one which can be assigned a port type.

@ekohl
Copy link
Member Author

ekohl commented Oct 16, 2019

https://github.com/theforeman/foreman_remote_execution/tree/master/extra/cockpit

You're right that Environment=PORT=9999 and Environment=ADDRESS=127.0.0.1 for that would be nice. theforeman/foreman_remote_execution#439 should do that.

@ehelms
Copy link
Member

ehelms commented Oct 21, 2019

No, I won't block on the naming. Just express a strong desire for things to be named such that they reflect as specific as possible their use case in order to avoid confusion by users and developers.

@ehelms
Copy link
Member

ehelms commented Oct 21, 2019

Does this new service need to be added to foreman-maintain to ensure it gets started/stopped/restarted when performing a full refresh of services? (e.g. if the certificates change)

@ares
Copy link
Member

ares commented Oct 24, 2019

What is the progress on this? Can we get it in and open tracking issue on foreman-maintain? Is there something blocking this PR? It's hard to verify SElinux work without installer nightlies that would install it.

@ekohl ekohl merged commit dc92995 into theforeman:master Oct 24, 2019
@ekohl ekohl deleted the rex-cockpit branch October 24, 2019 14:36
cegeka-jenkins pushed a commit to cegeka/puppet-foreman that referenced this pull request Feb 3, 2022
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

6 participants