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

Add support for ECDSA in dgraph cert #3269

Merged
merged 12 commits into from Apr 11, 2019

Conversation

srfrog
Copy link
Contributor

@srfrog srfrog commented Apr 8, 2019

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 Reviewable

srfrog added 6 commits April 8, 2019 12:37
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.
@srfrog srfrog requested a review from a team April 8, 2019 19:59
dgraph/cmd/cert/info.go Outdated Show resolved Hide resolved
}
switch k := key.(type) {
case *ecdsa.PrivateKey:
h := md5.Sum(elliptic.Marshal(k.PublicKey.Curve, k.PublicKey.X, k.PublicKey.Y))

Choose a reason for hiding this comment

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

G401: Use of weak cryptographic primitive (from gosec)

dgraph/cmd/cert/info.go Outdated Show resolved Hide resolved
Copy link
Contributor

@codexnull codexnull left a comment

Choose a reason for hiding this comment

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

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.

srfrog added 5 commits April 9, 2019 12:30
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.
Copy link
Contributor Author

@srfrog srfrog left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@codexnull codexnull left a comment

Choose a reason for hiding this comment

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

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."

Copy link
Contributor

@codexnull codexnull left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@srfrog srfrog left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@codexnull codexnull left a comment

Choose a reason for hiding this comment

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

:lgtm:

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

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm: 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)

@srfrog srfrog merged commit 3456be7 into master Apr 11, 2019
@srfrog 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 keys in dgraph cert and update cert API for adding more encryption types.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants