Refactor TLS code with a new `tlsconfig` package by tiborvass · Pull Request #13636 · moby/moby

tiborvass · 2015-06-01T17:34:39Z

This patch creates a new tlsconfig package to handle creation of
secure-enough TLS configurations for clients and servers.

The package was created by refactoring TLS code in the client and the
daemon. After this patch, it is expected that all code creating TLS
configurations use this tlsconfig package for greater security,
consistency and readability.

On the server side, this fixes a bug where --tlsverify was not taken
into account. Now, if specified, it will require the client to
authenticate.

Signed-off-by: Tibor Vass tibor@docker.com

Note: this patch does not modify code under registry/ although it is expected for other patches touching code that creates TLS configurations (including registry/), to reuse this tlsconfig package.

Ping @diogomonica @NathanMcCauley @dmcgowan @docker/core-maintainers

calavera · 2015-06-01T17:39:13Z

do we really need a new package just for this configuration? maybe it should live within the sockets package. We could also rename that package to something else if it's not descriptive enough.

tiborvass · 2015-06-01T17:40:21Z

@calavera in my opinion, yes.

tiborvass · 2015-06-01T17:40:49Z

pkg/tlsutil/config.go

I find this weird but it was like that: https://github.com/docker/docker/pull/13636/files#diff-d6f439c36bc4930ab43c13d5ad3868d2L58

I wonder if this shouldn't be if ca != "" && verify {, on the other hand, why would you provide a CA if you don't want to verify the certificate.

The verify flag has no meaning in this context, if the CA is provided the client should always be verified against it. Using require any client certificate has no value unless you plan on using the certificate for something else after after the handshake.

diogomonica · 2015-06-01T18:08:28Z

I don't see any issues with this. LGTM

aluzzardi · 2015-06-01T18:57:25Z

/cc @ehazlett @nathanleclaire for machine

ehazlett · 2015-06-01T18:59:29Z

Yes!! Thanks!

On Monday, June 1, 2015, Andrea Luzzardi notifications@github.com wrote:

/cc @ehazlett https://github.com/ehazlett @nathanleclaire
https://github.com/nathanleclaire for machine

—
Reply to this email directly or view it on GitHub
#13636 (comment).

estesp · 2015-06-01T19:15:03Z

docker/docker.go

I think this is why you have a broken CI test on janky.. currently TestRunTLSverify assumes that specifying --tls-verify without enabling tls will cause a docker run failure, but it isn't anymore.

yes fixing now :) Thanks!

nathanleclaire · 2015-06-01T19:45:43Z

Nice.

tiborvass · 2015-06-01T20:01:06Z

pkg/tlsutil/config.go

@dmcgowan verify is never used here, should we remove it?

I vote remove it, it is unused and without meaning

stevvooe · 2015-06-01T20:15:57Z

api/server/server.go

TLSConfig

stevvooe · 2015-06-01T20:18:22Z

@tiborvass This looks like a good change. I would prefer to call it tlcconf or something like that, rather than overload the "util" terminology.

aluzzardi · 2015-06-01T20:22:26Z

ping @vieux @abronan

This is awesome - we have exactly that same code in Swarm. We could replace it with this pkg.

I agree with @stevvooe - tlsconfig or something less generic.

tiborvass · 2015-06-01T20:25:42Z

@aluzzardi @stevvooe I can rename it to tlsconfig and have the functions be New(), Client() and Server()

tiborvass · 2015-06-01T20:35:34Z

Renamed the package from tlsutil to tlsconfig, and changed the name of the functions as well to make them shorter.

stevvooe · 2015-06-01T20:38:01Z

There are still a few docs and naming issues but they don't matter.

LGTM!

thaJeztah · 2015-06-01T20:44:39Z

pkg/tlsconfig/config.go

(OCD warning ;-)) change to "Could not load ...", to be consistent with the other errors? (Don't forget the related test)

NathanMcCauley · 2015-06-05T02:50:22Z

LGTM

dmcgowan · 2015-06-05T15:21:15Z

docker/docker.go

This logic is backwards, it should only check this if the flag is NOT set. When the flag is set, then not exists should cause an error later on. This should only check if the default file exists if the flag is not set.

nice catch... fixing

icecrime · 2015-06-05T16:02:13Z

@jfrazelle Still LGTM for you?

jessfraz · 2015-06-05T16:03:27Z

re LGTM

icecrime · 2015-06-05T16:05:48Z

Waiting on CI.

estesp · 2015-06-05T16:22:24Z

I'm testing the actual options based on the docs list of supported settings. But if someone else has already finished that then I'm fine with the existing LGTMs

Sent from my iPhone

On Jun 5, 2015, at 12:06 PM, Arnaud Porterie notifications@github.com wrote:

Waiting on CI.

—
Reply to this email directly or view it on GitHub.

icecrime · 2015-06-05T16:30:55Z

Well, tests fail: back to 2-needs-code-review, ping @tiborvass.

This patch creates a new `tlsconfig` package to handle creation of secure-enough TLS configurations for clients and servers. The package was created by refactoring TLS code in the client and the daemon. After this patch, it is expected that all code creating TLS configurations use this `tlsconfig` package for greater security, consistency and readability. On the server side, this fixes a bug where --tlsverify was not taken into account. Now, if specified, it will require the client to authenticate. Signed-off-by: Tibor Vass <tibor@docker.com>

tiborvass · 2015-06-05T16:40:01Z

Fixed the failing test.

dmcgowan · 2015-06-05T17:02:08Z

LGTM

Refactor TLS code with a new `tlsconfig` package

ewindisch · 2015-06-05T18:06:54Z

Great. LGTM, although I'd love to see the server and client have separate defaults. That could easily come in a follow-up patch.

GordonTheTurtle added the status/0-triage label Jun 1, 2015

tiborvass added status/2-code-review and removed status/0-triage labels Jun 1, 2015

tiborvass reviewed Jun 1, 2015
View reviewed changes

estesp reviewed Jun 1, 2015
View reviewed changes

tiborvass force-pushed the refactor-tls branch from 972a397 to cdd5619 Compare June 1, 2015 19:20

tiborvass force-pushed the refactor-tls branch 2 times, most recently from 5ddf913 to 5a9f05e Compare June 1, 2015 19:53

tiborvass reviewed Jun 1, 2015
View reviewed changes

tiborvass force-pushed the refactor-tls branch from 5a9f05e to dc6cafa Compare June 1, 2015 20:06

stevvooe reviewed Jun 1, 2015
View reviewed changes

tiborvass force-pushed the refactor-tls branch from dc6cafa to 7b3fe58 Compare June 1, 2015 20:34

tiborvass changed the title ~~Refactor TLS code with a new tlsutil package~~ Refactor TLS code with a new tlsconfig package Jun 1, 2015

thaJeztah reviewed Jun 1, 2015
View reviewed changes

tiborvass force-pushed the refactor-tls branch from 7b3fe58 to 13e669e Compare June 1, 2015 20:47

tiborvass force-pushed the refactor-tls branch 2 times, most recently from 82b06b8 to 855db5a Compare June 5, 2015 02:25

tiborvass force-pushed the refactor-tls branch 2 times, most recently from ccb860f to 6460f90 Compare June 5, 2015 12:37

dmcgowan reviewed Jun 5, 2015
View reviewed changes

tiborvass force-pushed the refactor-tls branch from 6460f90 to 4484504 Compare June 5, 2015 15:47

icecrime added status/4-merge and removed status/2-code-review labels Jun 5, 2015

icecrime added status/2-code-review and removed status/4-merge labels Jun 5, 2015

tiborvass force-pushed the refactor-tls branch from 4484504 to bfed4b7 Compare June 5, 2015 16:38

icecrime added status/4-merge and removed status/2-code-review labels Jun 5, 2015

icecrime pushed a commit that referenced this pull request Jun 5, 2015

Merge pull request #13636 from tiborvass/refactor-tls

efe5c64

Refactor TLS code with a new `tlsconfig` package

icecrime merged commit efe5c64 into moby:master Jun 5, 2015

tiborvass mentioned this pull request Jun 5, 2015

Replacing -d/--daemon with daemon command. Implementation of 'docker daemon' command #10535

Closed

tiborvass mentioned this pull request Jun 16, 2015

Fix DOCKER_TLS_VERIFY being ignored #13970

Merged

tiborvass deleted the refactor-tls branch July 17, 2019 00:34

Conversation

tiborvass commented Jun 1, 2015

Uh oh!

calavera commented Jun 1, 2015

Uh oh!

tiborvass commented Jun 1, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diogomonica commented Jun 1, 2015

Uh oh!

aluzzardi commented Jun 1, 2015

Uh oh!

ehazlett commented Jun 1, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathanleclaire commented Jun 1, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevvooe commented Jun 1, 2015

Uh oh!

aluzzardi commented Jun 1, 2015

Uh oh!

tiborvass commented Jun 1, 2015

Uh oh!

tiborvass commented Jun 1, 2015

Uh oh!

stevvooe commented Jun 1, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NathanMcCauley commented Jun 5, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

icecrime commented Jun 5, 2015

Uh oh!

jessfraz commented Jun 5, 2015

Uh oh!

icecrime commented Jun 5, 2015

Uh oh!

estesp commented Jun 5, 2015

Uh oh!

icecrime commented Jun 5, 2015

Uh oh!

tiborvass commented Jun 5, 2015

Uh oh!

dmcgowan commented Jun 5, 2015

Uh oh!

ewindisch commented Jun 5, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

16 participants