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

use an encrypted client certificate to connect to a docker daemon #31364

Conversation

@adshmh
Copy link
Contributor

commented Feb 26, 2017

An encrypted client certificate can be used to connect to a docker daemon.

Signed-off-by: Arash Deshmeh adeshmeh@ca.ibm.com

- What I did
This is a first attempt at fixing #30935. As discussed on #30935, the first draft of the PR includes changes to vendor repositories (docker/go-connections and docker/notary), to allow review of all the required changes. These will be submitted as PRs on corresponding repos after the approval of
the changes.

- How I did it

  • Added code to go-connections repo to make use of existing functionaliy in notary for decrypting private keys.
  • Added code for passing function for getting password from the client to the go-connections (and to notary)
    - added this as an option in tlsconfig options.
  • Added code for the functionality to be enabled only if a client-specific flag is present (named it 'tlsgetpass' for now)

- How to verify it

  • To reproduce, as shown in #30935, follow the tutorial here : https://docs.docker.com/engine/security/https/ , but generate an encrypted private key:
    $ openssl genrsa -aes256 -out key.pem 4096

  • Current behavior: running a docker command (include -H ) will result in an error, e.g.:
    $ docker --tlsverify --tlscacert=ca.pem --tlscert=cert.pem --tlskey=key.pem version
    Could not load X509 key pair: tls: failed to parse private key. Make sure the key is not encrypted

  • With this PR (include -H for host/port)
    $ docker --tlsverify --tlscacert=ca.pem --tlscert=cert.pem --tlskey=key.pem version
    Enter passphrase for key with ID private: ******** (passphrase for the private key should be entered)
    Client:
    Version: 17.04.0-dev
    API version: 1.26 (downgraded from 1.27)
    Go version: go1.7.5
    Git commit: ffc70d2
    Built: Sat Feb 25 20:14:54 2017
    OS/Arch: linux/amd64

Server:
Version: 1.14.0-dev
API version: 1.26 (minimum version 1.12)
Go version: go1.7.3
Git commit: 1b63529
Built: Wed Nov 30 08:16:55 2016
OS/Arch: linux/amd64
Experimental: false

- Description for the changelog
Encrypted client certificate can be used to connect to a docker daemon.

- A picture of a cute animal (not mandatory but encouraged)

@adshmh

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2017

note: the reason for introducing --tlsgetpass command option is to avoid breaking existing uses (e.g. without this option, a command with incorrect private key will not fail, instead will show a prompt for the passphrase). Some integration tests will also fail without this option (e.g. any test that passes an intentionally invalid key).

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Feb 28, 2017

ping @cyli @endophage PTAL

@@ -13,6 +13,9 @@ import (
"os"

"github.com/Sirupsen/logrus"
"github.com/docker/notary"

This comment has been minimized.

Copy link
@cyli

cyli Mar 1, 2017

Contributor

We should probably not vendor notary into go-connections, because notary also depends upon go-connections :) I think the goal is probably for go-connections to have as few dependencies as possible.

This comment has been minimized.

Copy link
@adshmh

adshmh Mar 1, 2017

Author Contributor

Thank you for the review. I will update the PR to remove the dependency on notary and include the TLS keys handling logic in go-connections instead.

@@ -28,7 +31,8 @@ type Options struct {
// client-only option
InsecureSkipVerify bool
// server-only option
ClientAuth tls.ClientAuthType
ClientAuth tls.ClientAuthType
PassphraseRetriever notary.PassRetriever `json:"-"`

This comment has been minimized.

Copy link
@cyli

cyli Mar 1, 2017

Contributor

Rather than go-connections taking a passphrase retriever, it could take an optional passphrase for the key. It'd be up to the caller to get it however they deem fit.

return tls.Certificate{}, fmt.Errorf(errTemplate, err)
}

if options.PassphraseRetriever != nil {

This comment has been minimized.

Copy link
@cyli

cyli Mar 1, 2017

Contributor

Rather than get the passphrase retriever here, since the TLS key must be PEM encoded (tls.LoadX509KeyPair also requires PEM encoded files), maybe we can just decode (using pem.Decode) the contents, determine if it's encrypted (x509.IsEncryptedPEMBlock), and if it is attempt to decrypt it using the passphrase (x509.DecryptPEMBlock). If that fails, return an error that indicates that the key was encrypted. If it succeeds, then we have the key.

That way the caller (docker in this case) could determine whether or not to fail, because the key was just malformed garbage, or to attempt to use a passphrase retriever because the key is encrypted.

@cyli

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2017

Thanks for working on this @adshmh - I have a couple comments about where things should be called from, etc., but appreciate the draft and look forward to the functionality!

@adshmh adshmh force-pushed the adshmh:30935-use-encrypted-client-certificate-to-connect-to-a-docker-host branch from 4d85c2b to cc412bd Mar 6, 2017

@@ -57,6 +57,7 @@ func newDockerCommand(dockerCli *command.DockerCli) *cobra.Command {
flags = cmd.Flags()
flags.BoolVarP(&opts.Version, "version", "v", false, "Print version information and quit")
flags.StringVar(&opts.ConfigDir, "config", cliconfig.Dir(), "Location of client config files")
flags.BoolVarP(&opts.TLSGetPass, "tlsgetpass", "", cliconfig.TLSGetPass(), "Request passphrase if needed to decrypt TLS keys")

This comment has been minimized.

Copy link
@endophage

endophage Mar 7, 2017

Contributor

I'm skeptical that this needs a flag. The cert should be stored in a format the allows us to detect if it's encrypted rather than require the user to know and inform the CLI that it should request a passphrase.

This comment has been minimized.

Copy link
@adshmh

adshmh Mar 7, 2017

Author Contributor

Thank you for the review. Yes, the PR checks whether a private key is encrypted.
I added the flag as the PR seems to make a behavior change, i.e. encountering an encrypted private key will result in the CLI prompting for a passphrase, and failing in such situations may be the desired behavior in some cases? (perhaps in a shell script?)
Please advise whether you consider this a valid reason or if I should go ahead and remove the flag.

This comment has been minimized.

Copy link
@cyli

cyli Mar 7, 2017

Contributor

I think it's probably fine to just prompt for a passphrase if it was encrypted - it just returned an error before, and so long as we message that the TLS key is encrypted, and that's why they're being asked for a passphrase now, that seems to be a fine behavior change. (we probably just need to make sure the user can control-C or something out of the password prompt if they wanted).

This comment has been minimized.

Copy link
@endophage

endophage Mar 7, 2017

Contributor

I agree with @cyli. If we're replacing what was an error with more intelligent behaviour, I think that's a great improvement that almost certainly doesn't need an exposed flag, even if it's new behaviour.

This comment has been minimized.

Copy link
@adshmh

adshmh Mar 8, 2017

Author Contributor

Thank you for the reviews. I have updated the PR to remove the flag.

@adshmh adshmh force-pushed the adshmh:30935-use-encrypted-client-certificate-to-connect-to-a-docker-host branch from cc412bd to 125399c Mar 8, 2017

passwd, giveup, err = passRetriever("private", "encrypted TLS private", false, attempts)
// Check if the passphrase retriever got an error or if it is telling us to give up
if giveup || err != nil {
return nil, fmt.Errorf("private key is encrypted, but could not get passphrase: %v", err)

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann Mar 13, 2017

Contributor

This would be a good place to use errors.Wrap from github.com/pkg/errors.

This comment has been minimized.

Copy link
@endophage

endophage Mar 13, 2017

Contributor

Ooooh, I'm going to have to look at that package for Notary (cc @cyli @riyazdf: it adds stack traces to errors)

// Initialize the dockerCli runs initialization that must happen after command
// line flags are parsed.
func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions) error {
cli.configFile = LoadDefaultConfigFile(cli.err)

var err error
cli.client, err = NewAPIClientFromFlags(opts.Common, cli.configFile)
passRetriever := getPassphraseRetriever(cli, make(map[string]string))

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann Mar 13, 2017

Contributor

Would it make sense to have getPassphraseRetriever allocate and return the map itself?

This comment has been minimized.

Copy link
@endophage

endophage Mar 13, 2017

Contributor

I've just been double checking the notary code for the passphrase retrievers and it's fine to pass nil for this map value. This map is a translation layer that takes a possibly internal password identifier/name and converts it to a user friendly representation. It never gets written, only read, and it's perfectly valid to index into a nil map, you'll get get back the zero value and false.

This comment has been minimized.

Copy link
@cyli

cyli Mar 13, 2017

Contributor

Agree, I think passing nil for this (or at least doing so in getPassphraseRetriever) would be clearer.


cert, err := ioutil.ReadFile(options.CertFile)
if err != nil {
return tls.Certificate{}, fmt.Errorf(errTemplate, err)

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann Mar 13, 2017

Contributor

Use errors.Wrap for all of these.

@@ -83,6 +88,65 @@ func certPool(caFile string) (*x509.CertPool, error) {
return certPool, nil
}

func IsErrEncryptedKey(err error) bool {
return err != nil && strings.Contains(err.Error(), encryptedPrivateKeyMessage)
}

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann Mar 13, 2017

Contributor

Once you switch to errors.Wrap, getPrivateKey can return a global error variable (var errCantDecrypt = errors.New("private key is encrypted, but could not decrypt it")), and then this function can compare errors.Cause(err) == errCantDecrypt instead of looking inside the string.

This comment has been minimized.

Copy link
@endophage

endophage Mar 13, 2017

Contributor

More concerning, this is a change to a vendored file but I don't see a corresponding update to vendor.conf.

This comment has been minimized.

Copy link
@cyli

cyli Mar 13, 2017

Contributor

@endophage I think the goal is to get the implementation stubbed out, and then port the changes to go-connections:

This is a first attempt at fixing #30935. As discussed on #30935, the first draft of the PR includes changes to vendor repositories (docker/go-connections and docker/notary), to allow review of all the required changes. These will be submitted as PRs on corresponding repos after the approval of
the changes.

@aaronlehmann

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2017

ping @cyli @endophage: It looks like the PR has been updated to address your comments. Can you take a look?

@@ -83,6 +88,65 @@ func certPool(caFile string) (*x509.CertPool, error) {
return certPool, nil
}

func IsErrEncryptedKey(err error) bool {
return err != nil && strings.Contains(err.Error(), encryptedPrivateKeyMessage)
}

This comment has been minimized.

Copy link
@endophage

endophage Mar 13, 2017

Contributor

More concerning, this is a change to a vendored file but I don't see a corresponding update to vendor.conf.

@adshmh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2017

@aaronlehmann, @endophage Thank you for the reviews.

  • I will update the PR to use errors.Wrap.
  • Regarding the change to vendored file, the idea was to initially use a single PR to show all the changes in one place for easier review, and submit separate PRs to get the changes committed to corresponding repos one all changes are acceptable. I will start submitting the PR on go-connections once the changes are approved.

@adshmh adshmh force-pushed the adshmh:30935-use-encrypted-client-certificate-to-connect-to-a-docker-host branch from 125399c to 87853c2 Mar 16, 2017

@aaronlehmann

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2017

Design LGTM

@aaronlehmann

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2017

Can you please open a PR for go-connections?

@adshmh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2017

Thank you for the review. I will open the PR for go-connections.
Possibly for a separate PR, do you recommend removing the "errors" reference from cli/command/cli.go and changing the single reference to it to use pkg/errors as well?
(then the 'perrors' alias that I used for pkg/errors can also be removed)

if x509.IsEncryptedPEMBlock(pemBlock) {
keyBytes, err = x509.DecryptPEMBlock(pemBlock, []byte(passphrase))
if err != nil {
return nil, errCantDecrypt

This comment has been minimized.

Copy link
@adshmh

adshmh Mar 16, 2017

Author Contributor

I think if DecryptPEMBlock returns an error, the code here always returns the same error, i.e. errCantDecrypt. I did not find a clean way of including both the cant decrypt error and the error from DecryptPEMBlock.

This comment has been minimized.

Copy link
@cyli

cyli Mar 16, 2017

Contributor

That's true - we can probably just pass the error returned by DecryptPEMBlock, maybe wrapped like errors.Wrap(err, "private key is encrypted"). The docs from DecryptPEMBlock say:

DecryptPEMBlock takes a password encrypted PEM block and the password used to encrypt it and returns a slice of decrypted DER encoded bytes. It inspects the DEK-Info header to determine the algorithm used for decryption. If no DEK-Info header is present, an error is returned. If an incorrect password is detected an IncorrectPasswordError is returned. Because of deficiencies in the encrypted-PEM format, it's not always possible to detect an incorrect password. In these cases no error will be returned but the decrypted DER bytes will be random noise.

So in IsErrEncryptedKey maybe we can just check errors.Cause(err) == x509.IncorrectPasswordError?

This comment has been minimized.

Copy link
@adshmh

adshmh Mar 17, 2017

Author Contributor

Thanks, just updated the code. I will open a PR on go-connections for the tlsconfig/config.go (perhaps I need to add some unit tests as part of that PR for the new functionality).

This comment has been minimized.

Copy link
@cyli

cyli Mar 17, 2017

Contributor

That would be awesome, thank you.

@aaronlehmann

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2017

Possibly for a separate PR, do you recommend removing the "errors" reference from cli/command/cli.go and changing the single reference to it to use pkg/errors as well?
(then the 'perrors' alias that I used for pkg/errors can also be removed)

Yes. You can do it in this PR.

@adshmh adshmh force-pushed the adshmh:30935-use-encrypted-client-certificate-to-connect-to-a-docker-host branch from 87853c2 to 2e7606d Mar 16, 2017

@adshmh adshmh force-pushed the adshmh:30935-use-encrypted-client-certificate-to-connect-to-a-docker-host branch from 2e7606d to 2da3ffa Mar 17, 2017

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Mar 17, 2017

@aaronlehmann looks like the PR was updated, PTAL

@aaronlehmann

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2017

@thaJeztah: Still waiting for vendoring

@vdemeester

This comment has been minimized.

Copy link
Member

commented Mar 31, 2017

ping @adshmh docker/go-connections#35 is merged, can you rebase your PR and add a commit to vendor this commit of go-connections ?

@endophage

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2017

Something I should bring to everyone's attention as a consideration before merging this PR. We're actually planning to deprecate the encrypted PEMs in notary in place of PKCS#8 in the very near future (next 6-8 weeks). We didn't realize at the time but encrypted PEMs use md5 for deriving keys from passwords and we want to ultimately no longer rely on md5 for anything in notary.

We'll necessarily have to keep legacy reading code for some time, but plan to remove any code that creates encrypted PEMs.

@adshmh adshmh force-pushed the adshmh:30935-use-encrypted-client-certificate-to-connect-to-a-docker-host branch from 2da3ffa to 24f593c Apr 5, 2017

@adshmh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2017

@vdemeester Thank you for merging the go-connections PR. I have updated the PR and added a commit to vendor go-connections.

@@ -146,13 +148,38 @@ func getConfiguredCredentialStore(c *configfile.ConfigFile, serverAddress string
return c.CredentialsStore
}

func newAPIClientWithTLSKeyPass(opts *cliflags.CommonOptions, configFile *configfile.ConfigFile, passRetriever notary.PassRetriever) (client.APIClient, error) {

This comment has been minimized.

Copy link
@cpuguy83

cpuguy83 Apr 6, 2017

Contributor

Why not handle this inline rather than wrapping the function again?

@@ -279,3 +306,7 @@ func newHTTPClient(host string, tlsOptions *tlsconfig.Options) (*http.Client, er
func UserAgent() string {
return "Docker-Client/" + dockerversion.Version + " (" + runtime.GOOS + ")"
}

func getPassphraseRetriever(streams Streams) notary.PassRetriever {

This comment has been minimized.

Copy link
@cpuguy83

cpuguy83 Apr 6, 2017

Contributor

I'm not sure of the usefulness of this

This comment has been minimized.

Copy link
@adshmh

adshmh Apr 6, 2017

Author Contributor

Thank you for the review. These functions are now removed.

@adshmh adshmh force-pushed the adshmh:30935-use-encrypted-client-certificate-to-connect-to-a-docker-host branch from 24f593c to 654c000 Apr 6, 2017

passwd string
giveup bool
)
passRetriever := passphrase.PromptRetrieverWithInOut(cli.In(), cli.Out(), nil)

This comment has been minimized.

Copy link
@cpuguy83

cpuguy83 Apr 6, 2017

Contributor

Check for encrypted key error before getting a new pass retriever

This comment has been minimized.

Copy link
@adshmh

adshmh Apr 6, 2017

Author Contributor

Would it be OK if I put all the block under that condition? It results in one extra call to IsErrEncryptedKey, but perhaps it is more readable?

      if tlsconfig.IsErrEncryptedKey(err) {
                var (
                        passwd string
                        giveup bool
                )
                passRetriever := passphrase.PromptRetrieverWithInOut(cli.In(), cli.Out(), nil)

                for attempts := 0; tlsconfig.IsErrEncryptedKey(err); attempts++ {
                        // some code and comments borrowed from notary/trustmanager/keystore.go
                        passwd, giveup, err = passRetriever("private", "encrypted TLS private", false, attempts)
                        // Check if the passphrase retriever got an error or if it is telling us to give up
                        if giveup || err != nil {
                                return errors.Wrap(err, "private key is encrypted, but could not get passphrase")
                        }

                        opts.Common.TLSOptions.Passphrase = passwd
                        cli.client, err = NewAPIClientFromFlags(opts.Common, cli.configFile)
                }
        }

        if err != nil {
                return err
        }

This comment has been minimized.

Copy link
@cpuguy83

cpuguy83 Apr 7, 2017

Contributor

SGTM

adshmh added 2 commits Apr 5, 2017
vendor docker/go-connections to include the enhacement for decrypting…
… keys

Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>
use an encrypted client certificate to connect to a docker daemon
Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>

@adshmh adshmh force-pushed the adshmh:30935-use-encrypted-client-certificate-to-connect-to-a-docker-host branch from 654c000 to 603dd8b Apr 7, 2017

@adshmh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2017

@cpuguy83 Thank you for the review. I have updated the PR with the requested changes.

@cpuguy83
Copy link
Contributor

left a comment

LGTM

@vdemeester
Copy link
Member

left a comment

LGTM 🐯
/cc @n4ss @diogomonica 👼

@diogomonica

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2017

No picture of a cute animal 👎

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Apr 10, 2017

@diogomonica best I could find, good to go?

monkey-tomato

@thaJeztah
Copy link
Member

left a comment

LGTM

@thaJeztah thaJeztah merged commit 2daa2b8 into moby:master Apr 10, 2017

7 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 32668 has succeeded
Details
janky Jenkins build Docker-PRs 41278 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 1459 has succeeded
Details
vendor Jenkins build Docker-PRs-vendor 3134 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 12395 has succeeded
Details
z Jenkins build Docker-PRs-s390x 1292 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 17.05.0 milestone Apr 10, 2017

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017
Merge pull request moby#31364 from adshmh/30935-use-encrypted-client-…
…certificate-to-connect-to-a-docker-host

use an encrypted client certificate to connect to a docker daemon
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.