-
Notifications
You must be signed in to change notification settings - Fork 99
feat: Add CAWG validation to Reader
#1370
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1370 +/- ##
===========================================
- Coverage 78.41% 28.13% -50.29%
===========================================
Files 162 144 -18
Lines 39515 27439 -12076
===========================================
- Hits 30986 7719 -23267
- Misses 8529 19720 +11191 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c2pa_c_ffi/src/c_api.rs
Outdated
|
||
// Run CAWG post-validation - this is async and requires a runtime. | ||
fn post_validate(result: Result<C2paReader, c2pa::Error>) -> Result<C2paReader, c2pa::Error> { | ||
if true { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will you need sync APIs here?
sdk/examples/cawg_identity.rs
Outdated
let mut reader = Reader::from_file(dest)?; | ||
|
||
reader.post_validate_async(&CawgValidator {}).await?; | ||
let reader = Reader::from_file_async(dest).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not keep the sync interface too? I am not sure we can make everything async
sdk/src/reader.rs
Outdated
Self::from_store(store, &validation_log) | ||
let /* mut */ result = Self::from_store(store, &validation_log)?; | ||
if false { | ||
// QUESTION: What to do if we're in the _sync version and there |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I wouldn't want everything to become async. I wonder if we need the sync interface for some native bindings? It's not great but maybe we need a blocking API? Or a cawg validation opt-out for the sync path?
CodSpeed Performance ReportMerging #1370 will not alter performanceComparing Summary
Footnotes
|
Reader
ba322ff
to
5270494
Compare
I think I've solved the sync/async issues by leaving the existing approach in C API unchanged. I've added a new convenience function (async only) which does the combined test. That's what we were doing in the C API wrapper functions already and I've removed my changes to those functions. |
sdk/src/reader.rs
Outdated
|
||
#[test] | ||
fn test_reader_post_validate() -> Result<()> { | ||
if false { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed this was still here. Will remove shortly.
sdk/src/reader.rs
Outdated
format: &str, | ||
stream: impl Read + Seek, | ||
) -> Result<Reader> { | ||
let mut reader = Self::from_stream_async(format, stream).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementations in
#[cfg(target_arch = "wasm32")]
pub async fn from_stream_with_cawg_async
and
#[cfg(not(target_arch = "wasm32"))]
pub async fn from_stream_with_cawg_async
are the same. Is there no way to reuse instead of duplicating? No trick to bypass the differences in requesteds traits on the stream ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish … I wish …
Sadly, there is not such a trick. (I've looked.)
sdk/src/reader.rs
Outdated
if get_settings_value::<bool>("verify.verify_after_reading")? { | ||
reader.post_validate_async(&CawgValidator {}).await?; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So without verify.verify_after_reading
, this will behave the same as Reader::from_stream_async
?
Unless I'm mistaken, that feels a bit unintuitive to me. I'd think that calling from_stream_with_cawg_async
would be "enough" from an end-user perspective to opt in to CAWG validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just say that Reader::from_stream_async() supports cawg but reader_from_stream() doesn't yet. The same would go for Reader:from_file() I don't see why we should add new cawg specific methods here.
Eventually, hopefully the sync methods can support cawg too.
Another option is that we can configure the cawg support with a setting. So if you have cawg enabled we always do the cawg call and if you don't there's no extra overhead.
Maybe there's a separate (or the same) option that allows the sync methods to block on the async call. you can can disable the setting if you don't want that behavior.
sdk/src/reader.rs
Outdated
Self::from_store(store, &validation_log) | ||
let /* mut */ result = Self::from_store(store, &validation_log)?; | ||
if false { | ||
// QUESTION: What to do if we're in the _sync version and there |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to support both sync and async identity validation if we are going to include it at this level in the API. If it is part of validation, it can't just be async validation.
} | ||
|
||
// Run CAWG post-validation - this is async and requires a runtime. | ||
fn post_validate(result: Result<C2paReader, c2pa::Error>) -> Result<C2paReader, c2pa::Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PostValidate could be useful for validating and formatting any 3rd party assertions we don't have in the SDK.
Ideally there would be some way to integrate this into the definition of an Assertion such that you would just need to add an assertion handler to the SDK, but that's a problem for tomorrow.
sdk/src/reader.rs
Outdated
/// [CAWG identity]: https://cawg.io/identity | ||
/// [from_stream_with_cawg_async()]: Self::from_stream_with_cawg_async | ||
#[cfg(not(target_arch = "wasm32"))] | ||
pub async fn from_stream_with_cawg_async( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TO DO: Delete this and fold back into the from_stream_async call. Add a note (depending on outcome) that says not supported on sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TO DO: If CAWG identity assertion is encountered, do the asynchronous blocking call in the sync implementation (borrow from the C bindings). Gavin may add a setting to disable if the blocking behavior is undesirable.
TO DO: Review c2patool usage. |
TO DO: Check in to whether verify-on-sign does CAWG identity assertion verification and if it becomes async as a result. |
This is a sketch of the changes I'd like to see made to the
Reader
interface to make CAWG more directly part of the reading process from clients' point of view.