Open lmammino opened 2 years ago
This one will make @allevo proud! 😇
Any idea how to implement it? just convert it to &[u8]
and convert into Cwt
? or the string has a completely different implementation?
If the parse is made by ciborium
, just use a ciborium::de::from_reader
into it, from_reader
takes anything that impl trait Read
, and Read
is implemented for &[u8]
, a String
becomes &[u8]
with the method as_bytes
My idea was to just be an alternative to calling this function directly: https://github.com/rust-italia/dgc/blob/main/src/parse.rs#L173
So literally something like this:
impl FromStr for Cwt {
type Err = ParseError;
fn from_str(s: &str) -> Result<Self, Self::Err> {
// remove prefix
let data = remove_prefix(data)?;
// base45 decode
let decoded = decode_base45(data)?;
// decompress the data
let decompressed = decompress(decoded)?;
// parse cose payload
let cwt = parse_cwt_payload(decompressed)?;
Ok(cwt)
}
}
I only have one doubt in terms of ergonomics:
decode
returns the payload of the cwt (DgcContainer
)from_str
returns the cwt object (Cwt
)I am not really sure if it will be more ergonomic and consistent to always return a Cwt
...
One of the advantages of having a CWT is that you have access to more data (headers, signature, etc). But at the same time you don't care about this data unless you want to verify the signature (which is the reason why decode
returns directly the paylod, it is a use case that assumes you don't care about the signature, otherwise you'd use validate()
).
Thoughts?
I tried to reverse the logic: implement the parse into Cwt and DgcContainer validate
invokes it. So in this way, the decode*
public APIs are implemented in this way:
pub fn decode_cwt(data: &str) -> Result<Cwt, CwtParseError> {
data.parse()
}
pub fn decode(data: &str) -> Result<DgcCertContainer, CwtParseError> {
let cwt = decode_cwt(data)?;
Ok(cwt.payload)
}
As you can see, i changed the errortype reversing the inclusion (CwtParseError
has a variant called ParseError
). Is it make sense? Which differences are between the 2 errors?
@allevo, i like the idea of decode_cwt
and decode
. I think it will save us from a bit of code duplication.
With that being said, I am starting to think that maybe we shouldn't leak the CWT at all. At the end of the day, it is a little bit of an implementation detail. What really matter is not the encoding format (CWT) but the DgcCertContainer
which contains all the "user facing" data.
CWT makes sense only in the context of validating the signature but that's something we are covering with validate()
...
What about implementing FromStr
only for DgcCertContainer
(and not exposing decode_cwt
publicly?
I know this is a bit philosophical, but it's probably better to keep the api surface small and simple...
Regarding the 2 errors. I think i was implementing CWT as a standalone lib initially, that's maybe why i came up with 2 different error sets (one for CWT decoding and one for the whole parsing). Let's definitely revisit this decision if it does not make too much sense to you...
Removing Cwt
from the public interface would make the problem of having to return the tuple in the result - because one might want to read the data even if it's not valid - rather difficult to solve.
To hide the implementation details there are other options, like we could make Cwt
have all its fields private and make it impl Deref<Target=DgcCertContainer>
. But even then, I don't see why someone shouldn't be able to access the key id, the algorithm, and the signature.
Regarding the FromStr
proposal I don't quite like it because rustdoc hides doc comments on trait method implementations. So one can't know for sure what data the parse method parses, and implementing it for DgcCertContainer
would be even more confusing. It could be written in the documentation but from the method itself it's difficult to do understand it.
To give an example, in the docs will be written "DgcCertContainer::parse Parses a string s to return a value of this type." and someone may understand that it parases the JSON object of the greenpass, someone else might think that it parses the CBOR of the HCERT, another one may think that it parses the raw COSE data, etc.
What I agree on is merging the ParseError
types in one.
Some things I want to add that I also referenced in the other issue:
I think that those functions (decode_cwt
, and validate
) should be methods of the Cwt
struct and that the Cwt
struct should be renamed to a more specific name as we are not parsing a generic CWT but one defined by the European Commission that contains an HCERT and that is signed.
validate
might even be a method of the TrustList
but in my mind, one should be able to write
Cwt::decode(data)?.verify(trust_list) == SignatureValidity::Valid
You raise some very good points, @Rimpampa!
I am sold on the fact that it might make sense to expose the CWT (even though right now some important fields are private, maybe we should revisit that).
On the FromStr
in am not highly opinionated, so I am more than ok not adding this feature and closing this issue if we don't see a strong benefit to it! It's definitely not a priority (it does not add any new feature and it does not solve any bug).
Regarding API ergonomics I think the validation is the one that seems to be raising more skepticism, so there are probably many opportunities to revisit that.
I like your suggestions to make validate
a method of Cwt
(which gives us one more reason to keep Cwt
publicly accessible), but again, I would like to consider validation in a more comprehensive way as being discussed in #21.
If we can figure out a decent API that will allow us in the future to do "all types" of validation easily that would be awesome.
Should we close this issue then and continue the API conversation in #21?
I forgot to mention that i did some renaming in #29 and also added a diagram that should help with understanding how the data is laid out across the various containers/structs.
It would be nice to be able to do something like this:
This will be possible by implementing the
FromStr
trait onCwt
.