bug(dast): ccp, sshconfig missing
Describe the bug
Due to the incorporation of https://github.com/hyperledger-cacti/cacti/pull/3578 task, the DAST again got broken as it now requires a sshconfig, ccp input
To Reproduce
The failing CI test can be seen in every PR at the moment
Expected behavior
The DAST scan should run without failing
Logs/Stack traces
https://github.com/hyperledger-cacti/cacti/actions/runs/12153966297/job/33892946268?pr=3659
Additional context
Adding empty sshConfig and connectionProfile` to this line: https://github.com/hyperledger-cacti/cacti/blob/fbe68c12bf35a69cac8df7044ede937ca8c7dcd6/.github/workflows/.dast-nuclei-cmd-api-server.yaml#L88 shall fix the broken test
Hello Team,
Please assign me this task. Thank you!
Rayn
@jagpreetsinghsasan Once you have a working fix for this (and it is merged onto main) please also make sure to set the DAST scan to be required in the branch protection rules. That way we prevent it breaking again next time a similar change is made.
One more point: The fact that DAST starting breaking with existing configuration is pointing to the idea that maybe we accidentally created a breaking change in the Fabric connector. Could you please double check? New features should not make previously working functionality break with previously valid configuration (unless we are issuing a new major release where breaking changes are allowed). If it turns out that we did make an accidental breaking change then please open an issue to tackle that by way of investigating what can be done where the two main options are:
- Revert the change for now and schedule it for v3.0.0
- Refactor the code so that the new code in the connector is backward compatible (if this is possible then it's the preferred way, but it's not always possible to refactor things with perfect backward compatibility)
Hello @petermetz ,
As per my investigation, DAST on other connectors are working since there is no validation for sshConfig
Here is the validation on plugin-ledger-connector-fabric:
Here is the actual error log:
I want to know if sshConfig is a required field or not. If it is not required, maybe we can remove the validation that throws an error when there is no sshConfig.
I created a PR to perform initial investigation together with the proposed solution that I am looking at to solve this issue.
Cheers! Rayn
@petermetz @aldousalvarez @raynatopedrajeta
We have another breaking change when we added an authorization token to the cmd-api-server healthcheck endpoint in #2693 but this didn't get tested in DAST scan as DAST scan was failing due to another error (the besu aio not starting). When we fixed the besu aio error, the DAST scan started failing due to another PR (where we serialized ccp and sshconfig inputs for fabric connector) and while fixing that, we found the auth token missing for /healthcheck endpoint.
So 2 points here,
- We have to include #2693 as a breaking change for v3.0 release here: #3663
- For the fix of this task, #3671, we can remove fabric based breaking changes and remove the DAST scan check for the
healthcheckendpoint and that will be the correct solution to this ticket. Also, @raynatopedrajeta please create another ticket to add the healthcheck endpoint again after v3.0 release is done.
@petermetz @aldousalvarez @raynatopedrajeta
We have another breaking change when we added an authorization token to the cmd-api-server healthcheck endpoint in #2693 but this didn't get tested in DAST scan as DAST scan was failing due to another error (the besu aio not starting). When we fixed the besu aio error, the DAST scan started failing due to another PR (where we serialized ccp and sshconfig inputs for fabric connector) and while fixing that, we found the auth token missing for /healthcheck endpoint.
So 2 points here,
- We have to include refactor(cmd-api-server): pull OAuth2 endpoint scopes from openapi.json #2693 as a breaking change for v3.0 release here: chore(release): publish v3.0.0 #3663
- For the fix of this task, bug(dast): ccp, sshconfig missing #3671, we can remove fabric based breaking changes and remove the DAST scan check for the
healthcheckendpoint and that will be the correct solution to this ticket. Also, @raynatopedrajeta please create another ticket to add the healthcheck endpoint again after v3.0 release is done.
@jagpreetsinghsasan Oh wow, nice investigation! Agreed on both points.
This is also a great reminder for everyone - myself included - how easy it is to accidentally sneak in a breaking change and not even realize it for months.
@raynatopedrajeta Are you still working on this?
@raynatopedrajeta Are you still working on this?
Hello @petermetz ,
I will work on the remaining review points today.
Thank you!
Rayn