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

Update requests to fix CVEs (security) #6062

Closed
wants to merge 21 commits into from

Conversation

jk464
Copy link
Contributor

@jk464 jk464 commented Nov 6, 2023

Fixes up a host of CVEs in the st2 package:

Note: The XRAY references are vulnerabilities listed by JFrog Xray, that don't seem to have a corresponding CVE. JFrog doesn't seem to publish these references publicly - but I've linked to the issue disclosing the vulnerability thats referenced by the XRAY entry.

Bump cryptography to 41.0.4, pyopenssl to 23.2.0

Fixes:

pyopenssl 23.2.0 required for cryptography to 41.0.x support

Bump virtualenv to 20.16.7

Fixes:

Bump importlib-metadata to 4.10.1

Fixes:

Bump requests to 2.31.0

Fixes:

Bump gitpython to 3.1.37

Fixes:

Supercedes/Implements

@pull-request-size pull-request-size bot added the size/M PR that changes 30-99 lines. Good size to review. label Nov 6, 2023
jk464 added a commit to jk464/st2 that referenced this pull request Nov 6, 2023
@arm4b arm4b added the security label Nov 6, 2023
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

Thanks!

Left a few first pointers. The main indicator is 🟢 tests, where some more work will be needed to get them all happy for both py3.6 and 3.8+.

fixed-requirements.txt Outdated Show resolved Hide resolved
fixed-requirements.txt Outdated Show resolved Hide resolved
@arm4b arm4b added this to In progress in StackStorm v3.8.1 via automation Nov 6, 2023
@arm4b arm4b added this to the 3.8.1 milestone Nov 6, 2023
@jk464
Copy link
Contributor Author

jk464 commented Nov 11, 2023

@armab As you can probably see from my commits I've hit a bit of a depedency hell trying to get requirement ranges that:

  • Allow the last supported py3.6
  • A version with relevant CVEs fixed in py3.8

I can see in #6063 you've hopefully got gitpython handled.

I'll probably do the same as you here and split this into bit size PRs to make it more manageable.

I did look at fixing fixate-requriements.py to support reading python_version which I actually got working (See 61b47db) (which is probably worth having, even after dropping py3.6 in 3.9.0) - issue I hit was the stackstorm/packagingbuild:bionic was missing the package that I wanted to using (packaging) so that image would also need bumped.

If you think its worth our time adding that support, I'll take a look at updating the image as well :)

@arm4b
Copy link
Member

arm4b commented Nov 11, 2023

@jk464 Your enhancement to fixate-requriements.py looks really clean. But overall I felt like env markers are buggy in many places, including older pip version we're locked to (because of py3.6) and even pants that doesn't support them in requirements-pants.txt so touching them might be like opening a can of worms.
I hope the builds would be migrated to pants from all the old machinery and so fixate-requriements.py may be obsolete by then.

@cognifloyd do you think it's doable to migrate to the pants builds in the upcoming v3.9.0?

arm4b
arm4b previously requested changes Nov 20, 2023
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

With the changes extracted into a dedicated PRs one dependency at a time, let's make sure this PR updates only the following:

requests[security] and importlib-metadata

I think that's the only thing left to update for security.

@pull-request-size pull-request-size bot removed the size/M PR that changes 30-99 lines. Good size to review. label Nov 24, 2023
@pull-request-size pull-request-size bot added the size/S PR that changes 10-29 lines. Very easy to review. label Nov 24, 2023
@pull-request-size pull-request-size bot added size/M PR that changes 30-99 lines. Good size to review. and removed size/S PR that changes 10-29 lines. Very easy to review. labels Nov 24, 2023
@arm4b arm4b changed the title Bump dependencies to fix CVEs Update requests and importlib-metadata to fix CVEs Nov 24, 2023
six==1.13.0
sseclient-py==1.7
typing-extensions<4.2
urllib3<2; python_version < '3.7'
Copy link
Member

@arm4b arm4b Nov 24, 2023

Choose a reason for hiding this comment

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

Yeah, environment markers are all buggy with the current pip version 🤯

Under py3.8 https://app.circleci.com/pipelines/github/StackStorm/st2/4267/workflows/6d50c81f-3524-4f8d-8279-71f47ee5dd67/jobs/16065?invite=true#step-109-925583_94:

[package: st2] [20:53:43]      The conflict is caused by:
[package: st2] [20:53:43]          requests[security] 2.31.0 depends on urllib3<3 and >=1.21.1
[package: st2] [20:53:43]          requests 2.31.0 depends on urllib3<3 and >=1.21.1
[package: st2] [20:53:43]          st2client 3.9.dev0 depends on urllib3<2

which is wrong.

I can't see any path forward for updating requests[security] so far as it requires urllib3 v2 which is incompatible with python3.6. And for some reason urllib3 v2 is pulled.

arm4b added a commit that referenced this pull request Nov 26, 2023
@arm4b
Copy link
Member

arm4b commented Nov 26, 2023

So at least importlib-metadata changes are extracted into a dedicated PR which could be merged ASAP: #6072

@jk464
Copy link
Contributor Author

jk464 commented Nov 27, 2023

@armab if the build is failing for requests and the update to importlib-metadata was merged in #6072 is there anything left to do in this PR?

The only CVE I see listed against requests is CVE-2023-32681 - which is only medium Sev - so I think we could just drop fixing that to 3.9.0 at which point we can(?) drop python 3.6 and bump this w/o issue. (And I assume for 3.9.0 we'd probably rather being bumping dependencies to the highest support by 3.8.)

Let me know what you think and I can close this PR if there's nothing further to do

@pull-request-size pull-request-size bot added size/S PR that changes 10-29 lines. Very easy to review. and removed size/M PR that changes 30-99 lines. Good size to review. labels Nov 27, 2023
@arm4b arm4b changed the title Update requests and importlib-metadata to fix CVEs Update requests to fix CVEs (security) Nov 27, 2023
@arm4b
Copy link
Member

arm4b commented Nov 27, 2023

@jk464 Yeah, let's reassign this PR to the v3.9.0 roadmap
It'll be a reminder that requests will need an update after dropping the py3.6.

@arm4b arm4b removed this from In progress in StackStorm v3.8.1 Nov 27, 2023
@arm4b arm4b modified the milestones: 3.8.1, 3.9.0 Nov 27, 2023
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

This is no longer needed as we have dropped py3.6 in the master branch, and we've already bumped requests to 2.31.0.

@cognifloyd cognifloyd closed this Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security size/S PR that changes 10-29 lines. Very easy to review.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants