-
Notifications
You must be signed in to change notification settings - Fork 282
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
Conversation
️✅ 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. 🦉 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. |
Thanks for contributing! Izuru and I will check the codes and review it soon!
@123wyl123 |
@izuru0 |
...in-ledger-connector-tcs-huawei-socketio/src/main/typescript/connector/ServerMonitorPlugin.ts
Fixed
Show fixed
Hide fixed
There was a problem hiding this 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:
- 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
- The versions of the packages need to be updated to match the current release version in
lerna.json
- 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.
- 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
- 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:
@petermetz Thank you for your reply, I just saw this part of the reply, I will finish all the changes as soon as possible |
862ff88
to
9ecd0b3
Compare
@petermetz |
9ecd0b3
to
c418353
Compare
There was a problem hiding this 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.
- 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. - 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.
- 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 :-)) - 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.
54a1b02
to
0465c13
Compare
@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. |
There was a problem hiding this 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.
...in-ledger-connector-tcs-huawei-socketio/src/main/typescript/connector/ServerMonitorPlugin.ts
Fixed
Show fixed
Hide fixed
...in-ledger-connector-tcs-huawei-socketio/src/main/typescript/connector/ServerMonitorPlugin.ts
Fixed
Show fixed
Hide fixed
@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. |
0465c13
to
6aed7a7
Compare
@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) |
There was a problem hiding this 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)
@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. |
There was a problem hiding this 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>
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