fix(core): add attestation sig validation #794
Conversation
There was a problem hiding this comment.
This is a bit of a tricky problem if we do want to guarantee that an attestation always has a valid signature.
This issue with the approach in this PR is that you are using the default signature verifier provider, which will not work when the user wants to use a custom signature scheme.
Instead of doing this using TryFrom, and during deserialization, I would suggest exposing AttestationUnchecked as a public type (perhaps #[doc(hidden)]) and having a validation method, e.g. Attestation::try_from_unchecked(attestation: AttestationUnchecked, provider: &CryptoProvider) -> Result<Attestation, E>.
tlsn-prover would explicitly use this validation upon receiving the attestation.
|
@sinu, ready for review |
| signature_alg: SignatureAlgId, | ||
| secret: EncoderSecret, | ||
| ) -> Attestation { | ||
| ) -> (Attestation, CryptoProvider) { |
There was a problem hiding this comment.
I don't think we need to couple the attestation fixture to a provider. They can be constructed separately.
| .get(&unchecked.signature.alg) | ||
| .map_err(|_| { | ||
| InvalidAttestation(format!( | ||
| "invalid signature algorithm id {:?}", |
There was a problem hiding this comment.
This isn't quite accurate. The algorithm id isn't necessarily invalid, but rather the provider is not configured with a signature verifier for this id.
This PR adds the missing validation of attestation signature.
Closes #622