Open tgross35 opened 1 year ago
Another alternative is to always hold a Vec<Signatures>
and skip any enums, which just has the downside of always allocating
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.
That is saying "signatures" (plural) must not be included, but "signature" (singular) is, correct? the example directly below has a signature
field
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>)
}
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.
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)
?
What value do you see in separating
payload
fromsignature(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)
The current representation is as follows:
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:
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