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

fix(cloudevents/server): fix ssl verify error for internal server url #134

Merged
merged 1 commit into from
Apr 18, 2024

Conversation

wuhuizuo
Copy link
Contributor

Signed-off-by: wuhuizuo wuhuizuo@126.com

Copy link

ti-chi-bot bot commented Apr 18, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

Based on the pull request diff, it looks like the change is adding a custom transport with InsecureSkipVerify set to true to handle SSL verify errors when making HTTP requests to an internal server URL.

Potential problems with this change:

  • By setting InsecureSkipVerify to true, the SSL certificate verification is skipped, which could lead to security vulnerabilities. It is not recommended to use this in a production environment.
  • The change is not adding any tests to validate the SSL certificate verification.

Suggestions to address the problems:

  • Instead of disabling SSL verification, a better solution would be to add the internal server's SSL certificate to the trusted CA list. This way, the verification will work correctly, and there will be no security vulnerabilities.
  • Add tests to validate the SSL certificate verification and ensure that the code works as expected.

Overall, the change is solving the SSL verification issue, but it is not addressing the problem in a recommended way. It would be better to add the internal server's SSL certificate to the trusted CA list instead of disabling SSL verification.

@ti-chi-bot ti-chi-bot bot added the size/S label Apr 18, 2024
@wuhuizuo
Copy link
Contributor Author

/approve

Copy link

ti-chi-bot bot commented Apr 18, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wuhuizuo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added the approved label Apr 18, 2024
@ti-chi-bot ti-chi-bot bot merged commit 8d08984 into main Apr 18, 2024
3 checks passed
@ti-chi-bot ti-chi-bot bot deleted the fix/container-log-request-ssl-verify branch April 18, 2024 03:19
@wuhuizuo
Copy link
Contributor Author

/review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant