Skip to content
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

print config v2 Issue #191 #307

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

oam7575
Copy link
Contributor

@oam7575 oam7575 commented Nov 24, 2024

@jtesta @daniejstriata

  • printconfig script
  • test_printconfig for tox testing
  • update globals for GUIDES_UPDATED date value
  • update ssh_audit for print_config argument and checks

Quotes should no longer be needed.
usage : ssh-audit.py --print-config Ubuntu 2204 Server

 - printconfig script
 - test_printconfig for tox testing
 - update globals for GUIDES_UPDATED date value
 - update ssh_audit for print_config argument and checks
Copy link
Owner

@jtesta jtesta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting this PR! Here's some feedback on this draft.


from ssh_audit import exitcodes
from ssh_audit.globals import VERSION
from ssh_audit.globals import GUIDES_UPDATED
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think each platform should track an independent last-updated field. Just like the guides on the website, sometimes one platform needs an update, but the others don't.

Also, it would be a good idea to track a change log per platform (also like the online guides).

Perhaps the guides can be stored as a dictionary with "last_update", "change_log", and "instructions" fields. That would organize it in a cleaner way in the code, I think.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an example of what I mean by using a dictionary, see here:

BUILTIN_POLICIES: Dict[str, Dict[str, Union[Optional[str], Optional[List[str]], bool, Dict[str, Any]]]] = {

from ssh_audit.globals import GUIDES_UPDATED


class PrintConfig:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this class could be called HardeningGuides.

src/ssh_audit/printconfig.py Show resolved Hide resolved
sys.exit(retval)
else:
print(" ")
print(f"\033[1mSSH-Audit Version : {VERSION}\033[0m")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the class constructor accepts an OutputBuffer, then we can print using its good() method to handle colors for us.

Also, in the code, ssh-audit always refers to itself as "ssh-audit" (in lowercase).

src/ssh_audit/printconfig.py Show resolved Hide resolved
@@ -816,7 +817,10 @@ def process_commandline(out: OutputBuffer, args: List[str]) -> 'AuditConf': # p
parser.add_argument("--skip-rate-test", action="store_true", dest="skip_rate_test", default=False, help="skip the connection rate test during standard audits (used to safely infer whether the DHEat attack is viable)")
parser.add_argument("--threads", action="store", dest="threads", metavar="N", type=int, default=32, help="number of threads to use when scanning multiple targets (-T/--targets) (default: %(default)s)")

# The mandatory target option. Or rather, mandatory when -L, -T, or --lookup are not used.
# Print Suggested Configurations from : https://www.ssh-audit.com/hardening_guides.html
parser.add_argument("--print-config", nargs="*", action="append", metavar="OS Ver Client/Server", dest="print_configuration", type=str, default=None, help="print suggested server or client configurations. Usage Example : Ubuntu 2404 Server")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this option could be called --get-hardening-guide?

Also, the user would want to see what hardening guides are available, so perhaps the supported arguments to --get-hardening-guide can be listed with --list-hardening-guides.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the argument is called with out any further data ( server / version / edition ) or incorrectly formatted data it will print out the supported configurations.

With that said I have updated the configuration to print an example.

Would this be acceptable ?

python3 ssh-audit.py --get-hardening-guides

ssh-audit Version : v3.4.0-dev

Guides Last modified : 2024-10-01

Error unknown varient : OS Version Edition

For current, community developed and legacy guides
check the website : https://www.ssh-audit.com/hardening_guides.html

Supported Server Configurations :
Amazon 2023 Server
Debian Bookworm Server
Debian Bullseye Server
Rocky 9 Server
Ubuntu 2404 Server
Ubuntu 2204 Server
Ubuntu 2004 Server

Supported Client Configurations :
Amazon 2023 Client
Debian Bookworm Client
Mint 22 Client
Mint 21 Client
Mint 20 Client
Rocky 9 Client
Ubuntu 2404 Client
Ubuntu 2204 Client
Ubuntu 2004 Client

Example Usage :
python3 ssh-audit.py --get-hardening-guides Ubuntu 2404 Server

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the argument is called with out any further data ( server / version / edition ) or incorrectly formatted data it will print out the supported configurations.

Thinking from a user's perspective, usability would be better if we presented an obvious --list-hardening-guides option.

Would this be acceptable ?

Sure, that would work. My only feedback would be that having one last-modified field for all guides wouldn't be helpful to users; each platform should have its own history (date, integer version number, change log message, and hardening instructions).

Perhaps the current -L and -L -v behavior could serve as a model here. Notice how ssh-audit -L only shows the latest version of policies, but ssh-audit -L -v lists previous versions of the policies as well, including their change log messages. It would be nice if --list-hardening-guides did the same thing.



# pylint: disable=attribute-defined-outside-init
class TestAuditConf:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this class name should be updated to TestHardeningGuides.

@oam7575
Copy link
Contributor Author

oam7575 commented Nov 26, 2024

Hi Joe,

As always, thanks for the review and feedback.
I'll look into the changes and get back to you.

Again I will try and update and attempt to squash everything, and again if it turns into a mess - I'll close this one of and create a clean PR when the code is a bit closer to a second review.

Cheers.

@jtesta
Copy link
Owner

jtesta commented Nov 26, 2024

I will try and update and attempt to squash everything, and again if it turns into a mess - I'll close this one of and create a clean PR

One way to do it is to make a working branch for your next revision, add as many commits as you'd like to that, then simply make one squash merge commit back into print_guide_v2.

For example:

$ git checkout print_guide_v2  # Start with the branch that corresponds to this PR
$ git checkout -b pr307_revision_2  # Copy this branch, and use it for experimentation
$ git add [...]; git commit [...]  # Make as many commits as you'd like to this pr307_revision_2 branch
$ git checkout print_guide_v2; git merge --squash pr307_revision_2 branch  # Squash all commits from the experimental branch into one commit in print_guide_v2.

Once you push the one squashed commit in print_guide_v2, this PR will be automatically updated with that one commit. Hope this helps!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants