-
Notifications
You must be signed in to change notification settings - Fork 51
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
create ssl cert per vhost, not one monster #2129
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
if $letsencrypt { | |
include web::letsencrypt | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there was a typo here, but at least it was used consistently.
No description provided.