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(cmd-api-server): use ncc bundle in container image - CVE-2024-29415 #3375

Conversation

petermetz
Copy link
Contributor

Fixes the CVE mentioned and also improves our response time to future CVEs
by a very wide margin. Details below.

  1. Fixing the mentioned vulnerability in the API server and doing so in
    a way so that in the future our dependency upgrades automatically propagate
    to the container builds as well.
  2. The way we are achieving this is by making the container image build
    use the pre-built bundle instead of pulling the package contents from npm.
  3. This has the advantage of breaking the chicken egg problem with releases
    to npm and container images, so from now on if we are adding a fix to the
    API server in the code, the built container image will automatically contain
    that fix when building on the CI for the pull request.
  4. This is also a new pattern for how to create our container images that
    has a couple more improvements to it:
    4.1. The .dockerignore file is now specific to the particular package's
    container image instead of the global one in the project root being used.
    This was needed because we are copying files from the ./dist/ folder of the
    package to the container image at build time but this was not possible while
    the root dir .dockerignore file was in effect because it blanket ignores
    the ./dist/ folders overall and so the image building was failing with errors
    that it couldn't locate the bundle (which is inside the ./dist/ directory)
    4.2. The healthcheck of the container is now 100% self-contained and needs
    no external dependencies of any kind (neither npm nor operating system level ones)
    This is beneficial because it reduces the attack-surface of the image and also
    reduce the size of the image by at least a 100 MB.
    4.3. With the introduction of the usage of the bundled version of the code
    we have dramatically reduced the image size overall. The image built from
    this revision of the code is 221 MB while the previous image versions were
    hovering closer to a 0.5 GB.
  5. Also updated the README of the package so that all the examples pertaining
    to the container image are now fully functional once again.
  6. Simplified the container image's definition: the custom docker entrypoint
    script and the healthcheck bash script are no longer necessary.
  7. Renamed the container image definition file from Dockerfile to
    cmd-api-server.Dockerfile because this is mandated by Docker when building
    images with custom .dockerignore files (it needs the custom filename to
    disambiguate the .dockerignore files based on it)
  8. Refactored how the CI executes the Trivy scan to reduce resource usage:
    8.1. There is no separate image build job now. This was necessary because
    with the new image definition we have to have the project compiled first
    (since we no longer install directly from npm) so it would've been a lot of
    duplicated compute time to recompile the project in yet another CI job for the
    image build.

The image built from this revision is also published on the official repository
with the canary tag of:
ghcr.io/hyperledger/cactus-cmd-api-server:2024-07-03T19-32-51-dev-3f5e97893

Signed-off-by: Peter Somogyvari [email protected]

Pull Request Requirements

  • Rebased onto upstream/main branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.
  • Have git sign off at the end of commit message to avoid being marked red. You can add -s flag when using git commit command. You may refer to this link for more information.
  • Follow the Commit Linting specification. You may refer to this link for more information.

Character Limit

  • Pull Request Title and Commit Subject must not exceed 72 characters (including spaces and special characters).
  • Commit Message per line must not exceed 80 characters (including spaces and special characters).

A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.

@petermetz petermetz enabled auto-merge (rebase) July 3, 2024 19:38
@petermetz petermetz requested a review from RafaelAPB July 3, 2024 19:38
@petermetz petermetz force-pushed the fix-cmd-api-server-cve-2024-29415 branch 3 times, most recently from 0d10229 to 1e3797d Compare July 3, 2024 22:12
Copy link
Contributor

@jagpreetsinghsasan jagpreetsinghsasan left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@RafaelAPB RafaelAPB left a comment

Choose a reason for hiding this comment

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

LGTM.
Why did we skip one fabric test in this PR?

Fixes the CVE mentioned and also improves our response time to future CVEs
by a very wide margin. Details below.

1. Fixing the mentioned vulnerability in the API server and doing so in
a way so that in the future our dependency upgrades automatically propagate
to the container builds as well.
2. The way we are achieving this is by making the container image build
use the pre-built bundle instead of pulling the package contents from npm.
3. This has the advantage of breaking the chicken egg problem with releases
to npm and container images, so from now on if we are adding a fix to the
API server in the code, the built container image will automatically contain
that fix when building on the CI for the pull request.
4. This is also a new pattern for how to create our container images that
has a couple more improvements to it:
4.1. The .dockerignore file is now specific to the particular package's
container image instead of the global one in the project root being used.
This was needed because we are copying files from the ./dist/ folder of the
package to the container image at build time but this was not possible while
the root dir .dockerignore file was in effect because it blanket ignores
the ./dist/ folders overall and so the image building was failing with errors
that it couldn't locate the bundle (which is inside the ./dist/ directory)
4.2. The healthcheck of the container is now 100% self-contained and needs
no external dependencies of any kind (neither npm nor operating system level ones)
This is beneficial because it reduces the attack-surface of the image and also
reduce the size of the image by at least a 100 MB.
4.3. With the introduction of the usage of the bundled version of the code
we have **dramatically** reduced the image size overall. The image built from
this revision of the code is 221 MB while the previous image versions were
hovering closer to a 0.5 GB.
5. Also updated the README of the package so that all the examples pertaining
to the container image are now fully functional once again.
6. Simplified the container image's definition: the custom docker entrypoint
script and the healthcheck bash script are no longer necessary.
7. Renamed the container image definition file from `Dockerfile` to
`cmd-api-server.Dockerfile` because this is mandated by Docker when building
images with custom .dockerignore files (it needs the custom filename to
disambiguate the .dockerignore files based on it)
8. Refactored how the CI executes the Trivy scan to reduce resource usage:
8.1. There is no separate image build job now. This was necessary because
with the new image definition we have to have the project compiled first
(since we no longer install directly from npm) so it would've been a lot of
duplicated compute time to recompile the project in yet another CI job for the
image build.

The image built from this revision is also published on the official repository
with the canary tag of:
`ghcr.io/hyperledger/cactus-cmd-api-server:2024-07-03T19-32-51-dev-3f5e97893`

Signed-off-by: Peter Somogyvari <[email protected]>
@petermetz petermetz force-pushed the fix-cmd-api-server-cve-2024-29415 branch from 1e3797d to 1802a84 Compare July 4, 2024 15:20
@petermetz petermetz merged commit 9eefa66 into hyperledger-cacti:main Jul 4, 2024
147 of 150 checks passed
@petermetz petermetz deleted the fix-cmd-api-server-cve-2024-29415 branch July 7, 2024 23:42
@petermetz
Copy link
Contributor Author

LGTM. Why did we skip one fabric test in this PR?

@RafaelAPB Sorry for the slow response! That test became broken due to it installing the latest version from the official npm repositories (which for v2.0.0-rc.1 was broken). So I figured for now it's pointless to have the test and always fail, but now that rc.2 is out I'm hoping when I try to reenable it will be good.

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