Skip to content

Added check for egress rule to allow traffic on all ports in MS NSG. #300

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
merged 5 commits into from
Feb 28, 2025
Merged

Added check for egress rule to allow traffic on all ports in MS NSG. #300

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6f73111
Added check for egress rule to allow traffic on all ports in Managed …
ManviGupta-prog Jan 7, 2025
01b7864
Fixed comments
ManviGupta-prog Jan 20, 2025
f306213
Fixed comments
ManviGupta-prog Feb 4, 2025
0a0664e
Fixed comments
ManviGupta-prog Feb 4, 2025
3553e25
Minor change to follow naming conventions.
ManviGupta-prog Feb 4, 2025
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
57 changes: 56 additions & 1 deletion terraform/modules/network-validator/scripts/network_validation.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,52 @@ function check_udp_port_open_in_seclist_or_nsg() {
echo 1
fi
}
Copy link
Member

Choose a reason for hiding this comment

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

The copyright header needs to change to
Copyright (c) 2023, 2025, Oracle and/or its affiliates.

# Checks if there is an egress rule to ensure that the network can establish outbound communication to any destination, utilizing all available protocols and ports.
Args:
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to use the same format for function comments used in other functions.
In addition to that, looks like this line does not begin with comment sign (#) . Doesn't this cause an error when the script is run?

# nsg_ocid: OCID for the nsg.
Copy link
Member

Choose a reason for hiding this comment

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

I see the code expects two parameters:

  • seclist_or_nsg_ocid
  • ocid_type
    and here I see only one listed

# Returns:
# 0|1
function check_egress_all_traffic_in_nsg() {
Copy link
Member

Choose a reason for hiding this comment

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

I see you renamed the function in your second commit, to remove the seclist part from the function, and leave nsg only,
But I see the code still supports sect lists.
Why was this done? is this intended?

local seclist_or_nsg_ocid=$1
local ocid_type=$2
local icmp_protocol="1"
local egress_is_open=false
Copy link
Member

Choose a reason for hiding this comment

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

is this variable really needed? I see is declared here, and is set to true in line 383, and then the code returns in line 385. Si, this variable is never read. Unless I am missing something

Copy link
Member

Choose a reason for hiding this comment

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

I mean, variable in line 359:
local egress_is_open=false

local egress_rules_count=0
declare -A nsg_sec_list_array

if [[ $ocid_type == "nsg" ]]; then
egress_rules=$(oci network nsg rules list --nsg-id $seclist_or_nsg_ocid --direction EGRESS | jq -r '.data')
else
egress_rules=$(oci network security-list get --security-list-id $seclist_or_nsg_ocid | jq -r '.data["egress-security-rules"]')
fi

egress_rules_count=$(echo $egress_rules | jq '. | length')

if [[ $egress_rules_count -gt 0 ]]; then
for ((j = 0; j < egress_rules_count; j++)); do
egress_protocol=$(echo $egress_rules | jq -r --arg i "$j" '.[$i|tonumber].protocol')
egress_destination=$(echo $egress_rules | jq -r --arg i "$j" '.[$i|tonumber].destination')
egress_destination_type=$(echo $egress_rules | jq -r --arg i "$j" '.[$i|tonumber]."destination-type"')

if [[ $egress_destination_type != "CIDR_BLOCK" ]]; then
nsg_sec_list_array[$j]="WARNING: Source type is either NSG or Service. Skipping the validation check for ${egress_destination}."
Copy link
Member

Choose a reason for hiding this comment

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

should this be destination type or target type (I do not know the correct names) instead of source type? we are checking they type of egress destination type

continue
fi

if [[ $egress_destination == "0.0.0.0/0" && ( $egress_protocol == "all" || $egress_protocol == $icmp_protocol ) ]]; then
egress_is_open=true
echo 0
return
fi
done
fi

if [[ ${#nsg_sec_list_array[@]} != 0 ]]; then
echo "${nsg_sec_list_array[@]}"
else
echo 1
fi
}


####################################################
Expand Down Expand Up @@ -852,7 +898,16 @@ fi
if [[ -n ${WLS_SUBNET_OCID} && -n ${ADMIN_SRV_NSG_OCID} && -n ${MANAGED_SRV_NSG_OCID} ]]
then
wls_subnet_cidr_block=$(oci network subnet get --subnet-id ${WLS_SUBNET_OCID} | jq -r '.data["cidr-block"]')

# Check if egress rule to allow traffic on all ports in Managed Server NSG.
res=$(check_egress_all_traffic_in_nsg ${MANAGED_SRV_NSG_OCID} "nsg")
if [[ $res == *"WARNING"* ]]; then
for warning in "${res[@]}"; do
echo "$warning"
done
elif [[ $res -ne 0 ]]; then
echo "ERROR: Missing egress rule to allow traffic on all ports in Managed Server NSG [$MANAGED_SRV_NSG_OCID]. ${NETWORK_VALIDATION_MSG}"
validation_return_code=2
fi
# Check if SSH port is open for access by WLS subnet CIDR in Admin Server NSG
res=$(check_tcp_port_open_in_seclist_or_nsg $MANAGED_SRV_NSG_OCID "${SSH_PORT}" "$wls_subnet_cidr_block" "nsg")
if [[ $res == *"WARNING"* ]]
Expand Down