volatility3 icon indicating copy to clipboard operation
volatility3 copied to clipboard

Plugin: update plugin version, logger, dump options of `Certificates` plugin.

Open digitalisx opened this issue 3 years ago • 6 comments

Description

As I modified this PR (#734), I checked and updated some improvements to this plugin.

  • Most plugins of Volatility3 can choose the dump option if they have data that can be dumped or if they write it to a local file system. However, the Certificates plugin does not match the plugin's description that when it runs, it automatically dumps and simply lists against our intentions. So I added the dump option.
  • However, given existing plugin improvements, radical changes to certain features and commands can be a problem for other current users and systems. So I added a warning log and encouraged you to use the option. This will be removed one day. (Please let me know if you don't need this step, and I will modify it.)
  • The plugin does not have a KeyError, so I added it because does not show a log for the case that went over.
  • I changed the value to underscore (_) for the unused value in the iterator statement.

digitalisx avatar May 12 '22 11:05 digitalisx

Thanks, this mostly seems fine, a couple of minor points, nothing that'll stop it getting merged, but I'll leave a few days in case you would like to make changes, before I merge it.

Just so I document this in as many places as possible (and I know you've already heard this in #734): This plugin is specifically under volatility3/plugins rather than volatility3/framework/plugins because it's an example of a plugin we've had submitted that we're not intending to explicitly support, update or maintain. This is so that people know the purpose of the volatility3/plugins directory, and how they should use it for their own plugins rather than modifying the core ones in volatility3/framework. This is also the reason all imports should be for volatility3.plugins rather than volatility3.framework.plugins. I feel we should probably document this somewhere more clearly, but since it came up here, here's where I'm putting the explanation for now... 5;)

@ikelos First of all, thank you for taking the time to review.

Oh, that's right, I remember the comment you told me last time. I understood that this plugin is officially the plugin that we keep, but it went into that folder as an example. Please understand my inexperience.. hehe 😅

All right... Now that I understand the intentions, I probably won't be asking for a review or PR for the code in that path. But this time, thankfully, you proceeded with the review!

I think it's a good opportunity to give a little better user experience to some users who don't understand the intentions. 🙌 (The plugins shown in the help command do not distinguish them, so the average user thinks that this plugin is also an official plug-in and likely used as an official plugin.)

If you give me time to work on the review, I will solve it and request the review again. 🙂

digitalisx avatar May 15 '22 16:05 digitalisx

Sure, there's no rush! 5:) You're right it doesn't really make the distinction to the user, but it was more intended for developers to be aware. It is really as an example for others, so I'm happy to update it, I just wanted to set people's expectations that it might not get as much love as plugins we've accepted into core. 5:)

ikelos avatar May 15 '22 17:05 ikelos

After reviewing the code again, I forgot to return the file handle and specify the exact return type. I will check it carefully again before requesting a review.. 😅

digitalisx avatar May 16 '22 15:05 digitalisx

@ikelos Thank you for your review and suggestion, I will work on your suggestion. However, due to the schedule, it may take some time. 🙂

digitalisx avatar May 24 '22 01:05 digitalisx

While I was working on the documentation a recent, I found that some of the results I worked on had an old form. Therefore, I changed this to ensure that our code is consistent. Additionally, exception handling has been enhanced to allow users to see more results.

digitalisx avatar Jul 02 '22 10:07 digitalisx

@ikelos I'm sorry I couldn't keep my word that I would work on additional comments. Could you merge this when you have time? I am satisfied with the current version as well as I intended.. :) I'm going to focus on the remaining Drafts PR and the ones with a little higher priority.

digitalisx avatar Aug 07 '22 07:08 digitalisx