Fixes #31049 - Introduce server CA file setting #8076

kamils-iRonin · 2020-10-14T12:04:25Z

No description provided.

theforeman-bot · 2020-10-14T12:04:37Z

Issues: #31049

ezr-ondrej · 2020-10-21T08:41:41Z

Do we have installer PR for this?

ekohl

I'm concerned that this would break registration via Smart Proxies. Should we introduce a different macro for that?

lib/foreman/renderer/scope/macros/base.rb

ares · 2020-10-23T15:15:25Z

I think @ekohl is right about breaking the through proxy registration. However with this new setting, the macro should load certificates from both files. That way, we'd make sure the user can use whatever method if they deploy certificates provided by this macro.

The other option is to go with what is proposed here and introduce a second macro to provide the same for proxies and then pick whichever is needed in the template. However the added value of this imho doesn't outbalance the effort. I'd lean towards the existing macro providing certs from both files.

ekohl · 2020-10-26T13:32:49Z

Not needed here, but I also thought about introducing something like /ca-bundle.pem as a route. It would return a bundle to connect to Foreman and the Smart Proxies. This does feel like a good place to suggest it. Thoughts on it are welcome :)

ares · 2020-10-26T20:13:52Z

introducing something like /ca-bundle.pem

should it be served by the application or rather just apache from /public? Anyway that's not in scope for this PR. Next steps I think are

do we need an installer change for this and if so, is there already some PR for that?
the foreman_server_ca_cert should load content from both settings now, to avoid breaking through proxy registration

@kamils-iRonin please let me know if there are any questions from your side, thanks!

kamils-iRonin · 2020-10-27T09:22:51Z

2. the foreman_server_ca_cert should load content from both settings now, to avoid breaking through proxy registration

@ares @ekohl we could introduce server_ca_file_enabled and ssl_ca_file_enabled flags (which are true by default), so that you can load exactly the file you want, something like:

def foreman_server_ca_cert(server_ca_file_enabled: true, ssl_ca_file_enabled: true)
  if server_ca_file_enabled && File.exist?(Setting[:server_ca_file])
    File.read(Setting[:server_ca_file])
  elsif ssl_ca_file_enabled && File.exist?(Setting[:ssl_ca_file])
    File.read(Setting[:ssl_ca_file])
  else
    msg = N_("SSL CA file not found, check the 'Server CA file' in Settings > Authentication")
    raise Foreman::Exception.new(msg)
  end
end

ekohl · 2020-10-27T10:11:28Z

From the signature I would expect it concat both when both are true but the implementation only returns one. I think curl is happy to have both CA bundles together and use whatever validates it.

ekohl

Mostly some readability suggestions. Overall I think it looks good but let me know if you want to apply the suggestions.

lib/foreman/renderer/scope/macros/base.rb

ekohl

LGTM. @ares want to have a look at latest iteration?

ares

I'm sorry it took me a while to get back to this. I have one little ask about error handling, otherwise I tested and it works well indeed. Ready to merge after that comments is resolved.

ares · 2020-12-09T10:21:09Z

lib/foreman/renderer/scope/macros/base.rb

+            setting_values << Setting[:ssl_ca_file] if ssl_ca_file_enabled
+            files_content = setting_values.compact.map do |setting_value|
+              File.read(setting_value)
+            rescue Errno::ENOENT, Errno::EACCES


We should rescue the File.read for more exceptions, if I specify wrong path (e.g. a directory) it's not catched by this.
I'd recommend wrapping the File.read in begin end block and just rescue instead of trying to list all Errno::* here. Also I'd say we should log something in case of such error, currently we silently ignore this and admin has no way to find out the set file is not accessible. This would deserve a warning in a log I think.

I've added logger and rescue from StandardError

ares · 2020-12-09T10:24:10Z

Also reiterating a question around the installer PR that ondrej asked. This settings work out of the box so it's not strictly necessary, but I wonder if we should preconfigure this setting from the installer and if someone started working on that already or we need to look into it.

laugmanuel · 2020-12-16T12:58:02Z

Also reiterating a question around the installer PR that ondrej asked. This settings work out of the box so it's not strictly necessary, but I wonder if we should preconfigure this setting from the installer and if someone started working on that already or we need to look into it.

I've opened a first draft here: theforeman/puppet-foreman#910
@ares / @ezr-ondrej, let me know if this is what you were asking about. I'm open for comments or other ideas.

lib/foreman/renderer/scope/macros/base.rb

ekohl

Small nits, otherwise it looks OK to me.

test/fixtures/settings.yml

ares · 2021-03-24T09:54:02Z

my appologies, this now requires a rebase, I think the installer PR is ready so this can get in once rebased

ekohl

CI fails with:

NameError: undefined local variable or method `error' for #<Class:0x000000000e8e98e8>
/home/jenkins/workspace/test_develop_pr_core/database/postgresql/ruby/2.5/slave/fast/test/unit/foreman/renderer/renderers_shared_tests.rb:274:in `block (2 levels) in <module:RenderersSharedTests>'

I'm not sure if it's related, but would you mind taking a look?

ezr-ondrej · 2021-05-12T08:57:32Z

[test foreman][test upgrade]

ezr-ondrej

Just trying to push this forward, I think I found the cause of test failures.

ezr-ondrej · 2021-05-12T22:41:20Z

test/unit/foreman/renderer/renderers_shared_tests.rb

+          Setting[:server_ca_file] = cert_path
+          Setting[:ssl_ca_file] = 'not-existing-file'
+
+          assert_raise Foreman::Exception do


This could be bit more specific by adding assertion for the message

Suggested change

assert_raise Foreman::Exception do

assert_raise Foreman::Exception, '<expected message>' do

ezr-ondrej · 2021-05-12T22:41:44Z

test/unit/foreman/renderer/renderers_shared_tests.rb

+        end
      end

      assert_includes error.message, '[Foreman::Exception]: No such file or directory'


This seems to be quite forgotten and I belive that makes the tests to fail.

ezr-ondrej · 2021-08-09T11:50:56Z

@ares tests seems to be 🍏 now ^_^

ezr-ondrej

Works for me 👍 Thanks @kamils-iRonin

Addressed

ares · 2021-08-16T18:44:07Z

Thanks @kamils-iRonin and @ezr-ondrej for taking care of the merge.

ekohl · 2021-08-17T14:30:30Z

Note to self: we can introduce some URL to expose this. It would replace Katello's http[s]://$HOSTNAME/pub/katello-server-ca.crt.

theforeman-bot added Needs testing Not yet reviewed labels Oct 14, 2020

timogoebel requested review from ares and ekohl October 14, 2020 12:23

ezr-ondrej assigned ares Oct 21, 2020

ekohl reviewed Oct 21, 2020

View reviewed changes

lib/foreman/renderer/scope/macros/base.rb Outdated Show resolved Hide resolved

lib/foreman/renderer/scope/macros/base.rb Outdated Show resolved Hide resolved

kamils-iRonin force-pushed the 31049-Add-more-reliable-way-to-detect-Foreman-CA-certificate branch 2 times, most recently from c2286ea to c767580 Compare October 29, 2020 16:13

kamils-iRonin requested a review from ekohl November 12, 2020 11:25

ekohl reviewed Nov 19, 2020

View reviewed changes

lib/foreman/renderer/scope/macros/base.rb Outdated Show resolved Hide resolved

lib/foreman/renderer/scope/macros/base.rb Outdated Show resolved Hide resolved

kamils-iRonin force-pushed the 31049-Add-more-reliable-way-to-detect-Foreman-CA-certificate branch from c767580 to 82673da Compare November 24, 2020 09:30

ekohl previously approved these changes Nov 24, 2020

View reviewed changes

theforeman-bot removed the Not yet reviewed label Nov 24, 2020

ares previously requested changes Dec 9, 2020

View reviewed changes

theforeman-bot added the Waiting on contributor label Dec 9, 2020

kamils-iRonin dismissed ekohl’s stale review via e49bad1 December 15, 2020 09:35

kamils-iRonin force-pushed the 31049-Add-more-reliable-way-to-detect-Foreman-CA-certificate branch from 82673da to e49bad1 Compare December 15, 2020 09:35

theforeman-bot added Needs re-review and removed Waiting on contributor labels Dec 15, 2020

laugmanuel mentioned this pull request Dec 16, 2020

Introduce server_ca_file parameter theforeman/puppet-foreman#910

Draft

ezr-ondrej reviewed Dec 22, 2020

View reviewed changes

lib/foreman/renderer/scope/macros/base.rb Outdated Show resolved Hide resolved

ekohl reviewed Dec 23, 2020

View reviewed changes

test/fixtures/settings.yml Outdated Show resolved Hide resolved

kamils-iRonin force-pushed the 31049-Add-more-reliable-way-to-detect-Foreman-CA-certificate branch 2 times, most recently from 1a5f6a5 to 614efad Compare February 9, 2021 15:52

ares added the Waiting on contributor label Mar 24, 2021

kamils-iRonin force-pushed the 31049-Add-more-reliable-way-to-detect-Foreman-CA-certificate branch from 614efad to 7f1e777 Compare April 12, 2021 22:13

theforeman-bot removed the Waiting on contributor label Apr 12, 2021

kamils-iRonin force-pushed the 31049-Add-more-reliable-way-to-detect-Foreman-CA-certificate branch from 7f1e777 to de46ba0 Compare April 12, 2021 22:19

ekohl reviewed May 5, 2021

View reviewed changes

ezr-ondrej reviewed May 12, 2021

View reviewed changes

Fixes #31049 - Introduce server CA file setting

0a023b8

kamils-iRonin force-pushed the 31049-Add-more-reliable-way-to-detect-Foreman-CA-certificate branch from de46ba0 to 0a023b8 Compare August 4, 2021 13:56

ezr-ondrej added the Demo worthy label Aug 11, 2021

ezr-ondrej approved these changes Aug 11, 2021

View reviewed changes

theforeman-bot removed the Needs re-review label Aug 11, 2021

ezr-ondrej merged commit 0d08d7a into theforeman:develop Aug 11, 2021

	assert_raise Foreman::Exception do
	assert_raise Foreman::Exception, '<expected message>' do

Fixes #31049 - Introduce server CA file setting #8076

Fixes #31049 - Introduce server CA file setting #8076

Uh oh!

Conversation

kamils-iRonin commented Oct 14, 2020

Uh oh!

theforeman-bot commented Oct 14, 2020

Uh oh!

ezr-ondrej commented Oct 21, 2020

Uh oh!

ekohl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ares commented Oct 23, 2020

Uh oh!

ekohl commented Oct 26, 2020

Uh oh!

ares commented Oct 26, 2020

Uh oh!

kamils-iRonin commented Oct 27, 2020

Uh oh!

ekohl commented Oct 27, 2020

Uh oh!

ekohl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ekohl left a comment

Choose a reason for hiding this comment

Uh oh!

ares left a comment

Choose a reason for hiding this comment

Uh oh!

ares Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

kamils-iRonin Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

ares commented Dec 9, 2020

Uh oh!

laugmanuel commented Dec 16, 2020

Uh oh!

Uh oh!

ekohl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ares commented Mar 24, 2021

Uh oh!

ekohl left a comment

Choose a reason for hiding this comment

Uh oh!

ezr-ondrej commented May 12, 2021

Uh oh!

ezr-ondrej left a comment

Choose a reason for hiding this comment

Uh oh!

ezr-ondrej May 12, 2021

Choose a reason for hiding this comment

Uh oh!

ezr-ondrej May 12, 2021

Choose a reason for hiding this comment

Uh oh!

ezr-ondrej commented Aug 9, 2021

Uh oh!

ezr-ondrej left a comment

Choose a reason for hiding this comment

Uh oh!

ares commented Aug 16, 2021

Uh oh!

ekohl commented Aug 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects