Add support for ECDSA in dgraph cert by srfrog · Pull Request #3269 · dgraph-io/dgraph

srfrog · 2019-04-08T19:59:09Z

This PR adds support for Weierstrass curves to create private and public keys in Dgraph TLS certificates, dgraph cert command.

Summary of changes:

Modify makeKey() and readKey() to use crypto.PrivateKey and support any type that implements it.
Add new flag for ECDSA curves --elliptic-curve and -e
Detection code when reading .key files to know which type it is (RSA or ECDSA).
Add new flag for RSA bit length -r
Add 'Algorithm' to indicate the key type in dgraph cert ls

Reference #2642

This change is

Add short flag -r for RSA bit size. Add flags --curve and -e for EDCSA curve type. When EDCSA value is set, RSA is ignored.

… types Change signature of readKey and makeKey to support any key type that implements crypto.PrivateKey. This change is made to add support for EDCSA keys, but in the future any other type should be simple to add. Note that crypto.PrivateKey is just a plain dynamic interface type, so it should already support crypto.Signer used in certificate functions.

golangcibot · 2019-04-08T20:00:26Z

+		}
+		switch k := key.(type) {
+		case *ecdsa.PrivateKey:
+			h := md5.Sum(elliptic.Marshal(k.PublicKey.Curve, k.PublicKey.X, k.PublicKey.Y))


G401: Use of weak cryptographic primitive (from gosec)

codexnull

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @golangcibot and @srfrog)

dgraph/cmd/cert/info.go, line 88 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

G401: Use of weak cryptographic primitive (from gosec)

There is another PR pending merge to replace MD5.

…ue-2642_support_ecdsa_in_dgraph_cert

Now that we support different key encryption types, we must display the name and parameters when displaying its info.

Add a new line to `dgraph cert ls` to display the key encryption type. Also, don't panic when command parameters fail, simply exit with error code.

srfrog

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @codexnull and @golangcibot)

dgraph/cmd/cert/info.go, line 88 at r1 (raw file):

Previously, codexnull (Javier Alvarado) wrote…

There is another PR pending merge to replace MD5.

Done.

dgraph/cmd/cert/info.go, line 133 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

G401: Use of weak cryptographic primitive (from gosec)

Done.

dgraph/cmd/cert/info.go, line 136 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

G401: Use of weak cryptographic primitive (from gosec)

Done.

codexnull

Reviewed 3 of 4 files at r2.
Reviewable status: 3 of 4 files reviewed, 4 unresolved discussions (waiting on @codexnull, @golangcibot, and @srfrog)

dgraph/cmd/cert/run.go, line 142 at r2 (raw file):

		}
		if f.encType != "" {
			fmt.Printf("%14s: %s\n", "Encryption", f.encType)

I would suggest changing "Encryption" to "Key Type" or "Key Algorithm" or just "Algorithm". openssl, for example, calls it "Public Key Algorithm."

codexnull

Reviewed 1 of 4 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @golangcibot and @srfrog)

… 'Algorithm' in the listing

srfrog

Reviewable status: 2 of 4 files reviewed, 4 unresolved discussions (waiting on @codexnull and @golangcibot)

dgraph/cmd/cert/run.go, line 142 at r2 (raw file):

Previously, codexnull (Javier Alvarado) wrote…

I would suggest changing "Encryption" to "Key Type" or "Key Algorithm" or just "Algorithm". openssl, for example, calls it "Public Key Algorithm."

Done.

codexnull

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @golangcibot)

manishrjain

Didn't look too carefully, but the change looks pretty contained in the TLS package.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @golangcibot)

Add support for ECDSA keys in dgraph cert and update cert API for adding more encryption types.

srfrog added 6 commits April 8, 2019 12:37

cmd/cert/run.go: add EDCSA run option --curve

8cd55d7

Add short flag -r for RSA bit size. Add flags --curve and -e for EDCSA curve type. When EDCSA value is set, RSA is ignored.

cmd/cert/info.go: support both RSA and EDCSA key types in info

c2214ab

cmd/cert/cert.go: update to use crypto.Signer

14deab0

cmd/cert/run.go: rename EDCSA flag to --elliptic-curve to be specific

ca60ea1

cmd/cert/info.go: add edcsa support to public cert info

41a1b25

srfrog requested a review from a team April 8, 2019 19:59

golangcibot reviewed Apr 8, 2019

View reviewed changes

srfrog mentioned this pull request Apr 8, 2019

Support ED25519 in dgraph cert #2642

Closed

codexnull approved these changes Apr 8, 2019

View reviewed changes

srfrog added 5 commits April 9, 2019 12:30

Merge branch 'master' of github.com:/dgraph-io/dgraph into srfrog/iss…

e267e9a

…ue-2642_support_ecdsa_in_dgraph_cert

cmd/cert/cert.go: minor changes for code legibility

08dea40

cmd/cert/create.go: minor changes for code legibility and usability

07ce93f

cmd/cert/info.go: add encryption type to info output

3464628

Now that we support different key encryption types, we must display the name and parameters when displaying its info.

cmd/cert/run.go: display key enc type in listing info

cc3246a

Add a new line to `dgraph cert ls` to display the key encryption type. Also, don't panic when command parameters fail, simply exit with error code.

srfrog commented Apr 10, 2019

View reviewed changes

codexnull approved these changes Apr 10, 2019

View reviewed changes

codexnull reviewed Apr 10, 2019

View reviewed changes

cmd/cert/info.go: rename certInfo.encType to 'algo' and display it as…

57b5a0f

… 'Algorithm' in the listing

srfrog commented Apr 10, 2019

View reviewed changes

codexnull approved these changes Apr 11, 2019

View reviewed changes

manishrjain approved these changes Apr 11, 2019

View reviewed changes

srfrog merged commit 3456be7 into master Apr 11, 2019

srfrog deleted the srfrog/issue-2642_support_ecdsa_in_dgraph_cert branch April 11, 2019 20:18

dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019

Add support for ECDSA in dgraph cert (dgraph-io#3269)

2b19978

Add support for ECDSA keys in dgraph cert and update cert API for adding more encryption types.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for ECDSA in dgraph cert#3269

Add support for ECDSA in dgraph cert#3269
srfrog merged 12 commits intomasterfrom
srfrog/issue-2642_support_ecdsa_in_dgraph_cert

srfrog commented Apr 8, 2019 •

edited

Loading

Uh oh!

Uh oh!

golangcibot Apr 8, 2019

Uh oh!

Uh oh!

codexnull left a comment

Uh oh!

srfrog left a comment

Uh oh!

codexnull left a comment

Uh oh!

codexnull left a comment

Uh oh!

srfrog left a comment

Uh oh!

codexnull left a comment

Uh oh!

manishrjain left a comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

Conversation

srfrog commented Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

golangcibot Apr 8, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codexnull left a comment

Choose a reason for hiding this comment

Uh oh!

srfrog left a comment

Choose a reason for hiding this comment

Uh oh!

codexnull left a comment

Choose a reason for hiding this comment

Uh oh!

codexnull left a comment

Choose a reason for hiding this comment

Uh oh!

srfrog left a comment

Choose a reason for hiding this comment

Uh oh!

codexnull left a comment

Choose a reason for hiding this comment

Uh oh!

manishrjain left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

srfrog commented Apr 8, 2019 •

edited

Loading