feat(corda): support 5.1 via TS/HTTP (no JVM)

[adrianbatuto]

Commit to be reviewed

---

feat(corda): support 5.1 via TS/HTTP (no JVM)

Primary Changes
----------------
1. Updated plugin-ledger-connector corda to support Corda 5
2. Created corda-v5-flow.test for testing the Corda 5 endpoints

Fixes #2978 Fixes #3293 Pull Request Requirements

• [ ] Rebased onto upstream/main branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.
• [ ] Have git sign off at the end of commit message to avoid being marked red. You can add -s flag when using git commit command. You may refer to this link for more information.
• [ ] Follow the Commit Linting specification. You may refer to this link for more information.

Character Limit

• [ ] Pull Request Title and Commit Subject must not exceed 72 characters (including spaces and special characters).
• [ ] Commit Message per line must not exceed 80 characters (including spaces and special characters).

A Must Read for Beginners For rebasing and squashing, here's a must read guide for beginners.

[adrianbatuto]

@adrianbatuto Two questions:

1. Does this work without the JVM app? I see you've made changes to the kotlin code.
2. Would merging this still retain compatibility with 4.x Corda ledgers?

1. Yes it is working without the JVM app. The changes to the kotlin code happened after running yarn run codegen.
2. It should retain compatibility with Corda 4. I also ran some of the existing tests for Corda 4 to make sure.

[adrianbatuto]

The corda-v5-flow.test.ts is getting flaky results so I added a timeout as a temporary fix. I will look further into the issue and a separate ticket might be created for it.

[adrianbatuto]

Blocking the PR for now as the image used in this from @adrianbatuto personal github registry. Once the AIO docker image gets published from the Cacti account itself, the process of merging of this PR can progress.

@jagpreetsinghsasan this has been resolved.

[TheJagpreet]

@petermetz we have added the follow up tasks with the flakiness fix being the highest priority task in the queue followed by the cordapps feature issue. This PR looks good to me now and can be merged once @adrianbatuto rebase it and fix conflicts and we have a merge consensus from other maintainers.

[TheJagpreet]

@hyperledger/cacti-maintainers

[petermetz]

@petermetz we have added the follow up tasks with the flakiness fix being the highest priority task in the queue followed by the cordapps feature issue. This PR looks good to me now and can be merged once @adrianbatuto rebase it and fix conflicts and we have a merge consensus from other maintainers.

@jagpreetsinghsasan Thank you! Agreed on all of those!

[petermetz]

@jagpreetsinghsasan Have your change requests been addressed to your satisfaction?

[TheJagpreet]

@jagpreetsinghsasan Have your change requests been addressed to your satisfaction?

Yes @petermetz . Everything's addressed, looks good to me now

[petermetz]

@jagpreetsinghsasan Have your change requests been addressed to your satisfaction?

Yes @petermetz . Everything's addressed, looks good to me now

@jagpreetsinghsasan Got it, thank you! I'll dismiss your pending [1] change request then.

[1]

[petermetz]

Closing and reopening to try and trigger the GitHub workflow executions (they seem to be stuck for no reason)

[petermetz]

The CI is still stuck for some reason, I'll try to push a change to nudge it into action (hopefully).

[adrianbatuto]

@adrianbatuto Please fix the failing mandatory checks (lint, codegen, custom-checks etc.) and also address the CodeQL security issues.

Hi @petermetz, I will look into those. For the codeql issues please see Jag's comments on #3322.

[TheJagpreet]

@adrianbatuto Please fix the failing mandatory checks (lint, codegen, custom-checks etc.) and also address the CodeQL security issues.

Hi @petermetz, I will look into those. For the codeql issues please see Jag's comments on #3322.

@adrianbatuto if you see the solution of CodeQL reviews, I think that's something we can definitely implement (reconstructing the entire URL and it will still be a variable input as per our needs) (A nice learning from security perspective !)

[adrianbatuto]

@adrianbatuto Please fix the failing mandatory checks (lint, codegen, custom-checks etc.) and also address the CodeQL security issues.

Hi @petermetz, I will look into those. For the codeql issues please see Jag's comments on #3322.

@adrianbatuto if you see the solution of CodeQL reviews, I think that's something we can definitely implement (reconstructing the entire URL and it will still be a variable input as per our needs) (A nice learning from security perspective !)

I'll also look into this then. Thanks Jag.

[TheJagpreet]

@adrianbatuto Please fix the failing mandatory checks (lint, codegen, custom-checks etc.) and also address the CodeQL security issues.

Hi @petermetz, I will look into those. For the codeql issues please see Jag's comments on #3322.

OK, we'll figure something out with Jagpreet and let you know!

@petermetz we had looked into the solution provided by CodeQL and we can definitely incorporate those. @adrianbatuto is currently doing the same (please re-request for review once done)

[adrianbatuto]

The scans have run after rebasing and fixing the merge conflicts. However, the CodeQL is still giving warnings despite my changes. These are false positives in my opinion but let me know what you think as well. @jagpreetsinghsasan @petermetz

[petermetz]

The scans have run after rebasing and fixing the merge conflicts. However, the CodeQL is still giving warnings despite my changes. These are false positives in my opinion but let me know what you think as well. @jagpreetsinghsasan @petermetz

@adrianbatuto Try doing these things to get us closer to a more safe solution and if CodeQL still fails we can consider marking the issues as false positives (but before we do that I want to make sure we've done proper a thorough evaluation)

1. Specify the API host through the constructor arguments so that it's fixed for the lifetime of the connector instance.
2. Use a URL builder library such as https://github.com/balazsbotond/urlcat
3. Check if there is a way for us to retrieve the possible values of holdingIDShortHash from Corda.
4. Check if there are any more pattern (RegExp) based restrictions we could infer from Corda documentation or Corda's source code: I often find that when I read the other project's source code they themselves have regular expressions defined that narrow the scope of characters allowed, the length it can be, etc. For example if we can safely assume that holdingIDShortHash is always alpha-numeric and a certain length, then I'd already feel much much safer with marking the CodeQL warning about it a false positive because with those restrictions it is already very unlikely that an SSRF could happen based on that.
5. The same logic (roughly) can be applied to other potential parameters as well

[adrianbatuto]

The scans have run after rebasing and fixing the merge conflicts. However, the CodeQL is still giving warnings despite my changes. These are false positives in my opinion but let me know what you think as well. @jagpreetsinghsasan @petermetz

@adrianbatuto Try doing these things to get us closer to a more safe solution and if CodeQL still fails we can consider marking the issues as false positives (but before we do that I want to make sure we've done proper a thorough evaluation)

1. Specify the API host through the constructor arguments so that it's fixed for the lifetime of the connector instance.
2. Use a URL builder library such as https://github.com/balazsbotond/urlcat
3. Check if there is a way for us to retrieve the possible values of holdingIDShortHash from Corda.
4. Check if there are any more pattern (RegExp) based restrictions we could infer from Corda documentation or Corda's source code: I often find that when I read the other project's source code they themselves have regular expressions defined that narrow the scope of characters allowed, the length it can be, etc. For example if we can safely assume that holdingIDShortHash is always alpha-numeric and a certain length, then I'd already feel much much safer with marking the CodeQL warning about it a false positive because with those restrictions it is already very unlikely that an SSRF could happen based on that.
5. The same logic (roughly) can be applied to other potential parameters as well

Thanks @petermetz. I'll look into these points you mentioned.

[adrianbatuto]

@adrianbatuto Try doing these things to get us closer to a more safe solution and if CodeQL still fails we can consider marking the issues as false positives (but before we do that I want to make sure we've done proper a thorough evaluation)

1. Specify the API host through the constructor arguments so that it's fixed for the lifetime of the connector instance.
2. Use a URL builder library such as https://github.com/balazsbotond/urlcat
3. Check if there is a way for us to retrieve the possible values of holdingIDShortHash from Corda.
4. Check if there are any more pattern (RegExp) based restrictions we could infer from Corda documentation or Corda's source code: I often find that when I read the other project's source code they themselves have regular expressions defined that narrow the scope of characters allowed, the length it can be, etc. For example if we can safely assume that holdingIDShortHash is always alpha-numeric and a certain length, then I'd already feel much much safer with marking the CodeQL warning about it a false positive because with those restrictions it is already very unlikely that an SSRF could happen based on that.
5. The same logic (roughly) can be applied to other potential parameters as well

Hi @petermetz , the above points have been addressed in my latest commit.

[adrianbatuto]

@adrianbatuto Looking good! Now the last thing (hopefully) is to fix the failing corda connector server build

Cactus_CI / ghcr-connector-corda-server (pull_request) Failing after 1m Details

@petermetz, I fixed the build error and now the check is failing because of vulnerability issues.

[petermetz]

@adrianbatuto Thanks for the fix, we can address the trivy vulnerabilities later in a different PR, so, looking good to me now.