Skip to content
This repository has been archived by the owner on May 25, 2022. It is now read-only.

Address issues detected by gosec #157

Merged
merged 6 commits into from
Jun 9, 2021

Conversation

djaglowski
Copy link
Member

@djaglowski djaglowski commented May 21, 2021

@djaglowski djaglowski requested a review from a team May 21, 2021 15:02
@djaglowski djaglowski requested a review from jsirianni May 24, 2021 14:01
@tigrannajaryan
Copy link
Member

Please resolve the merge conflict.

@djaglowski
Copy link
Member Author

There appears to be an issue with the way gosec is printing the results.

The GitHub action is using a container to run gosec.

It runs securego/gosec "-fmt sarif -out gosec.sarif ./...", which yeilds:

{
	"runs": [
		{
			"taxonomies": [
				{
					"downloadUri": "https://cwe.mitre.org/data/xml/cwec_v4.4.xml.zip",
					"guid": "f2856fc0-85b7-373f-83e7-6f8582243547",
					"informationUri": "https://cwe.mitre.org/data/published/cwe_v4.4.pdf/",
					"isComprehensive": true,
					"language": "en",
					"minimumRequiredLocalizedDataSemanticVersion": "4.4",
					"name": "CWE",
					"organization": "MITRE",
					"releaseDateUtc": "2021-03-15",
					"shortDescription": {
						"text": "The MITRE Common Weakness Enumeration"
					},
					"version": "4.4"
				}
			],
			"tool": {
				"driver": {
					"guid": "8b518d5f-906d-39f9-894b-d327b1a421c5",
					"informationUri": "https://github.com/securego/gosec/",
					"name": "gosec",
					"semanticVersion": "dev",
					"supportedTaxonomies": [
						{
							"guid": "f2856fc0-85b7-373f-83e7-6f8582243547",
							"name": "CWE"
						}
					],
					"version": "dev"
				}
			}
		}
	],
	"$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json",
	"version": "2.1.0"
}

Locally, gosec is installed as go install github.com/securego/gosec/v2/cmd/gosec.

Running gosec -fmt sarif -out gosec.sarif ./...:

{
        "$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json",
        "version": "2.1.0",
        "runs": [
                {
                        "tool": {
                                "driver": {
                                        "name": "gosec",
                                        "version": "2.1.0",
                                        "informationUri": "https://github.com/securego/gosec/"
                                }
                        },
                        "results": []
                }
        ]
}

@xukaren Do you know of any obvious causes for this issue? If not, I can look into it more later.

.github/workflows/gosec.yml Outdated Show resolved Hide resolved
@kxyr
Copy link

kxyr commented May 26, 2021

@djaglowski I am getting the same sarif file on my local run with gosec -fmt sarif -out gosec.sarif ./... as the output through the Github Action.

{
	"runs": [
		{
			"taxonomies": [
				{
					"downloadUri": "https://cwe.mitre.org/data/xml/cwec_v4.4.xml.zip",
					"guid": "f2856fc0-85b7-373f-83e7-6f8582243547",
					"informationUri": "https://cwe.mitre.org/data/published/cwe_v4.4.pdf/",
					"isComprehensive": true,
					"language": "en",
					"minimumRequiredLocalizedDataSemanticVersion": "4.4",
					"name": "CWE",
					"organization": "MITRE",
					"releaseDateUtc": "2021-03-15",
					"shortDescription": {
						"text": "The MITRE Common Weakness Enumeration"
					},
					"version": "4.4"
				}
			],
			"tool": {
				"driver": {
					"guid": "8b518d5f-906d-39f9-894b-d327b1a421c5",
					"informationUri": "https://github.com/securego/gosec/",
					"name": "gosec",
					"semanticVersion": "dev",
					"supportedTaxonomies": [
						{
							"guid": "f2856fc0-85b7-373f-83e7-6f8582243547",
							"name": "CWE"
						}
					],
					"version": "dev"
				}
			}
		}
	],
	"$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json",
	"version": "2.1.0"
}

I'll investigate further into why the codeql /upload-sarif action is requiring a results array be generated.

@djaglowski
Copy link
Member Author

Really appreciate you looking into it @xukaren. I'll remove the sarif file as suggested.

@kxyr
Copy link

kxyr commented May 26, 2021

I just checked other repos as well which does not seem to have SARIF uploading either (e.g. Go). Perhaps it is fine to leave it as is without the automatic alerts, since it is already checking on every PR not only just checking on a routine basis.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 8, 2021

CLA Signed

The committers are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Jun 8, 2021

Codecov Report

Merging #157 (4e2c8ae) into main (5fcffde) will decrease coverage by 0.3%.
The diff coverage is 41.6%.

❗ Current head 4e2c8ae differs from pull request most recent head 1d40a70. Consider uploading reports for the commit 1d40a70 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##            main    #157     +/-   ##
=======================================
- Coverage   75.9%   75.6%   -0.4%     
=======================================
  Files         95      95             
  Lines       4347    4362     +15     
=======================================
- Hits        3301    3299      -2     
- Misses       730     740     +10     
- Partials     316     323      +7     
Impacted Files Coverage Δ
operator/builtin/input/journald/journald.go 52.3% <0.0%> (ø)
operator/builtin/input/udp/udp.go 71.1% <0.0%> (-2.0%) ⬇️
operator/builtin/output/file/file.go 0.0% <0.0%> (ø)
operator/builtin/parser/time/time.go 66.6% <0.0%> (+4.1%) ⬆️
pipeline/config.go 87.8% <0.0%> (-5.9%) ⬇️
testutil/util.go 51.6% <0.0%> (-6.5%) ⬇️
operator/builtin/input/file/reader.go 63.9% <33.3%> (-4.0%) ⬇️
...perator/builtin/transformer/recombine/recombine.go 70.5% <33.3%> (-1.5%) ⬇️
operator/builtin/input/file/file.go 71.8% <42.8%> (-1.5%) ⬇️
operator/builtin/transformer/filter/filter.go 57.1% <75.0%> (+1.9%) ⬆️
... and 4 more

djaglowski added a commit that referenced this pull request Jun 8, 2021
I would like to get #157 merged in, but the changes are almost entirely difficult to test error cases. I'm not suggesting that these are unimportant, but that there are more important things to do right now. We can set this higher later, and increase coverage over time.
tigrannajaryan pushed a commit that referenced this pull request Jun 8, 2021
I would like to get #157 merged in, but the changes are almost entirely difficult to test error cases. I'm not suggesting that these are unimportant, but that there are more important things to do right now. We can set this higher later, and increase coverage over time.
@djaglowski djaglowski merged commit b344305 into open-telemetry:main Jun 9, 2021
@djaglowski djaglowski deleted the gosec-issues branch June 9, 2021 01:41
@djaglowski djaglowski mentioned this pull request Jun 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants