Skip to content
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

JWS: Does the General vs. Flattened format make sense? #25

Open
tgross35 opened this issue Apr 16, 2023 · 7 comments
Open

JWS: Does the General vs. Flattened format make sense? #25

tgross35 opened this issue Apr 16, 2023 · 7 comments

Comments

@tgross35
Copy link
Contributor

tgross35 commented Apr 16, 2023

The current representation is as follows:

pub enum Jws {
    General(General),
    Flattened(Flattened),
}

pub struct General {
    pub payload: Option<Bytes>,
    pub signatures: Vec<Signature>,
}

pub struct Flattened {
    pub payload: Option<Bytes>,
    pub signature: Signature,
}

It seems a bit unusual to define general vs. flattened at the top level since it's only relevant for signatures. It seems like maybe something like this would work a bit better:

pub enum Jws {
    pub payload: Option<Bytes>,
    pub signatures: SomeSignatureEnum,
}

enum SomeSignatureEnum {
    Flattened(Signature),
    General(Vec<Signature>)
}

And this would at least make it easier to work with payload without needing to branch on General/Flattened.

Just curious what thoughts are here

Related: #22

@tgross35
Copy link
Contributor Author

Another alternative is to always hold a Vec<Signatures> and skip any enums, which just has the downside of always allocating

@tannaurus
Copy link

tannaurus commented Jun 28, 2024

Late to the party here, but I believe your suggestion wouldn't comply with the spec

For Flattened:

The "signatures" member MUST NOT be present when using this syntax.
Other than this syntax difference, JWS JSON Serialization objects
using the flattened syntax are processed identically to those using
the general syntax.

https://datatracker.ietf.org/doc/html/rfc7515#section-7.2.2

@tgross35
Copy link
Contributor Author

That is saying "signatures" (plural) must not be included, but "signature" (singular) is, correct? the example directly below has a signature field

@tannaurus
Copy link

That is saying "signatures" (plural) must not be included, but "signature" (singular) is, correct? the example directly below has a signature field

signatures will always be present when you serialize this.

pub struct Jws {
    pub payload: Option<Bytes>,
    pub signatures: SomeSignatureEnum, // This will always be `signatures`
}

enum SomeSignatureEnum {
    Flattened(Signature),
    General(Vec<Signature>)
}

@tgross35
Copy link
Contributor Author

tgross35 commented Jul 2, 2024

Oh I wasn't reading my own code, you're right. Thanks for the catch.

I still think it makes sense to keep bytes separate rather than being redundant. I think this would work by #[serde(flatten)]ing signatures in the JWS, then #[serde(rename = "signature[s]")] on the enum variants and marking the enum untagged.

@tannaurus
Copy link

tannaurus commented Jul 2, 2024

This implementation reflects the naming conventions of RFC, where the flattened JWS JSON Serialization contains the same values that the Flattened struct contains, and vice versa. I've historically found this helpful when reviewing implementations like this, since you can more easily compare the code to the specification. This is especially useful for this crate, in its current state, where the verification of the jwk is left to crates downstream to implement for themselves.

What value do you see in separating payload from signature(s)?

@tgross35
Copy link
Contributor Author

tgross35 commented Jul 3, 2024

What value do you see in separating payload from signature(s)?

Mostly because it better matches typical Rust design patterns to keep only varying data in enums, and extract redundant data into the parent struct. You should be able to work with the payload (copy it, decode it, append to it, etc) without thinking about the representation of how it will be signed and serialized.

This is especially useful for this crate, in its current state, where the verification of the jwk is left to crates downstream to implement for themselves.

I think that my proposal actually better aligns with what you say here. With my proposal, you can pass a Jws around and work with the payload without caring if your auth crate expects one signature or multiple.

With the existing version, you (a user working with the payload but not signing) need to either match on the signature type and handle both whenever you get a Jws object, or you need to have separate functions that take Flattened or General, or wrap this handling in a convenience method. That is, you can't read/write a Jws's payload in your application code while remaining naive about what your auth and serialization frameworks expect.

This implementation reflects the naming conventions of RFC, where the flattened JWS JSON Serialization contains the same values that the Flattened struct contains, and vice versa.

This is probably the disconnect, you can look at it from either direction. To me it seems like JWS = payload AND signature(s), i.e. the payload is always there and the signatures are just varying data. Then "flattened" or "general" are just names for how you might serialize it.

It seems like perhaps you see "flattened" and "general" as more core parts of the mental model, rather than just being related to serialization? That is, something like JWS = flattened(payload AND signature) OR general(payload AND signatures)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants