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

Secrel Decoupling #2870

Merged
merged 29 commits into from
Apr 18, 2024
Merged

Secrel Decoupling #2870

merged 29 commits into from
Apr 18, 2024

Conversation

msnwatson
Copy link
Contributor

After the changes in my PR, users of the SecRel workflow can choose to run existing images through SecRel, unrelated of when those images were published. For instance, a common issue that we used to face with running SecRel was that if there were no changes made from the last time SecRel ran, then SecRel would not run as there would be no new images published to be fed into the next step of the workflow. However, now images can be published in one run of the workflow, and then future runs of the workflow can perform SecRel scanning as many times as the user desires.

Essentially, the changes allow for publishing to be skipped and only SecRel performed whereas historically we only allowed for skipping secrel but always required image publishing as the images published were fed directly into the SecRel pipeline. Now, any published image can be fed into the SecRel pipeline.

On PR merge to develop, image publishing and SecRel will continue to run in the same way it has historically: publishing all images and running SecRel on all of them simultaneously.

With these changes, partner team engineers could select only their images for a SecRel run, decoupling their SecRel Aqua image scanning from the rest of the platform. This means that even if there are vulnerabilities found on VRO images, if the EE team’s images passes SecRel, deployment of EE team’s applications could continue to all environments without interruption. The same goes for any VRO images that we seek to deploy. Any image can be deployed with a passing Aqua status, even if other images in the platform do not pass Aqua.

Snyk and SDE alerts must continue to pass or block deployment across the whole platform; however, Aqua is the most frequent offender in terms of deployment blockers and SecRel failures.

In the short-term, VRO team will need to continue to manage SecRel on behalf of our partner teams, as there will definitely be an education component. However, these changes get us closer to partner teams being able to manage their own images’ SecRel statuses and to partner teams being able to perform their own deployments to LHDI environments that require SecRel-signed images.

The changes will be available for use as soon as the PR containing the changes is merged. The changes can be easily reverted through a quick PR reversion. This work does not seem to be impacted at all by the recent work on removing versioning, beyond simplifying the mental model of the process.

Associated tickets or Slack threads:

How to test this PR

  • Tested through manual workflow dispatching
  • Will have to followup with some testing after merge as certain triggers can only be performed once the workflow is in the develop branch

@msnwatson msnwatson requested a review from a team as a code owner April 17, 2024 20:02
@msnwatson msnwatson requested review from tejans24 and chengjie8 April 17, 2024 20:02
Copy link
Contributor

github-actions bot commented Apr 17, 2024

Test Results

151 tests  ±0   151 ✅ ±0   48s ⏱️ +2s
 46 suites ±0     0 💤 ±0 
 46 files   ±0     0 ❌ ±0 

Results for commit e8d4c03. ± Comparison against base commit 1a4f255.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Apr 17, 2024

JaCoCo Test Coverage

Overall Project 76.18%

There is no coverage information present for the Files changed

Copy link
Contributor

@nelsestu nelsestu left a comment

Choose a reason for hiding this comment

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

This sounds and looks just like I hoped it would. Thanks for making this happen and for communicating the significance of this enhancement so clearly. The only other question I had was whether we need to be handling the case where images have already been signed? This might be adding scope to this ticket, and if so, we can answer with a new backlog item. But what i was thinking is something like this:

# Perform signature verification
          if docker trust inspect --pretty --signer <SIGNER_NAME> ${{ github.repository }}:${{ github.sha }}; then
            echo "Image is signed by expected signer"
            echo "::set-output name=signature_verified::true"
          else
            echo "Image is not signed by expected signer"
            echo "::set-output name=signature_verified::false"
          fi

Copy link
Contributor

@tejans24 tejans24 left a comment

Choose a reason for hiding this comment

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

LGTM, nice work @msnwatson . Excited to try this out!

@msnwatson
Copy link
Contributor Author

This sounds and looks just like I hoped it would. Thanks for making this happen and for communicating the significance of this enhancement so clearly. The only other question I had was whether we need to be handling the case where images have already been signed? This might be adding scope to this ticket, and if so, we can answer with a new backlog item. But what i was thinking is something like this:

# Perform signature verification
          if docker trust inspect --pretty --signer <SIGNER_NAME> ${{ github.repository }}:${{ github.sha }}; then
            echo "Image is signed by expected signer"
            echo "::set-output name=signature_verified::true"
          else
            echo "Image is not signed by expected signer"
            echo "::set-output name=signature_verified::false"
          fi

So I actually would not want to skip signing a new image that has already been signed in the past because SecRel signatures do expire an I'm not sure I'd have access to all that information from this script. As far as I know, there are no downsides to signing images multiple times beyond just computation time in the SecRel pipeline which we haven't had a need to optimize at this point.

@msnwatson msnwatson force-pushed the mwatson/secrel-decouple branch from 72df4a7 to e8d4c03 Compare April 18, 2024 23:18
@msnwatson msnwatson merged commit 102932c into develop Apr 18, 2024
1 check passed
@msnwatson msnwatson deleted the mwatson/secrel-decouple branch April 18, 2024 23:55
msnwatson added a commit that referenced this pull request Apr 19, 2024
msnwatson added a commit that referenced this pull request Apr 19, 2024
Revert "Secrel Decoupling (#2870)"

This reverts commit 102932c.
Ponnia-M pushed a commit that referenced this pull request Jul 22, 2024
* first pass at changing secrel to be more decoupled

* work on syntax error

* put secrel inputs step in separate script

* change conditions

* checkout repo in secrel-inputs step

* add back double bracing

* add space in braces

* move script back

* add gate step

* change run conditions

* add debug echo statement

* change form of run condition

* change form of run condition

* change form of run condition

* change form of run condition

* change form of run condition

* change form of run condition

* change form of run condition

* change form of run condition

* change form of run condition

* change form of run condition

* change form of run condition

* change form of run condition

* change form of run condition

* change form of run condition

* change form of run condition

* change form of run condition

* change form of run condition

* update list
Ponnia-M pushed a commit that referenced this pull request Jul 22, 2024
Revert "Secrel Decoupling (#2870)"

This reverts commit 102932c.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants