Skip to content

Remove support for encrypted TLS private keys #3220

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.

Sign up for GitHub

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

Merged
justincormack merged 1 commit into from
Jan 27, 2022
Merged

Remove support for encrypted TLS private keys #3220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 0 additions & 24 deletions cli/command/cli.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,7 @@ import (
"github.com/moby/term"
"github.com/pkg/errors"
"github.com/spf13/cobra"
"github.com/theupdateframework/notary"
notaryclient "github.com/theupdateframework/notary/client"
"github.com/theupdateframework/notary/passphrase"
)

// Streams is an interface which exposes the standard input and output streams
Expand Down Expand Up @@ -252,14 +250,6 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions, ops ...Initialize

if cli.client == nil {
cli.client, err = newAPIClientFromEndpoint(cli.dockerEndpoint, cli.configFile)
if tlsconfig.IsErrEncryptedKey(err) {
passRetriever := passphrase.PromptRetrieverWithInOut(cli.In(), cli.Out(), nil)
newClient := func(password string) (client.APIClient, error) {
cli.dockerEndpoint.TLSPassword = password //nolint: staticcheck // SA1019: cli.dockerEndpoint.TLSPassword is deprecated
return newAPIClientFromEndpoint(cli.dockerEndpoint, cli.configFile)
}
cli.client, err = getClientWithPassword(passRetriever, newClient)
}
if err != nil {
return err
}
Expand Down Expand Up @@ -377,20 +367,6 @@ func (cli *DockerCli) initializeFromClient() {
cli.client.NegotiateAPIVersionPing(ping)
}

func getClientWithPassword(passRetriever notary.PassRetriever, newClient func(password string) (client.APIClient, error)) (client.APIClient, error) {
for attempts := 0; ; attempts++ {
passwd, giveup, err := passRetriever("private", "encrypted TLS private", false, attempts)
if giveup || err != nil {
return nil, errors.Wrap(err, "private key is encrypted, but could not get passphrase")
}

apiclient, err := newClient(passwd)
if !tlsconfig.IsErrEncryptedKey(err) {
return apiclient, err
}
}
}

// NotaryClient provides a Notary Repository to interact with signed metadata for an image
func (cli *DockerCli) NotaryClient(imgRefAndAuth trust.ImageRefAndAuth, actions []string) (notaryclient.Repository, error) {
return trust.GetNotaryRepository(cli.In(), cli.Out(), UserAgent(), imgRefAndAuth.RepoInfo(), imgRefAndAuth.AuthConfig(), actions...)
Expand Down
68 changes: 0 additions & 68 deletions cli/command/cli_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package command
import (
"bytes"
"context"
"crypto/x509"
"fmt"
"io/ioutil"
"net/http"
Expand Down Expand Up @@ -209,73 +208,6 @@ func TestExperimentalCLI(t *testing.T) {
}
}

func TestGetClientWithPassword(t *testing.T) {
expected := "password"

var testcases = []struct {
doc string
password string
retrieverErr error
retrieverGiveup bool
newClientErr error
expectedErr string
}{
{
doc: "successful connect",
password: expected,
},
{
doc: "password retriever exhausted",
retrieverGiveup: true,
retrieverErr: errors.New("failed"),
expectedErr: "private key is encrypted, but could not get passphrase",
},
{
doc: "password retriever error",
retrieverErr: errors.New("failed"),
expectedErr: "failed",
},
{
doc: "newClient error",
newClientErr: errors.New("failed to connect"),
expectedErr: "failed to connect",
},
}

for _, testcase := range testcases {
testcase := testcase
t.Run(testcase.doc, func(t *testing.T) {
passRetriever := func(_, _ string, _ bool, attempts int) (passphrase string, giveup bool, err error) {
// Always return an invalid pass first to test iteration
switch attempts {
case 0:
return "something else", false, nil
default:
return testcase.password, testcase.retrieverGiveup, testcase.retrieverErr
}
}

newClient := func(currentPassword string) (client.APIClient, error) {
if testcase.newClientErr != nil {
return nil, testcase.newClientErr
}
if currentPassword == expected {
return &client.Client{}, nil
}
return &client.Client{}, x509.IncorrectPasswordError
}

_, err := getClientWithPassword(passRetriever, newClient)
if testcase.expectedErr != "" {
assert.ErrorContains(t, err, testcase.expectedErr)
return
}

assert.NilError(t, err)
})
}
}

func TestNewDockerCliAndOperators(t *testing.T) {
// Test default operations and also overriding default ones
cli, err := NewDockerCli(
Expand Down
12 changes: 2 additions & 10 deletions cli/context/docker/load.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"crypto/tls"
"crypto/x509"
"encoding/pem"
"fmt"
"net"
"net/http"
"os"
Expand Down Expand Up @@ -67,17 +66,10 @@ func (c *Endpoint) tlsConfig() (*tls.Config, error) {
keyBytes := c.TLSData.Key
pemBlock, _ := pem.Decode(keyBytes)
if pemBlock == nil {
return nil, fmt.Errorf("no valid private key found")
return nil, errors.New("no valid private key found")
}

var err error
// TODO should we follow Golang, and deprecate RFC 1423 encryption, and produce a warning (or just error)? see https://github.com/docker/cli/issues/3212
if x509.IsEncryptedPEMBlock(pemBlock) { //nolint: staticcheck // SA1019: x509.IsEncryptedPEMBlock is deprecated, and insecure by design
keyBytes, err = x509.DecryptPEMBlock(pemBlock, []byte(c.TLSPassword)) //nolint: staticcheck // SA1019: x509.IsEncryptedPEMBlock is deprecated, and insecure by design
if err != nil {
return nil, errors.Wrap(err, "private key is encrypted, but could not decrypt it")
}
keyBytes = pem.EncodeToMemory(&pem.Block{Type: pemBlock.Type, Bytes: keyBytes})
return nil, errors.New("private key is encrypted - support for encrypted private keys has been removed, see https://docs.docker.com/go/deprecated/")
}

x509cert, err := tls.X509KeyPair(c.TLSData.Cert, keyBytes)
Expand Down
17 changes: 12 additions & 5 deletions docs/deprecated.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ The table below provides an overview of the current status of deprecated feature

Status | Feature | Deprecated | Remove
-----------|------------------------------------------------------------------------------------------------------------------------------------|------------|------------
Deprecated | [Support for encrypted TLS private keys](#support-for-encrypted-tls-private-keys) | v20.10 | -
Removed | [Support for encrypted TLS private keys](#support-for-encrypted-tls-private-keys) | v20.10 | v21.xx
Deprecated | [Kubernetes stack and context support](#kubernetes-stack-and-context-support) | v20.10 | -
Deprecated | [Pulling images from non-compliant image registries](#pulling-images-from-non-compliant-image-registries) | v20.10 | -
Removed | [Linux containers on Windows (LCOW)](#linux-containers-on-windows-lcow-experimental) | v20.10 | v21.xx
Expand Down Expand Up @@ -103,10 +103,17 @@ Removed | [Three arguments form in `docker import`](#three-arguments-form-in-

**Deprecated in Release: v20.10**

Use of encrypted TLS private keys has been deprecated, and will be removed in a
future release. Golang has deprecated support for legacy PEM encryption (as
specified in [RFC 1423](https://datatracker.ietf.org/doc/html/rfc1423)), as it
is insecure by design (see [https://go-review.googlesource.com/c/go/+/264159](https://go-review.googlesource.com/c/go/+/264159)).
**Removed in Release: v21.xx**

Use of encrypted TLS private keys has been deprecated, and has been removed.
Golang has deprecated support for legacy PEM encryption (as specified in
[RFC 1423](https://datatracker.ietf.org/doc/html/rfc1423)), as it is insecure by
design (see [https://go-review.googlesource.com/c/go/+/264159](https://go-review.googlesource.com/c/go/+/264159)).
Copy link
Contributor

@justincormack justincormack Aug 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could say what to do? Basically as the password was next to the key anyway, it wasnt secure.

Copy link
Member Author

@thaJeztah thaJeztah Aug 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a paragraph below, but I can remove the Golang quote if you think it doesn't make sense to include.

I had trouble finding a "canonical" walk-through on removing password-encryption (openssl is fun, as there's many ways to do things), so I left that as an exercise to the user, but if you know of a good resource we should link to, I can add that


This feature allowed using an encrypted private key with a supplied password,
but did not provide additional security as the encryption is known to be broken,
and the key is sitting next to the password in the filesystem. Users are recommended
to decrypt the private key, and store it un-encrypted to continue using it.

### Kubernetes stack and context support

Expand Down