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

Candlepin CA should be owned by tomcat user #232

Merged
merged 1 commit into from
Dec 6, 2018

Conversation

jturel
Copy link
Contributor

@jturel jturel commented Dec 4, 2018

Need to test this still but I want to see if a test fails so I can fix it or write a new one.

@jturel
Copy link
Contributor Author

jturel commented Dec 4, 2018

I think there may be something else at play with selinux despite the fact I did a restorecon in /etc/candlepin/certs. Will look into it..

@ehelms
Copy link
Member

ehelms commented Dec 5, 2018

I am also doing some further testing. One thing I think I missed in the initial commit was adding "stripping" of the certificate to remove the x509 header information from the file we had originally been using.

@ehelms
Copy link
Member

ehelms commented Dec 5, 2018

For me, the following additions fixed it:

diff --git a/manifests/candlepin.pp b/manifests/candlepin.pp
index 3d84eb2..548eff7 100644
--- a/manifests/candlepin.pp
+++ b/manifests/candlepin.pp
@@ -84,11 +84,14 @@ class certs::candlepin (
       key_pair      => $default_ca,
       key_file      => $ca_key,
       cert_file     => $ca_cert,
-      cert_owner    => $user,
-      cert_group    => $group,
+      cert_owner    => 'tomcat',
+      cert_group    => 'tomcat',
+      key_owner     => 'tomcat',
+      key_group     => 'tomcat',
       key_mode      => '0440',
       cert_mode     => '0640',
       unprotect     => true,
+      strip         => true,
       password_file => $ca_key_password_file,
     } ~>
     certs::keypair { 'tomcat':
diff --git a/manifests/keypair.pp b/manifests/keypair.pp
index 9e083c5..b6ea9b4 100644
--- a/manifests/keypair.pp
+++ b/manifests/keypair.pp
@@ -11,6 +11,7 @@ define certs::keypair (
   $cert_group  = undef,
   $cert_mode   = undef,
   $unprotect   = false,
+  $strip       = false,
   $password_file = undef,
 ) {
   $key_pair ~>
@@ -21,6 +22,7 @@ define certs::keypair (
   } ~>
   pubkey { $cert_file:
     key_pair => $key_pair,
+    strip    => $strip,
   }
 
   if $manage_key {

@ekohl
Copy link
Member

ekohl commented Dec 5, 2018

It'd be great if you could add a test to https://github.com/theforeman/puppet-certs/blob/master/spec/acceptance/candlepin_spec.rb where you can use https://serverspec.org/resource_types.html#file to write tests about files on an actual installed system.

Note that it's my goal to add a similar test to puppet-katello where we combine the two

@jturel
Copy link
Contributor Author

jturel commented Dec 5, 2018

@ehelms those additions look sane - but could you explain the stripping piece a bit more? especially since it seems to work without.

The selinux issue I mentioned:

type=AVC msg=audit(1543981267.690:4266): avc:  denied  { read } for  pid=19475 comm="java" name="candlepin-ca.crt" dev="vda1" ino=2491120 scontext=system_u:system_r:tomcat_t:s0 tcontext=system_u:object_r:candlepin_etc_certs_ca_cert_r_t:s0 tclass=file
type=SYSCALL msg=audit(1543981267.690:4266): arch=c000003e syscall=2 success=no exit=-13 a0=7f40257d8cb0 a1=0 a2=1b6 a3=7f4069052780 items=0 ppid=1 pid=19475 auid=4294967295 uid=53 gid=53 euid=53 suid=53 fsuid=53 egid=53 sgid=53 fsgid=53 tty=(none) ses=4294967295 comm="java" exe="/usr/lib/jvm/java-1.8.0-openjdk-1.8.0.191.b12-1.el7_6.x86_64/jre/bin/java" subj=system_u:system_r:tomcat_t:s0 key=(null)
type=PROCTITLE msg=audit(1543981267.690:4266): proctitle=2F7573722F6C69622F6A766D2F6A72652F62696E2F6A617661002D586D73313032346D002D586D78343039366D002D636C61737370617468002F7573722F73686172652F746F6D6361742F62696E2F626F6F7473747261702E6A61723A2F7573722F73686172652F746F6D6361742F62696E2F746F6D6361742D6A756C692E6A

I'm interpreting this as tomcat doesn't have the ability to read files with the candlepin_etc_certs_ca_r_t context. Is that a responsibility of candlepin-selinux? I'm testing this on nightly btw.

@ehelms
Copy link
Member

ehelms commented Dec 5, 2018

@jturel We were stripping it before and perhaps we don't need to anymore. The stripping removes the x509 header information from the certificate file can be useful for debugging but you can output it using openssl commands fairly easily as well.

I was not experiencing any selinux issues.

@ehelms
Copy link
Member

ehelms commented Dec 5, 2018

Here is my testing playbook:

- hosts: all
  become: true
  vars:
    katello_repositories_environment: staging
    foreman_installer_module_branches:
      - ehelms/certs/fix-candlepin
  roles:
    - role: katello

@jturel
Copy link
Contributor Author

jturel commented Dec 5, 2018

After restarting tomcat you can foreman-rake db:seed without any issue? IOW how are you validating the change?

@jturel
Copy link
Contributor Author

jturel commented Dec 5, 2018

I can reproduce the selinux issue on the nightly pipeline and a box I've defined like this:

nightly-test:                                                                                                                                                                                                      
  box: centos7-katello-nightly                                                                                                                                                                                     
  ansible:                                                                                                                                                                                                         
    variables:                                                                                                                                                                                                     
      katello_repositories_environment: staging                                                                                                                                                                    
      foreman_installer_module_branches:                                                                                                                                                                           
        - ehelms/certs/fix-candlepin

The puppet module changes are part of the fix, but there are selinux denials:

[root@nightly-test certs]# audit2allow -a

#============= tomcat_t ==============
allow tomcat_t candlepin_etc_certs_ca_cert_r_t:file { getattr open read };
allow tomcat_t cgroup_t:dir search;
allow tomcat_t cgroup_t:file { getattr open read };

I applied these and after restarting tomcat I can curl the status API successfully which failed previously. The correct place to fix this is in the candlepin code itself which defines the rest of the policy[1] since I don't think we're in the business of managing selinux by hand (with puppet). I think we're in a bit of a sticky situation. Any opinions on the next step?

[1] https://github.com/candlepin/candlepin/blob/2c24e79dd5438b2c87f6beaa045b22a9e724a3e7/server/selinux/candlepin.te#L89

@jturel
Copy link
Contributor Author

jturel commented Dec 5, 2018

Opened a PR for candlepin: candlepin/candlepin#2171

@ehelms
Copy link
Member

ehelms commented Dec 5, 2018 via email

@jturel
Copy link
Contributor Author

jturel commented Dec 5, 2018

rake db:seed fails because candlepin is not in a good state as it can't read the cert :(

@ehelms
Copy link
Member

ehelms commented Dec 5, 2018 via email

@jturel
Copy link
Contributor Author

jturel commented Dec 6, 2018

This does fix the devel environment but I'm still not sure about nightly. I think this should be merged asap nevertheless. If it that's OK I can add tests during business hours in a separate PR

@jturel
Copy link
Contributor Author

jturel commented Dec 6, 2018

nvm added tests ;)

@ehelms ehelms merged commit 5837db1 into theforeman:master Dec 6, 2018
@jturel jturel deleted the candlepin_ca_owner_group branch December 10, 2018 15:17
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.

4 participants