rust-italia / dgc

A parser and validator for the EU Digital Green Certificate (dgc) a.k.a. greenpass
https://github.com/rust-italia/dgc
MIT License
26 stars 11 forks source link

In-depth validation of certificates #21

Open lmammino opened 3 years ago

lmammino commented 3 years ago

Right now the library only validates a certificate based ONLY on the status of the signature. In reality a certificate can be considered invalid even if the signature is validated correctly.

As far as I understand there are several other factors that we should support in terms of validation:

Maybe we could have a dedicated CertificateValidity struct that can contain various fields like this:

pub struct CertificateValidity {
  signature: SignatureValidity,
  time: TimeValidity,
  business_rules: BusinessRulesValidity
}

SignatureValidity, TimeValidity and BusinessRulesValidity could be enums that can encapsulate all the different state of validation that is relevant for them. For instance:

pub enum TimeValidity {
  Valid,
  NotValidYet,
  Expired
}

Finally we could have a is_valid() method on the CertificateValidity struct that simply returns true or false if all the conditions are satisfied or not...

allevo commented 3 years ago

For my point of view validate function should throw (aka return a Result::Err) when some validations fail. If you need to know also the info, we can split the "parse" and the "validation" phase into 2 function call. Something like

let cwt: Cwt = decode_cwt(data).unwrap();
match cwt.validate() {
  Ok(_) => { },
  Err(err) => { .... },
}

As shortcut

fn validate(data: &str) -> Result<(), ValidationError> {
  let cwt: Cwt = decode_cwt(data).unwrap(); // this error could be transformed into a ValidationError using a dedicated variant
  cwt.validate()
}
Rimpampa commented 3 years ago

Regarding this problem I've seen that CwtHeader has kid and alg declared as Options but it seems to me that if any of those information are missing it should be considered an error.

From the European Commission

3.2 Structure of the payload

[...]

The integrity and authenticity of origin of payload data MUST be verifiable by the Verifier. To provide this mechanism, the issuer of MUST sign the CWT using an asymmetric electronic signature scheme as defined in the COSE specification (RFC 8152).


For the parsing and verification I agree with @allevo that they should be in two separate functions.

I would also add that given that the decode_cwt function returns a Cwt should be declared as a static method of the Cwt struct. This counts also for validate which directly interactes with its data.

I my mind, one should be able to write Cwt::decode(data)?.verify(trust_list) == SignatureValidity::Valid

Also it might be a good idea to rename the Cwt struct as we are not parsing a generic CWT but a specific one that contains an HCERT.

EDIT: I missed the discussion on #8, I'll talk about this last part over there