Security: switch to using image's digest sha256 value on promotion #249
Conversation
WalkthroughThe pull request introduces several modifications across multiple files, primarily enhancing command functionalities related to image deployment. Key changes include the addition of a method to utilize Docker image digests in command options, updates to the deployment logic to incorporate these digests, and refinements in error handling. Test cases have also been expanded to ensure the new features are adequately verified. The Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| @@ -32,7 +32,7 @@ def copy_image_from_upstream | |||
| def deploy_image | |||
| args = [] | |||
| args.push("--run-release-phase") if config.current[:release_script] | |||
| run_cpflow_command("deploy-image", "-a", config.app, *args) | |||
| run_cpflow_command("deploy-image", "-a", config.app, "--use-digest-ref", *args) | |||
There was a problem hiding this comment.
Maybe we should provide use_digest_ref option for promote-app-from-upstream command too - user may not want this behaviour enabled by default
lib/command/deploy_image.rb
Outdated
| raise "Image '#{image}' does not exist in the Docker repository on Control Plane " \ | ||
| "(see https://console.cpln.io/console/org/#{config.org}/repository/#{config.app}). " \ | ||
| "Use `cpflow build-image` first." | ||
| end | ||
|
|
||
| image = "#{image_details['name']}@#{image_details['digest']}" if config.options[:use_digest_ref] |
There was a problem hiding this comment.
image_details["name"] contains image name and tag and we can keep the tag - it will be ignored due to existence of the digest value
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
lib/command/deploy_image.rb (1)
37-37: Consider strengthening image name validationThe regex pattern could be more strict to prevent potential path traversal or malformed image names.
- next unless container["image"].match?(%r{^/org/#{config.org}/image/#{config.app}}) + next unless container["image"].match?(%r{^/org/#{Regexp.escape(config.org)}/image/#{Regexp.escape(config.app)}$})spec/command/deploy_image_spec.rb (1)
53-58: Enhance digest reference testingWhile the basic functionality is tested, consider adding tests for:
- Invalid digest formats
- Edge cases with special characters in image names
- Integration with release phase
Example test cases to add:
it "validates digest format" do # Mock invalid digest allow(cp).to receive(:fetch_image_details) .and_return({'name' => app, 'digest' => 'invalid-digest'}) result = run_cpflow_command("deploy-image", "-a", app, "--use-digest-ref") expect(result[:status]).not_to eq(0) expect(result[:stderr]).to include("Invalid digest format") end it "handles special characters in image names" do special_app = "app-with.special_chars" # Test setup... end it "works with release phase", :slow do result = run_cpflow_command("deploy-image", "-a", app, "--use-digest-ref", "--run-release-phase") expect(result[:status]).to eq(0) expect(result[:stderr]).to include("Finished running release script") expect(result[:stderr]).to match(/sha256:[a-fA-F0-9]{64}/) end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)lib/command/base.rb(1 hunks)lib/command/deploy_image.rb(2 hunks)lib/command/promote_app_from_upstream.rb(1 hunks)spec/command/deploy_image_spec.rb(1 hunks)spec/command/promote_app_from_upstream_spec.rb(3 hunks)
🔇 Additional comments (9)
lib/command/deploy_image.rb (3)
8-9: LGTM: Option addition is clean and follows existing pattern
The use_digest_ref_option is properly added to the OPTIONS array, maintaining consistency with existing options.
25-30: Improved error handling for missing images
The error handling has been enhanced with:
- Clear error message with specific image details
- Helpful link to the console
- Actionable suggestion for resolution
32-32: Verify digest reference construction
The image reference construction looks correct but should be tested with various image name formats.
✅ Verification successful
Image digest reference construction verified
The implementation is correct and well-tested:
- The digest reference format
name@digestis consistently used across the codebase - Test files verify the pattern with proper SHA256 digest format (
sha256:[a-fA-F0-9]{64}) - The feature is properly integrated with a
--use-digest-refCLI option - The functionality is tested in both direct deployment (
deploy-image) and promotion (promote-app-from-upstream) scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for different image reference patterns in tests
rg -g '*.rb' 'image.*@sha256:[a-fA-F0-9]{64}'
Length of output: 47
Script:
#!/bin/bash
# Let's search for test files that might contain image digest references
fd -e rb -e spec test spec
# Then search for any image reference patterns in the test files
rg -g '*.rb' -g '*.spec' 'image.*@'
# Also search for test examples or fixtures with digest references
rg -g '*.rb' -g '*.yml' -g '*.yaml' 'digest'
# Look for actual implementation of digest reference handling
ast-grep --pattern 'use_digest_ref'
Length of output: 3564
lib/command/promote_app_from_upstream.rb (1)
35-35: Consider making digest reference configurable
While using digest references by default is good for security, some users might need to opt-out. Consider adding a configuration option or command flag to disable this behavior.
spec/command/promote_app_from_upstream_spec.rb (3)
33-33: LGTM: Proper verification of SHA256 digest in deployment message.
The test correctly verifies that the deployment message includes the image reference with SHA256 digest format.
68-68: LGTM: Proper negative test for deployment failure.
The test correctly verifies that no deployment message is shown when the release script fails.
103-103: LGTM: Comprehensive test coverage for successful deployment.
The test properly verifies the deployment message with SHA256 digest in the successful case, maintaining consistency with other test cases.
lib/command/base.rb (1)
446-455: LGTM: Well-structured option method for digest reference.
The implementation follows the established pattern for option methods and provides clear documentation.
CHANGELOG.md (1)
25-28: LGTM: Clear and well-formatted changelog entry.
The entry properly documents the change and follows the project's changelog format.
zzaakiirr
force-pushed
the
232-use-image-digest-on-promotion-from-upstream
branch
from
fc4d4a6 to
083ea58
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)lib/command/base.rb(1 hunks)lib/command/deploy_image.rb(2 hunks)lib/command/promote_app_from_upstream.rb(1 hunks)spec/command/deploy_image_spec.rb(1 hunks)spec/command/promote_app_from_upstream_spec.rb(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/command/base.rb
- CHANGELOG.md
- spec/command/promote_app_from_upstream_spec.rb
- lib/command/promote_app_from_upstream.rb
- spec/command/deploy_image_spec.rb
🔇 Additional comments (2)
lib/command/deploy_image.rb (2)
8-9: LGTM: Command option addition is well-placed
The addition of use_digest_ref_option to the OPTIONS array aligns with the PR's security objectives.
25-30: LGTM: Robust error handling implementation
The error handling for non-existent images is well implemented with a clear, actionable error message that includes the relevant Console URL.
|
Hi, I guess it is dangerous to switch existing default behaviour all at once. Overall, it should be:
Sure, sometimes can be shortened, but better follow the process, at least don't break. Especially for libs that are used in scripting. I mean some people may be using tags and relying on old way in "unimaginable" ways in their scripts. Can we please change this PR to be totally optional? Plus it may be not worth only an command-line option, but a yaml option as well. I mean, personally, I'd better configure this in the yaml as an app default vs updating all the cmd calls. |
…32-use-image-digest-on-promotion-from-upstream
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
spec/dummy/.controlplane/controlplane.yml (1)
136-141: Consider adding documentation comments.While the configuration is correctly structured, consider adding YAML comments to document:
- The security implications of using digest references
- When to enable this option
- Migration considerations for existing deployments
Example addition:
dummy-test-use-digest-image-ref-{GLOBAL_IDENTIFIER}: + # Enables secure image deployment using SHA256 digest references + # Benefits: Prevents tag hijacking and ensures image immutability + # Note: Enable this after testing with your CI/CD pipeline <<: *commonlib/command/deploy_image.rb (2)
29-33: Enhance error message clarity.Consider making the error message more specific about the image reference format.
- raise "Image '#{image}' does not exist in the Docker repository on Control Plane " \ + raise "Image reference '#{image}' does not exist in the Docker repository on Control Plane " \ "(see https://console.cpln.io/console/org/#{config.org}/repository/#{config.app}). " \ "Use `cpflow build-image` first."
35-35: Consider adding debug logging for reference changes.When switching to digest reference, it would be helpful to log this security enhancement for audit purposes.
- image = "#{image_details['name']}@#{image_details['digest']}" if config.use_digest_image_ref? + if config.use_digest_image_ref? + original_ref = image + image = "#{image_details['name']}@#{image_details['digest']}" + Shell.debug("Enhanced security: Using digest reference instead of tag") + Shell.debug("Original: #{original_ref}") + Shell.debug("Digest: #{image}") + enddocs/commands.md (1)
130-130: Consider documenting the security benefits of using digest referencesThe documentation should explain why using digest references is more secure than using tags. This would help users understand the importance of this feature and make informed decisions about using it.
Add a note explaining that digest references provide immutable, cryptographically secure references to exact image content, preventing potential supply chain attacks through tag manipulation.
README.md (2)
319-321: Enhance configuration documentation with security contextThe
use_digest_image_refconfiguration is correctly placed in the production app section, but its documentation could be improved.Consider expanding the comment to:
- # Used by the `cpflow deploy-image` and `cpflow promote-app-from-upstream` commands to include Docker image's digest (SHA256 value) in its reference. + # Security enhancement: When true, uses cryptographically secure SHA256 digest references instead of mutable tags + # for Docker images. This prevents supply chain attacks through tag manipulation and ensures exact image deployment. + # Recommended for production environments and any deployment where image integrity is critical. use_digest_image_ref: true
319-321: Consider a phased rollout strategy for digest referencesThe current implementation changes default behavior for
promote-app-from-upstream, which could impact existing deployments. Consider implementing a phased approach as suggested in the PR comments:
- Keep the current behavior as default
- Allow opt-in through configuration
- Add deprecation warnings for the old behavior
- Change defaults in a major version release
This would provide a smoother transition path while maintaining security benefits.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.md(1 hunks)README.md(1 hunks)docs/commands.md(2 hunks)lib/command/base.rb(1 hunks)lib/command/deploy_image.rb(1 hunks)lib/command/promote_app_from_upstream.rb(3 hunks)lib/core/config.rb(1 hunks)spec/command/deploy_image_spec.rb(1 hunks)spec/command/promote_app_from_upstream_spec.rb(2 hunks)spec/dummy/.controlplane/controlplane.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- spec/command/deploy_image_spec.rb
- lib/command/promote_app_from_upstream.rb
- lib/command/base.rb
- spec/command/promote_app_from_upstream_spec.rb
- CHANGELOG.md
🧰 Additional context used
📓 Learnings (1)
lib/command/deploy_image.rb (1)
Learnt from: zzaakiirr
PR: shakacode/control-plane-flow#249
File: lib/command/deploy_image.rb:32-32
Timestamp: 2024-12-16T11:52:41.732Z
Learning: In the `DeployImage` class (`lib/command/deploy_image.rb`), the `image_details` hash returned by `cp.fetch_image_details(image)` always contains a non-empty `digest` key.
🔇 Additional comments (2)
lib/core/config.rb (1)
148-150: LGTM! Well-structured configuration method.
The implementation follows the established pattern of checking both the current configuration and command options, with proper nil-safety using the safe navigation operator.
lib/command/deploy_image.rb (1)
28-35: LGTM! Secure implementation of digest references.
The implementation correctly:
- Fetches image details
- Validates image existence
- Constructs digest reference when configured
|
@dzirtusss Hi! I applied your suggestions, can you take another look?
|
What does this PR do?
This PR:
--use-digest-refoption tocpflow deploy imagecommand. If it's true then docker image's reference will include image's digest (SHA256 value)--use-digest-refoption tocpflow deploy imagecommand by default forcpflow promote-app-from-upstreamcommandIssue
#232
Tests
RSpec specific pipeline - https://github.com/shakacode/control-plane-flow/actions/runs/12348992443/job/34458894605
Summary by CodeRabbit
Summary by CodeRabbit
New Features
use_digest_image_refin deployment commands.--docker-contextoption.Bug Fixes
Documentation
Tests
--use-digest-image-refoption.