Merged
Conversation
Co-Authored-By: William Stearns <[email protected]>
0x6d6f7468
reviewed
May 2, 2025
installer/install_scripts/sshprep
Outdated
| ip4=$(dig +short ${2} A) | ||
| ip6=$(dig +short ${2} AAAA) | ||
| ip4=$(dig +nocomment +short ${2} A 2>/dev/null | sed -e 's/;;.*//' | grep -v '^$') | ||
| ip6=$(dig +nocomment +short ${2} AAAA 2>/dev/null | sed -e 's/;;.*//' | grep -v '^$') |
Contributor
There was a problem hiding this comment.
A few things here:
- Should we handle cases where one hostname corresponds to multiple IP addresses? There are a few cases that I'm aware of in which this could happen and it might cause commands to fail. If for some reason we needed to treat each IP address as different, we could possibly handle it by iterating over the value of
ip4andip6variables. However, I think in most cases that this could happen, we would be better off just picking one IP address. - To my knowledge, adding
| sed -e 's/;;.*//' | grep -v '^$'is made redundant by the combination of+nocommentand+shortdig arguments. Unless I'm mistaken, we could probably cut those.
Contributor
Author
There was a problem hiding this comment.
- You are correct - the Hostname field where $ipv4 and/or $ipv6 are used only supports a single address. I agree, limiting this to a single IP makes sense. Would you please add
| head -1after the grep -v '^$' on both the ipv4 and ipv6 lines and before the right parenthesis? - I thought that was true as well and that's why the first version did not have the sed and grep commands. Unfortunately I was wrong and these have to be part of the lookup.
Contributor
Author
There was a problem hiding this comment.
I've updated the file in the update-sshprep branch (to include head -1 for both the ipv4 and ipv6 variable assignments. It's not clear whether the change propagated to the PR or not.
Contributor
There was a problem hiding this comment.
Thanks for making that change. For future reference, any pushes you make to a branch after PR will be associated with that PR. Makes for easy response to change requests (meaning that you did it correctly here).
Just curious, can you provide a scenario that you found during testing where having +nocomment +short still resulted in empty lines and comments (;;)? From the looks of it, +nocomment was added to the dig command at the same time as the sed and grep commands.
Thanks!
Add Bradley's suggestion of using head -1 to limit to a single address.
Contributor
Author
|
Thanks for letting me know that!
Off the top of my head I can't. I can say that the change was
specifically required because a bunch of dns-style comments were getting
stuck in places that only expected an IP address.
…On Mon, May 5, 2025 at 4:50 PM moth ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In installer/install_scripts/sshprep
<#45 (comment)>:
> @@ -230,8 +230,8 @@ check_config_block() {
prepend_config_block "\nHost ${2} ${local_ip}\n\tHostname\t\t${local_ip}\n\tHostKeyAlias\t\t${2}\n${user_line}\n"
else
status "No user-supplied target IP or hostname for $2, look one up"
- ip4=$(dig +short ${2} A)
- ip6=$(dig +short ${2} AAAA)
+ ip4=$(dig +nocomment +short ${2} A 2>/dev/null | sed -e 's/;;.*//' | grep -v '^$')
+ ip6=$(dig +nocomment +short ${2} AAAA 2>/dev/null | sed -e 's/;;.*//' | grep -v '^$')
Thanks for making that change. For future reference, any pushes you make
to a branch after PR will be associated with that PR. Makes for easy
response to change requests.
Just curious, can you provide a scenario that you found during testing
where having +nocomment +short still resulted in empty lines and comments
(;;)? Thanks.
—
Reply to this email directly, view it on GitHub
<#45 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA272WJTBRRMAKURQ5HFOXL247FJRAVCNFSM6AAAAAB3XMR46CVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDQMJWGA2TOOBWGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
lisaSW
approved these changes
May 5, 2025
caffeinatedpixel
added a commit
that referenced
this pull request
Sep 22, 2025
* Update on config structure, functionality, and tests Co-Authored-By: Naomi Kramer <[email protected]> * Extend subnet type to read/write from db, update tests Co-Authored-By: Liza Tsibur <[email protected]> * updated read file config test and subnet tests * fixed config and util tests, updated subnet related functions Co-Authored-By: Naomi Kramer <[email protected]> * Remove error return from GetDefaultConfig Co-Authored-By: Liza Tsibur <[email protected]> * added json tags to database struct * Updated beacon weights validation for config * updates to score thresholds validation tags * changes to config subnet validation and testing * Update subnet.go * Write missing host entries to http to populate http_proto * Updating some fields to uint64 * WIP update some field types * Update zeek count types and fix tests * Add clickhouse credentials * Misc fixes * Update pointer * Add ability to mark datasets as sample datasets * fix column name * Fix datasets exiting import if hour is empty * Fix zeek count parsing from TSV files * Remove storing dns conns in arrays, Fix historical first seen dns lag * Remove unused columns * Update config.hjson * Update config.hjson * updated impact category score functions to use float64 Co-Authored-By: Naomi Kramer <[email protected]> * Update subnet.go * Store import version in imports table * Fix duplicated SNI/IP long connections * Update subnet_test.go * Cleanup output * Rolling files updates (#39) * Limit number of days to import for rolling datasets * Fix breaking imports when import was interrupted * Remove debug output --------- Co-authored-by: Naomi Kramer <[email protected]> * Omit parts of env from output * Set max for threat intel datasize * Remove SELinux neutering for QA * Add network size column * Fix http_proto for missing host, update tests for missing host fixes * Add online feeds to default config * Update sshprep (#45) * Update sshprep Co-Authored-By: William Stearns <[email protected]> * Update sshprep Add Bradley's suggestion of using head -1 to limit to a single address. --------- Co-authored-by: Naomi Kramer <[email protected]> Co-authored-by: William Stearns <[email protected]> * Installer Behavior Tweaks (#41) * Add --yes flag to add-apt-repository command * Add missing sudo flags, make sure we're using the SUDO variable instead * Add ability to perform zone transfers (#48) * Store zone transfer records Co-Authored-By: moth <[email protected]> * Update config * Add tests * Tests, connectivity test * Update tests --------- Co-authored-by: moth <[email protected]> * Support RedHat/RHEL as a valid target (#47) * Update sshprep Co-Authored-By: William Stearns <[email protected]> * Supporrt RedHat/RHEL as a valid target --------- Co-authored-by: Naomi Kramer <[email protected]> Co-authored-by: William Stearns <[email protected]> Co-authored-by: moth <[email protected]> * Fix tests (#49) * Fix tests * Update WalkFiles to use UTC * fixed issue with rolling datasets over 24hours old not getting historical first seen timestamp set (#52) * Change values from float32 to float64 (#50) * Switch float32 to float64 * Update threat category calculation to match CalculateBucketedScore (#51) --------- Co-authored-by: Liza Tsibur <[email protected]> * Bump max query execution time default value * Use string instead of error for ZoneTransferConnectivityErrors struct fields (#61) * Upgrade Golang to version 1.24 (#59) (#60) * Replace get_url with shell and curl (#58) * Update sshprep Co-Authored-By: William Stearns <[email protected]> * Replace get_url with shell and curl * Use get_url by default, fall back to curl if it fails --------- Co-authored-by: Naomi Kramer <[email protected]> Co-authored-by: William Stearns <[email protected]> Co-authored-by: moth <[email protected]> * add automated log transfer, AC-Hunter issue 135 (#62) * Update sshprep Co-Authored-By: William Stearns <[email protected]> * add automated log transfer, PR135 * cron requires non-executable permission * Specify suggested YAML plugin and config in VSCode workspace * Linting and light cleanup * Update generate_installer.sh Download zeek_log_transport.sh to send to the sensor. * Create cron file if remote zeek installation * Only run zeek log import steps for remote sensor installations --------- Co-authored-by: Naomi Kramer <[email protected]> Co-authored-by: William Stearns <[email protected]> Co-authored-by: moth <[email protected]> * Temporarily disable RITA/Zeek log transport until installer is modular (#66) * Uniform -y flag usage for repo management/package installation; Uniform SUDO variable usage (#68) * Resolve Installer Side Effects and Formalize RHEL Support (#73) * Add missing necessary wildcards for RHEL versions * Remove Ansible task replacing python3-requests to avoid RHEL distro installation side effects * Update supported distros in README * Update scoring defaults * Resolve Ansible Reboot Errors (#75) * Clean up conditionals; Fix reboot step for Ubuntu * Suppress erroneous error output on RPM systems, ignore errors on reboot necessity checks * Ignore missing host rows for openhttp (#76) * Fix integration tests due to prevalence (#77) --------- Co-authored-by: Liza Tsibur <[email protected]> Co-authored-by: moth <[email protected]> Co-authored-by: William Stearns <[email protected]> Co-authored-by: William Stearns <[email protected]> Co-authored-by: moth <[email protected]>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
No description provided.