Adding trustee_name back to several windows test states and items #172
Conversation
|
@solind thanks for catching this, although if you can do a sanity check for me, it would appear that in file/reg effectiverights53 and file/reg auditing53, there isn't a "trustee_name" element (which baffles me), it was either removed entirely vs being deprecated, or never used as part of the object/state, but then deprecated from just the item. I assume that adding trustee_name to either to object or the state would mandate a new test (fileeffectiverights512 etc..) which isn't what I'm looking to add here. It would appear that content is accurate as-is, I'm just looking to add some value back to the results, via the collected item. I don't know exactly how this all came to be, as OVAL53 is way before my time with OVAL. |
|
The convention is that every item entity must have a corresponding state entity. I don't believe a new version of the state would be strictly necessary to do that here. On the other hand, I assume you're un-deprecating these for debugging purposes, and not for the purpose of creating OVAL checks relying on trustee names to have specific values, in which case it's not really even necessary. Or, I think this might be a case of two different versions of an object resulting in the collection of the same class of item. Since the item needs to be able to work for both objects, it's possible for an item entity to correspond to only one of the versions of the state. |
|
@solind sorry if I'm a bit dense today, but I've read your last comment a couple times, and I'm not entirely sure which way we should be proceeing |
|
@solind I'm assuming we should proceed with adding the elements to the state option b above), and leaving the test name/version as-is. This matches existing precedence with updates to tests such as auditeventpolicysubcategories_state, which has grown over time, but not had new versions of the same test created. I'll wait for confirmation, but plan to add the trustee_name (or user/group/subgroup) depending on the test) to the states of all affected tests fileffectiverights53_state |
vanderpol
changed the title
vanderpol
changed the title
|
Hi @vanderpol yes my opinion is that you should add trustee_name to the 53_states. I believe they're already present in the original versions of the states, e.g., fileauditedpermissions_state.. |
vanderpol
changed the title
@solind I've committed the required updates to the windows definitions schema, please re-review. Thanks! |
| <xsd:documentation>A string the represents the name of a particular group. In Windows, group names are case-insensitive. As a result, it is recommended that the case-insensitive operations are used for this entity. In a domain environment, groups should be identified in the form: "domain\group name". For local groups use: "computer name\group name". For built-in accounts on the system, use the group name without a domain.</xsd:documentation> | ||
| </xsd:annotation> | ||
| </xsd:element> | ||
| <xsd:element name="user" type="oval-def:EntityStateStringType" minOccurs="0" maxOccurs="unbounded"> |
There was a problem hiding this comment.
State elements should never be "unbounded". The entity_check attribute governs the evaluation of a single state element against the potentially multiple corresponding item elements. Multiple states should be used if multiple comparisons are required.
There was a problem hiding this comment.
Good catch, sorry for yet another round of updates and review. I'll get better with this eventually
| <xsd:documentation>If the specified group has more than one user as a member, then multiple user elements should exist. If the specified group does not contain a single user, then a single user element should exist with a status of 'does not exist'. If there is an error determining the users that are members of the group, then a single user element should be included with a status of 'error'.</xsd:documentation> | ||
| </xsd:annotation> | ||
| </xsd:element> | ||
| <xsd:element name="subgroup" type="oval-def:EntityStateStringType" minOccurs="0" maxOccurs="unbounded"> |
There was a problem hiding this comment.
In addition to the problems I've described in comments, I question the addition of the user and group names added to the group_sid object in the context of this PR, which I thought was limited to un-deprecating the trustee_name.
Did you want to do that in a separate PR? Is there a Github issue associated with that particular change?
I can split it into to two different pull requests, but the over-arching issue #156 elaborated both the un-deprecating of the 53_items/states, but also stated that the user_sid55 and group_sid were lacking the trustee names (which are referred to as user/group) in those tests. It's really all the same issue, whoever updated all of the tests, overlooked the fact that results (while accurate) were meaningless without the trusteename (which can be a username or group) the "user" and "group" tests are both deprecated, replaced by the SID replacements, which IMHO aren't really complete replacements, as they lack meaningful data as to why something failed. The user -> user_sid55 has bigger issues, as several other elements were dropped when going to the SID only method, but I'm not sure they are worth dealing with. |
|
Well, it's meaningful, just not to a human intelligence! What you're doing is combining the user_test with the user_sid_test, which is not the same scope as un-deprecating things. I personally think that deserves a separate issue and PR. I'm not trying to be troublesome, being troublesome comes effortlessly to me LOL. |
If I didn't dislike github so much I wouldn't be as reluctant to redoing all of this :) I was pondering just not deprecating the 'user' test as the user_sid55 test, which in theory replaced it, really doesn't replace it. I'll fight with github and see if I can split this all apart into to PRs... |
…d updates moving them to another PR
|
Assuming I didn't make any mistakes in my changes, I think the un-deprecated 53_item/state update are now committed. I'll do the user/group commits under a different issue/pr |
Un-deprecated trusteename from windows file/registry permissions/audit items
Added user/group back to user_sid_item and group_sid_item