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

(PA-6881) Adding rexml gem to agent-runtime-main for CVE-2024-41123 and CVE-2024-41946 #901

Merged
merged 1 commit into from
Aug 30, 2024

Conversation

@imaqsood imaqsood marked this pull request as ready for review August 23, 2024 09:47
@imaqsood imaqsood requested review from a team as code owners August 23, 2024 09:47
@cthorn42
Copy link
Collaborator

Is it expected that are leaving the older version of Rexml installed? From using the EL-7 artifact you built

/opt/puppetlabs/puppet/bin/gem list | grep rexml
rexml (3.3.4, 3.3.2)

@mhashizume
Copy link
Contributor

We're already including REXML in agent-runtime-7.x as well:

# We add rexml explicitly in here because even though ruby 2 ships with rexml as its default gem, the version
# of rexml it ships with contains CVE-2024-35176 and CVE-2024-39908. So, we add it here to update to a higher version
# free from the CVEs.
proj.component 'rubygem-rexml'

Would it make more sense to include REXML in _shared-agent-components than each agent runtime separately?

@imaqsood
Copy link
Contributor Author

imaqsood commented Aug 26, 2024

Is it expected that are leaving the older version of Rexml installed? From using the EL-7 artifact you built

/opt/puppetlabs/puppet/bin/gem list | grep rexml
rexml (3.3.4, 3.3.2)

I didn't notice any changes in 7.x to remove the older REXML version that comes as the default, so I believe it should be fine with the main version as well.

@mhashizume
Copy link
Contributor

mhashizume commented Aug 26, 2024

I didn't notice any changes in 7.x to remove the older REXML version that comes as the default, so I believe it should be fine with the main version as well.

I believe that we do not do anything in 7.x to clean up older REXML gems because REXML is packaged differently in different Ruby versions; it's a default gem in Ruby 2.7, but became a bundled gem in Ruby >= 3.1.

Having multiple versions of REXML available has caused issues in CI in the past, see this thread in our private Slack channel: https://perforce.slack.com/archives/G047N5B7KK5/p1721074588851319.

It looks like @shubhamshinde360 worked on this last time and may have guidance on how to proceed.

@shubhamshinde360
Copy link
Contributor

@imaqsood,
I think we could do something like this: https://github.com/puppetlabs/puppet-runtime/pull/878/files#diff-73a9ffb9a7e1fd33926c952ab5c2592afcff624d71bb8b2f4a99dfac80ac6076R51-R57 but also address Cas' comment. This will run gem clean rexml command and remove the older of the rexml version.

@mhashizume
Copy link
Contributor

I see that you've updated your PR, could you generated updated artifacts? I looked at the Vanagon generic builder and didn't see anything.

@imaqsood
Copy link
Contributor Author

I see that you've updated your PR, could you generated updated artifacts? I looked at the Vanagon generic builder and didn't see anything.
I have this passing vanagon runtime build
https://jenkins-platform.delivery.puppetlabs.net/view/vanagon-generic-builder/job/platform_vanagon-generic-builder_vanagon-packaging_generic-builder/3207/console

Copy link
Collaborator

@cthorn42 cthorn42 left a comment

Choose a reason for hiding this comment

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

Looks good, built an el-7-x86_64 agent-runtime-7.x and then the puppet-agent out of that. Installed the agent and then ran this to confirm that the 3.3.4 rexml gem is updated for the agent:

/opt/puppetlabs/puppet/bin/gem list rexml

*** LOCAL GEMS ***

rexml (3.3.4, default: 3.2.3.1)

@cthorn42 cthorn42 merged commit 3224edb into puppetlabs:master Aug 30, 2024
3 checks passed
donoghuc added a commit to donoghuc/puppet-runtime that referenced this pull request Sep 6, 2024
With the addition logic in puppetlabs#901 for deduplicating rexml gems an inadvertant requirement was imposed on projects to define a `gem_uninstall` command. This command should be ubiquitous (especially with impending ruby 3 only streams)  so instead of requiring all projects to configure it a default is added. This setting is still configurable at a project level, but is not required.
@donoghuc
Copy link
Contributor

donoghuc commented Sep 6, 2024

This appears to have resulted in a new "required" configuration for all projects. I dont believe we should make requirements like that as it encourages even more copy paste here. I propose we add a default as i believe it should be a pretty stable command. #910

In the future @imaqsood can you please check this view https://jenkins-platform.delivery.puppetlabs.net/view/puppet-runtime/ when you are making changes that affect everybodies runtimes to make sure there are not widespread failures?

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.

5 participants