fortinetdev / terraform-provider-fortios

Terraform Fortios provider
https://www.terraform.io/docs/providers/fortios/
Mozilla Public License 2.0
67 stars 50 forks source link

fortios_firewall_localinpolicy order is indeterminate #247

Open timwsuqld opened 1 year ago

timwsuqld commented 1 year ago

Creating multiple fortios_firewall_localinpolicy rules will end up with a different order based on different runs.

Policy ID doesn't determine rule order, execution order determines order. And just because they are sequential in terraform config, doesn't mean they'll be executed as such.

2 rules such as (simplified) can end up with rule 2 being created before rule 1, so the deny occurs first.

resource "fortios_firewall_localinpolicy" "allow_whitelist_management_access" {
  action   = "accept"
  intf     = fortios_system_interface.wan.name
  policyid = 1
  schedule = "always"
  status   = "enable"
  comments = "Allow access to management ports for Whitelisted IPs"

  dstaddr {
    name = "all"
  }

  service {
    name = fortios_firewallservice_custom.management_https.name
  }

  srcaddr {
    name = fortios_firewall_addrgrp.trusted_management_clients.name
  }
}

resource "fortios_firewall_localinpolicy" "block_management_access" {
  action            = "deny"
  intf              = fortios_system_interface.wan.name
  policyid          = 2
  schedule          = "always"
  status            = "enable"
  comments = "Deny everyone else to management ports"

  dstaddr {
    name = "all"
  }

  service {
    name = fortios_firewallservice_custom.management_https.name
  }

  srcaddr {
    name = "all"
  }
}

On 1 router, where the rule order was forced (created policy 1, then updated the config to include policy 2 and ran again) the order came out as

# show firewall local-in-policy 
config firewall local-in-policy
    edit 1
        set uuid 7c223d90-45f5-51ed-da77-d31138bfff4e
        set intf "wan"
        set srcaddr "trusted_management_clietns"
        set dstaddr "all"
        set action accept
        set service "Management HTTPS"
        set schedule "always"
        set comments "Allow access to management ports for Whitelisted IPs"
    next
    edit 2
        set uuid 9c91836e-45f6-51ed-6d21-e61477042d8a
        set intf "wan"
        set srcaddr "all"
        set dstaddr "all"
        set service "Management HTTPS"
        set schedule "always"
        set comments "Deny everyone else to management ports"
    next
end

On another router where the whole config was applied at once, the rules ended up as

# show firewall local-in-policy 
config firewall local-in-policy
    edit 2
        set uuid 9f2857fe-45fd-51ed-2874-91c411f98615
        set intf "wan"
        set srcaddr "all"
        set dstaddr "all"
        set service "Management HTTPS"
        set schedule "always"
        set comments "Deny everyone else to management ports"
    next
    edit 1
        set uuid 9f386374-45fd-51ed-1127-1a4be0729fa0
        set intf "wan"
        set srcaddr "trusted_management_clietns"
        set dstaddr "all"
        set action accept
        set service "Management HTTPS"
        set schedule "always"
        set comments "Allow access to management ports for Whitelisted IPs"
    next
end

Ideally we need a way to specify the order, and also to reorder them.

lix-fortinet commented 1 year ago

Hi @timwsuqld,

Thank you for raising this issue. Could you try resource fortios_firewall_security_policysort which could sort the policy? Here is the reference: https://registry.terraform.io/providers/fortinetdev/fortios/latest/docs/resources/fortios_firewall_security_policysort

Thanks, Xing

drs143 commented 1 year ago

@lix-fortinet wouldn't it be possible to simplify the process of organizing the policies by adding another field to the policy resource (e.g. priority) determining it's position? That "policysort" thing really discourage people from using/managing FG by Terraform. I am still using Azure firewall instead exactly for that reason.

lix-fortinet commented 1 year ago

Hi @drs143,

Thank you for your suggestion. We will consider it and make some improvement.

Thanks, Xing

timwsuqld commented 1 year ago

I totally agree with @drs143, having a priority or position parameter is what people expect in states like this. It's actually already confusing that setting policyid doesn't set the order, as people expect that an ID like that would determine the order, and is how many routers have worked for the last 20 years, order is based on the ID of the policy. Yes, this means reordering policies changes the ID, so having a priority or position parameter would be even better. I've not had a chance to try fortios_firewall_security_policysort yet.