-
Notifications
You must be signed in to change notification settings - Fork 16
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
verification attestor #55
Conversation
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.
A couple of suggestions.
070f09d
to
2eec808
Compare
Additional changes pushed to this:
This still needs tests, but I am able to run our witness test scripts with these changes with this branch on witness: https://github.com/testifysec/witness/tree/feat/verification-attestation |
policyResult, policyErr := pol.Verify(ctx.Context(), policy.WithSubjectDigests(vo.subjectDigests), policy.WithVerifiedSource(verifiedSource)) | ||
if _, ok := policyErr.(policy.ErrPolicyDenied); ok { | ||
accepted = false | ||
} else if err != nil { |
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 the code should check for policyErr
instead of err
here.
var accepted bool
policyResult, policyErr := pol.Verify(ctx.Context(), policy.WithSubjectDigests(vo.subjectDigests), policy.WithVerifiedSource(verifiedSource))
if _, ok := policyErr.(policy.ErrPolicyDenied); ok {
accepted = false
} else if policyErr != nil {
return fmt.Errorf("failed to verify policy: %w", policyErr)
} else {
accepted = true
}
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.
Yup, good catch
return map[string]cryptoutil.DigestSet{} | ||
} | ||
|
||
func (vo *Attestor) Attest(ctx *attestation.AttestationContext) error { |
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.
A nit vo
. Having consistent receiver name with vs
helps :)
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.
Agreed, the receiver name is a byproduct of the code moving where the variable was originally of type verifyOptions
, this should be renamed to be consistent with our new receiver.
39f78db
to
7fdb712
Compare
I added a second commit to this to support some work I'm doing in a separate project that's using go-witness. It's not directly related to VSAs, but I needed it for work in the archivista-data-provider. When this PR gets moved out of draft and is ready for merge, the second commit will be broken out into it's own PR as it is a sizeable change in it's own right. |
ab29b9a
to
ab4f36c
Compare
Currently, the cryptoutil.DigestValue type is used extensively across the codebase. While this works well for our current needs, it might make the code more rigid and harder to test. One way to improve this could be to define an interface that cryptoutil.DigestValue satisfies. This would allow us to create mock implementations for testing different scenarios and make the code more flexible. Here's a simple example of what this might look like: type Digest interface {
New() hash.Hash
// other methods...
}
type DigestValue struct {
// fields...
}
func (d DigestValue) New() hash.Hash {
// implementation...
}
func SomeFunction(d Digest) {
hash := d.New()
// use hash...
} In this example, SomeFunction can now accept any object that satisfies the Digest interface. This makes it easier to test SomeFunction with different implementations of Digest. This is just a suggestion. |
a7a43aa
to
876c7d2
Compare
956e6a7
to
48472c7
Compare
48472c7
to
93f8c86
Compare
cd2c7c0
to
8557fc7
Compare
type Attestor struct { | ||
slsa.VerificationSummary | ||
|
||
policyTimestampAuthorities []timestamp.TimestampVerifier |
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.
Just being sure to ask what springs to mind, do we need all of these extra fields in the attestor? I can imagine that this information can be of use when performing validation on what was verified and how, but I supose this comes at the consequence of bloating the predicate output? Isn't one of the benefits of a VSA that it is lightweight and provides minimal predicate inspection by the final verifier? @mikhailswift @jkjell
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.
Oh ignore me here. I see that these are not fields to be added to the predicate but instead supporting fields for attestor functionality. The slsa.VerificationSummary
is the only field that is used as the predicate.
One thing that springs to mind is that when we have the time, better moulding the structures so that predicates are defined very clearly could be a good thing 😄
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.
This is a nit so no action is required 👍
}, | ||
TimeVerified: time.Now(), | ||
Policy: slsa.ResourceDescriptor{ | ||
URI: policyDigest[cryptoutil.DigestValue{Hash: crypto.SHA256, GitOID: false}], //TODO: find a better value for this... |
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.
just picking up on the TODO here. Given that we can't provide a public location of the policies, wouldn't we be better (for now) using https://witness.testifysec.com/policy/v0.1
for this URI field?
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.
This is addressed in a PR I created #230
}, nil | ||
} | ||
|
||
func verifyPolicySignature(ctx context.Context, vo *Attestor) error { |
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.
Noticed that this is a copy of https://github.com/in-toto/go-witness/blob/main/verify.go#L182. Would we not be better exporting the original function? alternatively we could stick it in an /internal
location so that both verify.go
and policyverify.go
can make use of it without the copying? Unless there's something I am missing and this function is slightly different to the one in verify.go
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.
ahhhh I see what you've done here. You've moved a lot of the logic from verify.go
into policyverify.go
. I am not averse to this, however I do think that there may be a better shared place for this function to live, as I reckon this functionality might need to be leveraged elsewhere in the codebase in the future.
Having said all this, it's not necessary right now and I am okay leaving it as is and kicking the can down the road so to speak until we need it elsewhere.
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.
This is addressed in a PR I created #230
const ( | ||
Name = "policyverify" | ||
Type = slsa.VerificationSummaryPredicate | ||
RunType = attestation.ExecuteRunType |
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 wish to make life more difficult in the process of getting this merged, but I think a new type should be declared here in to define its type.
I just ran the code locally, and in its current form, there's nothing from stopping me from trying to run this attestor during witness run
. We should have an easy way to distinguish this attestor from the others that exist, and to me a verify
type is the way we should to it.
Of course this is a small thing to implement (a variable declaration and a line somewhere in the Run
logic to ensure that any verify
attestors can't be executed, but I think it's crucial for user experience.
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.
This is addressed in a PR I created here
I've created a PR here in the hope that it will speed up the process of getting this merged. I think there are still tests needed for the VSA logic, @mikhailswift let me know if you want me to take the reigns on writing these 😄 |
6fc599a
to
1bd15fa
Compare
Co-authored-by: Kris Coleman <kriscodeman@gmail.com>
Co-authored-by: Brandon Hunter <brandon_hunter-wm@discovery.com> Signed-off-by: Tom Meadows <tom@tmlabs.co.uk>
1bd15fa
to
d33965f
Compare
Improvements to VSA internal policy handling and other minor fixups. --------- Signed-off-by: Tom Meadows <tom@tmlabs.co.uk> Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk> Signed-off-by: John Kjell <john@testifysec.com> Co-authored-by: Mikhail Swift <mikhail@testifysec.com> Co-authored-by: Kris Coleman <kriscodeman@gmail.com> Co-authored-by: Brandon Hunter <brandon_hunter-wm@discovery.com> Co-authored-by: John Kjell <john@testifysec.com>
…th verify attestor Signed-off-by: John Kjell <john@testifysec.com>
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
chore: upgrade kratos
Fixes https://github.com/testifysec/witness/issues/14
The rough idea here is to have
go-witness.Verify
actually be ago-witness.Run
with a verification attestor as it'sExecute
attestor. This allows a few different things:There are a few philosophical questions and changes to the go-witness API happening here we should think a bit about:
In main today, the DSSE signing code will throw up if at least one signer isn't provided. This makes sense; why wrap something in a DSSE envelope if it won't be signed? However, we don't want every verifier to have keys distributed to them. Users should be able to run
go-witness.Verify
without providing a key. If one is provided, a signed VSA should be created. In this draft changeset, this error is removed. An unsigned envelope will be returned if no signers are provided to the DSSE code.This has the side-effect of allowing users to run
go-witness.Run
without a signer provided. This may not be a bad thing, we would hand back an unsigned envelope of attestations. This could allow users to play with Witness more quickly and sort out key distribution once ready.While we offer no guarantees of backward compatibility, this may involve some breaking changes to
go-witness
's surface APIs. We shouldn't let this stop us from making choices, but we should be cognizant of the pain this will cause and ensure we do it for the right reasons.