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 owasp dependency check action #31511

Merged
merged 1 commit into from
May 16, 2023

Conversation

sberyozkin
Copy link
Member

I'm not 100% sure it is correct since I've never done github actions before, I just copied the codeql one and modified.

Also, I'm not sure if, instead of doing a check goal it would be better to have

    <reporting>
        ...
        <plugins>
            ...
            <plugin>
                <groupId>org.owasp</groupId>
                <artifactId>dependency-check-maven</artifactId>
                <version>8.1.2</version>
                <reportSets>
                    <reportSet>
                        <reports>
                            <report>aggregate</report>
                        </reports>
                    </reportSet>
                </reportSets>
            </plugin>
            ...
        </plugins>
        ...
    </reporting>

@sberyozkin sberyozkin requested review from aloubyansky and gsmet March 1, 2023 13:58
@sberyozkin
Copy link
Member Author

I'll probably need to tune it in a few iterations to get the best format, unfortunately it is not run as part of the PR which brings it

pom.xml Show resolved Hide resolved
@sberyozkin sberyozkin force-pushed the owasp_github_action branch from 0506e56 to 5a500db Compare March 7, 2023 18:30
@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure labels Mar 7, 2023
@sberyozkin sberyozkin force-pushed the owasp_github_action branch 2 times, most recently from ee40415 to dd11330 Compare March 8, 2023 17:27
@sberyozkin
Copy link
Member Author

sberyozkin commented Mar 8, 2023

@gsmet @aloubyansky Hi, I've experimented quite a lot with running an aggregate report with and without <reporting> and it looks like to be the best option is simply run mvn -Dowasp-report which will run dependency-check:aggregate, avoiding mvn site required for reporting altogether as mvn site, depending on whether it is run in the root or in the extensions directories, can ignore a selective report configuration and just will do 15 of them.

mvn -Dowasp-report is quite fast, creates a single HTML report in quarkus-root-dir/target. This might work well for RHBQ as well avoiding various Snyk issues. As far as the github action is concerned, I'm not sure yet how this quarkus-root-dir/target will be accessible, I guess one way or another it will be possible to make it easily accessible from the action result page, I'll just need to experiment with it, and in meantime both mvn -Dowasp-report and mvn -Dowasp-check can be run directly anyway.

FYI, the difference between mvn -Dowasp-report and mvn -Dowasp-check is that the former won't be building the reports in every subdirectory.

I'll open for review tomorrow once Guillaume does the alpha 5 release

@sberyozkin sberyozkin marked this pull request as ready for review March 9, 2023 12:37
@quarkus-bot

This comment has been minimized.

@gsmet
Copy link
Member

gsmet commented Apr 4, 2023

@sberyozkin getting there... I had a look at the CI and what bugs me is that we won't have any report if something's failing. Should we include it in the usual reporting?

I'm also not sure if something is failing if there is a security problem or if it's just a report?

@sberyozkin sberyozkin force-pushed the owasp_github_action branch from dd11330 to c651698 Compare April 10, 2023 20:34
@sberyozkin sberyozkin marked this pull request as draft April 10, 2023 20:43
@quarkus-bot

This comment has been minimized.

@sberyozkin
Copy link
Member Author

Hi @gsmet, apologies, seeing your comment only now.

I had a look at the CI and what bugs me is that we won't have any report if something's failing. Should we include it in the usual reporting?

and

I'm also not sure if something is failing if there is a security problem or if it's just a report?

I'm not sure I understand the questions, I think I might be partially, OK, so right now, the only output of this action is an HTML report in quarkus-root-dir/target - I don't know how to have it reported similarly to how it is done for CodeQL. I've been hoping I can just somehow access that HTML report in quarkus-root-dir/target and that would suffice to start with. Is it what you are concerned about, there is no obvious way to access the results ?

The other thing is that it is not expected to fail, it is possible to configure it to fail when it sees the first CVE of some category, but I believe it is simpler to get the HTML report in quarkus-root-dir/target and then look at it and check what public CVEs are reported against various extensions.

So right now, the main challenge, as I see it, is to get an access to the HTML report in quarkus-root-dir/target once the action is complete, either somehow directly to start with, or have this HTML report page shown in the action page somehow.

What do you think ?

@jmartisk
Copy link
Contributor

Could we simply attach the report using the upload-artifact action?

    - uses: actions/upload-artifact@v3
      with:
        name: owasp-report
        path: target/dependency-check-report.html

Potentially we could also figure out how to send it via email to interested people?

I would also, for better troubleshooting, add the workflow_dispatch trigger to the workflow so we can trigger it manually if needed.

@jmartisk
Copy link
Contributor

Uhm, the HTML report is 20 megabytes big, so it's probably not a good idea to receive it by email every day. We probably should thus also decrease the retention period (default is 90 days):

    - uses: actions/upload-artifact@v3
      with:
        name: owasp-report
        path: target/dependency-check-report.html
        retention-days: 5

@jmartisk
Copy link
Contributor

If we get the report published daily in the workflow's artifacts, then the responsible engineer(s) would just have to open it and skim through it once every few days

@sberyozkin sberyozkin force-pushed the owasp_github_action branch from c651698 to 4c3ae87 Compare April 17, 2023 16:03
@sberyozkin
Copy link
Member Author

Hey @jmartisk Thanks, that was was missing, an upload artifact step, thanks

Uhm, the HTML report is 20 megabytes big, so it's probably not a good idea to receive it by email every day.

I agree, uploading it shoul be enough, though I guess what we can send, is a link, but then the question of managing emails, with all the GDPR angle, would make it tricky.
Also, we can review and decide if we want some extensions (or libraries) skipped, not sure how to configure the plugin, but some related tuning should be possible

If we get the report published daily in the workflow's artifacts,

May be we can try say every 3 days ?

then the responsible engineer(s) would just have to open it and skim through it once every few days

Sure, that would be the plan, myself, two of us, can look at it periodically

@jmartisk
Copy link
Contributor

Yeah let's make it run, say, on Sunday, Tuesday, Thursday, or something like that :)

@sberyozkin
Copy link
Member Author

@jmartisk How about Sunday, Wednesday end of the day (0 0 * * 0,3), twice per week, in order to keep the system stress to the minimum and then, with your update, a Friday run can be triggered manually, when running closer to one of Final releases ?

@jmartisk
Copy link
Contributor

Sure, works for me

@sberyozkin sberyozkin force-pushed the owasp_github_action branch from df1c254 to 5782319 Compare April 18, 2023 16:33
@sberyozkin
Copy link
Member Author

It should be ready for review now, I can open tomorrow

@sberyozkin sberyozkin marked this pull request as ready for review April 19, 2023 10:49
@sberyozkin
Copy link
Member Author

Hi @gsmet @aloubyansky, I think/hope, after @jmartisk review that it can prove to be practically useful, now that the HTML reports will be uploaded, we can continue tuning things like the action frequency (twice per week now), retention period of the uploaded reports, etc.
Have a look please if you have something else to add, it would be good to get it into 3.1.0 and work with it in the run to 3.2.0

<nuspecAnalyzerEnabled>false</nuspecAnalyzerEnabled>
<retireJsAnalyzerEnabled>false</retireJsAnalyzerEnabled>
</configuration>
</plugin>
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to have it configured in the root pom and here?

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 @aloubyansky, sorry, missed your ping, OK, right now on main, one can only run the check from the individual extensions which won't work with the action, but I can see I can run it from the extensions folder, which I believe would require setting a working-directory property to point to extensions - but I'm not sure it will be equivalent to running it from the root folder, let me experiment a bit later on and I'll get back to you with the update, thanks

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 Alexey @aloubyansky Sorry for a delay, I just run, on this branch, mvn -Dowasp-report in the root (not possible if the root pom does not have it configured) and in extensions (which is possible on main, for -Dowasp-check for now).
The dependency-check.html is 20 MB if created in the root, and 13 MB if created in extensions.
I think it makes sense to run from the root as it can check more dependencies, for dev tools, etc...
Do you agree ?
Thanks

Copy link
Member

Choose a reason for hiding this comment

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

Sure, my question was about the reason why the config is duplicated. If you configure it in the pluginManagement of the root POM then here you could remove the version and the configuration element.

Copy link
Member Author

Choose a reason for hiding this comment

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

@aloubyansky Oh sorry, see what you mean, totally misunderstood your question :-), I'll have a look to clean it up, thanks

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 @aloubyansky I removed the owasp plugin section from the build parent pom, hopefully I did it correctly, thanks.

@quarkus-bot

This comment has been minimized.

@sberyozkin sberyozkin force-pushed the owasp_github_action branch from 5782319 to 9ae0d45 Compare May 8, 2023 14:36
@quarkus-bot
Copy link

quarkus-bot bot commented May 8, 2023

Failing Jobs - Building 9ae0d45

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 17 Build Failures Logs Raw logs
✔️ JVM Tests - JDK 19

Failures

⚙️ JVM Tests - JDK 17 #

- Failing: integration-tests/kafka-oauth-keycloak 

📦 integration-tests/kafka-oauth-keycloak

Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.0.0:test (default-test) on project quarkus-integration-test-kafka-oauth-keycloak: There was a timeout in the fork

@jmartisk
Copy link
Contributor

I think this is ready to go, isn't it? @sberyozkin @aloubyansky

@aloubyansky aloubyansky merged commit 1773aa5 into quarkusio:main May 16, 2023
@quarkus-bot quarkus-bot bot added this to the 3.1 - main milestone May 16, 2023
@sberyozkin
Copy link
Member Author

Thanks @aloubyansky @jmartisk @gsmet

Hopefully it will prove useful and we can optimize the action to minimize some noise in the report, etc

@sberyozkin sberyozkin deleted the owasp_github_action branch May 16, 2023 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants