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

feat(secret): add line from dockerfile where secret was added to secret result #2780

Merged
merged 15 commits into from Sep 15, 2022
Merged
11 changes: 3 additions & 8 deletions pkg/fanal/applier/docker.go
Expand Up @@ -137,7 +137,7 @@ func ApplyLayers(layers []types.BlobInfo) types.ArtifactDetail {
Digest: layer.Digest,
DiffID: layer.DiffID,
}
secretsMap = mergeSecrets(secretsMap, secret, l)
secretsMap = mergeSecrets(secretsMap, secret, l, layer.CreatedBy)
}

// Apply license files
Expand Down Expand Up @@ -178,13 +178,7 @@ func ApplyLayers(layers []types.BlobInfo) types.ArtifactDetail {
return nil
})

lastDiffID := layers[len(layers)-1].DiffID
for _, s := range secretsMap {
for i, finding := range s.Findings {
if finding.Layer.DiffID != lastDiffID {
s.Findings[i].Deleted = true // This secret is deleted in the upper layer
}
}
mergedLayer.Secrets = append(mergedLayer.Secrets, s)
}

Expand Down Expand Up @@ -271,9 +265,10 @@ func aggregate(detail *types.ArtifactDetail) {

// We must save secrets from all layers even though they are removed in the uppler layer.
// If the secret was changed at the top level, we need to overwrite it.
func mergeSecrets(secretsMap map[string]types.Secret, newSecret types.Secret, layer types.Layer) map[string]types.Secret {
func mergeSecrets(secretsMap map[string]types.Secret, newSecret types.Secret, layer types.Layer, createdBy string) map[string]types.Secret {
for i := range newSecret.Findings { // add layer to the Findings from the new secret
newSecret.Findings[i].Layer = layer
newSecret.Findings[i].CreatedBy = createdBy
}

secret, ok := secretsMap[newSecret.FilePath]
Expand Down
7 changes: 5 additions & 2 deletions pkg/fanal/applier/docker_test.go
Expand Up @@ -335,6 +335,7 @@ func TestApplyLayers(t *testing.T) {
SchemaVersion: 2,
Digest: "sha256:932da51564135c98a49a34a193d6cd363d8fa4184d957fde16c9d8527b3f3b02",
DiffID: "sha256:a187dde48cd289ac374ad8539930628314bc581a481cdb41409c9289419ddb72",
CreatedBy: "Line_1",
Secrets: []types.Secret{
{
FilePath: "usr/secret.txt",
Expand Down Expand Up @@ -368,6 +369,7 @@ func TestApplyLayers(t *testing.T) {
SchemaVersion: 2,
Digest: "sha256:24df0d4e20c0f42d3703bf1f1db2bdd77346c7956f74f423603d651e8e5ae8a7",
DiffID: "sha256:aad63a9339440e7c3e1fff2b988991b9bfb81280042fa7f39a5e327023056819",
CreatedBy: "Line_2",
Secrets: []types.Secret{
{
FilePath: "usr/secret.txt",
Expand Down Expand Up @@ -422,6 +424,7 @@ func TestApplyLayers(t *testing.T) {
SchemaVersion: 2,
Digest: "sha256:932da51564135c98a49a34a193d6cd363d8fa4184d957fde16c9d8527b3f3b02",
DiffID: "sha256:a187dde48cd289ac374ad8539930628314bc581a481cdb41409c9289419ddb72",
CreatedBy: "Line_3",
WhiteoutFiles: []string{
"usr/secret.txt",
},
Expand All @@ -437,7 +440,7 @@ func TestApplyLayers(t *testing.T) {
Category: "GitHub",
Severity: "CRITICAL",
Title: "GitHub Personal Access Token",
Deleted: true,
CreatedBy: "Line_2",
StartLine: 1,
EndLine: 1,
Match: "GITHUB_PAT=****************************************",
Expand All @@ -463,7 +466,7 @@ func TestApplyLayers(t *testing.T) {
Category: "AWS",
Severity: "CRITICAL",
Title: "AWS Access Key ID",
Deleted: true,
CreatedBy: "Line_2",
StartLine: 2,
EndLine: 2,
Match: "AWS_ACCESS_KEY_ID=********************",
Expand Down
56 changes: 41 additions & 15 deletions pkg/fanal/artifact/image/image.go
Expand Up @@ -39,6 +39,11 @@ type Artifact struct {
artifactOption artifact.Option
}

type LayerInfo struct {
DiffID string
CreatedBy string
}

func NewArtifact(img types.Image, c cache.ArtifactCache, opt artifact.Option) (artifact.Artifact, error) {
misconf := opt.MisconfScannerOption
// Register config analyzers
Expand Down Expand Up @@ -93,7 +98,7 @@ func (a Artifact) Inspect(ctx context.Context) (types.ArtifactReference, error)
log.Logger.Debugf("Base Layers: %v", baseDiffIDs)

// Convert image ID and layer IDs to cache keys
imageKey, layerKeys, layerKeyMap, err := a.calcCacheKeys(imageID, diffIDs)
imageKey, layerKeys, layerKeyMap, err := a.calcCacheKeys(imageID, diffIDs, configFile)
if err != nil {
return types.ArtifactReference{}, err
}
Expand Down Expand Up @@ -133,45 +138,65 @@ func (Artifact) Clean(_ types.ArtifactReference) error {
return nil
}

func (a Artifact) calcCacheKeys(imageID string, diffIDs []string) (string, []string, map[string]string, error) {
func (a Artifact) calcCacheKeys(imageID string, diffIDs []string, configFile *v1.ConfigFile) (string, []string, map[string]LayerInfo, error) {
// Pass an empty config scanner option so that the cache key can be the same, even when policies are updated.
imageKey, err := cache.CalcKey(imageID, a.analyzer.ImageConfigAnalyzerVersions(), nil, artifact.Option{})
if err != nil {
return "", nil, nil, err
}

layerKeyMap := map[string]string{}
var createdBy []string
// save createdBy fields in order of layers
for i := 0; i < len(configFile.History); i++ {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for i := 0; i < len(configFile.History); i++ {
for i, history := range configFile.History {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

h := configFile.History[i]
// skip histories for empty layers
if h.EmptyLayer {
continue
}
c := strings.TrimPrefix(strings.TrimPrefix(h.CreatedBy, "/bin/sh -c "), "#(nop) ")
createdBy = append(createdBy, c)
}

layerKeyMap := map[string]LayerInfo{}
hookVersions := a.handlerManager.Versions()
var layerKeys []string
for _, diffID := range diffIDs {
for i, diffID := range diffIDs {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should compare len(diffIDs) and len(createdBy). They should be the same. If they don't match, we should warn it. It is probably our bug.

blobKey, err := cache.CalcKey(diffID, a.analyzer.AnalyzerVersions(), hookVersions, a.artifactOption)
if err != nil {
return "", nil, nil, err
}
layerKeys = append(layerKeys, blobKey)
layerKeyMap[blobKey] = diffID

c := ""
if len(createdBy) > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if len(createdBy) > 0 {
if len(createdBy) > i {

c = createdBy[i]
}
layerKeyMap[blobKey] = LayerInfo{
DiffID: diffID,
CreatedBy: c,
}
}
return imageKey, layerKeys, layerKeyMap, nil
}

func (a Artifact) inspect(ctx context.Context, missingImage string, layerKeys, baseDiffIDs []string, layerKeyMap map[string]string) error {
func (a Artifact) inspect(ctx context.Context, missingImage string, layerKeys, baseDiffIDs []string, layerKeyMap map[string]LayerInfo) error {
done := make(chan struct{})
errCh := make(chan error)

var osFound types.OS
for _, k := range layerKeys {
go func(ctx context.Context, layerKey string) {
diffID := layerKeyMap[layerKey]
lInfo := layerKeyMap[layerKey]

// If it is a base layer, secret scanning should not be performed.
var disabledAnalyers []analyzer.Type
if slices.Contains(baseDiffIDs, diffID) {
if slices.Contains(baseDiffIDs, lInfo.DiffID) {
disabledAnalyers = append(disabledAnalyers, analyzer.TypeSecret)
}

layerInfo, err := a.inspectLayer(ctx, diffID, disabledAnalyers)
layerInfo, err := a.inspectLayer(ctx, lInfo, disabledAnalyers)
if err != nil {
errCh <- xerrors.Errorf("failed to analyze layer: %s : %w", diffID, err)
errCh <- xerrors.Errorf("failed to analyze layer: %s : %w", layerInfo.DiffID, err)
return
}
if err = a.cache.PutBlob(layerKey, layerInfo); err != nil {
Expand Down Expand Up @@ -205,12 +230,12 @@ func (a Artifact) inspect(ctx context.Context, missingImage string, layerKeys, b

}

func (a Artifact) inspectLayer(ctx context.Context, diffID string, disabled []analyzer.Type) (types.BlobInfo, error) {
log.Logger.Debugf("Missing diff ID in cache: %s", diffID)
func (a Artifact) inspectLayer(ctx context.Context, layerInfo LayerInfo, disabled []analyzer.Type) (types.BlobInfo, error) {
log.Logger.Debugf("Missing diff ID in cache: %s", layerInfo.DiffID)

layerDigest, r, err := a.uncompressedLayer(diffID)
layerDigest, r, err := a.uncompressedLayer(layerInfo.DiffID)
if err != nil {
return types.BlobInfo{}, xerrors.Errorf("unable to get uncompressed layer %s: %w", diffID, err)
return types.BlobInfo{}, xerrors.Errorf("unable to get uncompressed layer %s: %w", layerInfo.DiffID, err)
}

// Prepare variables
Expand Down Expand Up @@ -239,7 +264,8 @@ func (a Artifact) inspectLayer(ctx context.Context, diffID string, disabled []an
blobInfo := types.BlobInfo{
SchemaVersion: types.BlobJSONSchemaVersion,
Digest: layerDigest,
DiffID: diffID,
DiffID: layerInfo.DiffID,
CreatedBy: layerInfo.CreatedBy,
OS: result.OS,
Repository: result.Repository,
PackageInfos: result.PackageInfos,
Expand Down
11 changes: 11 additions & 0 deletions pkg/fanal/artifact/image/image_test.go
Expand Up @@ -63,6 +63,7 @@ func TestArtifact_Inspect(t *testing.T) {
SchemaVersion: types.BlobJSONSchemaVersion,
Digest: "",
DiffID: "sha256:beee9f30bc1f711043e78d4a2be0668955d4b761d587d6f60c2c8dc081efb203",
CreatedBy: "ADD file:0c4555f363c2672e350001f1293e689875a3760afe7b3f9146886afe67121cba in / ",
OS: &types.OS{
Family: "alpine",
Name: "3.11.5",
Expand Down Expand Up @@ -273,6 +274,7 @@ func TestArtifact_Inspect(t *testing.T) {
SchemaVersion: types.BlobJSONSchemaVersion,
Digest: "",
DiffID: "sha256:932da51564135c98a49a34a193d6cd363d8fa4184d957fde16c9d8527b3f3b02",
CreatedBy: "bazel build ...",
OS: &types.OS{
Family: "debian",
Name: "9.9",
Expand Down Expand Up @@ -340,6 +342,7 @@ func TestArtifact_Inspect(t *testing.T) {
SchemaVersion: types.BlobJSONSchemaVersion,
Digest: "",
DiffID: "sha256:dffd9992ca398466a663c87c92cfea2a2db0ae0cf33fcb99da60eec52addbfc5",
CreatedBy: "bazel build ...",
PackageInfos: []types.PackageInfo{
{
FilePath: "var/lib/dpkg/status.d/libc6",
Expand Down Expand Up @@ -406,6 +409,7 @@ func TestArtifact_Inspect(t *testing.T) {
SchemaVersion: types.BlobJSONSchemaVersion,
Digest: "",
DiffID: "sha256:24df0d4e20c0f42d3703bf1f1db2bdd77346c7956f74f423603d651e8e5ae8a7",
CreatedBy: "COPY file:842584685f26edb24dc305d76894f51cfda2bad0c24a05e727f9d4905d184a70 in /php-app/composer.lock ",
Applications: []types.Application{
{
Type: "composer", FilePath: "php-app/composer.lock",
Expand Down Expand Up @@ -438,6 +442,7 @@ func TestArtifact_Inspect(t *testing.T) {
SchemaVersion: types.BlobJSONSchemaVersion,
Digest: "",
DiffID: "sha256:a4595c43a874856bf95f3bfc4fbf78bbaa04c92c726276d4f64193a47ced0566",
CreatedBy: "COPY file:c6d0373d380252b91829a5bb3c81d5b1afa574c91cef7752d18170a231c31f6d in /ruby-app/Gemfile.lock ",
Applications: []types.Application{
{
Type: types.Bundler, FilePath: "ruby-app/Gemfile.lock",
Expand Down Expand Up @@ -623,6 +628,7 @@ func TestArtifact_Inspect(t *testing.T) {
SchemaVersion: types.BlobJSONSchemaVersion,
Digest: "",
DiffID: "sha256:932da51564135c98a49a34a193d6cd363d8fa4184d957fde16c9d8527b3f3b02",
CreatedBy: "bazel build ...",
},
},
},
Expand All @@ -633,6 +639,7 @@ func TestArtifact_Inspect(t *testing.T) {
SchemaVersion: types.BlobJSONSchemaVersion,
Digest: "",
DiffID: "sha256:dffd9992ca398466a663c87c92cfea2a2db0ae0cf33fcb99da60eec52addbfc5",
CreatedBy: "bazel build ...",
},
},
},
Expand All @@ -643,6 +650,7 @@ func TestArtifact_Inspect(t *testing.T) {
SchemaVersion: types.BlobJSONSchemaVersion,
Digest: "",
DiffID: "sha256:24df0d4e20c0f42d3703bf1f1db2bdd77346c7956f74f423603d651e8e5ae8a7",
CreatedBy: "COPY file:842584685f26edb24dc305d76894f51cfda2bad0c24a05e727f9d4905d184a70 in /php-app/composer.lock ",
OpaqueDirs: []string{"php-app/"},
},
},
Expand All @@ -654,6 +662,7 @@ func TestArtifact_Inspect(t *testing.T) {
SchemaVersion: types.BlobJSONSchemaVersion,
Digest: "",
DiffID: "sha256:a4595c43a874856bf95f3bfc4fbf78bbaa04c92c726276d4f64193a47ced0566",
CreatedBy: "COPY file:c6d0373d380252b91829a5bb3c81d5b1afa574c91cef7752d18170a231c31f6d in /ruby-app/Gemfile.lock ",
OpaqueDirs: []string{"ruby-app/"},
},
},
Expand Down Expand Up @@ -778,6 +787,7 @@ func TestArtifact_Inspect(t *testing.T) {
SchemaVersion: types.BlobJSONSchemaVersion,
Digest: "",
DiffID: "sha256:beee9f30bc1f711043e78d4a2be0668955d4b761d587d6f60c2c8dc081efb203",
CreatedBy: "ADD file:0c4555f363c2672e350001f1293e689875a3760afe7b3f9146886afe67121cba in / ",
OS: &types.OS{
Family: "alpine",
Name: "3.11.5",
Expand Down Expand Up @@ -909,6 +919,7 @@ func TestArtifact_Inspect(t *testing.T) {
SchemaVersion: types.BlobJSONSchemaVersion,
Digest: "",
DiffID: "sha256:beee9f30bc1f711043e78d4a2be0668955d4b761d587d6f60c2c8dc081efb203",
CreatedBy: "ADD file:0c4555f363c2672e350001f1293e689875a3760afe7b3f9146886afe67121cba in / ",
OS: &types.OS{
Family: "alpine",
Name: "3.11.5",
Expand Down
1 change: 1 addition & 0 deletions pkg/fanal/types/artifact.go
Expand Up @@ -135,6 +135,7 @@ type BlobInfo struct {
SchemaVersion int
Digest string `json:",omitempty"`
DiffID string `json:",omitempty"`
CreatedBy string `json:",omitempty"`
OS *OS `json:",omitempty"`
Repository *Repository `json:",omitempty"`
PackageInfos []PackageInfo `json:",omitempty"`
Expand Down
2 changes: 1 addition & 1 deletion pkg/fanal/types/secret.go
Expand Up @@ -16,6 +16,6 @@ type SecretFinding struct {
EndLine int
Code Code
Match string
Deleted bool
Layer Layer `json:",omitempty"`
CreatedBy string
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we put CreatedBy into Layer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DOne

}
12 changes: 7 additions & 5 deletions pkg/report/table/secret.go
Expand Up @@ -119,12 +119,14 @@ func (r *secretRenderer) renderCode(secret types.SecretFinding) {
lineInfo = tml.Sprintf("%s<blue>-%d", lineInfo, secret.EndLine)
}
}
var note string
if secret.Deleted {
note = " (deleted in the intermediate layer)"
}
r.printf(" <blue>%s%s<magenta>%s\r\n", r.target, lineInfo, note)

r.printf(" <blue>%s%s<magenta>\r\n", r.target, lineInfo)
r.printSingleDivider()
if secret.CreatedBy != "" {
r.printf(" <magenta>This secret is added in '%s'\r\n", secret.CreatedBy)
r.printSingleDivider()
}

for i, line := range lines {
if line.Truncated {
r.printf("<dim>%4s ", strings.Repeat(".", len(fmt.Sprintf("%d", line.Number))))
Expand Down
6 changes: 4 additions & 2 deletions pkg/report/table/secret_test.go
Expand Up @@ -68,7 +68,7 @@ this is a title
Category: ftypes.SecretRuleCategory("category"),
Title: "this is a title",
Severity: "HIGH",
Deleted: true,
CreatedBy: "COPY my-file my-file",
StartLine: 3,
EndLine: 4,
Code: ftypes.Code{
Expand Down Expand Up @@ -115,7 +115,9 @@ HIGH: category (rule-id)
════════════════════════════════════════
this is a title
────────────────────────────────────────
my-file:3-4 (deleted in the intermediate layer)
my-file:3-4
────────────────────────────────────────
This secret is added in 'COPY my-file my-file'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some consideration, I came up with another idea. One line looks better. If it has a file hash, what if we take the first 7 characters like git rev-parse --short HEAD?

Suggested change
my-file:3-4
────────────────────────────────────────
This secret is added in 'COPY my-file my-file'
my-file:3-4 (added by 'COPY my-file my-file')

Then, if it is still too long, we can cut it like the first 40 chars, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

────────────────────────────────────────
1 #!/bin/bash
2
Expand Down
4 changes: 2 additions & 2 deletions pkg/rpc/convert.go
Expand Up @@ -102,7 +102,7 @@ func ConvertToRPCSecretFindings(findings []ftypes.SecretFinding) []*common.Secre
StartLine: int32(f.StartLine),
Code: ConvertToRPCCode(f.Code),
Match: f.Match,
Deleted: f.Deleted,
CreatedBy: f.CreatedBy,
Layer: ConvertToRPCLayer(f.Layer),
})
}
Expand Down Expand Up @@ -313,7 +313,7 @@ func ConvertFromRPCSecretFindings(rpcFindings []*common.SecretFinding) []ftypes.
EndLine: int(finding.EndLine),
Code: ConvertFromRPCCode(finding.Code),
Match: finding.Match,
Deleted: finding.Deleted,
CreatedBy: finding.CreatedBy,
Layer: ftypes.Layer{
Digest: finding.Layer.Digest,
DiffID: finding.Layer.DiffId,
Expand Down