From 5d60472d78de5ff531bbf6cace272a939dafb489 Mon Sep 17 00:00:00 2001
From: Juan Antonio Osorio Robles <jaosorior@redhat.com>
Date: Mon, 20 Aug 2018 08:36:33 +0300
Subject: [PATCH] Use exec for CA CRL instead of file resource
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This is because the file resource doesn't properly handle query
parameters in URLs. So we are forced to use an exec resource here. It's
fine if we always trigger the CRL downloading, as that's a file that
gets udpated often.

Also ensure we get proper escaped source/destination for the download.

Co-Authored-By: Cédric Jeanneret <cjeanner@redhat.com>
Change-Id: I15ad3ab0cd129a8e1b9261341c0510265bda8016
Closes-Bug: #1787878
---
 manifests/certmonger/ca/crl.pp                | 47 +++++++++++++------
 .../classes/tripleo_certmonger_ca_crl_spec.rb | 30 ++++++++----
 2 files changed, 53 insertions(+), 24 deletions(-)

diff --git a/manifests/certmonger/ca/crl.pp b/manifests/certmonger/ca/crl.pp
index e1d8072c5..1e0db58e9 100644
--- a/manifests/certmonger/ca/crl.pp
+++ b/manifests/certmonger/ca/crl.pp
@@ -85,8 +85,35 @@ class tripleo::certmonger::ca::crl (
   $maxdelay                   = 0,
   $reload_cmds                = [],
 ) {
+  if $process {
+    $fetched_crl = $crl_preprocessed
+  } else {
+    $fetched_crl = $crl_dest
+  }
+
+  $esc_fetched_crl = shell_escape($fetched_crl)
+  $esc_crl_src = shell_escape($crl_source)
+
   if $crl_source {
     $ensure = 'present'
+    # LP(1787878): We need to use an explicit command instead of the file
+    # resource, because puppet won't use query parameters when handling
+    # redirects.
+    # If FreeIPA is being installed in a similar time as the overcloud, the tries
+    # and time in between tries gives it a chance to generate the CRL.
+    exec {'tripleo-ca-crl':
+      command   => "curl -Ls --connect-timeout 120 -o ${esc_fetched_crl} ${esc_crl_src}",
+      path      => '/usr/bin/',
+      creates   => $fetched_crl,
+      tries     => 5,
+      try_sleep => 5,
+    } ~>
+    file {'tripleo-ca-crl-file':
+      group => 'root',
+      mode  => '0644',
+      owner => 'root',
+      path  => $fetched_crl,
+    }
   } else {
     $ensure = 'absent'
   }
@@ -97,31 +124,21 @@ class tripleo::certmonger::ca::crl (
     $sleep = "sleep `expr \${RANDOM} \\% ${maxdelay}`; "
   }
 
-  if $process {
-    $fetched_crl = $crl_preprocessed
-  } else {
-    $fetched_crl = $crl_dest
-  }
-
-  file { 'tripleo-ca-crl' :
-    ensure => $ensure,
-    path   => $fetched_crl,
-    source => $crl_source,
-    mode   => '0644',
-  }
-
   if $process and $ensure == 'present' {
     $crl_dest_format = $crl_preprocessed_format ? {
       'PEM' => 'DER',
       'DER' => 'PEM'
     }
     # transform CRL from DER to PEM or viceversa
-    $process_cmd = "openssl crl -in ${$crl_preprocessed} -inform ${crl_preprocessed_format} -outform ${crl_dest_format} -out ${crl_dest}"
+    $process_cmd = "openssl crl -in ${crl_preprocessed} -inform ${crl_preprocessed_format} -outform ${crl_dest_format} -out ${crl_dest}"
     exec { 'tripleo-ca-crl-process-command' :
       command     => $process_cmd,
       path        => '/usr/bin',
       refreshonly => true,
-      subscribe   => File['tripleo-ca-crl']
+      subscribe   => [
+        Exec['tripleo-ca-crl'],
+        File['tripleo-ca-crl-file']
+      ]
     }
   } else {
     $process_cmd = []
diff --git a/spec/classes/tripleo_certmonger_ca_crl_spec.rb b/spec/classes/tripleo_certmonger_ca_crl_spec.rb
index d557a1bc7..3c45a58bf 100644
--- a/spec/classes/tripleo_certmonger_ca_crl_spec.rb
+++ b/spec/classes/tripleo_certmonger_ca_crl_spec.rb
@@ -24,9 +24,7 @@ describe 'tripleo::certmonger::ca::crl' do
 
     context 'with default parameters (no crl_source)' do
       it 'should ensure no CRL nor cron job are present' do
-        is_expected.to contain_file('tripleo-ca-crl').with(
-          :ensure => 'absent'
-        )
+        is_expected.not_to contain_exec('tripleo-ca-crl')
         is_expected.to contain_cron('tripleo-refresh-crl-file').with(
           :ensure => 'absent'
         )
@@ -51,9 +49,16 @@ describe 'tripleo::certmonger::ca::crl' do
       end
 
       it 'should create and process CRL file' do
-        is_expected.to contain_file('tripleo-ca-crl').with(
-          :ensure => 'present',
-          :source => params[:crl_source]
+        is_expected.to contain_exec('tripleo-ca-crl').with(
+          :command   => "curl -Ls --connect-timeout 120 -o #{params[:crl_preprocessed]} #{params[:crl_source]}",
+          :tries     => 5,
+          :try_sleep => 5
+        )
+        is_expected.to contain_file('tripleo-ca-crl-file').with(
+          :group => 'root',
+          :mode  => '0644',
+          :owner => 'root',
+          :path  => "#{params[:crl_preprocessed]}"
         )
         is_expected.to contain_exec('tripleo-ca-crl-process-command').with(
           :command => process_cmd
@@ -79,9 +84,16 @@ describe 'tripleo::certmonger::ca::crl' do
       end
 
       it 'should create and process CRL file' do
-        is_expected.to contain_file('tripleo-ca-crl').with(
-          :ensure => 'present',
-          :source => params[:crl_source]
+        is_expected.to contain_exec('tripleo-ca-crl').with(
+          :command   => "curl -Ls --connect-timeout 120 -o #{params[:crl_dest]} #{params[:crl_source]}",
+          :tries       => 5,
+          :try_sleep   => 5
+        )
+        is_expected.to contain_file('tripleo-ca-crl-file').with(
+          :group => 'root',
+          :mode  => '0644',
+          :owner => 'root',
+          :path  => "#{params[:crl_dest]}"
         )
         is_expected.to_not contain_exec('tripleo-ca-crl-process-command')
         is_expected.to contain_cron('tripleo-refresh-crl-file').with(