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

Intoto v0.0.2 #973

Merged
merged 8 commits into from
Aug 29, 2022
Merged

Intoto v0.0.2 #973

merged 8 commits into from
Aug 29, 2022

Conversation

pxp928
Copy link
Contributor

@pxp928 pxp928 commented Aug 15, 2022

Summary

The current implementation of the intoto type within Rekor does not persist the signatures from the wrapping DSSE envelope into the log entry stored by Trillian. This makes it impossible to independently verify the cryptographic validity of the entry without possession of the original DSSE envelope.

Based on the discussion and design doc, it was decided to create a v0.0.2 of the intoto Rekor type with changes from v0.0.1. In addition also persisting multiple public keys and signatures.

Design Doc - https://docs.google.com/document/d/17gB598uEkoxx8j9sDhrvfhuNFHW5BSsWEiMP8POn8xo/edit?resourcekey=0-1H4eG4-4-UQYIEXZgj6AKQ#heading=h.6dq5va2kfzsw

Fixes #582

Release Note

Adds new v0.0.2 for intoto type into rekor for support of DSSE envelope with multi signature and public key

Documentation

@pxp928 pxp928 requested review from bobcallaway and a team as code owners August 15, 2022 15:26
@pxp928 pxp928 force-pushed the intoto_v0.0.2 branch 2 times, most recently from 3c47eb1 to 404d041 Compare August 15, 2022 15:30
pkg/types/intoto/v0.0.2/entry.go Outdated Show resolved Hide resolved
pkg/types/intoto/v0.0.2/entry.go Outdated Show resolved Hide resolved
pkg/types/intoto/v0.0.2/entry.go Show resolved Hide resolved
// verifyEnvelope takes in an array of possible key bytes and attempts to parse them as x509 public keys.
// it then uses these to verify the envelope and makes sure that every signature on the envelope is verified.
// it returns a map of verifiers indexed by the signature the verifier corresponds to.
func verifyEnvelope(allPubKeyBytes [][]byte, env *dsse.Envelope) (map[string]*x509.PublicKey, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way we can use the DSSE signer/verifier implementation from https://pkg.go.dev/github.com/sigstore/[email protected]/pkg/signature/dsse instead of re-implementing it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me look to use this instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the implementation provided by sigstore does not provide the functionality in mapping the keys to the valid signature. The sigstore implementation just validates all the signatures with the pub keys but we further do mapping of the keys to sigs to create the log entry. We could create a issue to track this and update sigstore to add this functionality there?

pkg/types/intoto/v0.0.2/intoto_v0_0_2_schema.json Outdated Show resolved Hide resolved
pkg/types/intoto/v0.0.2/entry.go Outdated Show resolved Hide resolved
pkg/types/intoto/v0.0.2/entry.go Outdated Show resolved Hide resolved
pkg/types/entries.go Outdated Show resolved Hide resolved
pkg/types/intoto/v0.0.2/entry.go Outdated Show resolved Hide resolved
@pxp928
Copy link
Contributor Author

pxp928 commented Aug 17, 2022

@bobcallaway Ready for another review.

@pxp928 pxp928 force-pushed the intoto_v0.0.2 branch 2 times, most recently from c4bc858 to d855eea Compare August 17, 2022 18:16
@codecov-commenter
Copy link

Codecov Report

Merging #973 (fd253b5) into main (828f689) will increase coverage by 0.11%.
The diff coverage is 47.85%.

❗ Current head fd253b5 differs from pull request most recent head d855eea. Consider uploading reports for the commit d855eea to get more accurate results

@@            Coverage Diff             @@
##             main     #973      +/-   ##
==========================================
+ Coverage   48.06%   48.17%   +0.11%     
==========================================
  Files          61       62       +1     
  Lines        5413     5741     +328     
==========================================
+ Hits         2602     2766     +164     
- Misses       2528     2676     +148     
- Partials      283      299      +16     
Impacted Files Coverage Δ
cmd/rekor-cli/app/root.go 20.68% <ø> (ø)
pkg/types/entries.go 6.25% <ø> (ø)
pkg/types/intoto/intoto.go 37.03% <0.00%> (ø)
pkg/types/intoto/v0.0.2/entry.go 43.75% <43.75%> (ø)
cmd/rekor-cli/app/pflag_groups.go 85.91% <76.47%> (-1.29%) ⬇️
cmd/rekor-cli/app/pflags.go 85.29% <85.00%> (-0.04%) ⬇️
pkg/sharding/ranges.go 46.01% <0.00%> (-1.26%) ⬇️
pkg/types/rekord/v0.0.1/entry.go 49.00% <0.00%> (+0.66%) ⬆️
pkg/types/alpine/v0.0.1/entry.go 56.01% <0.00%> (+2.48%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@dlorenc
Copy link
Member

dlorenc commented Aug 18, 2022

Looks like something is up with codegen here....

@bobcallaway
Copy link
Member

looks like some of the path names in Makefile.swagger have an extra / in them for some reason.

@pxp928
Copy link
Contributor Author

pxp928 commented Aug 18, 2022

Looks like the Makefile is doing unwanted substitution?

@echo "SWAGGER_GEN=`find pkg/generated/client pkg/generated/models/ pkg/generated/restapi/ -iname '*.go' | grep -v 'configure_rekor_server' | sort -d | tr '\n' ' ' | sed 's/ $$//'`" >> Makefile.swagger;

@bobcallaway
Copy link
Member

Looks like the Makefile is doing unwanted substitution?

@echo "SWAGGER_GEN=`find pkg/generated/client pkg/generated/models/ pkg/generated/restapi/ -iname '*.go' | grep -v 'configure_rekor_server' | sort -d | tr '\n' ' ' | sed 's/ $$//'`" >> Makefile.swagger;

#984 pushed to fix this

cmd/rekor-server/app/serve.go Show resolved Hide resolved
cmd/rekor-server/app/serve.go Outdated Show resolved Hide resolved
pkg/types/intoto/intoto_schema.json Show resolved Hide resolved
cmd/rekor-cli/app/pflag_groups.go Outdated Show resolved Hide resolved
pkg/types/intoto/v0.0.2/intoto_v0_0_2_schema.json Outdated Show resolved Hide resolved
@dlorenc
Copy link
Member

dlorenc commented Aug 18, 2022

Tests seem stuck here.

@dlorenc
Copy link
Member

dlorenc commented Aug 18, 2022

Github is back, you might need to force push/rebase to kick the tests though.

@pxp928 pxp928 force-pushed the intoto_v0.0.2 branch 2 times, most recently from 994694d to f0417be Compare August 18, 2022 21:42
@pxp928
Copy link
Contributor Author

pxp928 commented Aug 19, 2022

Updating the tests.

@pxp928 pxp928 force-pushed the intoto_v0.0.2 branch 2 times, most recently from 9ddbccb to c2cc210 Compare August 19, 2022 12:55
@pxp928
Copy link
Contributor Author

pxp928 commented Aug 19, 2022

Any thoughts on why the e2e test fail on [GET /api/v1/log/entries/{entryUUID}][404] getLogEntryByUuidNotFound. The entry seems to get created successfully.

@priyawadhwa
Copy link
Contributor

Any thoughts on why the e2e test fail on [GET /api/v1/log/entries/{entryUUID}][404] getLogEntryByUuidNotFound. The entry seems to get created successfully.

I ran it locally and got this error:

    e2e_test.go:527: [POST /api/v1/log/entries][400] createLogEntryBadRequest  &{Code:400 Message:Error processing entry: intoto implementation for version '0.0.1' not found: unable to locate entry for version 0.0.1}

probably related to this comment https://github.com/sigstore/rekor/pull/973/files#r948283591

@pxp928
Copy link
Contributor Author

pxp928 commented Aug 23, 2022

Any thoughts on why the e2e test fail on [GET /api/v1/log/entries/{entryUUID}][404] getLogEntryByUuidNotFound. The entry seems to get created successfully.

I ran it locally and got this error:

    e2e_test.go:527: [POST /api/v1/log/entries][400] createLogEntryBadRequest  &{Code:400 Message:Error processing entry: intoto implementation for version '0.0.1' not found: unable to locate entry for version 0.0.1}

probably related to this comment https://github.com/sigstore/rekor/pull/973/files#r948283591

Still getting the same error on the e2e test: [GET /api/v1/log/entries/{entryUUID}][404] getLogEntryByUuidNotFound. Did some testing and it looks like the entry does get created. When I run it twice, it does say an entry is present already.

Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

Thanks! This may be a very late drive-by on the schema: but I have a major question about the schema's payload and payloadHash

cmd/rekor-cli/app/search.go Show resolved Hide resolved
if err != nil {
return nil, fmt.Errorf("error reading public key file: %w", err)
if len(publicKeyBytes) == 0 {
if len(props.PublicKeyPath) > 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: for simplicity just use if len(props.PublicKeyPath) != 1? then no need for the second if clause

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

publicKeyBytes, err = ioutil.ReadFile(filepath.Clean(props.PublicKeyPath.Path))
if err != nil {
return nil, fmt.Errorf("error reading public key file: %w", err)
if len(props.PublicKeyPath) > 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

again, could consolidate the >1 and ==1 check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

} else {
re.AlpineModel.PublicKey.Content = (*strfmt.Base64)(&publicKeyBytes)
re.AlpineModel.PublicKey.Content = (*strfmt.Base64)(&publicKeyBytes[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

should there be a warning when more than one public key byte files are provided?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup will add.

pkg/types/intoto/v0.0.2/entry.go Outdated Show resolved Hide resolved
pkg/types/intoto/v0.0.2/entry.go Show resolved Hide resolved
mikhailswift and others added 6 commits August 26, 2022 17:13
Adds a DSSE envelope type to rekor.  If the DSSE envelope's payload is
an in-toto statement the in-toto subjects will be used as indices for
the envelope's rekord.  If the envelope's payload is within the server's
configured attestation size the payload will be stored as an
attestation.

Signed-off-by: Mikhail Swift <[email protected]>
Signed-off-by: pxp928 <[email protected]>
Signed-off-by: pxp928 <[email protected]>
@dlorenc
Copy link
Member

dlorenc commented Aug 28, 2022

Everything is passing! Is this on track to merge this week @bobcallaway?

@bobcallaway
Copy link
Member

Everything is passing! Is this on track to merge this week @bobcallaway?

@pxp928 and I are debugging hopefully the final issue before merge now

tests/e2e_test.go Show resolved Hide resolved
pkg/types/alpine/v0.0.1/entry.go Outdated Show resolved Hide resolved
pkg/types/cose/v0.0.1/entry.go Outdated Show resolved Hide resolved
pkg/types/hashedrekord/v0.0.1/entry.go Outdated Show resolved Hide resolved
pkg/types/helm/v0.0.1/entry.go Outdated Show resolved Hide resolved
pkg/types/intoto/v0.0.1/entry.go Outdated Show resolved Hide resolved
pkg/types/intoto/v0.0.2/intoto_v0_0_2_schema.json Outdated Show resolved Hide resolved
pkg/types/rekord/v0.0.1/entry.go Outdated Show resolved Hide resolved
pkg/types/rpm/v0.0.1/entry.go Outdated Show resolved Hide resolved
pkg/types/tuf/v0.0.1/entry.go Outdated Show resolved Hide resolved
@pxp928 pxp928 force-pushed the intoto_v0.0.2 branch 2 times, most recently from 2fa4649 to 0826315 Compare August 29, 2022 16:07
Copy link
Member

@bobcallaway bobcallaway left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the hard work!

Copy link
Contributor

@priyawadhwa priyawadhwa left a comment

Choose a reason for hiding this comment

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

nice!!

@dlorenc
Copy link
Member

dlorenc commented Aug 29, 2022

YESSS!

@bobcallaway bobcallaway merged commit 568e31a into sigstore:main Aug 29, 2022
@github-actions github-actions bot added this to the v1.0.0 milestone Aug 29, 2022
@pxp928 pxp928 mentioned this pull request Sep 19, 2022
haydentherapper added a commit to haydentherapper/slsa-github-generator that referenced this pull request Feb 29, 2024
The intoto v001 type does not persist signatures of the DSSE envelope,
as noted in sigstore/rekor#973. We introduced an
intoto v002 type shortly after to fix this, but since then, we've
introduced another newer type, DSSE v001, which also does not persist
the attestation in Rekor (as we discourage using Rekor as storage).

I also updated the verifier in slsa-framework/slsa-verifier#742
to search for both Rekor entry types.

Signed-off-by: Hayden Blauzvern <[email protected]>
laurentsimon added a commit to slsa-framework/slsa-github-generator that referenced this pull request Mar 26, 2024
The intoto v001 type does not persist signatures of the DSSE envelope,
as noted in sigstore/rekor#973. We introduced an
intoto v002 type shortly after to fix this, but since then, we've
introduced another newer type, DSSE v001, which also does not persist
the attestation in Rekor (as we discourage using Rekor as storage).

I also updated the verifier in
slsa-framework/slsa-verifier#742 to search for
both Rekor entry types.

# Summary

...

## Testing Process

...

## Checklist

- [ ] Review the contributing [guidelines](./../CONTRIBUTING.md)
- [ ] Add a reference to related issues in the PR description.
- [ ] Update documentation if applicable.
- [ ] Add unit tests if applicable.
- [ ] Add changes to the [CHANGELOG](./../CHANGELOG.md) if applicable.

---------

Signed-off-by: Hayden Blauzvern <[email protected]>
Signed-off-by: Hayden B <[email protected]>
Signed-off-by: laurentsimon <[email protected]>
Co-authored-by: laurentsimon <[email protected]>
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.

in-toto records don't contain signatures
7 participants