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 for python3-requests 2.32.2 #5435

Merged
merged 2 commits into from
Jun 27, 2024
Merged

fix for python3-requests 2.32.2 #5435

merged 2 commits into from
Jun 27, 2024

Conversation

eaglegai
Copy link
Contributor

@eaglegai eaglegai commented Jun 24, 2024

Fix for requests 2.32.2+:
Add get_connection_with_tls_context() for requests 2.32.2 as suggests: psf/requests#6710

@holmanb holmanb added the CLA not signed The submitter of the PR has not (yet) signed the CLA label Jun 24, 2024
@a-dubs
Copy link
Collaborator

a-dubs commented Jun 24, 2024

Thank you for this contribution! Would you mind adding your GitHub username to the tools/.github-cla-signers file and committing that change to this PR?

Additionally, you need to sign the Canonical contributor licence agreement. This grants us your permission to use your contributions in the project.

Thanks in advance!

@a-dubs
Copy link
Collaborator

a-dubs commented Jun 24, 2024

Oh also, would you mind editing your PR description so that it matches the desired pull request template as such:

Proposed Commit Message

fix: Add get_connection_with_tls_context() for requests 2.32.2+

get_connection() is deprecated in requests 2.32.2+ so this will
allow for the LXDSocketAdapter to avoid using a deprecated api.

Fixes GH-5434

Additional Context

Fixes: #5434

Test Steps

Checklist

Merge type

  • Squash merge using "Proposed Commit Message"
  • Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

@eaglegai
Copy link
Contributor Author

Thank you for this contribution! Would you mind adding your GitHub username to the tools/.github-cla-signers file and committing that change to this PR?

Additionally, you need to sign the Canonical contributor licence agreement. This grants us your permission to use your contributions in the project.

Thanks in advance!

I'm trying.

get_connection() is deprecated in requests 2.32.2+ so this will
allow for the LXDSocketAdapter to avoid using a deprecated api.

Fixed:canonical#5434

Signed-off-by: eaglegai <eaglegai@163.com>
@eaglegai
Copy link
Contributor Author

@a-dubs hello, I have finished the CLA, and changed the commit message.

@blackboxsw
Copy link
Collaborator

@eaglegai @a-dubs I just saw the "Check ruff" job failed because we didn't run tox -e do_format on this branch to sort line-length automatically. I just performed that step locally, squash merged into the original commit and git --force pushed the branch to sort the formatting complaint. Thanks again! When we see the CLA signing show up on our internal docs I think we are probably good here.

@a-dubs
Copy link
Collaborator

a-dubs commented Jun 26, 2024

Thank you @blackboxsw !

@holmanb holmanb self-assigned this Jun 27, 2024
@holmanb
Copy link
Member

holmanb commented Jun 27, 2024

Thanks for proposing this @eaglegai!

To quote the description in psf/requests#6710, the proposed solution (which matches the example code) should be backwards compatible between versions of Requests.

Addressing the CVE in requests and its impact on cloud-init:

From the description of CVE-2024-34195:

When making requests through a Requests Session, if the first request is made with verify=False to disable cert verification, all subsequent requests to the same origin will continue to ignore cert verification regardless of changes to the value of verify. This behavior will continue for the lifecycle of the connection in the connection pool.

The proposes remediations are:

Any of these options can be used to remediate the current issue, we highly recommend upgrading as the preferred mitigation.

  • Upgrade to requests>=2.32.0.
  • For requests<2.32.0, avoid setting verify=False for the first request to a host while using a Requests Session.
  • For requests<2.32.0, call close() on Session objects to clear existing connections if verify=False is used.

In cloud-init, this code is used only to communicate over a local unix socket to a trusted service, the LXD IMDS. It does not use verify=False for any requests.

While requiring a dependency on 2.32.0 is ultimately a downstream choice to make, from an upstream perspective we already have a recommended remediation in place.

Copy link
Member

@holmanb holmanb left a comment

Choose a reason for hiding this comment

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

I've requested Canonical's security team take a look at this, but I feel comfortable with merging this since cloud-init already remediates the issue and the change in this PR is simply implementing the recommended way to work with the api changes in 2.32.2 and greater that resulted from this CVE.

@holmanb holmanb added CLA signed The submitter of the PR has signed the CLA and removed CLA not signed The submitter of the PR has not (yet) signed the CLA labels Jun 27, 2024
@holmanb holmanb merged commit 790d229 into canonical:main Jun 27, 2024
23 checks passed
@mdeslaur
Copy link

ACK from the Security Team, change looks reasonable, and is backwards compatible.

holmanb pushed a commit that referenced this pull request Jun 28, 2024
get_connection() is deprecated in requests 2.32.2+ so this will
allow for the LXDSocketAdapter to avoid using a deprecated api.

Fixes GH-5434

Signed-off-by: eaglegai <eaglegai@163.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA signed The submitter of the PR has signed the CLA security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants