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

Bump PyYAML's version from 5.3.1 to avoid security risks #36119

Closed
wants to merge 4 commits into from

Conversation

shmsr
Copy link
Member

@shmsr shmsr commented Jul 19, 2023

What does this PR do?

CPython 3.0 was released recently which has introduced a regression that leads to failures when installing PyYAML (and perhaps other packages too). This is a temporary fix and this commits needs to be reverted when a proper fix is available. See: yaml/pyyaml#601

#36091 pinned the version of PyYAML to 5.3.1 but I found from a recent discussion yaml/pyyaml#601 (comment) that there's a CVE associated with the same: CVE-2020-14343.

Also, see: aws/aws-cli#8036 (comment) and aws/aws-cli#8036 (comment)

But currently we are facing an issue because of docker-compose i.e., #36119 (comment)
Bumping to 6.x.x for PyYAML results in dependency conflict but as we know that we cannot go with 5.4.1 as that will again result in CI failure because CPython latest is used instead of CPython<3

Why is it important?

Fixes CI issues temporarily and also makes us safe from CVE-2020-14343.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Related issues

@shmsr shmsr added the bug label Jul 19, 2023
@shmsr shmsr requested review from a team as code owners July 19, 2023 21:54
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jul 19, 2023
@botelastic
Copy link

botelastic bot commented Jul 19, 2023

This pull request doesn't have a Team:<team> label.

@mergify mergify bot assigned shmsr Jul 19, 2023
@mergify
Copy link
Contributor

mergify bot commented Jul 19, 2023

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @shmsr? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

@shmsr shmsr changed the title Pin PyYAML version to 6.0.1 to avoid CI errors temporarily Bump PyYAML's version from 5.3.1 to 6.0.1 to avoid security risks Jul 19, 2023
@shmsr shmsr force-pushed the fix-ci-py-issue-security-risk branch from 5268a06 to 31550a5 Compare July 19, 2023 22:03
@shmsr shmsr changed the title Bump PyYAML's version from 5.3.1 to 6.0.1 to avoid security risks Bump PyYAML's version from 5.3.1 to >=6.0.0 to avoid security risks Jul 19, 2023
@shmsr
Copy link
Member Author

shmsr commented Jul 19, 2023

Okay this is not going to be easy to fix because:

The conflict is caused by:
The user requested PyYAML>=6.0.0
docker-compose 1.29.2 depends on PyYAML<6 and >=3.10

For docker-compose, the latest Py release was 1.29.2 and after that they deprecated it. So, we have to use PyYAML<6

@elasticmachine
Copy link
Collaborator

elasticmachine commented Jul 19, 2023

💔 Build Failed

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-07-19T22:32:07.636+0000

  • Duration: 39 min 59 sec

Test stats 🧪

Test Results
Failed 0
Passed 24968
Skipped 1206
Total 26174

Steps errors 55

Expand to view the steps failures

Show only the first 10 steps failures

x-pack/metricbeat-unitTest - mage build unitTest
  • Took 1 min 4 sec . View more details here
  • Description: mage build unitTest
x-pack/metricbeat-goIntegTest - mage goIntegTest
  • Took 5 min 38 sec . View more details here
  • Description: mage goIntegTest
x-pack/metricbeat-goIntegTest - mage goIntegTest
  • Took 4 min 51 sec . View more details here
  • Description: mage goIntegTest
x-pack/metricbeat-goIntegTest - mage goIntegTest
  • Took 4 min 51 sec . View more details here
  • Description: mage goIntegTest
x-pack/metricbeat-pythonIntegTest - mage pythonIntegTest
  • Took 2 min 36 sec . View more details here
  • Description: mage pythonIntegTest
x-pack/metricbeat-pythonIntegTest - mage pythonIntegTest
  • Took 0 min 21 sec . View more details here
  • Description: mage pythonIntegTest
x-pack/metricbeat-pythonIntegTest - mage pythonIntegTest
  • Took 0 min 21 sec . View more details here
  • Description: mage pythonIntegTest
x-pack/osquerybeat-unitTest - mage build unitTest
  • Took 3 min 56 sec . View more details here
  • Description: mage build unitTest
x-pack/packetbeat-unitTest - mage build unitTest
  • Took 3 min 1 sec . View more details here
  • Description: mage build unitTest
Error signal
  • Took 0 min 0 sec . View more details here
  • Description: Error 'hudson.AbortException: script returned exit code 1'

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@shmsr shmsr changed the title Bump PyYAML's version from 5.3.1 to >=6.0.0 to avoid security risks Bump PyYAML's version from 5.3.1 to avoid security risks Jul 19, 2023
@gizas
Copy link
Contributor

gizas commented Jul 20, 2023

I am not quite sure how both work as bumping to 5.3.1 and setting in the next file >=6.0.0 seems conflicting to me. I am approving because changes only involve the versions

@shmsr shmsr marked this pull request as draft July 20, 2023 07:18
@shmsr
Copy link
Member Author

shmsr commented Jul 20, 2023

I am not quite sure how both work as bumping to 5.3.1 and setting in the next file >=6.0.0 seems conflicting to me. I am approving because changes only involve the versions

No @gizas. It is not working. I've mentioned the issue I'm currently facing in the PR description. docker-compose requires PyYAML<6 so we cannot use PyYAML 6.0.1 (that was released recently with Cython<3 to avoid the regression introduced in Cython 3) as we would get dependency conflicts. The only place where we can use PyYAML 6.0.1 is where we are not using docker-compose as a dependency. Now, we cannot even use PyYAML 5.4.1 as it uses Cython latest and that's why we previously pinned the PyYAML version to 5.3.1 to avoid the CI issues but it seems that version is associated with a CVE.

Moving this PR to draft as the fix is not ready. We have to see how to fix it. Please take a look at the linked issues in the PR description as well.

Suggestions are welcomed. One ugly fix I saw in PyYAML's discussion is this: yaml/pyyaml#702 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug needs_team Indicates that the issue/PR needs a Team:* label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants