Add signing and verification CLI scripts. #240

susperius · 2024-07-16T20:29:01Z

Summary

This PR adds scripts to sign and verify FileLevelManifest. It combines the existing functionality to create a fully working proof of concept.

Signing workflow:

The model folder is hashed using the SimpleFileHasher and FilesSerializer
The resulting FileLevelManifest is converted into an in-toto statement. This is only done because sigstore does not support any other payloads in DSSE envelopes.
The in-toto statement is signed and wrapped into an DSSE envelope, which in turn is wrapped into a sigstore bundle.
The sigstore bundle is converted to json and stored to disk

Verification workflow:

The sigstore bundle is loaded from disk
The bundle's signature is checked
The local model files are serialized into a FileLevelManifest using the SimpleFileHasher and FilesSerializer.
The in-toto statement is converted to a FileLevelManifest
The hashes are compared

Release Note

CLI scripts sign.py and verify.py to sign and verify models
Conversion functionality from FileLevelManifest to in-toto statement and the other way around
Adjusted README to reflect the new scripts.
Removed old main.py and model.py

mihaimaruseac · 2024-07-16T20:32:44Z

Hmm, this is slightly working against the support needed for the internal support (BCID) :-/

susperius · 2024-07-17T12:42:43Z

Hmm, this is slightly working against the support needed for the internal support (BCID) :-/

Is it about the conversion? the changes to the manifest or the CLI tool itself?

PLMK if I can help and if it's worth to try and fix the unit tests. I'd like to avoid wasting time on that if the final call is going to be that we can't merge this change.

mihaimaruseac · 2024-07-17T13:00:57Z

Let's wait a little, as I'm still testing those parts (mostly the serialization of the manifest need to support multiple formats, so it cannot be done from FileManifest itself).

susperius · 2024-07-18T13:05:03Z

Let's wait a little, as I'm still testing those parts (mostly the serialization of the manifest need to support multiple formats, so it cannot be done from FileManifest itself).

This is just the start to finally bring the functionality of the PoC into the repo. Afterwards we can work on supporting the other manifest types.

model_signing/README.md

model_signing/manifest/in_toto.py

model_signing/manifest/manifest.py

model_signing/sign.py

model_signing/serialization/serialize_by_file.py

THIS IS DRAFT, WIP. Will split into separate PRs once it works. But posting publicly to show what the plans are (sigstore#224, sigstore#248, sigstore#240, sigstore#111). Signed-off-by: Mihai Maruseac <[email protected]>

mihaimaruseac · 2024-08-05T22:41:20Z

Sorry, just saw this was sent at the same time as #276. Can you rebase again, please? There are several changes here that are no longer needed.

While here, can we use the existing main.py for the CLI instead of 2 files for signing and verification? argparse supports command modes, and this would also match sigstore-python's CLI (which would be nice, since then we can say that signatures can be verified with sigstore-python too, but not at the same level of detail (and not always))

susperius · 2024-08-06T12:37:00Z

Sorry, just saw this was sent at the same time as #276. Can you rebase again, please? There are several changes here that are no longer needed.

While here, can we use the existing main.py for the CLI instead of 2 files for signing and verification? argparse supports command modes, and this would also match sigstore-python's CLI (which would be nice, since then we can say that signatures can be verified with sigstore-python too, but not at the same level of detail (and not always))

That's very unfortunate.

Then I'm going to rebase. I decided against a single main script because it becomes way too cluttered so I prefer the one script per function approach.

mihaimaruseac

I would still prefer just one CLI script instead of two. A lot of the changes would no longer be needed if there is just one script.

Also, it seems you're redefining existing machinery.

model_signing/model.py

sign_model.py

model_signing/signature/__init__.py

model_signing/README.md

model_signing/model.py

verify_model.py

laurentsimon · 2024-08-14T00:24:22Z

Just catching up on this PR. What remains to be updated before we get a chance to merge?

mihaimaruseac · 2024-08-14T01:11:41Z

Just catching up no this PR. What remains to be updated before we get a chance to merge?

Most importantly, moving all the experimental IntotoSignature/IntotoSigner/IntotoVerifier to signature/ (#240 (comment)) as the place where they are right now require a large amount of copybara changes (via flaky rules that remove text if found in file -- as soon as the text changes the rule does not trigger). The same would then happen to the changes in the test (model_signing/signing/in_toto_test.py).

I'd still say we should have sign/verify in a single script, to match sigstore-python (#240 (comment)). The last iteration of the code here already uses subcommands to select the signer, but that should be via a flag, with a default, so we don't force users to always pass sigstore/pki/etc. Instead, sign/verify, which are related modes, with related arguments, should be unified.

There's also the fact that the README has unrelated changes (https://github.com/sigstore/model-transparency/pull/240/files#r1705707987) but I have to edit it anyway in/after #283 so this is not high priority/blocker.

susperius · 2024-08-14T14:37:10Z

Just catching up no this PR. What remains to be updated before we get a chance to merge?

Most importantly, moving all the experimental IntotoSignature/IntotoSigner/IntotoVerifier to signature/ (#240 (comment)) as the place where they are right now require a large amount of copybara changes (via flaky rules that remove text if found in file -- as soon as the text changes the rule does not trigger). The same would then happen to the changes in the test (model_signing/signing/in_toto_test.py).

I'd still say we should have sign/verify in a single script, to match sigstore-python (#240 (comment)). The last iteration of the code here already uses subcommands to select the signer, but that should be via a flag, with a default, so we don't force users to always pass sigstore/pki/etc. Instead, sign/verify, which are related modes, with related arguments, should be unified.

There's also the fact that the README has unrelated changes (https://github.com/sigstore/model-transparency/pull/240/files#r1705707987) but I have to edit it anyway in/after #283 so this is not high priority/blocker.

I'm still arguing that it isn't experimental code. I'm not sure whether you get the irony but you are complaining in a sigstore project about importing sigstore.
For script splits I already made my position clear.

BTW if you haven't seen I restored most of the README a week ago.

laurentsimon · 2024-08-19T16:31:05Z

The last PR from @mihaimaruseac got merged so last rebasing needed.
Is there a way to create tracking issues for some of the unsettled points, so we can fix them async?

mihaimaruseac · 2024-08-19T16:46:35Z

I'd say the first two (moving the code to signature -- maybe rename it to pki?) and merging the CLI scripts into a single one are blockers for merging this.

susperius · 2024-08-19T18:21:50Z

I'd say the first two (moving the code to signature -- maybe rename it to pki?) and merging the CLI scripts into a single one are blockers for merging this.

I laid out the technical reason in the lengthy thread above for why it's implemented like it is as well as why two scripts should be preferable.

Furthermore, the implementation shows a technically solid way to provide multiple raw_signers producing sigstore bundles in a single class responsible for intoto dsse payloads. Since, it is not experimental I'd like to keep there. It also aligns with the general API design outlined by @laurentsimon.

mihaimaruseac · 2024-08-20T00:16:50Z

This got to be too long, so let's summarize here the remaining points.

Location of `IntotoSignature`, `IntotoSigner` and `IntotoVerifier` classes

Add signing and verification CLI scripts. #240 (comment): we want to use sigstore-python's Bundle class to re-use existing validation checks
- Add signing and verification CLI scripts. #240 (comment): currently the class does not work outside of sigstore scenarios
  - Add signing and verification CLI scripts. #240 (comment): then we should move it to signature/, like the rest of the stuff we added before to speed up work on CLI, as we agreed on the last meeting with NVidia a month ago
    - Add signing and verification CLI scripts. #240 (comment):
      
      They actually follow the base class layout as defined by you and provide functionality for intoto based payloads. IMO we should keep the classes in the in_toto module as they fit there.
    - Add signing and verification CLI scripts. #240 (comment):
      
      We still need to define where we merge the signing mechanisms though. We should add the in_toto parts where they are right now
Add signing and verification CLI scripts. #240 (comment): with the classes as in the PR the code will be harder to import internally given the dependency on Sigstore (breaking layering)
- Add signing and verification CLI scripts. #240 (comment): use copybara
  - Add signing and verification CLI scripts. #240 (comment): it's flaky when we need to remove text from files
  - Add signing and verification CLI scripts. #240 (comment): the place where this PR adds these classes requires a large amount of copybara changes: as soon as the text changes the copybara rules no longer trigger, it is something that is hard to debug and has caused silent failures in other repositories in the past
    - Add signing and verification CLI scripts. #240 (comment)

To summarize: the PR adds several classes that depend on missing features from sigstore-python, and which would result in skipping validation. The classes are added to remove a data format round-trip, which is a good benefit. However, it would either require us to duplicate validation from sigstore-python or ask sigstore-python to give more support in handling these scenarios. This is the first reason to move.

The second reason to move is that having these classes where they are would block importing the code into the Google monorepo in a reliable way, given that it would require Copybara changes that are known to be flaky.

Since these classes can be imported in exactly the same way regardless of where they are located, I'd still say they should be in signature/.

We might rename signature/ to pki/ and have it contain all the deviations from Sigstore to support other signature formats. So, another benefit for moving them.

Number of CLI tools/scripts

Add signing and verification CLI scripts. #240 (comment): we should use just one script for sign/verify, it matches sigstore-python CLI, it matches how cosign works, it matches other tools in the ecosystem. Using argparse we can have modes.
- Add signing and verification CLI scripts. #240 (comment):
  
  I decided against a single main script because it becomes way too cluttered so I prefer the one script per function approach.
Add signing and verification CLI scripts. #240 (review): a lot of the changes in the PR would not be needed if a single CLI script were to be used instead of two (for example, a significant number of README changes)
Add signing and verification CLI scripts. #240 (comment): the iteration of the PR already uses argparse to select the signer, but this makes things worse: now users have to always specify the signer they use, instead of using a default one. Plus, the code to create the CLI arguments is now duplicated
- Add signing and verification CLI scripts. #240 (comment)
  
  For script splits I already made my position clear.

To summarize: the PR removes a single CLI script for signing and verifying and instead creates 2 separate ones. This results in duplicated code and also forces users to always specify a signer/verifier instead of having one by default.

Code can be written in a way that reduces duplication and clutter.

This is where we are before we can make progress here. I'd be willing to compromise and accept 2 CLI scripts for now, but we definitely cannot continue with the layering breakage, those classes have to move.

susperius · 2024-08-20T15:29:00Z

@mihaimaruseac I hope this solves your import issues.

Renaming signature to PKI does not make sense it supports signing with sigstore, pki and ec keys.

wrt two scripts:
You can have default values with two scripts too. Overall, it shows an easy example of how to use the library on a use case basis.

mihaimaruseac · 2024-08-20T18:12:12Z

@mihaimaruseac I hope this solves your import issues.

Sure, but since you created the file, you could have created it in the signature/ directory as said in the PR review. I won't insist on it though, just like the other PRs we might have to do cleanup after.

Renaming signature to PKI does not make sense it supports signing with sigstore, pki and ec keys.

There is signing with sigstore under signing, testing with a mocked Sigstore, 100% coverage.

wrt two scripts: You can have default values with two scripts too. Overall, it shows an easy example of how to use the library on a use case basis.

Can you have signing/verifying with Sigstore be default and show this in the README, please?

Please make sure to have good test coverage and fix the failing checks.

susperius · 2024-08-20T19:10:39Z

@mihaimaruseac I hope this solves your import issues.

Sure, but since you created the file, you could have created it in the signature/ directory as said in the PR review. I won't insist on it though, just like the other PRs we might have to do cleanup after.

Renaming signature to PKI does not make sense it supports signing with sigstore, pki and ec keys.

There is signing with sigstore under signing, testing with a mocked Sigstore, 100% coverage.

Awesome, then it can be used in a later PR. It also looks like the tests are flaky. I haven't touched the code and the tests are failing sometimes in py3.11 and sometimes in py3.12.

wrt two scripts: You can have default values with two scripts too. Overall, it shows an easy example of how to use the library on a use case basis.

Can you have signing/verifying with Sigstore be default and show this in the README, please?

We can do it in a later PR.

Please make sure to have good test coverage and fix the failing checks.

Please Mihai stop moving the goal posts. All checks worked before I was forced to rebase again and you continue to add new requests on top of it.
We can talk about tests afterwards if you want to. As you know all too well tests can also be added in a separate PR.

mihaimaruseac · 2024-08-20T19:32:15Z

We want to have good quality of code in here. I'm not moving the goalposts and the tests are definitely not flaky. But if you show me two runs on the same code where a test fails then passes I can revisit it.

You said that we can have a default mode with the 2 scripts in the CLI, I wanted to see that in this PR.

Regarding tests: in all of your PRs you've been asked to write them. Now we have a checker for coverage, but this is not moving the goalposts. We always asked for tests.

susperius · 2024-08-20T19:43:45Z

We want to have good quality of code in here. I'm not moving the goalposts and the tests are definitely not flaky. But if you show me two runs on the same code where a test fails then passes I can revisit it.

You said that we can have a default mode with the 2 scripts in the CLI, I wanted to see that in this PR.

It fails here in py3.11: https://github.com/sigstore/model-transparency/actions/runs/10475773857/job/29013190889
And py3.11 here: https://github.com/sigstore/model-transparency/actions/runs/10477683799/job/29019352610
It fails here in py3.12: https://github.com/sigstore/model-transparency/actions/runs/10477482257/job/29018678803

without me touching the code.

I won't add any major things to this PR.
We can do follow up PRs with tests. I don't see any reason why you can do that but I can't.

README.model_signing.md

src/verify.py

tests/signing/in_toto_signature_test.py

mihaimaruseac · 2024-08-20T19:57:36Z

We want to have good quality of code in here. I'm not moving the goalposts and the tests are definitely not flaky. But if you show me two runs on the same code where a test fails then passes I can revisit it.
You said that we can have a default mode with the 2 scripts in the CLI, I wanted to see that in this PR.

It fails here in py3.11: https://github.com/sigstore/model-transparency/actions/runs/10475773857/job/29013190889 And py3.11 here: https://github.com/sigstore/model-transparency/actions/runs/10477683799/job/29019352610 It fails here in py3.12: https://github.com/sigstore/model-transparency/actions/runs/10477482257/job/29018678803

Thank you. I'll look into those. They are all on Windows, fail with the same error message. Windows is not required to merge.

I won't add any major things to this PR. We can do follow up PRs with tests. I don't see any reason why you can do that but I can't.

The only tests I moved for later was for mocking sigstore and that was pushed for later since you wanted that PR to land to continue on your pushes. This is not the case here.

di · 2024-08-20T20:12:54Z

We want to have good quality of code in here. I'm not moving the goalposts and the tests are definitely not flaky. But if you show me two runs on the same code where a test fails then passes I can revisit it.
You said that we can have a default mode with the 2 scripts in the CLI, I wanted to see that in this PR.

It fails here in py3.11: https://github.com/sigstore/model-transparency/actions/runs/10475773857/job/29013190889 And py3.11 here: https://github.com/sigstore/model-transparency/actions/runs/10477683799/job/29019352610 It fails here in py3.12: https://github.com/sigstore/model-transparency/actions/runs/10477482257/job/29018678803

Thank you. I'll look into those. They are all on Windows, fail with the same error message. Windows is not required to merge.

Two possible culprits here are a) TUF filepath lengths being too long for Windows to handle and b) files being moved/renamed while a file handler still has them open. Not sure if either are relevant here but thought I'd offer a pointer. (see also theupdateframework/python-tuf#454), h/t @woodruffw).

mihaimaruseac · 2024-08-20T20:55:16Z

I think it is because I didn't mock it properly, testing in #297

mihaimaruseac · 2024-08-20T21:18:50Z

I think it is because I didn't mock it properly, testing in #297

That seemed to be the reason, after more mocking 5 consecutive CI runs passed without problems. There should be no merge conflict and there is no requirement to rebase this on top of #297.

The flakyness raised an issue on the future e2e test though: it seems we run into the issue b) that Dustin mentioned. We'll solve that when we have e2e tests.

mihaimaruseac

Almost there, please change type of ignored_paths. The others could be considered as nits, but easy to fix in this PR too, since they are one-line fixes each.

Sad that this makes CLI work with anything but sigstore.

src/model_signing/model.py

README.model_signing.md

tests/signing/in_toto_signature_test.py

src/sign.py

mihaimaruseac · 2024-08-21T16:31:27Z

Oh, one of your commits fails DCO.

src/sign.py

src/model_signing/model.py

susperius · 2024-08-21T17:06:08Z

Oh, one of your commits fails DCO.

Ok I think I broke it completely

Edit: fixed it (?)

Signed-off-by: Martin Sablotny <[email protected]>

mihaimaruseac

Thank you!

susperius requested review from a team as code owners July 16, 2024 20:29

susperius force-pushed the feat/new_manifest_cli branch from cdd4e12 to 7e90e2f Compare July 16, 2024 20:32

susperius force-pushed the feat/new_manifest_cli branch 2 times, most recently from 8242bbb to a168f07 Compare July 19, 2024 13:14

mihaimaruseac requested changes Jul 23, 2024

View reviewed changes

mihaimaruseac mentioned this pull request Jul 24, 2024

WIP - DO NOT MERGE - Will be split: Signing machinery #253

Closed

mihaimaruseac mentioned this pull request Jul 27, 2024

Add in-toto format with hashes of shards as subjects #267

Merged

susperius force-pushed the feat/new_manifest_cli branch from 3743871 to eca7b4a Compare August 5, 2024 20:43

susperius requested a review from mihaimaruseac August 5, 2024 20:44

mihaimaruseac requested changes Aug 6, 2024

View reviewed changes

mihaimaruseac added this to the V1 release milestone Aug 6, 2024

mihaimaruseac reviewed Aug 6, 2024

View reviewed changes

model_signing/model.py Outdated Show resolved Hide resolved

verify_model.py Outdated Show resolved Hide resolved

spencerschrock mentioned this pull request Aug 7, 2024

Setup automated generation of model_signing documentation #279

Merged

This was referenced Aug 8, 2024

Automated generation of API docs #209

Closed

Prepare for wheel publishing (alpha patch version of 0.0.1) #283

Merged

mihaimaruseac requested changes Aug 20, 2024

View reviewed changes

mihaimaruseac reviewed Aug 21, 2024

View reviewed changes

mihaimaruseac previously approved these changes Aug 21, 2024

View reviewed changes

src/sign.py Outdated Show resolved Hide resolved

src/model_signing/model.py Outdated Show resolved Hide resolved

susperius force-pushed the feat/new_manifest_cli branch from 1d65b94 to 78fa0ff Compare August 21, 2024 17:04

susperius dismissed mihaimaruseac’s stale review via 414e9a1 August 21, 2024 17:18

susperius force-pushed the feat/new_manifest_cli branch from 78fa0ff to 414e9a1 Compare August 21, 2024 17:18

<3 git

91ecc58

Signed-off-by: Martin Sablotny <[email protected]>

susperius force-pushed the feat/new_manifest_cli branch from 414e9a1 to 91ecc58 Compare August 21, 2024 17:22

linter

7048a7a

Signed-off-by: Martin Sablotny <[email protected]>

mihaimaruseac approved these changes Aug 21, 2024

View reviewed changes

mihaimaruseac merged commit e18b1d5 into sigstore:main Aug 21, 2024
17 checks passed

susperius deleted the feat/new_manifest_cli branch August 21, 2024 17:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add signing and verification CLI scripts. #240

Add signing and verification CLI scripts. #240

susperius commented Jul 16, 2024

mihaimaruseac commented Jul 16, 2024

susperius commented Jul 17, 2024 •

edited

Loading

mihaimaruseac commented Jul 17, 2024

susperius commented Jul 18, 2024

mihaimaruseac commented Aug 5, 2024

susperius commented Aug 6, 2024 •

edited

Loading

mihaimaruseac left a comment

laurentsimon commented Aug 14, 2024 •

edited

Loading

mihaimaruseac commented Aug 14, 2024

susperius commented Aug 14, 2024

laurentsimon commented Aug 19, 2024

mihaimaruseac commented Aug 19, 2024

susperius commented Aug 19, 2024

mihaimaruseac commented Aug 20, 2024 •

edited

Loading

susperius commented Aug 20, 2024

mihaimaruseac commented Aug 20, 2024 •

edited

Loading

susperius commented Aug 20, 2024

mihaimaruseac commented Aug 20, 2024 •

edited

Loading

susperius commented Aug 20, 2024

mihaimaruseac commented Aug 20, 2024 •

edited

Loading

di commented Aug 20, 2024

mihaimaruseac commented Aug 20, 2024

mihaimaruseac commented Aug 20, 2024

mihaimaruseac left a comment

mihaimaruseac commented Aug 21, 2024

susperius commented Aug 21, 2024 •

edited

Loading

mihaimaruseac left a comment

Add signing and verification CLI scripts. #240

Add signing and verification CLI scripts. #240

Conversation

susperius commented Jul 16, 2024

Summary

Release Note

mihaimaruseac commented Jul 16, 2024

susperius commented Jul 17, 2024 • edited Loading

mihaimaruseac commented Jul 17, 2024

susperius commented Jul 18, 2024

mihaimaruseac commented Aug 5, 2024

susperius commented Aug 6, 2024 • edited Loading

mihaimaruseac left a comment

Choose a reason for hiding this comment

laurentsimon commented Aug 14, 2024 • edited Loading

mihaimaruseac commented Aug 14, 2024

susperius commented Aug 14, 2024

laurentsimon commented Aug 19, 2024

mihaimaruseac commented Aug 19, 2024

susperius commented Aug 19, 2024

mihaimaruseac commented Aug 20, 2024 • edited Loading

Location of IntotoSignature, IntotoSigner and IntotoVerifier classes

Number of CLI tools/scripts

susperius commented Aug 20, 2024

mihaimaruseac commented Aug 20, 2024 • edited Loading

susperius commented Aug 20, 2024

mihaimaruseac commented Aug 20, 2024 • edited Loading

susperius commented Aug 20, 2024

mihaimaruseac commented Aug 20, 2024 • edited Loading

di commented Aug 20, 2024

mihaimaruseac commented Aug 20, 2024

mihaimaruseac commented Aug 20, 2024

mihaimaruseac left a comment

Choose a reason for hiding this comment

mihaimaruseac commented Aug 21, 2024

susperius commented Aug 21, 2024 • edited Loading

mihaimaruseac left a comment

Choose a reason for hiding this comment

susperius commented Jul 17, 2024 •

edited

Loading

susperius commented Aug 6, 2024 •

edited

Loading

laurentsimon commented Aug 14, 2024 •

edited

Loading

mihaimaruseac commented Aug 20, 2024 •

edited

Loading

Location of `IntotoSignature`, `IntotoSigner` and `IntotoVerifier` classes

mihaimaruseac commented Aug 20, 2024 •

edited

Loading

mihaimaruseac commented Aug 20, 2024 •

edited

Loading

mihaimaruseac commented Aug 20, 2024 •

edited

Loading

susperius commented Aug 21, 2024 •

edited

Loading