Enable Security Scan for CRT #11956
Enable Security Scan for CRT #11956
Conversation
| @@ -1,5 +1,5 @@ | |||
| # This Dockerfile creates a production release image for the project using crt release flow. | |||
| FROM alpine:3.13 as default | |||
| FROM alpine:3 as default | |||
There was a problem hiding this comment.
I'm curious if this was intentionally updated? I think there has been issues in the past upgrading point releases of Alpine, so we might want to stay pin to 3.13.
There was a problem hiding this comment.
Yes, this was intentionally updated to allow alpine:3 to pick up the latest + cleanest version the alpine image. Security scanned alpine:3.13 and found some known CVEs. If the goal is to be more prescriptive here, Security scanned alpine:3.15 (which is latest + most secure) to be clean of any vulnerabilities, I can absolutely make that change.
There was a problem hiding this comment.
Ok, makes sense. I did a little searching around Slack to see if there was any reason why we are pinned to 3.13 and I didn't find anything. However, we may want to do this as a separate change considering it could have ramifications that are outside the scope of security scanning. It's up to you though.
Also, I see comments like this from other PRs and am realizing that this is not a new topic for you :)
To be clear and reiterate, I think this is probably a safe change, but it's your call as to whether we include it here vs in a separate PR.
There was a problem hiding this comment.
I think it'd be ok to keep this change in this PR and haven't seen any ramifications with this change among other products. Not to be over-generalizing, but to agree with you that this is a safe change :D
| } | ||
|
|
||
| event "security-scan-containers" { | ||
| depends = ["security-scan-binaries"] |
There was a problem hiding this comment.
Can these be run in parallel?
There was a problem hiding this comment.
I see that the other repositories that have been onboarded to CRT do it this way, so if you don't feel like bucking the trend or explaining, that's fine too. I am just curious.
There was a problem hiding this comment.
I don't think these can be run in parallel as each event has a depends requirement. This is to allow workflows to be run consecutively. If the security-scanner picks up any vulnerabilities, it would stop any subsequent workflows which I believe is what we want. If there are any outputted vulnerabilities, it will need to be reported to #team-prodsec in Slack. cc: @sarahethompson to ensure I'm not leaving out any vital context.
.release/security-scan.hcl
Outdated
| container { | ||
| dependencies = true | ||
| alpine_secdb = true | ||
| secrets = true | ||
| } | ||
|
|
||
| binary { | ||
| secrets = true | ||
| go_modules = true | ||
| osv = true | ||
| oss_index = true | ||
| nvd = true | ||
| } |
There was a problem hiding this comment.
I'm curious why these aren't the default settings?
There was a problem hiding this comment.
This is a really good question that I don't think I quite have the answer to. Let me do some sleuthing and circle back to you on this.
There was a problem hiding this comment.
Alrightie @eculver -- I got some answers :D -- so, there aren't any defaults at the security scanner level and we need to provide these specific values in the file, however we are looking at doing some work to abstract a lot the default settings in ci.hcl and security-scan.hcl into our CRT tooling in order to reduce the complexity of those config file in product repos! You can follow our CRT newsletter for updates!
.release/security-scan.hcl
Outdated
| container { | ||
| dependencies = true | ||
| alpine_secdb = true | ||
| secrets = true |
There was a problem hiding this comment.
Is this supposed to be a block type? I'm looking at the config docs here
There was a problem hiding this comment.
Yes, this is supposed to be a block type. This is the configuration that has been set up for CRT (Common Release Tooling). I'm not sure if I quite answered your question here, but please add follow-up questions and I'd be happy to clarify any bits!
There was a problem hiding this comment.
It not being a block type must not be causing any issues given that all the other projects using the scanner, but I'm curious if this means that it's the same as:
secrets {
all = true
}
Ok, I went down the rabbit hole. Apparently, secrets = true is a deprecated configuration option, but it is still supported. Using secrets = true is the same as secrets { all = true }.
I wonder if we should be using the updated block type here to future-proof it?
There was a problem hiding this comment.
really good catch. It seems like this update was recent, so I will get that updated for this PR! Thank you for going down the rabbit hole, lol!
There was a problem hiding this comment.
Update has been made!
…to enable-security-scan
|
PS - I added a changelog entry for it to satisfy the changelog-checker |
|
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/576550. |
What this PR does is it enables security scanning (owned by the Security team) for this repository. This is a part of the Common Release Tooling (CRT) effort which
enables a consistent workflow for security scanning that can be used across products and services. It provides a single abstraction point to allow us integrate our own scanning capabilities, as well as leverage external tools and APIs from vendors, open source, or free-to-use projects.Learn more about HashiCorp's security-scanner here.While running the security scan tests, I ran into a false positive with
go_modulesin the binary stanza in the security-scan.hcl file. This is apparently a known issue within your team-- this will need to be updated in order for the workflows to pass thesecurity-scan-binariesevent.I was able to test out
security-scan-containersby turninggo_modulesto false and the most recent tests are passing.You'll also see that I had to upgrade the Dockerfile to use alpine:3 as the older version caused the security-scanner to pick up some vulnerabilities, CVEs to be specific. See error outputs here that arose with consul-terraform-sync. We worked with Security and they scanned different versions of the alpine base image and concluded that version 3 picks up the most stable + clean version of alpine and suggested to upgrade to that version to avoid any friction and potential vulnerabilities in the future and simply tidy up the container.
If anyone needs any further clarification or if nothing makes sense, feel free to comment or reach out to me! ❤️