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

feat: self-contained container image build #3544

Merged
merged 1 commit into from
Feb 14, 2025

Conversation

COLDTURNIP
Copy link
Contributor

Which issue(s) this PR fixes:

Supports security build.

What this PR does / why we need it:

  • Merge binary building process into container build as a separate building stage.
  • Prepare for potential secure release

Special notes for your reviewer:

The secrets provided by EIO MUST NOT present in any visible output.

Additional documentation or context

Guidance on achieving SLSA Level 3

Copy link

coderabbitai bot commented Feb 7, 2025

Walkthrough

This pull request enhances the build and packaging processes by modifying the GitHub Actions workflow to incorporate dynamic versioning outputs and streamline Docker image publishing steps. It updates the Makefile to alter build configuration variables and define new phony targets, while the Dockerfile implements a multi-stage build. The packaging scripts are revised to dynamically generate image variables and conditionally execute packaging steps. Additional minor updates include changes to auxiliary configuration files like .gitignore and Dockerfile.dapper.

Changes

File(s) Change Summary
.github/workflows/build.yml Added job outputs (version_major, version_minor, version_patch, image_tag), introduced a build_info step for version extraction, modified the Run ci step to use NO_PACKAGE=true make ci, and replaced redundant image build steps with a Build and publish image step.
Makefile Updated MACHINE variable from rancher to longhorn, added DEFAULT_PLATFORMS (linux/amd64,linux/arm64), and introduced new phony targets (buildx-machine, workflow-image-build-push, workflow-image-build-push-secure).
package/Dockerfile Implemented a multi-stage build by introducing a builder stage using a Go environment and updated binary copy commands in the release stage to source from the builder.
scripts/package, scripts/ci Refactored build scripts to dynamically construct the IMAGE variable using IMAGE_NAME, added variables (PUSH, IS_SECURE, etc.), replaced static binary checks, and conditionally executed the packaging step based on the NO_PACKAGE variable (with a variable rename from image to IMAGE).
.gitignore, Dockerfile.dapper Added coverage.out to .gitignore and updated DAPPER_ENV in Dockerfile.dapper to include NO_PACKAGE.

Sequence Diagram(s)

sequenceDiagram
    participant Trigger as CI Trigger
    participant Build as Build Job
    participant BuildInfo as build_info Step
    participant RunCI as Run ci Step
    participant Publish as Build & Publish Step

    Trigger->>Build: Initiate build job
    Build->>BuildInfo: Execute build_info (extract version info)
    BuildInfo-->>Build: Return version outputs
    Build->>RunCI: Execute "NO_PACKAGE=true make ci"
    RunCI-->>Build: Complete CI step
    Build->>Publish: Pass version outputs to build_push_image job
    Publish->>Publish: Build and publish Docker image via make command
Loading

Possibly related PRs

  • fix: don't build longhorn-manager twice before packaging #3226: The changes in the main PR, which introduce a new output mechanism and modify the build process in the GitHub Actions workflow, are related to the retrieved PR as both involve modifications to the scripts/package file, specifically in the context of building the longhorn-manager binary.

Suggested reviewers

  • FrankYang0529
  • ChanYiLin
  • c3y1huang
  • innobead
  • mantissahz

📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9843826 and 63e38a4.

📒 Files selected for processing (8)
  • .github/workflows/build.yml (2 hunks)
  • .gitignore (1 hunks)
  • Dockerfile.dapper (1 hunks)
  • Makefile (2 hunks)
  • package/Dockerfile (2 hunks)
  • scripts/build (1 hunks)
  • scripts/ci (1 hunks)
  • scripts/package (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • .gitignore
  • scripts/ci
  • Dockerfile.dapper
  • Makefile
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Build binaries
  • GitHub Check: Summary
🔇 Additional comments (1)
scripts/build (1)

23-41: LGTM! Architecture handling is secure and robust.

The implementation safely handles architecture detection and build configuration.


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (5)
scripts/package (3)

10-19: Add usage documentation for newly introduced environment variables.
Defining environment variables (e.g., REPO, IMAGE_NAME, TAG, etc.) is useful, but it’s helpful to document their meaning and any dependencies in a README or usage comments.


26-27: Confirm space-splitting behavior for IID_FILE_FLAG.
You’re splitting IID_FILE_FLAG by spaces. If the variable includes unexpected spacing, the subsequent parameters might break. Double-check this is intended.


32-36: Use a more robust approach to parse/update base images.
Grepping for “FROM.*/” may fail if lines are split or contain unusual syntax. Consider a Docker parser or a more structured approach to update base images.

scripts/ci-workflow (1)

1-8: Enhance test coverage reporting and checks.
Running “./test -cover” is good, but consider storing coverage reports and verifying coverage thresholds. This would strengthen CI validation.

package/Dockerfile (1)

2-13: Multi-stage build approach is commendable.
Separating the build environment from the runtime environment helps reduce final image size and improves security. However, consider adding a .dockerignore to limit context size if large directories are not needed for the build.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cd474c1 and 5b4a39e.

📒 Files selected for processing (5)
  • .github/workflows/build.yml (1 hunks)
  • Makefile (2 hunks)
  • package/Dockerfile (2 hunks)
  • scripts/ci-workflow (1 hunks)
  • scripts/package (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Build binaries
  • GitHub Check: Summary
🔇 Additional comments (14)
scripts/package (6)

8-8: Ensure Docker version compatibility for the buildx fallback.
By falling back to "docker buildx" if "buildx" is not found in PATH, older Docker installations that lack the buildx subcommand might break.

Please confirm that your environment always provides a Docker version (19.03 or later) supporting "docker buildx".


21-21: Validate or default the final image name to avoid invalid references.
If REPO, IMAGE_NAME, or TAG is unset or empty, this might produce an invalid image reference. Consider handling that scenario explicitly.


23-24: Confirm “MACHINE” alignment with a valid buildx builder name.
Including “--builder” relies on $MACHINE matching an existing builder instance. Inconsistent naming could cause the build command to fail.


29-30: Verify buildx version support for provenance flags.
The flags “--sbom=true” and “--attest type=provenance” require a modern buildx version. Older versions may not recognize them.


49-49: Informational log is fine.
This “Built $image” log helps track the final image tag. No issues here.


51-52: Good practice: Recording the last built image reference.
Persisting the final image name in “./bin/latest_image” is helpful for subsequent scripts or CI steps.

package/Dockerfile (1)

31-33: Copying final artifacts from builder stage is aligned with best practices.
Storing binaries in /app/bin and copying them by architecture helps support multi-arch builds effectively. This approach looks correct.

Makefile (3)

2-6: New Variable Declarations for Buildx Configuration
The addition of the MACHINE variable (set to "rancher") and the DEFAULT_PLATFORMS variable (listing target platforms) clearly configures the Docker Buildx environment. The accompanying comments effectively explain that TARGET_PLATFORMS used in specific projects must be a subset of these defaults.


18-22: Buildx-Machine Target Implementation
The buildx-machine target is well implemented. It checks for an existing Docker Buildx instance identified by $(MACHINE) and creates one using the predefined $(DEFAULT_PLATFORMS) if it’s not found. This approach is concise and aligns with best practices for multi-platform builds.


28-33: Workflow Image Build and Push Targets
The new phony targets workflow-image-build-push and workflow-image-build-push-secure extend the Makefile’s capabilities by invoking the build process script with environment variables. This modular design supports a more dynamic and secure image build process, in line with the PR objectives.

.github/workflows/build.yml (4)

14-18: GitHub Actions Build Outputs Declaration
The addition of outputs (version_major, version_minor, version_build, image_tag) in the build job is well structured. Mapping these outputs directly from the build_info step ensures that subsequent jobs can consume the versioning and tagging information reliably.


24-57: Build Info Step for Version Extraction
The build_info step effectively extracts version components using standard shell utilities (sed, cut) based on the ref type (tag, branch, or pull request). Consider adding error checking or fallback handling in case the version string does not conform to the expected pattern (e.g., missing or extra delimiters) to avoid producing empty outputs.


59-60: CI Workflow Command Update
Switching the command from "make ci" to "make ci-workflow" indicates integration with an updated CI process. This change appears intentional to incorporate more robust testing and validation routines, which supports the overall security improvements of the build process.


84-88: Build and Publish Image Step in Push Job
The "Build and publish image" step uses environment variables (REPO and TAG) from the output of the build job to trigger the Makefile target for image building. This aligns neatly with the new Makefile targets. Additionally, ensure that no sensitive data (like secrets) are inadvertently exposed during the process, as emphasized by the PR objectives.

scripts/package Outdated Show resolved Hide resolved
@COLDTURNIP COLDTURNIP force-pushed the feat-secure_build branch 2 times, most recently from b9febb7 to 6124bf8 Compare February 7, 2025 09:26
@COLDTURNIP COLDTURNIP force-pushed the feat-secure_build branch 6 times, most recently from 1f1e5b8 to 4cbf0d7 Compare February 12, 2025 02:38
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🔭 Outside diff range comments (1)
package/Dockerfile (1)

25-29: ⚠️ Potential issue

Pin package versions for reproducible builds.

Using zypper update -y and installing packages without version constraints could lead to inconsistent builds and potential security vulnerabilities.

-RUN zypper -n ref && \
-    zypper update -y
-
-RUN zypper -n install iputils iproute2 nfs-client cifs-utils bind-utils e2fsprogs xfsprogs zip unzip kmod && \
+RUN zypper -n ref && \
+    zypper -n install \
+    iputils=20211215-150600.3.6.1 \
+    iproute2=5.15-150600.3.3.1 \
+    nfs-client=2.6.2-150600.3.6.1 \
+    cifs-utils=6.15-150600.3.3.1 \
+    bind-utils=9.16.27-150600.3.12.1 \
+    e2fsprogs=1.46.5-150600.3.3.1 \
+    xfsprogs=5.14.2-150600.3.3.1 \
+    zip=3.0-150600.3.3.1 \
+    unzip=6.0-150600.3.3.1 \
+    kmod=29-150600.3.3.1 && \
     rm -rf /var/cache/zypp/*
♻️ Duplicate comments (1)
scripts/package (1)

45-53: ⚠️ Potential issue

Avoid echoing the full build command to prevent secret leakage.

If environment variables or additional flags contain secrets, printing them could leak sensitive data in logs, conflicting with the PR's confidentiality requirement.

-echo "${BUILD_CMD[@]}" "${IMAGE_BUILD_CMD_ARGS[@]}"
+"${BUILD_CMD[@]}" "${IMAGE_BUILD_CMD_ARGS[@]}"
🧹 Nitpick comments (5)
scripts/package (3)

8-8: Enhance buildx command detection.

The current check could be more robust by verifying both docker buildx and standalone buildx commands.

-command -v buildx >/dev/null && BUILD_CMD=(buildx) || BUILD_CMD=(docker buildx)
+if command -v buildx >/dev/null 2>&1; then
+  BUILD_CMD=(buildx)
+elif docker buildx version >/dev/null 2>&1; then
+  BUILD_CMD=(docker buildx)
+else
+  echo "Error: buildx not found. Please install Docker buildx." >&2
+  exit 1
+fi

10-19: Add input validation for critical parameters.

Consider validating the input parameters, especially for security-sensitive variables like IS_SECURE.

 # read configurable parameters
 REPO=${REPO:-longhornio}
 IMAGE_NAME=${IMAGE_NAME:-'longhorn-manager'}
 TAG=${TAG:-${IMAGE_TAG_PREFIX}}
 PUSH=${PUSH:-'false'}
 IS_SECURE=${IS_SECURE:-'false'}
 MACHINE=${MACHINE:-''}
 TARGET_PLATFORMS=${TARGET_PLATFORMS:-''}
 IID_FILE=${IID_FILE:-''}
 IID_FILE_FLAG=${IID_FILE_FLAG:-''}
+
+# Validate boolean parameters
+if [[ "${PUSH}" != 'true' && "${PUSH}" != 'false' ]]; then
+  echo "Error: PUSH must be 'true' or 'false'" >&2
+  exit 1
+fi
+if [[ "${IS_SECURE}" != 'true' && "${IS_SECURE}" != 'false' ]]; then
+  echo "Error: IS_SECURE must be 'true' or 'false'" >&2
+  exit 1
+fi

57-58: Improve error handling for directory creation.

The current approach using || true silently ignores failures. Consider explicit error handling.

-mkdir ./bin || true
+if ! mkdir -p ./bin; then
+  echo "Error: Failed to create bin directory" >&2
+  exit 1
+fi
 echo "${IMAGE}" > ./bin/latest_image
Makefile (1)

18-21: Improve error handling in buildx-machine target.

Suppressing errors with 2>/dev/null || true could mask legitimate issues.

 .PHONY: buildx-machine
 buildx-machine:
-	@docker buildx create --name=$(MACHINE) --platform=$(DEFAULT_PLATFORMS) 2>/dev/null || true
+	@if ! docker buildx inspect $(MACHINE) >/dev/null 2>&1; then \
+		docker buildx create --name=$(MACHINE) --platform=$(DEFAULT_PLATFORMS); \
+	fi
 	docker buildx inspect $(MACHINE)
.github/workflows/build.yml (1)

24-55: Enhance version parsing robustness.

The version parsing logic could fail silently for malformed tags.

         if [[ "$ref" =~ 'refs/tags/' ]]; then
-          version=$(sed -E 's/^v([0-9]*\.[0-9]*\.[0-9]*).*$/\1/' <<<${{ github.ref_name }} )
+          if [[ ! "${{ github.ref_name }}" =~ ^v[0-9]+\.[0-9]+\.[0-9]+.*$ ]]; then
+            echo "Error: Invalid version tag format: ${{ github.ref_name }}" >&2
+            exit 1
+          fi
+          version=$(sed -E 's/^v([0-9]+\.[0-9]+\.[0-9]+).*$/\1/' <<<${{ github.ref_name }} )
           version_major=$(cut -d. -f1 <<<$version)
           version_minor=$(cut -d. -f2 <<<$version)
           version_patch=$(cut -d. -f3 <<<$version)
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5b4a39e and 01ef144.

📒 Files selected for processing (6)
  • .github/workflows/build.yml (2 hunks)
  • .gitignore (1 hunks)
  • Makefile (2 hunks)
  • package/Dockerfile (2 hunks)
  • scripts/ci-workflow (1 hunks)
  • scripts/package (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
  • scripts/ci-workflow
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Build binaries
  • GitHub Check: Summary

package/Dockerfile Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

♻️ Duplicate comments (2)
scripts/package (1)

52-52: ⚠️ Potential issue

Avoid echoing the full build command to prevent secret leakage.

If environment variables or additional flags contain secrets, printing them could leak sensitive data in logs, conflicting with the PR's confidentiality requirement.

-echo "${BUILD_CMD[@]}" "${IMAGE_BUILD_CMD_ARGS[@]}"
package/Dockerfile (1)

6-7: ⚠️ Potential issue

Security Concern: Inadvertent Exposure of Sensitive Files in Build Context

The execution confirmed that a .dockerignore file is missing, which means the entire application context is being copied during the Docker image build. This could potentially expose sensitive files.

🧹 Nitpick comments (3)
scripts/package (3)

8-8: Add docker command verification.

The script checks for buildx but should also verify if docker is installed.

+command -v docker >/dev/null || { echo "Error: docker is not installed"; exit 1; }
 command -v buildx >/dev/null && BUILD_CMD=(buildx) || BUILD_CMD=(docker buildx)

11-19: Add validation for boolean parameters.

The script should validate boolean parameters to prevent unexpected behavior.

 PUSH=${PUSH:-'false'}
+[[ "${PUSH}" != 'true' && "${PUSH}" != 'false' ]] && { echo "Error: PUSH must be 'true' or 'false'"; exit 1; }
 IS_SECURE=${IS_SECURE:-'false'}
+[[ "${IS_SECURE}" != 'true' && "${IS_SECURE}" != 'false' ]] && { echo "Error: IS_SECURE must be 'true' or 'false'"; exit 1; }

57-58: Use atomic file operations for latest_image.

The current approach to writing latest_image is not atomic and could lead to race conditions if multiple builds run concurrently.

 mkdir ./bin || true
-echo "${IMAGE}" > ./bin/latest_image
+echo "${IMAGE}" > ./bin/latest_image.tmp && mv ./bin/latest_image.tmp ./bin/latest_image
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 01ef144 and d40d590.

📒 Files selected for processing (6)
  • .github/workflows/build.yml (2 hunks)
  • .gitignore (1 hunks)
  • Makefile (2 hunks)
  • package/Dockerfile (2 hunks)
  • scripts/ci-workflow (1 hunks)
  • scripts/package (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • .gitignore
  • scripts/ci-workflow
  • Makefile
  • .github/workflows/build.yml
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Build binaries
  • GitHub Check: Summary

scripts/package Show resolved Hide resolved
package/Dockerfile Show resolved Hide resolved
Comment on lines 1 to 8
#!/bin/bash
set -e

cd $(dirname $0)

./build
./validate
./test -cover
Copy link
Member

Choose a reason for hiding this comment

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

Can we reuse the scripts/ci and add a new parameter like workflow?
If workflow is true, then no need to run package.

Copy link
Member

Choose a reason for hiding this comment

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

@COLDTURNIP Let's remove this file. Thank you.

@COLDTURNIP COLDTURNIP force-pushed the feat-secure_build branch 3 times, most recently from 3789d0f to 7def6f7 Compare February 13, 2025 12:58
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (2)
scripts/package (1)

53-54: ⚠️ Potential issue

Avoid echoing the full build command to prevent secret leakage.

If environment variables or additional flags contain secrets, printing them could leak sensitive data in logs, conflicting with the PR's confidentiality requirement.

-echo "${BUILD_CMD[@]}" "${IMAGE_BUILD_CMD_ARGS[@]}"
"${BUILD_CMD[@]}" "${IMAGE_BUILD_CMD_ARGS[@]}"
package/Dockerfile (1)

2-2: ⚠️ Potential issue

Pin base images to specific digests.

Using floating tags for base images could lead to supply chain attacks. Pin both images to specific digests.

-FROM registry.suse.com/bci/golang:1.23 AS builder
+FROM registry.suse.com/bci/golang:1.23@sha256:<digest> AS builder

-FROM registry.suse.com/bci/bci-base:15.6 AS release
+FROM registry.suse.com/bci/bci-base:15.6@sha256:<digest> AS release

Also applies to: 15-15

🧹 Nitpick comments (1)
Dockerfile.dapper (1)

12-12: Review privileged container usage for security compliance.

The --privileged flag grants extensive host access which could:

  1. Expose sensitive information
  2. Conflict with SLSA Level 3 security requirements
  3. Create potential security vulnerabilities

Consider:

  • Limiting container privileges using --cap-add for specific capabilities instead of --privileged
  • Documenting why privileged access is required
  • Evaluating if this aligns with your security objectives
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d40d590 and 7def6f7.

📒 Files selected for processing (8)
  • .github/workflows/build.yml (1 hunks)
  • .gitignore (1 hunks)
  • Dockerfile.dapper (1 hunks)
  • Makefile (2 hunks)
  • package/Dockerfile (2 hunks)
  • scripts/ci (1 hunks)
  • scripts/ci-workflow (1 hunks)
  • scripts/package (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • .gitignore
  • scripts/ci-workflow
  • .github/workflows/build.yml
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Build binaries
  • GitHub Check: Summary
🔇 Additional comments (5)
scripts/ci (1)

10-17: LGTM! Clean and effective changes.

The conditional packaging step and variable renaming improve the script's flexibility and follow shell script conventions.

scripts/package (1)

37-38: LGTM! Secure build configuration looks good.

The secure build options with SBOM generation and provenance attestation align well with SLSA Level 3 requirements.

package/Dockerfile (1)

2-14: LGTM! Multi-stage build improves security.

The multi-stage build reduces the attack surface by separating build dependencies from the runtime image.

Makefile (1)

18-32: LGTM! Clean implementation of secure build targets.

The buildx configuration and secure build targets are well-structured and support the security requirements effectively.

Dockerfile.dapper (1)

11-11:

❓ Verification inconclusive

LGTM! Verify the NO_PACKAGE variable usage.

The addition of NO_PACKAGE to DAPPER_ENV aligns with the PR's security objectives. This variable appears to be used to control package generation during builds.

Let's verify how this variable is used in the codebase:


🏁 Script executed:

#!/bin/bash
# Search for NO_PACKAGE usage to understand its security implications
rg "NO_PACKAGE" -A 3

Length of output: 1595


Security Verification: Assess NO_PACKAGE and Privileged Usage

  • The inclusion of NO_PACKAGE in DAPPER_ENV is correctly leveraged in the CI script (in scripts/ci) to control package generation, aligning with the intended security improvements.
  • However, note that the Dockerfile still sets the container to run in privileged mode (--privileged in DAPPER_RUN_ARGS on line 12), which poses potential risks and could undermine SLSA Level 3 compliance. Please confirm that this privileged configuration is strictly necessary and that any additional mitigations are in place.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7def6f7 and 6c774f0.

📒 Files selected for processing (1)
  • .github/workflows/build.yml (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Build binaries
  • GitHub Check: Summary
🔇 Additional comments (4)
.github/workflows/build.yml (4)

14-18: LGTM! Well-structured version outputs.

The job outputs are properly defined following semantic versioning principles and are appropriately scoped.


59-60: LGTM! Space optimization implemented.

The modification to skip packaging aligns with the previous comment about avoiding out-of-space errors in the GitHub builder VM.


77-81: Verify Docker Hub authentication requirement.

The Docker Hub login step is commented out. If image pushing is required, authentication should be properly configured.

Please confirm:

  1. Is image pushing required for this workflow?
  2. If yes, is there an alternative authentication mechanism in place?

84-89: Add security measures for image publishing.

Consider adding image signing and scanning steps before publishing.

.github/workflows/build.yml Show resolved Hide resolved
.github/workflows/build.yml Show resolved Hide resolved
@COLDTURNIP COLDTURNIP force-pushed the feat-secure_build branch 2 times, most recently from 328fcb5 to 5d458bb Compare February 14, 2025 03:02
@COLDTURNIP COLDTURNIP requested a review from derekbit February 14, 2025 03:03
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (3)
scripts/package (1)

53-54: ⚠️ Potential issue

Remove echo of build command to prevent secret leakage.

Echoing the full build command could expose secrets passed through environment variables.

-echo "${BUILD_CMD[@]}" "${IMAGE_BUILD_CMD_ARGS[@]}"
"${BUILD_CMD[@]}" "${IMAGE_BUILD_CMD_ARGS[@]}"
package/Dockerfile (2)

2-2: ⚠️ Potential issue

Pin base images to specific digests.

Using floating tags for base images could lead to supply chain attacks.

-FROM registry.suse.com/bci/golang:1.23 AS builder
+FROM registry.suse.com/bci/golang:1.23@sha256:<digest> AS builder

-FROM registry.suse.com/bci/bci-base:15.6 AS release
+FROM registry.suse.com/bci/bci-base:15.6@sha256:<digest> AS release

Also applies to: 15-15


6-7: ⚠️ Potential issue

Limit build context exposure.

Copying the entire context could expose sensitive files.

Add a .dockerignore file to exclude unnecessary and sensitive files from the build context:

.git
.github
.gitignore
*.md
*.log
**/node_modules
**/dist
**/build
**/.env*
🧹 Nitpick comments (1)
scripts/build (1)

23-38: Validate architecture input to prevent command injection.

The ARCHS environment variable is split and used directly in commands. While the current usage is safe due to the controlled context, consider adding explicit validation for allowed architectures.

 ARCHS=${ARCHS:-''}
 if [[ -z "${ARCHS}" ]]; then
     case $(uname -m) in
     aarch64 | arm64)
         ARCHS=(arm64)
         ;;
     x86_64)
         ARCHS=(amd64)
         ;;
     *)
         echo "$(uname -a): unsupported architecture"
         exit 1
     esac
 else
+    # Validate provided architectures
+    for arch in ${ARCHS}; do
+        case "${arch}" in
+            arm64|amd64) ;;
+            *) echo "Error: Unsupported architecture: ${arch}"; exit 1 ;;
+        esac
+    done
     IFS=' ' read -r -a ARCHS <<<"${ARCHS}"
 fi
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5d458bb and 9843826.

📒 Files selected for processing (8)
  • .github/workflows/build.yml (2 hunks)
  • .gitignore (1 hunks)
  • Dockerfile.dapper (1 hunks)
  • Makefile (2 hunks)
  • package/Dockerfile (2 hunks)
  • scripts/build (1 hunks)
  • scripts/ci (1 hunks)
  • scripts/package (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
  • .gitignore
  • scripts/ci
  • .github/workflows/build.yml
  • Dockerfile.dapper
  • Makefile
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Build binaries
  • GitHub Check: Summary
🔇 Additional comments (3)
scripts/build (1)

6-8:

✅ Verification successful

Verify sensitive information in version metadata.

The version metadata is embedded into the binary. Ensure no sensitive information is included in VERSION, GITCOMMIT, or BUILDDATE variables.


🏁 Script executed:

#!/bin/bash
# Check for potential sensitive info in version metadata
git log -1 --pretty=format:%s
echo "VERSION: $VERSION"
echo "BUILDDATE: $BUILDDATE"

Length of output: 61


Sensitive Metadata Verification: No Sensitive Information Detected

The executed script confirms that the environment variables for version metadata (i.e., VERSION and BUILDDATE) are unset, meaning no sensitive information is embedded into the binary. Although GITCOMMIT wasn’t explicitly checked, the current setup indicates that these values are not provided during the build. Please ensure that if these variables ever become populated, they continue to avoid including sensitive data.

  • Verified VERSION and BUILDDATE are empty.
  • Ensure proper handling if these values are set in other environments.
scripts/package (1)

37-38: LGTM! Secure build configuration.

The secure build configuration with SBOM generation and provenance attestation aligns with SLSA Level 3 requirements.

package/Dockerfile (1)

33-33:

❓ Verification inconclusive

Verify file permissions for security-sensitive files.

Ensure appropriate permissions are set for security-sensitive files like nsmounter.


🏁 Script executed:

#!/bin/bash
# Check file permissions in the final image
docker build -t temp-image . -f package/Dockerfile
docker create --name temp-container temp-image
docker cp temp-container:/usr/local/sbin/nsmounter ./nsmounter
ls -l ./nsmounter
docker rm temp-container
docker rmi temp-image

Length of output: 781


Action: Please Verify File Permissions Manually

The verification script did not produce any output because the Docker commands weren't available in the current environment. As a result, we couldn't confirm that the permissions for the security-sensitive file nsmounter (copied to /usr/local/sbin/) are set appropriately. Please re-run the Docker-based verification in an environment where Docker is installed or perform a manual check to ensure that nsmounter has the proper permissions.

  • Location: package/Dockerfile (Line 33).
  • Recommendation: Execute the permissions verification manually, verifying that /usr/local/sbin/nsmounter in the built image has appropriate, secure permissions.

scripts/package Show resolved Hide resolved
Signed-off-by: Raphanus Lo <yunchang.lo@suse.com>
@derekbit derekbit merged commit f2d94df into longhorn:master Feb 14, 2025
9 checks passed
@COLDTURNIP COLDTURNIP deleted the feat-secure_build branch February 17, 2025 01:59
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.

2 participants