-
Notifications
You must be signed in to change notification settings - Fork 90
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 JSON Object Signing capabilities #1105
Conversation
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.
Thanks for working on this!
I understand that it is hard to know exactly what to bring in here as this crate is intended to be a building block for higher level functionality which we have yet to flesh out. Taking that into account I think this is already pretty good!
What I am missing is some more documentation for public items, perhaps especially in the jws
module. In fact it would be really nice with some module level documentation for encoder
and decoder
.
I also wish it was a bit easier to get an overview over where the tests for a given function can be found since the tests are not in the same file (in this case understandably so). Maybe the cov_mark crate can help with that? (Matklad has written some pretty good blog posts about that crate here and here).
On the architecture side I am not sure whether we want to include the possibility to attach private key parameters in a JWK at all as we want to discourage in-memory keys, but that is perhaps something we can revisit in a later PR.
This was my first read through. I will go through the tests more carefully tomorrow.
There are only high-level tests of the encoder/decoder API for now, most based on test vectors, so even with cov_mark you could not find a test for a certain function. Coverage is certainly not great right now. We can eventually add more unit tests in the respective modules, and then we will be able to find tests in the same file as the definition.
I think we should leave it as-is for having a complete JWK implementation but mostly because the test vectors include the private keys as JWKs, so we couldn't decode those, or would have to do so manually. |
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.
Thanks for the improved documentation!
I approve this with the understanding that we will iterate heavily upon this crate before our next release as there are still a lot of missing docs and there should be more tests (also for the unhappy path).
Description of change
This PR introduces JOSE (JSON Object Signing and Encryption) capabilities in the
identity_jose
crate. This is in preparation to adding support for signing credentials withVC-JWT
. See also #1103 for more details.The primary source of the implementation is our (unused)
libjose
crate, from which the JWK, JWS and JWT implementations were extracted. JWE is not required for now and so was not added to the new crate.The biggest change to the
libjose
crate is how signatures are crated and how they are verified. In particular,libjose
included signing capabilities for RSA, P256, HMAC and Ed25519 using in-memory keys. Because we want to use our more secure architecture that delegates signing to some KMS (like Stronghold, a cloud KMS or potentially a hardware-secure module), the API was changed accordingly.The included capabilities were removed to remove the dependency on various cryptographic crates. Instead the
Encoder
andDecoder
API essentially take closures that are expected to sign and verify, respectively.A full roundtrip example:
Links to any relevant issues
part of #1103.
Type of change
Add an
x
to the boxes that are relevant to your changes.How the change has been tested
Tests with test vectors from various RFCs are included.
Change checklist
Add an
x
to the boxes that are relevant to your changes.