Issue 303250043: [plaso] Added attribute container identifiers #771 (Closed)
|
Created:
7 years, 8 months ago by Joachim Metz Modified:
6 years, 3 months ago Reviewers:
CC:
kiddi, log2timeline-dev_googlegroups.com Visibility:
Public. |
Description[plaso] Added attribute container identifiers #771
Patch Set 1 #
Total comments: 4
Patch Set 2 : Worked on attribute container identifiers #771 #
Total comments: 2
Patch Set 3 : Changes after merge. #
Total comments: 62
Patch Set 4 : Changes after split #Patch Set 5 : Changes after merge #
MessagesTotal messages: 18
https://codereview.appspot.com/303250043/diff/1/plaso/containers/interface.py File plaso/containers/interface.py (right): https://codereview.appspot.com/303250043/diff/1/plaso/containers/interface.py... plaso/containers/interface.py:43: self.__identifier__ = AttributeContainerIdentifier() Note to reviewer, I'm not yet entirely set on having __x__ based attribute names.
Sign in to reply to this message.
https://codereview.appspot.com/303250043/diff/1/plaso/analysis/interface.py File plaso/analysis/interface.py (right): https://codereview.appspot.com/303250043/diff/1/plaso/analysis/interface.py#n... plaso/analysis/interface.py:143: self._event_uuids_by_pathspec = defaultdict(list) TODO: get context if this is used?
Sign in to reply to this message.
https://codereview.appspot.com/303250043/diff/1/plaso/analysis/interface.py File plaso/analysis/interface.py (right): https://codereview.appspot.com/303250043/diff/1/plaso/analysis/interface.py#n... plaso/analysis/interface.py:143: self._event_uuids_by_pathspec = defaultdict(list) On 2016/08/09 05:26:15, Joachim Metz wrote: > TODO: get context if this is used? Done. https://codereview.appspot.com/303250043/diff/1/plaso/containers/interface.py File plaso/containers/interface.py (right): https://codereview.appspot.com/303250043/diff/1/plaso/containers/interface.py... plaso/containers/interface.py:43: self.__identifier__ = AttributeContainerIdentifier() On 2016/08/08 20:00:48, Joachim Metz wrote: > Note to reviewer, I'm not yet entirely set on having __x__ based attribute > names. Done.
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
https://codereview.appspot.com/303250043/diff/20001/plaso/analysis/interface.py File plaso/analysis/interface.py (right): https://codereview.appspot.com/303250043/diff/20001/plaso/analysis/interface.... plaso/analysis/interface.py:142: # the specified type nit: add trailing dot.
Sign in to reply to this message.
https://codereview.appspot.com/303250043/diff/20001/plaso/analysis/interface.py File plaso/analysis/interface.py (right): https://codereview.appspot.com/303250043/diff/20001/plaso/analysis/interface.... plaso/analysis/interface.py:142: # the specified type On 2016/08/11 04:53:53, Joachim Metz wrote: > nit: add trailing dot. Done.
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
https://codereview.appspot.com/303250043/diff/100001/plaso/analysis/interface.py File plaso/analysis/interface.py (right): https://codereview.appspot.com/303250043/diff/100001/plaso/analysis/interface... plaso/analysis/interface.py:114: resulting digest hash recording to. This doesn't make sense - what's a "digest hash recording"? I assume you've renamed HashAnalysis to DigestHashRecording? If so, the renaming make the purpose of the class much less clear. https://codereview.appspot.com/303250043/diff/100001/plaso/analysis/interface... plaso/analysis/interface.py:144: self._path_spec_by_event_identifier = {} path_specification, for consistency I suppose https://codereview.appspot.com/303250043/diff/100001/plaso/analysis/interface... plaso/analysis/interface.py:293: digest_hash = None Please don't use this terminology - "digest hash" seems to be a very uncommon usage (I can't find _any_ other usage of this term). Use "message digest", "hash" or "hash value". https://codereview.appspot.com/303250043/diff/100001/plaso/analysis/nsrlsvr.py File plaso/analysis/nsrlsvr.py (right): https://codereview.appspot.com/303250043/diff/100001/plaso/analysis/nsrlsvr.p... plaso/analysis/nsrlsvr.py:32: digest_hash_recording_queue (Queue.queue): that the analyzer will add See previous re: "digest hash" and "recording" https://codereview.appspot.com/303250043/diff/100001/plaso/analysis/nsrlsvr.p... plaso/analysis/nsrlsvr.py:79: found_in_library = nsrl_result == u'1' Remove this line https://codereview.appspot.com/303250043/diff/100001/plaso/analysis/tagging.py File plaso/analysis/tagging.py (right): https://codereview.appspot.com/303250043/diff/100001/plaso/analysis/tagging.p... plaso/analysis/tagging.py:15: from efilter.transforms import asdottysql # pylint: disable=unused-import This isn't true any more, please remove. https://codereview.appspot.com/303250043/diff/100001/plaso/containers/events.py File plaso/containers/events.py (right): https://codereview.appspot.com/303250043/diff/100001/plaso/containers/events.... plaso/containers/events.py:191: """Sets the event tag. Please describe what this method does, and why it exists. It looks like the previous tag is overwritten, and all it's labels are lost. Are you sure this is what you want? It looks like the tag is modified as well, so if you try to Set the same tag on multiple events, you actually only set it on the last one you call SetEventTag on. Is this really what's desired? What happens if a parser sets an attribute called "tag" on an event? https://codereview.appspot.com/303250043/diff/100001/plaso/containers/events.... plaso/containers/events.py:319: """Retrieves the event identifier. "Retrieves the identifier of the event associated with this tag." https://codereview.appspot.com/303250043/diff/100001/plaso/containers/events.... plaso/containers/events.py:325: AttributeContainerIdentifier: event identifier. + which may be none. https://codereview.appspot.com/303250043/diff/100001/plaso/containers/events.... plaso/containers/events.py:330: """Sets the event identifier. "Set the identifier of the event associated with this tag." https://codereview.appspot.com/303250043/diff/100001/plaso/containers/hashes.py File plaso/containers/hashes.py (right): https://codereview.appspot.com/303250043/diff/100001/plaso/containers/hashes.... plaso/containers/hashes.py:2: """Digest hashes related attribute container object definitions.""" See previous comments regarding "digest hash" https://codereview.appspot.com/303250043/diff/100001/plaso/containers/hashes.... plaso/containers/hashes.py:12: cryptographic digest hash, such as SHA-256 or MD5. The recording can Recording doesn't make much sense here. What's been recorded? Please rename. Also, remove the sentence about "good" and "bad". This class doesn't make value judgements, nor does it have a good/bad boolean. https://codereview.appspot.com/303250043/diff/100001/plaso/containers/hashes.... plaso/containers/hashes.py:17: found_in_library (bool): True if the digest hash was found in the library, What library? This seems unnecessarily limiting/specific in terms of design - there may be a lot more things we want to say about a hash value. Also the contents of the "library" might not be same from day to day. https://codereview.appspot.com/303250043/diff/100001/plaso/containers/hashes.... plaso/containers/hashes.py:19: labels (list[str]): labels for an event tag. I guess this is where we might add extra content about a hash value is, but isn't that we're going to end up doing with "library" attributes anyway? This class needs some more thought. https://codereview.appspot.com/303250043/diff/100001/plaso/containers/hashes.... plaso/containers/hashes.py:20: library (str): name of the digest hash library. What's a digest hash library? I've never seen this term used before. I strongly recommend dropping this term. https://codereview.appspot.com/303250043/diff/100001/plaso/containers/interfa... File plaso/containers/interface.py (right): https://codereview.appspot.com/303250043/diff/100001/plaso/containers/interfa... plaso/containers/interface.py:9: class AttributeContainerIdentifier(object): Why does this need to be a class of its own? I assume there's a reason, please add it to the class docstring. Container.Identifier.identifier is pretty ugly, is there another way to do this? https://codereview.appspot.com/303250043/diff/100001/plaso/containers/interfa... plaso/containers/interface.py:52: dict[str, object]: attribute values. mapping of attribute names to attribute values. https://codereview.appspot.com/303250043/diff/100001/plaso/containers/interfa... plaso/containers/interface.py:92: AttributeContainerIdentifier: identifier. "A unique identifier for the container which does not persist across serialization" or something similarly verbose. https://codereview.appspot.com/303250043/diff/100001/plaso/containers/reports.py File plaso/containers/reports.py (right): https://codereview.appspot.com/303250043/diff/100001/plaso/containers/reports... plaso/containers/reports.py:14: images: a list containing ??? I think we can just remove this "images" attribute. https://codereview.appspot.com/303250043/diff/100001/plaso/containers/reports... plaso/containers/reports.py:15: plugin_name: a string containing the name of the analysis plugin that Please add type information to these attributes, so that it's consistent. https://codereview.appspot.com/303250043/diff/100001/plaso/engine/knowledge_b... File plaso/engine/knowledge_base.py (right): https://codereview.appspot.com/303250043/diff/100001/plaso/engine/knowledge_b... plaso/engine/knowledge_base.py:36: """str: codepage of the current session.""" -of +associated with This goes for the other docstrings like this too. Perhaps even "in"? or "for"? https://codereview.appspot.com/303250043/diff/100001/plaso/engine/knowledge_b... plaso/engine/knowledge_base.py:78: session_identifier (Optional[str])): session identifier, where "current" Please correct docstring, this is an int not a str. and knowledge_base.CURRENT_SESSION is what represents the active session. This applies to similar args/docstrings here too. https://codereview.appspot.com/303250043/diff/100001/plaso/engine/knowledge_b... plaso/engine/knowledge_base.py:269: """Reads the knowledge base values from a system configuration artifact. -the https://codereview.appspot.com/303250043/diff/100001/plaso/engine/knowledge_b... plaso/engine/knowledge_base.py:332: ValueError: if the timezone is not supported. -not supported +is not known to pytz https://codereview.appspot.com/303250043/diff/100001/plaso/engine/path_helper.py File plaso/engine/path_helper.py (right): https://codereview.appspot.com/303250043/diff/100001/plaso/engine/path_helper... plaso/engine/path_helper.py:54: path_spec (dfvfs.PathSpec): path specification. path_specification, to be consistent https://codereview.appspot.com/303250043/diff/100001/plaso/engine/path_helper... plaso/engine/path_helper.py:91: def GetRelativePathForPathSpec(cls, path_spec, mount_path=None): path_specification, to be consistent https://codereview.appspot.com/303250043/diff/100001/plaso/output/timesketch_... File plaso/output/timesketch_out.py (right): https://codereview.appspot.com/303250043/diff/100001/plaso/output/timesketch_... plaso/output/timesketch_out.py:73: list[str]: names of argument that are required by the module and have arguments https://codereview.appspot.com/303250043/diff/100001/plaso/parsers/mediator.py File plaso/parsers/mediator.py (right): https://codereview.appspot.com/303250043/diff/100001/plaso/parsers/mediator.p... plaso/parsers/mediator.py:47: """bool: True if the parsing should be aborted.""" -the https://codereview.appspot.com/303250043/diff/100001/plaso/parsers/mediator.p... plaso/parsers/mediator.py:52: """stt: codepage.""" str https://codereview.appspot.com/303250043/diff/100001/plaso/parsers/mediator.p... plaso/parsers/mediator.py:132: """Retrieves the inode from the inode value. "Normalizes an inode value to an integer" https://codereview.appspot.com/303250043/diff/100001/plaso/parsers/mediator.p... plaso/parsers/mediator.py:333: """Retrieves the current parser chain. Finished reviewing here.
Sign in to reply to this message.
https://codereview.appspot.com/303250043/diff/100001/plaso/analysis/interface.py File plaso/analysis/interface.py (right): https://codereview.appspot.com/303250043/diff/100001/plaso/analysis/interface... plaso/analysis/interface.py:114: resulting digest hash recording to. Technically "hash analysis" does not make sense either. Since it it not an analysis but a lookup of previous recorded information about an hash. https://codereview.appspot.com/303250043/diff/100001/plaso/analysis/interface... plaso/analysis/interface.py:144: self._path_spec_by_event_identifier = {} Ack, will revisit at a later time. https://codereview.appspot.com/303250043/diff/100001/plaso/analysis/interface... plaso/analysis/interface.py:293: digest_hash = None "digest hash" is widely used terminology https://www.ietf.org/rfc/rfc4701.txt section 3.5 > I can't find _any_ other usage of this term https://www.google.ch/webhp?sourceid=chrome-instant&ion=1&espv=2&ie=UTF-8#q=%... https://codereview.appspot.com/303250043/diff/100001/plaso/analysis/nsrlsvr.py File plaso/analysis/nsrlsvr.py (right): https://codereview.appspot.com/303250043/diff/100001/plaso/analysis/nsrlsvr.p... plaso/analysis/nsrlsvr.py:32: digest_hash_recording_queue (Queue.queue): that the analyzer will add see previous reply. https://codereview.appspot.com/303250043/diff/100001/plaso/analysis/nsrlsvr.p... plaso/analysis/nsrlsvr.py:79: found_in_library = nsrl_result == u'1' Ack, will revisit at a later time https://codereview.appspot.com/303250043/diff/100001/plaso/analysis/tagging.py File plaso/analysis/tagging.py (right): https://codereview.appspot.com/303250043/diff/100001/plaso/analysis/tagging.p... plaso/analysis/tagging.py:15: from efilter.transforms import asdottysql # pylint: disable=unused-import On 2017/01/19 16:46:24, onager wrote: > This isn't true any more, please remove. Acknowledged. Likely a merge left over. https://codereview.appspot.com/303250043/diff/100001/plaso/containers/events.py File plaso/containers/events.py (right): https://codereview.appspot.com/303250043/diff/100001/plaso/containers/events.... plaso/containers/events.py:191: """Sets the event tag. Ack, will revisit at a later date https://codereview.appspot.com/303250043/diff/100001/plaso/containers/events.... plaso/containers/events.py:319: """Retrieves the event identifier. On 2017/01/19 16:46:24, onager wrote: > "Retrieves the identifier of the event associated with this tag." > Done. https://codereview.appspot.com/303250043/diff/100001/plaso/containers/events.... plaso/containers/events.py:325: AttributeContainerIdentifier: event identifier. On 2017/01/19 16:46:24, onager wrote: > + which may be none. Done. https://codereview.appspot.com/303250043/diff/100001/plaso/containers/events.... plaso/containers/events.py:330: """Sets the event identifier. On 2017/01/19 16:46:24, onager wrote: > "Set the identifier of the event associated with this tag." Done. https://codereview.appspot.com/303250043/diff/100001/plaso/containers/hashes.py File plaso/containers/hashes.py (right): https://codereview.appspot.com/303250043/diff/100001/plaso/containers/hashes.... plaso/containers/hashes.py:2: """Digest hashes related attribute container object definitions.""" See previous remark. https://codereview.appspot.com/303250043/diff/100001/plaso/containers/hashes.... plaso/containers/hashes.py:12: cryptographic digest hash, such as SHA-256 or MD5. The recording can See previous remark. https://codereview.appspot.com/303250043/diff/100001/plaso/containers/hashes.... plaso/containers/hashes.py:17: found_in_library (bool): True if the digest hash was found in the library, A library in this context is a collection of hashes. https://codereview.appspot.com/303250043/diff/100001/plaso/containers/hashes.... plaso/containers/hashes.py:19: labels (list[str]): labels for an event tag. > This class needs some more thought. Please suggest an alternative that is better than the current implementation of "hash analysis" https://codereview.appspot.com/303250043/diff/100001/plaso/containers/hashes.... plaso/containers/hashes.py:20: library (str): name of the digest hash library. > I've never seen this term used before. I don't understand this remark: https://www.google.ch/webhp?sourceid=chrome-instant&ion=1&espv=2&ie=UTF-8#q=%... The L in NSRL is for Library > I strongly recommend dropping this term. Please suggest an alternative that is better than the current implementation of "hash analysis" https://codereview.appspot.com/303250043/diff/100001/plaso/containers/interfa... File plaso/containers/interface.py (right): https://codereview.appspot.com/303250043/diff/100001/plaso/containers/interfa... plaso/containers/interface.py:9: class AttributeContainerIdentifier(object): On 2017/01/19 16:46:24, onager wrote: > Why does this need to be a class of its own? I assume there's a reason, please > add it to the class docstring. > > Container.Identifier.identifier is pretty ugly, is there another way to do this? Acknowledged. https://codereview.appspot.com/303250043/diff/100001/plaso/containers/interfa... plaso/containers/interface.py:52: dict[str, object]: attribute values. On 2017/01/19 16:46:24, onager wrote: > mapping of attribute names to attribute values. Acknowledged. https://codereview.appspot.com/303250043/diff/100001/plaso/containers/interfa... plaso/containers/interface.py:92: AttributeContainerIdentifier: identifier. On 2017/01/19 16:46:24, onager wrote: > "A unique identifier for the container which does not persist across > serialization" or something similarly verbose. Acknowledged. https://codereview.appspot.com/303250043/diff/100001/plaso/containers/reports.py File plaso/containers/reports.py (right): https://codereview.appspot.com/303250043/diff/100001/plaso/containers/reports... plaso/containers/reports.py:14: images: a list containing ??? On 2017/01/19 16:46:24, onager wrote: > I think we can just remove this "images" attribute. Done. https://codereview.appspot.com/303250043/diff/100001/plaso/containers/reports... plaso/containers/reports.py:15: plugin_name: a string containing the name of the analysis plugin that On 2017/01/19 16:46:24, onager wrote: > Please add type information to these attributes, so that it's consistent. Acknowledged. https://codereview.appspot.com/303250043/diff/100001/plaso/engine/knowledge_b... File plaso/engine/knowledge_base.py (right): https://codereview.appspot.com/303250043/diff/100001/plaso/engine/knowledge_b... plaso/engine/knowledge_base.py:36: """str: codepage of the current session.""" Ack, will revisit at a later time. https://codereview.appspot.com/303250043/diff/100001/plaso/engine/knowledge_b... plaso/engine/knowledge_base.py:78: session_identifier (Optional[str])): session identifier, where "current" Ack, will revisit at a later time. https://codereview.appspot.com/303250043/diff/100001/plaso/engine/knowledge_b... plaso/engine/knowledge_base.py:269: """Reads the knowledge base values from a system configuration artifact. Ack, will revisit at a later time. https://codereview.appspot.com/303250043/diff/100001/plaso/engine/knowledge_b... plaso/engine/knowledge_base.py:332: ValueError: if the timezone is not supported. Ack, will revisit at a later time. https://codereview.appspot.com/303250043/diff/100001/plaso/engine/path_helper.py File plaso/engine/path_helper.py (right): https://codereview.appspot.com/303250043/diff/100001/plaso/engine/path_helper... plaso/engine/path_helper.py:54: path_spec (dfvfs.PathSpec): path specification. Ack, will revisit at a later time. https://codereview.appspot.com/303250043/diff/100001/plaso/engine/path_helper... plaso/engine/path_helper.py:91: def GetRelativePathForPathSpec(cls, path_spec, mount_path=None): Ack, will revisit at a later time. https://codereview.appspot.com/303250043/diff/100001/plaso/output/timesketch_... File plaso/output/timesketch_out.py (right): https://codereview.appspot.com/303250043/diff/100001/plaso/output/timesketch_... plaso/output/timesketch_out.py:73: list[str]: names of argument that are required by the module and have Ack, will revisit at a later time. https://codereview.appspot.com/303250043/diff/100001/plaso/parsers/mediator.py File plaso/parsers/mediator.py (right): https://codereview.appspot.com/303250043/diff/100001/plaso/parsers/mediator.p... plaso/parsers/mediator.py:47: """bool: True if the parsing should be aborted.""" Ack, will revisit at a later time. https://codereview.appspot.com/303250043/diff/100001/plaso/parsers/mediator.p... plaso/parsers/mediator.py:52: """stt: codepage.""" Ack, will revisit at a later time. https://codereview.appspot.com/303250043/diff/100001/plaso/parsers/mediator.p... plaso/parsers/mediator.py:132: """Retrieves the inode from the inode value. Ack, will revisit at a later time. https://codereview.appspot.com/303250043/diff/100001/plaso/parsers/mediator.p... plaso/parsers/mediator.py:333: """Retrieves the current parser chain. Ack, moved container ids to CL: https://codereview.appspot.com/312320043/
Sign in to reply to this message.
<snip> Is this CL still valid, or have the changes been moved elsewhere? https://codereview.appspot.com/312320043/ has an identical description. Please add more detail to the description to allow reviewers to track reviews.
Sign in to reply to this message.
<snip> Is this CL still valid, or have the changes been moved elsewhere? https://codereview.appspot.com/312320043/ has an identical description. Please add more detail to the description to allow reviewers to track reviews.
Sign in to reply to this message.
It's in the spreadsheet, as indicated earlier and in the spreadsheet I'm splitting this CL. Please review: https://codereview.appspot.com/312320043/ I'll salvage this one at a later point
Sign in to reply to this message.
for the reviewer no need to review this CL yet
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
