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 ECDSA generation functions to crypto funcs #1388

Merged
merged 4 commits into from
May 7, 2022

Conversation

jbro
Copy link
Contributor

@jbro jbro commented May 6, 2022

ECDSAGenerateKey takes one of Go's supported named NIST curves and an
argument and returns a newly generated EC private key PEM encoded.

ECDSADerivePublicKey takes a PEM encoded EC private key and derives the
corresponding public key, which is returned PEM encoded.

ECDSAGenerateKey takes one of Go's supported named NIST curves and an
argument and returns a newly generated EC private key PEM encoded.

ECDSADerivePublicKey takes a PEM encoded EC private key and derives the
corresponding public key, which is returned PEM encoded.
@jbro jbro requested a review from hairyhenderson as a code owner May 6, 2022 14:09
@hairyhenderson
Copy link
Owner

Thanks for this, @jbro!! This looked super familiar and then I realized that I wrote almost exactly the same function in a local branch that I never ended up pushing 😅

I'll review this a bit later today when I have more time - there are a few changes from my original (local) design that I'd like made, as well we'll need documentation added. But overall this looks good! Thanks again!

@jbro
Copy link
Contributor Author

jbro commented May 6, 2022

Great minds and all that :-)

I'll` have a look and see if I can figure out how to add some documentation.

Comment on lines 129 to 132
- |
$ gomplate -i '{{ $key := crypto.ECDSAGenerateKey "P-521" -}}
{{ $pub := crypto.ECDSADerivePublicKey $key -}}'
hello
Copy link
Owner

Choose a reason for hiding this comment

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

No need for this example (plus hello as an output doesn't make sense)

Suggested change
- |
$ gomplate -i '{{ $key := crypto.ECDSAGenerateKey "P-521" -}}
{{ $pub := crypto.ECDSADerivePublicKey $key -}}'
hello

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That example is "adapted" from the RSAGenerateKey example, so maybe you want to remove the "hello" from there as well?

Copy link
Owner

Choose a reason for hiding this comment

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

The difference is that the RSAGenerateKey example also adds an encrypt & decrypt, so that output makes sense in that context.

Comment on lines 149 to 151
$ gomplate -c privKey=./privKey.pem \
-i '{{ $pub := crypto.ECDSADerivePublicKey .privKey -}}'
hello
Copy link
Owner

Choose a reason for hiding this comment

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

This'll fail with an error like:

20:40:00 ERR  error="Datasources of type application/x-x509-ca-cert not yet supported" experimental=true

We'll need to use -d instead to defer parsing, and then we can use include to include without parsing. Alternatively we could use file.Read, but I don't really like to encourage that 😉

How about this:

Suggested change
$ gomplate -c privKey=./privKey.pem \
-i '{{ $pub := crypto.ECDSADerivePublicKey .privKey -}}'
hello
$ gomplate -d key=priv.pem -i '{{ crypto.ECDSADerivePublicKey (include "key") }}'
-----BEGIN PUBLIC KEY-----
MIGbMBAGByqGSM49AgEGBSuBBAAjA4GGAAQBZvTS1wcCJSsGYQUVoSVctynkuhke
kikB38iNwx/80jzdm+Z8OmRGlwH6OE9NX1MyxjvYMimhcj6zkaOKh1/HhMABrfuY
+hIz6+EUt/Db51awO7iCuRly5L4TZ+CnMAsIbtUOqsqwSQDtv0AclAuogmCst75o
aztsmrD79OXXnhUlURI=
-----END PUBLIC KEY-----

Signed-off-by: Dave Henderson <dhenderson@gmail.com>
@hairyhenderson
Copy link
Owner

@jbro thanks again for this - I've made a few tweaks and will merge soon.

I've been thinking about some more generic crypto.DerivePublicKey/crypto.GenerateKey functions that take the algorithm name as the first arg... That way it would make it simpler to support more algorithms (Ed25519, RSAPKCS1, etc...). What do you think about that approach?

@hairyhenderson hairyhenderson merged commit b32fed7 into hairyhenderson:master May 7, 2022
@jbro
Copy link
Contributor Author

jbro commented May 7, 2022

@jbro thanks again for this - I've made a few tweaks and will merge soon.

That was super quick, thanks for the feedback and the merge :)

I've been thinking about some more generic crypto.DerivePublicKey/crypto.GenerateKey functions that take the algorithm name as the first arg... That way it would make it simpler to support more algorithms (Ed25519, RSAPKCS1, etc...). What do you think about that approach?

That's a good question. I don't know if the signing and encrypt/decrypt APIs for the different algorithms are similar enough that it is practical.

Another thing to consider is that functions with more than one parameter look a bit weird and as I understand it can't be used with the | syntax. So keeping a sort of implicit parameter in the function name makes a lot of sense, I think.

@hairyhenderson
Copy link
Owner

as I understand it can't be used with the | syntax

It'd work with | just as multi-parameter functions work right now... The way to think about it is that whatever's on the left half of the pipeline ends up as the right-most parameter of the right half of the pipeline.

In other words, these are equivalent:

{{ crypto.DecryptAES $key 512 "some encrypted input" }}
{{ "some encrypted input" | crypto.DecryptAES $key 512 }}

@jbro
Copy link
Contributor Author

jbro commented May 7, 2022

as I understand it can't be used with the | syntax

It'd work with | just as multi-parameter functions work right now... The way to think about it is that whatever's on the left half of the pipeline ends up as the right-most parameter of the right half of the pipeline.

In other words, these are equivalent:

{{ crypto.DecryptAES $key 512 "some encrypted input" }}
{{ "some encrypted input" | crypto.DecryptAES $key 512 }}

In that case I think it would look much neater as you suggest, with a single function per operation, which take the key scheme as a parameter.

Do you think it would somehow be possible to "decorate" the output of {{ $key := crypto.GenerateKey rsa 4096 }} such that when it is used like {{ crypto.Encrypt $key "super secret message" }} then crypto.Encrypt knows that it is an RSA key, but when it is used as {{ $key }} then it is still printed correctly? I mean, it's easy enough to see the difference between en RSA and an EC private key in PEM, but for the decrypt step you don't get any indication of the scheme used in the PEM header for the public key.

@hairyhenderson
Copy link
Owner

Do you think it would somehow be possible to "decorate" the output [...]

Perhaps! I will take that into consideration 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants