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

[StepSecurity] Apply security best practices #1365

Merged

Conversation

step-security-bot
Copy link
Contributor

Summary

This pull request is created by StepSecurity at the request of @egecetin. Please merge the Pull Request to incorporate the requested changes. Please tag @egecetin on your message if you have any questions related to the PR.

Security Fixes

Least Privileged GitHub Actions Token Permissions

The GITHUB_TOKEN is an automatically generated secret to make authenticated calls to the GitHub API. GitHub recommends setting minimum token permissions for the GITHUB_TOKEN.

Pinned Dependencies

GitHub Action tags and Docker tags are mutable. This poses a security risk. GitHub's Security Hardening guide recommends pinning actions to full length commit.

Add Dependency Review Workflow

The Dependency Review Workflow enforces dependency reviews on your pull requests. The action scans for vulnerable versions of dependencies introduced by package version changes in pull requests, and warns you about the associated security vulnerabilities. This gives you better visibility of what's changing in a pull request, and helps prevent vulnerabilities being added to your repository.

Add OpenSSF Scorecard Workflow

OpenSSF Scorecard is an automated tool that assesses a number of important heuristics ("checks") associated with software security and assigns each check a score of 0-10. You can use these scores to understand specific areas to improve in order to strengthen the security posture of your project.

Scorecard workflow also allows maintainers to display a Scorecard badge on their repository to show off their hard work.

Feedback

For bug reports, feature requests, and general feedback; please email [email protected]. To create such PRs, please visit https://app.stepsecurity.io/securerepo.

Signed-off-by: StepSecurity Bot [email protected]

@egecetin egecetin changed the base branch from master to dev April 17, 2024 05:53
@seladb
Copy link
Owner

seladb commented Apr 18, 2024

@egecetin can you please provide more context on this PR?

@egecetin
Copy link
Collaborator

egecetin commented Apr 18, 2024

@seladb While I was looking for OpenSSF badge criteria (https://www.bestpractices.dev/en) and how PcapPlusPlus is eligible for this which is an indicator of how a repository is secure, I found that they also have OpenSSF scorecards for all opensource repositories. Similarly to badge, the scorecard tries to determine how secure a library is but in more and more simpler terms than the badge. Similar to oss-fuzz they already have a scanner for popular repos which you can find it here. Currently the value is a bit low but with these fixes we can increase the security score and advertise this on the README along with the badge if decided to also get a OpenSSF badge otherwise just scorecard also enough. Also the scorecard workflow ensures that the rating is updated regularly and the results are pushed the GitHub's security tab. I think these can make the library more discoverable by people since nowadays security is more important than ever and if a network library has some security points people might notice it more easily.

About the badge if I'm not mistaken PcapPlusPlus already satisfies most of the required criteria for "passing" level which you can find here. For silver and gold probably we need more complicated changes like

The project SHOULD have a legal mechanism where all developers of non-trivial amounts of project software assert that they are legally authorized to make these contributions. ...

So, these fixes are like a first step of the OpenSSF criteria and harden security of the workflows.

Additionally I noticed now we should add a PAT to scorecard workflow because it can't detect "Branch protection" without a PAT which mentioned here

.github/workflows/dependency-review.yml Outdated Show resolved Hide resolved
Copy link
Owner

Choose a reason for hiding this comment

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

Do we want to show this scorecard somewhere? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes but for now score is a bit low. I can add a badge to README with this PR or later with another PR. I'm ok for both options

Copy link
Collaborator

Choose a reason for hiding this comment

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

@seladb Added badge to README

Copy link
Owner

Choose a reason for hiding this comment

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

I just noticed the score is quite low (4.8), but it should improve with this PR + #1374

What score would you recommend to expose the badge?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@seladb Sorry for late response. Since this is an automated check there is no direct target, but I think both PRs should increase the score around 7/10 (±0.5) which is quite good, I think. We can than think later about the signed releases and OpenSSF to increase the score more

Copy link
Owner

Choose a reason for hiding this comment

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

maybe we should see the score before adding the badge?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@seladb I just commented out the from README. So, we can add later easily.

@egecetin
Copy link
Collaborator

egecetin commented Apr 23, 2024

@seladb As a reminder did you add the PAT described here and here I used the same naming so name of the PAT should be SCORECARD_TOKEN

@seladb
Copy link
Owner

seladb commented Apr 24, 2024

@seladb As a reminder did you add the PAT described here and here I used the same naming so name of the PAT should be SCORECARD_TOKEN

Should I create a new PAT or will it use the default GITHUB_TOKEN?

@egecetin
Copy link
Collaborator

@seladb It already uses GITHUB_TOKEN as default but to detect branch protection settings it still requires a fine-grained PAT unfortunately.

@seladb
Copy link
Owner

seladb commented Apr 26, 2024

I think there is already a PAT set up:
image

@egecetin
Copy link
Collaborator

I can point this 👍

@@ -7,6 +7,9 @@ on:
schedule:
- cron: '15 14 * * 1'

permissions:
contents: read
Copy link
Contributor

@sashashura sashashura May 5, 2024

Choose a reason for hiding this comment

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

There is one job and it already has permissions defined. This adds no value.

Copy link
Contributor

Choose a reason for hiding this comment

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

But if the scorecard tool is so stupid that it lowers the score without it... so be it.

Copy link
Collaborator

@egecetin egecetin May 6, 2024

Choose a reason for hiding this comment

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

Yes, unfortunately their automated check is not a very clever one but even if there is one job, defining a permission level is better to remove the possibility of misleading/unwanted results.

@egecetin egecetin requested a review from seladb May 15, 2024 15:27
@egecetin egecetin merged commit 4c32a95 into seladb:dev May 20, 2024
68 checks passed
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.

5 participants