-
Notifications
You must be signed in to change notification settings - Fork 3
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
JWK Implementation for import/export #61
Conversation
e284c5e
to
575b54d
Compare
575b54d
to
8ff0bd5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job @olegbespalov 👏🏻 LGTM 🚀 🙇🏻
Before I forget, we should also update the README tables to match the newly introduced support for JWK in |
import { crypto } from "k6/x/webcrypto"; | ||
|
||
export default async function () { | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stupid question; why do this example is surrounded by a try..catch
, while other (e.g. import-export-jwk-aes-key.js
) isn't, if the operations are the same? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the other way around, I think it's just copy-paste issue, I will adjust the import-export-jwk-aes-key.js
to also have a try...catch
, which won't harm 👍
webcrypto/aes.go
Outdated
// check the key length | ||
var ( | ||
has128Bits = len(keyData) == 16 | ||
has192Bits = len(keyData) == 24 | ||
has256Bits = len(keyData) == 32 | ||
) | ||
|
||
if !has128Bits && !has192Bits && !has256Bits { | ||
return nil, NewError(DataError, fmt.Sprintf("invalid key length %v bytes", len(keyData))) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit; this can be hide behind a method, similar to isHashAlgorithm
webcrypto/jwk.go
Outdated
return nil, fmt.Errorf("failed to marshal JWK key: %w", err) | ||
} | ||
|
||
// wrap result result into the object that is expected to be returned |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// wrap result result into the object that is expected to be returned | |
// wrap result into the object that is expected to be returned |
return promise | ||
} | ||
default: | ||
reject(NewError(ImplementationError, "unsupported format "+format)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reject(NewError(ImplementationError, "unsupported format "+format)) | |
reject(NewError(ImplementationError, "unsupported format "+format)) | |
return promise |
Shouldn't we return the promise here as well, as we do for the rest of the error cases? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should! Great catch!
|
||
b, ok := result.([]byte) | ||
if !ok { | ||
reject(NewError(ImplementationError, "for "+format+" []byte expected as result")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the comment above, should we return here right after the reject
? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks generally good, great work! 🎉
I left a few minor comments, but almost none of them is blocking, except the ones on webcrypto/subtle_crypto.go
, which I'm wondering if needed for correctness (correct behavior).
Thanks!
What?
This is an implementation of the JWK export/import.
I've decided to use the popular library instead of self-tailored since, for the following algorithms, the JWK could be more complicated in terms of maintaining standards.
How did I test it?
Related PRs:
This PR is based on #62
Why?
JWK is part of the specification.
Closes: #37