Fixes #11936 - Add support for kerberos auth #351

adamruzicka · 2017-06-01T13:08:12Z

No description provided.

theforeman-bot · 2017-06-01T13:08:14Z

@adamruzicka, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #11936: Support kerberized SSH as an alternative to keys is the correct one
move ticket #11936 from Foreman Remote Execution to the Installer project
or file a new ticket in the Installer project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

ekohl · 2017-06-01T13:18:36Z

manifests/plugin/remote_execution/ssh.pp

+      $scl_prefix = '' # lint:ignore:empty_string_assignment
+    }
+
+    foreman_proxy::plugin { 'net-ssh-krb':


I think you should use package since this isn't a plugin, it's a library. Another thing is that this will break on Debian where the package would most likely be named ruby-net-ssh-krb but isn't packaged.

Good point about package, will do. I'll have to look into the debian naming, debian packaging pr is here

It will be ruby-net-ssh-krb per Debian policies.

ekohl · 2017-06-01T13:20:06Z

manifests/plugin/remote_execution/ssh.pp

@@ -49,6 +52,18 @@
    template_path => 'foreman_proxy/plugin/remote_execution_ssh.yml.erb',
  }

+  if $ssh_kerberos_auth {
+    if $::osfamily == 'RedHat' and $::operatingsystem != 'Fedora' {


The smart proxy doesn't run in SCL.

This is an optional dependency for the smart_proxy_dynflow_core service which runs in SCL

theforeman-bot · 2017-06-01T13:20:17Z

@adamruzicka, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #11936: Support kerberized SSH as an alternative to keys is the correct one
move ticket #11936 from Foreman Remote Execution to the Installer project
or file a new ticket in the Installer project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

theforeman-bot · 2017-06-01T13:20:19Z

@adamruzicka, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #11936: Support kerberized SSH as an alternative to keys is the correct one
move ticket #11936 from Foreman Remote Execution to the Installer project
or file a new ticket in the Installer project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

theforeman-bot · 2017-06-01T13:24:45Z

@adamruzicka, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #11936: Support kerberized SSH as an alternative to keys is the correct one
move ticket #11936 from Foreman Remote Execution to the Installer project
or file a new ticket in the Installer project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

theforeman-bot · 2017-06-01T13:25:52Z

@adamruzicka, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #11936: Support kerberized SSH as an alternative to keys is the correct one
move ticket #11936 from Foreman Remote Execution to the Installer project
or file a new ticket in the Installer project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

theforeman-bot · 2017-06-01T13:26:55Z

@adamruzicka, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #11936: Support kerberized SSH as an alternative to keys is the correct one
move ticket #11936 from Foreman Remote Execution to the Installer project
or file a new ticket in the Installer project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

ekohl · 2017-06-05T20:57:35Z

manifests/plugin/remote_execution/ssh.pp

+    if $::osfamily == 'RedHat' {
+      $ruby_prefix = 'rubygem-'
+      if $::operatingsystem != 'Fedora' {
+        $ruby_prefix = 'tfm-' + $ruby_prefix


This isn't valid since variables can't re-declared.

ekohl · 2017-07-12T23:30:59Z

manifests/plugin/remote_execution/ssh.pp

@@ -49,6 +52,23 @@
    template_path => 'foreman_proxy/plugin/remote_execution_ssh.yml.erb',
  }

+  if $ssh_kerberos_auth {
+    if $::osfamily == 'RedHat' {
+      $rh_ruby_prefix = 'rubygem'


I wonder if this additional variable is actually needed. I'd just inline string below.

ekohl · 2017-07-12T23:32:09Z

manifests/plugin/remote_execution/ssh.pp

+      $ruby_prefix = 'ruby'
+    }
+
+    package { "${ruby_prefix}-net-ssh-krb":


Should this restart the dynflow_core service? Note that it's only used on EL and not Fedora or other distro's.

adamruzicka · 2017-08-14T13:27:17Z

@ekohl Sorry for taking so long, hopefully I addressed all your comments and added some tests

ekohl

One trivial thing, other than that 👍

ekohl · 2017-08-14T13:43:51Z

spec/classes/foreman_proxy__plugin__remote_execution__ssh_spec.rb

@@ -71,6 +78,10 @@

    it { should_not contain_exec('generate_ssh_key') }
    it { should_not contain_file('/root/.ssh') }
+
+    it 'should not install tfm-rubygem-net-ssh-krb package' do


nit: s/not // (the actual test is correct)

Right, bad copy-paste

ekohl · 2017-08-14T14:13:06Z

@mmoll mind re-reviewing this?

mmoll · 2017-08-14T20:21:44Z

manifests/plugin/remote_execution/ssh.pp

@@ -49,6 +52,22 @@
    template_path => 'foreman_proxy/plugin/remote_execution_ssh.yml.erb',
  }

+  if $ssh_kerberos_auth {
+    if $::osfamily == 'RedHat' {
+      if $::operatingsystem != 'Fedora' {


(untested) the following looks shorter and more readable to me:

unless $::osfamily == 'RedHat' { $ruby_prefix = 'ruby' } else { $ruby_prefix ? $::operatingsystem { 'Fedora' => 'rubygem', default => 'tfm-rubygem', } }

Forgot puppet has selector expressions, changed.

ekohl · 2017-08-15T08:33:38Z

manifests/plugin/remote_execution/ssh.pp

@@ -49,6 +52,22 @@
    template_path => 'foreman_proxy/plugin/remote_execution_ssh.yml.erb',
  }

+  if $ssh_kerberos_auth {
+    unless $::osfamily == 'RedHat' {


This might be a bit picky, but in my mind if x == y is easier to read and if both true and false are present I'd prefer to use if.

ekohl

I'll squash & merge when the tests are green.

ekohl · 2017-08-15T11:05:41Z

Thanks @adamruzicka!

adamruzicka · 2017-08-15T11:26:38Z

@ekohl, @mmoll Thanks for bearing with me

Fixes #11936 - Add support for kerberos auth

d2a83c0

ekohl requested changes Jun 1, 2017

View reviewed changes

Fixes #19918 - Allow enabling kerberos auth for REX

dd67ce1

ekohl reviewed Jun 5, 2017

View reviewed changes

Fixes #19918 - Correct puppet syntax

ad6a02a

adamruzicka mentioned this pull request Jul 12, 2017

Fixes #11936 - Allow authenticating with kerberos theforeman/foreman_remote_execution#250

Merged

Fixes #19918 - Add trailing comma

c9e8bf5

ekohl reviewed Jul 12, 2017

View reviewed changes

Fixes #19918 - Addressing comments, tests

f3298f7

ekohl requested changes Aug 14, 2017

View reviewed changes

Fixes #19918 - Fix test caption

ed57284

ekohl approved these changes Aug 14, 2017

View reviewed changes

mmoll approved these changes Aug 14, 2017

View reviewed changes

Fixes #19918 - Make it nicer

c9cf9f1

ekohl reviewed Aug 15, 2017

View reviewed changes

Fixes #19918 - Make it even nicer

0ac27ce

ekohl approved these changes Aug 15, 2017

View reviewed changes

ekohl merged commit a11b521 into theforeman:master Aug 15, 2017

adamruzicka deleted the kerberos branch August 15, 2017 11:26

juliantodt pushed a commit to juliantodt/puppet-foreman_proxy that referenced this pull request Jun 25, 2018

Fixes #11936 - Add support for REX kerberos auth (theforeman#351)

d6cde51

Fixes #11936 - Add support for kerberos auth #351

Fixes #11936 - Add support for kerberos auth #351

adamruzicka commented Jun 1, 2017

theforeman-bot commented Jun 1, 2017

ekohl Jun 1, 2017

adamruzicka Jun 1, 2017

mmoll Jun 1, 2017

ekohl Jun 1, 2017

adamruzicka Jun 1, 2017

theforeman-bot commented Jun 1, 2017

theforeman-bot commented Jun 1, 2017

theforeman-bot commented Jun 1, 2017

theforeman-bot commented Jun 1, 2017

theforeman-bot commented Jun 1, 2017

ekohl Jun 5, 2017

ekohl Jul 12, 2017

ekohl Jul 12, 2017

adamruzicka commented Aug 14, 2017

ekohl left a comment

ekohl Aug 14, 2017

adamruzicka Aug 14, 2017

ekohl commented Aug 14, 2017

mmoll Aug 14, 2017

adamruzicka Aug 15, 2017

ekohl Aug 15, 2017

ekohl left a comment

ekohl commented Aug 15, 2017

adamruzicka commented Aug 15, 2017

Fixes #11936 - Add support for kerberos auth #351

Fixes #11936 - Add support for kerberos auth #351

Conversation

adamruzicka commented Jun 1, 2017

theforeman-bot commented Jun 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theforeman-bot commented Jun 1, 2017

theforeman-bot commented Jun 1, 2017

theforeman-bot commented Jun 1, 2017

theforeman-bot commented Jun 1, 2017

theforeman-bot commented Jun 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamruzicka commented Aug 14, 2017

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl commented Aug 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

ekohl commented Aug 15, 2017

adamruzicka commented Aug 15, 2017