-
Notifications
You must be signed in to change notification settings - Fork 127
Asymmetric authentication #731
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
base: main
Are you sure you want to change the base?
Conversation
kixelated
left a comment
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.
Super cool.
Very fast review on my phone. I would use aws-lc-rs for the crypto operations and remove the feature flags.
rs/moq-token/Cargo.toml
Outdated
| serde_json = "1" | ||
| serde_with = { version = "3", features = ["base64"] } | ||
|
|
||
| rsa = { version = "0.9.9", optional = true } |
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'd use ring or aws-lc-rs. The family of Rust crypto crates are pretty bad for usability and performance.
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 it with aws-lc-rs at first but unfortunately it seems like the KeyPair doesn't expose the private_key components as in the rsa crate, please correct me if I'm missing something. I also didn't really want to bother parsing the DER that I could export from the KeyPair. ring doesn't seem much different. I've implemented the RngCore trait using aws-lc-rs and then use it as the RNG for rsa.

rs/moq-token/src/generate.rs
Outdated
| } | ||
|
|
||
| #[cfg(feature = "jwk-ec")] | ||
| fn generate_ec_key(curve: EllipticCurve) -> Key { |
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.
There's no easier way to do this?
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.
Updated the function
kixelated
left a comment
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.
Ideally we can reuse the same key files so it's not a breaking relay change.
| decode: Default::default(), | ||
| encode: Default::default(), | ||
| }; | ||
| match key { |
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.
key? is simpler.
|
|
||
| let x = point | ||
| .x() | ||
| .ok_or_else(|| anyhow::anyhow!("Missing x() point in EC key"))? |
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.
use anyhow::Context;
| .ok_or_else(|| anyhow::anyhow!("Missing x() point in EC key"))? | |
| .context("Missing x() point in EC key")? |
| #[derive(Clone)] | ||
| pub struct Auth { | ||
| key: Option<Arc<moq_token::Key>>, | ||
| key: Option<Arc<moq_token::JWK>>, |
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.
Maybe just keep this as Key? It's fine if it's not backwards compatible.
Adds support for asymmetric JWKs to the Rust implementation.
I've added the "kty" parameter to the JWK as per RFC (https://datatracker.ietf.org/doc/html/rfc7517#section-4.1), this was not present before but is required now (as stated in the RFC "This member MUST be present in a JWK."). This is a breaking change though as all previously generated tokens would have to be updated, I'm not sure whether that is a big deal? If so this would require "oct" to be the default value.
Also the TypeScript part is not implemented yet.