Skip to content

Commit

Permalink
Refactor removal of redundant all-users rules; improve rule descriptions
Browse files Browse the repository at this point in the history
  • Loading branch information
@KyleKotowick
KyleKotowick committed Jul 18, 2024
1 parent f4ebeca commit 53b9a45
Show file tree
Hide file tree
Showing 11 changed files with 462 additions and 353 deletions.
2 changes: 2 additions & 0 deletions .terraform.lock.hcl
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

92 changes: 90 additions & 2 deletions README.md
View file
Original file line number Diff line number Diff line change
@@ -1,2 +1,90 @@
# terraform-aws-client-vpn-authorization-rules
Calculates and creates a minimal set of AWS Client VPN authorization rules that meet given requirements.
# Terraform AWS Client VPN Authorization Rule Calculator

This module takes a list of Client VPN authorization rules for zero or more groups and returns a minimal set of authorization rules that is functionally equivalent. This is useful for handling the Client VPN [longest prefix path](https://docs.aws.amazon.com/vpn/latest/clientvpn-admin/cvpn-working-rules.html) networking, which can lead to unexpected behaviour as documented [here](https://docs.aws.amazon.com/vpn/latest/clientvpn-admin/troubleshooting.html#ad-group-auth-rules). It also minimizes the number of authorization rules that need to be created by merging rules where possible and eliminating redundant rules.

Note that this module **does not** actually create the rules. The set of inputs for rules is returned, and the user can then create the rules or do something else with the result.

The module works by:
1. For each group (considering "all users" rules as a separate group), reduce the rules to the minimum equivalent set by merging CIDR blocks where possible (using the [Invicton-Labs/merge-cidrs/null](https://registry.terraform.io/modules/Invicton-Labs/merge-cidrs/null/latest) module).
2. Remove any rules where the same effect is achieved by an "all-users" rule, as they are redundant.
3. For each remaining rule, if a different group has a rule within the first rule's prefix (but with a longer prefix length), add a rule with the same prefix to the first group. This handles funny behaviour with longest-prefix evaluation.
4. Eliminate any duplicate rules within a single group that may have occurred if the same longer prefix rule was present in multiple other groups (a corresponding one would have been added for each).
5. Eliminate any rules within a group where other, longer-prefix rules within the same group combine to be functionally equivalent.


Example:
```terraform
module "vpn_authorization_rules" {
source = "Invicton-Labs/client-vpn-authorization-rules/aws"
authorization_rules = [
{
description = "Admin 1"
target_network_cidr = "1.0.0.0/16"
access_group_id = "admin"
},
{
description = "Everyone 1"
authorize_all_groups = true
target_network_cidr = "1.0.0.0/17"
},
{
description = "Dev 1"
target_network_cidr = "1.0.0.0/18"
access_group_id = "dev"
},
{
description = "Dev 2"
target_network_cidr = "1.0.128.0/19"
access_group_id = "dev"
},
{
description = "Dev 3"
target_network_cidr = "1.0.160.0/19"
access_group_id = "dev"
},
]
}
output "merged_authorization_rules" {
value = module.vpn_authorization_rules.merged_authorization_rules
}
```

```
$ terraform apply -auto-approve
Apply complete! Resources: 0 added, 0 changed, 0 destroyed.
Outputs:
Apply complete! Resources: 0 added, 0 changed, 0 destroyed.
Outputs:
merged_authorization_rules = {
"admin|1.0.0.0/16" = {
"access_group_id" = "admin"
"authorize_all_groups" = tobool(null)
"description" = "Admin 1 [1.0.0.0/16]"
"target_network_cidr" = "1.0.0.0/16"
}
"admin|1.0.128.0/18" = {
"access_group_id" = "admin"
"authorize_all_groups" = tobool(null)
"description" = "Admin 1 [1.0.0.0/16] (covering longer prefix path from \"Dev 2 [1.0.128.0/19]; Dev 3 [1.0.160.0/19]\")"
"target_network_cidr" = "1.0.128.0/18"
}
"dev|1.0.128.0/18" = {
"access_group_id" = "dev"
"authorize_all_groups" = tobool(null)
"description" = "Dev 2 [1.0.128.0/19]; Dev 3 [1.0.160.0/19]"
"target_network_cidr" = "1.0.128.0/18"
}
"eb8325b4-c0c4-4b49-962f-71a5949efb24|1.0.0.0/17" = {
"access_group_id" = tostring(null)
"authorize_all_groups" = true
"description" = "Everyone 1 [1.0.0.0/17]"
"target_network_cidr" = "1.0.0.0/17"
}
}
```
144 changes: 52 additions & 92 deletions main.tf
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ For each rule:
2. If there's nothing smaller within the CIDR or larger that covers the CIDR, AND there's an everyone rule that covers it, delete it (done in rules_with_everyone_duplicates_removed)
3. If there's a smaller one within the CIDR for another group, add that smaller one specifically (done in rules_with_additional_cidrs)
4. If a larger CIDR is entirely completed by smaller CIDRs that are required, delete the larger one (done in rules_with_unnecessary_larger_removed)
NOTE: we use ternary operators instead of &&/|| throughout because it enables lazy evaluation, thereby speeding up the process by not running unnecessary checks.
*/

// Start by reducing all rules within each group to the minimum set.
Expand Down Expand Up @@ -67,7 +69,7 @@ locals {
{
group_id = group_id
cidr = cidr_data.cidr
description = join(var.merged_rule_description_joiner, [for contained in cidr_data.contains : contained.metadata.description])
description = join(var.merged_rule_description_joiner, [for contained in cidr_data.contains : "${contained.metadata.description} [${contained.cidr}]"])
first_ip = cidrhost(cidr_data.cidr, 0)
last_ip = cidrhost(cidr_data.cidr, pow(2, 32 - tonumber(split("/", cidr_data.cidr)[1])) - 1)
}
Expand All @@ -86,76 +88,42 @@ locals {
]
}

// Add a field that indicates if this rule is covered by an everyone group rule,
// i.e. if an everyone rule would provide the same access if this rule didn't exist.
rules_with_everyone_meta = {
// Remove any rules that are covered by "everyone" rules. This is possible because:
// - any smaller rules that are subsets of a given rule will also be removed if the given rule
// is removed, as it will also be covered by the same everyone rule. Therefore, there is no
// need to duplicate the longer prefix path of that smaller rule.
// - any larger rule that subsumes a given rule, which is also covered by an everyone rule,
// will also be removed. Therefore, there is no reason to keep this smaller prefix path rule.
// - any larger rule that is larger than the everyone rule will not be considered in AWS routing
// evaluation, as the everyone rule will have a longer prefix length and will be considered first.
rules_with_everyone_duplicates_removed = contains(keys(local.rules_with_first_last_decimal), local.everyone_group_uuid) ? {
for group_id, rules in local.rules_with_first_last_decimal :
group_id => [
for rule in rules :
merge(rule, {
covered_by_everyone_rule = length([
for everyone_rule in local.rules_with_first_last_decimal[local.everyone_group_uuid] :
true
if(
rule.group_id != local.everyone_group_uuid ? (
everyone_rule.first_ip_decimal <= rule.first_ip_decimal ? (
everyone_rule.last_ip_decimal >= rule.last_ip_decimal
) : false
) : false
)
]) > 0
})
]
}

// Create a flat list of all rules across all groups.
all_rules_with_everyone_meta = concat(values(local.rules_with_everyone_meta)...)

// Remove any rules where the same access is granted by an everyone rule.
// There are many conditions to doing this, check the comments within.
rules_with_everyone_duplicates_removed = {
for group_id, rules in local.rules_with_everyone_meta :
group_id => [
for rule in rules :
rule
if(
// If it's an everyone group rule, it has to remain
group_id == local.everyone_group_uuid ||
// Always include it if it's not subsumed by an everyone rule
!rule.covered_by_everyone_rule ||
// OR, if there's any other CIDR from a different group that includes this one, or is a part of this one.
length([
for compare_rule in local.all_rules_with_everyone_meta :
true
if(
// The rule has to be for a different group to qualify
compare_rule.group_id != rule.group_id &&
// That other group can't be the everyone group, since we're considering deleting in favour of the everyone group rule
compare_rule.group_id != local.everyone_group_uuid &&
// The rule can't be one that is covered by an everyone rule, since we'd like to disappear that rule too.
!compare_rule.covered_by_everyone_rule &&
(
// The other rule subsumes this rule, OR
(compare_rule.first_ip_decimal <= rule.first_ip_decimal && compare_rule.last_ip_decimal >= rule.last_ip_decimal) ||
// The other rule is subsumed by this one
(rule.first_ip_decimal <= compare_rule.first_ip_decimal && rule.last_ip_decimal >= compare_rule.last_ip_decimal)
) &&
// There can't be an everyone rule that is the same size or smaller than the compare rule
length([
for everyone_rule in local.rules_with_first_last_decimal[local.everyone_group_uuid] :
true
if(
// The everyone rule has to subsume this rule
(everyone_rule.first_ip_decimal <= rule.first_ip_decimal && everyone_rule.last_ip_decimal >= rule.last_ip_decimal) &&
// The everyone rule has to be subsumed by the compare rule
(compare_rule.first_ip_decimal <= everyone_rule.first_ip_decimal && compare_rule.last_ip_decimal >= everyone_rule.last_ip_decimal)
)
]) == 0
)
]) > 0
group_id == local.everyone_group_uuid ? true : (
// Always include it if it's not subsumed by an everyone rule
length([
for everyone_rule in local.rules_with_first_last_decimal[local.everyone_group_uuid] :
true
if(
rule.group_id != local.everyone_group_uuid ? (
everyone_rule.first_ip_decimal <= rule.first_ip_decimal ? (
everyone_rule.last_ip_decimal >= rule.last_ip_decimal
) : false
) : false
)
]) == 0
)
)
]
}
// If there are no everyone rules, skip and move on
} : local.rules_with_first_last_decimal

// Create a flat list of all rules across all groups.
all_rules_with_everyone_meta = concat(values(local.rules_with_everyone_duplicates_removed)...)

// For each rule, add additional rules to match any longer-prefix rules for other groups.
rules_with_additional_cidrs = {
Expand All @@ -169,20 +137,23 @@ locals {
], [
for compare_rule in local.all_rules_with_everyone_meta :
merge(compare_rule, {
description = "${rule.description} (covering longest prefix path from \"${compare_rule.description}\")"
description = "${rule.description} (covering longer prefix path from \"${compare_rule.description}\")"
group_id = rule.group_id
extra_due_to_other_group = true
})
if(
// Only consider rules from other groups
compare_rule.group_id != rule.group_id &&
// That other group can't be the everyone group, since the everyone group would provide access
// to this group as well anyways.
compare_rule.group_id != local.everyone_group_uuid &&
// We don't need to add a duplicate of the compare rule for this rule if it's identical, since that wouldn't accomplish anything.
!(compare_rule.first_ip_decimal == rule.first_ip_decimal && compare_rule.last_ip_decimal == rule.last_ip_decimal) &&
// The compare rule needs to be subsumed by this rule (longer prefix length).
(rule.first_ip_decimal <= compare_rule.first_ip_decimal && rule.last_ip_decimal >= compare_rule.last_ip_decimal)
compare_rule.group_id != rule.group_id ? (
// That other group can't be the everyone group, since the everyone group would provide access
// to this group as well anyways.
compare_rule.group_id != local.everyone_group_uuid ? (
// We don't need to add a duplicate of the compare rule for this rule if it's identical, since that wouldn't accomplish anything.
!(compare_rule.first_ip_decimal == rule.first_ip_decimal ? compare_rule.last_ip_decimal == rule.last_ip_decimal : false) ? (
// The compare rule needs to be subsumed by this rule (longer prefix length).
(rule.first_ip_decimal <= compare_rule.first_ip_decimal ? rule.last_ip_decimal >= compare_rule.last_ip_decimal : false)
) : false
) : false
) : false
)
])
])
Expand Down Expand Up @@ -230,14 +201,15 @@ locals {
rule
if(
// Keep it if it had to be added due to a smaller prefix in another group
rule.extra_due_to_other_group ||
(
// Keep it if none of the merged CIDRs of required added rules subsume this CIDR
length([
for compare_rule in module.merge_cidr_for_redundancy_check.merged_cidr_sets_ipv4_with_meta[group_id] :
true
if(compare_rule.first_ip_decimal <= rule.first_ip_decimal && compare_rule.last_ip_decimal >= rule.last_ip_decimal)
]) == 0
rule.extra_due_to_other_group ? true : (
(
// Keep it if none of the merged CIDRs of required added rules subsume this CIDR
length([
for compare_rule in module.merge_cidr_for_redundancy_check.merged_cidr_sets_ipv4_with_meta[group_id] :
true
if(compare_rule.first_ip_decimal <= rule.first_ip_decimal ? compare_rule.last_ip_decimal >= rule.last_ip_decimal : false)
]) == 0
)
)
)
]
Expand All @@ -253,16 +225,4 @@ locals {
description = rule.description
}
}

# TODO: replace and/or with ternary
}

// If desired, create the rules
# resource "aws_ec2_client_vpn_authorization_rule" "this" {
# for_each = var.create_rules ? local.all_rules : {}
# client_vpn_endpoint_id = var.client_vpn_endpoint_id
# target_network_cidr = each.value.target_network_cidr
# authorize_all_groups = each.value.authorize_all_groups
# access_group_id = each.value.access_group_id
# description = each.value.description
# }
21 changes: 5 additions & 16 deletions outputs.tf
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,6 @@ output "authorization_rules" {
description = "The value of the `authorization_rules` input variable."
value = var.authorization_rules
}
output "client_vpn_endpoint_id" {
description = "The value of the `client_vpn_endpoint_id` input variable."
value = var.client_vpn_endpoint_id
}
output "merged_rule_description_joiner" {
description = "The value of the `merged_rule_description_joiner` input variable, or the default value if the input was `null`."
value = var.merged_rule_description_joiner
Expand All @@ -21,10 +17,6 @@ output "merged_authorization_rules" {
description = "The reduced/merged inputs that can/will be used to create the actual rules."
value = local.all_rules
}
# output "authorization_rule_resources" {
# description = "The aws_ec2_client_vpn_authorization_rule resources that were created, if the `create_rules` input variable was `true` (otherwise, `null`)."
# value = var.create_rules ? aws_ec2_client_vpn_authorization_rule.this : null
# }

//==================================================
// Debugging outputs
Expand All @@ -44,21 +36,18 @@ output "merged_authorization_rules" {
# output "_05_rules_with_first_last_decimal" {
# value = local.rules_with_first_last_decimal
# }
# output "_06_rules_with_everyone_meta" {
# value = local.rules_with_everyone_meta
# }
# output "_07_rules_with_everyone_duplicates_removed" {
# output "_06_rules_with_everyone_duplicates_removed" {
# value = local.rules_with_everyone_duplicates_removed
# }
# output "_08_rules_with_additional_cidrs" {
# output "_07_rules_with_additional_cidrs" {
# value = local.rules_with_additional_cidrs
# }
# output "_09_rules_with_additional_cidrs_distinct" {
# output "_08_rules_with_additional_cidrs_distinct" {
# value = local.rules_with_additional_cidrs_distinct
# }
# output "_10_merge_cidr_for_redundancy_check" {
# output "_09_merge_cidr_for_redundancy_check" {
# value = module.merge_cidr_for_redundancy_check.merged_cidr_sets_ipv4_with_meta
# }
# output "_11_rules_with_unnecessary_larger_removed" {
# output "_10_rules_with_unnecessary_larger_removed" {
# value = local.rules_with_unnecessary_larger_removed
# }
24 changes: 0 additions & 24 deletions tests/.terraform.lock.hcl
View file

This file was deleted.

Loading

0 comments on commit 53b9a45

Please sign in to comment.