Robottelo: API::EntityTestCase::test_negative_get_unauthorized locks out the many tests due to Brute-force attack prevention

[rplevka]

test_negative_get_unauthorized (49127c71-55a2-42d1-b418-59229e9bad00) tries to repeatedly access resources with providing invalid (no) credentials to test that HTTP 401 is being raised.

While this is by design a fine approach, it recently collides with the new feature which got introduced to nightly in Dec 2017 - brute-force attack prevention. The BZ is not align with any version of satellite, however, this will keep locking out tests in our nightly automation (since we lack further isolation).

• there might be a way to OPT-out from the BFA prevention feature.

according logs, this issue affects around __185__ tests in the upstream-nightly

[renzon]

@ldjebran I've heard we would port almost entirely tyer 1 tests to minitest. Is it the case for this one? If it will be ported we can maybe wait on that. If not I'will try an approach mentioned on foreman's redmine:

Interesting problem. I see we count to 30 if the login attempt fails. Does that mean that automation tried 30 times wrong password? Is that a single test that does that? Or what does the automation try to achieve? A workaround might be cleaning Rails cache, which is normally located at /usr/share/foreman/tmp/. Or you can try running foreman-rake tmp:clear

[rplevka]

@renzon,

I see we count to 30 if the login attempt fails

where can you see this?
The test iterates through individual API endpoints and tries a simple GET with no auth header set (unauthed).

there's (at least) one more test that follows this approach: test_negative_post_unauthorized (2ec82336-5bcc-451a-90ed-9abcecc5a0a8)

A workaround might be cleaning Rails cache, which is normally located at /usr/share/foreman/tmp/

yes, but we must ensure this won't affect other tests that run in parralel.

[jhutar]

For porting to MiniTest status, I would ask @ldjebran

[sghai]

@rplevka : At first place, do we know if we can opt out the brute-force attack prevention ? If not, can we check with devs. Or at least if that's the only test then can we skip it until we know the way to fix ?

[rplevka]

@sghai http://projects.theforeman.org/issues/22778

• either we skip the problematic tests, or let's clear the cache while running them single-threaded

[renzon]

@rplevka I've just looked on the same issue on redmine. By reading it I would try to clean the cache after each negative test call to avoid it blocking any other test. Since each test run in only one thread, I should prevent it if, and only if, this is the only test trying to login with wrong credentials. Do you see any flaw on this approach? Anyway I've decided to ask @ldjebran before trying this solution since porting test to minitest would also avoid the problem, at least on robottelo.

[rplevka]

@renzo, i would vote for porting this to minitest on priority. In the meantime, i would stub the tests (there's one more test - doing the same with POST requests)

[ldjebran]

@rplevka @renzon @sghai we have marked these tests to not be ported as they involve many entities from different projects, we have decided that this will be considered as smoke tests in robottelo, when many other tier1 tests will disappear these tests will have a big value in robottelo.

[ldjebran]

@rplevka @renzon @sghai we can try to create a dummy function and use with "with locking_function" contextmanager
eg:

from robottelo.decorators.func_locker import locking_function, lock_function

@lock_function
def dummy_function_to_lock():
    # do nothing

with locking_function(dummy_function_to_lock):
    # do some server operation
    # this will lock any other call (from other processes too) to dummy_function_to_lock

[ldjebran]

@rplevka @renzon @sghai @lpramuk @omaciel Think these tests are too important to be removed, if there is time escalation we have to deal with it, or find a better solution.
Clearing the cache my be a good solution, the dummy function can implement that part of code to prevent collision and ensure that only one process do that operation.

@lock_function
def clear_rails_cache():
    # call some ssh command

test_some_test(self):
   clear_rails_cache()
   # do some api calls 

[ldjebran]

@rplevka @renzon @sghai @lpramuk @omaciel perhaps we should organize a meeting on how we can resolve this task. as locking the caching process is not the end of the story (we have to lock any other call to server when the clear is in action - except if this is an atomic operation).

[ares]

Honestly, I don't see the big value for this test. It tests only subset of endpoints anyway, e.g. I don't see things related to openscap, discovery, config groups etc. It also seems to do GET/POST only, no DELETE or PUT. If only subset is being tested, I'd suggest limiting it further and not lock yourself out, if you can't do it in isolation. Since the code that handles bruteforce is implemented in parent controller and is always inherited to each other controller (API endpoint), I don't think this would catch more regressions than if limited to let's say 5 endpoints.

But you can argue with black box testing and that it's not my responsibility, which is both valid :-) So I opened a PR with setting, that you could use to disable bruteforce protection - https://github.com/theforeman/foreman/pull/5619 testing from QE appreciated :-) Don't expect this to land sooner than in Foreman 1.19. For other possible workarounds, see http://projects.theforeman.org/issues/22778#note-2 I suggested 3 months ago.

[ldjebran]

@ares many thanks for the PR resolution and recommendations.
@rplevka @renzon @sghai @lpramuk @omaciel to add more value perhaps we should add the DELETE PUT tests (to some created entities - eg: one entity from one foreman sub-project).
what do you think?

[ares]

as suggested in the PR, the setting could be actually a number defaulting to 30, you could set that to 0 to disable the check entirely or e.g. 2 if you want to test the bruteforce prevention too, but that would probably cause problems for tests running in parallel

[omaciel]

@ldjebran +1 for adding more code coverage for sure.

[rplevka]

@ldjebran there's no need to create the entity for testing that the PUT/DELETE request throws 401. Let's avoid unnecessarily heavy setups, please.

[rplevka]

Can we merge this PR until we actually decide what to do, please, it's causing like ~120 tests to fail

[pondrejk]

imho, the discussion about how to solve shouldn't block merging the PR with stubs, as this issue potentially prevents us from discovering other problems

[ldjebran]

I can agree with @pondrejk to disable that problematic tests for the moment to unblock other tests, but cannot agree with you @rplevka that there is no need for that tests.

Any way if we add 5 entities for example one from each foreman plugin , do not think that this a heavy setup.

Tests are here running and passing until a problem occur. When nothing happen (tests not failing) for a long time it does not mean that they are unnecessary (for me it means that there is no regression).

(1) I do not want to imagine when this tests have to fail, and are in disabled state and every body is sure that we have such tests covering that security functionality.
EG this is not about response code but about the behavior of the application when submitting a command when user is not authenticated.

From the developer perspective, they can be sure that all is covered as necessary as @ares mentioned that all that functionality is located at a base class level and they can have unit-tests that cover that functionality at some level.

But for integration testing, if If any one think that this is not important I have a tons of questions in head I can ask you for.
But will ask for the moment only one of the questions related to (1), who is going to take responsibility in that case when code is in production.

[ares]

IMHO the test has some value and I'd look at the risk and time it takes to finish. If it takes few 1 or 2 minutes, I wouldn't care. If it takes more, I'd pick most important endpoints only, as it's very likely that if it works for 5 endpoints, it will work for the rest. I don't think it's long term possible to keep coverage for all endpoints as we're continuously adding more. Also from my perspective, adding more endpoints does not increase code coverage as the endpoint is not even reached, the authorization filter is executed even before and rejects the request. But I understand that this is white box knowledge and yes, there's tiny chance that one endpoint will somehow skip the filter.

[ldjebran]

From the latest upstream nightly build 106 these tests passed and took together 4.7 sec
and was green for all previous run
Test name | Duration | Status
test_negative_get_unauthorized | 1.8 sec | Passed
test_negative_post_unauthorized | 2.9 sec | Passed

From the latest sat-6.4 build 14 these tests passed and took together 4.6 sec and was green for all previous run
Test name | Duration | Status
test_negative_get_unauthorized | 1.6 sec | Passed
test_negative_post_unauthorized | 3 sec | Passed

@rplevka @renzon do not see the logs were it fails, or perhaps there is an other test run that I can not see.

[ldjebran]

@rplevka @renzon the timing in my previous comment is very important also, as if had to be affected , it had to affect the tests them selves as they are in iteration or the brute-force attack prevention is working for only the successful logins (as understand it has to work for unauthorized) or there is something I am missing.

[ldjebran]

@rplevka @renzon or there is a bug in "brute-force attack prevention" it self.
@ares what do you think?

[ldjebran]

@rplevka @renzon @ares Addional note:

The others tests that failed
test_positive_get_status_code | 86 ms | Failed
test_positive_post_status_code | 12 sec | Failed

test_positive_get_status_code
requests.exceptions.HTTPError: 401 Client Error: Unauthorized for url

test_positive_post_status_code (for this test the command passed but the returned http status was wrong, we can open a bug and create a work-around if bz open)
AssertionError: != 200

We take in consideration for now only test_positive_get_status_code that took 86 ms, as understand "brute-force attack prevention" the response should be returned with delay >= 1 min.
The same for when not providing credentials, the server must respond with increasing time delay.

Think the functionality should be reviewed.

Suggest to skip the negative tests only for upstream-nightly for now (waiting to allow disabling "brute-force attack prevention" for that tests), by creating a decorator (hint for upstream we have robottelo.properties file with setting upstream=true)

[jhutar]

Hello @ldjebran . Please could this be closed as #6040 was merged?

[rplevka]

@jhutar yes. verified that the tests now don't pass the treshold and don't trigger the BFA prevention mechanism