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

GODRIVER-364 Support PKCS8 encrypted client private keys #565

Merged
merged 9 commits into from Feb 1, 2021
Merged

GODRIVER-364 Support PKCS8 encrypted client private keys #565

merged 9 commits into from Feb 1, 2021

Conversation

benjirewis
Copy link
Contributor

GODRIVER-364

Adds support for decrypting PKCS8 encrypted PEM keys. Private keys encrypted with PKCS8 should begin with -----BEGIN ENCRYPTED PRIVATE KEY---- according to documentation, so the new parsing in clientoptions#addClientCertFromBytes assumes this.

Adds two new test certificates used in the TestClient/x509 test. One is a PKCS8 encrypted PEM key, and one is unencrypted.

return "", err
}
} else if strings.Contains(currentBlock.Type, "ENCRYPTED") {
// The pkcs8 package only handles the PKCS #5 v2.0 scheme.
Copy link
Contributor Author

@benjirewis benjirewis Jan 21, 2021

Choose a reason for hiding this comment

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

This parsing code is from the TOOLS-2013 PR. This was also brought up there, but youmark/pkcs8 can only handle the PKCS 5 v2.0 scheme. I don't much about the various PKCS schemes, but it might be worth documenting somewhere how to create this scheme. I used:

openssl pkcs8 -v2 des3 -topk8 -inform PEM -outform PEM -in client.pem -out client-pkcs8-encrypted.pem

Furthermore, did we need support for a specific scheme?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, the command used to generate seems worth leaving a comment in GODRIVER-364 for future reference (anytime I need to generate certs, I need to google around...).

Given the conversation in TOOLS-2013 and related tickets, I think v2.0 is only requirement. It seems v1.5 was superseded (judging from RFC-8018 and and an article describing the v2.0 key derivation function.

cc @markbenvenuto in case you'd like to weigh in. I do not think we need to support more than PKCS 5 v2.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can leave the two commands (one for encrypted and one for unencrypted) in the original ticket. And good to know 2.0 supersedes other versions; thanks for doing that research.

Choose a reason for hiding this comment

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

Yes, we only need to support PKCS#5 v2.0. We don't need to support the older version since it uses a weaker key derivation function.

@@ -97,9 +97,12 @@ github.com/tidwall/pretty
github.com/xdg/scram
# github.com/xdg/stringprep v0.0.0-20180714160509-73f8eece6fdc
github.com/xdg/stringprep
# golang.org/x/crypto v0.0.0-20190530122614-20be4c3c3ed5
# github.com/youmark/pkcs8 v0.0.0-20201027041543-1326539a0a0a
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure on our process for adding dependencies... I ran go get github.com/youmark/pkcs8, and go mod vendor after using the new package.

@benjirewis benjirewis marked this pull request as ready for review January 21, 2021 23:11
Copy link
Contributor

@divjotarora divjotarora left a comment

Choose a reason for hiding this comment

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

Code changes LGTM. There's a merge conflict with mongo/integration/client_test.go, so you may have to rebase locally and force-push to this branch.

DB string
certificates := [3]string{"client.pem", "client-pkcs8-encrypted.pem", "client-pkcs8-unencrypted.pem"}
for _, certificate := range certificates {
keyFilePassword := ""
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: For empty declarations, I generally prefer using the zero value: var keyFilePassword string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! Good to learn common Go patterns :)

Copy link
Contributor

@iwysiu iwysiu left a comment

Choose a reason for hiding this comment

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

lgtm with a test nit

User string
DB string
certificates := [3]string{"client.pem", "client-pkcs8-encrypted.pem", "client-pkcs8-unencrypted.pem"}
for _, certificate := range certificates {
Copy link
Contributor

Choose a reason for hiding this comment

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

Each certificate type can be run as its own subtest for debugging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

LGTM!

return "", err
}
} else if strings.Contains(currentBlock.Type, "ENCRYPTED") {
// The pkcs8 package only handles the PKCS #5 v2.0 scheme.
Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, the command used to generate seems worth leaving a comment in GODRIVER-364 for future reference (anytime I need to generate certs, I need to google around...).

Given the conversation in TOOLS-2013 and related tickets, I think v2.0 is only requirement. It seems v1.5 was superseded (judging from RFC-8018 and and an article describing the v2.0 key derivation function.

cc @markbenvenuto in case you'd like to weigh in. I do not think we need to support more than PKCS 5 v2.0.

{"roles", bson.A{
bson.D{{"role", "readWrite"}, {"db", "test"}},
}},
testCases := []struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, good idea making this into a table test!

mongo/options/clientoptions_1_9.go Outdated Show resolved Hide resolved
Co-authored-by: Kevin Albertson <kevin.albertson@10gen.com>
@benjirewis benjirewis merged commit 15a1051 into mongodb:master Feb 1, 2021
@benjirewis benjirewis deleted the pkcs8.364 branch February 1, 2021 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants