-
Notifications
You must be signed in to change notification settings - Fork 55
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
Initial DSSE verify APIs #962
Conversation
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
I need to do a second pass over this, but DSSE verification is now functional on this branch! You can use this snippet to test-drive it (using production Sigstore, although replacing the import hashlib
import logging
from sigstore.dsse import _StatementBuilder, _Subject
from sigstore.oidc import Issuer
from sigstore.sign import SigningContext
from sigstore.verify import Verifier, policy
logger = logging.getLogger("sigstore")
logger.setLevel("DEBUG")
ctx = SigningContext.production()
stmt = (
_StatementBuilder()
.subjects(
[_Subject(name="null", digest={"sha256": hashlib.sha256(b"").hexdigest()})]
)
.predicate_type("https://cosign.sigstore.dev/attestation/v1")
.predicate(
{
"Data": "",
"Timestamp": "2023-12-07T00:37:58Z",
}
)
).build()
with ctx.signer(Issuer.production().identity_token()) as signer:
bundle = signer.sign(stmt)
verifier = Verifier.production()
verifier.verify_dsse(bundle, policy.UnsafeNoOp())
print(f"OK: integrated at index: {bundle.log_entry.log_index}") |
"invalid signing cert: expired at time of Rekor entry" | ||
) | ||
|
||
def verify_dsse( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NB: This is slightly out of whack with
sign_intoto
in #956, since this operates at the DSSE level while #956 operates at the in-toto level. It might make more sense to turn this into_verify_dsse
and then exposeverify_intoto
that performs the payload type check as a wrapper on top of_verify_dsse
.
maybe. On the other hand payload handling is an application problem with intoto anyway so maybe this is fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, verify_dsse
seems more appropriate to me ultimately (especially since verify_intoto
would imply that we're actually verifying the in-toto statement, which is not actually happening).
Conversely, any thoughts on turning sign_intoto
into sign_dsse
? The main problem I see with that is that it'll rely on the user passing in an opaque bytes
to sign, which means they'll also need to pass in an appropriate payload_type
for the PAE encoding. I can see users doing all kinds of potentially incorrect things there, so maybe keeping signing at the JSON payload + in-toto layer is preferable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think sign_dsse()
would work if the docs are clear about how to use it for the specific intoto use case... but no strong opinions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I'm tempted to make it sign_dsse
then, but I'll do that with a follow-up PR.
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Thanks for putting this out. I've tested in my branch sigstore/model-transparency#112 and it is working. The code currently expects an intoto statement instead of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't pretend to fully understand the DSSE verification details but the changes look correct to me.
The API also looks roughly right: verify_artifact()
is not like verify_intoto/dsse()
so should be separate -- I don't have a strong opinion on which of verify_intoto() and verify_ddse() makes most sense
"invalid signing cert: expired at time of Rekor entry" | ||
) | ||
|
||
def verify_dsse( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NB: This is slightly out of whack with
sign_intoto
in #956, since this operates at the DSSE level while #956 operates at the in-toto level. It might make more sense to turn this into_verify_dsse
and then exposeverify_intoto
that performs the payload type check as a wrapper on top of_verify_dsse
.
maybe. On the other hand payload handling is an application problem with intoto anyway so maybe this is fine?
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
This was unused. Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
latest changes seems good
__all__ = [ | ||
"RekorClient", | ||
"SignedCheckpoint", | ||
"_hashedrekord_from_parts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose at this point it should be just hashedrekord_from_parts()
without underscore. Since rekor
is private, it does not seem important tough...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it probably should since it's entirely private APIs. I'll iterate on this with the follow-up PRs for refactoring.
@laurentsimon @mihaimaruseac FYI we merged here, so you may need to update your refs to |
Very WIP.This follows #956, splitting
verify
intoverify_artifact
andverify_dsse
.TODO: