Add postgresql analyzer #1084
Add postgresql analyzer #1084
Conversation
There was a problem hiding this comment.
Wow, this is really comprehensive, thanks!
Here are some initial comments, though I haven't gone through the entire PR yet so feel free to ignore these comments until I do a second pass tomorrow. Mostly just some small code style kinds of nits.
FYI, I fixed the recent merge conflict too.
| _BASH_HISTORY = '.bash_history' | ||
| _PSQL_HISTORY = '.psql_history' | ||
|
|
||
| _POSTGRESQL_ARTIFACTS = """--- |
There was a problem hiding this comment.
I think this is good here for now, but should we upstream this Artifact into the main forensics artifact repo?
There was a problem hiding this comment.
I just realized that these artifacts are already in the ForensicArtifacts repo, or at least ones with the same name are there. Is there anything custom about these? If not, we can probably just use those by name with extract_artifacts() instead of re-defining it here.
| # Module: PostgreSQL Configuration Analysis | ||
| # 1.1. find postgresql.conf and copy to artifact directory | ||
| try: | ||
| artifact_locations, err = self._collect_artifact( |
There was a problem hiding this comment.
Looks like you're probably coming from golang based on the patterns here :).
I don't actually see err being returned with a value other than None in the method being called here, though there are other places that TurbiniaException is being raised. Rather than duplicating the error handling, and also to match style with the rest of the codebase (as well as be more idiomatic python), should we change these to just return the artifact locations and keep the try/except block error handling for these calls?
There was a problem hiding this comment.
Ack the recommendation. Removing unsued err return from code.
| return result | ||
|
|
||
| if not artifact_locations: | ||
| result.close(self, success=False, |
There was a problem hiding this comment.
There are a few locations that don't quite match the style guide and will probably fail the yapf unit test (e.g. normally the newline comes after the method name and paren here). Do you want to run yapf on the codebase to auto-fix some of these? If you installed the dev dependencies (pip install -e .[dev]) you should have yapf installed and can do something like yapf -i -r --style .style.yapf ./turbinia/.
There was a problem hiding this comment.
I will run yapf and fix the style.
|
|
||
| return final_report, final_priority, final_summary | ||
|
|
||
| def read_file(filepath): |
There was a problem hiding this comment.
We also have a file_to_str util method for this that checks for a couple error conditions (though I'm not sure why we don't raise TurbiniaExceptions there for errors). Could we potentially use that or check for errors here?
| return artifacts, None | ||
|
|
||
| def _get_artifact_disk_path(self, collected_artifact_path): | ||
| """Returns the artifact disk path. |
There was a problem hiding this comment.
Maybe just to be a bit clearer here, this could be something like "Returns the absolute path of the artifact on the original disk without the local mount prefix" or similar? That probably won't fit on one line though, so maybe add it as a second line or something?
There was a problem hiding this comment.
Done - Updated with a more clear description.
|
|
||
| (report, priority, summary), err = self._analyze_postgresql_config( | ||
| config_data) | ||
| if err: |
There was a problem hiding this comment.
See comments below, but we might be able to remove this.
|
|
||
| if not artifact_locations: | ||
| result.close(self, success=False, | ||
| status='Error setting artifact location') |
There was a problem hiding this comment.
Should this be "No pg_hba.conf found" as well? If not, maybe we can clarify what this means a bit?
There was a problem hiding this comment.
Replaced with "pg_hba.conf not found"
| for artifact_location in artifact_locations: | ||
| # we only want to process /etc/passwd | ||
| if '/etc/passwd' not in artifact_location: | ||
| result.log(f'Ignore passwd file {artifact_location}') |
There was a problem hiding this comment.
Maybe to be explicit here you could say "Ignoring filename {} not matching '/etc/passwd'"?
| result.log(f'Ignore passwd file {artifact_location}') | ||
| continue | ||
|
|
||
| result.log(f'Processing passwd: {artifact_location}') |
| # Module: User Bash History Analysis | ||
| # It includes all user bash history including postgres user account. | ||
| # 4. Find and analyze .bash_history | ||
| artifact_locations, err = self._collect_artifact(_BASH_HISTORY, evidence) |
There was a problem hiding this comment.
Should we just use the already existing BashShellHistoryFile artifact for this? That should also handle the directory so we don't need to search the whole disk for it as well (though we should make sure that it actually works with a mounted disk due to the mount prefix).
| summary=summary, report=report)) | ||
|
|
||
| # Module: PostgreSQL Database User Analysis | ||
| # 6. Find and analyze PostgreSQL database user analysis. |
There was a problem hiding this comment.
small nit: s/user analysis/users/
There was a problem hiding this comment.
Actually, should we just remove this whole comment block? It looks like it might be for functionality that was removed?
| with open(artifact_definition_file, 'wb') as fh: | ||
| fh.write(_POSTGRESQL_ARTIFACTS.encode('utf8')) | ||
|
|
||
| artifact_names = ['PostgreSQLLogFiles'] |
There was a problem hiding this comment.
See comment above, but unless this artifact actually has some differences from the one in the ForensicArtifacts repo, we should be able to just use the extract_artifacts() method and extract it by name instead of writing out the yaml file.
| _BASH_HISTORY = '.bash_history' | ||
| _PSQL_HISTORY = '.psql_history' | ||
|
|
||
| _POSTGRESQL_ARTIFACTS = """--- |
There was a problem hiding this comment.
I just realized that these artifacts are already in the ForensicArtifacts repo, or at least ones with the same name are there. Is there anything custom about these? If not, we can probably just use those by name with extract_artifacts() instead of re-defining it here.
| for line in re.findall(pattern, data): | ||
| if module_priority > event_priority: | ||
| module_priority = event_priority | ||
| module_summary = f'{name} detected' |
There was a problem hiding this comment.
Do we want to do something here similar to one of the other analyzer methods here that counts the number of findings and adds that as a summary instead of just using the last one found?
| report.insert(4, '{0:s}\n'.format(fmt.heading2('Detailed Analysis'))) | ||
|
|
||
| final_report = '\n'.join(report) | ||
| final_summary = '\n'.join(x[1] for x in summary) |
There was a problem hiding this comment.
The value that gets used as the final status as part of the result (as set in result.close()) should be a single line so that it fits into the final report for all Tasks. Should we change this to be something like N findings reported or similar?
| # Evidence mount point location i.e. Evidence.local_path | ||
| # Use export EVIDENCE_LOCAL_PATH='/mnt/mock' where test image is mounted | ||
| # to /mnt/mock | ||
| EVIDENCE_LOCAL_PATH = os.environ.get('EVIDENCE_LOCAL_PATH') |
| task = postgresql.PostgreSQLAnalysisTask() | ||
|
|
||
| # pylint: disable=protected-access | ||
| pg_config = task._read_postgresql_config(self.POSTGRESQL_CONF) |
There was a problem hiding this comment.
For some reason I can't seem to find this method? Is this supposed to be read_file()?
There was a problem hiding this comment.
Old function that is no longer in use. Removed from the test file.
|
|
||
|
|
||
| class PostgreSQLAnalysisTaskTest(TestTurbiniaTaskBase): | ||
| """Tests for PostgreSQLAnalysisTask.""" |
There was a problem hiding this comment.
Can you also add a quick test for the run() method and make sure that the result output is what you're expecting?
|
Hi @roshanmaskey , Just wanted to check in on this one to see if you had any updates. |
Postgres analyzer that analyzes postgresql installation and provides a TLDR and details findings report.