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 #13634 - Adding Katello cert to ca-trust #72

Merged
merged 1 commit into from
Feb 10, 2016

Conversation

parthaa
Copy link
Contributor

@parthaa parthaa commented Feb 9, 2016

Adding Katello Server cert to the machines local ca-trust

@@ -129,6 +129,7 @@

$katello_server_ca_cert = "${certs::pki_dir}/certs/${server_ca_name}.crt"

class { 'trusted_ca': } ->
Copy link
Member

Choose a reason for hiding this comment

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

This should be ::trusted_ca

@ekohl
Copy link
Member

ekohl commented Feb 9, 2016

Dependencies should also be in .fixtures.yml, preferably their git versions so we can spot failures sooner.

@@ -9,6 +9,10 @@
"issues_url": "http://projects.theforeman.org/projects/katello/issues",
"dependencies": [
{
"name": "evenup-trusted_ca",
"version_requirement": ">= 1.0.1"
Copy link
Member

Choose a reason for hiding this comment

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

Please add < 2.0.0 to the end to help us avoid major version breakages in the future.

@@ -18,6 +18,12 @@
$candlepin_consumer_summary = "Subscription-manager consumer certificate for Katello instance ${::fqdn}"
$candlepin_consumer_description = 'Consumer certificate and post installation script that configures rhsm.'

class { 'trusted_ca': }
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed if you are using the defined type? If it is needed, can you use include instead?

class { ::trusted_ca }
trusted_ca::ca { 'katello_server-host-cert':
source => "${certs::pki_dir}/certs/${certs::server_ca_name}.crt",
require => File["${certs::pki_dir}/certs/${certs::server_ca_name}.crt"],
Copy link
Member

Choose a reason for hiding this comment

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

Is it able to resolve $::certs::katello_server_ca_cert here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will test your suggestion. But I got the variables from 2 lines below this.

@parthaa
Copy link
Contributor Author

parthaa commented Feb 10, 2016

Dependencies should also be in .fixtures.yml, preferably their git versions so we can spot failures sooner.

thanks will look into that

@parthaa parthaa force-pushed the add-store branch 2 times, most recently from 5208286 to 2ae5f45 Compare February 10, 2016 00:15
@@ -4,5 +4,6 @@ fixtures:
extlib: "git://github.com/puppet-community/puppet-extlib.git"
foreman: "git://github.com/theforeman/puppet-foreman.git"
common: "git://github.com/katello/puppet-common.git"
evenup: "git@github.com:evenup/evenup-trusted_ca.git"
Copy link
Member

Choose a reason for hiding this comment

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

Please use git:// because it's anonymous.

@parthaa
Copy link
Contributor Author

parthaa commented Feb 10, 2016

thanks will look into that

@ekohl added

@@ -4,5 +4,6 @@ fixtures:
extlib: "git://github.com/puppet-community/puppet-extlib.git"
foreman: "git://github.com/theforeman/puppet-foreman.git"
common: "git://github.com/katello/puppet-common.git"
evenup: "git://github.com/evenup/evenup-trusted_ca.git"
Copy link
Member

Choose a reason for hiding this comment

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

Ideally the name here would be the name of the puppet module and not the provider of the module, i.e. trusted_ca and not evenup

@parthaa
Copy link
Contributor Author

parthaa commented Feb 10, 2016

Also tested this the following way

  • Installed katello-devel
  • saw this file /etc/pki/ca-trust/source/anchors/katello_server-host-cert.crt
  • tried curl https://<fqdn> . It responded with a 503 but not a 401, insecure.
  • I then did
$ rm -f /etc/pki/ca-trust/source/anchors/katello_server-host-cert.crt
$ update-ca-trust
  • Again tried curl https://<fqdn>
  • It responded with a 401, insecure error.

@parthaa parthaa force-pushed the add-store branch 4 times, most recently from 5aa8843 to 9018eb8 Compare February 10, 2016 23:36
@@ -0,0 +1,27 @@
require 'spec_helper'
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: typically the test name follows the full manifest name as a convention, i.e. certs_katello_spec.rb

@ehelms
Copy link
Member

ehelms commented Feb 10, 2016

Just to double check, we want this to be deployed on both the main server and all Capsules is that correct? Currently, the capsule uses the katello.pp manifest -- https://github.com/Katello/puppet-capsule/blob/f05a17386fc7575d7944ea752853696af3ebf6d3/manifests/init.pp#L104

Adding Katello Server cert to the machines local ca-trust
@parthaa
Copy link
Contributor Author

parthaa commented Feb 10, 2016

Just to double check, we want this to be deployed on both the main server and all Capsules is that correct? Currently, the capsule uses the katello.pp manifest -- https://github.com/Katello/puppet-capsule/blob/f05a17386fc7575d7944ea752853696af3ebf6d3/manifests/init.pp#L104

Cert should probably be trusted only the main server for post sync to work afaik. @jlsherrill with the nodes work do think this will be useful in a capsule?
One thing running this in a capsule would be if customer wants to turn that capsule into a Docker node.

@ehelms
Copy link
Member

ehelms commented Feb 10, 2016

ACK - thanks @parthaa !

ehelms added a commit that referenced this pull request Feb 10, 2016
Fixes #13634 - Adding Katello cert to ca-trust
@ehelms ehelms merged commit fd9f9cf into theforeman:master Feb 10, 2016
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.

3 participants