Skip to content

[Bug]: ACL Interface Assignment Should Be Unique per Interface and Direction #258

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pheus opened this issue Mar 25, 2025 · 4 comments
Open

Comments

@pheus
Copy link
Contributor

pheus commented Mar 25, 2025

NetBox access-list plugin version

v1.8.1

NetBox version

v4.2.5

Steps to Reproduce

  1. Create a Device (with its dependencies).
  2. Create an Interface for the Device.
  3. Create an Access List "ACL1" (type: "standard") bound to the Device.
  4. Create an ACL Interface Assignment for the Interface with direction set to "ingress."
  5. Create another Access List "ACL2" (type: "standard") bound to the same Device.
  6. Attempt to assign "ACL2" to the same interface with the same direction via nb_shell.

Reproduction via nb_shell:

from dcim.models import (
    Device,
    DeviceRole,
    DeviceType,
    Manufacturer,
    Site,
    VirtualChassis,
)
from netbox_acls.models import AccessList, ACLInterfaceAssignment
from ipam.models import Prefix

# Create Site
site = Site.objects.create(
    name="Site 1",
    slug="site-1",
)

# Create Manufacturer and Device Type
manufacturer = Manufacturer.objects.create(
    name="Manufacturer 1",
    slug="manufacturer-1",
)
device_type = DeviceType.objects.create(
    manufacturer=manufacturer,
    model="Device Type 1",
)

# Create Device Role
device_role = DeviceRole.objects.create(
    name="Device Role 1",
    slug="device-role-1",
)

# Create Device and Interface
device1 = Device.objects.create(
    name="Device 1",
    site=site,
    device_type=device_type,
    role=device_role,
)
device_interface1 = Interface.objects.create(
    name="Interface 1",
    device=device1,
    type="1000baset",
)

# Create and Assign Access List 1
device_acl1 = AccessList.objects.create(
    name="STANDARD_ACL1",
    assigned_object=device1,
    type="standard",
    default_action="permit",
    comments="STANDARD_ACL",
)
ACLInterfaceAssignment.objects.create(
    access_list=device_acl1,
    direction="ingress",
    assigned_object=device_interface1,
)

# Create and Attempt to Assign Access List 2
device_acl2 = AccessList.objects.create(
    name="STANDARD_ACL2",
    assigned_object=device1,
    type="standard",
    default_action="permit",
    comments="STANDARD_ACL",
)
acl_device_interface2 = ACLInterfaceAssignment(
    access_list=device_acl2,
    direction="ingress",
    assigned_object=device_interface1,
)
acl_device_interface2.full_clean()  # Expected to fail, but it succeeds
acl_device_interface2.save()

Expected Behavior

The assignment should fail, as only one Access List should be assignable to an interface per direction.

Observed Behavior

The assignment via nb_shell succeeds, even though the Web UI correctly enforces this restriction.

@pheus pheus added the bug Something isn't working label Mar 25, 2025
@pheus
Copy link
Contributor Author

pheus commented Mar 25, 2025

I encountered this issue while developing a test case to validate the uniqueness of ACL assignments. The root cause appears to be an incorrect unique_together constraint in the model configuration.

Currently, the uniqueness constraint is enforced on (assigned_object_type, assigned_object_id, direction, access_list) (e.g., Interface + direction + AccessList), which allows multiple ACL assignments with the same direction on the same interface, as long as the access lists are different.

However, the correct constraint should be on (assigned_object_type, assigned_object_id, direction) (e.g., Interface + direction) alone. Once an ACL assignment exists for a given interface and direction, no other ACL should be assignable in that direction on the same interface.

This misconfiguration allows nb_shell to bypass the expected uniqueness check, whereas the Web UI correctly prevents the duplicate assignment.

pheus added a commit to pheus/netbox-acls that referenced this issue Mar 28, 2025
Fixes the unique constraint for ACLInterfaceAssignment by removing the
access_list field from the uniqueness check.
Ensures that only one ACL can be assigned per interface and direction.
Adds validation tests to prevent multiple ACLs from being assigned in
the same direction on the same interface.

Fixes netbox-community#258
@cruse1977
Copy link
Member

cruse1977 commented Apr 20, 2025

I'm a little torn on this one @pheus, what if say you had an ipv4 and ipv6 acl against the same interface which is a really common use case ?

@pheus
Copy link
Contributor Author

pheus commented Apr 20, 2025

Thanks for bringing that up, @cruse1977! You’re absolutely right that a lot of folks want to bind both an IPv4 ACL and an IPv6 ACL on the same interface/direction.

I’m going to remove the bugfix from PR #257 for now so I can think through a more flexible solution. I’ll follow up here once I have a proposal - any feedback in the meantime is welcome!

@cruse1977 cruse1977 added status: under review and removed bug Something isn't working labels May 17, 2025
@pheus
Copy link
Contributor Author

pheus commented May 17, 2025

Proposal

To support this while still enforcing uniqueness, one possible solution is to introduce an address_family field to ACLInterfaceAssignment, with values like "ipv4" or "ipv6". This field could be explicitly set or derived from the associated AccessList.

The uniqueness constraint would then be updated to:

unique_together = (
    "assigned_object_type",
    "assigned_object_id",
    "direction",
    "address_family",
)

This approach allows one ACL per address family in a given direction on a given interface. For example, it would permit both an ingress IPv4 ACL and an ingress IPv6 ACL on the same interface, while preventing multiple assignments for the same direction and address family.

Let me know if this seems like a reasonable direction or if you'd prefer a different approach. If this falls outside the scope of the issue, I'm happy to open a separate feature request to explore it further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants