Twingate / terraform-provider-twingate

Mozilla Public License 2.0
42 stars 11 forks source link

should `DENY_ALL` really be `ALLOW_NONE` instead? #118

Closed tjstansell closed 1 year ago

tjstansell commented 2 years ago

Completely new user here and the use of the term DENY_ALL implies there's a deny action (possibly overriding an allow elsewhere), which I think has led to a little bit of confusion on my part how resources and policies work. It's my understanding after reading your docs is that you can only grant folks access to resources and not explicitly deny access. If you don't want a specific group of folks to access a resource, those folks cannot overlap with another group that does have access as users get a union of all permissions. Assuming that I'm reading and understanding all of this correctly, I wonder if it's less confusing if this option be called ALLOW_NONE instead, which is a bit more accurate.

Please correct me if my understanding is wrong! Just an initial thought on this as I become familiar with your product.

alexmensch commented 2 years ago

It's my understanding after reading your docs is that you can only grant folks access to resources and not explicitly deny access. If you don't want a specific group of folks to access a resource, those folks cannot overlap with another group that does have access as users get a union of all permissions.

Hi @tjstansell your understanding on that point is totally correct!

In this case, the DENY_ALL option only applies to port restrictions on the resource itself. The usage of DENY_ALL, ALLOW_ALL and RESTRICTED are modifiers on what ports are allowed for UDP and TCP traffic on a specific resource. Our Terraform documentation is relatively brief, but we have more details in the primary product documentation here. Currently the UI doesn't allow setting TCP/UDP/ICMP restrictions individually (this will be allowed in the UI very soon), but we do allow this via our public API and in our Terraform provider.

Let me know if this clears things up, and if you have any other questions. If you have some ideas on how we can clarify the Terraform docs, we'd be happy to do that as well, just let me know.

tjstansell commented 2 years ago

Thanks for the response. I was simply making an observation that when you get started with Twingate, especially when comparing it with other similar products where you define policies as an ordered set that can have both allow and deny rules in them, the use of the term DENY_ here is a bit confusing. You're not actually denying anything as that's not a concept you support ... you just aren't allowing any ports. I feel like you could have just easily supported the same feature set by not including a policy field at all, but instead just looking at ports where one of those could be the keyword ALL.

Taking the example ...

  protocols {
    allow_icmp = true
    tcp  {
      policy = "RESTRICTED"
      ports = ["80", "82-83"]
    }
    udp {
      policy = "ALLOW_ALL"
    }
  }

It could have just as easily been represented by

  protocols {
    allow_icmp = true
    tcp  {
      ports = ["80", "82-83"]
    }
    udp {
      ports = ["ALL"]
    }
  }

And there would be no question that you're only allowing specific ports.

If you only wanted to allow specific ports on tcp only, you could do:

  protocols {
    allow_icmp = true
    tcp  {
      ports = ["80", "82-83"]
    }
    udp {
      ports = []
    }
  }

Perhaps leaving the policy field makes sense if there's some desire to be able to support an actual DENY concept in the future. Hopefully that helps clarify my point. You could also probably clarify this by explicitly stating in the docs that policy = "DENY_ALL" is just a shortcut for policy = "RESTRICTED" with ports = [].

alexmensch commented 2 years ago

Thanks @tjstansell -- this makes sense, and I like the clarity of your suggestion. This requires a schema change and/or some backwards compatibility, but I'll add this as a backlog item to consider once we've tackled the higher priorities issues that we've moving through right now (new data sources, etc.)

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.