-
Notifications
You must be signed in to change notification settings - Fork 270
Feature/kafka mode threat guardrails #3768
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 AI Security analysis: "A medium-severity path traversal issue exists in apps/guardrails-service/container/src/pkg/kafka/consumer.go, risking unauthorized file access when consumer code uses user-controlled paths without proper sanitization."
| Risk Level | AI Score |
|---|---|
| 🟢 LOW | 35.0/100 |
Top 1 security issues / 1 total (Critical: 0, High: 0, Medium: 1, Low: 0)
| Title | Location | Recommendation |
|---|---|---|
apps/guardrails-service/container/src/pkg/kafka/consumer.go:184 |
Use filepath.Base to only use the filename and not path information. Always validate the… |
| @@ -0,0 +1,5 @@ | |||
| --- | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gitignore .cursor files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding rules for everyone.
| zap.Int("policiesCount", len(policies))) | ||
| return policies, auditPolicies, compiledRules, hasAuditRules, nil | ||
| } | ||
| s.cache.mu.RUnlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can add a comment to highlight the criticality of this line to avoid deadlock since the regular defer unlock way won't work here.
| config := zap.NewProductionConfig() | ||
| config := zap.NewDevelopmentConfig() | ||
| config.Level = zap.NewAtomicLevelAt(level) | ||
| config.EncoderConfig.TimeKey = "timestamp" | ||
| config.EncoderConfig.EncodeTime = zapcore.ISO8601TimeEncoder | ||
| config.EncoderConfig.EncodeTime = zapcore.TimeEncoderOfLayout("2006-01-02 15:04:05") | ||
| config.EncoderConfig.EncodeLevel = zapcore.CapitalColorLevelEncoder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were the logs not readable before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they were in JSON, this is single line text, slightly more convenient
| } | ||
| defer consumer.Close() | ||
|
|
||
| ctx := context.Background() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)
defer stop()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sigTerm handling is present inside the Start function.
So adding here will be redundant.
| if err != nil { | ||
| if errors.Is(err, context.DeadlineExceeded) { | ||
| // Timeout is expected when no messages available, continue silently | ||
| continue | ||
| } | ||
| if errors.Is(err, context.Canceled) { | ||
| return nil | ||
| } | ||
| c.logger.Error("Failed to read message from Kafka", zap.Error(err)) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can add more error scenarios
if ctx.Err() != nil { // parent context canceled
return nil
}
if errors.Is(err, context.DeadlineExceeded) {
continue
}
if errors.Is(err, context.Canceled) {
return nil
}
if errors.Is(err, io.EOF) { // kafka-go errors
return nil
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 AI Security analysis: "A medium-severity path traversal issue exists in apps/guardrails-service/container/src/pkg/kafka/consumer.go, risking unauthorized file access when consumer code uses user-controlled paths without proper sanitization."
| Risk Level | AI Score |
|---|---|
| 🟢 LOW | 35.0/100 |
Top 1 security issues / 1 total (Critical: 0, High: 0, Medium: 1, Low: 0)
| Title | Location | Recommendation |
|---|---|---|
apps/guardrails-service/container/src/pkg/kafka/consumer.go:185 |
Use filepath.Base to only use the filename and not path information. Always validate the… |
Co-authored-by: devsecopsbot[bot] <177726887+devsecopsbot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 AI Security analysis: "A medium-severity path traversal issue exists in apps/guardrails-service/container/src/pkg/kafka/consumer.go, risking unauthorized file access when consumer code uses user-controlled paths without proper sanitization."
| Risk Level | AI Score |
|---|---|
| 🟢 LOW | 35.0/100 |
Top 1 security issues / 1 total (Critical: 0, High: 0, Medium: 1, Low: 0)
| Title | Location | Recommendation |
|---|---|---|
apps/guardrails-service/container/src/pkg/kafka/consumer.go:191 |
Use filepath.Base to only use the filename and not path information. Always validate the… |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 AI Security analysis: "A medium-severity path traversal issue exists in apps/guardrails-service/container/src/pkg/kafka/consumer.go, risking unauthorized file access when consumer code uses user-controlled paths without proper sanitization."
| Risk Level | AI Score |
|---|---|
| 🟢 LOW | 35.0/100 |
Top 1 security issues / 1 total (Critical: 0, High: 0, Medium: 1, Low: 0)
| Title | Location | Recommendation |
|---|---|---|
apps/guardrails-service/container/src/pkg/kafka/consumer.go:193 |
Use filepath.Base to only use the filename and not path information. Always validate the… |
Co-authored-by: devsecopsbot[bot] <177726887+devsecopsbot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 AI Security analysis: "A medium-severity path traversal issue exists in apps/guardrails-service/container/src/pkg/kafka/consumer.go, risking unauthorized file access when consumer code uses user-controlled paths without proper sanitization."
| Risk Level | AI Score |
|---|---|
| 🟢 LOW | 35.0/100 |
Top 1 security issues / 1 total (Critical: 0, High: 0, Medium: 1, Low: 0)
| Title | Location | Recommendation |
|---|---|---|
apps/guardrails-service/container/src/pkg/kafka/consumer.go:199 |
Use filepath.Base to only use the filename and not path information. Always validate the… |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 AI Security analysis: "A medium-severity path traversal issue exists in apps/guardrails-service/container/src/pkg/kafka/consumer.go, risking unauthorized file access when consumer code uses user-controlled paths without proper sanitization."
| Risk Level | AI Score |
|---|---|
| 🟢 LOW | 35.0/100 |
Top 1 security issues / 1 total (Critical: 0, High: 0, Medium: 1, Low: 0)
| Title | Location | Recommendation |
|---|---|---|
apps/guardrails-service/container/src/pkg/kafka/consumer.go:184 |
Use filepath.Base to only use the filename and not path information. Always validate the… |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 AI Security analysis: "Hard-coded AWS secret in .github/workflows/prod.yml risks credential exposure and account compromise. Medium risks include a path traversal in the Kafka consumer (arbitrary file reads) and workflow_dispatch inputs that allow user-influenced build output, creating a supply-chain risk."
| Risk Level | AI Score |
|---|---|
| 🔴 CRITICAL | 85.0/100 |
Top 4 security issues / 4 total (Critical: 0, High: 1, Medium: 2, Low: 1)
| docker buildx build --platform linux/arm64/v8,linux/amd64 -t $ECR_REGISTRY/guardrails-service:local -t $ECR_REGISTRY/guardrails-service:$IMAGE_VERSION . --push | ||
| echo "::set-output name=image::$ECR_REGISTRY/guardrails-service:$IMAGE_VERSION" | ||
| - name: Configure AWS Credentials for ECR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - name: Configure AWS Credentials for ECR | |
| - name: Configure AWS Credentials for ECR | |
| uses: aws-actions/configure-aws-credentials@v2 | |
| with: | |
| role-to-assume: ${{ secrets.AWS_OIDC_ROLE_ARN }} | |
| aws-region: us-east-1 |
🟠 HIGH: Hard-coded AWS secret access key
Replace long-lived access key usage with OIDC-based role assumption (aws-actions/configure-aws-credentials v2). This avoids storing long-term AWS secrets in the repo and uses short-lived credentials via an IAM role (set AWS_OIDC_ROLE_ARN in repo secrets and configure the role trust for GitHub OIDC).
| run: | | ||
| docker buildx create --use | ||
| cd apps/guardrails-service/container | ||
| docker buildx build --platform linux/arm64/v8,linux/amd64 -t $ECR_REGISTRY/guardrails-service:local -t $ECR_REGISTRY/guardrails-service:$IMAGE_VERSION . --push |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| docker buildx build --platform linux/arm64/v8,linux/amd64 -t $ECR_REGISTRY/guardrails-service:local -t $ECR_REGISTRY/guardrails-service:$IMAGE_VERSION . --push | |
| echo "image=$ECR_REGISTRY/guardrails-service:$IMAGE_VERSION" >> $GITHUB_OUTPUT |
🟢 LOW: Deprecated ::set-output workflow command
The ::set-output command is deprecated. Write the output to the GITHUB_OUTPUT file to set the step output (preserves the same step id output 'image').
| InsecureSkipVerify: os.Getenv("KAFKA_INSECURE_SKIP_VERIFY") == "true", | ||
| MinVersion: tls.VersionTLS12, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | |
| // Sanitize path to prevent path traversal: use only the filename and force local path | |
| if tlsCACertPath != "" { | |
| if i := strings.LastIndexAny(tlsCACertPath, "/\\"); i != -1 { | |
| tlsCACertPath = tlsCACertPath[i+1:] | |
| } | |
| // Ensure we always use a local relative path (avoid absolute paths) | |
| tlsCACertPath = "./" + strings.TrimLeft(tlsCACertPath, "/\\") | |
| } |
🟡 MEDIUM: Path Traversal
Prevent path traversal by stripping any directory components from KAFKA_TLS_CA_CERT_PATH and forcing a local relative path. This avoids using attacker-controlled paths (e.g., ../../etc/passwd) when reading the CA file without adding new imports.
No description provided.