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

In-depth validation of certificates #21

Open
lmammino opened this issue Nov 24, 2021 · 2 comments
Open

In-depth validation of certificates #21

lmammino opened this issue Nov 24, 2021 · 2 comments
Labels
enhancement New feature or request low priority

Comments

@lmammino
Copy link
Member

lmammino commented Nov 24, 2021

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:

In the context of this issue I think it will important to figure out an ergonomic API that:

  1. should make it easy to validate the certificate in one single operation (function call)
  2. Return the certificate data (if we can parse that correctly)
  3. Return a clear error in case of validation failed (for instance it's very important to distinguish whether a certificate is expired or whether it doesn't satisfy a specific regional rule)

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
Copy link
Contributor

allevo commented Nov 26, 2021

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
Copy link
Collaborator

Rimpampa commented Nov 27, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request low priority
Projects
None yet
Development

No branches or pull requests

3 participants