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

Implemented LETSENCRYPT_STANDALONE_CERTS support from container env #610

Closed
wants to merge 3 commits into from

Conversation

SilverFire
Copy link
Contributor

Hi, @buchdag

I've followed your idea with LETSENCRYPT_STANDALONE_CERTS and ported it to environment variables of Nginx container.

The usage will be pretty simple:

docker-compose.yml

version: '3'

services:
  nginx:
    image: nginx:latest
    environment:
      - "LETSENCRYPT_STANDALONE_CERTS=true"
      - "VIRTUAL_HOST=example.com,private.acme.com,dev.acme.com"
      - "LETSENCRYPT_HOST=example.com,private.acme.com,dev.acme.com"

This config will produce three separate certificates. It seems more correct in terms of decoupling to claim separate certificates from the Nginx container env, than touching the main LE companion.

If the PR is acceptable, I will adjust the documentations

buchdag and others added 3 commits October 17, 2019 13:59
Standalone certificates are generated from a static user provided
configuration file rather than from the dynamicaly generated (from
running containers environment variables) letsencrypt_service_data file.
@buchdag
Copy link
Member

buchdag commented Jan 9, 2020

Hi.

Sorry if I'm not getting it but how is that any different from using a dummy container to generate certificates ?

@buchdag
Copy link
Member

buchdag commented Jan 9, 2020

The idea behind the standalone cert feature was to generate certificate that aren't tied to any specific container's Environment.

@SilverFire
Copy link
Contributor Author

I didn't see the dummy containers idea in the documentation. Do you have such?

There are a few use cases I address with this PR:

  1. Container that listens to 10-20 domain names, but I don't want to have a common certificate for them since they represent different white labels.
  2. I don't want to reconfigure LE companion when I change the domains list
  3. If one of the domains fails to obtain certificate because DNS is messed up, I don't want other certificates to fails.

@buchdag
Copy link
Member

buchdag commented Jan 16, 2020

Oh, it sounds like an attempt to implement something similar to #410 then.

Did I get that right ?

@SilverFire
Copy link
Contributor Author

Yes, it actually implements issue #410, but with a little bit simpler way of configuration.

Instead of having something like this

LETSENCRYPT_STANDALONE_CERTS=('foo_com' 'bar_com', 'www_bar_com')
LETSENCRYPT_foo_com_HOST=foo.com
LETSENCRYPT_foo_com_EMAIL=letsencrypt@foo.com
LETSENCRYPT_bar_com_HOST=bar.com
LETSENCRYPT_bar_com_EMAIL=letsencrypt@bar.com
LETSENCRYPT_www_bar_com_HOST=www.bar.com
LETSENCRYPT_www_bar_com_EMAIL=letsencrypt@bar.com

I suggest to have

LETSENCRYPT_STANDALONE_CERTS=true
LETSENCRYPT_HOST=foo.com,bar.com,www.bar.com
LETSENCRYPT_EMAIL=letsencrypt@foo.com

The only drawback I see is that we can not merge into a single SAN group of domains foo.com / www.foo.com, but we can enhance it later upon request.

@buchdag
Copy link
Member

buchdag commented Feb 1, 2020

Ok, standalone certs (#368) and the splitting of SAN certificates (#410) are two completely different features, hence the confusion.

Standalone certs refers to the ability to issue certificate that aren't tied to a specific container's environment.

You PR will have to use another env var than LETSENCRYPT_STANDALONE_CERTS as that's reserved for the standalone certs feature and a different term than 'standalone'.

LETSENCRYPT_SPLIT_CERTS ?
LETSENCRYPT_SINGLE_CERTS ?

@SilverFire
Copy link
Contributor Author

I'd vote for LETSENCRYPT_SINGLE_CERTS or a bit more specific: LETSENCRYPT_SINGLE_DOMAIN_CERTS

@buchdag
Copy link
Member

buchdag commented Mar 9, 2020

I'm okay with LETSENCRYPT_SINGLE_DOMAIN_CERTS.

Don't forget to create the documentation for the new environment variable.

@buchdag
Copy link
Member

buchdag commented Mar 9, 2020

Other than that, the PR look fine to me.

@buchdag buchdag changed the base branch from dev to master March 10, 2020 08:42
@SilverFire
Copy link
Contributor Author

Wilco

@buchdag
Copy link
Member

buchdag commented Mar 12, 2020

Didn't see you based you branch on dev, looks like I fucked up things a bit when I force pushed an up to date version of dev thee days ago, sorry for that 😒

@buchdag
Copy link
Member

buchdag commented Mar 28, 2020

@SilverFire could you re-open this PR with a new feature branch (like you did in #612) rather than your fork's master, dev or stable branch ?

I'd like to merge the standalone cert feature of dev soonish and this PR is blocking me, plus the previously mentioned issue.

@SilverFire
Copy link
Contributor Author

Will do it today eve

@SilverFire
Copy link
Contributor Author

Sorry for the delay. Replaced with #647

@SilverFire SilverFire closed this Apr 21, 2020
@buchdag buchdag added the kind/feature-request Issue requesting a new feature label May 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature-request Issue requesting a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants