Skip to content

Remove support for encrypted TLS private keys - #3220

Merged
justincormack merged 1 commit into
docker:masterfrom
thaJeztah:remove_encrypted_tls_support
Jan 27, 2022
Merged

Remove support for encrypted TLS private keys#3220
justincormack merged 1 commit into
docker:masterfrom
thaJeztah:remove_encrypted_tls_support

Conversation

@thaJeztah

@thaJeztahthaJeztah commented Jul 28, 2021

Copy link
Copy Markdown
Member

Legacy PEM encryption as specified in RFC 1423 is insecure by design. Since
it does not authenticate the ciphertext, it is vulnerable to padding oracle
attacks that can let an attacker recover the plaintext

From https://go-review.googlesource.com/c/go/+/264159

It's unfortunate that we don't implement PKCS#8 encryption so we can't
recommend an alternative but PEM encryption is so broken that it's worth
deprecating outright.

- What I did

- How I did it

- How to verify it

- Description for the changelog

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

@thaJeztah

Copy link
Copy Markdown
MemberAuthor

Note that there's still support for encrypted keys as part of docker trust subcommands, as part of the Notary integration;

funcvalidateAndGenerateKey(streams command.Streams, keyNamestring, workingDirstring) error {
freshPassRetGetter:=func() notary.PassRetriever { returntrust.GetPassphraseRetriever(streams.In(), streams.Out()) }
iferr:=validateKeyArgs(keyName, workingDir); err!=nil {
returnerr
}

Not sure if that should be changed /cc @justincormack

@thaJeztah
thaJeztahforce-pushed the remove_encrypted_tls_support branch from 8c1da14 to 13c982fCompareJuly 28, 2021 14:15
@codecov-commenter

codecov-commenter commented Jul 28, 2021

Copy link
Copy Markdown

Codecov Report

Merging #3220 (4fa9695) into master (48cbe0b) will increase coverage by 1.63%.
The diff coverage is n/a.

❗ Current head 4fa9695 differs from pull request most recent head 08a1ccc. Consider uploading reports for the commit 08a1ccc to get more accurate results

@@ Coverage Diff @@## master #3220 +/- ##
==========================================
+ Coverage 56.36% 57.99% +1.63% 
==========================================
Files 304 302 -2 Lines 26833 21734 -5099 ==========================================
- Hits 15124 12605 -2519 + Misses 10789 8206 -2583 - Partials 920 923 +3 

@thaJeztah

Copy link
Copy Markdown
MemberAuthor

@justincormack@cpuguy83 PTAL

Comment threadcli/context/docker/load.go Outdated
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")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could link to an issue here rather than just saying "has been removed"? or docs?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Opened docker/docs#13329, and added a link to https://docs.docker.com/go/deprecated/ (which will contain the information from docs/deprecated.md from this PR)

Comment threaddocs/deprecated.md
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
Copy Markdown
Contributor

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
Copy Markdown
MemberAuthor

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

@thaJeztah

Copy link
Copy Markdown
MemberAuthor

@justincormack ptal

@thaJeztah

Copy link
Copy Markdown
MemberAuthor

@justincormack ptal if this the description looks OK now

> Legacy PEM encryption as specified in RFC 1423 is insecure by design. Since
> it does not authenticate the ciphertext, it is vulnerable to padding oracle
> attacks that can let an attacker recover the plaintext
From https://go-review.googlesource.com/c/go/+/264159
> It's unfortunate that we don't implement PKCS#8 encryption so we can't
> recommend an alternative but PEM encryption is so broken that it's worth
> deprecating outright.
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.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
thaJeztahforce-pushed the remove_encrypted_tls_support branch from 4fa9695 to 08a1cccCompareJanuary 27, 2022 13:28

@chris-cronechris-crone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

context: consider deprecating support for legacy PEM encryption as specified in RFC 1423

4 participants

@thaJeztah@codecov-commenter@justincormack@chris-crone