WsgiLimiterProxy code looks suspect

[Eric Harney]

WsgiLimiterProxy check_for_delay() has the following:

        if http_client.OK >= resp.status < http_client.MULTIPLE_CHOICES:

Ignoring that the intent of this to check for 2xx responses was obfuscated by the refactoring done here: https://review.openstack.org/#/c/426748/ , the logic here is wrong.

This translates to
    if 200 >= resp.status < 300

Which is equivalent to
    if resp.status <= 200

which means that the MULTIPLE_CHOICES/300 test is meaningless.

I assume this was supposed to be
    if 200 <= resp.status < 300

to look for 2xx returns. I'm not sure what the impact of this is.

[Jeremy Stanley (fungi)]

Please don't switch a bug to Public Security (or Private Security) without first explaining why you suspect it represents a security vulnerability.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.opendev.org/669124

[OpenStack Infra (hudson-openstack)]

Change abandoned by Rajat Dhasmana (<email address hidden>) on branch: master
Review: https://review.opendev.org/669124

[Vida Haririan (vhariria)]

Additional comments http://eavesdrop.openstack.org/meetings/manila/2020/manila.2020-08-27-15.01.log.html

[Goutham Pacha Ravi (gouthamr)]

Hi,

The change on Cinder was abandoned, and so I suspect you agree that this isn't a bug in Cinder (as well)...

Summarizing from the discussion Vida pointed to above:

This check:

   200 >= resp.status < 300

effectively checks if

  resp.status ∈ [200, 299)

which means, "is the response from the WSGILimiterProxy service a 2xx status code [1]?"
The way this is written, the WsgiLimiterProxy that we're hitting returns a non 2xx when the API request must be rate limited. As such, I don't see any problem with this logic.

So, I'm setting this bug to Invalid, If you think otherwise, please let me know! :)

[1] https://tools.ietf.org/html/rfc7231#page-47

[Goutham Pacha Ravi (gouthamr)]

I'll answer myself here,

I should have run the code to test, I apologize - thankfully another Manila contributor did fact check me just now:

  http://paste.openstack.org/show/797216/

for resp.status ∈ [200, 299), this check must be "200 <= resp.status < 300"

[Rajat Dhasmana (whoami-rajat)]

Hi Goutham,

The reason of me abandoning it was i didn't want to continue work on it.

Sorry for not being clear about it but since this has been causing some confusion, I've updated the logic to be more readable.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.opendev.org/748493

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/748493
Committed: https://git.openstack.org/cgit/openstack/manila/commit/?id=090d903b707e0dd84c7cd68cf15476356ce5a83f
Submitter: Zuul
Branch: master

commit 090d903b707e0dd84c7cd68cf15476356ce5a83f
Author: Goutham Pacha Ravi <email address hidden>
Date: Thu Aug 27 10:31:22 2020 -0700

    Fix WsgiLimiterProxy check

    The code in the WsgiLimiterProxy checks the response
    from the proxy to be in the range [200, 299] to
    determine if an API request must be rate limited, but
    the check currently corresponds to checking for <=200.

    This may not have been consequential since a Proxy
    is not expected to set the "X-Wait-Seconds" header in
    the response when there is nothing to delay - so, even
    with this incorrect check, we weren't rate limiting
    incorrectly with a proxy that was setup correctly.

    Change-Id: Id58a87b24b19e1d489fe808d11d59cc93df390f2
    Closes-Bug: #1823750
    Signed-off-by: Goutham Pacha Ravi <email address hidden>

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/669124
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=d72f738c014fc76596d2f323e94cdb0e97116e71
Submitter: Zuul
Branch: master

commit d72f738c014fc76596d2f323e94cdb0e97116e71
Author: whoami-rajat <email address hidden>
Date: Thu Jul 4 15:14:29 2019 +0530

    Correcting the response status range in WsgiLimiterProxy

    The response status should lie between 200 and 300 but the operator
    used makes the condition true when response status is <= 200.
    This patch corrects the same.

    Change-Id: I7368eb7674642032b1313afe900b5f36f8ab0dea
    Closes-Bug: #1823750

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/cinder 18.0.0.0b1 development milestone.