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

feat(connector-tcs-huawei): add initial version #2417

Merged
merged 1 commit into from
Jul 3, 2023

Conversation

123wyl123
Copy link

tcs-huawei and the blockchains connected to tcs-huawei can be integrated to cactus by this connector.

Signed-off-by: Wang Yinglun wangyinglun3@huawei.com

@gitguardian
Copy link

gitguardian bot commented May 8, 2023

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
Once a secret has been leaked into a git repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

takeutak
takeutak previously approved these changes May 9, 2023
@takeutak takeutak self-requested a review May 9, 2023 10:04
@takeutak takeutak dismissed their stale review May 9, 2023 10:06

Thanks for contributing! Izuru and I will check the codes and review it soon!

@izuru0
Copy link
Contributor

izuru0 commented May 12, 2023

@123wyl123
can you remove hardcoded secrets in sample-config and Server_Cert directories?
you can do this by creating necessary files in the build scripts, for example

@123wyl123
Copy link
Author

@izuru0
Thank you for your reply, I will revise that part

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@123wyl123 Thank you for submitting this! I've gone through it on a high level and I noticed a couple of minor things:

  1. Right now the packages you added are not part of the tsc build step because they are not declared in the tsconfig.json file in the root
  2. The versions of the packages need to be updated to match the current release version in lerna.json
  3. The changelog.md file needs to be deleted (because right now it is a copy paste of another such file but these get auto-generated as part of the release process so you don't need to commit one.
  4. The git commit message should also match what the PR title is and be passing the linter with something like feat(connector-tcs-huawei): add initial version
  5. The packages need to be set to not private otherwise the release scripts will ignore them instead of publishing them.

I'm also attaching a diff with the suggested changes I made locally to see if the compilation works that way:

diff.log

@123wyl123
Copy link
Author

@petermetz Thank you for your reply, I just saw this part of the reply, I will finish all the changes as soon as possible

@123wyl123 123wyl123 changed the title feat(connector-tcs-huawei): Initial commit of feature tcs-huawei connector. feat(connector-tcs-huawei): add initial version Jun 18, 2023
@123wyl123
Copy link
Author

@petermetz
Hello, I made the corresponding changes according to the request, but github still has some checking tools not passed, do I need to cresubmit a new MR request

@123wyl123 123wyl123 requested a review from petermetz June 19, 2023 00:41
@petermetz petermetz changed the title feat(connector-tcs-huawei): add initial version feat(connector-tcs-huawei): add initial version Jun 21, 2023
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@petermetz
Hello, I made the corresponding changes according to the request, but github still has some checking tools not passed, do I need to cresubmit a new MR request

@123wyl123 Thank you very much for the updates! You do not have to (nor should you) submit a brand new pull request because then we would lose the review comments/history/context that was already made on this one.

Regarding the failing checks, some of them are under renovation right now and therefore don't matter, others do need to be addressed though so I will just list here the ones that do need to be addressed and you can ignore the rest.

  1. PR Title Lint: I've fixed this one for you. The issue was that you had a space character (" ") in the beginning and so the linter failed the check because of that. I just removed the space character and now it's passing the check so we are good here but I wanted to explain it anyway just for future reference.
  2. Commit Linting checks: You'll need to squash your commits into a single one at the end and make the subject line of it match what you have in the PR title on github (note that the git commit message and the PR title/description are not the same thing unfortunately. One is stored in the database owned and controlled by github the commercial entity and the other (the git log) is what the Cacti contributor community owns via the license. We like to keep the two in sync as much as possible to make things less confusing. The reason I explain this in such detail is because sometimes people update the PR description and expect the commit linter to start passing but what you need to do instead (or in addition) is to do an amend of your git commit message and then force push with lease.
  3. CodeQL failure due to disabled certificate validation in code that appears to be made for production (packages/cactus-plugin-ledger-connector-tcs-huawei-socketio/src/main/typescript/connector/ServerMonitorPlugin.ts From what I understand this code file is meant to be deployed in production but it disables certificate validation which would make any deployment immediately very not secure and so this definitely has to be changed. I'm guessing that the disabling of the certificate validation was done for testing purposes so that you can get it to work in a development environment for self signed certificates. This is a legitimate use-case but the way I would handle it is to 1) document in the developer/build/test documentation that the cert validation needs to be disabled via environment variables (so not through the code) and 2) remove the code that does the unconditional disabling of the cert validation so that CodeQL passes. This way the default for production deployments becomes the safe option of certificate validation (safe defaults is one of our core design principles for the project hence me writing this long essay about the issue :-))
  4. The GitGuardian secrets that are being hard-coded: I can see that these are all meant to be for testing, so if it's needed we can dismiss the alerts but if there is an easy option to avoid hardcoding them then please do so. I'm totally fine with dismissing the alerts though because it's obvious that they are for testing/examples so this should not make us fail any reasonable security audit (there's a balance to be struck with convenience/documentation quality and automated security check obedience)

So these above are the things I would try to address and as always, if you need any help with the git mechanics just let me know and I can step in to fix it for you on your branch with your permission (please do not close the PR - no matter what, it's almost never the right solution to git trouble).

The build-dev check is failing right now because of unrelated issues that are stemming from problems with an external service (search.maven.org) so you can safely ignore that issue for now as I am working on the fix for that for all PRs on the project globally.

@123wyl123 123wyl123 force-pushed the dev-tcs-huawei branch 2 times, most recently from 54a1b02 to 0465c13 Compare June 23, 2023 12:15
@123wyl123
Copy link
Author

@petermetz Thank you for your help, your reply solved my problem with submitting and self-signed certificates(rejectUnauthorized: false or other more elegant ways ) very well. for hard coding I have added a new script.

@123wyl123 123wyl123 requested a review from petermetz June 24, 2023 01:27
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@123wyl123 Thank you for the updates!

rejectUnauthorized: false, is still not a good solution unfortunately because you are hardcoding the parameter to be set to false in the startMonitor() and periodicMonitoring() methods in packages/cactus-plugin-ledger-connector-tcs-huawei-socketio/src/main/typescript/connector/ServerMonitorPlugin.ts so the problem is the same as before in the sense that the code (as it is) poses a critical security vulnerability when deployed because anyone can mount a man-in-the-middle attack against it when it comes to the responses of the /v1/cross/transaction/query endpoint.

There is a more detailed explanation about the issue here: https://httptoolkit.com/blog/node-https-vulnerability/
On the above post they also talk about problems with parameterizing the value passed in for rejectUnauthorized and then leaving it optional with the falsy value being the default.

@123wyl123
Copy link
Author

@petermetz thanks for your help, Do you mean I set it to true to check or delete the section ? Then use comments to indicate that if self-signature certificate is used, you need to set environment variables to ensure certificate verification. I don't think I fully understand the /build/test documentation part, so I just used rejectUnauthorized.

@123wyl123 123wyl123 requested a review from petermetz June 27, 2023 12:50
@petermetz
Copy link
Contributor

@petermetz thanks for your help, Do you mean I set it to true to check or delete the section ? Then use comments to indicate that if self-signature certificate is used, you need to set environment variables to ensure certificate verification. I don't think I fully understand the /build/test documentation part, so I just used rejectUnauthorized.

@123wyl123 Your latest changes from 6 hours ago is what I meant, so I think we are good to go now (assuming that the CodeQL scan will pass now)

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@123wyl123 Please open an issue to track the addition of automated tests to the connector (which will also need the container image builds to be fixed)

@123wyl123
Copy link
Author

@petermetz Hello, peter, thank you for your help, I will add the functionality related to this part to our long running service as soon as possible, as well as the subsequent development.
Is there anything else I need to do for these failed checks on github?

Copy link
Contributor

@izuru0 izuru0 left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: wangyinglun <wangyinglun3@huawei.com>
@petermetz petermetz enabled auto-merge (rebase) July 3, 2023 01:42
@petermetz petermetz merged commit d8d538d into hyperledger-cacti:main Jul 3, 2023
109 of 117 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.

4 participants