-
Notifications
You must be signed in to change notification settings - Fork 272
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
Changes to support ACME, including JWS #359
base: master
Are you sure you want to change the base?
Conversation
7821a61
to
fd60bf2
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.
It would also need a README update
/// | ||
/// Defined in [RFC8555#6.5.2](https://datatracker.ietf.org/doc/html/rfc8555#section-6.5.2). | ||
#[serde(skip_serializing_if = "Option::is_none")] | ||
pub nonce: Option<String>, |
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.
Can you add #347 (comment) while you're there?
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.
I added crit, enc, and zip -- I think I have the values there right, but I'm not familiar with their use so double checking it would probably be good.
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. @inferiorhumanorgans can you have a look?
return Err(new_error(ErrorKind::InvalidSignature)); | ||
} | ||
|
||
Ok(header) |
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.
We can probably macro it out to avoid duplicating the code from verify_signature
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.
I tried factoring out a function, let me know if this works for you.
One test here. Works great. Thanks @andrewbaxter! |
Awesome, and thanks for the comments! Sorry, I didn't quite get to this today. I'll try to do it tomorrow. |
I added a (very simple) example to the readme, let me know if there's other additions you think would be good. |
My (limited) real world testing worked. |
I will have to read the specs and review the PR properly as I don't really know/use JWK/JWS myself. That's going to take some time but if more people can try that would be great |
Can I support the work on this somehow? I don't know if I have the technical aptitude to help review, but if I can be of other assistance I would love to. |
Ideally maybe we could get one single PR (@andrewbaxter can change the target of other PRs to this one) so we can point at a single point for people to test |
Keep them separate but just change the target? Or should I combine them all? Aside from loading urlsafe base64 hmacs I don't think I have any others incoming |
change the target, we review them and then we can do a big review of the whole thing on this PR |
@randomairborne do you have any use cases you could try it with? I looked at various acme libraries and they all seemed to be doing their own jwt stuff (and didn't support EAB). I ended up hacking it into Poem's acme implementation, but I have some reservations about that |
Okay, great! |
@Keats I'm not sure I can re-target MRs in this repo to branches in my fork... sorry, my github ineptitude on full display. AFAICT it only allows me to select branches in this repo. Edit: I'll try stacking them and recreating the MRs in my fork. |
I'm attempting to write a new ACME library 😅. If this has a somewhat functional public API, I'll start using it. |
Sorry didn't get to this yesterday. I'll do it now |
Okay I made targeted at this branch, and andrewbaxter#4 with them all merged together if you want to try stuff out. I think this is everything needed for ACME, but I haven't read through all the different flows in ACME so it's possible some stuff is still missing. I added you @Keats as a collaborator in case you wanted to merge them into this one. I can close the other MRs in this repo if that would be good. |
Add support for JWK thumbprints
Method to load urlsafe base64 hmac encoding key
Add method to generate JWK from EncodingKey
Ok if there are any JWS/ACME users, can they try that branch? |
Has anyone tried it? |
Aside from me I assume... 😅 |
I tried to test decoding of a Jws. I added the following line in my Cargo.toml
But I don't know how to transform the string slice containing the Jws into a Jws type which is required by the decode_jws function. I can't hand over just the string slice. This is possible with the regular decode function for jwt.
|
Ah yeah, so it looks like you have a JWS in the "JWS Compact Serialization" form which is There are two representations discussed in the RFC: JWS JSON Serialization and JWS Compact Serialization. The JWS support I added here doesn't provide a convenience method for the compact serialization representation. It implements In order to use this with the Compact Serialization I think you'd need to split it by let mut parts = jws_token.split(".");
let jws = JWS<serde_json::Value>{
protected: parts.next()?,
payload: parts.next()?,
signature: parts.next()?,
_pd: Default::default(),
};
decode_jws(&jws, &decoding_key, &validation)... Assuming that works, convenience methods might be a good addition but I'm not in a great position to switch gears and add it atm. |
Thank you, I will try it in the next days! |
Decoding my JWS in compact form works now for me. I had to disable also the default validation of "exp" via
|
Oh excellent! |
any updates on this MR? |
Just need some usage feedback, I'm not knowledgeable about it so I can't really have a good opinion on it |
I was thinking about this, and I wonder if it wouldn't be fine to merge as is? It's new functionality, so it won't break existing users, and if there are design or implementation issues that make it unusable making changes (even ones that are nominally breaking) it won't affect anyone because there wouldn't be any users in the first place. The main risks are that this 1. misses some use cases and the design prevents modification to allow those use cases, but that wouldn't necessarily be found by light usage feedback anyway or 2. isn't ergonomic, but I think it's a simple enough interface that that's not a large risk. |
#358