Implement FromStr for Cwt

[lmammino]

It would be nice to be able to do something like this:

let cert_data = "HC1:NCFOXNEG2NBJ5*H:QO-.OMBN+XQ99N*6RFS5YUCH%BM*4ODMT0NSRHAL9.4I92P*AVAN9I6T5XH4PIQJAZGA2:UG%U:PI/E2$4JY/KB1TFTJ:0EPLNJ58G/1W-26ALD-I2$VFVVE.80Z0 /KY.SKZC*0K5AFP7T/MV*MNY$N.R6 7P45AHJSP$I/XK$M8TH1PZB*L8/G9YPDN*I4OIMEDTJCJKDLEDL9CZTAKBI/8D:8DKTDL+S/15A+2XEN QT QTHC31M3+E3+T4D-4HRVUMNMD3323623423.LJX/KQ968X2+36/-KKTC 509UE1YH/T1NTICZUI 16PPT1M:YUKQU7/EAFQ+JU2+PFQ51C5EWAC1ASBE/V9.Q5F$PYBEO.0:E5 96KA7N95ZTM L7HHP4F5G+P%YQ+GONPPCHPMR73SOM352Q4FIR L7 SP5.PDSOSNQKIR%*O* OUKRTSOL0PNLE.$FW2I9R7P3LEERQ2Q$CS8-QL4W.X0WB0/89-M7O9F:4AV0OGXP7MEKC53WRXY41A1/:R/J9URTB%LKXAD-OAZ0GA5%UCI/I910G-8H3";
let cwt: dgc::Cwt = cert_data.parse().unwrap();

This will be possible by implementing the FromStr trait on Cwt.

[lmammino]

This one will make @allevo proud! 😇

[allevo]

Any idea how to implement it? just convert it to &[u8] and convert into Cwt ? or the string has a completely different implementation?

[nappa85]

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

[lmammino]

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

[lmammino]

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)
    }
}

[lmammino]

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?

[allevo]

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?

[lmammino]

@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...

[Rimpampa]

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

[lmammino]

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?

[lmammino]

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.