Skip to content
This repository has been archived by the owner on Dec 11, 2024. It is now read-only.

(MINOR) allow clients to sign with a process outside of c2patool #169

Merged
merged 11 commits into from
Mar 20, 2024

Conversation

dyro
Copy link
Contributor

@dyro dyro commented Mar 14, 2024

Changes in this pull request

This PR introduces a way for clients to sign claim bytes using an external process. There are two open questions I have:

  1. I had to introduce a couple of bins for integration tests. The problem is one of these binaries, namely the signer-process-success uses openssl, so I had to add that as a dependency to c2patool. I tried adding these as examples, but cargo examples aren't compiled before running cargo test. Those tests would fail unless we ran cargo build --examples; cargo test.
  2. I don't think we can calculate the --reserve-size argument dynamically. This will change if a user includes a custom TSA URL. In claims-signer we hard-coded a value, but I'm worried about doing that here. Any suggestions? I'm currently requiring the user to supply the value, is that okay? We could also use the cert size for the reserve size if there's no TSA url in the sign config and only require the reserve size argument if the user specified a tsa url.

I'll write docs after we resolve the reserve-size question!

Checklist

  • This PR represents a single feature, fix, or change.
  • All applicable changes have been documented.
  • Any TO DO items (or similar) have been entered as GitHub issues and the link to that issue has been included in a comment.

@dyro dyro force-pushed the dyross/remote-signer branch from d7bd699 to f4cecc6 Compare March 14, 2024 17:37
@codecov-commenter
Copy link

codecov-commenter commented Mar 14, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 62.73885% with 117 lines in your changes missing coverage. Please review.

Project coverage is 46.31%. Comparing base (effdb8d) to head (fd8e890).
Report is 46 commits behind head on main.

Files with missing lines Patch % Lines
src/bin/signer-path-success.rs 0.00% 52 Missing ⚠️
src/callback_signer.rs 80.08% 49 Missing ⚠️
src/main.rs 0.00% 11 Missing ⚠️
src/bin/signer-path-fail.rs 0.00% 3 Missing ⚠️
src/bin/signer-path-no-stdout.rs 0.00% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

❗ There is a different number of reports uploaded between BASE (effdb8d) and HEAD (fd8e890). Click for more details.

HEAD has 6 uploads less than BASE
Flag BASE (effdb8d) HEAD (fd8e890)
9 3
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #169       +/-   ##
===========================================
- Coverage   76.10%   46.31%   -29.80%     
===========================================
  Files           3        7        +4     
  Lines         406      719      +313     
===========================================
+ Hits          309      333       +24     
- Misses         97      386      +289     

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

@dyro dyro force-pushed the dyross/remote-signer branch from c8db773 to f0d498e Compare March 14, 2024 21:22
tests/integration.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@dyro dyro force-pushed the dyross/remote-signer branch from 4340f8a to b5dcc38 Compare March 18, 2024 17:59
@dyro dyro changed the title allow clients to sign with a process outside of c2patool (minor) allow clients to sign with a process outside of c2patool Mar 20, 2024
@dyro dyro changed the title (minor) allow clients to sign with a process outside of c2patool (Minor) allow clients to sign with a process outside of c2patool Mar 20, 2024
@dyro dyro changed the title (Minor) allow clients to sign with a process outside of c2patool (MINOR) allow clients to sign with a process outside of c2patool Mar 20, 2024
@dyro dyro merged commit bc99ee1 into main Mar 20, 2024
10 checks passed
@dyro dyro deleted the dyross/remote-signer branch March 20, 2024 17:22
scouten-adobe pushed a commit to contentauth/c2pa-rs that referenced this pull request Dec 10, 2024
…ntauth/c2patool#169)

* allow clients to sign with a process outside of c2patool

* builds bins for coverage tests

* removes pub key

* removes extra println

* includes stderr; shows stderr from child process is included in error

* adds better docs

* improves docs; renames signer-process to signer-path.

* adds docs and default value for reserve_size

* replaces TODO with proper docs

* removes extraneous else

---------

Co-authored-by: Dylan Ross <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants