cidr: removing exceptions

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

source/common/listener_manager/filter_chain_manager_impl.cc

@@ -194,7 +194,8 @@ void FilterChainManagerImpl::addFilterChains(
         std::vector<std::string> ips;
         ips.reserve(prefix_ranges.size());
         for (const auto& ip : prefix_ranges) {
-          const auto& cidr_range = Network::Address::CidrRange::create(ip);
+          const auto& cidr_range = THROW_OR_RETURN_VALUE(Network::Address::CidrRange::create(ip),
+                                                         Network::Address::CidrRange);
           ips.push_back(cidr_range.asString());
         }
         return ips;
@@ -457,15 +458,18 @@ std::pair<T, std::vector<Network::Address::CidrRange>> makeCidrListEntry(const s
   std::vector<Network::Address::CidrRange> subnets;
   if (cidr == EMPTY_STRING) {
     if (Network::SocketInterfaceSingleton::get().ipFamilySupported(AF_INET)) {
-      subnets.push_back(
-          Network::Address::CidrRange::create(Network::Utility::getIpv4CidrCatchAllAddress()));
+      subnets.push_back(THROW_OR_RETURN_VALUE(
+          Network::Address::CidrRange::create(Network::Utility::getIpv4CidrCatchAllAddress()),
+          Network::Address::CidrRange));
     }
     if (Network::SocketInterfaceSingleton::get().ipFamilySupported(AF_INET6)) {
-      subnets.push_back(
-          Network::Address::CidrRange::create(Network::Utility::getIpv6CidrCatchAllAddress()));
+      subnets.push_back(THROW_OR_RETURN_VALUE(
+          Network::Address::CidrRange::create(Network::Utility::getIpv6CidrCatchAllAddress()),
+          Network::Address::CidrRange));
     }
   } else {
-    subnets.push_back(Network::Address::CidrRange::create(cidr));
+    subnets.push_back(THROW_OR_RETURN_VALUE(Network::Address::CidrRange::create(cidr),
+                                            Network::Address::CidrRange));
   }
   return std::make_pair<T, std::vector<Network::Address::CidrRange>>(T(data), std::move(subnets));
 }

source/common/network/cidr_range.cc

@@ -99,30 +99,42 @@ std::string CidrRange::asString() const {
 }
 
 // static
-CidrRange CidrRange::create(InstanceConstSharedPtr address, int length) {
+absl::StatusOr<CidrRange>
+CidrRange::create(InstanceConstSharedPtr address, int length,
+                  absl::optional<absl::string_view> original_address_str) {
   InstanceConstSharedPtr ptr = truncateIpAddressAndLength(std::move(address), &length);
-  return {std::move(ptr), length};
+  if (!ptr) {
+    return absl::InvalidArgumentError(absl::StrCat(
+        "malformed IP address: ", original_address_str.has_value() ? *original_address_str : ""));
+  }
+  CidrRange ret = CidrRange(std::move(ptr), length);
+  if (ret.isValid()) {
+    return ret;
+  }
+  return absl::InvalidArgumentError("Invalid CIDR range");
 }
 
 // static
-CidrRange CidrRange::create(const std::string& address, int length) {
-  return create(Utility::parseInternetAddress(address), length);
+absl::StatusOr<CidrRange> CidrRange::create(const std::string& address, int length) {
+  return create(Utility::parseInternetAddressNoThrow(address), length, address);
 }
 
-CidrRange CidrRange::create(const envoy::config::core::v3::CidrRange& cidr) {
-  return create(Utility::parseInternetAddress(cidr.address_prefix()), cidr.prefix_len().value());
+absl::StatusOr<CidrRange> CidrRange::create(const envoy::config::core::v3::CidrRange& cidr) {
+  return create(Utility::parseInternetAddressNoThrow(cidr.address_prefix()),
+                cidr.prefix_len().value(), cidr.address_prefix());
 }
 
-CidrRange CidrRange::create(const xds::core::v3::CidrRange& cidr) {
-  return create(Utility::parseInternetAddress(cidr.address_prefix()), cidr.prefix_len().value());
+absl::StatusOr<CidrRange> CidrRange::create(const xds::core::v3::CidrRange& cidr) {
+  return create(Utility::parseInternetAddressNoThrow(cidr.address_prefix()),
+                cidr.prefix_len().value(), cidr.address_prefix());
 }
 
 // static
-CidrRange CidrRange::create(const std::string& range) {
+absl::StatusOr<CidrRange> CidrRange::create(const std::string& range) {
   const auto parts = StringUtil::splitToken(range, "/");
   if (parts.size() == 2) {
-    InstanceConstSharedPtr ptr = Utility::parseInternetAddress(std::string{parts[0]});
-    if (ptr->type() == Type::Ip) {
+    InstanceConstSharedPtr ptr = Utility::parseInternetAddressNoThrow(std::string{parts[0]});
+    if (ptr && ptr->type() == Type::Ip) {
       uint64_t length64;
       if (absl::SimpleAtoi(parts[1], &length64)) {
         if ((ptr->ip()->version() == IpVersion::v6 && length64 <= 128) ||
@@ -132,7 +144,7 @@ CidrRange CidrRange::create(const std::string& range) {
       }
     }
   }
-  return {nullptr, -1};
+  return absl::InvalidArgumentError("Invalid CIDR range");
 }
 
 // static
@@ -202,12 +214,13 @@ IpList::create(const Protobuf::RepeatedPtrField<envoy::config::core::v3::CidrRan
   }
   return ret;
 }
+
 IpList::IpList(const Protobuf::RepeatedPtrField<envoy::config::core::v3::CidrRange>& cidrs) {
   ip_list_.reserve(cidrs.size());
   for (const envoy::config::core::v3::CidrRange& entry : cidrs) {
-    CidrRange list_entry = CidrRange::create(entry);
-    if (list_entry.isValid()) {
-      ip_list_.push_back(std::move(list_entry));
+    absl::StatusOr<CidrRange> range_or_error = CidrRange::create(entry);
+    if (range_or_error.status().ok()) {
+      ip_list_.push_back(std::move(range_or_error.value()));
     } else {
       error_ = fmt::format("invalid ip/mask combo '{}/{}' (format is <ip>/<# mask bits>)",
                            entry.address_prefix(), entry.prefix_len().value());

source/common/network/cidr_range.h

@@ -68,43 +68,34 @@ class CidrRange {
   std::string asString() const;
 
   /**
-   * @return true if this instance is valid; address != nullptr && length is appropriate for the
-   *         IP version (these are checked during construction, and reduced down to a check of
-   *         the length).
-   */
-  bool isValid() const { return length_ >= 0; }
-
-  /**
-   * TODO(ccaraman): Update CidrRange::create to support only constructing valid ranges.
    * @return a CidrRange instance with the specified address and length, modified so that the only
    *         bits that might be non-zero are in the high-order length bits, and so that length is
    *         in the appropriate range (0 to 32 for IPv4, 0 to 128 for IPv6). If the address or
    *         length is invalid, then the range will be invalid (i.e. length == -1).
    */
-  static CidrRange create(InstanceConstSharedPtr address, int length);
-  static CidrRange create(const std::string& address, int length);
+  static absl::StatusOr<CidrRange>
+  create(InstanceConstSharedPtr address, int length,
+         absl::optional<absl::string_view> original_address_str = "");
+  static absl::StatusOr<CidrRange> create(const std::string& address, int length);
 
   /**
    * Constructs an CidrRange from a string with this format (same as returned
    * by CidrRange::asString above):
    *      <address>/<length>    e.g. "10.240.0.0/16" or "1234:5678::/64"
-   * TODO(ccaraman): Update CidrRange::create to support only constructing valid ranges.
    * @return a CidrRange instance with the specified address and length if parsed successfully,
    *         else with no address and a length of -1.
    */
-  static CidrRange create(const std::string& range);
+  static absl::StatusOr<CidrRange> create(const std::string& range);
 
   /**
    * Constructs a CidrRange from envoy::config::core::v3::CidrRange.
-   * TODO(ccaraman): Update CidrRange::create to support only constructing valid ranges.
    */
-  static CidrRange create(const envoy::config::core::v3::CidrRange& cidr);
+  static absl::StatusOr<CidrRange> create(const envoy::config::core::v3::CidrRange& cidr);
 
   /**
    * Constructs a CidrRange from xds::core::v3::CidrRange.
-   * TODO(ccaraman): Update CidrRange::create to support only constructing valid ranges.
    */
-  static CidrRange create(const xds::core::v3::CidrRange& cidr);
+  static absl::StatusOr<CidrRange> create(const xds::core::v3::CidrRange& cidr);
 
   /**
    * Given an IP address and a length of high order bits to keep, returns an address
@@ -120,6 +111,13 @@ class CidrRange {
                                                            int* length_io);
 
 private:
+  /**
+   * @return true if this instance is valid; address != nullptr && length is appropriate for the
+   *         IP version (these are checked during construction, and reduced down to a check of
+   *         the length).
+   */
+  bool isValid() const { return length_ >= 0; }
+
   CidrRange(InstanceConstSharedPtr address, int length);
 
   InstanceConstSharedPtr address_;

source/common/router/router_ratelimit.cc

@@ -170,8 +170,10 @@ bool MaskedRemoteAddressAction::populateDescriptor(RateLimit::DescriptorEntry& d
   }
 
   // TODO: increase the efficiency, avoid string transform back and forth
+  // Note: we don't do validity checking for CIDR range here because we know
+  // from addressAsString this is a valid address.
   Network::Address::CidrRange cidr_entry =
-      Network::Address::CidrRange::create(remote_address->ip()->addressAsString(), mask_len);
+      *Network::Address::CidrRange::create(remote_address->ip()->addressAsString(), mask_len);
   descriptor_entry = {"masked_remote_address", cidr_entry.asString()};
 
   return true;

source/extensions/common/matcher/trie_matcher.h

@@ -144,7 +144,9 @@ class TrieMatcherFactoryBase : public ::Envoy::Matcher::CustomMatcherFactory<Dat
       for (const auto& range : range_matcher.ranges()) {
         TrieNode<DataType> node = {i, range.prefix_len().value(), range_matcher.exclusive(),
                                    on_match};
-        data.push_back({node, {Network::Address::CidrRange::create(range)}});
+        data.push_back({node,
+                        {THROW_OR_RETURN_VALUE(Network::Address::CidrRange::create(range),
+                                               Network::Address::CidrRange)}});
       }
     }
     auto lc_trie = std::make_shared<Network::LcTrie::LcTrie<TrieNode<DataType>>>(data);

source/extensions/filters/common/rbac/matchers.h

@@ -159,7 +159,9 @@ class IPMatcher : public Matcher {
   enum Type { ConnectionRemote = 0, DownstreamLocal, DownstreamDirectRemote, DownstreamRemote };
 
   IPMatcher(const envoy::config::core::v3::CidrRange& range, Type type)
-      : range_(Network::Address::CidrRange::create(range)), type_(type) {}
+      : range_(THROW_OR_RETURN_VALUE(Network::Address::CidrRange::create(range),
+                                     Network::Address::CidrRange)),
+        type_(type) {}
 
   bool matches(const Network::Connection& connection, const Envoy::Http::RequestHeaderMap& headers,
                const StreamInfo::StreamInfo& info) const override;

source/extensions/filters/common/rbac/matchers/upstream_ip_port.cc

@@ -23,7 +23,8 @@ UpstreamIpPortMatcher::UpstreamIpPortMatcher(
   }
 
   if (proto.has_upstream_ip()) {
-    cidr_ = Network::Address::CidrRange::create(proto.upstream_ip());
+    cidr_ = THROW_OR_RETURN_VALUE(Network::Address::CidrRange::create(proto.upstream_ip()),
+                                  Network::Address::CidrRange);
   }
   if (proto.has_upstream_port_range()) {
     port_ = proto.upstream_port_range();

source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc

@@ -38,10 +38,10 @@ IpTaggingFilterConfig::IpTaggingFilterConfig(
     cidr_set.reserve(ip_tag.ip_list().size());
     for (const envoy::config::core::v3::CidrRange& entry : ip_tag.ip_list()) {
 
-      // Currently, CidrRange::create doesn't guarantee that the CidrRanges are valid.
-      Network::Address::CidrRange cidr_entry = Network::Address::CidrRange::create(entry);
-      if (cidr_entry.isValid()) {
-        cidr_set.emplace_back(std::move(cidr_entry));
+      absl::StatusOr<Network::Address::CidrRange> cidr_or_error =
+          Network::Address::CidrRange::create(entry);
+      if (cidr_or_error.status().ok()) {
+        cidr_set.emplace_back(std::move(cidr_or_error.value()));
       } else {
         throw EnvoyException(
             fmt::format("invalid ip/mask combo '{}/{}' (format is <ip>/<# mask bits>)",

source/extensions/matching/input_matchers/ip/config.cc

@@ -19,13 +19,8 @@ Config::createInputMatcherFactoryCb(const Protobuf::Message& config,
   for (const auto& cidr_range : cidr_ranges) {
     const std::string& address = cidr_range.address_prefix();
     const uint32_t prefix_len = cidr_range.prefix_len().value();
-    const auto range = Network::Address::CidrRange::create(address, prefix_len);
-    // We only assert that the range is valid because:
-    // * if "address" can't be parsed, it will throw an EnvoyException
-    // * prefix_len can't be < 0 as per the protobuf definition as an uint32_t
-    // * if prefix_len is too big, CidrRange::create clamps it to a valid value
-    // => it is thus not possible to create an invalid range.
-    ASSERT(range.isValid(), "address range should be valid!");
+    const auto range = THROW_OR_RETURN_VALUE(
+        Network::Address::CidrRange::create(address, prefix_len), Network::Address::CidrRange);
     ranges.emplace_back(std::move(range));
   }