improvements to rekor cataloger code #1175
Conversation
Signed-off-by: Marco Deicas <mdeicas@google.com>
| @@ -52,6 +52,7 @@ type Application struct { | |||
| FileMetadata FileMetadata `yaml:"file-metadata" json:"file-metadata" mapstructure:"file-metadata"` | |||
| FileClassification fileClassification `yaml:"file-classification" json:"file-classification" mapstructure:"file-classification"` | |||
| FileContents fileContents `yaml:"file-contents" json:"file-contents" mapstructure:"file-contents"` | |||
| RekorCataloger rekorCataloger `yaml:"rekor-cataloger" json:"rekor-cataloger" mapstructure:"rekor-cataloger"` | |||
There was a problem hiding this comment.
I think I've talked with @wagoodman about this and if we were to introduce individual cataloger configurations into this struct they would all exist under the same top-level parent struct rather than be in line with each other at the Application level. Leaving this comment open to discuss where we should include individual cataloger config
| @@ -30,26 +31,25 @@ func (c *Cataloger) UsesExternalSources() bool { | |||
| return true | |||
| } | |||
|
|
|||
| func (c *Cataloger) Catalog(resolver source.FileResolver) ([]pkg.Package, []artifact.Relationship, error) { | |||
| func (c *Cataloger) Catalog(resolver source.FileResolver) ([]artifact.Relationship, error) { | |||
There was a problem hiding this comment.
design: The return signature here is interesting and I think I need to explore the file package code a bit more before giving good feedback here.
I looked at the other catalogers in this package:
- classification
- contents
- digest
- metadata
- secrets
Each of those returns something akin to map[source.Coordinates]someValue. Even though this is not enforced by an interface implementation at a higher level, do we want the rekor_cataloger to be different in this case and return []Relationship as the output for Catalog?
Personally, I think this is ok since we're describing a union to an external source and we're doing all of the work upfronts in the cataloger of creating the external ref and relationships without additional overhead. I just wanted to point out that this cataloger was a little different from its current sibblings.
There was a problem hiding this comment.
I agree that this is ok. I think that the ability to implement catalogers with different return types is one of the benefits of implementing a cataloger at the task level as opposed to the package-cataloger level. syft.CatalogPackages in generateCatalogPackagesTask also has a different return type.
| @@ -93,10 +94,11 @@ func parseEntry(entry *models.LogEntryAnon) (*models.IntotoV001Schema, error) { | |||
| return intotoV001, nil | |||
| } | |||
|
|
|||
| // validateAttestation does some checks that the attestation contains the necessary fields to proceed | |||
| // validateAttestation validates that the attestation contains the necessary fields to proceed, and | |||
There was a problem hiding this comment.
nit: I think the function name changed here validateAttestation => validateAndGetFirsts
| @@ -108,40 +110,42 @@ func validateAttestation(att *InTotoAttestation) error { | |||
| } else if len(att.Subject) > 1 { | |||
| err = errors.New("attestation found on rekor contains multiple subjects, which is not supported. Ignoring log entry") | |||
| } else if _, ok := att.Subject[0].Digest["sha256"]; !ok { | |||
There was a problem hiding this comment.
nit: Just to protect from any incomplete att that could be uploaded it might be best to verify the Subject at the zero index exists before doing this map check. There also might be an opportunity to exit early in the len(att.Subject) == 0 case
|
cc @wagoodman |
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
This PR, to
kubecon-draft, addresses many of the comments from the original PR for SBOM discovery with Rekor.One of the bigger changes is returning a
subjectandsbomEntryfromparseAndValidateAttestationinstead of aInTotoAttestation. This is to reduce the amount of validations that are needed in the code.Another change is adding a configuration for
rekor-cataloger. As the cataloger is off-by-default, the following must be specified in a configuration file:To avoid this, a CLI flag specifically for the rekor-cataloger could be added. Alternatively, a more general CLI flag that filters the file catalogers or the non-package catalogers could be implemented, similar to the cli flag for package catalogers.
I had some difficulty enabling the
rekor-catalogervia the configuration when there were other configuration fields also specified. If this is encountered, use a configuration file containing only the rekor-cataloger configuration. I will look at this issue further.Signed-off-by: Marco Deicas mdeicas@google.com