Validation doesn't catch CIDR blocks that Azure NSG won't accept

[phealy]

What happened:

An entry in loadbalancerSourceRanges was added that contained bits set in the host portion of the address (example: 10.0.0.1/24 instead of 10.0.0.0/24). Kubernetes accepts this because the validation on the service object is just checking for a properly formatted CIDR block. Azure NSG rules will not accept this because it's a host address rather than a network address, and this breaks the reconcile loop for the service. This prevents any further updates from occurring until the bad address entry is removed.

An example error in the event is as follows:

Error syncing load balancer: failed to ensure load balancer: Retriable: false, RetryAfter: 0s, HTTPStatusCode: 400, RawError: {
  "error": {
    "code": "SecurityRuleInvalidAddressPrefix",
    "message": "Security rule /subscriptions/SUBSCRIPTION/resourceGroups/RESOURCEGROUP/providers/Microsoft.Network/networkSecurityGroups/aks-agentpool-16933646-nsg/securityRules/a94f2d0ba146146b2be9e9b3d753d449-TCP-80-10.0.0.1_24 has invalid Address prefix. Value provided: 10.0.0.1/24",
    "details": []
  }
}

What you expected to happen:

cloud-provider-azure should have validation to make sure that the host portion of the address is valid and skip that one entry from loadBalancerSourceRanges when creating the NSG rules (with an error event logged to Kubernetes), so that other updates are not blocked. All other updates should be applied.

EDIT: The default deny rule should still be created even if all of the entries in loadBalancerSourceRanges fail validation - this ensures that we fail-secure by not leaving access open the customer wanted to close off. This addresses @jwtty's point here about making sure that we don't leave something unrestricted when it shouldn't be.

How to reproduce it (as minimally and precisely as possible):

Add the following to a LoadBalancer service:

loadBalancerSourceRanges:
- 10.0.0.1/24

Anything else we need to know?:

Here is some quick go sample code to check if the CIDR block will be accepted by Azure networking:

package main

import (
  "fmt"
  "net"
  "os"
  "strings"
)

func main() {
  for i := 1; i < len(os.Args); i++ {
    ipAddr, ipNet, err := net.ParseCIDR(os.Args[i])
    if err != nil {
      fmt.Printf("%s failed to parse as a valid CIDR block\n", os.Args[i])
    }

    ipNetworkAddress := net.ParseIP(strings.Split(ipNet.String(), "/")[0])
    if ipAddr.Equal(ipNetworkAddress) {
      fmt.Printf("%s is a valid network CIDR block\n", os.Args[i])
    } else {
      fmt.Printf("%s is not a valid network CIDR block\n", os.Args[i])
    }
  }
}

$ go build ip.go && ./ip 10.0.0.0/24 10.0.0.1/24 fe80::/64 fe80::1/64
10.0.0.0/24 is a valid network CIDR block
10.0.0.1/24 is not a valid network CIDR block
fe80::/64 is a valid network CIDR block
fe80::1/64 is not a valid network CIDR block

Environment:

• Kubernetes version (use kubectl version): checked in AKS 1.27 and 1.28
• Cloud provider or hardware configuration: AKS
• OS (e.g: cat /etc/os-release): n/a
• Kernel (e.g. uname -a): n/a
• Install tools: n/a
• Network plugin and version (if this is a network-related bug): n/a
• Others: n/a

[jwtty]

Hi @phealy, I'm not sure if continuing the NSG deployment with just an event is a good idea. cx may just look at their service, see it's working without noticing the error event. Ignoring the "bad" network prefixes may generate an nsg rule that allows Internet access and can pose security issue. I'd rather make a validation in controller manager and report error before sending it to NRP.

[phealy]

Ignoring the "bad" network prefixes may generate an nsg rule that allows Internet access and can pose security issue.

@jwtty I'll edit my original comment to be clearer about this case - I'm not suggesting that we skip adding the NSG rules entirely if an entry is malformed; I'm saying we add all of the rules except the one that's bad, especially including the default deny at the end. This way we fail-secure - if any entry is bad, the service ends up more locked down than the customer wants, not less. If all of the entries are malformed, then the service ends up with no access allowed. The cx would likely notice the failure immediately because the source they tried to add isn't actually allowed access. This still isn't great for troubleshooting if they don't see the event, but at least it's failing secure instead of now where it can fail either way depending on the state prior to the update.

Consider this scenario for why I'd prefer to do it this way:

1. Customer deploys a service without loadBalancerSourceRanges, so it's wide open to the internet.
2. Customer realizes they need to add a source range list and restrict it.
3. When the list is added, one of the entries is wrong and blocks the updates entirely.
4. The service is still there and still works but isn't protected in any way because the NSG update never went through, and the only output was a log event. This means that the default deny rule was never added and the service is still wide open to the internet. Additionally, other updates continue to fail until they figure out what's wrong.

I do agree that validating up front would be a much better user experience, so what if we do both options? Let's fix the validation here to fail gracefully and securely and add a validating webhook in AKS to reject the update up front with an immediate failure and clear error message. That way non-AKS systems that are using cloud-provider-azure without our webhook still have good failure behavior, but AKS customers (or anyone else who wants to add a validating webhook for this) get better validation up front and an immediate rejection. I don't know if we'd get support upstream for adding this validation in kube-controller-manager - the restriction on not using a host address in the rule is likely not applicable to all clouds and thus adding that restriction in could break other environments where that sort of setup is allowed.

What do you think?

[jwtty]

As per discussion, we can include the correct network prefix in the route table rule and generate an event informing this.

[phealy]

@jwtty Sorry, I have to reverse myself on this - we need to make sure to fail secure by rejecting the bad entry and still including the deny rule, even if all of the entries are malformed (have host bits set). I was thinking about the possible failure cases of correcting this automatically and realized there's a really bad one - when the mistake is in the prefix mask, rather than the IP address portion, we can't assume intent.

Example 1: customer sets 10.240.0.1/24, which we mask and set as 10.240.0.0/24. There are two possible interpretations of this:

• The mistake was in the address portion of the CIDR block and the customer meant to set 10.240.0.0/24, and we've correctly interpreted their intent and opened the service to the subnet.
• The mistake was in the prefix portion of the CIDR block and the customer meant to set 10.240.0.1/32, and we've just opened up the service to 254 IP addresses that shouldn't have access.

Example 2: customer tries to set their VNet CIDR of 10.240.0.0/16 but accidentally drops the last digit on the prefix mask, so they actually set 10.240.0.0/1.

• If we automatically mask the address and log an event, the end result in the NSG rule is 0.0.0.0/1 - so instead of opening up their service to their VNet CIDR, we've opened it to half the internet (0.0.0.0-127.255.255.255) ❗

Given that failure case, can we please adjust the behavior to something like what I originally described, where we log an error/emit an event about the malformed CIDR and skip adding it to the allow rule, but still add everything else including the deny rule? A few examples to make sure we're on the same page:

•  loadBalancerSourceRanges:
   - 172.16.0.0/12
   - 10.0.0.1/24
  

  End result: Allow rule is in place with 172.16.0.0/12 and deny all rule is in place. 10.0.0.1/24 is logged in an event as a mis-masked CIDR and does not have access.

•  loadBalancerSourceRanges:
   - 10.0.0.1/24
  

  End result: no allow rule is in place but the deny rule is added; nothing is allowed access. Event is logged.

Let's discuss internally about add front-end validation for this via a webhook.