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

C2PA metadata handling #662

Merged
merged 2 commits into from
Dec 26, 2024
Merged

C2PA metadata handling #662

merged 2 commits into from
Dec 26, 2024

Conversation

kornelski
Copy link
Contributor

@kornelski kornelski commented Dec 23, 2024

There's Adobe's initiative to embed "provenance" metadata in files along with a cryptographically signed history of file changes. The metadata is invalidated if the file is changed in any way without updating the signature. Only parties trusted by Adobe are allowed to sign files.

AFAIK an open-source program can't be given a private key to sign its changes, so oxipng's only choices are to either avoid changing the signed files at all, or strip the invalidated c2pa metadata when optimizing files.

I've made oxipng strip the c2pa metadata by default. If explicit --keep=caBX is used, then the file will be skipped with a warning "The image contains C2PA manifest that would be invalidated by any file changes".

Copy link
Collaborator

@AlexTMjugador AlexTMjugador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you a bunch for the PR!

I do question how open a standard truly is if, in practice, implementers end up hardcoding yet another centralized trust list curated by Adobe, especially when considering that a explicit design goal of the C2PA spec is "Whole Workflow Applicability". To be fair, the C2PA Implementation Guide mentions DLTs and NFTs as solutions to that problem, but I don't see them being relevant in practical applications. That said, this definitely is a topic better suited for discussion elsewhere, not on a technically good PR review 😂

I also like how you refactored the main function to return an ExitCode instead of calling exit, which plays nicer with destructors.

I just have a small nitpick before we can merge this.

src/headers.rs Outdated Show resolved Hide resolved
@kornelski
Copy link
Contributor Author

Yes, the C2PA standard rides on hype and buzzwords. The NFT part is a handwavey nonsense just like NFTs themselves. The chain of trust for applications only makes sense for Cloud/SaaS image processing, and really works only on trust.

I don't mean to endorse C2PA with this PR, only provide tools for dealing with images that contain this data.

@AlexTMjugador AlexTMjugador merged commit 8270c6a into shssoichiro:master Dec 26, 2024
12 checks passed
@AlexTMjugador
Copy link
Collaborator

AlexTMjugador commented Dec 26, 2024

Thanks again, all is clear and looks good to me! Merging this 🚀

@kornelski kornelski deleted the c2pa branch December 26, 2024 23:14
// StripChunks::None is the default value, so to keep optimizing by default,
// interpret it as stripping the C2PA metadata.
// The C2PA metadata is invalidated if the file changes, so it shouldn't be kept.
if opts.strip == StripChunks::None {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also strip automatically if StripChunks::Strip was used? I.e. only error if caBX was explicitly kept with StripChunks::Keep.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This point is certainly open to discussion and could be adjusted in the future based on user feedback and similar inputs. For now, I believe the logic implemented in this PR offers a good default behavior without overburdening the Strip and Keep variants with additional complexity, as users relying on them are, in my view, more likely to be sensitive to different chunk selection strategies. But I don't have a strong opinion one way or another, caBX is by design a bit of a weird chunk anyway.

src/main.rs Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants