Add identity-based TLS auth and UI for client/daemon auth #8265

Closed
wants to merge 20 commits into
from

Conversation

Projects
None yet
@dmcgowan
Member

dmcgowan commented Sep 26, 2014

Add identity-based TLS authentication and use public keys to authenticate access.
This is an implementation of the proposal from #7667.

FSNotify support has not been added to detect changes to authorized keys but may be added in a follow up PR.

@dmp42 dmp42 added this to the 1.3.0 milestone Sep 26, 2014

docker/docker.go
@@ -53,6 +57,7 @@ func main() {
}
flHosts = append(flHosts, defaultHost)
}
+ *flTlsVerify = true

This comment has been minimized.

@dmcgowan

dmcgowan Sep 30, 2014

Member

@vieux seems the flag is not used, just the variable. This could definitely be cleaned up.

@dmcgowan

dmcgowan Sep 30, 2014

Member

@vieux seems the flag is not used, just the variable. This could definitely be cleaned up.

@dmcgowan

This comment has been minimized.

Show comment
Hide comment
@dmcgowan

dmcgowan Sep 30, 2014

Member

Updated authorized keys and allowed hosts flags according to @bfirsh recommendation

Member

dmcgowan commented Sep 30, 2014

Updated authorized keys and allowed hosts flags according to @bfirsh recommendation

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Sep 30, 2014

Collaborator

@dmcgowan error in tests: 2014/09/30 00:45:14 TLS Handshake error: tls: oversized record received with length 20527

Collaborator

vieux commented Sep 30, 2014

@dmcgowan error in tests: 2014/09/30 00:45:14 TLS Handshake error: tls: oversized record received with length 20527

@dmcgowan

This comment has been minimized.

Show comment
Hide comment
@dmcgowan

dmcgowan Oct 1, 2014

Member

@vieux the integration tests have been fixed

Member

dmcgowan commented Oct 1, 2014

@vieux the integration tests have been fixed

api/server/server.go
@@ -1457,8 +1516,8 @@ func ListenAndServe(proto, addr string, job *engine.Job) error {
// Basic error and sanity checking
switch proto {
case "tcp":
- if !strings.HasPrefix(addr, "127.0.0.1") && !job.GetenvBool("TlsVerify") {
- log.Infof("/!\\ DON'T BIND ON ANOTHER IP ADDRESS THAN 127.0.0.1 IF YOU DON'T KNOW WHAT YOU'RE DOING /!\\")
+ if !strings.HasPrefix(addr, "127.0.0.1") && job.GetenvBool("Insecure") {

This comment has been minimized.

@discordianfish

discordianfish Oct 3, 2014

Contributor

I think we should just print the warning everytime you use insecure

@discordianfish

discordianfish Oct 3, 2014

Contributor

I think we should just print the warning everytime you use insecure

This comment has been minimized.

@dmcgowan

dmcgowan Oct 3, 2014

Member

Hmm, I agree we are trying to steer users away from using non-TLS on any tcp connection. Let me think more about how the wording will need to change. Should definitely be switched back to log.Infof, not sure why it changed to Debugf.

@dmcgowan

dmcgowan Oct 3, 2014

Member

Hmm, I agree we are trying to steer users away from using non-TLS on any tcp connection. Let me think more about how the wording will need to change. Should definitely be switched back to log.Infof, not sure why it changed to Debugf.

This comment has been minimized.

@dmcgowan

dmcgowan Oct 3, 2014

Member

maybe "DON'T BIND INSECURELY ON A TCP ADDRESS IF YOU DON'T KNOW WHAT YOU'RE DOING" and the check is just for "Insecure"

@dmcgowan

dmcgowan Oct 3, 2014

Member

maybe "DON'T BIND INSECURELY ON A TCP ADDRESS IF YOU DON'T KNOW WHAT YOU'RE DOING" and the check is just for "Insecure"

This comment has been minimized.

@jessfraz

jessfraz Oct 3, 2014

Contributor

LOL

@jessfraz

jessfraz Oct 3, 2014

Contributor

LOL

docker/docker.go
+ tlsConfig.InsecureSkipVerify = true
+ if testConn, connErr := tls.Dial(protoAddrParts[0], protoAddrParts[1], &tlsConfig); connErr != nil {
+ log.Fatalf("TLS Handshake error: %s", connErr)
+ } else {

This comment has been minimized.

@discordianfish

discordianfish Oct 3, 2014

Contributor

If we fatal we don't need the else, no?

@discordianfish

discordianfish Oct 3, 2014

Contributor

If we fatal we don't need the else, no?

docker/docker.go
+ if i == 0 {
+ continue
+ }
+ opts.Intermediates.AddCert(cert)

This comment has been minimized.

@discordianfish

discordianfish Oct 3, 2014

Contributor

Does that mean we read certs from a possible insecure tls connection?

@discordianfish

discordianfish Oct 3, 2014

Contributor

Does that mean we read certs from a possible insecure tls connection?

This comment has been minimized.

@dmcgowan

dmcgowan Oct 3, 2014

Member

Yes, the idea is the verification is run manually after the connection is established. If the verification fails (meaning the tls connection truly is insecure because it is from an unknown host), the user is prompted and allowed to to add the public key of the remote server to the allowed hosts. The biggest drawback of this approach is that there is no other way (right now) for the user to verify the host except by looking at the fingerprint of the public key.

@dmcgowan

dmcgowan Oct 3, 2014

Member

Yes, the idea is the verification is run manually after the connection is established. If the verification fails (meaning the tls connection truly is insecure because it is from an unknown host), the user is prompted and allowed to to add the public key of the remote server to the allowed hosts. The biggest drawback of this approach is that there is no other way (right now) for the user to verify the host except by looking at the fingerprint of the public key.

api/server/server.go
+ }
+ trustKey, keyErr := libtrust.LoadKeyFile(trustKeyFile)
+ if keyErr == libtrust.ErrKeyFileDoesNotExist {
+ trustKey, keyErr = libtrust.GenerateECP256PrivateKey()

This comment has been minimized.

@proppy

proppy Oct 3, 2014

Contributor

can you add a different error message when the keygen is failing? currently it would print Error loading key file below.

@proppy

proppy Oct 3, 2014

Contributor

can you add a different error message when the keygen is failing? currently it would print Error loading key file below.

api/server/server.go
+ if !os.IsNotExist(err) {
+ return fmt.Errorf("Couldn't load X509 key pair (%s, %s): %s. Key encrypted?",
+ tlsCert, tlsKey, err)
+ } else {

This comment has been minimized.

@proppy

proppy Oct 3, 2014

Contributor

no need for else here, since you have an early return.

@proppy

proppy Oct 3, 2014

Contributor

no need for else here, since you have an early return.

api/server/server.go
+ domains = []string{"docker"}
+ } else {
+ domains = []string{host, "docker"}
+ }

This comment has been minimized.

@proppy

proppy Oct 3, 2014

Contributor

maybe you could split all of this in a helper function parseAddr?

@proppy

proppy Oct 3, 2014

Contributor

maybe you could split all of this in a helper function parseAddr?

api/server/server.go
return fmt.Errorf("Couldn't read CA certificate: %s", err)
+ } else {

This comment has been minimized.

@proppy

proppy Oct 3, 2014

Contributor

no need for else, since you have an early return.

@proppy

proppy Oct 3, 2014

Contributor

no need for else, since you have an early return.

api/server/server.go
+ if err != nil {
+ return fmt.Errorf("Error creating directory: %s", err)
+ }
+ trustKey, keyErr := libtrust.LoadKeyFile(trustKeyFile)

This comment has been minimized.

@jessfraz

jessfraz Oct 10, 2014

Contributor

is there a reason this isn't just named err

@jessfraz

jessfraz Oct 10, 2014

Contributor

is there a reason this isn't just named err

This comment has been minimized.

@dmcgowan

dmcgowan Oct 10, 2014

Member

not likely a good one, just didn't get changed when refactored, I can updated

@dmcgowan

dmcgowan Oct 10, 2014

Member

not likely a good one, just didn't get changed when refactored, I can updated

api/server/server.go
+ }
+ // add default docker domain for docker clients to look for
+ domains = append(domains, "docker")
+ x509Cert, certErr := libtrust.GenerateSelfSignedServerCert(trustKey, domains, ips)

This comment has been minimized.

@jessfraz

jessfraz Oct 10, 2014

Contributor

same with this being named certErr

@jessfraz

jessfraz Oct 10, 2014

Contributor

same with this being named certErr

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass Oct 22, 2014

Collaborator

@dmcgowan Needs rebase + nits correction

Collaborator

tiborvass commented Oct 22, 2014

@dmcgowan Needs rebase + nits correction

@dmcgowan

This comment has been minimized.

Show comment
Hide comment
@dmcgowan

dmcgowan Oct 22, 2014

Member

Rebased and fixed changes. Another commit was added to put back what was removed from #8563. The reason for the removal (accidental key file root ownership) has still not been address though and likely should be address before merging this.

Member

dmcgowan commented Oct 22, 2014

Rebased and fixed changes. Another commit was added to put back what was removed from #8563. The reason for the removal (accidental key file root ownership) has still not been address though and likely should be address before merging this.

@SvenDowideit

This comment has been minimized.

Show comment
Hide comment
@SvenDowideit

SvenDowideit Oct 29, 2014

Contributor

seems to need docs to go with it.

Contributor

SvenDowideit commented Oct 29, 2014

seems to need docs to go with it.

@bfirsh

This comment has been minimized.

Show comment
Hide comment
@bfirsh

bfirsh Oct 29, 2014

Contributor

@dmcgowan This is a pretty major change to how Docker works, but this pull request doesn't make it clear what it does. Is there a document somewhere with an overview of how this system works, both from a UI and technical perspective? Perhaps we could link to that or include it directly in the pull request description here.

I think changing the title might help people understand the implications of this change too. "Add UI for client/daemon auth and enable TLS by default" maybe.

Contributor

bfirsh commented Oct 29, 2014

@dmcgowan This is a pretty major change to how Docker works, but this pull request doesn't make it clear what it does. Is there a document somewhere with an overview of how this system works, both from a UI and technical perspective? Perhaps we could link to that or include it directly in the pull request description here.

I think changing the title might help people understand the implications of this change too. "Add UI for client/daemon auth and enable TLS by default" maybe.

@bfirsh

This comment has been minimized.

Show comment
Hide comment
@bfirsh

bfirsh Oct 29, 2014

Contributor

Okay. I've had a bit of a think about this. I have written some rough documentation to figure out how to design it in a way that is easy to explain.

It seems like Docker authentication now has 3 modes:

  1. libtrust auth, which this pull request is adding
  2. certificate auth, as it works at the moment
  3. no auth and no TLS

So to make it clear to the user how it works, and to avoid having a big bundle of confusing options, I think there should be a single option to switch between these three modes. Something like --auth=(host|cert|none).

When you choose one of these modes, then different options could apply in different ways, as the documentation explains.

For backwards compatibility, the --tls and --tlsverify options could be deprecated and still work as they do. We could remove them completely in Docker 1.5.

Thoughts?

Contributor

bfirsh commented Oct 29, 2014

Okay. I've had a bit of a think about this. I have written some rough documentation to figure out how to design it in a way that is easy to explain.

It seems like Docker authentication now has 3 modes:

  1. libtrust auth, which this pull request is adding
  2. certificate auth, as it works at the moment
  3. no auth and no TLS

So to make it clear to the user how it works, and to avoid having a big bundle of confusing options, I think there should be a single option to switch between these three modes. Something like --auth=(host|cert|none).

When you choose one of these modes, then different options could apply in different ways, as the documentation explains.

For backwards compatibility, the --tls and --tlsverify options could be deprecated and still work as they do. We could remove them completely in Docker 1.5.

Thoughts?

@dmcgowan

This comment has been minimized.

Show comment
Hide comment
@dmcgowan

dmcgowan Oct 29, 2014

Member

I like the idea of having a flag which makes the authentication method explicit and making it clear which authentication mode is being used. I see the --auth flag as replacing the need for a --insecure flag. When using certification authentication, would whether or not verification be used be determined by whether a CA has been provided?

For the identity file, it will be used for more purposes than just auth so prefixing with --auth might not make sense. For the authorized keys and allowed hosts file the --auth prefix makes sense.

@shykes also expressed interest in this PR, the useability input will be useful

Member

dmcgowan commented Oct 29, 2014

I like the idea of having a flag which makes the authentication method explicit and making it clear which authentication mode is being used. I see the --auth flag as replacing the need for a --insecure flag. When using certification authentication, would whether or not verification be used be determined by whether a CA has been provided?

For the identity file, it will be used for more purposes than just auth so prefixing with --auth might not make sense. For the authorized keys and allowed hosts file the --auth prefix makes sense.

@shykes also expressed interest in this PR, the useability input will be useful

@dmcgowan dmcgowan changed the title from Tls libtrust auth to Enable TLS by default and add UI for client/daemon auth Oct 29, 2014

@bfirsh bfirsh referenced this pull request in bfirsh/docker Oct 29, 2014

Closed

Add softlayer driver #5

@wking wking referenced this pull request in docker/docker-registry Oct 29, 2014

Closed

Error 401 when uploading through Apache SSL proxy #660

@SvenDowideit

This comment has been minimized.

Show comment
Hide comment
@SvenDowideit

SvenDowideit Oct 30, 2014

Contributor

@bfirsh @dmcgowan please add that google document to this PR, so that the PR is self documenting, and that if it gets merged, we're sure what it is that is being merged. (and the PR review process will have matched updates)

Contributor

SvenDowideit commented Oct 30, 2014

@bfirsh @dmcgowan please add that google document to this PR, so that the PR is self documenting, and that if it gets merged, we're sure what it is that is being merged. (and the PR review process will have matched updates)

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Oct 30, 2014

Member

Something like --auth=(host|cert|none).

+1 good suggestion; clear way of handling this from a UX perspective

Member

thaJeztah commented Oct 30, 2014

Something like --auth=(host|cert|none).

+1 good suggestion; clear way of handling this from a UX perspective

@dmcgowan

This comment has been minimized.

Show comment
Hide comment
@dmcgowan

dmcgowan Oct 30, 2014

Member

@SvenDowideit I will update the https documentation under docs/articles/https.md. Is there any other docs you think this should touch, other than the cli docs.

Member

dmcgowan commented Oct 30, 2014

@SvenDowideit I will update the https documentation under docs/articles/https.md. Is there any other docs you think this should touch, other than the cli docs.

@SvenDowideit

This comment has been minimized.

Show comment
Hide comment
@SvenDowideit

SvenDowideit Oct 31, 2014

Contributor

@dmcgowan sounds perfect to me :)

Contributor

SvenDowideit commented Oct 31, 2014

@dmcgowan sounds perfect to me :)

docs/sources/articles/https.md
+and remember it for future connections.
+ - **Certificate-based authentication** uses a certificate authority to
+authorize connections. Using this method requires additional setup to enable
+client authentication.

This comment has been minimized.

@SvenDowideit

SvenDowideit Oct 31, 2014

Contributor

might be worth mentioning that this second is what was TLS in 1.3 and before

@SvenDowideit

SvenDowideit Oct 31, 2014

Contributor

might be worth mentioning that this second is what was TLS in 1.3 and before

This comment has been minimized.

@bfirsh

bfirsh Nov 3, 2014

Contributor

👍

@bfirsh

bfirsh Nov 3, 2014

Contributor

👍

@SvenDowideit

This comment has been minimized.

Show comment
Hide comment
@SvenDowideit

SvenDowideit Oct 31, 2014

Contributor

much yay - Docs LGTM, but then I already know about this feature a little - @fredlf @jamtur01 ?

Contributor

SvenDowideit commented Oct 31, 2014

much yay - Docs LGTM, but then I already know about this feature a little - @fredlf @jamtur01 ?

@@ -5,11 +5,58 @@ page_keywords: docker, docs, article, example, https, daemon, tls, ca, certifica
# Running Docker with https

This comment has been minimized.

@bfirsh

bfirsh Oct 31, 2014

Contributor

I wonder whether these docs would be better called "Controlling Docker remotely" or "Securing connections between the client and daemon". HTTPS seems like the means rather than the problem a user is trying to solve.

@bfirsh

bfirsh Oct 31, 2014

Contributor

I wonder whether these docs would be better called "Controlling Docker remotely" or "Securing connections between the client and daemon". HTTPS seems like the means rather than the problem a user is trying to solve.

This comment has been minimized.

@dmcgowan

dmcgowan Oct 31, 2014

Member

Agreed although this particular article seems more focused on the means. Not sure what the difference between this and a userguide would be, I'll let @fredlf chime in for clarity.

@dmcgowan

dmcgowan Oct 31, 2014

Member

Agreed although this particular article seems more focused on the means. Not sure what the difference between this and a userguide would be, I'll let @fredlf chime in for clarity.

@@ -5,11 +5,58 @@ page_keywords: docker, docs, article, example, https, daemon, tls, ca, certifica
# Running Docker with https
By default, Docker runs via a non-networked Unix socket. It can also

This comment has been minimized.

@bfirsh

bfirsh Oct 31, 2014

Contributor

It isn't via a Unix socket on boot2docker or host management, which is the "default" for a lot of users. It might be worth explaining that this is how it works by default on boot2docker (and I can amend the docs for host management ;).

@bfirsh

bfirsh Oct 31, 2014

Contributor

It isn't via a Unix socket on boot2docker or host management, which is the "default" for a lot of users. It might be worth explaining that this is how it works by default on boot2docker (and I can amend the docs for host management ;).

This comment has been minimized.

@dmcgowan

dmcgowan Nov 7, 2014

Member

@SvenDowideit what do you think is the best way to explain this

@dmcgowan

dmcgowan Nov 7, 2014

Member

@SvenDowideit what do you think is the best way to explain this

This comment has been minimized.

@SvenDowideit

SvenDowideit Nov 14, 2014

Contributor

Given the docker hosts and other cluster Docker setups we're seeing, I'd just drop 'defaults' and re-write to something like

Docker can communicate locally using a unix socket or systemd socket (or whatever that thing's called), or, can communicate using a HTTP socket.
@SvenDowideit

SvenDowideit Nov 14, 2014

Contributor

Given the docker hosts and other cluster Docker setups we're seeing, I'd just drop 'defaults' and re-write to something like

Docker can communicate locally using a unix socket or systemd socket (or whatever that thing's called), or, can communicate using a HTTP socket.

This comment has been minimized.

@tianon

tianon Nov 20, 2014

Member

FWIW, systemd socket (or whatever that thing's called) is also "just" a unix socket (passed to Docker itself as a file descriptor) -- the only difference is that systemd manages/"owns" it.

@tianon

tianon Nov 20, 2014

Member

FWIW, systemd socket (or whatever that thing's called) is also "just" a unix socket (passed to Docker itself as a file descriptor) -- the only difference is that systemd manages/"owns" it.

docker/flags.go
- trustKeyDefault := filepath.Join(dockerCertPath, defaultTrustKeyFile)
- flTrustKey = &trustKeyDefault
-
+ flTrustHosts = flag.String([]string{"-allowed-hosts-file"}, filepath.Join(dockerCertPath, defaultHostKeysFile), "Path to file containing allowed hosts")

This comment has been minimized.

@bfirsh

bfirsh Oct 31, 2014

Contributor

What's the reasoning behind using "allowed hosts" rather than "known hosts" to match SSH?

@bfirsh

bfirsh Oct 31, 2014

Contributor

What's the reasoning behind using "allowed hosts" rather than "known hosts" to match SSH?

This comment has been minimized.

@dmcgowan

dmcgowan Oct 31, 2014

Member

I think we initially thought calling it known hosts may cause confusion with SSH especially with the difference in formats. There really is no good reason not to call it known hosts since the behavior is so similar.

@dmcgowan

dmcgowan Oct 31, 2014

Member

I think we initially thought calling it known hosts may cause confusion with SSH especially with the difference in formats. There really is no good reason not to call it known hosts since the behavior is so similar.

docs/sources/articles/https.md
+
+## Host-based authentication
+
+Host-based authentication is similar to how SSH does authentication. When

This comment has been minimized.

@bfirsh

bfirsh Oct 31, 2014

Contributor

Maybe this could be "identity" based authentication?

The upside of "host-based" is that is the language that SSH uses and it's actually based on host names. The upside of "identity" is that a user will think – oh, it uses the --identity option.

@bfirsh

bfirsh Oct 31, 2014

Contributor

Maybe this could be "identity" based authentication?

The upside of "host-based" is that is the language that SSH uses and it's actually based on host names. The upside of "identity" is that a user will think – oh, it uses the --identity option.

This comment has been minimized.

@dmcgowan

dmcgowan Oct 31, 2014

Member

I think you might be right about calling it "identity" based. What is not touched in this article (because its probably not the right place) is that the identity is also what would be used for build signature and what is registered via login to tie a hub user to that public key. It is probably more analogous to identity such as a PGP key than a host login key just for SSH.

@dmcgowan

dmcgowan Oct 31, 2014

Member

I think you might be right about calling it "identity" based. What is not touched in this article (because its probably not the right place) is that the identity is also what would be used for build signature and what is registered via login to tie a hub user to that public key. It is probably more analogous to identity such as a PGP key than a host login key just for SSH.

@tianon

This comment has been minimized.

Show comment
Hide comment
@tianon

tianon Jan 3, 2015

Member

I'm indeed +1 on the daemon saving configuration-type data to /etc, since if I want to back up the essence of a host, /etc is one of the things that will be backed up (basically exactly the same to how SSHd handles this, which should be our model to emulate for this kind of stuff IMO). I definitely won't be backing up /var/lib/docker any time soon. 😉

Member

tianon commented Jan 3, 2015

I'm indeed +1 on the daemon saving configuration-type data to /etc, since if I want to back up the essence of a host, /etc is one of the things that will be backed up (basically exactly the same to how SSHd handles this, which should be our model to emulate for this kind of stuff IMO). I definitely won't be backing up /var/lib/docker any time soon. 😉

@dmcgowan

This comment has been minimized.

Show comment
Hide comment
@dmcgowan

dmcgowan Jan 6, 2015

Member

Rebased

Member

dmcgowan commented Jan 6, 2015

Rebased

docs/sources/articles/authentication.md
+and remember it for future connections.
+ - **Certificate-based authentication** uses a Certificate Authority to
+authorize connections. Using this method requires additional setup to enable
+client authentication.

This comment has been minimized.

@fredlf

fredlf Jan 6, 2015

Contributor

This should be a numbered list.

@fredlf

fredlf Jan 6, 2015

Contributor

This should be a numbered list.

docs/sources/articles/authentication.md
-optionally communicate using a HTTP socket.
+optionally communicate using a HTTP socket. Enabling communication
+through HTTP will by default use TLS and require daemon configuration
+to allow client connections.

This comment has been minimized.

@fredlf

fredlf Jan 6, 2015

Contributor

I think we need to separate out some of the info packed in this sentence. Something like: By default, communication over HTTP uses TLS. To allow client connections, you also need to configure the Docker daemon.

@fredlf

fredlf Jan 6, 2015

Contributor

I think we need to separate out some of the info packed in this sentence. Something like: By default, communication over HTTP uses TLS. To allow client connections, you also need to configure the Docker daemon.

This comment has been minimized.

@dmcgowan

dmcgowan Jan 7, 2015

Member

I am thinking I should just remove this additional sentence for now since we pulled back on making it the default. Not sure how to make this sentence useful if it is not describing the default behavior.

@dmcgowan

dmcgowan Jan 7, 2015

Member

I am thinking I should just remove this additional sentence for now since we pulled back on making it the default. Not sure how to make this sentence useful if it is not describing the default behavior.

docs/sources/articles/authentication.md
+
+The authentication method is selected using the `--auth` flag with values
+ `identity`, `cert`, or `none` . The `none` method is currently the default but
+`identity` will become the default in a future version.

This comment has been minimized.

@fredlf

fredlf Jan 6, 2015

Contributor

I don't think we should speculate about future dev in the docs. We just need to state what things are now.

@fredlf

fredlf Jan 6, 2015

Contributor

I don't think we should speculate about future dev in the docs. We just need to state what things are now.

docs/sources/articles/authentication.md
+## Identity-based authentication
+
+Identity-based authentication is similar to how SSH does authentication. When
+connecting to a daemon for the first time, a client will ask whether a user

This comment has been minimized.

@fredlf

fredlf Jan 6, 2015

Contributor

Docker daemon

@fredlf

fredlf Jan 6, 2015

Contributor

Docker daemon

+
+To enable identity-based authentication, add the flag `--auth=identity`.
+The default identity and authorization files may be overridden through the
+flags:

This comment has been minimized.

@fredlf

fredlf Jan 6, 2015

Contributor

Should we state what/where the defaults are? Just a thought.

@fredlf

fredlf Jan 6, 2015

Contributor

Should we state what/where the defaults are? Just a thought.

This comment has been minimized.

@dmcgowan

dmcgowan Jan 7, 2015

Member

Added the defaults, will make it more clear now that the defaults are different for client and daemon.

@dmcgowan

dmcgowan Jan 7, 2015

Member

Added the defaults, will make it more clear now that the defaults are different for client and daemon.

docs/sources/articles/authentication.md
+
+Certificate-based authentication uses TLS certificates provided by a
+Certificate Authority (CA). This is for advanced usage where you may want to
+integrate Docker with other TLS-compatible tools or you may already use PKI

This comment has been minimized.

@fredlf

fredlf Jan 6, 2015

Contributor

Expand acronym: public key infrastructure (PKI)

@fredlf

fredlf Jan 6, 2015

Contributor

Expand acronym: public key infrastructure (PKI)

docs/sources/articles/authentication.md
+Certificate Authority (CA). This is for advanced usage where you may want to
+integrate Docker with other TLS-compatible tools or you may already use PKI
+within your organisation. You can just get the client to verify the server’s
+certificate against a CA, or do full two-way authentication by getting the

This comment has been minimized.

@fredlf

fredlf Jan 6, 2015

Contributor

Change to "You can get the client to just verify..."

@fredlf

fredlf Jan 6, 2015

Contributor

Change to "You can get the client to just verify..."

@fredlf

This comment has been minimized.

Show comment
Hide comment
@fredlf

fredlf Jan 7, 2015

Contributor

A few minor comments/revisions, otherwise docs LGTM. Nice work! Many thanks.

Contributor

fredlf commented Jan 7, 2015

A few minor comments/revisions, otherwise docs LGTM. Nice work! Many thanks.

dmcgowan and others added some commits Oct 22, 2014

Add default TLS using engine keys
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Update https documentation
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Move default flag values to flags.go
Signed-off-by: Ben Firshman <ben@firshman.co.uk>
Add cli tests for existing TLS behaviour
- Removed integration tests which are covered by new tests.
- Disabled tests on Drone for same reason as 07cedaa

Signed-off-by: Ben Firshman <ben@firshman.co.uk>
Fail integration CLI tests if daemon exits
Signed-off-by: Ben Firshman <ben@firshman.co.uk>
Add auth option to switch between auth methods
- Can switch between "identity", "cert" and "none"
- Replaced --insecure with --auth=none
- Moved logic to generate TLS configs to separate auth files
- Renamed --tlscert -> --auth-cert
- Renamed --tlskey -> --auth-key
- Renamed --tlscacert -> --auth-ca
- Add DOCKER_AUTH_{CERT,KEY,CA} envvars for configuring cert auth.
- Added backwards compatibility for --tls... options. The existing tests
  for this have been left to ensure correct behaviour.

The DOCKER_CERT_PATH environment variable has been disabled for these
new options. Changing behaviour on whether files exist in the default
location (~/.docker/cert.pem, etc) seems dangerous if these files
are accidentally missing. This option was added in the first place to
make boot2docker work with cert auth, but now we have identity auth,
this is unnecessary.

Signed-off-by: Ben Firshman <ben@firshman.co.uk>
Update auth flags and documentation
- Change allowed-hosts to known-hosts since known hosts is a more familiar concept than allowed hosts for similar functionality
- Add deprecation warning to old tls flags
- Use dash for key files instead of underscore for consistency with other key files

Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Create public key file along with private key
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Add support for authorized key directory
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Add note to docs about deprecated options
Signed-off-by: Ben Firshman <ben@firshman.co.uk>
Remove flag and reference to authorized keys file
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Fix documentation style
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Correctly document new auth environment variables
Signed-off-by: Ben Firshman <ben@firshman.co.uk>
Add commas to list of extensions
Signed-off-by: Ben Firshman <ben@firshman.co.uk>
Rename HTTPS docs to authentication
Signed-off-by: Ben Firshman <ben@firshman.co.uk>
add redirect for https docs to authentication
Docker-DCO-1.1-Signed-off-by: Sven Dowideit <SvenDowideit@docker.com> (github: SvenDowideit)
Use path/filepath instead of path (for Windows)
the code is using path.Split, path.Join etc. instead of filepath.Split, filepath.Join. path assumes '/' separators, but Windows of course uses '\'. Talk about the gift that keeps on giving...I've been fixing bugs like this in portable code since 1983.

See docker/machine#76

Signed-off-by: John Gossman <johngos@microsoft.com>
Refactor large blocks and clean up code
Signed-off-by: Derek McGowan <derek@mcgstyle.net>
Use separate defaults for client and daemon
Update the flag processor to use the HOME directory for client configuration file defaults and the /etc directory for daemon configuration file defaults

Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Add recommended revisions to authentication documentation
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
@icecrime

This comment has been minimized.

Show comment
Hide comment
@icecrime

icecrime Jan 8, 2015

Contributor

Considering the amount of review required, and the extra eyes that security related PR mandates, I'm removing the 1.5 milestone (freezes on Jan 20th).

Contributor

icecrime commented Jan 8, 2015

Considering the amount of review required, and the extra eyes that security related PR mandates, I'm removing the 1.5 milestone (freezes on Jan 20th).

@icecrime icecrime removed this from the 1.5.0 milestone Jan 8, 2015

+ if _, ok := err.(x509.UnknownAuthorityError); ok {
+ if ca, err := checkTrustedPeer(trustKey, certs[0], addr, knownHostsPath); err != nil {
+ return nil, err
+ } else {

This comment has been minimized.

@ewindisch

ewindisch Jan 8, 2015

Contributor

This else is unnecessary and reduces readability

@ewindisch

ewindisch Jan 8, 2015

Contributor

This else is unnecessary and reduces readability

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Jan 27, 2015

Contributor

@bfirsh can you please get us some information on the reasons why on this approach? We can continue this discussion on the original proposal.

Please reopen when it is ready and that auth code is reviewed by the qualified parties.

Contributor

crosbymichael commented Jan 27, 2015

@bfirsh can you please get us some information on the reasons why on this approach? We can continue this discussion on the original proposal.

Please reopen when it is ready and that auth code is reviewed by the qualified parties.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment