Skip to content
This repository has been archived by the owner on Nov 30, 2021. It is now read-only.

fix(router): include deis.conf if no match with an SSL cert #3519

Merged
merged 2 commits into from Apr 22, 2015

Conversation

bacongobbler
Copy link
Member

The idea behind not including deis.conf was so that custom domains which
are different from the platform domain would not attach the platform SSL
certificate when it was enabled. However, this has the side effect of
not attaching domains which are subdomains of the platform domain, which
is intentional.

A better fix would be to check if the domain is a subdomain of the platform domain but text/template or confd doesn't have strings.Contains included as a function.

closes #3470

@bacongobbler bacongobbler added this to the v1.5.2 milestone Apr 17, 2015
@bacongobbler bacongobbler self-assigned this Apr 17, 2015
@bacongobbler
Copy link
Member Author

filed kelseyhightower/confd#270 for this request

@carmstrong
Copy link
Contributor

We should manually test that the catchall router-level certificate takes precedence and is applied for all domains unless a domain has an SSL certificate attached, in which case that certificate is served only for that domain.

@bacongobbler bacongobbler force-pushed the 3470-use-conf-when-no-match branch 3 times, most recently from 46dacb5 to 9bc0919 Compare April 20, 2015 19:21
The idea behind not including deis.conf was so that custom domains which
are different from the platform domain would not attach the platform SSL
certificate when it was enabled. However, this has the side effect of
not attaching domains which are subdomains of the platform domain, which
is intentional.
@bacongobbler
Copy link
Member Author

fixed:

><> deis info
=== go Application
{
  "updated": "2015-04-20T17:03:24UTC",
  "uuid": "c306b6e2-2192-469e-bf27-3126ed3ad6c8",
  "created": "2015-04-20T17:00:40UTC",
  "url": "go.bacongobbler.com",
  "owner": "bacongobbler",
  "id": "go",
  "structure": {
    "web": 1
  }
}

=== go Processes

--- web:
web.1 up (v2)

=== go Domains
go.23.253.174.228.xip.io
www.bacongobbler.com
foo.fishworks.io
><> deis certs
Common Name       Expires
----------------  ----------------------
foo.fishworks.io  2016-04-19T18:04:53UTC

Output of nginx.conf in the routers (only showing the relevant parts): https://gist.github.com/bacongobbler/28f61e38e16474ab9127

Note that every domain except for foo.fishworks.io is relying on the default deis.conf, whereas foo.fishworks.io is using app SSL.

@mboersma
Copy link
Member

Code LGTM.

@carmstrong
Copy link
Contributor

$ openssl s_client -connect foo.fishworks.io:443
CONNECTED(00000003)
depth=0 /C=CA/ST=British-Columbia/L=Vancouver/O=Fishworks Development and Consulting/CN=*.bacongobbler.com/emailAddress=matthewf@fishworks.io
verify error:num=18:self signed certificate
verify return:1
depth=0 /C=CA/ST=British-Columbia/L=Vancouver/O=Fishworks Development and Consulting/CN=*.bacongobbler.com/emailAddress=matthewf@fishworks.io
verify return:1
---
Certificate chain
 0 s:/C=CA/ST=British-Columbia/L=Vancouver/O=Fishworks Development and Consulting/CN=*.bacongobbler.com/emailAddress=matthewf@fishworks.io
   i:/C=CA/ST=British-Columbia/L=Vancouver/O=Fishworks Development and Consulting/CN=*.bacongobbler.com/emailAddress=matthewf@fishworks.io
---

Looks to me like foo.fishworks.io is using the wildcard certificate?

@bacongobbler
Copy link
Member Author

I see that too when using the openssl client...

><> openssl s_client -connect foo.fishworks.io:443
CONNECTED(00000003)
depth=0 /C=CA/ST=British-Columbia/L=Vancouver/O=Fishworks Development and Consulting/CN=*.bacongobbler.com/emailAddress=matthewf@fishworks.io

but when I use Chrome:

screen shot 2015-04-20 at 3 11 24 pm

Same for Safari, which I never use:

screen shot 2015-04-20 at 3 12 50 pm

Is there something I'm doing wrong here?

@carmstrong
Copy link
Contributor

I dunno... are you running multiple routers, some of which may not have the fix? Maybe it's the luck of the draw for a request.

@bacongobbler
Copy link
Member Author

Just did a fresh deploy. All three have the same config :S

@carmstrong
Copy link
Contributor

Now that I think about it, maybe s_client doesn't send a Host header (just connects TCP on port 443), so it makes sense that the router serves the default cert. LGTM.

@carmstrong
Copy link
Contributor

@carmstrong
Copy link
Contributor

This has failed twice in a row in the same place:

domains:add test.test-7cc8a64e6a.deis.works --app domainsample
Adding test.test-7cc8a64e6a.deis.works to domainsample...  ... ����� o.. �����done
ok
certs:add /var/lib/jenkins/jobs/test-ec2/workspace/src/github.com/deis/deis/tests/test.test-7cc8a64e6a.deis.works.cert /var/lib/jenkins/jobs/test-ec2/workspace/src/github.com/deis/deis/tests/test.test-7cc8a64e6a.deis.works.key
Adding SSL endpoint...  ... �����done
test.test-7cc8a64e6a.deis.works
ok
sleeping for 60 seconds until certs are generated...
ok
SIGQUIT: quit
PC=0x43ec61

@bacongobbler I suspect something is wrong with this PR. Can you try running the integration tests locally and see if you see the same issue?

@bacongobbler
Copy link
Member Author

The common problem on Jenkins passes for me locally on both Rackspace and EC2:

sleeping for 60 seconds until certs are generated...
ok
Powered by Deis

certs:remove test.fishr.pw
Removing test.fishr.pw... Done.

confd turns hyphens to dashes, so we need to account for that in
domains. Since confd does not have native support for strings.Replace,
I've had to fork our current branch and backport strings.Replace as a
template function.
@bacongobbler
Copy link
Member Author

Gotta love heisenbugs 🐛

@mboersma
Copy link
Member

Reviewed this again, plus deis/confd@4c50136. Code LGTM.

@carmstrong
Copy link
Contributor

🚢

bacongobbler pushed a commit that referenced this pull request Apr 22, 2015
fix(router): include deis.conf if no match with an SSL cert
@bacongobbler bacongobbler merged commit a4bf040 into deis:master Apr 22, 2015
@bacongobbler bacongobbler deleted the 3470-use-conf-when-no-match branch April 22, 2015 16:53
wenzowski pushed a commit to dialoghq/deis that referenced this pull request Apr 24, 2015
…match

fix(router): include deis.conf if no match with an SSL cert
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SSL is broken after upgrade to 1.5.1
3 participants