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

Replace MD5 in dgraph cert ls #3254

Merged
merged 4 commits into from
Apr 9, 2019
Merged

Replace MD5 in dgraph cert ls #3254

merged 4 commits into from
Apr 9, 2019

Conversation

codexnull
Copy link
Contributor

@codexnull codexnull commented Apr 4, 2019

MD5 digests are vulnerable to collisions nowadays. This is a simple change to replace them with SHA-256 digests (aka hashes, fingerprints, thumbprints) in dgraph cert ls output.


This change is Reviewable

@codexnull codexnull requested a review from a team April 4, 2019 21:10
Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r1.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @codexnull)


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

groupSize

nit: rename to groupByteSize so the units are explicit.


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

hex := fmt.Sprintf("%0*X", groupSize*2, digest[0:groupSize])

A small comment with an example of how the output is supposed to look would be helpful.

Copy link
Contributor Author

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

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @martinmr)


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

Previously, martinmr (Martin Martinez Rivera) wrote…
groupSize

nit: rename to groupByteSize so the units are explicit.

Done.


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

Previously, martinmr (Martin Martinez Rivera) wrote…
hex := fmt.Sprintf("%0*X", groupSize*2, digest[0:groupSize])

A small comment with an example of how the output is supposed to look would be helpful.

Done.

Copy link
Contributor

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

:lgtm:

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @martinmr)

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:

Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @martinmr)

@codexnull codexnull merged commit cbc7b8b into master Apr 9, 2019
@codexnull codexnull deleted the javier/replace_md5 branch April 9, 2019 02:20
mangalaman93 pushed a commit that referenced this pull request Apr 10, 2019
* Change cert ls command to output SHA-256 digests instead of MD5.
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
* Change cert ls command to output SHA-256 digests instead of MD5.
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.

4 participants