search

alexjurkiewicz / ecr-scan-image

Github Action to run AWS ECR vulnerability scan on Docker image
MIT License
29 stars 23 forks source link

Option to not include description in findingsDetails #49

Open ChrisPage-AT opened 3 months ago

ChrisPage-AT commented 3 months ago

Would it be possible to include an input field to disable including the descriptions of the CVEs in the findingsDetails output? Sometimes the descriptions of CVEs can be excessively long or contain special characters that break automations. The description can still be found by following the uri link, so having this as an option would be really nice.

alexjurkiewicz commented 3 months ago

Hey Chris, do you mean the generated JSON is invalid? Can you describe your use case in more detail where length is an issue?

On Fri, 9 Aug 2024, 11:37 pm ChrisPage-AT, @.***> wrote:

Would it be possible to include an input field to disable including the descriptions of the CVEs in the findingsDetails output? Sometimes the descriptions of CVEs can be excessively long or contain special characters that break automations. The description can still be found by following the uri link, so having this as an option would be really nice.

— Reply to this email directly, view it on GitHub https://github.com/alexjurkiewicz/ecr-scan-image/issues/49, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC4U5LS3KAS2BZWXLSOVNLZQTOZ5AVCNFSM6AAAAABMIWHCO2VHI2DSMVQWIX3LMV43ASLTON2WKOZSGQ2TQMJYHAZTMOA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

ChrisPage-AT commented 3 months ago

Of course! The JSON itself is fine, it just ends up not being usable in later steps when certain CVEs are present. I have some bash steps after I run ecr-scan-image that parse through the findingsDetails and generate a .sarif to upload to the Code Scanning section of the Security tab on GitHub for the repo.

The length issue is when you get enough super long descriptions like this. That isn't the longest one I've seen and it doesn't take very many of those to push the findingDetails JSON's total length beyond the bash argument character limit. I can't find an example of a bad character CVE because we've since resolved that one, but I believe the description contained an extended ascii character that bash apparently doesn't support.

In both situations it ends up causing the job to fail. I was initially thinking about asking if there could be a character limit on what findingDetails returns, but it seemed easier and cleaner just to have the option to not return the description fields in the JSON. If there's anything else you need or if you're interested in pulling the sarif logic into this action, please let me know!

alexjurkiewicz commented 3 months ago

I see your point.

Github have a limit of 1mb per output: https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions#outputs-for-docker-container-and-javascript-actions

I think it's reasonable to add an option to disable description fields. And it's reasonable to make this the default for new users. Unfortunately that requires a major version bump, so let's make the change in two parts. First add the option to disable description fields. Release this as a minor version bump. Then, change behaviour/option so that description field is disabled by default and release as a major version bump.

Does this sound sensible to you?

On Mon, 12 Aug 2024 at 21:46, ChrisPage-AT @.***> wrote:

Of course! The JSON itself is fine, it just ends up not being usable in later steps when certain CVEs are present. I have some bash steps after I run ecr-scan-image that parse through the findingsDetails and generate a .sarif to upload to the Code Scanning section of the Security tab on GitHub for the repo.

The length issue is when you get enough super long descriptions like this https://security-tracker.debian.org/tracker/CVE-2024-26907. That isn't the longest one I've seen and it doesn't take very many of those to push the findingDetails JSON's total length beyond the bash argument character limit. I can't find an example of a bad character CVE because we've since resolved that one, but I believe the description contained an extended ascii character that bash apparently doesn't support.

In both situations it ends up causing the job to fail. I was initially thinking about asking if there could be a character limit on what findingDetails returns, but it seemed easier and cleaner just to have the option to not return the description fields in the JSON. If there's anything else you need or if you're interested in pulling the sarif logic into this action, please let me know!

— Reply to this email directly, view it on GitHub https://github.com/alexjurkiewicz/ecr-scan-image/issues/49#issuecomment-2284038985, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC4U5JLX2EKW4TK3STLE3TZRC4DBAVCNFSM6AAAAABMIWHCO2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOBUGAZTQOJYGU . You are receiving this because you commented.Message ID: @.***>

ChrisPage-AT commented 3 months ago

Awesome! And yeah that totally makes sense to me. Definitely don't want to change default behaviors on folks without proper warning.