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.

Sign up for GitHub

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

Jump to bottom

🌱 Refactoring: never assign unacceptable TLS versions #2037

Merged
merged 1 commit into from
Apr 25, 2024
Merged

🌱 Refactoring: never assign unacceptable TLS versions #2037

merged 1 commit into from
Apr 25, 2024

Conversation

pierreprinetti
Copy link
Contributor

@pierreprinetti pierreprinetti commented Apr 25, 2024 •
edited
Loading

What this PR does / why we need it:

Our downstream security scan is confused by GetTLSVersion returning 0 as a value (even if coupled by a non-nil error), which could end up being assigned to the same identifier that (in a non-error context) would set the TLS version.

This patch makes security linting easier by never setting a TLS version outside v1.2 or v1.3, even in case of an unacceptable user input.

Special notes for your reviewer:

This is expected to be a pure refactoring. Please reject this patch if it introduces any change in behaviour.

TODOs:

  • squashed commits
  • if necessary:
    • includes documentation
    • adds unit tests

/hold

Fixes: #2034

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 25, 2024
@netlify Netlify
Copy link

netlify bot commented Apr 25, 2024 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name Link
🔨 Latest commit 27526d5
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/662a79c8b541e500098bbd28
😎 Deploy Preview https://deploy-preview-2037--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@pierreprinetti pierreprinetti force-pushed the snyk_workaround branch 3 times, most recently from 9875542 to 5bc74ed Compare April 25, 2024 13:44
@mdbooth
Copy link
Contributor

mdbooth commented Apr 25, 2024

Thanks. Can you please do a couple of things for me before merging?

  • Mark this as fixing Static analyser gets confused about minimum TLS version #2034 in the description.
  • Add a comment in the affected function which explains why the code is a bit weird. Something like: "To make a static analyzer happy, ensure there is no code path...". That'll be a bit more discoverable than the commit message.

@pierreprinetti
Copy link
Contributor Author

  • To make a static analyzer happy, ensure there is no code path

done. PTAL

@mdbooth
Copy link
Contributor

mdbooth commented Apr 25, 2024

/approve

Thank you!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mdbooth

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 25, 2024
@pierreprinetti
Copy link
Contributor Author

/hold cancel
I think this is ready for a review.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 25, 2024
@MaysaMacedo
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 25, 2024
@pierreprinetti
Copy link
Contributor Author

/hold
good that I fixed the tests

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 25, 2024
@EmilienM
Copy link
Contributor

tests still failing. Happy to LGTM once they're fixed.

@pierreprinetti
Refactoring: never assign unacceptable TLS versions
27526d5 
This commit makes security linting easier by never setting a TLS version
outside v1.2 or v1.3, even in case of an unacceptable user input.
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 25, 2024
@pierreprinetti
Copy link
Contributor Author

/hold cancel
🤞

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 25, 2024
@mdbooth
Copy link
Contributor

mdbooth commented Apr 25, 2024

Unit tests pass, and this PR doesn't do anything which should affect the e2e tests.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 25, 2024
mdbooth pushed a commit to shiftstack/cluster-api-provider-openstack that referenced this pull request Apr 25, 2024
@pierreprinetti @mdbooth
CHERRY-PICK: Refactoring: never assign unacceptable TLS versions
49c1705 
This commit makes security linting easier by never setting a TLS version
outside v1.2 or v1.3, even in case of an unacceptable user input.

Upstream PR: kubernetes-sigs#2037
(cherry picked from commit 27526d5)
@mdbooth mdbooth mentioned this pull request Apr 25, 2024
Merged
1 task
@k8s-ci-robot k8s-ci-robot merged commit 2b6995e into kubernetes-sigs:main Apr 25, 2024
9 checks passed
@pierreprinetti pierreprinetti deleted the snyk_workaround branch April 25, 2024 18:22
@EmilienM
Copy link
Contributor

EmilienM commented May 6, 2024

@pierreprinetti do we want/need it in release-0.10?

@pierreprinetti
Copy link
Contributor Author

@pierreprinetti do we want/need it in release-0.10?

that'd be swell.

In the meantime, the bug has been validated by Snyk and reportedly passed on to their engineering.

@EmilienM
Copy link
Contributor

EmilienM commented May 6, 2024

/cherry-pick release-0.10

@k8s-infra-cherrypick-robot k8s-infra-cherrypick-robot mentioned this pull request May 6, 2024
Merged
@k8s-infra-cherrypick-robot
Copy link

@EmilienM: new pull request created: #2062

In response to this:

/cherry-pick release-0.10

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

This was referenced May 20, 2024
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dulek dulek Awaiting requested review from dulek

@lentzi90 lentzi90 Awaiting requested review from lentzi90

Assignees

@mdbooth mdbooth

@MaysaMacedo MaysaMacedo

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
CAPO Roadmap
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Static analyser gets confused about minimum TLS version
6 participants
@pierreprinetti @mdbooth @k8s-ci-robot @MaysaMacedo @EmilienM @k8s-infra-cherrypick-robot