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

name stored attestations by digest instead of UUID #769

Merged
merged 5 commits into from
May 24, 2022

Conversation

bobcallaway
Copy link
Member

This changes the way attestations are named within the storage mechanism; previously, the attestations were stored with a filename of the Merkle leaf hash (aka uuid); however, with the sharding work now merged, there is a risk of conflict since there may be duplicate entries across multiple shards.

Note: this also changes the encoding of persisted data in the attestation store from base64-encoded base64-encoded strings (yes, not a typo) to the actual decoded data. This may require clients to update their parsing logic.

before:

bcallaway@bcallaway01:~/git/sigstore/rekor$ ./rekor-cli get --uuid 48fc5c19a81510895b37d85a2a6ba058058b1ed067d8d13460db71853e8bdc53 --rekor_server=http://localhost:3000
LogID: 2f3d4dceded28ddcd557bfa5412bdb5b3d855aa0060b64a437a3ed819b0e34c2
Attestation: eyJfdHlwZSI6Imh0dHBzOi8vaW4tdG90by5pby9TdGF0ZW1lbnQvdjAuMSIsInByZWRpY2F0ZVR5cGUiOiJodHRwczovL3Nsc2EuZGV2L3Byb3ZlbmFuY2UvdjAuMiIsInN1YmplY3QiOlt7Im5hbWUiOiJmb29iYXIiLCJkaWdlc3QiOnsiZm9vIjoiYmFyIn19XSwicHJlZGljYXRlIjp7ImJ1aWxkZXIiOnsiaWQiOiJmb29UR1VieU1rMTBtdE9Ndz09In0sImJ1aWxkVHlwZSI6IiIsImludm9jYXRpb24iOnsiY29uZmlnU291cmNlIjp7fX19fQ==
Index: 10
IntegratedTime: 2022-04-11T23:33:37Z
UUID: 48fc5c19a81510895b37d85a2a6ba058058b1ed067d8d13460db71853e8bdc53
Body: {
  "IntotoObj": {
    "content": {
      "hash": {
        "algorithm": "sha256",
        "value": "6b242efab1c076a87e7c5a7d2fe85aaed3a05bce259d9cf3ce3c3e59a681b986"
      },
      "payloadHash": {
        "algorithm": "sha256",
        "value": "b2386a526e74a36964c06b73a71ff8a26922124a01907b2b10e29e60504de1eb"
      }
    },
    "publicKey": "LS0tLS1CRUdJTiBQVUJMSUMgS0VZLS0tLS0KTUZrd0V3WUhLb1pJemowQ0FRWUlLb1pJemowREFRY0RRZ0FFeCtpa3FVeFh1cmx4Wmx0YWpSQlYyanUzMWozMgpiYVQyYXgyZFhCY3BJbldhRkVTcUdGMzVLSVNmbFAxRW1NdkVuZkcrQXpIZWNRMFdRcDVRek5JZCt3PT0KLS0tLS1FTkQgUFVCTElDIEtFWS0tLS0tCg=="
  }
}

after:

bcallaway@bcallaway01:~/git/sigstore/rekor$ ./rekor-cli get --uuid f1d0ad9b7d53517d7f664e0e8bc3db9cb182b15b863c2dca1ce8841fd61f2d83 --rekor_server=http://localhost:3000
LogID: 01e950421cb954b5c91261cb3050c8d3b6c3e17e31e5bc9e49b5cd2456f8019d
Attestation: {"_type":"https://in-toto.io/Statement/v0.1","predicateType":"https://slsa.dev/provenance/v0.2","subject":[{"name":"foobar","digest":{"foo":"bar"}}],"predicate":{"builder":{"id":"foogxFPgONx5hjXng=="},"buildType":"","invocation":{"configSource":{}}}}
Index: 10
IntegratedTime: 2022-04-11T23:23:04Z
UUID: f1d0ad9b7d53517d7f664e0e8bc3db9cb182b15b863c2dca1ce8841fd61f2d83
Body: {
  "IntotoObj": {
    "content": {
      "hash": {
        "algorithm": "sha256",
        "value": "085f73da65e022d13e4cfe6eecb3ac369d872682a7e17469c8a4c2756d8c226b"
      },
      "payloadHash": {
        "algorithm": "sha256",
        "value": "79d8d4ddd583b20d0ea380f903530a47a537dfbaaa67f02a2c9c1d6202b4a68d"
      }
    },
    "publicKey": "LS0tLS1CRUdJTiBQVUJMSUMgS0VZLS0tLS0KTUZrd0V3WUhLb1pJemowQ0FRWUlLb1pJemowREFRY0RRZ0FFeCtpa3FVeFh1cmx4Wmx0YWpSQlYyanUzMWozMgpiYVQyYXgyZFhCY3BJbldhRkVTcUdGMzVLSVNmbFAxRW1NdkVuZkcrQXpIZWNRMFdRcDVRek5JZCt3PT0KLS0tLS1FTkQgUFVCTElDIEtFWS0tLS0tCg=="
  }
}

Signed-off-by: Bob Callaway bcallaway@google.com

Signed-off-by: Bob Callaway <bcallaway@google.com>
Signed-off-by: Bob Callaway <bcallaway@google.com>
@bobcallaway bobcallaway requested review from asraa and a team as code owners April 11, 2022 23:36
@codecov-commenter
Copy link

codecov-commenter commented Apr 11, 2022

Codecov Report

Merging #769 (45cd3e5) into main (547eb3c) will decrease coverage by 0.14%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #769      +/-   ##
==========================================
- Coverage   46.56%   46.42%   -0.15%     
==========================================
  Files          60       60              
  Lines        5094     5116      +22     
==========================================
+ Hits         2372     2375       +3     
- Misses       2447     2467      +20     
+ Partials      275      274       -1     
Impacted Files Coverage Δ
pkg/types/alpine/v0.0.1/entry.go 55.78% <0.00%> (+0.78%) ⬆️
pkg/types/entries.go 6.25% <ø> (ø)
pkg/types/hashedrekord/v0.0.1/entry.go 51.28% <0.00%> (-0.67%) ⬇️
pkg/types/helm/v0.0.1/entry.go 48.08% <0.00%> (-0.42%) ⬇️
pkg/types/intoto/v0.0.1/entry.go 35.77% <0.00%> (-0.67%) ⬇️
pkg/types/jar/v0.0.1/entry.go 38.64% <0.00%> (-0.38%) ⬇️
pkg/types/rekord/v0.0.1/entry.go 48.18% <0.00%> (-0.33%) ⬇️
pkg/types/rfc3161/v0.0.1/entry.go 38.51% <0.00%> (-0.58%) ⬇️
pkg/types/rpm/v0.0.1/entry.go 55.38% <0.00%> (-0.43%) ⬇️
pkg/types/test_util.go 0.00% <0.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 547eb3c...45cd3e5. Read the comment docs.

@dlorenc
Copy link
Member

dlorenc commented Apr 12, 2022

Does this mean we'll lose access to all the existing attestations unless we migrate their names to the new format?

@dlorenc
Copy link
Member

dlorenc commented Apr 12, 2022

Nevermind, I see you handled this in the code:

image

Should we try to do a mass migration to keep the code simpler, or would that be too hard?

@loosebazooka
Copy link
Member

I think a mass migration would make sense after this PR is in, since those attestations are still open to modification without detection?

@dlorenc
Copy link
Member

dlorenc commented Apr 12, 2022

I think a mass migration would make sense after this PR is in, since those attestations are still open to modification without detection?

Yeah makes sense.

@bobcallaway
Copy link
Member Author

How would we do a migration? The digest of the attestation is only stored in intoto entries after #764 was merged, so there's no ideal way to do a lookup of attestation digest from entries made before #764 went live (unless I'm missing something)...

@loosebazooka
Copy link
Member

🤦 no, I missed something here. Never mind.

Signed-off-by: Bob Callaway <bcallaway@google.com>
dlorenc
dlorenc previously approved these changes Apr 16, 2022
Signed-off-by: Bob Callaway <bcallaway@google.com>
Signed-off-by: Bob Callaway <bcallaway@google.com>
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.

5 participants