Adding --output-filename command line option like in Vol2.

[nathan-out]

With this option, Vol3 can produce by itself an output file. It can be useful in some cases, especially when Vol3 is launched by other tools which don't support output redirection operator ">". Usable with --output=<json|csv|text> to set the output format.

[eve-mem]

Hello @nathan-out - thank you for spending time on this and raising a PR!

I was going to suggest just using the --log option that already exists but it only includes log messages and not the text output - so that's a no go.

One thing that might be useful to consider is checks around being able to write to the file given on the command line, at the moment it'll crash vol. Have a look at how the --save-config option is doing that here:

volatility3/volatility3/cli/__init__.py

Lines 442 to 455 in 8f0d1b3

	if args.save_config:
	vollog.debug("Writing out configuration data to {args.save_config}")
	if os.path.exists(os.path.abspath(args.save_config)):
	parser.error(
	f"Cannot write configuration: file {args.save_config} already exists"
	)
	with open(args.save_config, "w") as f:
	json.dump(
	dict(constructed.build_configuration()),
	f,
	sort_keys=True,
	indent=2,
	)
	f.write("\n")

A very minor point is the (like in Volatility2). part of the help feels a bit ugly to me. I'd probably just leave it at ``Write output in this file. Default is stdout.`. That's maybe just personal preference.

Thanks again, keep them coming!

🦊 Just a random internet vol user

[nathan-out]

Hello @eve-mem thanks for your feedback !
I just improved the help : Write output in this file. Default is stdout. and add a check on the output file, like you described.

Do you know if volatility integrates pull requests into its core branch? It's my very first PR so I don't know how it works.

[eve-mem]

Yup, once a core dev takes a look and thinks the way it's done etc is good. I'd imagine it'll be @ikelos to look over this one.

Assuming it's all approved it'll be merged into the develop branch, and then a few times a year develop is merged into main. Although frankly i always just use develop :)

It might be that this isn't merged in, because it doesn't fit well or it's already there in some way, or there is a better way to do the way thing. If that's the case don't be disheartened, there are plenty of ways to get involved and everything helps. :)

[nathan-out]

Great! Thanks for the information, I'll be stopping by regularly to see what the core developers have to say :)

[atcuno]

I really like this idea. It would be good to have it merged @ikelos

[ikelos]

Well, I'm not super keen on the idea because I can't generally think of a situation where a program can run volatility and get a file from the filesystem but cannot access the stdout output stream, however, since it only affect the CLI and it seems as though there are some supporters of the idea, I can't see a good reason to block it.

However, there are comments to this PR that need to be looked at before it gets merged (notably, how the output filename is being passed to the renderers). It might be better to pass in a file descriptor as a parameter, and however it happens, it should probably be as a RendererOption (fleshing them out in the process) rather than the current method.

[ikelos]

Can I ask why this change was included? It doesn't seem relevant to the purpose of the PR, and it makes what's going on much harder for a person to parse. What benefit does the change provide?

[ikelos]

This also doesn't adhere to the renderer interface. output_filename isn't one of the parameters expected by the Renderer class and I'm surprised you can just access output_filename and have it work? This should probably be a RendererOption (which were never fully fleshed out, but we probably should get round to doing at some point), which renderers can then choose to act on or not.

See https://github.com/volatilityfoundation/volatility3/blob/develop/volatility3/framework/interfaces/renderers.py#L38.

[nathan-out]

You are right this change was not necessary. It was a dirty way, I juste add args.output_filename as constructor attribute. See fa7e97a

[ikelos]

Since this is a repetitive change of code, it suggests I didn't do a great job when designing these classes, it probably should have been an input parameter you could provide somewhere, but not to worry. 5:)

[ikelos]

This code should probably be drawn out into a class method, so it can be reused by all the renderers, which will also mean there's only one place to update given this needs to check whether the file already exists and then decide to write to it, rename the file or abort (and giving the user a message saying which it did).

[nathan-out]

I fixed this, I enriched the CLIRender class with a new attribute : outfd. It's more elegant and avoid code duplication.

[nathan-out]

Hello @ikelos thanks for your comments. My first attempt was not optimal, as I duplicated code. It's now fixed by adding a new attribute to the CLIRenderer class; outfd (output file descriptor). Its value is set in __init.py__ line 484.

One word regarding your note :
My usecase was to automate Vol3 with KAPE tool. In a nutshell, KAPE is an orchestrator tool used in DFIR. It does not support redirection operator '>' and most of the embeded tools inside KAPE produces output files. To me, it was weird that Vol3 was not able to produce an output file on its own. (To be precise, KAPE can handle this kind of problem but not in the same way). The use case can be useful when you want to automate the tool.

[ikelos]

Thanks for that explanation. If you're getting an external tool to run volatility, then you could get it to run a script that pipes the output somewhere? It also occurs to me that this will cause confusion since there's now an output filename, and an output folder, which can be different?

It also seems as though this problem isn't unique to volatility. So much so that there's an option in KAPE to save the output of the tool as a result: https://ericzimmerman.github.io/KapeDocs/#!Pages\2.2-Modules.md#exportfile ?

The plugins were designed to make it easy to redirect their output, but this PR feels like it's been done quickly to solve a single problem rather than thinking through the use cases and designing something that will be possible to add to in the future. Also, given the suggested use case already had a solution, I'm going to reject this one for now I'm afraid.

If you really want to work up redirection, then focus on building out the RendererOptions so that they're generic and could be useful for lots of different features. I'll be happy to take a look at another attempt if you come up with one and have kept these comments in mind...

[ikelos]

This slightly break inheritance, and the flag added to control the CLI somewhat conflicts with the existing output directory flag.

[ikelos]

This doesn't inherit from the renderer's __init__ (https://github.com/nathan-out/volatility3-output-module/blob/ffaa7738329da6e21f4ae3505576dc003f05ae0f/volatility3/framework/interfaces/renderers.py#L38) so will break inheritance. As mentioned we've never fleshed out what the render options are, but the idea was to allow renderers to accept options and parameters as they needed, whereas this would be rushing the process for a single feature that hasn't yet been demonstrated to be necessary?

[nathan-out]

this PR feels like it's been done quickly to solve a single problem

Absolutely @ikelos, which is why I've tried to make my code cleaner after your initial comments.

For the use case, maybe you're right, the idea was also to provide the same options as what volatility2 had. Thanks for your feedback.