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

Add FINOS Security Actions #693

Conversation

TheJuanAndOnly99
Copy link
Member

This PR adds 2 GitHub actions that perform a CVE and static code scan.

With the CVE scanning I could use some help as we are a bit unfamiliar with Gradle. The scan is passing but I am not sure if dependencyCheckAnalyze task is taking in consideration all project sub-modules. Could you please point me in the right direction here?

As far as the static code scan goes there were also many findings. Please also take a look at these as we believe this tool does a good job at pointing out any potential vulnerabilities. If you need to bypass any you can do so (like I did here).

Additional steps that would greatly improve the security posture of this project:

  • Enable RenovateBot - please let me know if I can enable it on this repo. This tool will help greatly with keeping your dependencies up to date.
  • Add GitHub (security) Action badges at the top of the project README
  • Apply for the OpenSSF Passing badge - https://bestpractices.coreinfrastructure.org/

Eager to hear your thoughts on it, and happy to adjust the aim based on your feedback.

@TheJuanAndOnly99
Copy link
Member Author

Hi @yinan-symphony I added the Renovate bot I mentioned in the PR on my fork. You can see the results here TheJuanAndOnly99#3.


on:
push:
paths:
Copy link
Contributor

@symphony-soufiane symphony-soufiane Nov 21, 2022

Choose a reason for hiding this comment

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

I would prefer this action to run on every push/pull_request if it lasts a reasonable duration. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the documentation mentions "dependencyCheckUpdate" task to update NVD from NIST. Do we need to add it to be sure the NVD is uptodate?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @symphony-soufiane fair question. I think for a GitHub action it is not needed as the action does not cache any build artifacts.

As for the action to run on every push/pull/request. It seems it takes around ~6 min to complete. I have added the change to the action if you want to go ahead with it.

- name: Build with Gradle
run: ./gradlew build
- name: CVEs
run: ./gradlew dependencyCheckAnalyze
Copy link
Contributor

@symphony-soufiane symphony-soufiane Nov 21, 2022

Choose a reason for hiding this comment

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

I think that 'dependencyCheckAggregate' should be used rather to scan all modules.
Running locally dependencyCheckAnalyze showed in the report "Dependencies Scanned: 0 (0 unique)" while with 'dependencyCheckAggregate' I can see 'Dependencies Scanned: 151 (151 unique)'

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, in multi-module project, Aggregate should be the one to use.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @symphony-soufiane and @yinan-symphony! I've changed it in the PR. As you can see there are a few CVEs. Can you please take a look. If they are safe to ignore you can use the allow-list.xml file to do so.

@yinan-symphony
Copy link
Contributor

yinan-symphony commented Nov 22, 2022

Hi @yinan-symphony I added the Renovate bot I mentioned in the PR on my fork. You can see the results here TheJuanAndOnly99#3.

sure, thx ! we will have a look

@yinan-symphony
Copy link
Contributor

Hi @yinan-symphony I added the Renovate bot I mentioned in the PR on my fork. You can see the results here TheJuanAndOnly99#3.

@TheJuanAndOnly99 could you please configure the Renovate to set
{ "dependencyDashboardApproval": true }
before enabling it in main branch.
many thx

@TheJuanAndOnly99
Copy link
Member Author

@yinan-symphony I have added a renovate.json file to the PR with the { "dependencyDashboardApproval": true } config.

@yinan-symphony
Copy link
Contributor

@TheJuanAndOnly99 thank you for your contribute, could you please create a new PR to feature/security-scanning branch, so that i can update some config files before merging to main branch. Thank you in advance.

@maoo maoo changed the base branch from main to feature/security-scanning November 30, 2022 11:20
@yinan-symphony yinan-symphony merged commit 2dabc3e into finos:feature/security-scanning Nov 30, 2022
yinan-symphony added a commit that referenced this pull request Dec 5, 2022
* Add FINOS Security Actions (#693)

* add security scanning action

* add dependecycheck

* add renovate.json + change analyze to aggregate

* change action to also trigger on PR

Co-authored-by: Maurizio Pillitu <maoo@finos.org>

* Add renovate.json (#696)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Fix security scanning issues

Fix a security scanning reported issue in PresentationMLParser.java

Update semgrep config to ignore files in template and example folders

Use dashboardapproval feature from renovate bot

* Remove check on push action, keep it only for pull request

Ignore two issues detected in the code

* Ignore false positive CVE and non-fix-version CVE

Co-authored-by: Juan Estrella <juan.estrella@finos.org>
Co-authored-by: Maurizio Pillitu <maoo@finos.org>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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.

4 participants