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.

Sign up for GitHub

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

Jump to bottom

Add support for Ed25519 #1900

Merged
merged 6 commits into from
Dec 17, 2023
Merged

Add support for Ed25519 #1900

merged 6 commits into from
Dec 17, 2023

Conversation

horvski
Copy link
Contributor

@horvski horvski commented Nov 7, 2023

No description provided.

@hairyhenderson
Copy link
Owner

Hi @horvski, thanks for working on this. I don't have time to review this in depth yet, but on the surface it looks like a reasonable new function.

Can I ask why you decided to name it EDDSA rather than something more specific like Ed25519? I think the more specific name would make more sense, given that EdDSA also has a variant based on edwards448.

Also in future please file an issue before issuing a PR (see CONTRIBUTING.md).

@hairyhenderson hairyhenderson self-assigned this Nov 10, 2023
@hairyhenderson hairyhenderson added this to the v4.0.0 milestone Nov 10, 2023
@horvski
Copy link
Contributor Author

horvski commented Nov 10, 2023

Hi @hairyhenderson, no problem.

Can I ask why you decided to name it EDDSA rather than something more specific like Ed25519? I think the more specific name would make more sense, given that EdDSA also has a variant based on edwards448.

  • I was going off of your ECDSA implementation, so that it would be ready somebody wanted to add Ed448 or Ed25519ph. For now though, I agree with you and renamed the function more specifically to Ed25519.

I will keep that in mind in the future, thanks.

@horvski
Copy link
Contributor Author

horvski commented Nov 10, 2023

@hairyhenderson

I also added signing and verifying with the key pair, to ensure that the encoding and decoding work as expected. This may be better off being split into separate unit tests. I figured this was good for now though, to at least know that the key pair was working.

hairyhenderson
hairyhenderson reviewed Nov 12, 2023
View reviewed changes
crypto/eddsa.go Outdated Show resolved Hide resolved
hairyhenderson
hairyhenderson reviewed Nov 12, 2023
View reviewed changes
crypto/eddsa.go Outdated Show resolved Hide resolved
hairyhenderson
hairyhenderson reviewed Nov 12, 2023
View reviewed changes
crypto/eddsa.go Outdated Show resolved Hide resolved
hairyhenderson
hairyhenderson reviewed Nov 12, 2023
View reviewed changes
crypto/eddsa.go Outdated Show resolved Hide resolved
hairyhenderson
hairyhenderson reviewed Nov 12, 2023
View reviewed changes
crypto/eddsa.go Outdated Show resolved Hide resolved
hairyhenderson
hairyhenderson reviewed Nov 12, 2023
View reviewed changes
crypto/eddsa.go Outdated Show resolved Hide resolved
hairyhenderson
hairyhenderson requested changes Nov 12, 2023
View reviewed changes
Copy link
Owner

@hairyhenderson hairyhenderson left a comment

Choose a reason for hiding this comment

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

I've had a bit of a closer look, and made some review comments - thanks again for working on this @horvski!

In order to make this fully usable in gomplate, it'll need to also be added as a function (in funcs/crypto.go, etc...).

It'll also need documentation. I can handle those parts if you're not up for it, though it'll take me some time to get to it.

@hairyhenderson
Copy link
Owner

BTW you'll also need to rebase for the CI to pass

hairyhenderson
hairyhenderson reviewed Nov 29, 2023
View reviewed changes
crypto/eddsa.go Outdated Show resolved Hide resolved
hairyhenderson
hairyhenderson reviewed Nov 29, 2023
View reviewed changes
crypto/eddsa.go Outdated Show resolved Hide resolved
hairyhenderson
hairyhenderson reviewed Nov 29, 2023
View reviewed changes
crypto/eddsa.go Outdated Show resolved Hide resolved
@hairyhenderson
Copy link
Owner

Sorry for the delay - I've taken another pass and added some comments. If you rebase again the tests should pass!

@horvski
Copy link
Contributor Author

horvski commented Nov 29, 2023

No worries! Sounds good, I'll do that now. I agree with your recent review, and yes, I think we should rename the file. I'll do that before I rebase.

hairyhenderson
hairyhenderson reviewed Nov 30, 2023
View reviewed changes
Copy link
Owner

@hairyhenderson hairyhenderson left a comment

Choose a reason for hiding this comment

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

Just one more minor change in the docs, otherwise this LGTM!

Can you also please regenerate the docs by running make docs/content/functions/crypto.md and committing the updated file?

hairyhenderson
hairyhenderson reviewed Nov 30, 2023
View reviewed changes
docs-src/content/functions/crypto.yml Outdated
description: the random seed encoded in either base64 or hex
examples:
- |
$ gomplate -i '{{ crypto.Ed25519GenerateKeyFromSeed base64 MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDA= }}'
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
$ gomplate -i '{{ crypto.Ed25519GenerateKeyFromSeed base64 MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDA= }}'
$ gomplate -i '{{ crypto.Ed25519GenerateKeyFromSeed "base64" MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDA= }}'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why does the encoding need to be in quotes and the seed does not?

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, I missed that - they both need to be in quotes as they're both strings 😉

@horvski
Copy link
Contributor Author

horvski commented Nov 30, 2023

Just one more minor change in the docs, otherwise this LGTM!

Can you also please regenerate the docs by running make docs/content/functions/crypto.md and committing the updated file?

I ran the command that you mentioned to create the new docs, but am getting this error:

$ make docs/content/functions/crypto.md
2023/11/30 10:36:00 Invalid Semantic Version
exit status 1
gomplate -d data=docs-src/content/functions/crypto.yml -f docs-src/content/functions/func_doc.md.tmpl -o docs/content/functions/crypto.md
10:36:08 ERR error="renderTemplate: failed to render template docs-src/content/functions/func_doc.md.tmpl: template: docs-src/content/functions/func_doc.md.tmpl:12:4: executing \"usage\" at <index $arguments $last>: error calling index: index out of range: -1"
make: *** [Makefile:214: docs/content/functions/crypto.md] Error 1
make: *** Deleting file 'docs/content/functions/crypto.md'

My guess is that since my fork does not have the semantic versioning, that's where the first error is coming from. The command seems to continue after this though, and I am not sure why it fails.

I got two different errors, but I can't get the first one again for some reason. At first, it was saying that crypto.md is already up to date. Now when I am running it, it keeps deleting the file and I have to git restore it.

Any clue of what I am doing wrong here?

@hairyhenderson
Copy link
Owner

I ran the command that you mentioned to create the new docs, but am getting this error:

I was able to duplicate this locally - it's because of pipeline - see #1900 (comment)

@hairyhenderson
Copy link
Owner 

$ make docs/content/functions/crypto.md
2023/11/30 10:36:00 Invalid Semantic Version
exit status 1

This can be safely ignored - it's from the Makefile, here. It might be happening because of how you forked the repo (maybe you didn't pull down tags? or maybe it's because you're working from your main branch?), but either way it's unimportant.

hairyhenderson
hairyhenderson reviewed Dec 5, 2023
View reviewed changes
docs-src/content/functions/crypto.yml Outdated
description: the random seed encoded in either base64 or hex
examples:
- |
$ gomplate -i '{{ crypto.Ed25519GenerateKeyFromSeed "base64" MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDA= }}'
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry - I missed that the second value was also not in quotes 🤦‍♂️

Suggested change
$ gomplate -i '{{ crypto.Ed25519GenerateKeyFromSeed "base64" MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDA= }}'
$ gomplate -i '{{ crypto.Ed25519GenerateKeyFromSeed "base64" "MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDA=" }}'

@hairyhenderson
Copy link
Owner

@horvski just one final thing to fix, then a rebase, and it'll be good to go! Thanks for your patience and persistence on this 🙇‍♂️

hairyhenderson
hairyhenderson approved these changes Dec 17, 2023
View reviewed changes
Copy link
Owner

@hairyhenderson hairyhenderson left a comment

Choose a reason for hiding this comment

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

Thanks @horvski! LGTM :shipit:

@hairyhenderson hairyhenderson merged commit 914960f into hairyhenderson:main Dec 17, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hairyhenderson hairyhenderson hairyhenderson approved these changes

Assignees

@hairyhenderson hairyhenderson

Labels
new function
Projects
None yet
Milestone
v4.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@horvski @hairyhenderson