scion-pki: use the same emoji encoding mapping as smallstep #4252
Conversation
….com/) The usage of the emojisum pkg (https://pkg.go.dev/github.com/emojisum/emojisum) led to the issue that the emoji encodings were not comparable with the encodings from smallstep due to different mappings. Changes: - add the emoji mapping from small smallstep - remove and replace usage of the emojisum pkg
bunert
changed the title
There was a problem hiding this comment.
Thank you for this contribution, @bunert!
Reviewed 5 of 5 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @bunert and @oncilla)
scion-pki/certs/fingerprint.go line 71 at r1 (raw file):
if flags.format == "emoji" { output = encoding.ToEmoji(fingerprint)
Nit, remove empty line.
scion-pki/encoding/emoji.go line 16 at r1 (raw file):
// Package encoding contains helper functions for encoding. // source: https://github.com/smallstep/cli/blob/1e0b1667db00f1b0756e0d833fca496a323338b7/crypto/fingerprint/emoji.go#L5
Nit, I think we don't need to give credit for reusing this very straight-forward mapper function. The mapping itself should be referenced though (see comment below).
scion-pki/encoding/emoji.go line 32 at r1 (raw file):
// // The mapping is based on draft+2 of https://github.com/emojisum/emojisum. // (see: https://github.com/emojisum/emojisum/releases/tag/draft%2B2)
This comment seems misleading, as there are some differences (which triggered this PR in the first place). Mention that this is compatible with smallstep and that this is where this mapping comes from. For future reference, it could also be helpful to document what the difference between the two mappings really is -- as far as I can tell, the diff is only that this mapping here uses "gender qualified" emojis.
There was a problem hiding this comment.
Reviewed 3 of 5 files at r1, 2 of 3 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @bunert)
There was a problem hiding this comment.
Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status:
…to#4252) Use the same emoji encoding as [smallstep cli](https://github.com/smallstep/cli) for better compatibilty. The mapping in the previously used [emojisum package](https://pkg.go.dev/github.com/emojisum/emojisum) has slight differences to smallstep, which would lead to mismatches of some of the digests.
Use the same emoji encoding as smallstep cli for better compatibilty.
The mapping in the previously used emojisum package has slight
differences to smallstep, which would lead to mismatches of some of the digests.
This change is