chore: Move `c2pa::CoseValidator` to `c2pa_crypto::RawSignatureValidator` #683

scouten-adobe · 2024-11-16T18:24:50Z

No description provided.

codecov · 2024-11-16T18:26:32Z

Codecov Report

Attention: Patch coverage is 77.81250% with 71 lines in your changes missing coverage. Please review.

Project coverage is 81.55%. Comparing base (f84c581) to head (df219df).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
sdk/src/time_stamp.rs	77.93%	32 Missing ⚠️
internal/crypto/src/raw_signature/validator.rs	31.81%	30 Missing ⚠️
internal/crypto/src/openssl/validators/mod.rs	80.00%	6 Missing ⚠️
sdk/src/cose_validator.rs	81.81%	2 Missing ⚠️
...l/crypto/src/openssl/validators/ecdsa_validator.rs	96.42%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #683      +/-   ##
==========================================
- Coverage   81.77%   81.55%   -0.22%     
==========================================
  Files         101      104       +3     
  Lines       30916    30602     -314     
==========================================
- Hits        25281    24958     -323     
- Misses       5635     5644       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

…ve existing OpenSSL implementations to it

…653

Enable tests on WASM

scouten-adobe · 2024-11-18T02:41:33Z

internal/crypto/src/openssl/validators/ecdsa_validator.rs

+            let s = BigNum::from_slice(&sig[sig_len..])?;
+            EcdsaSig::from_private_components(r, s)?.to_der()?
+        } else {
+            sig.to_vec()


@mauricefisher64 do you know of a case where the signature is not encoded as P1363?

OpenSSL/AWS KMS/X509 certs use DER. Web standards and Cose/C2PA use P1363

So when we sign with OpenSSL or we are checking certificates the values are DER.

@mauricefisher64 reason I'm asking is that the unit test framework isn't generating coverage for line 70 (the DER case) so I'm looking for sample data that would exercise that case.

Is you sign something with straight OpenSSL it will be a DER signature. You can't use a signature that came from Cose because it will be in the other format.

internal/crypto/src/openssl/validators/rsa_validator.rs

internal/crypto/src/raw_signature/validator.rs

internal/crypto/src/tests/openssl/validators/rsa_validator.rs

scouten-adobe · 2024-11-18T02:47:22Z

internal/crypto/src/webcrypto/validators/ecdsa_validator.rs

+    /// ECDSA with SHA-384
+    Es384,
+    // ECDSA with SHA-512
+    // Es512, // not yet implemented (check with Colin)


@cdmurph32 why was Es512 not implemented in #653?

Looks like the switch to Rustls here is a regression. We currently support ES512 with SubtleCrypto.

OK, that's the reason to keep the async path open. SubtleCrypto requires async.

This code path is synchronous-only.

internal/crypto/src/openssl/validators/mod.rs

internal/crypto/src/raw_signature/validator.rs

internal/crypto/src/tests/raw_signature/validator.rs

sdk/src/time_stamp.rs

scouten-adobe · 2024-11-18T22:42:45Z

sdk/src/validator.rs

@@ -29,53 +25,3 @@ pub struct ValidationInfo {
    pub cert_chain: Vec<u8>, // certificate chain used to validate signature
    pub revocation_status: Option<bool>,
 }
-
-/// Trait to support validating a signature against the provided data
-pub(crate) trait CoseValidator {


Replaced by c2pa_crypto::raw_signature::RawSignatureValidator.

scouten-adobe · 2024-11-18T22:43:12Z

sdk/src/validator.rs

-
-/// return validator for supported C2PA  algorithms
-#[cfg(feature = "openssl")]
-pub(crate) fn get_validator(alg: SigningAlg) -> Box<dyn CoseValidator> {


Replaced by c2pa_crypto::raw_signature::validator::validator_for_signing_alg.

scouten-adobe · 2024-11-18T22:43:59Z

sdk/src/openssl/mod.rs

@@ -16,33 +16,16 @@ mod rsa_signer;
 #[cfg(feature = "openssl_sign")]
 pub(crate) use rsa_signer::RsaSigner;

-#[cfg(feature = "openssl")]


These all moved into c2pa_crypto::openssl::validators.

Unify logic as much as possible

sdk/src/time_stamp.rs

gpeacock · 2024-11-19T04:20:05Z

internal/crypto/src/raw_signature/validator.rs

+    /// lacks an Eq implementation. Instead we capture the error description.
+    #[cfg(feature = "openssl")]
+    #[error("an error was reported by OpenSSL native code: {0}")]
+    OpenSslError(String),


I think this is a very good choice.

Should we allow the RawSignatureValidationError to return LogItem to get the detailed error info?

@mauricefisher64 the LogItem code was never plumbed down this far in the code being replaced. Certainly open to a subsequent PR that would add it.

@mauricefisher64 I saw this error case a few times while debugging the new code and I was pretty pleased with the description that was provided. It included the OpenSSL file name and line number.

mauricefisher64 · 2024-11-19T13:18:47Z

internal/crypto/src/raw_signature/validator.rs

+///
+/// Which validators are available may vary depending on the platform and
+/// which crate features were enabled.
+pub fn validator_for_signing_alg(alg: SigningAlg) -> Option<Box<dyn RawSignatureValidator>> {


I have to think about this. It may be too restrictive. There are cases where we validate patterns that don't match our predefined signature types. For example You can have an EC signature that uses an arbitrary hash length. They are not limited to for example ES256 where the sig is on curve prime256v1 but the hash is Sha256. You can use that curve with other hash values. We see this when validating certs.

@mauricefisher64 this is pretty directly taken from existing code in validator.rs: https://github.com/contentauth/c2pa-rs/blob/main/sdk/src/validator.rs#L62-L75

There was a separate function get_local_validator that had more flexibility and I've carried that over as validator_for_sig_and_alg_oids a little farther down in this file.

I see. We will have to do some testing against those odd cert cases.

@mauricefisher64 would love to have new test cases for anything else you're familiar with.

cdmurph32 · 2024-11-19T13:59:34Z

internal/crypto/Cargo.toml

+p256 = "0.13.2"
+p384 = "0.13.0"
+rsa = { version = "0.9.6", features = ["sha2"] }
+spki = "0.7.3"
 wasm-bindgen = "0.2.83"


wasm-bindgen, web-sys, web-time can not be included in the wasi target. I know this PR isn't really about adding WASI support, but there are other wasi targets in the Cargo.toml

mauricefisher64 · 2024-11-19T14:52:35Z

internal/crypto/src/tests/raw_signature/validator.rs

+
+const SAMPLE_DATA: &[u8] = b"some sample content to sign";
+
+#[test]


With these changes I thought these unit tests would work for both WASM and non-WASM cases?

Not sure what you're saying here. Next line flags it was a WASM-compatible test.

Unfortunately, the WASM test framework doesn't respect the standard #[test] annotation. 😢

mauricefisher64 · 2024-11-19T15:01:45Z

internal/crypto/src/webcrypto/validators/mod.rs

+    match alg {
+        SigningAlg::Es256 => Some(Box::new(EcdsaValidator::Es256)),
+        SigningAlg::Es384 => Some(Box::new(EcdsaValidator::Es384)),
+        SigningAlg::Es512 => None, /* why is this unimplemented? */


Interesting? This is implemented in the current c2pa-rs

In the synchronous case for WASM, it's not implemented. @cdmurph32 can you explain?

scouten-adobe self-assigned this Nov 16, 2024

chore: Create new RawSignatureValidator trait in c2pa_crypto and mo…

b11b39a

…ve existing OpenSSL implementations to it

scouten-adobe force-pushed the move-cose-validator branch from e4755c9 to b11b39a Compare November 17, 2024 19:54

scouten-adobe added 9 commits November 17, 2024 12:52

Add WASM (not WASI) implementation of ES256 and ES384 signatures from #…

9c2fdd1

…653

Clippy fix

3640cce

Enable tests on WASM

Fix a few WASM build issues

095c75c

Hook in webcrypto impl when available

0e29d38

cargo fmt

e32612c

Add coverage for unified RawSignatureValidator interface

53f0a1f

Bring over Ed25519 validator

dce71ee

Rename EcValidator to EcdsaValidator

e7a0f24

WASM implementation of Ed25519

e8f51a4

scouten-adobe commented Nov 18, 2024

View reviewed changes

internal/crypto/src/openssl/validators/rsa_validator.rs Outdated Show resolved Hide resolved

scouten-adobe commented Nov 18, 2024

View reviewed changes

internal/crypto/src/raw_signature/validator.rs Show resolved Hide resolved

scouten-adobe commented Nov 18, 2024

View reviewed changes

internal/crypto/src/raw_signature/validator.rs Show resolved Hide resolved

scouten-adobe commented Nov 18, 2024

View reviewed changes

internal/crypto/src/tests/openssl/validators/rsa_validator.rs Show resolved Hide resolved

scouten-adobe commented Nov 18, 2024

View reviewed changes

tmathern reviewed Nov 18, 2024

View reviewed changes

internal/crypto/src/openssl/validators/mod.rs Outdated Show resolved Hide resolved

internal/crypto/src/raw_signature/validator.rs Show resolved Hide resolved

internal/crypto/src/tests/raw_signature/validator.rs Show resolved Hide resolved

scouten-adobe added 10 commits November 18, 2024 11:30

Mark EcdsaValidator as non-exhaustive

ed095cc

Bring over RSA-PSS validators

cb2af66

Clean up EcdsaValidator code a bit

e121f4b

Add tests for impl-generic API of RsaValidator

3410c19

Bring over RSA-PSS validators for WASM from #653

2688f3d

Simplify

fbed3bd

Enable OpenSSL version of RsaLegacyValidator

3cb30d0

Add support for legacy raw-sig validator (used in time stamp validation)

825dab1

Clippy

608227e

Fix WASM build issues

ec7b255

scouten-adobe added 2 commits November 18, 2024 14:30

Remove c2pa's CoseValidator trait and implementations

949fdc2

Fix WASM build issues

450b8d3

scouten-adobe commented Nov 18, 2024

View reviewed changes

sdk/src/time_stamp.rs Show resolved Hide resolved

scouten-adobe commented Nov 18, 2024

View reviewed changes

sdk/src/time_stamp.rs Outdated Show resolved Hide resolved

scouten-adobe commented Nov 18, 2024

View reviewed changes

scouten-adobe added 11 commits November 18, 2024 15:10

Enable sync code path for verify_cose on WASM

478fdbd

Unify logic as much as possible

Add WASM impl of RSALegacyValidator

6852392

Test coverage for RsaLegacyValidator::Rsa256

a078d0f

Test coverage for RsaLegacyValidator::Rs384

30bb623

Add test coverage for RsaLegacyValidator::Rs512

5cae35d

Add test coverage for RsaLegacyValidator::Sha1

df5f04c

Add coverage for RsaLegacyValixdator edge cases

a267e5c

Test coverage for WASM impl of RsaLegacyValidator

c50e7b1

Remove redundant comparisons

341a2d5

Add test coverage for validator_for_sig_and_hash_algs

f213ab9

Add README for test signature material

9ed67b7

scouten-adobe commented Nov 19, 2024

View reviewed changes

sdk/src/time_stamp.rs Show resolved Hide resolved

Merge branch 'main' into move-cose-validator

7adf8c2

gpeacock approved these changes Nov 19, 2024

View reviewed changes

mauricefisher64 reviewed Nov 19, 2024

View reviewed changes

cdmurph32 reviewed Nov 19, 2024

View reviewed changes

mauricefisher64 reviewed Nov 19, 2024

View reviewed changes

Clippy

df219df

scouten-adobe merged commit 707e56d into main Nov 19, 2024
24 of 26 checks passed

scouten-adobe deleted the move-cose-validator branch November 19, 2024 17:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Move `c2pa::CoseValidator` to `c2pa_crypto::RawSignatureValidator` #683

chore: Move `c2pa::CoseValidator` to `c2pa_crypto::RawSignatureValidator` #683

scouten-adobe commented Nov 16, 2024

codecov bot commented Nov 16, 2024 •

edited

Loading

scouten-adobe Nov 18, 2024

mauricefisher64 Nov 19, 2024 •

edited

Loading

mauricefisher64 Nov 19, 2024

scouten-adobe Nov 19, 2024

mauricefisher64 Nov 19, 2024

scouten-adobe Nov 18, 2024 •

edited

Loading

mauricefisher64 Nov 19, 2024

scouten-adobe Nov 19, 2024

scouten-adobe Nov 19, 2024

scouten-adobe Nov 18, 2024

scouten-adobe Nov 18, 2024

scouten-adobe Nov 18, 2024

gpeacock Nov 19, 2024

mauricefisher64 Nov 19, 2024 •

edited

Loading

scouten-adobe Nov 19, 2024

scouten-adobe Nov 19, 2024

mauricefisher64 Nov 19, 2024

scouten-adobe Nov 19, 2024

mauricefisher64 Nov 19, 2024

scouten-adobe Nov 19, 2024

cdmurph32 Nov 19, 2024

mauricefisher64 Nov 19, 2024

scouten-adobe Nov 19, 2024

mauricefisher64 Nov 19, 2024

scouten-adobe Nov 19, 2024


		const SAMPLE_DATA: &[u8] = b"some sample content to sign";

		#[test]

chore: Move c2pa::CoseValidator to c2pa_crypto::RawSignatureValidator #683

chore: Move c2pa::CoseValidator to c2pa_crypto::RawSignatureValidator #683

Conversation

scouten-adobe commented Nov 16, 2024

codecov bot commented Nov 16, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

mauricefisher64 Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scouten-adobe Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauricefisher64 Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chore: Move `c2pa::CoseValidator` to `c2pa_crypto::RawSignatureValidator` #683

chore: Move `c2pa::CoseValidator` to `c2pa_crypto::RawSignatureValidator` #683

codecov bot commented Nov 16, 2024 •

edited

Loading

mauricefisher64 Nov 19, 2024 •

edited

Loading

scouten-adobe Nov 18, 2024 •

edited

Loading

mauricefisher64 Nov 19, 2024 •

edited

Loading