Patrole

Enhance validation decorator with expected error code (403, 404)

Registered by Felipe Monteiro

Currently, certain services like Neutron and Keystone throw a 404 instead of a 403 if a user with a certain role cannot perform an action. Rather than wrapping each API call in a try/except block in the event that a 404 rather than a 403 is thrown due to lack of permissions, the decorator rbac_rule_validation.action could be enhanced with the a new kwarg called 'expected_error_code'. For example:

@rbac_rule_validation.action(service='keystone', rule='identity:get_endpoint', expected_error_code=404)
    ...

Then the code in the decorator implementation:
            try:
                func(*args)
            except exceptions.Forbidden as e:

Could be changed to:
            expected_exception, irregular_msg = _get_exception_type(expected_error_code)
            try:
                func(*args)
            except expected_exception as e:
               ...
               if irregular_msg:
                  LOG.warning(irregular_msg.format(service, rule))
              ...

Blueprint information

Status:
Complete
Approver:
Samantha Blanco
Priority:
Undefined
Drafter:
Felipe Monteiro
Direction:
Needs approval
Assignee:
Rick Bartra
Definition:
New
Series goal:
None
Implementation:
Implemented
Milestone target:
None
Started by
Rick Bartra
Completed by
Rick Bartra

Related branches

Sprints

Whiteboard

I disagree with this. 404 errors are not the correct response to an RBAC validation failure and should not be treated as acceptable. There is a single correct error code for an RBAC validation failure and that is 403 forbidden. -David

@David
While I agree with you regarding error codes conforming to HTTP standards, what if other services, like Neutron and Keystone, philosophically don't care, because they believe that higher security is more important than fidelity to HTTP codes.

Regardless of whether other services are adamant or receptive to changing their returned code from 404 to 403, all this does is redesign the framework so that a try/catch block followed by a lengthy LOG.whatever, is refactored into reusable code, because what we have now is bad design: It's needlessly long, it makes the code harder to read, it ignores the DRY principle, etc.

If you're worried about whether expected_error_code=404 is noticeable enough, that's what the
LOG.warning is for: to warn others that something irregular or not cool has happened. Also, documentation can be updated to tell others that this is something to look out for.

Finally, I think that just because other services may insist on throwing a 404 for forbidden exceptions, shouldn't make Patrole, as a testing framework, resistant to change. Patrole,
as a testing framework, can't really itself drive change; it can only point out errors and hope that other services make changes to their framework accordingly. -Felipe

@Felipe
If other projects intend to do it this way we should support them. I was a bit too zealous in the above comment. I am fine with 404/403, in the light of giving a 403 can be a security concern. I do think that when and how we use 404 vs 403 should be looked into in more detail, as I am unsure why some resources are deemed "protected" enough to need the extra security by obscurity offered by a 404 while others are not.

Gerrit topic: https://review.openstack.org/#q,topic:bp/enhance-validation-decorator-with-error-code,n,z

Addressed by: https://review.openstack.org/444042
    WIP: Enhance validation decorator

(?)

Work Items

This blueprint contains Public information 
Everyone can see this information.
Edit subscription

Subscribers

No subscribers.