feat: Enable Witness Policy verify from Archivista #438
Conversation
✅ Deploy Preview for witness-project canceled.
|
kairoaraujo
force-pushed
the
retrieve_archivista_policy
branch
from
00d38f9
to
b6acb3a
Compare
cmd/verify.go
Outdated
| @@ -83,16 +95,9 @@ func runVerify(ctx context.Context, vo options.VerifyOptions, verifiers ...crypt | |||
| verifiers = append(verifiers, v) | |||
| } | |||
|
|
|||
| inFile, err := os.Open(vo.PolicyFilePath) | |||
| policyEnvelope, err := pkg.LoadPolicy(ctx, vo.PolicyFilePath, pkg.NewArchivistaClient(vo.ArchivistaOptions.Url, archivistaClient)) | |||
There was a problem hiding this comment.
So I reckon that we could benefit from splitting the options up between --policy-file and --policy-sha. We have gone down the route of a more declarative experience on Witness rather than the "magic" approach.
Having said this I am not 100% sure what the best flag definitions would be for this.
There was a problem hiding this comment.
I avoided it because we break compatibility with existent deployments, as it will break the verification.
I'm ok with doing a more declarative.
Should I go for this change?
There was a problem hiding this comment.
This is a good point that I hadn't fully considered. Let me think about it for a couple of minutes 😄
pkg/archivista.go
Outdated
There was a problem hiding this comment.
Usually I find splitting go packages inside pkg into logical directories. The convention that I would usually expect here would be pkg/archivista/archivista.go, so that when pkg expands it is well organized 😄.
There was a problem hiding this comment.
Also I think that maybe this client would be better defined in the Archivista repo possibly?
There was a problem hiding this comment.
I thought of adding the interface to Archivista, but Archivista is moving toward being a service.
Later, I thought of doing it in the go-witness; in the end, I felt that having it in Witness is less opinionated and leaves the applications to build their interfaces as go-witness provides the struct.
I think it is another piece that should be in internal as you well mentioned in the comment below.
What do you think about my thoughts here?
pkg/policy.go
Outdated
| // Load policy from a file or Archivista | ||
| // | ||
| // It prefers to load from a file, if it fails, it tries to load from Archivista | ||
| func LoadPolicy(ctx context.Context, policy string, ac ArchivistaClienter) (dsse.Envelope, error) { |
There was a problem hiding this comment.
I am not totally 100% sure on this, but again this could go in go-witness rather than in here?
I suppose we don't want people to depend on this the Witness CLI as a library. In which case, we probably would be better off changing pkg/ to internal/. That way, the packages defined inside internal/ can only be used internally within the witness repo and other people can't depend on the logic 😄.
There was a problem hiding this comment.
I agree with you. LoadPolicy is an implementation for Witness in the sense that it has a specific use case.
I also agree that it should be moved to internal. Thanks.
|
Just as a side note, we probably want to add to the docs so this feature is clear for users to see that they can use / try out 😄. Awesome functionality, Really excited to start using it. |
kairoaraujo
force-pushed
the
retrieve_archivista_policy
branch
2 times, most recently
from
1d21153
to
adc1318
Compare
Enables Witness to retrieve Policy from Archivista. If a user provides a Policy `gitoid`, Witness will attempt to retrieve it from Archivista. To maintain backward compatibility, Witness first tries to load the file locally. If the local file loading fails and an Archivista is configured, it will retrieve the Policy from Archivista. Signed-off-by: Kairo Araujo <[email protected]>
Signed-off-by: Kairo Araujo <[email protected]>
kairoaraujo
force-pushed
the
retrieve_archivista_policy
branch
from
adc1318
to
c34388c
Compare
What this PR does / why we need it
Enables Witness to retrieve Policy from Archivista.
If a user provides a Policy
gitoid, Witness will attempt to retrieve it from Archivista.To maintain backward compatibility, Witness first tries to load the file locally. If the local file loading fails and an Archivista is configured, it will retrieve the Policy from Archivista.
Which issue(s) this PR fixes (optional)
Requires:
Acceptance Criteria Met
Special notes for your reviewer:
Besides, it requires feat: Backend for Policy upload and store archivista#251. It does not break the compatibility if the user uses a version of Archivista that doesn't contain policies.
I decided to create pkg/policy.go to follow the TODO in the
verify.gohttps://github.com/in-toto/witness/blob/main/cmd/verify.go#L64