Skip to content

Add protocol number support for security group rules #3736

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.

Sign up for GitHub

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

Merged
andrijapanicsb merged 1 commit into from
Jan 8, 2020
Merged

Add protocol number support for security group rules #3736

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...rg/apache/cloudstack/api/command/user/securitygroup/AuthorizeSecurityGroupIngressCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public class AuthorizeSecurityGroupIngressCmd extends BaseAsyncCmd {
// ////////////// API parameters /////////////////////
// ///////////////////////////////////////////////////

@Parameter(name = ApiConstants.PROTOCOL, type = CommandType.STRING, description = "TCP is default. UDP is the other supported protocol")
@Parameter(name = ApiConstants.PROTOCOL, type = CommandType.STRING, description = "the protocol for the ACL rule. Valid values are TCP/UDP/ICMP/ALL or valid protocol number (see /etc/protocols). ALL is default.")
private String protocol;

@Parameter(name = ApiConstants.START_PORT, type = CommandType.INTEGER, description = "start port for this ingress rule")
Expand Down
21 changes: 16 additions & 5 deletions scripts/vm/network/security_group.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1032,7 +1032,18 @@ def add_network_rules(vm_name, vm_id, vm_ip, vm_ip6, signature, seqno, vmMac, ru
action = "ACCEPT"
direction = "-s"

range = str(start) + ':' + str(end)
if start == 0 and end == 0:
dport = ""
else:
dport = " --dport " + str(start) + ":" + str(end)

if protocol != 'all' and protocol != 'icmp' and protocol != 'tcp' and protocol != 'udp':

@wido wido Dec 3, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For ip6tables it's not 'icmp' which is used for the protocol, it's icmpv6, so don't we need to add a check there?

@ravening ravening Dec 3, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the UI, we only have options for "TCP", "UDP", "ICMP" and "ALL". There is no option for "ICMPv6" in UI. Once this is added in ui, we can have that check here as well.

@wido wido Dec 16, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rakgenius I'm aware, but underneath we translate ICMP to ICMPv6 when the CIDR/IP is an IPv6 adress. That's what we need to check.

@weizhouapache weizhouapache Jan 6, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wido the protocol is "icmp" in cloudstack, no matter the ip/cidr is ipv4 or ipv6.

look at the code below
https://github.com/apache/cloudstack/pull/3736/files/c27fdfee8c9f5ab06cf18dd6dc41ba356d75f4c4#diff-d10df45583c471e8de4a64128f95c802R1066-R1070

protocol_all = " -p " + protocol
protocol_state = " "
else:
protocol_all = " -p " + protocol + " -m " + protocol
protocol_state = " -m state --state NEW "

if 'icmp' == protocol:
range = str(start) + '/' + str(end)
if start == -1:
Expand All @@ -1041,16 +1052,16 @@ def add_network_rules(vm_name, vm_id, vm_ip, vm_ip6, signature, seqno, vmMac, ru
for ip in rule['ipv4']:
if protocol == 'all':
execute('iptables -I ' + vmchain + ' -m state --state NEW ' + direction + ' ' + ip + ' -j ' + action)
elif protocol != 'icmp':
execute('iptables -I ' + vmchain + ' -p ' + protocol + ' -m ' + protocol + ' --dport ' + range + ' -m state --state NEW ' + direction + ' ' + ip + ' -j ' + action)
elif protocol == 'icmp':
execute("iptables -I " + vmchain + " -p icmp --icmp-type " + range + " " + direction + " " + ip + " -j " + action)
else:
execute('iptables -I ' + vmchain + ' -p icmp --icmp-type ' + range + ' ' + direction + ' ' + ip + ' -j ' + action)
execute("iptables -I " + vmchain + protocol_all + dport + protocol_state + direction + " " + ip + " -j "+ action)

for ip in rule['ipv6']:
if protocol == 'all':
execute('ip6tables -I ' + vmchain + ' -m state --state NEW ' + direction + ' ' + ip + ' -j ' + action)
elif 'icmp' != protocol:
execute('ip6tables -I ' + vmchain + ' -p ' + protocol + ' -m ' + protocol + ' --dport ' + range + ' -m state --state NEW ' + direction + ' ' + ip + ' -j ' + action)
execute("ip6tables -I " + vmchain + protocol_all + dport + protocol_state + direction + " " + ip + " -j "+ action)
else:
# ip6tables does not allow '--icmpv6-type any', allowing all ICMPv6 is done by not allowing a specific type
if range == 'any':
Expand Down
33 changes: 27 additions & 6 deletions server/src/main/java/com/cloud/network/security/SecurityGroupManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
import org.apache.cloudstack.managed.context.ManagedContextRunnable;
import org.apache.cloudstack.utils.identity.ManagementServerNode;
import org.apache.commons.codec.digest.DigestUtils;
import org.apache.commons.lang.StringUtils;
import org.apache.log4j.Logger;

import com.cloud.agent.AgentManager;
Expand Down Expand Up @@ -617,10 +618,27 @@ private List<SecurityGroupRuleVO> authorizeSecurityGroupRule(final Long security
}
}

if (!NetUtils.isValidSecurityGroupProto(protocol)) {
throw new InvalidParameterValueException("Invalid protocol " + protocol);
//Validate Protocol
protocol = protocol.trim().toLowerCase();
//Check if protocol is a number
if(StringUtils.isNumeric(protocol)){
int protoNumber = Integer.parseInt(protocol);
// Deal with ICMP(protocol number 1) specially because it need to be paired with icmp type and code
if (protoNumber == 1) {
protocol = "icmp";
icmpCode = -1;
icmpType = -1;
} else if(protoNumber < 0 || protoNumber > 255){
throw new InvalidParameterValueException("Invalid protocol number: " + protoNumber);
}
} else {
//Protocol is not number
//Check for valid protocol strings
if (!NetUtils.isValidSecurityGroupProto(protocol)) {
throw new InvalidParameterValueException("Invalid protocol " + protocol);
}
}
if ("icmp".equalsIgnoreCase(protocol)) {
if (protocol.equals(NetUtils.ICMP_PROTO)) {
if ((icmpType == null) || (icmpCode == null)) {
throw new InvalidParameterValueException("Invalid ICMP type/code specified, icmpType = " + icmpType + ", icmpCode = " + icmpCode);
}
Expand All @@ -641,7 +659,7 @@ private List<SecurityGroupRuleVO> authorizeSecurityGroupRule(final Long security
}
startPortOrType = 0;
endPortOrCode = 0;
} else {
} else if (protocol.equals(NetUtils.TCP_PROTO) || protocol.equals(NetUtils.UDP_PROTO)) {
if ((startPort == null) || (endPort == null)) {
throw new InvalidParameterValueException("Invalid port range specified, startPort = " + startPort + ", endPort = " + endPort);
}
Expand All @@ -660,10 +678,13 @@ private List<SecurityGroupRuleVO> authorizeSecurityGroupRule(final Long security
}
startPortOrType = startPort;
endPortOrCode = endPort;
} else {
// in 4.6, the start port and end port are ignored in definition of ProtocolAclRule
// see core/src/com/cloud/agent/resource/virtualnetwork/facade/SetNetworkAclConfigItem.java
startPortOrType = 0;
endPortOrCode = 0;
}

protocol = protocol.toLowerCase();

List<SecurityGroupVO> authorizedGroups = new ArrayList<SecurityGroupVO>();
if (groupList != null) {
Collection userGroupCollection = groupList.values();
Expand Down
Loading