create ssl cert per vhost, not one monster #2129
Conversation
There was a problem hiding this comment.
tl;dr: 👍 on the concept, some implementation notes inline.
I recently read there's some benefit in having all names on a certificate so browsers can reuse connections, but given we're mostly behind a CDN I think the benefit of that is really limited and the operational simplicity of one cert per vhost is way more important.
In the installer we use datacat to gather all values so you could do it that way per host, but it's complex. Also, the module is unmaintained and collections is a maintained alternative.
| @@ -15,37 +15,6 @@ | |||
| letsencrypt => $https, | |||
There was a problem hiding this comment.
I'd prefer to drop this parameter and move the inclusion into the vhost:
foreman-infra/puppet/modules/web/manifests/base.pp
Lines 8 to 10 in cc1300e
There was a problem hiding this comment.
That will break using web::base with HTTPS directly, like it's done by redmine and profiles::jenkins::controller, as those do not use web::vhost.
There was a problem hiding this comment.
Can we modify those to also include the base? I think b31f6de should work and makes it all more consistent.
| @@ -52,6 +52,12 @@ | |||
| } | |||
|
|
|||
| if $web::https { | |||
There was a problem hiding this comment.
As noted above, let's include the prerequisite here
| if $web::https { | |
| if $web::https { | |
| include web::letsencrypt |
|
This solution works for me as an approach. Thanks for taking a look. |
| @@ -15,37 +15,6 @@ | |||
| letsencrypt => $https, | |||
| } | |||
|
|
|||
| if $https { | |||
| $letsencypt_domain = 'theforeman.org' | |||
There was a problem hiding this comment.
Looks like there was a typo here, but at least it was used consistently.
No description provided.