-
Notifications
You must be signed in to change notification settings - Fork 92
Improve RedHat OS detection #1079
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 11 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
abf7d22
Improve RedHat OS detection
JSCU-CNI 5097c42
change parse_hostname_string to callable tuple
JSCU-CNI 1f90a29
Apply suggestions from code review
JSCU-CNI ce49013
add os detection tests
JSCU-CNI f075088
fix linter
JSCU-CNI 4c5f8a2
Merge branch 'main' into improvement/redhat-detection
JSCU-CNI 2de869f
make _parse_hostname_string return tuple
JSCU-CNI eb633af
Apply suggestions from code review
JSCU-CNI 577e76a
fix py3.9 tests
JSCU-CNI de813d5
Merge branch 'main' into improvement/redhat-detection
JSCU-CNI 371dc0a
Merge branch 'main' into improvement/redhat-detection
Schamper 3097eb7
implement review comments
JSCU-CNI 43505c6
Change _parse_hosts_string & add test_parse_domain test case
Horofic 9394c1c
Adjust tests
Horofic 3f6f6bb
Revert "Adjust tests"
Horofic 343c12a
Change domain property behaviour
Horofic c065510
Merge branch 'main' into improvement/redhat-detection
JSCU-CNI File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,19 +1,26 @@ | ||
| from typing import Optional | ||
| from __future__ import annotations | ||
|
|
||
| from dissect.target.filesystem import Filesystem | ||
| from dissect.target.plugins.os.unix.linux._os import LinuxPlugin | ||
| from dissect.target.target import Target | ||
|
|
||
|
|
||
| class RedHatPlugin(LinuxPlugin): | ||
| """RedHat, CentOS and Fedora Plugin.""" | ||
|
|
||
| def __init__(self, target: Target): | ||
| super().__init__(target) | ||
|
|
||
| @classmethod | ||
| def detect(cls, target: Target) -> Optional[Filesystem]: | ||
| # also applicable to centos (which is a red hat derivative) | ||
| for fs in target.filesystems: | ||
| if fs.exists("/etc/sysconfig/network-scripts"): | ||
| return fs | ||
| def detect(cls, target: Target) -> Filesystem | None: | ||
| REDHAT_PATHS = { | ||
| "/etc/centos-release", | ||
| "/etc/fedora-release", | ||
| "/etc/redhat-release", | ||
| "/etc/sysconfig/network-scripts", # legacy detection | ||
| } | ||
|
|
||
| return None | ||
| for fs in target.filesystems: | ||
| for path in REDHAT_PATHS: | ||
| if fs.exists(path): | ||
| return fs |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Empty file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| from dissect.target.filesystem import VirtualFilesystem | ||
| from dissect.target.plugin import OperatingSystem | ||
| from dissect.target.plugins.os.unix.bsd.freebsd._os import FreeBsdPlugin | ||
| from dissect.target.target import Target | ||
| from tests._utils import absolute_path | ||
|
|
||
|
|
||
| def test_bsd_freebsd_os_detection(target_bare: Target) -> None: | ||
| """test if we detect FreeBSD correctly.""" | ||
|
|
||
| fs = VirtualFilesystem() | ||
| fs.map_file("/bin/freebsd-version", absolute_path("_data/plugins/os/unix/bsd/freebsd/freebsd-freebsd-version")) | ||
|
|
||
| target_bare.filesystems.add(fs) | ||
| target_bare.apply() | ||
|
|
||
| assert FreeBsdPlugin.detect(target_bare) | ||
| assert isinstance(target_bare._os, FreeBsdPlugin) | ||
| assert target_bare.os == OperatingSystem.BSD |
Empty file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| from io import BytesIO | ||
|
|
||
| from dissect.target.filesystem import VirtualFilesystem | ||
| from dissect.target.plugin import OperatingSystem | ||
| from dissect.target.plugins.os.unix.bsd.openbsd._os import OpenBsdPlugin | ||
| from dissect.target.target import Target | ||
|
|
||
|
|
||
| def test_bsd_openbsd_os_detection(target_bare: Target) -> None: | ||
| """test if we detect OpenBSD correctly.""" | ||
|
|
||
| fs = VirtualFilesystem() | ||
| fs.map_file_fh("/etc/myname", BytesIO(b"hostname")) | ||
| fs.makedirs("/bsd") | ||
|
|
||
| target_bare.filesystems.add(fs) | ||
| target_bare.apply() | ||
|
|
||
| assert OpenBsdPlugin.detect(target_bare) | ||
| assert isinstance(target_bare._os, OpenBsdPlugin) | ||
| assert target_bare.os == OperatingSystem.BSD |
Schamper marked this conversation as resolved.
Show resolved
Hide resolved
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| from io import BytesIO | ||
|
|
||
| import pytest | ||
|
|
||
| from dissect.target.filesystem import VirtualFilesystem | ||
| from dissect.target.plugin import OperatingSystem | ||
| from dissect.target.plugins.os.unix.linux.redhat._os import RedHatPlugin | ||
| from dissect.target.target import Target | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "file_name", | ||
| [ | ||
| ("/etc/redhat-release"), | ||
| ("/etc/centos-release"), | ||
| ("/etc/fedora-release"), | ||
| ("/etc/sysconfig/network-scripts"), | ||
| ], | ||
| ) | ||
| def test_unix_linux_redhat_os_detection(target_bare: Target, file_name: str) -> None: | ||
| """test if we detect RedHat OS correctly.""" | ||
|
|
||
| fs = VirtualFilesystem() | ||
| fs.map_file_fh(file_name, BytesIO(b"")) | ||
|
|
||
| target_bare.filesystems.add(fs) | ||
| target_bare.apply() | ||
|
|
||
| assert RedHatPlugin.detect(target_bare) | ||
| assert isinstance(target_bare._os, RedHatPlugin) | ||
| assert target_bare.os == OperatingSystem.LINUX |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah, that looked like a fun bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just because we're here anyway, could you add a unit test that would cover this scenario?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you ask the author of this piece of code instead (iirc @Horofic)? Not sure what the original purpose is of setting domain to "localhost".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof.
I've committed some small changes in 43505c6 and added a simple test. The second parameterised test case was my original intent. Which I believe, mimicked Debian, but not 100% clear on that anymore. This case intentionally fails now, feel free to implement it or take it in the direction you believe is more correct!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the content of
/etc/hostscontains127.0.0.1 mydomain(and no/etc/hostnameis found) in my opinion the parsed hostname should bemydomain. In this limited context we have no way to safely determine the difference between a hostname and domain.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very fair. I adjusts the test case in 9394c1c and added some additional test cases just to be sure. Please see if you agree with them. I also adjusted some other tests to take into account some of the
localhostbehaviour.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I think leaving the domain to
Nonemakes more sense instead of assuminglocalhost.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ill change the behaviour back later today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted in 3f6f6bb and adjusted the behaviour in 343c12a. This should do the trick now.