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

build: FORMS-1799 node update for security fixes #1599

Merged
merged 4 commits into from
Feb 5, 2025

Conversation

WalterMoar
Copy link
Collaborator

@WalterMoar WalterMoar commented Feb 4, 2025

Description

The Red Hat Advanced Cluster Security (ACS) application has identified the image node as having vulnerabilities that are fixable. To satisfy the requirements outlined in the Security Threat and Risk Assessment's (STRA) Statement of Acceptable Risks (SoAR), this image must be updated to resolve fixable vulnerabilities (or mitigated in some other way, if updating the image is not possible).

Acceptance Criteria

  • We are using the most recent version of Node
  • Node is updated both for the devcontainer and the deployed runtime

Type of Change

build (change in build system or dependencies)

Checklist

  • I have read the CONTRIBUTING doc
  • I have checked that unit tests pass locally with my changes
  • I have run the npm script lint on the frontend and backend
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • I have approval from the product owner for the contribution in this pull request

Further comments

  • Removed setting the VARIANT value so that we have fewer places to update
  • Addressed a Sonar complaint about packages not being in alphabetical order
  • Addressed a Sonar complaint about apt-get clean not being run
  • Addressed a Sonar complaint about RUN commands being able to be combined

This comment has been minimized.

"context": "..",
"args": {
"VARIANT": "20.18.1-bookworm"
}
"context": ".."
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While we could just update the value of this VARIANT argument and leave the Dockerfile as-is, I think it will be less confusing for future updates if we just remove this and update the Dockerfile value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good. Fine with me.

Comment on lines +10 to +20
libasound2 \
libgbm-dev \
libgtk-3-0 \
libgtk2.0-0 \
libnotify-dev \
libnss3 \
libxss1 \
libxtst6 \
vim \
xauth \
xvfb \
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sonar wants the new packages to be in alphabetical order. While I'm usually 100% onboard with alphabetizing things, this does make it a bit more effort to check that we have all the Cypress prerequisites (in case those change).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is not a production image we could just ignore it in Sonar. Up to you... but I would be ok with ignoring this file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe it's good to follow the Sonar recommendation - I don't think we'll be checking the Cypress prerequisites that often.

vim \
xauth \
xvfb \
&& apt-get clean
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sonar recommended the clean step to help reduce image size.

Comment on lines -14 to -16
RUN mkdir /.npm
RUN chgrp -R 0 /.npm && \
chmod -R g=u /.npm
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sonar recommended combining RUN steps so that there are fewer layers in the image.

Copy link
Collaborator

@jasonchung1871 jasonchung1871 left a comment

Choose a reason for hiding this comment

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

Looks clean!

@WalterMoar WalterMoar force-pushed the build/1799-node-update branch from c6c05fb to d6ba595 Compare February 5, 2025 17:42
Update the node image to address security vulnerabilities reported by RedHat Advanced Cluster Security.
While the VARIANT argument is useful in some situations, there's the possibility that the Dockerfile is updated with a newer image version but the argument in the devcontainer.json overrides it. Remove it and just use what's in the Dockerfile.
Sonar was complaining that the installed packages were not in alphabetical order. It was also complaining that the install wasn't cleaned afterwards.
Sonar was complaining that the multiple RUN commands should be combined. Good point!
Copy link

sonarqubecloud bot commented Feb 5, 2025

Copy link
Collaborator

@usingtechnology usingtechnology left a comment

Choose a reason for hiding this comment

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

LGTM, certainly doesn't hurt to follow some standards!
Caveat, I didn't try it out as I have too many local changes in flight.

@WalterMoar WalterMoar merged commit 6146642 into bcgov:main Feb 5, 2025
7 checks passed
@WalterMoar WalterMoar deleted the build/1799-node-update branch February 5, 2025 19:10
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.

3 participants