[RFE] Can't protect the "default" security group from regular users

[Paolo E. Mazzon]

The 'default' security group is applied to all VMs in a tenant. This means that tampering with it from one user can prevent other users' VMs from working (e.g. deleting the "ssh ingress" rule). While you can limit actions on the whole security group matching the "name" field (field:security_groups:name=default), when limiting APIs dealing with SG *rules* there is no way of accessing the SG the rule belongs to. This means I can prevent deletion of rules from any SG - disallowing a regular user from managing her own SG - or I must let her delete rules from any SG.

Steps to reproduce:

- policy.yaml

"sg_is_default": "field:security_groups:name=default"
"delete_security_group_rule": "role:member and project_id:%(project_id)s and not rule:sg_is_default or (rule:sg_is_default and role:admin)"

- user can still delete rules from 'default'

[Elvira García Ruiz (elviragr)]

Hi there! Thanks for your report.

My initial thought on this is that maybe if you want to prevent a user from modifying the security group of other user, using different projects might be a good idea for that, since then the user will be isolated on their project and not able to change the security groups or any other resource from a project they don't have access to.

However there might be a different use case I'm not aware of that cannot be resolved this way, I will be happy to know more about it. For now I'm setting this as opinion since this is expected behaviour and not a bug, but we can keep discussing and change the status as needed.

[Paolo E. Mazzon (pemazzon)]

Hi, this is, in my opinion, the problem: "if you want to prevent a user from modifying the security group of other user". The default SG is not another user's SG: it's the SG you inherit - and that is 'forced' on your stuff - when you join a project (OK, you can duplicate it and remove the default one from your VMs...). Nothing wrong if this is managed by the project manager which, I think, is responsible for the shared resources but I see a problem when my VMs are working properly and suddenly they stop working because someone else - with no particular privileges - tamper with something they don't own exclusively. Other resources have the concept of "owner" attached (VMs, volumes, networks,...) so they can have their access regulated through policies, why can't security groups? By the way: this bug report stems from this post

https://lists.openstack.org/pipermail/openstack-discuss/2023-May/033719.html

where 2 different neutron developers agreed that the default SG is missing something and/or should be handled differently. (please note: with this I don't mean to force them into solving this :-D )

[Elvira García Ruiz (elviragr)]

I see, thanks for the background! :) At first sight it looks like an RFE then, but will ask other developers to further discuss this.

[Rodolfo Alonso (rodolfo-alonso-hernandez)]

Hello:

The SG rule object only have the SG ID reference. In order to create a rule to prevent the deletion of a SG rule depending on the SG, that could be something like this:
  "delete_security_group_rule":
    "not rule:field:security_group_rules:security_group_id=<SG ID>"

Of course this is not practical because that implies adding to the policies file a random UUID.

In order to implement this feature, it could be needed a standardized field in the SG rule object. For example, a read only flag like "default_security_group". This field will be populated by the Neutron server during the SG rule creation and will be accessible via API (as commented, read only). This field could be used by the policier with a rule like:
  "delete_security_group_rule":
    "rule:field:security_group_rules:default_security_group=False"

I'm marking this bug as RFE.

Regards.

[Paolo E. Mazzon (pemazzon)]

Hello: doesn't the security group object already contain a "is_default" field? I see in neutron/objects/securitygroup.py

class SecurityGroup(rbac_db.NeutronRbacObject):
    # Version 1.0: Initial version
    # Version 1.1: Add RBAC support
    # Version 1.2: Added stateful support
    # Version 1.3: Added support for remote_address_group_id in rules
    # Version 1.4: Added support for normalized_cidr in rules
    # Version 1.5: Make the shared field nullable
    VERSION = '1.5'

    # required by RbacNeutronMetaclass
    rbac_db_cls = SecurityGroupRBAC
    db_model = sg_models.SecurityGroup

    fields = {
        'id': common_types.UUIDField(),
        'name': obj_fields.StringField(nullable=True),
        'project_id': obj_fields.StringField(nullable=True),
        'shared': obj_fields.BooleanField(nullable=True),
        'stateful': obj_fields.BooleanField(default=True),
        'is_default': obj_fields.BooleanField(default=False),
        'rules': obj_fields.ListOfObjectsField(
            'SecurityGroupRule', nullable=True
        ),
        # NOTE(ihrachys): we don't include source_rules that is present in the
        # model until we realize it's actually needed

As far as I understand this prevents the default SG from being deleted (even by the cloud admin). I think that what is missing here is a SG owner because now every user can delete other user created SGs too.

[Rodolfo Alonso (rodolfo-alonso-hernandez)]

The SG rule object needs a similar flag as in the SG object to determine if that SG rule belongs or not to a default SG. From the Neutron drivers meeting logs [1], I'm going to try using the SG.is_default field in the SG rule OVO, using a back reference (at least I'll try that option initially).

[1]https://meetings.opendev.org/meetings/neutron_drivers/2023/neutron_drivers.2023-05-19-14.00.log.html#l-93

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/neutron/+/883902

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/neutron/+/883907

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/neutron-lib/+/883939

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/neutron/+/883902
Committed: https://opendev.org/openstack/neutron/commit/43ef447a5787c6065638652466d1c61c30750d76
Submitter: "Zuul (22348)"
Branch: master

commit 43ef447a5787c6065638652466d1c61c30750d76
Author: Rodolfo Alonso Hernandez <email address hidden>
Date: Mon May 22 23:41:06 2023 +0200

    SG rule dict method allows DB object and Neutron OVO

    This change allows to pass to ``_make_security_group_rule_dict`` method
    the Neutron OVO. That could include synthetic fields added in the OVO
    (SQL view) that are not present in the database register.

    This change will be needed in next patches to increase the information
    returned by this method, using new synthetic fields added to the
    security group rule OVO.

    Partial-Bug: #2019960
    Change-Id: Ic0e697bb212c7795a40c0b9be01345db26c2874e

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/neutron-lib/+/883939
Committed: https://opendev.org/openstack/neutron-lib/commit/7da72b7f2d009e46807cc2c4fb30cc96f3693344
Submitter: "Zuul (22348)"
Branch: master

commit 7da72b7f2d009e46807cc2c4fb30cc96f3693344
Author: Rodolfo Alonso Hernandez <email address hidden>
Date: Tue May 23 01:53:00 2023 +0200

    Add new SG rule ext. ``security-groups-rules-belongs-to-default-sg``

    Added a new API extension
    ``security-groups-rules-belongs-to-default-sg`` that adds a new
    read only field ``belongs_to_default_sg`` in the security group
    rules. This flag determines if this security group rule belongs
    to the project's default security group.

    Related-Bug: #2019960

    Change-Id: Ibd8f57d82b28f5cdb8874f1ae22cb35adcd8e880

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/neutron/+/896222

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/neutron/+/896273

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/neutron/+/897211

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/neutron/+/883907
Committed: https://opendev.org/openstack/neutron/commit/e066cab875bef07308dc91a163ae03f82006d97f
Submitter: "Zuul (22348)"
Branch: master

commit e066cab875bef07308dc91a163ae03f82006d97f
Author: Rodolfo Alonso Hernandez <email address hidden>
Date: Mon May 22 21:21:17 2023 +0200

    Add a new extension "security-groups-rules-belongs-to-default-sg"

    This new extension adds a new synthetic field, "belongs_to_default_sg",
    to the security group rule OVO. This read only boolean field determines
    if the security group rule belongs to a default security group or not.

    This new field will be used in a new set of policy rules. By default,
    these new rules will allow to create and delete security group rules
    into the default security group of a project only to the admin user

    NOTE: the follow-up patch will introduce the policy rules check,
          during the creation/deletion operations, of the
          "belongs_to_default_sg" field and the user executing this action.

    Partial-Bug: #2019960

    Change-Id: I0b3ded52e1ff8160c5804c59635c0fd34ce9995b

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/neutron/+/896273
Committed: https://opendev.org/openstack/neutron/commit/78027da56ccb25d19ac2c3bc1c174acb2150e6a5
Submitter: "Zuul (22348)"
Branch: master

commit 78027da56ccb25d19ac2c3bc1c174acb2150e6a5
Author: Rodolfo Alonso Hernandez <email address hidden>
Date: Tue Sep 12 08:57:45 2023 +0000

    Remove the publish patch in SG rule BEFORE_DELETE and BEFORE_CREATE

    The method ``delete_security_group_rule`` is publishing the
    BEFORE_DELETE event before starting the security group rule deletion.
    This event is published using a wrap method called
    ``SecurityGroupDbMixin._registry_publish``. This method is capturing
    any ``CallbackFailure`` exception and raising a
    ``SecurityGroupRuleInUse`` one. That makes no sense because:
    * We are hidding the real cause of the callback failure.
    * The BEFORE_DELETE is not checking that the security group rule is
      being used (NOTE 1).
    * If any new implementation makes this check, the corresponding callback
      should return explicitly this exception.

    The method ``_create_security_group_rule`` is publishing the
    BEFORE_CREATE event before starting the security group rule creation.
    The same argument applies here: the callback manager should return the
    exception raise by the callback method (NOTE 2).

    In a follow-up patch, this events will be captured to check the
    permissions related to the user creating or deleting the security group
    rule. In case of error, it will be needed to raise a ``NotAuthorized``
    derived exception, instead of a ``InUse`` one.

    NOTE 1: this is the current use of BEFORE_DELETE event in the
    OpenStack repository:
    * [2] Omni project had no activity for the last 4 years.
    * [3] networking-arista: the method ``run_cmds_on_all_switches``, that
      calls ``run_openstack_sg_cmds``, returns its own exceptions.
    * [4] networking-opencontrail: same justification.
    * [5] The ML2/OVN mechanism driver, that will raise an exception if the
      OVN ACL deletion doesn't succeed.

    NOTE 2: this is the current use of BEFORE_DELETE event in the
    OpenStack repository:
    * [2] Omni project had no activity for the last 4 years.

    [1]https://codesearch.openstack.org/?q=%5C.SECURITY_GROUP_RULE&i=nope&literal=nope&files=&excludeFiles=&repos=
    [2]https://opendev.org/x/omni/src/branch/master/neutron/neutron/plugins/ml2/drivers/aws/callbacks.py
    [3]https://opendev.org/x/networking-arista/src/branch/master/networking_arista/ml2/security_groups/arista_security_groups.py
    [4]https://opendev.org/x/networking-opencontrail/src/branch/master/networking_opencontrail/ml2/opencontrail_sg_callback.py
    [5]https://opendev.org/openstack/neutron/src/branch/master/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py

    Partial-Bug: #2019960

    Change-Id: I8d5f5392fb7a6ab9b20e9222c143f4e67c925cae

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/neutron/+/897211
Committed: https://opendev.org/openstack/neutron/commit/1aa1f2f9cbc98de87e5b6f82c8307afeac216f28
Submitter: "Zuul (22348)"
Branch: master

commit 1aa1f2f9cbc98de87e5b6f82c8307afeac216f28
Author: Rodolfo Alonso Hernandez <email address hidden>
Date: Tue Oct 3 10:31:03 2023 +0000

    Missing SG rule default group extension in ``Ml2Plugin``

    The extension "security-groups-rules-belongs-to-default-sg" definition
    in the ``Ml2Plugin`` supported extension aliases was missing from
    patch [1].

    [1]https://review.opendev.org/c/openstack/neutron/+/883907

    Partial-Bug: #2019960
    Change-Id: I3488657c1f093db192681fcba931f2aa6e7e9c8d

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/neutron/+/896222
Committed: https://opendev.org/openstack/neutron/commit/96223931cae782a997271c17ea8092ed277d2db3
Submitter: "Zuul (22348)"
Branch: master

commit 96223931cae782a997271c17ea8092ed277d2db3
Author: Rodolfo Alonso Hernandez <email address hidden>
Date: Tue Oct 3 14:34:55 2023 +0000

    Create a policy rule to control if a rule belongs to the default SG

    The policy rule ``shared_security_group`` allows to create new policy
    rules checking if a security group rule belongs or not to the project
    default security group.

    By default the behaviour has not changed. If an administrator wants
    to prevent a non-privileged user from creating or deleting rules in the
    default security group, the ``create_security_group_rule`` and
    ``delete_security_group_rule`` can be overriden. An example is provided
    in the unit tests.

    Closes-Bug: #2019960

    Change-Id: I6c90b61df0e726ef07f177801069baf30c49de67

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/neutron 24.0.0.0b1 development milestone.