Skip to content

Commit

Permalink
Merge pull request #652 from fasaxc/icmp-validation
Browse files Browse the repository at this point in the history
Fix validation for ICMP type and code in backend rules.
  • Loading branch information
fasaxc authored Nov 6, 2017
2 parents fd037e8 + ac10600 commit 4a01a23
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 5 deletions.
11 changes: 7 additions & 4 deletions lib/backend/model/rule.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,13 @@ type Rule struct {
Protocol *numorstring.Protocol `json:"protocol,omitempty" validate:"omitempty"`
NotProtocol *numorstring.Protocol `json:"!protocol,omitempty" validate:"omitempty"`

ICMPType *int `json:"icmp_type,omitempty" validate:"omitempty,gte=1,lte=255"`
ICMPCode *int `json:"icmp_code,omitempty" validate:"omitempty,gte=1,lte=255"`
NotICMPType *int `json:"!icmp_type,omitempty" validate:"omitempty,gte=1,lte=255"`
NotICMPCode *int `json:"!icmp_code,omitempty" validate:"omitempty,gte=1,lte=255"`
// ICMP validation notes: 0 is a valid (common) ICMP type and code. Type = 255 is not assigned
// to any protocol and the Linux kernel doesn't support matching on it so we validate against
// it.
ICMPType *int `json:"icmp_type,omitempty" validate:"omitempty,gte=0,lt=255"`
ICMPCode *int `json:"icmp_code,omitempty" validate:"omitempty,gte=0,lte=255"`
NotICMPType *int `json:"!icmp_type,omitempty" validate:"omitempty,gte=0,lt=255"`
NotICMPCode *int `json:"!icmp_code,omitempty" validate:"omitempty,gte=0,lte=255"`

SrcTag string `json:"src_tag,omitempty" validate:"omitempty,tag"`
SrcNet *net.IPNet `json:"src_net,omitempty" validate:"omitempty"`
Expand Down
8 changes: 7 additions & 1 deletion lib/backend/model/rule_test.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) 2016 Tigera, Inc. All rights reserved.
// Copyright (c) 2016-2017 Tigera, Inc. All rights reserved.

// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -35,6 +35,8 @@ var icmpProto = numorstring.ProtocolFromString("icmp")
var intProto = numorstring.ProtocolFromInt(123)
var icmpType = 10
var icmpCode = 6
var icmpTypeZero = 0
var icmpCodeZero = 0
var portRange, _ = numorstring.PortFromRange(10, 20)
var ports = []numorstring.Port{
numorstring.SinglePort(1234),
Expand All @@ -59,6 +61,10 @@ var ruleStringTests = []ruleTest{
"deny icmp type 10"},
{model.Rule{Protocol: &icmpProto, ICMPType: &icmpType, ICMPCode: &icmpCode},
"allow icmp type 10 code 6"},
{model.Rule{Action: "deny", Protocol: &icmpProto, ICMPType: &icmpTypeZero},
"deny icmp type 0"},
{model.Rule{Protocol: &icmpProto, ICMPType: &icmpTypeZero, ICMPCode: &icmpCodeZero},
"allow icmp type 0 code 0"},
// And negations of packet-wide matches.
{model.Rule{Action: "allow", NotProtocol: &tcpProto}, "allow !tcp"},
{model.Rule{Action: "deny", Protocol: &icmpProto, NotICMPType: &icmpType},
Expand Down
14 changes: 14 additions & 0 deletions lib/validator/validator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,20 @@ func init() {
// Empty rule is valid, it means "allow all".
Entry("empty rule (m)", model.Rule{}, true),

// (Backend model) ICMP.
Entry("should accept ICMP 0/any (m)", model.Rule{ICMPType: &V0}, true),
Entry("should accept ICMP 0/0 (m)", model.Rule{ICMPType: &V0, ICMPCode: &V0}, true),
Entry("should accept ICMP 128/0 (m)", model.Rule{ICMPType: &V128, ICMPCode: &V0}, true),
Entry("should accept ICMP 254/255 (m)", model.Rule{ICMPType: &V254, ICMPCode: &V255}, true),
Entry("should reject ICMP 255/255 (m)", model.Rule{ICMPType: &V255, ICMPCode: &V255}, false),
Entry("should reject ICMP 128/256 (m)", model.Rule{ICMPType: &V128, ICMPCode: &V256}, false),
Entry("should accept not ICMP 0/any (m)", model.Rule{NotICMPType: &V0}, true),
Entry("should accept not ICMP 0/0 (m)", model.Rule{NotICMPType: &V0, NotICMPCode: &V0}, true),
Entry("should accept not ICMP 128/0 (m)", model.Rule{NotICMPType: &V128, NotICMPCode: &V0}, true),
Entry("should accept not ICMP 254/255 (m)", model.Rule{NotICMPType: &V254, NotICMPCode: &V255}, true),
Entry("should reject not ICMP 255/255 (m)", model.Rule{NotICMPType: &V255, NotICMPCode: &V255}, false),
Entry("should reject not ICMP 128/256 (m)", model.Rule{NotICMPType: &V128, NotICMPCode: &V256}, false),

// (Backend model) Actions.
Entry("should accept allow action (m)", model.Rule{Action: "allow"}, true),
Entry("should accept deny action (m)", model.Rule{Action: "deny"}, true),
Expand Down

0 comments on commit 4a01a23

Please sign in to comment.