docker-archive / dockercloud-haproxy

HAproxy image that autoreconfigures itself when used in Docker Cloud
https://cloud.docker.com/
651 stars 187 forks source link

Handling multiple ssl certs (per service) #212

Closed steffkes closed 6 years ago

steffkes commented 7 years ago

ever since i've been using this haproxy i've been wondering about how others handle a situation where one service might use multiple ssl certs?

i know about EXTRA_CERTS which can be assigned to the haproxy instance itself - but my point is more about maintenance. why is a certificate that obviously belongs to a service required to be configured on the load balancer level?

for me, the intuitive way would be to specify multiple ssl certs on the service where they are needed. therefore the cert(s) would be installed once the service comes up and are removed once it goes down. w/o any additional steps that need to be taken care of - and probably get forgotten more than once.

Skimming through the code i see Specs and the EnvParser - where only the allowed options are parsed and afterwards useable. given my python knowledge, i don't know how such dynamic/prefixed names would/could/should be handled?

anyone any thoughts?

tifayuki commented 7 years ago

@steffkes you can set SSL_CERT in the linked service, and the certs will be loaded and removed automatically when the service starts and dies.

steffkes commented 7 years ago

@tifayuki i've thought i've mentioned that .. obviously didn't ;o I know about SSL_CERT.. what i'm curious about is that you're talking about the certs (plural) - i was not aware that it's possible to load multiple certs that way?

SSL_CERT did and still does sound like singular to me. and everything mentioned in the README is just about the/one ssl cert needs to be created (order + linebreaks). or am i mistaken here?

my understanding of haproxy was/is that it needs to have one file per certificate, which includes all relevant information for that very cert. at least that is what i'm reading from the haproxy docs

tifayuki commented 7 years ago

@steffkes Yes, in the linked server, it only accepts one cert file per service. If you have multiple certificates, you can always set them directly in Haproxy container. Or, you can use a volume container to hold the certificates or even secrets.

Also, I think it is OK to set the all the certificates directly on LB, we do the same thing on ELB for SSL termination.

And, ya, PRs are always welcome. ;)

steffkes commented 7 years ago

okay, thanks for clarifying @tifayuki - at least my understanding is correct.

to try again: i know that it is possible - in general - to handle multiple certs, but it's only possible to assign one cert on a linked service. if you need multiple certs you have to do it on the LB container given the EXTRA_CERTS option. and that is the step i've talked about earlier, maintenance that is probably forgotten at least once .. and yes, speaking from experience here ;>

I'd like to provide a patch that allows multiple certs on a linked service - because that is the way i'd assume it works w/o knowing any details of the project itself. therefore i'd like to get a bit of guidance, how/where to go with it.

i did play around with a static setup, therefore i could easily come up with a parse_ssl_cert_cert1 method .. but that actually doesn't do the trick for a generic solution ;o

how about a mapping? if the key matches ^SSL_CERT_ then parse those certs (using the existing methods) and assign them to a single property ssl_certs or something along the lines? right now, it's basically a 1:1 mapping - which isn't that useful for what i'm thinking to come up with.

or is there a way to have dynamic methods? because if i'm extending EnvParser like described above, i can see all the keys in Specs.details and we could iterate over them and filter everything that starts with ssl_cert_..

just to put out random thoughts, probably there are better ways to solve the problem and i'm just thinking too complicated? :) don't hesitate to suggest other ways

steffkes commented 7 years ago

before i'm creating a PR, i'd like to know if that is something acceptable @tifayuki .. tried the following modification on a local instance and it works at least :) obviously missing tests and probably some other things i didn't even think about?

diff --git a/haproxy/haproxycfg.py b/haproxy/haproxycfg.py
index efc3af8..ae727e8 100644
--- a/haproxy/haproxycfg.py
+++ b/haproxy/haproxycfg.py
@@ -209,6 +209,7 @@ class Haproxy(object):
         certs.extend(SslHelper.get_extra_ssl_certs(EXTRA_SSL_CERT))
         certs.extend(self.specs.get_default_ssl_cert())
         certs.extend(self.specs.get_ssl_cert())
+        certs.extend(self.specs.get_ssl_certs())
         if certs:
             if set(certs) != set(Haproxy.cls_certs):
                 Haproxy.cls_certs = copy.copy(certs)
diff --git a/haproxy/parser/base_parser.py b/haproxy/parser/base_parser.py
index 48c1ef3..a671427 100644
--- a/haproxy/parser/base_parser.py
+++ b/haproxy/parser/base_parser.py
@@ -100,6 +100,14 @@ class Specs(object):
                                                  "ssl_cert" in attr])
         return self.ssl_cert

+    def get_ssl_certs(self):
+        if not hasattr(self, "ssl_certs"):
+            self.ssl_certs = []
+            for attr in self.details.itervalues():
+                self.ssl_certs.extend(attr["ssl_certs"].values())
+
+        return self.ssl_certs
+

 class EnvParser(object):
     def __init__(self):
diff --git a/haproxy/parser/new_parser.py b/haproxy/parser/new_parser.py
index 9804645..08f3858 100644
--- a/haproxy/parser/new_parser.py
+++ b/haproxy/parser/new_parser.py
@@ -1,5 +1,6 @@
 import haproxy.config
 from haproxy.parser.base_parser import Specs, EnvParser
+import re

 class NewSpecs(Specs):
@@ -55,6 +56,7 @@ class NewEnvParser(EnvParser):
             for attr in self.attrs:
                 self.details[service_aliase][attr] = self.__getattribute__("parse_%s" % attr)("")
         self.service_aliases = service_aliases
+        self.details[service_aliase]["ssl_certs"] = {}

     def parse(self, service, key, value):
         key = key.lower()
@@ -62,3 +64,5 @@ class NewEnvParser(EnvParser):
             for attr in self.attrs:
                 if key == attr and not self.details[service][key]:
                     self.details[service][key] = getattr(self, "parse_%s" % key)(value)
+            if re.match("ssl_cert_", key):
+                self.details[service]["ssl_certs"][key] = self.parse_ssl_cert(value)

tried it using this docker-compose.yml:

version: '2'
services:

  lb:
    build:
      context: ./dockercloud-haproxy
      dockerfile: Dockerfile-dev
    volumes:
      - /var/run/docker.sock:/var/run/docker.sock
    ports:
      - 7000:7000
    links:
      - test_01

  test_01:
    image: dockercloud/hello-world
    environment:
      VIRTUAL_HOST: https://test-01.dev:7000
      FORCE_SSL: 'yes'
      SSL_CERT_CERT1: "-----BEGIN RSA PRIVATE KEY-----\n..."
      SSL_CERT_CERT2: "-----BEGIN RSA PRIVATE KEY-----\n..."
      SSL_CERT_CERT3: "-----BEGIN RSA PRIVATE KEY-----\n..."
steffkes commented 6 years ago

@tifayuki i'm not sure why you've closed this thread while the discussion is somewhat in progress?

steffkes commented 6 years ago

@tifayuki i'll see if i can bring up a PR that integrates what i'm looking for, based on my previous try - hope that's okay with you since i didn't get any feedback