-
Notifications
You must be signed in to change notification settings - Fork 11
Merge jwe decrypt #47
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?
Changes from all commits
ac8ca14
bf3790e
0852f90
324b9ad
b3fb454
b9080c6
ea28a50
ceaf68c
bb29760
fd9de55
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,11 +1,19 @@ | ||||||||||||||
| pub mod jw_error; | ||||||||||||||
| pub mod jw_parser; | ||||||||||||||
| pub mod jwe; | ||||||||||||||
| pub mod jwt; | ||||||||||||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Duplicate module tree between the binary and the library crate Both pub mod jw_error;
pub mod jw_parser;
pub mod jwe;
pub mod jwt;Rust compiles these as separate crates, this means every module (
Suggested change
Suggestion: remove the |
||||||||||||||
|
|
||||||||||||||
| use crate::jwe::handle_jwe; | ||||||||||||||
| use crate::jwt::stringify_token; | ||||||||||||||
|
Comment on lines
+6
to
+7
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on the previous comment, you should now import directly from the lib with:
Suggested change
|
||||||||||||||
| use clap::{Arg, ArgAction, Command}; | ||||||||||||||
| use jwtinfo::jwt; | ||||||||||||||
| use serde_json::to_string_pretty; | ||||||||||||||
| use std::io::{self, Read}; | ||||||||||||||
| use std::process; | ||||||||||||||
| use std::{ | ||||||||||||||
| error::Error, | ||||||||||||||
| fs, | ||||||||||||||
| io::{self, Read}, | ||||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
| #[doc(hidden)] | ||||||||||||||
| fn main() -> io::Result<()> { | ||||||||||||||
| fn main() -> Result<(), Box<dyn Error>> { | ||||||||||||||
| let matches = Command::new("jwtinfo") | ||||||||||||||
| .version(env!("CARGO_PKG_VERSION")) | ||||||||||||||
| .about("Shows information about a JWT (Json Web Token)") | ||||||||||||||
|
|
@@ -31,12 +39,17 @@ fn main() -> io::Result<()> { | |||||||||||||
| .index(1) | ||||||||||||||
| .allow_hyphen_values(true) | ||||||||||||||
| .required(true) | ||||||||||||||
| .help("the JWT as a string (use \"-\" to read from stdin)"), | ||||||||||||||
| .help("the JWT/JWE as a string (use \"-\" to read from stdin)"), | ||||||||||||||
| Arg::new("key") | ||||||||||||||
| .short('K') | ||||||||||||||
| .long("key") | ||||||||||||||
| .help("the path to the private key for the cek decryption in case of JWE"), | ||||||||||||||
| ]) | ||||||||||||||
| .get_matches(); | ||||||||||||||
|
|
||||||||||||||
| let full_flag = matches.get_flag("full"); | ||||||||||||||
| let should_pretty_print = matches.get_flag("pretty"); | ||||||||||||||
|
|
||||||||||||||
| let header_flag = matches.get_flag("header"); | ||||||||||||||
| let mut token = matches.get_one::<String>("token").unwrap().clone(); | ||||||||||||||
| let mut buffer = String::new(); | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -46,40 +59,23 @@ fn main() -> io::Result<()> { | |||||||||||||
| token = (*buffer.trim()).to_string(); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| let jwt_token = match jwt::parse(token) { | ||||||||||||||
| Ok(t) => t, | ||||||||||||||
| Err(e) => { | ||||||||||||||
| eprintln!("Error: {}", e); | ||||||||||||||
| process::exit(1); | ||||||||||||||
| } | ||||||||||||||
| }; | ||||||||||||||
| // if there is a key must be a JWE | ||||||||||||||
| if let Some(key_path) = matches.get_one::<String>("key") { | ||||||||||||||
| let key = fs::read(key_path)?; | ||||||||||||||
| // handle_jwe returns the JWE payload, which could be a UTF-8 string or a | ||||||||||||||
| // JWT to decode (currently we don't handle payload as byte arrays) | ||||||||||||||
| token = handle_jwe(token, key)?; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| let stringified = if matches.get_flag("full") { | ||||||||||||||
| // Show both header and claims | ||||||||||||||
| let full_output = serde_json::json!({ | ||||||||||||||
| "header": jwt_token.header, | ||||||||||||||
| "claims": jwt_token.body | ||||||||||||||
| }); | ||||||||||||||
| if should_pretty_print { | ||||||||||||||
| to_string_pretty(&full_output)? | ||||||||||||||
| } else { | ||||||||||||||
| full_output.to_string() | ||||||||||||||
| // if the token is a JWT, jwt::parse will handle it correctly, otherwise | ||||||||||||||
| // the raw string will be printed | ||||||||||||||
| match jwt::parse(&token) { | ||||||||||||||
| Ok(jwt_token) => { | ||||||||||||||
| let stringified = | ||||||||||||||
| stringify_token(jwt_token, full_flag, should_pretty_print, header_flag)?; | ||||||||||||||
| println!("{}", stringified); | ||||||||||||||
| } | ||||||||||||||
| } else { | ||||||||||||||
| // Show either header or body | ||||||||||||||
| let part = if matches.get_flag("header") { | ||||||||||||||
| jwt_token.header | ||||||||||||||
| } else { | ||||||||||||||
| jwt_token.body | ||||||||||||||
| }; | ||||||||||||||
| if should_pretty_print { | ||||||||||||||
| to_string_pretty(&part)? | ||||||||||||||
| } else { | ||||||||||||||
| part.to_string() | ||||||||||||||
| } | ||||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
| println!("{}", stringified); | ||||||||||||||
|
|
||||||||||||||
| Err(_) => println!("{}", token), | ||||||||||||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think here we are swallowing a possible error. Not sure this is the best option... We should probably print the error to stderr and exist with an error status code |
||||||||||||||
| } | ||||||||||||||
| Ok(()) | ||||||||||||||
| } | ||||||||||||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking that there was something a bit off here and I did a couple of AI reviews with a few different models. I really liked this suggestion which I think is worth considering: Error type inconsistencies
Suggested approach to address these issuesThe root cause is a two-layer error design that doesn't quite fit together. The crypto internals ( I'd suggest one of two approaches: Option A: Typed errors all the way down (cleaner, more work)Replace #[derive(Debug, Error)]
pub enum JweCryptoError {
#[error("CEK length mismatch: expected {expected} bytes, got {actual}")]
CekLengthMismatch { expected: usize, actual: usize },
#[error("IV length invalid: expected 12 bytes")]
InvalidIvLength,
#[error("Unsupported key length: {0}")]
UnsupportedKeyLength(usize),
#[error("Decryption failed: {0}")]
DecryptionFailed(String),
#[error("Invalid RSA key: {0}")]
InvalidRsaKey(String),
#[error("Unsupported algorithm: {0}")]
UnsupportedAlgorithm(String),
}Then have the traits return Option B: Just use
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| use std::{error::Error, str, string}; | ||
| use thiserror::Error; | ||
|
|
||
| #[derive(Debug, Error)] | ||
| pub enum ParseError { | ||
| /// Indicates that a given section was not correctly Base64-encoded | ||
| #[error("Base64 error, {0}")] | ||
| InvalidBase64(#[from] base64::DecodeError), | ||
| /// Indicates that a section did not contain a valid utf8 string | ||
| #[error("UTF8 error, {0}")] | ||
| InvalidStrUtf8(#[from] str::Utf8Error), | ||
| #[error("UTF8 error, {0}")] | ||
| InvalidStringUtf8(#[from] string::FromUtf8Error), | ||
| } | ||
|
|
||
| /// Represents an error while parsing a JWT | ||
| #[derive(Debug, Error)] | ||
| pub enum JWTParseError { | ||
| /// Indicates that an expected section (Header, Body or Signature) was not found | ||
| #[error("Missing token section")] | ||
| MissingSection(), | ||
| #[error("{0}")] | ||
| InvalidFormat(#[from] ParseError), | ||
| /// Indicates that a given section did not contain a valid JSON string | ||
| #[error("JSON error, {0}")] | ||
| InvalidJSON(#[from] serde_json::error::Error), | ||
| } | ||
|
|
||
| impl From<base64::DecodeError> for JWTParseError { | ||
| fn from(err: base64::DecodeError) -> JWTParseError { | ||
| JWTParseError::InvalidFormat(ParseError::InvalidBase64(err)) | ||
| } | ||
| } | ||
|
|
||
| /// Represents an error while parsing a given part of a JWT | ||
| #[derive(Debug, Error)] | ||
| pub enum JWTParsePartError { | ||
| /// Error while parsing the Header part | ||
| #[error("Invalid Header: {0}")] | ||
| Header(JWTParseError), | ||
| /// Error while parsing the Body part | ||
| #[error("Invalid Body: {0}")] | ||
| Body(JWTParseError), | ||
| /// Error while parsing the Signature part | ||
| #[error("Invalid Signature: {0}")] | ||
| Signature(JWTParseError), | ||
| /// Error because an additional part was found after the Signature part | ||
| #[error("Error: Unexpected fragment after signature")] | ||
| UnexpectedPart(), | ||
| } | ||
|
|
||
| #[derive(Debug, Error)] | ||
| pub enum JweParseError { | ||
| #[error("Missing JWE section")] | ||
| MissingParts(), | ||
| #[error("Unexpected section")] | ||
| TooManyParts(), | ||
| #[error("{0}")] | ||
| InvalidFormat(#[from] ParseError), | ||
| } | ||
|
|
||
| impl From<base64::DecodeError> for JweParseError { | ||
| fn from(err: base64::DecodeError) -> JweParseError { | ||
| JweParseError::InvalidFormat(ParseError::InvalidBase64(err)) | ||
| } | ||
| } | ||
|
|
||
| #[derive(Debug, Error)] | ||
| pub enum JweError { | ||
| #[error("{0}")] | ||
| ParseError(#[from] JweParseError), | ||
| #[error("{0}")] | ||
| StringError(String), | ||
| #[error("{0}")] | ||
| Internal(Box<dyn Error + Send + Sync + 'static>), | ||
| #[error("not serialized error")] | ||
| JsonError(#[from] serde_json::Error), | ||
| #[error("Invalid UTF-8 string: {0}")] | ||
| InvalidUtf8Error(#[from] string::FromUtf8Error), | ||
| } | ||
|
|
||
| impl From<String> for JweError { | ||
| fn from(e: String) -> Self { | ||
| JweError::StringError(e) | ||
| } | ||
| } | ||
|
|
||
| impl From<Box<dyn Error + Send + Sync + 'static>> for JweError { | ||
| fn from(e: Box<dyn Error + Send + Sync + 'static>) -> Self { | ||
| JweError::Internal(e) | ||
| } | ||
| } | ||
|
|
||
| impl From<ParseError> for JweError { | ||
| fn from(e: ParseError) -> Self { | ||
| JweError::ParseError(JweParseError::InvalidFormat(e)) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| use base64::{ | ||
| alphabet, | ||
| engine::{self, general_purpose}, | ||
| Engine as _, | ||
| }; | ||
| use std::{convert::TryInto, sync::OnceLock}; | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor: I think we can get rid of |
||
|
|
||
| use crate::jw_error::JweParseError; | ||
| use crate::jw_error::ParseError; | ||
| use crate::jwe::jwe_handler::JweToken; | ||
|
|
||
| static BASE64_ENGINE: OnceLock<engine::GeneralPurpose> = OnceLock::new(); | ||
|
|
||
| #[inline] | ||
| pub fn get_base64() -> &'static engine::GeneralPurpose { | ||
| BASE64_ENGINE | ||
| .get_or_init(|| engine::GeneralPurpose::new(&alphabet::URL_SAFE, general_purpose::NO_PAD)) | ||
| } | ||
|
|
||
| #[doc(hidden)] | ||
| fn parse_base64_string(string_to_parse: &str) -> Result<String, ParseError> { | ||
| let bytes = get_base64().decode(string_to_parse)?; | ||
| let string = String::from_utf8(bytes)?; | ||
| Ok(string) | ||
| } | ||
|
Comment on lines
+20
to
+25
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this the same as the one we already have in the |
||
|
|
||
| pub fn split_jwe(token: &str) -> Result<[&str; 5], JweParseError> { | ||
| token | ||
| .split('.') | ||
| .collect::<Vec<&str>>() | ||
| .try_into() | ||
| .map_err(|vec: Vec<&str>| { | ||
| if vec.len() < 5 { | ||
| JweParseError::MissingParts() | ||
| } else { | ||
| JweParseError::TooManyParts() | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| pub fn parse_jwe(token: &str) -> Result<JweToken, JweParseError> { | ||
| let [b64_header, b64_key, b64_iv, b64_cipher, b64_tag] = split_jwe(token)?; | ||
|
|
||
| let decode = |s: &str| get_base64().decode(s); | ||
|
|
||
| let aad = b64_header.as_bytes().to_vec(); | ||
| let header = parse_base64_string(b64_header)?; | ||
| let key_encrypted = decode(b64_key)?; | ||
| let iv = decode(b64_iv)?; | ||
| let ciphertext = decode(b64_cipher)?; | ||
| let tag = decode(b64_tag)?; | ||
|
|
||
| Ok(JweToken::new( | ||
| header, | ||
| aad, | ||
| key_encrypted, | ||
| iv, | ||
| ciphertext, | ||
| tag, | ||
| )) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| pub mod jwe_handler; | ||
|
|
||
| use crate::jwe::jwe_handler::{AlgorithmFactory, JweHeader}; | ||
|
|
||
| use crate::jw_error::JweError; | ||
| use crate::jw_parser::parse_jwe; | ||
|
|
||
| pub fn handle_jwe(token: String, key: Vec<u8>) -> Result<String, JweError> { | ||
| let jwe_token = parse_jwe(token.as_str())?; | ||
| let jwe_header: JweHeader = serde_json::from_str(&jwe_token.header)?; | ||
| let key_decryptor = AlgorithmFactory::get_key_decryptor(jwe_header.alg.as_str())?; | ||
| let key_decrypted = key_decryptor.decrypt_cek(&key, &jwe_token.key_encrypted)?; | ||
| let content_decryptor = AlgorithmFactory::get_content_decryptor(jwe_header.enc.as_str())?; | ||
| let cipher = jwe_token.decrypt_content(&*content_decryptor, &key_decrypted)?; | ||
| let payload_string = String::from_utf8(cipher)?; | ||
|
|
||
| Ok(payload_string) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod test; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| mod algorithms; | ||
| mod jwe_token; | ||
| pub use algorithms::AlgorithmFactory; | ||
| pub use jwe_token::{JweHeader, JweToken}; |
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 think the changes proposed here come with some inconsistencies that might be nice to address. Here's a list of examples that showcase how using certain combinations of tokens and flags can lead to confusing or unexpected results for the user:
Scenario 1: JWE with plaintext payload "works"...
... but the flow is probably not what we want (also considering a previous comment about swallowing a possible error):
cargo run -- --key src/jwe/examples/example_priv.pem "$(cat src/jwe/examples/example_token.txt)"Output:
Exit code:
0This looks correct but the control flow is:
It works only because of the error-swallowing
Err(_) => println!("{}", token)discussed before.Scenario 2: Display flags (
--pretty,--full,--header) are silently ignored for non-JWT payloadsAlready hinted at this possible problem in a previous comment, but here are a few practical examples that illustrate the discrepancies:
All three produce the exact same output: (
Questo e' un messaggio super segreto! (exit 0)). The--pretty,--full, and--headerflags are completely ignored because they only apply inside theOkbranch, but we always land in theErrbranch for non-JWT payloads.Scenario 3: Regular JWT with
--keygives a cryptic errorcargo run -- --key src/jwe/examples/example_priv.pem "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJmb28iOiJiYXIifQ.dtxWM6MIcgoeMgH87tGvsNDY6cHWL6MGW4LeYvnm1JA"Output:
Exit code:
1The user gets an opaque JWE parsing error instead of something helpful like "This is a JWT, not a JWE. The
--keyflag is only applicable to JWE tokens."Perhaps this is something we can address with a more generic parser that can automatically distinguish between JWS (what we call JWT here) and JWE and give us the result in some kind of
JWTokenenum (that can be either JWS or JWT). This would allow us to then check if specific options apply (given the detected token type) and probably help us to come up with more generic ways to address the flags discrepancies mentioned above.Scenario 4: Invalid input succeeds silently (the point 1 regression)
cargo run -- "not-a-token-at-all"Output:
Exit code:
0(wrong!)Previously this printed an error to stderr and exited with code 1. Now it echoes back the garbage input as if it were valid, with a success exit code. Any script using
jwtinfoin a pipeline can no longer detect failures.