In FWaaS v2 cross-tenant assignment of policies is inconsistent

[Nate Johnston]

In the unit tests associated with the FWaaS v2 DB (neutron_fwaas/tests/unit/db/firewall/v2/test_firewall_db_v2.py), there are two that demonstrate improper handling of cross-tenant firewall policy assignment.

First, the logic tested in test_update_firewall_rule_associated_with_other_tenant_policy succeeds, but it should not.

Second, the logic tested in test_update_firewall_group_with_public_fwp fails, but it should succeed.

[lavanya sirigudi (lavanya553)]

Hi Nate Johnston

Please find the complete analysis Response Inline <Lavanya>

1. First, the logic tested in test_update_firewall_rule_associated_with_other_tenant_policy succeeds, but it should not.

<Lavanya> : Logic tested for test_update_firewall_rule_associated_with_other_tenant_policy succeeds (but the expected behavior of the Unit test case is : http conflict code) and this behavior is Correct with the current implementation and we will need modify the Unit test case Expected assert code aligned to the Code behavior i.e. Changing the response Code with "200".
Here is my detail analysis as per the code implementation:

As part of the policy.json firewall Rules as shown below we have the Rules set with either of the credentials ADMIN/OWNER or SHARED as shown below:

    "create_firewall_rule": "", ---> Internally makes use of get_firewall_rule
    "get_firewall_rule": "rule:admin_or_owner or rule:shared_firewalls",
    "update_firewall_rule": "rule:admin_or_owner",

And the above credentials are enforced using the method as specified here using: _ENFORCER.enforce(ADMIN_CTX_POLICY, credentials, credentials). And as per the Neutron-specs we have an audit workflow wherein the firewall_policy can be audited by the relevant entity that is authorized (and can be different from the tenants which create or use the firewall_policy). So when the User is set with ADMIN credentials it sets Audited Flag to false in all policies associated with the corresponding firewall rule and that's how the behavior of the test case is also aligned with the Response code of 200.

2. Second, the logic tested in test_update_firewall_group_with_public_fwp fails, but it should succeed.

<Lavanya> : Logic tested for test_update_firewall_group_with_public_fwp is currently failing and that we have currently analysed and looks like there are some handling which is currently missing from the Code and here is my detail analysis for the same:

In unit test case test_update_firewall_group_with_public_fwp,the testcase is failing with "no result found" exception in _get_by_id method where the query parameters for model are set.Here in _model_query_scope method it is looking for attribute "shared" instead of "public".In firewall_policies schema ,there is "shared" attribute but in "firewall_policies_v2" schema ,the attribute is "public".So we need to filter the firewall_policies_v2 model with "public" attribute to fetch the fwp2 created by tenant2.I have validated the same by adding the case for checking "public" attribute in _model_query_scope method and the testcase is getting executed successfully.​
So we need to add the handling for "public attribute" as part of the current implementation.

[Reedip (reedip-banerjee-deactivatedaccount)]

The public and shared attribute is in discussion and should be resolved in the upcoming release, but thanks for the detailed information.

[lavanya sirigudi (lavanya553)]

Could you please comment on first unit test case analysis(test_update_firewall_rule_associated_with_other_tenant_policy).
Also,could you please confirm if the fix is already released for the second unit testcase or we can go ahead with pushing the patch.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/488438

[Yushiro FURUKAWA (y-furukawa-2)]

Hi lavanya, sorry for late reply. I just posted a patch regarding this bug. I think root cause was missing validation when update firewall_rule. Could you please check it?

[OpenStack Infra (hudson-openstack)]

Change abandoned by Yushiro FURUKAWA (<email address hidden>) on branch: master
Review: https://review.openstack.org/488438
Reason: will be combined into https://review.openstack.org/#/c/486377/

[Reedip (reedip-banerjee-deactivatedaccount)]

Since I am not active anymore, I would like to remove myself from the assignee. Its open for anyone else to take up.

[OpenStack Infra (hudson-openstack)]

Change abandoned by Slawek Kaplonski (<email address hidden>) on branch: master
Review: https://review.opendev.org/488438
Reason: As we are going to deprecate master branch in this project this patch is not needed anymore.

[Lajos Katona (lajos-katona)]

As this bug is inactive for years I changed it to "won't fix", feel free to reopen it if you would like to work on it.