search

DMTF / Redfish-Protocol-Validator

The Redfish Protocol Validator tests the HTTP protocol behavior of a Redfish service to validate that it conforms to the Redfish specification.
Other
14 stars 12 forks source link

About SEC_PRIV_OPERATION_TO_PRIV_MAPPING #27

Closed JojoWu19 closed 2 years ago

JojoWu19 commented 3 years ago

We get the below warning in the report:

未命名

The Validator PATCH a ManagerAccount resource with "RoleId":"ReadOnly", "UserName": "rfpv8f12" ..., and then using the identity rfpv8f12 to PATCH this resource with again, and Validator expects to get 401 as PASS, and other errors as WARN. For this case, I have below questions:

  1. Should Validator expect 403 for incorrect privilege here?
  2. A ReadOnly user should have available to change his Password, right?
  3. In our report, it looks like Validator uses incorrect Etag to PATCH Passord, right?
  4. Should Validator adjust this case? For example: using the identity rfpv8f12 to PATCH other existed ManagerAccount resource.
mraineri commented 3 years ago

401 is the correct HTTP status code. 401 indicates the client provided invalid credentials (such as a bad password or username), or did not provide any credentials.

This test is to verify the behavior of authentication and authorization; it's purposefully sending an invalid password to ensure the request is rejected due to invalid credentials.

It appears the service is performing ETag validation prior to authorization checking. The tool is not providing an ETag since ultimately it's expecting that the service will reject the request due to invalid credentials.

JojoWu19 commented 3 years ago

@mraineri , I think the warning is not raised in the part of sending an invalid password. Below is the logs of warning part in the web log:

unix: - - [09/Aug/2021:14:32:15 +0000] "GET /redfish/v1/AccountService/Accounts/6 HTTP/1.1" 200 797 "-" "python-requests/2.26.0"
unix: - - [09/Aug/2021:14:32:15 +0000] "PATCH /redfish/v1/AccountService/Accounts/6 HTTP/1.1" 200 799 "-" "python-requests/2.26.0"

Above request is PATCH with "Password": "**","RoleId": "ReadOnly","UserName": "rfpv8f12".

unix: - - [09/Aug/2021:14:32:16 +0000] "GET /redfish/v1/AccountService/Accounts/6 HTTP/1.1" 200 799 "-" "python-requests/2.26.0"
unix: - - [09/Aug/2021:14:32:16 +0000] "GET /redfish/v1/AccountService/Accounts/6 HTTP/1.1" 200 799 "-" "python-requests/2.26.0"
unix: - - [09/Aug/2021:14:32:16 +0000] "PATCH /redfish/v1/AccountService/Accounts/6 HTTP/1.1" 200 798 "-" "python-requests/2.26.0"

Above request is PATCH with "Enabled": true.

unix: - rfpv8f12 [09/Aug/2021:14:32:16 +0000] "GET /redfish/v1/AccountService/Accounts/6 HTTP/1.1" 200 460 "-" "python-requests/2.26.0"

Above request is GET by using correct password of rfpv8f12.

unix: - rfpv8f12 [09/Aug/2021:14:32:17 +0000] "PATCH /redfish/v1/AccountService/Accounts/6 HTTP/1.1" 412 446 "-" "python-requests/2.26.0"

Above request should be PATCH with "Password": "**" by using correct password of rfpv8f12 and Etag header but it is an incorrect Etag.

This warning is reported from here and the tool code is below:

def test_priv_operation_to_priv_mapping(sut: SystemUnderTest):
    """Perform test for Assertion.SEC_PRIV_OPERATION_TO_PRIV_MAPPING."""
...
        if response.ok:
            msg = ('PATCH request to account %s using credentials of other '
                   'user %s succeeded with status %s; expected it to fail '
                   'with status %s' % (uri, user, response.status_code,
                                       requests.codes.UNAUTHORIZED))
            sut.log(Result.FAIL, 'PATCH', response.status_code, uri,
                    Assertion.SEC_PRIV_OPERATION_TO_PRIV_MAPPING,
                    msg)
        else:
            if response.status_code == requests.codes.UNAUTHORIZED:
                sut.log(Result.PASS, 'PATCH', response.status_code, uri,
                        Assertion.SEC_PRIV_OPERATION_TO_PRIV_MAPPING,
                        'Test passed')
            else:
                msg = ('PATCH request to account %s using credentials of '
                       'other user %s failed with status %s, but expected it '
                       'to fail with status %s; extended error: %s' % (
                        uri, user, response.status_code,
                        requests.codes.UNAUTHORIZED,
                        utils.get_extended_error(response)))
                sut.log(Result.WARN, 'PATCH', response.status_code, uri,
                        Assertion.SEC_PRIV_OPERATION_TO_PRIV_MAPPING,
                        msg)
...

From the meaning of msg PATCH request to account %s using credentials of other user %s succeeded with status %s; expected it to fail with status %s, it should PATCH to other account not the read only account, is it right?

BTW, could I have way to enable debug to see the details data of request and response?

mraineri commented 3 years ago

I looked closer and you're right about the password here; this is a case where the tool is performing a request with a non-privileged account to change someone else's password. In this case, I would expect a 403 and not a 401. I would still think a 412 is not appropriate here since the user is attempting to perform an operation to which they do not have privilege.

Most operations are performed ahead of time since many of the tests check the operations performed on the same resource and just need to verify how the service behaves against each of the clauses. The following part of the test finds all cached responses for a user attempting to modify another account:

for uri, response in sut.get_responses_by_method(
            'PATCH', resource_type=ResourceType.MANAGER_ACCOUNT,
            request_type=RequestType.MODIFY_OTHER)

The MODIFY_OTHER path is found in resources.py at line 262 (def patch_other_account), which gets invoked early in the execution of the tool.

You should be able to enable more debug output with --log-level DEBUG, which appears to include more than just the status codes.

JojoWu19 commented 3 years ago

About this is a case where the tool is performing a request with a non-privileged account to change someone else's password. In this case, I would expect a 403 and not a 401. I would still think a 412 is not appropriate here since the user is attempting to perform an operation to which they do not have privilege., yes, I also expect the http status is 403. Would validator modify this case to PATCH other account and expect a 403?

BTW, I check the log with --log-level DEBUG and it doesn't include the payload (json body) of PATCH/POST, could validator also log these data?

mraineri commented 3 years ago

Sure, we can certainly find a way to add that. That would be a very useful feature.

I'll discuss the tool change further with others, and I might need to see if clarifications in the spec are needed.

JojoWu19 commented 3 years ago

@mraineri , get it. Thank you.

mraineri commented 3 years ago

@JojoWu19 will be updating the tool to check for 403 and 404 as valid responses. We added a clause to the spec some time last year to allow for 404 in case advertising the URI itself is considered sensitive.

401 will be a warning, and any 2XX status is a strict failure.

I'm going to leave this issue open though because the logging of request/response data for debug is going to take more work than I originally thought.

JojoWu19 commented 3 years ago

@mraineri , I check the report of new validator but it still uses new created readonly account(Accounts/6) to patch this new created readonly account(Accounts/6) so this issue still is existed. Could you help check this? Thank you.

mraineri commented 3 years ago

That's the intent; the tool creates a new read-only account and attempts to perform a PATCH of another user to ensure the service rejects the request.

Are you saying you're seeing the new account attempt to PATCH its own password?

JojoWu19 commented 3 years ago

Yes, it looks like a new account PATCH its own password. Below are related debug logs:

DEBUG:root:Available roles: {'GroupRole4', 'GroupRole7', 'CustomRole4', 'CustomRole9', 'GroupRole9', 'Administrator', 'CustomRole2', 'CustomRole1', 'CustomRole6', 'GroupRole12', 'CustomRole12', 'CustomRole8', 'GroupRole6', 'GroupRole2', 'GroupRole13', 'GroupRole14', 'CustomRole5', 'GroupRole16', 'GroupRole5', 'GroupRole11', 'GroupRole8', 'GroupRole10', 'GroupRole3', 'ReadOnly', 'Operator', 'CustomRole11', 'GroupRole15', 'CustomRole3', 'CustomRole7', 'GroupRole1', 'CustomRole10'}
DEBUG:root:Role selected for account creation: ReadOnly
DEBUG:urllib3.connectionpool:https://10.162.246.230:443 "POST /redfish/v1/AccountService/Accounts HTTP/1.1" 405 None
DEBUG:root:response status = 405, method = POST, uri = /redfish/v1/AccountService/Accounts, resource_type = None, request_type = RequestType.NORMAL
DEBUG:urllib3.connectionpool:https://10.162.246.230:443 "GET /redfish/v1/AccountService/Accounts/3 HTTP/1.1" 200 None
DEBUG:urllib3.connectionpool:https://10.162.246.230:443 "PATCH /redfish/v1/AccountService/Accounts/3 HTTP/1.1" 200 None
DEBUG:root:response status = 200, method = PATCH, uri = /redfish/v1/AccountService/Accounts/3, resource_type = ResourceType.MANAGER_ACCOUNT, request_type = RequestType.NORMAL
DEBUG:urllib3.connectionpool:https://10.162.246.230:443 "GET /redfish/v1/AccountService/Accounts/3 HTTP/1.1" 200 None
DEBUG:urllib3.connectionpool:https://10.162.246.230:443 "GET /redfish/v1/AccountService/Accounts/3 HTTP/1.1" 200 None
DEBUG:urllib3.connectionpool:https://10.162.246.230:443 "PATCH /redfish/v1/AccountService/Accounts/3 HTTP/1.1" 200 None
DEBUG:root:response status = 200, method = GET, uri = /redfish/v1/AccountService/Accounts/3, resource_type = ResourceType.MANAGER_ACCOUNT, request_type = RequestType.NORMAL
DEBUG:urllib3.connectionpool:Starting new HTTPS connection (1): 10.162.246.230:443
DEBUG:urllib3.connectionpool:https://10.162.246.230:443 "GET /redfish/v1/AccountService/Accounts/3 HTTP/1.1" 200 None
DEBUG:urllib3.connectionpool:https://10.162.246.230:443 "PATCH /redfish/v1/AccountService/Accounts/3 HTTP/1.1" 412 None
DEBUG:root:response status = 412, method = PATCH, uri = /redfish/v1/AccountService/Accounts/3, resource_type = ResourceType.MANAGER_ACCOUNT, request_type = RequestType.MODIFY_OTHER

You also can refer the logs of web side that I poested above.

mraineri commented 3 years ago

Okay, thanks; I'll take a closer look. I don't see that happening on the systems I've tried, but maybe there are some assumptions in the "PATCH Other" logic that might end up PATCHing the same account in certain scenarios.

mraineri commented 3 years ago

I'll need to do a bit more tracing, but from tracing the path, the test will create two new user accounts for the test.

Are you able to trace things back a bit further on your end to confirm this is happening?

JojoWu19 commented 3 years ago

@mraineri , I think below codes make this issue:

new_session = requests.Session()
new_session.auth = (user, password)
new_session.verify = sut.verify
pwd = patch_account(sut, new_session, new_acct_uri, request_type=RequestType.MODIFY_OTHER)

I modify this part as below and I can see the result is 403.

new_sut = SystemUnderTest(sut.rhost, user, password, verify=False)
new_sut.set_avoid_http_redirect(sut.avoid_http_redirect)
new_sut.login()
pwd = patch_account(new_sut, new_sut.session, new_acct_uri, request_type=RequestType.MODIFY_OTHER)
new_sut.logout()

But the test case will be NOT_TESTED because I don't add the response to original sut.

mraineri commented 3 years ago

Thanks; I'll need to take a closer look at this. This may be related to issue #28 .

JojoWu19 commented 3 years ago

Get it, and if you need to verify, please let me know, thanks.

mraineri commented 2 years ago

@JojoWu19 could you please run from the branch "Additional-Tracing" with --log-level debug? This should show traces for all HTTP requests/responses (minus the response body).

JojoWu19 commented 2 years ago

@mraineri , here is the data that you need. debug.zip

mraineri commented 2 years ago

Looking at the trace a bit, I'm going back to my original thought about the behavior. This is what I see in debug.log:

By this point, Accounts/2 contains the credentials rfpvd868:A4HGfJBsFW

So, it seems to me the tool is correcting trying to have a read-only user attempt to modify someone else's password. The credentials going in are valid, but the user doesn't have sufficient privilege to perform the operation.

JojoWu19 commented 2 years ago

@mraineri , yes, the behavior is correcr as you said, but our last question is: Line 3509 - 3511: why it get the reply is 412? With this more details debug log, I think the root cause could be the requests have different header Accept-Encoding: gzip in the Line 3484 and 3509. This makes redfish service that is base on the nginx to return week Etag. But why tool sometime sends request with Accept-Encoding: identity and sometime sends request with Accept-Encoding: gzip?

mraineri commented 2 years ago

Most of the requests should be performed via the sessions being tracked by the system under test object. The requests module has the ability to add common headers to a given session so you don't need to do that throughout your code, three of which we use are "Accept-Encoding: identity", "OData-Version: 4.0", and "X-Auth-Token: ".

However, in some tests, we want to intentionally create a new connection to the service since we don't want to pass in the same credentials used for the rest of testing (such as in this case). So, in these cases, we're relying on default header behavior for the requests module; requests will add some headers to the request if not specified by the user. As far as I can tell, the latest version of requests (and urllib3) will provide Accept-Encoding: identity, but it's possible other versions have different behavior and might provide other encoding values.

JojoWu19 commented 2 years ago

I think redfish service shall reply 403 than 412 here, and it maybe should support this weak etag that changed by nginx. I close this issue because this is not a tool issue. Thank you @mraineri