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

Clarification on SEC_ACCOUNTS_SUPPORT_ETAGS test #42

Closed edtanous closed 2 years ago

edtanous commented 2 years ago

In looking at the code for this test: https://github.com/DMTF/Redfish-Protocol-Validator/blob/master/assertions/accounts.py#L257

It looks like the flow: Requests the user from AccountService, stores the etag. Changes the password (using the appropriate etag in If-Match). Attempts to do another operation with If-Match set to the step 1 etag value.

The issue I'm seeing here on one system is that because the Password property is not returned as part of the payload (it's always set to null) the strong etag for the resource doesn't change, given that, per RFC7232.

Strong comparison: two entity-tags are equivalent if both are not weak and their opaque-tags match character-by-character.

This would imply that, given the character-by-character ManagerAccount resource is identical, the same ETag value could be returned.

This is also backed up by the Redfish spec which says.

An ETag can be:
• A hash
.....

Section 6.5 of the Redfish spec (by my reading) has nil to say about any special handling for payloads that have semantically changed, but contain the same data.

My guess is that this test was coded up against an implementation that had some internal semantics for generating etags from patched resources, was tracking modification dates as part of the etag implementation, or happened to implement PasswordExpiration, which likely changed when the password was changed, thereby changing the bytes in the response. Assuming my analysis is right (which is maybe 50/50 at this point), there's a couple things we could do.

mraineri commented 2 years ago

The specification is intentionally open-ended when it comes to producing ETags. Ultimately it's a matter of what makes the most sense for the implementation. However, from a client perspective, as long as the service accepts the client-provided ETag when performing a PATCH (assuming nothing has changed in the resource), then all should be good. As far as I can tell, clients simply read the ETag header from the response, and supply the same ETag in the If-Match header in the PATCH request. While RFC7232 does dictate specifics for strong vs weak ETags, the client really shouldn't care given its opaqueness.

edtanous commented 2 years ago

Reading into your response a little fruther, then the test needs to change such that it modifies a parameter that actually gets returned in the response, and not Password, which doesn't, so that the etag will change when the test is run?

mraineri commented 2 years ago

One thing to be careful of with Password though is if two clients are changing the password of an account and the password itself is not factored into the ETag (and just the null), then you can have a potential collision there between two clients: both have the indication that they successfully changed the password, but in reality only one of them will win.

This is part of the reason why we don't specifically call out how the ETag is computed; there are cases like this that aren't typical for web pages so simply computing a hash over the payload itself may be problematic.

mraineri commented 2 years ago

@edtanous could you try a branch I pushed? "Fix42-ETag-Correction" should have a fix to address this issue. It works on the systems I have available.

edtanous commented 2 years ago

Just checked it out and ran it; Test now passes on the implementation I was testing with before. Thanks!