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 code scanning alert no. 50: Disabled TLS certificate check #369

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jkrech
Copy link
Member

@jkrech jkrech commented Oct 31, 2024

Fixes https://github.com/Open-CMSIS-Pack/cpackget/security/code-scanning/50

To fix the problem, we should avoid setting InsecureSkipVerify to true even for localhost connections. Instead, we can configure the application to use a self-signed certificate for local development and testing. This way, we maintain the security of TLS connections without disabling certificate verification.

  • Generate a self-signed certificate and key for localhost.
  • Configure the http.Transport to use this certificate for connections to 127.0.0.1.
  • Update the DownloadFile function to load and use the self-signed certificate when connecting to 127.0.0.1.

Suggested fixes powered by Copilot Autofix. Review carefully before merging.

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
@jkrech jkrech requested a review from bgn42 November 18, 2024 14:26
Copy link
Collaborator

@bgn42 bgn42 left a comment

Choose a reason for hiding this comment

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

Sollen die "path/to/localhost.*" noch ersetzt werden?

@jkrech
Copy link
Member Author

jkrech commented Nov 18, 2024

I guess so. We could define a default location and filename for self-signed certificate, if I understand this correctly, or we simply bail out but would not be able to run tests locally easily.

Copy link
Collaborator

@bgn42 bgn42 left a comment

Choose a reason for hiding this comment

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

lgtm

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.

3 participants