OCPBUGS-99202: Prevent daemon panic on out-of-range rule order - #754
OCPBUGS-99202: Prevent daemon panic on out-of-range rule order#754smulje wants to merge 1 commit into
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughIngress rule order documentation and CRD schemas now enforce values from 1 through 99. Webhook validation and eBPF loading reject invalid orders, the shared maximum is a constant, and boundary validation tests were added. ChangesIngress rule order validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (1)
pkg/failsaferules/failsaferules.go (1)
5-11: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the rule bounds immutable. Exported
vars can be reassigned by any importer, which lets validation drift from the fixed eBPF array size again. Define both asconstso the limit stays coupled to the array capacity.🤖 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/failsaferules/failsaferules.go` around lines 5 - 11, Change MAX_INGRESS_RULES and MaxRuleOrder from exported variables to constants, preserving their existing values and deriving MaxRuleOrder from MAX_INGRESS_RULES so both bounds remain immutable and coupled to the eBPF array capacity.
🤖 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.
Nitpick comments:
In `@pkg/failsaferules/failsaferules.go`:
- Around line 5-11: Change MAX_INGRESS_RULES and MaxRuleOrder from exported
variables to constants, preserving their existing values and deriving
MaxRuleOrder from MAX_INGRESS_RULES so both bounds remain immutable and coupled
to the eBPF array capacity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7539d913-1ff5-498c-b71c-46d0dbf5196d
📒 Files selected for processing (12)
api/v1alpha1/ingressnodefirewall_types.gobundle/manifests/ingressnodefirewall.openshift.io_ingressnodefirewallnodestates.yamlbundle/manifests/ingressnodefirewall.openshift.io_ingressnodefirewalls.yamlconfig/crd/bases/ingressnodefirewall.openshift.io_ingressnodefirewallnodestates.yamlconfig/crd/bases/ingressnodefirewall.openshift.io_ingressnodefirewalls.yamlmanifests/stable/ingressnodefirewall.openshift.io_ingressnodefirewallnodestates.yamlmanifests/stable/ingressnodefirewall.openshift.io_ingressnodefirewalls.yamlpkg/ebpf/ingress_node_firewall_loader.gopkg/failsaferules/failsaferules.gopkg/failsaferules/failsaferules_test.gopkg/webhook/webhook.gopkg/webhook/webhook_suite_test.go
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
39fd0d7 to
22abf0c
Compare
smulje
changed the title
openshift-ci-robot
added
jira/severity-important
|
@smulje: This pull request references Jira Issue OCPBUGS-99202, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (core-networking-bot@redhat.com), skipping review request. The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
| // + index 0 is used internally as catch all for unclassified packets matching the same sourceCIDR. | ||
| // The minimum order value is 1, the maximum is 99, and the values must be unique. | ||
| // Index 0 is used internally as catch all for unclassified packets matching the same sourceCIDR. | ||
| // Order is consumed as an index into a fixed-size eBPF rules array of length 100. |
There was a problem hiding this comment.
That last sentence is not part of the API; it's an implementation detail, which could be changed in the future, so it shouldn't be documented here.
| if err := failsaferules.ValidateRuleOrder(idx); err != nil { | ||
| return keys, rules, err | ||
| } | ||
| rules.Rules[idx].RuleId = rule.Order |
There was a problem hiding this comment.
It would be more robust to just check idx against len(rules.Rules), so that even if ValidateRuleOrder was wrong, it wouldn't crash
| // ValidateRuleOrder reports whether order can be used as an index into the | ||
| // fixed-size eBPF rules array. Index 0 is reserved; values >= MAX_INGRESS_RULES | ||
| // are out of bounds. | ||
| func ValidateRuleOrder(order uint32) error { |
There was a problem hiding this comment.
failsaferules is about blocking valid-but-dangerous rules, like "deny all access to the apiserver". In this case we are checking for an invalid rule, so it doesn't belong in this package.
(But since you don't need to call this from ingress_node_firewall_loader.go now, the check can just be inlined into validateRules below, like the other checks.)
Order is used as a raw index into a fixed-size eBPF rules array of length 100. Without an upper bound, values >= 100 panic every node's daemon into CrashLoopBackOff. Cap order at 99 in the CRD and webhook, and bound-check against the rules array length before indexing so crafted NodeState objects cannot crash the daemon. Signed-off-by: Swati Mulje <smulje@redhat.com>
22abf0c to
8d5c01a
Compare
|
@smulje: This pull request references Jira Issue OCPBUGS-99202, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (core-networking-bot@redhat.com), skipping review request. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@smulje: all tests passed! 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, smulje The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
3 participants
Summary
IngressNodeFirewallProtocolRule.orderat 99 in the CRD schema (covers bothIngressNodeFirewallandIngressNodeFirewallNodeState)orderin the admission webhookorderin the daemon before using it as an eBPF rules-array index, so a crafted NodeState cannot panic every node's reconciler into CrashLoopBackOfforderis consumed as a raw index into a fixed[100]BpfRuleTypeStarray (index 0 reserved). Previously onlyminimum: 1was enforced, and the webhook only checked rule count / uniqueness — anyorder >= 100(up touint32max) panicked the daemon cluster-wide.Test plan
go test ./pkg/failsaferules/order == 100andorder == ^uint32(0); acceptsorder == 99IngressNodeFirewallwithorder: 100and confirm API/webhook rejectionIngressNodeFirewallNodeStatewithorder: 100and confirm CRD rejection (should be less than or equal to 99)order: 99is accepted and daemons attach XDP without panicCluster verification (OCP 4.21)
Reproduced the panic on stock 4.21 (
index out of range [100] with length 100), then verified the fix with custom images built from this branch:quay.io/smulje/ingress-node-firewall-operator:order-oob-fixquay.io/smulje/ingress-node-firewall-daemon:order-oob-fixResults with the custom images:
order: 100/order: 4294967295rejected by webhookAn error occurred Error "Invalid value: 100: spec.ingress[0].rules[0].order in body should be less than or equal to 99" for field "spec.ingress[0].rules[0].order".order: 99applied successfully (Synchronized), daemons remained RunningIngressNodeFirewallNodeStatewrite withorder: 100rejected by updated CRD schemaSummary by CodeRabbit
Bug Fixes
Documentation
Tests