Minor security hardening#1289
Conversation
- sha pinning of the remaining github-owned actions - dependabot setting for docker & actions - force tls client minversion - remove bad practice promoted in sample file (wildcard permissions)
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughDependabot adds GitHub Actions and Docker update entries. Several workflows pin GitHub Actions to commit SHAs, the release workflow switches to ChangesGitHub automation updates
Runtime security hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 2 linked repositories, but your current plan allows 0. Analyzed ``, skipped Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contrib/kubernetes/deployment.yaml (1)
17-35: 🩺 Stability & Availability | 🟠 MajorAdd CPU and memory requests/limits for
flowlogs-pipeline.The
flowlogs-pipelinecontainer (lines 17–35) lacks aresourcesblock, leaving scheduling and resource enforcement undefined. Althoughresources:appears at lines 91/101, those likely belong to another container or init container in this pod spec.Add the following to ensure stability and availability:
resources: requests: cpu: "100m" memory: "128Mi" limits: cpu: "500m" memory: "512Mi"Adjust values based on actual benchmark data.
Location:
contrib/kubernetes/deployment.yaml(add undersecurityContextin theflowlogs-pipelinecontainer).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contrib/kubernetes/deployment.yaml` around lines 17 - 35, The flowlogs-pipeline container definition is missing its own resources block, so add CPU and memory requests/limits directly in the flowlogs-pipeline container spec in the deployment manifest. Place the resources stanza alongside the existing image, args, volumeMounts, and securityContext fields for that container, and ensure it includes requests and limits for both cpu and memory. Use the flowlogs-pipeline container entry to locate the correct section, since other resources blocks in the pod spec may belong to different containers.Source: Path instructions
🧹 Nitpick comments (2)
.github/workflows/release.yml (1)
46-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider zizmor's
gh releasesuggestion optionally.zizmor flags
softprops/action-gh-releaseas superfluous. Usinggh release createdirectly is viable and reduces third-party action dependency:- name: create draft release and upload binaries env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | gh release create "${{ env.tag }}" \ --draft \ --title "${{ env.tag }}" \ ./release-assets/*However, the current action is legitimate and widely-used. This is optional.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 46 - 53, The release workflow currently uses softprops/action-gh-release, which zizmor flags as superfluous; optionally switch the “create draft release and upload binaries” step to a direct gh release create invocation instead. Update the release job to use GH_TOKEN with the built-in gh CLI, and preserve the existing behavior of creating a draft release, setting the title to env.tag, and uploading release-assets/*.Source: Linters/SAST tools
contrib/kubernetes/deployment.yaml (1)
30-35: 🩺 Stability & Availability | 🔵 TrivialAdd explicit runAsUser and runAsGroup for defense-in-depth
The image
contrib/docker/DockerfilesetsUSER 65532:65532, satisfyingrunAsNonRoot: true. Explicitly setrunAsUserandrunAsGroupincontrib/kubernetes/deployment.yaml(lines 30–35) to enforce the UID regardless of image changes.Current fragment
securityContext: allowPrivilegeEscalation: false readOnlyRootFilesystem: true runAsNonRoot: true capabilities: drop: [ALL]Add:
runAsUser: 65532 runAsGroup: 65532🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contrib/kubernetes/deployment.yaml` around lines 30 - 35, Add explicit user and group IDs to the Kubernetes pod securityContext in deployment.yaml: alongside runAsNonRoot, set runAsUser and runAsGroup to 65532 so the workload continues to run as the intended non-root identity even if the image USER changes. Update the securityContext block near the existing allowPrivilegeEscalation, readOnlyRootFilesystem, and capabilities settings.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/api/tls.go`:
- Line 37: The ClientTLS setup currently hardcodes tls.VersionTLS13, which
breaks TLS 1.2-only endpoints. Update the ClientTLS configuration in tls.go to
make MinVersion configurable instead of fixed, and ensure the code path that
builds the TLS config uses that field with a sensible default that preserves
existing TLS 1.2 compatibility. If you intentionally keep TLS 1.3 as the
minimum, then treat it as a breaking change by updating the release/migration
notes and adding tests that cover TLS 1.2 fallback behavior.
---
Outside diff comments:
In `@contrib/kubernetes/deployment.yaml`:
- Around line 17-35: The flowlogs-pipeline container definition is missing its
own resources block, so add CPU and memory requests/limits directly in the
flowlogs-pipeline container spec in the deployment manifest. Place the resources
stanza alongside the existing image, args, volumeMounts, and securityContext
fields for that container, and ensure it includes requests and limits for both
cpu and memory. Use the flowlogs-pipeline container entry to locate the correct
section, since other resources blocks in the pod spec may belong to different
containers.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 46-53: The release workflow currently uses
softprops/action-gh-release, which zizmor flags as superfluous; optionally
switch the “create draft release and upload binaries” step to a direct gh
release create invocation instead. Update the release job to use GH_TOKEN with
the built-in gh CLI, and preserve the existing behavior of creating a draft
release, setting the title to env.tag, and uploading release-assets/*.
In `@contrib/kubernetes/deployment.yaml`:
- Around line 30-35: Add explicit user and group IDs to the Kubernetes pod
securityContext in deployment.yaml: alongside runAsNonRoot, set runAsUser and
runAsGroup to 65532 so the workload continues to run as the intended non-root
identity even if the image USER changes. Update the securityContext block near
the existing allowPrivilegeEscalation, readOnlyRootFilesystem, and capabilities
settings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1fce5369-ca7b-4271-a061-0b46861b6a58
📒 Files selected for processing (9)
.github/dependabot.yml.github/workflows/build_image_pr.yml.github/workflows/pull_request.yml.github/workflows/pull_request_e2e.yml.github/workflows/push_image.yml.github/workflows/push_image_pr.yml.github/workflows/release.ymlcontrib/kubernetes/deployment.yamlpkg/api/tls.go
| func (c *ClientTLS) Build() (*tls.Config, error) { | ||
| tlsConfig := &tls.Config{ | ||
| InsecureSkipVerify: c.InsecureSkipVerify, | ||
| MinVersion: tls.VersionTLS13, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether docs/tests/config samples mention the new TLS 1.3 requirement
# or provide a migration path for existing TLS 1.2 deployments.
fd -e go -e md -e yaml -e yml -e json -e toml | xargs rg -n \
'ClientTLS|MinVersion|VersionTLS|TLS 1\.2|TLS 1\.3|tlsVersion|min.*tls|caCertPath|insecureSkipVerify'Repository: netobserv/flowlogs-pipeline
Length of output: 22179
TLS 1.3 enforcement is a breaking change for TLS 1.2-only endpoints.
Hardcoding MinVersion: tls.VersionTLS13 in pkg/api/tls.go immediately breaks connections to Kafka brokers or gRPC servers supporting only TLS 1.2. Add a configurable MinVersion field to ClientTLS or publish a major release with explicit migration notes and tests covering TLS 1.2 fallback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/api/tls.go` at line 37, The ClientTLS setup currently hardcodes
tls.VersionTLS13, which breaks TLS 1.2-only endpoints. Update the ClientTLS
configuration in tls.go to make MinVersion configurable instead of fixed, and
ensure the code path that builds the TLS config uses that field with a sensible
default that preserves existing TLS 1.2 compatibility. If you intentionally keep
TLS 1.3 as the minimum, then treat it as a breaking change by updating the
release/migration notes and adding tests that cover TLS 1.2 fallback behavior.
Source: Path instructions
jotak
added
the
ok-to-test
|
New image: quay.io/netobserv/flowlogs-pipeline:030d1a43It will expire in two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=030d1a43 make set-flp-image |
|
@jotak: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
1 participant
Description
Dependencies
n/a
Checklist
To run a perfscale test, comment with:
/test flp-node-density-heavy-25nodesSummary by CodeRabbit
New Features
Bug Fixes
Chores