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.

Sign up for GitHub

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

Jump to bottom

Add signing and verification methods based on in-toto statements #193

Merged
merged 11 commits into from
Jul 9, 2024
Merged

Add signing and verification methods based on in-toto statements #193

merged 11 commits into from
Jul 9, 2024

Conversation

susperius
Copy link
Contributor

This PR adds functionality to sign and verify models with Sigstore, "Bring Your Own Key" and "Bring Your Own PKI".

The signing part expects an in-toto statements and returns a signed sigstore bundle with the appropriate verification material added.
The verification part takes a sigstore bundle as input and verifies the signature over the DSSE envelope payload.

This PR is split out from #177 .

@susperius susperius requested a review from a team as a code owner June 3, 2024 14:40
@susperius
Copy link
Contributor Author

susperius commented Jun 3, 2024 •
edited
Loading

@laurentsimon, @font and @mihaimaruseac this is the first part split out from #177 . I started with it because it is a standalone part of the former change.

I did not address any comments to make sure nothing is missed. Please be so kind and repost your comments here.

font
font reviewed Jun 3, 2024
View reviewed changes
Copy link
Member

@font font left a comment

Choose a reason for hiding this comment

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

@susperius Thanks for splitting this out! I made a first pass and it generally looks really good. I made a few comments. The main one being that I think we should also offer a way to sign and upload to Sigstore's Rekor transparency log using an existing key instead of just the keyless option. What do you think?

model_signing/signature/__init__.py Show resolved Hide resolved
model_signing/signature/__init__.py Outdated Show resolved Hide resolved
model_signing/signature/signing.py Outdated Show resolved Hide resolved
model_signing/signature/signing.py Outdated Show resolved Hide resolved
model_signing/signature/signing.py Outdated Show resolved Hide resolved
mihaimaruseac
mihaimaruseac requested changes Jun 4, 2024
View reviewed changes
Copy link
Collaborator

@mihaimaruseac mihaimaruseac 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 for the PR. I think main worry I have is that this uses too much tight coupling with sigstore and won't work with other use cases we are engaged with

model_signing/signature/__init__.py Show resolved Hide resolved
model_signing/signature/__init__.py Outdated Show resolved Hide resolved
model_signing/signature/signing.py Show resolved Hide resolved
model_signing/signature/signing.py Show resolved Hide resolved
model_signing/signature/signing.py Outdated Show resolved Hide resolved
model_signing/signature/signing.py Outdated Show resolved Hide resolved
model_signing/signature/utils.py Outdated Show resolved Hide resolved
@susperius susperius force-pushed the main branch 2 times, most recently from d487ac1 to f638bc3 Compare June 4, 2024 10:14
@mihaimaruseac mihaimaruseac mentioned this pull request Jun 5, 2024
Merged
@susperius susperius force-pushed the main branch 2 times, most recently from 03f2a4d to af780b1 Compare June 6, 2024 08:25
@susperius
signing and verification
1fe45ea 
Signed-off-by: Martin Sablotny <msablotny@nvidia.com>
@susperius
Copy link
Contributor Author

@mihaimaruseac and @laurentsimon what tooling do you use to create/update the requirement (lock) files?

@mihaimaruseac
Copy link
Collaborator

@mihaimaruseac and @laurentsimon what tooling do you use to create/update the requirement (lock) files?

We have a GitHub action: https://github.com/sigstore/model-transparency/actions/workflows/pin_deps.yml

It can be manually triggered and the target branch could be set to a PR branch, but I have not tested it.

@mihaimaruseac mihaimaruseac added this to the V1 release milestone Jun 21, 2024
@susperius susperius requested a review from a team as a code owner June 24, 2024 08:21
@susperius
Copy link
Contributor Author

@mihaimaruseac what's the way forward with this pull request?
Could we merge it and create another one to change it to the manifest format?

@mihaimaruseac
Copy link
Collaborator

Hmm, let me check a couple of things. I'd like to have either this or the manifest progress by Wednesday meeting

mihaimaruseac
mihaimaruseac reviewed Jun 25, 2024
View reviewed changes
.gitignore Outdated Show resolved Hide resolved
@mihaimaruseac
Copy link
Collaborator

Hmm, let me check a couple of things. I'd like to have either this or the manifest progress by Wednesday meeting

Let's go with this one first, the manifest work seems to need more time :(.

Can you rebase this back on main branch, please?

@susperius
Copy link
Contributor Author

Hmm, let me check a couple of things. I'd like to have either this or the manifest progress by Wednesday meeting

Let's go with this one first, the manifest work seems to need more time :(.

Can you rebase this back on main branch, please?

Done

@susperius susperius force-pushed the main branch 3 times, most recently from 57e6316 to 0e0886a Compare July 1, 2024 13:29
@susperius
changed behavior to raise an exception on failure
5e02546 
license
gitignore newline
add dependencies

Signed-off-by: Martin Sablotny <msablotny@nvidia.com>

deps
@susperius
Copy link
Contributor Author

@mihaimaruseac friendly ping

@mihaimaruseac
Copy link
Collaborator

Sorry, been ~OOO for the past 1.5 weeks.

Hmm, there's a unit test failure, a module not found error.

I'm ok with the PR, leaving to @laurentsimon for final review.

@susperius
gitignore;requirements
1ab4b7f 
Signed-off-by: Martin Sablotny <msablotny@nvidia.com>
@susperius
Copy link
Contributor Author

Sorry, been ~OOO for the past 1.5 weeks.

Hmm, there's a unit test failure, a module not found error.

I'm ok with the PR, leaving to @laurentsimon for final review.

No worries!

Hope I fixed the deps issue.

@susperius susperius mentioned this pull request Jul 8, 2024
Open
@mihaimaruseac
Copy link
Collaborator

This is weird, we should not list the same dependency twice. Did you upgrade the frozen deps files?

@susperius
Copy link
Contributor Author

This is weird, we should not list the same dependency twice. Did you upgrade the frozen deps files?

I was surprised too.
No, only updated the requirements.in and requirements_test.in. Could you please run the workflow on the PR or try to run the test now?

(AFAIU it's not possible for me to fix the deps since I'd need a system each don't I?)

@mihaimaruseac
Copy link
Collaborator

I cannot run it on your fork repo. In your fork, you need to go to the "Pin dependencies" action (https://github.com/susperius/model-transparency/actions/workflows/pin_deps.yml) and trigger one run. It will create a PR on your fork against your main branch, merge that and then this will automatically update.

But before you do that, you'll have to remove it from the duplicate .in file

@susperius
Copy link
Contributor Author

I cannot run it on your fork repo. In your fork, you need to go to the "Pin dependencies" action (https://github.com/susperius/model-transparency/actions/workflows/pin_deps.yml) and trigger one run. It will create a PR on your fork against your main branch, merge that and then this will automatically update.

But before you do that, you'll have to remove it from the duplicate .in file

Ah ok, thanks!

susperius and others added 5 commits July 9, 2024 07:44
@susperius
dependencies
f9a9b3c 
Signed-off-by: Martin Sablotny <msablotny@nvidia.com>
@susperius @github-actions
Bump frozen dependencies
c3e5902 
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@susperius
Merge pull request #2 from susperius/create-pull-request/patch
749ff53 
Update frozen python dependencies
@susperius
Merge branch 'main' into main
6370e60 
Signed-off-by: Martin Sablotny <susperius@gmail.com>
@susperius
Merge branch 'main' into main
b202c20 
Signed-off-by: Martin Sablotny <susperius@gmail.com>
laurentsimon
laurentsimon previously approved these changes Jul 9, 2024
View reviewed changes
Copy link
Collaborator

@laurentsimon laurentsimon left a comment

Choose a reason for hiding this comment

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

We can merge this PR and make generalize / update the code w.r.t manifest / raw signers in follow-up PRs.

Thanks @susperius again!

@susperius
import paths
38ffc61 
Signed-off-by: Martin Sablotny <msablotny@nvidia.com>
@mihaimaruseac mihaimaruseac merged commit 99480e3 into sigstore:main Jul 9, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@font font font left review comments

@mihaimaruseac mihaimaruseac mihaimaruseac approved these changes

@laurentsimon laurentsimon laurentsimon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
V1 release
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@susperius @mihaimaruseac @font @laurentsimon