feat: implement the public metadata issuance protocol - #56
Conversation
thibmeu
left a comment
There was a problem hiding this comment.
two main comments:
- there should be cross implementation test, with typescript/go. Typescript provides a JSON that has been generated by the go implementation, might be the simplest to consume
- I'm not sure the interface is the right one. I feel that there are way to extend method solely based on the type without introducing
TokenProtocolstructure
|
I've addressed most of the review comments, and now I'm looking to see how to best address the compatibility requirements between the older and newer versions of |
|
I've decided to drop the backwards compatibility handling and conform strictly to |
|
Also, RFC 9577 §2.2.2 says the token field might be a quoted-string, so I'm implementing changes accordingly. |
thibmeu
left a comment
There was a problem hiding this comment.
a few more comments. the PR is starting to look better. some questions that remains unanswered:
- should we implement edtension/challenge negotiation. this is only partial atm
- generic batch is ok to be excluded. needs a clear documentation
- i feel that having a method for origin/issuer to validate the unknown metadata (given there will only be a few standardsied and the logic may be complex) seems important, or clearly documented and explain how to validate manually
…(challenge extension parameter)
thibmeu
left a comment
There was a problem hiding this comment.
only one comment left about required extensions. overall good for me. thanks for the back and forth @karx1
@raphaelrobert remains authoritative for a review, approval, and eventual merge though
* Add Privacy Pass expiration extension helper Register the Expiration extension type and add typed encoding/decoding helpers for draft-ietf-privacypass-expiration-extension-00. This keeps token formats and redemption policy unchanged; callers still decide how to choose and enforce timestamps. Also move tests off 0x0001 as a dummy extension type and cover the draft example. * Rename expiration error variants
See crossbeam-rs/crossbeam#1276 for more info
raphaelrobert
left a comment
There was a problem hiding this comment.
Thanks for the PR, it looks pretty clean in general and would be a good addition to the crate.
I made a first pass with a few comments and questions.
| }); | ||
| let verified = public_keys | ||
| .iter() | ||
| .any(|public_key| verify_token(public_key, &token, None).is_ok()); |
There was a problem hiding this comment.
Folding the token type and authenticator length checks into verify_token means their errors get swallowed by .is_ok(), so two RedeemTokenError variants are now unreachable through this method:
wrong token type -> InvalidSignature { token_type: PrivateP384 }
short authenticator -> InvalidSignature { token_type: Public }
Whereas before those returned TokenTypeMismatch and InvalidAuthenticatorLength. private_tokens::Server::redeem_token still returns them, so the two protocols now disagree on the same failure.
Another effect of this: a malformed token now reserves and releases a nonce before being rejected.
We should probably have better tests for this.
| #[allow(clippy::type_complexity)] | ||
| pub fn parse_www_authenticate_header_ext( | ||
| value: &HeaderValue, | ||
| ) -> Result<Vec<(Challenge, Option<ExtensionSet>, Option<Extensions>)>, ParseError> { |
There was a problem hiding this comment.
We could maybe extend `Challenge with these fields and reduce the type complexity here.
| pub fn extensions(&self) -> &Option<Extensions> { | ||
| &self.extensions | ||
| } |
There was a problem hiding this comment.
| pub fn extensions(&self) -> &Option<Extensions> { | |
| &self.extensions | |
| } | |
| pub fn extensions(&self) -> Option<&Extensions> { | |
| self.extensions.as_ref() | |
| } |
This would be more idiomatic and more in line with the rest of the API.
| return Err(CreateExtensionsError::ExtensionsUnsorted); | ||
| } | ||
|
|
||
| #[cfg(not(any(test, feature = "test-utils")))] |
| let (input, _) = opt_spaces(input)?; | ||
| let (input, value) = match key.to_lowercase().as_str() { | ||
| "challenge" | "token-key" => base64_char(input)?, | ||
| "challenge" | "token-key" => { |
There was a problem hiding this comment.
The extension draft mandates double quotes for token and extensions. It does not mandate them for challenge or token-key, and RFC 9577 allows both forms. With strict_quotes applied here, parse_www_authenticate_header_ext rejects an otherwise valid header with an unquoted challenge, including the output of build_www_authenticate_header.
|
|
||
| /// Error that occurs during extension negotiation | ||
| #[derive(Error, Debug)] | ||
| pub enum NegotiationError { |
There was a problem hiding this comment.
CreateExtensionsError and ExpirationExtensionError went into common::errors. Maybe you also want to move this one for consistency?
| }, | ||
| #[error("Expected to have a PBRSA state but there was none")] | ||
| /// Error when there is no PBRSA state when using `TokenType::PublicMetadata` | ||
| NoPbrsaState, |
There was a problem hiding this comment.
I think this variant exists only because TokenState can represent a combination that should not occur, a PublicMetadata token input with pbrsa_state == None.
Idea (TBC): Modelling the state as an enum over the two protocol shapes makes that unrepresentable and lets both NoPbrsaState and the InvalidTokenType arm in response.rs go away.
| derived_sk | ||
| .blind_sign(token_request.blinded_msg) | ||
| .inspect_err(|e| warn!(error:% = e; "Failed to blind_sign token")) | ||
| .map_err(|source| IssueTokenResponseError::BlindSignatureFailed { source })? |
There was a problem hiding this comment.
I think we want a dedicated variant like KeyNotMetadataCapable here so that the callers have a bit more information.
| /// } ExtensionType; | ||
| /// ``` | ||
| /// | ||
| /// Extension types are to be defined by the client, not by this crate |
There was a problem hiding this comment.
That's not entirely accurate, because EXPIRATION is defined below.
| #[derive(Clone, Debug, PartialEq, Eq, TlsSize, TlsDeserialize, TlsSerialize)] | ||
| pub struct TokenChallenge { | ||
| token_type: TokenType, | ||
| pub(crate) token_type: TokenType, |
There was a problem hiding this comment.
There's a token_type() getter we can use instead of making this pub(crate).
|
Hey @raphaelrobert I've tried to address your feedback in karx1#2 Yash's internship has sadly already ended so I'll be taking over this project I don't think it will be easy to take over this Pull Request so I will probably open another one |
This PR implements the issuance protocol for Publicly Verifiable tokens with Public Metadata (token type
0xDA7A).This is a revival of #25, rebased onto the current
mainbranch and updated to support draft-ietf-privacypass-public-metadata-issuance-03. This PR also addresses the feedback from that PR.This PR also pins
voprfto version0.6.0-pre.0, as cargo automatically updating to0.6.0-pre.1was causing compilation errors on a clean build with noCargo.lock.Finally, the functions
verify_tokenandparse_authorization_str/parse_authorization_str_extare some QoL additions that helped during development, but I'm happy to remove those if needed.