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

Implement elementary .pem keyfile parsing. #62

Closed
wants to merge 2 commits into from
Closed

Implement elementary .pem keyfile parsing. #62

wants to merge 2 commits into from

Conversation

asiplas
Copy link

@asiplas asiplas commented Nov 25, 2018

Relatively untested .pem parsing for RS256, etc.

Useful perhaps for RS256 per https://cloud.google.com/iot/docs/how-tos/credentials/jwts

See Also #39


Example

jose jws sig -I payload.json -p ~/gcloud.pem

File Contents

payload.json

{"iat": 1543125263, "exp": 1543125563, "aud": "my-project-101"}

gcloud.pem

-----BEGIN PRIVATE KEY-----
xxxxxxxxxxxxxxxxxxxxxxxxxx
xxxxxxxxxxxxxxxxxxxxxxxxxx
...
-----END PRIVATE KEY-----

e.g. `jose jws sig -p ~/gcloud.pem -I payload.json`
@codecov-io
Copy link

Codecov Report

Merging #62 into master will decrease coverage by 0.49%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #62     +/-   ##
=========================================
- Coverage   77.75%   77.25%   -0.5%     
=========================================
  Files          60       60             
  Lines        5794     5720     -74     
=========================================
- Hits         4505     4419     -86     
- Misses       1289     1301     +12
Impacted Files Coverage Δ
cmd/jws/sig.c 68.18% <ø> (-0.3%) ⬇️
cmd/jose.c 73.39% <0%> (-4.75%) ⬇️
cmd/jws/fmt.c 75.43% <0%> (-1.99%) ⬇️
cmd/jwe/fmt.c 75.67% <0%> (-1.25%) ⬇️
lib/openssl/ecdhes.c 77.06% <0%> (-1.12%) ⬇️
lib/openssl/ecmr.c 77.04% <0%> (-1.08%) ⬇️
cmd/jwe/dec.c 54.11% <0%> (-1.06%) ⬇️
cmd/jwk/gen.c 70% <0%> (-0.97%) ⬇️
lib/openssl/jwk.c 69.72% <0%> (-0.89%) ⬇️
cmd/jws/ver.c 80% <0%> (-0.71%) ⬇️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 028eab5...a21ace7. Read the comment docs.

Copy link
Collaborator

@sarroutbi sarroutbi left a comment

Choose a reason for hiding this comment

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

Thanks for PR. Apart from suggested changes, is it possible to add a test in file: "tests/jose-jws-sig "to handle this?

} else {
FILE_AUTO *file = fopen(arg, "r");
// TODO: encrypted key callback for password.
if (!(file && PEM_read_PrivateKey(file, &pkey, NULL, NULL)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here, it is not distinguished the case of !file vs the case of PEM_read_PrivateKey.

Distinguish between them, so that if the error has to do with PEM_read_PrivateKey, then fclose on file is done

@@ -84,6 +84,12 @@ static const jcmd_doc_t jcmd_doc_key[] = {
{}
};

static const jcmd_doc_t jcmd_doc_pem[] = {
{ .arg = "FILE", .doc="Import JWK from '.PEM' FILE" },
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it required for the file to be ".PEM"? Why not just "Import JWK from FILE"?

@@ -84,6 +84,12 @@ static const jcmd_doc_t jcmd_doc_key[] = {
{}
};

static const jcmd_doc_t jcmd_doc_pem[] = {
{ .arg = "FILE", .doc="Import JWK from '.PEM' FILE" },
{ .arg = "-", .doc="Import JWK from '.PEM' on standard input" },
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think "Import JWK from standard input" fits better

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants