starred/hardening

Fork 0

mirror of https://github.com/konstruktoid/hardening.git synced 2026-04-26 01:05:56 +03:00

[GH-ISSUE #59] Noticed Some Weirdness #25

New issue

Closed

opened 2026-03-03 13:58:29 +03:00 by kerem · 1 comment

kerem commented

2026-03-03 13:58:29 +03:00

Owner

Originally created by @ghost on GitHub (Jan 23, 2020).
Original GitHub issue: https://github.com/konstruktoid/hardening/issues/59

Hi,

I'm looking through some of your code now, and have some things like I'd like to address:

File: ubuntu.sh

There are a lot of, and I'd wager the same in other scripts, redundant processes. So many process make this very inefficient and only serve to raise the dependency count. You're using Bash, so I recommend using Bash. ;) For example, printf is more than capable of telling and storing the time and date, making date redundant in most cases.
You have a lot of error messages, but none of them seem to redirect to STDERR; is that by design? Also, since you're repeating the same code over and over, that's a good opportunity to make use of a function. For example:

die(){
  printf 'ERROR: %s\n' "$2" 1>&2
  [ $1 -gt 0 ] && exit $1
}

You seem to be needlessly storing command locations (determined by $PATH anyway, hence command) only to go ahead and use the variables. It's a lot of work and code for no gain, at least that I can see. Am I missing something here? If it's a security issue, you're already using $PATH, as mentioned, so I don't see the gain there.
You have 53 functions running in this file, as sourced by 53 other files. All that extra processing and hassle -- I feel like there's a better way, which will reduce the hassle. Perhaps a function library within the one file, then the main code in the other? That seems to be a fairly common approach.
I get the impression that you're a long-time shell programmer who's got much more experience with Bourne shell programming. The reason, is because I see a lot of Bourne-isms, I guess you could say. Bash has made a lot of that stuff entirely unnecessary. For example, there's $USER, instead of using yet another process for id -un. There's $UID instead of yet another process for id -u, and one more example, there's $HOSTNAME making hostname redundant in probably most cases. I suggest you read up on the Bash manual; it'll change your world.
Lines 88-93, you have a for loop iterating over files to be sourced, but you're only checking 'they' exist, not what they actually are; they could be directories, links, block specials, FIFOs, etc. I recommend using -f instead. It's usually good to be more specific than just -e.
This isn't too big a deal, at least how I've seen you use echo, but I would strongly recommend you ditch echo, saving it only for quick terminal stuff, as printf is more feature-rich (especially in Bash) and far more reliable and consistent.
While there are at least two naming conventions to things like variables and functions, there is one which is typically shunned, and that's the all-caps way. I actually did the same thing for a very long time, until I finally stopped being so stubborn. :P I'm glad I did, because ThisMethod is a lot easier to read.

Hope this helps. Let me know if you'd be interested in some pull requests.

BTW, this is some seriously awesome stuff! Must have taken a lot of research.

Originally created by @ghost on GitHub (Jan 23, 2020). Original GitHub issue: https://github.com/konstruktoid/hardening/issues/59 Hi, I'm looking through some of your code now, and have some things like I'd like to address: File: `ubuntu.sh` * There are a _lot_ of, and I'd wager the same in other scripts, redundant processes. So many process make this very inefficient and only serve to raise the dependency count. You're using Bash, so I recommend _using_ Bash. ;) For example, `printf` is more than capable of telling and storing the time and date, making `date` redundant in most cases. * You have a lot of error messages, but none of them seem to redirect to STDERR; is that by design? Also, since you're repeating the same code over and over, that's a good opportunity to make use of a function. For example: ``` die(){ printf 'ERROR: %s\n' "$2" 1>&2 [ $1 -gt 0 ] && exit $1 } ``` * You seem to be needlessly storing command locations (determined by `$PATH` anyway, hence `command`) only to go ahead and use the variables. It's a lot of work and code for no gain, at least that I can see. Am I missing something here? If it's a security issue, you're already using `$PATH`, as mentioned, so I don't see the gain there. * You have 53 functions running in this file, as sourced by 53 other files. All that extra processing and hassle -- I feel like there's a better way, which will reduce the hassle. Perhaps a function library within the one file, then the main code in the other? That seems to be a fairly common approach. * I get the impression that you're a long-time shell programmer who's got much more experience with Bourne shell programming. The reason, is because I see a lot of Bourne-isms, I guess you could say. Bash has made a lot of that stuff entirely unnecessary. For example, there's `$USER`, instead of using yet another process for `id -un`. There's `$UID` instead of yet another process for `id -u`, and one more example, there's `$HOSTNAME` making `hostname` redundant in probably most cases. I suggest you read up on the Bash manual; it'll change your world. * Lines 88-93, you have a for loop iterating over files to be sourced, but you're only checking 'they' exist, not what they actually _are_; they could be directories, links, block specials, FIFOs, etc. I recommend using `-f` instead. It's usually good to be more specific than just `-e`. * This isn't too big a deal, at least how I've seen you use `echo`, but I would strongly recommend you ditch `echo`, saving it only for quick terminal stuff, as `printf` is more feature-rich (especially in Bash) and far more reliable and consistent. * While there are at least two naming conventions to things like variables and functions, there is one which is typically shunned, and that's the all-caps way. I actually did the same thing for a very long time, until I finally stopped being so stubborn. :P I'm glad I did, because ThisMethod is a _lot_ easier to read. Hope this helps. Let me know if you'd be interested in some pull requests. BTW, this is some _seriously_ awesome stuff! Must have taken a lot of research.

kerem closed this issue

2026-03-03 13:58:29 +03:00

kerem commented

2026-03-03 13:58:30 +03:00

Author

Owner

@konstruktoid commented on GitHub (Jan 24, 2020):

Hi @terminalforlife, and thanks for your comments!

There are a lot of, and I'd wager the same in other scripts, redundant processes. So many process make this very inefficient and only serve to raise the dependency count. You're using Bash, so I recommend using Bash. ;) For example, printf is more than capable of telling and storing the time and date, making date redundant in most cases.

date is used in
https://github.com/konstruktoid/hardening/blob/master/scripts/18_sshdconfig#L9 as a simple way to create a backup since the SSH configuration file is the one file most people are nervous about messing with.

At https://github.com/konstruktoid/hardening/blob/master/scripts/18_sshdconfig#L107-L109 it's used to create a dump file, could use mktemp there of course.

https://github.com/konstruktoid/hardening/blob/master/ubuntu.sh#L41 is about filling the CHANGEME= variable, could be just any random string instead of date.

Nothing bad about printf but it's overly complicated is many situations.

You have a lot of error messages, but none of them seem to redirect to STDERR; is that by design? Also, since you're repeating the same code over and over, that's a good opportunity to make use of a function. For example:

die(){
printf 'ERROR: %s\n' "$2" 1>&2
[ $1 -gt 0 ] && exit $1
}

Do you mean a lot of error messages because of incorrect code or as a result of e.g. missing packages on the host?

since you're repeating the same code over and over

Which code is that?

You seem to be needlessly storing command locations (determined by $PATH anyway, hence command) only to go ahead and use the variables. It's a lot of work and code for no gain, at least that I can see. Am I missing something here? If it's a security issue, you're already using $PATH, as mentioned, so I don't see the gain there.

Are you referring to https://github.com/konstruktoid/hardening/blob/master/ubuntu.cfg#L14-L42? Those a the configuration files, as variables as they may change.

I can't find an occasion where I use command -v with a full path.

You have 53 functions running in this file, as sourced by 53 other files. All that extra processing and hassle -- I feel like there's a better way, which will reduce the hassle. Perhaps a function library within the one file, then the main code in the other? That seems to be a fairly common approach.

Splitting up the functions is just an easier way for me to find the code that messes with a specific thing, and also reducing the number of lines of code in one file.

And the functions are include at https://github.com/konstruktoid/hardening/blob/master/ubuntu.sh#L88-L93. See below.

I get the impression that you're a long-time shell programmer who's got much more experience with Bourne shell programming. The reason, is because I see a lot of Bourne-isms, I guess you could say. Bash has made a lot of that stuff entirely unnecessary. For example, there's $USER, instead of using yet another process for id -un. There's $UID instead of yet another process for id -u, and one more example, there's $HOSTNAME making hostname redundant in probably most cases. I suggest you read up on the Bash manual; it'll change your world.

Are you referring to https://github.com/konstruktoid/hardening/blob/master/ubuntu.sh#L40? That will catch the using logged in, not the user (since sudo is used) running the script.

$HOSTNAME only returns the hostname, not FQDN but sure, it could be used at https://github.com/konstruktoid/hardening/blob/master/ubuntu.sh#L152.

Lines 88-93, you have a for loop iterating over files to be sourced, but you're only checking 'they' exist, not what they actually are; they could be directories, links, block specials, FIFOs, etc. I recommend using -f instead. It's usually good to be more specific than just -e.

Absolutly.

This isn't too big a deal, at least how I've seen you use echo, but I would strongly recommend you ditch echo, saving it only for quick terminal stuff, as printf is more feature-rich (especially in Bash) and far more reliable and consistent.

Feel free to submit a PR with printf improved code.

While there are at least two naming conventions to things like variables and functions, there is one which is typically shunned, and that's the all-caps way. I actually did the same thing for a very long time, until I finally stopped being so stubborn. :P I'm glad I did, because ThisMethod is a lot easier to read.

https://google.github.io/styleguide/shell.xml#Naming_Conventions, so the variables names is in need of a touching up.

Hope this helps. Let me know if you'd be interested in some pull requests.

Thanks for the comments and I'm always interested in pull requests.

BTW, this is some seriously awesome stuff! Must have taken a lot of research.

Thanks!

@konstruktoid commented on GitHub (Jan 24, 2020): Hi @terminalforlife, and thanks for your comments! > There are a lot of, and I'd wager the same in other scripts, redundant processes. So many process make this very inefficient and only serve to raise the dependency count. You're using Bash, so I recommend using Bash. ;) For example, printf is more than capable of telling and storing the time and date, making date redundant in most cases. > `date` is used in https://github.com/konstruktoid/hardening/blob/master/scripts/18_sshdconfig#L9 as a simple way to create a backup since the SSH configuration file is the one file most people are nervous about messing with. At https://github.com/konstruktoid/hardening/blob/master/scripts/18_sshdconfig#L107-L109 it's used to create a dump file, could use `mktemp` there of course. https://github.com/konstruktoid/hardening/blob/master/ubuntu.sh#L41 is about filling the `CHANGEME=` variable, could be just any random string instead of date. Nothing bad about `printf` but it's overly complicated is many situations. > You have a lot of error messages, but none of them seem to redirect to STDERR; is that by design? Also, since you're repeating the same code over and over, that's a good opportunity to make use of a function. For example: > > die(){ > printf 'ERROR: %s\n' "$2" 1>&2 > [ $1 -gt 0 ] && exit $1 > } Do you mean a lot of error messages because of incorrect code or as a result of e.g. missing packages on the host? > since you're repeating the same code over and over Which code is that? > You seem to be needlessly storing command locations (determined by $PATH anyway, hence command) only to go ahead and use the variables. It's a lot of work and code for no gain, at least that I can see. Am I missing something here? If it's a security issue, you're already using $PATH, as mentioned, so I don't see the gain there. > Are you referring to https://github.com/konstruktoid/hardening/blob/master/ubuntu.cfg#L14-L42? Those a the configuration files, as variables as they may change. I can't find an occasion where I use `command -v` with a full path. > You have 53 functions running in this file, as sourced by 53 other files. All that extra processing and hassle -- I feel like there's a better way, which will reduce the hassle. Perhaps a function library within the one file, then the main code in the other? That seems to be a fairly common approach. > Splitting up the functions is just an easier way for me to find the code that messes with a specific thing, and also reducing the number of lines of code in one file. And the functions are include at https://github.com/konstruktoid/hardening/blob/master/ubuntu.sh#L88-L93. See below. > I get the impression that you're a long-time shell programmer who's got much more experience with Bourne shell programming. The reason, is because I see a lot of Bourne-isms, I guess you could say. Bash has made a lot of that stuff entirely unnecessary. For example, there's $USER, instead of using yet another process for id -un. There's $UID instead of yet another process for id -u, and one more example, there's $HOSTNAME making hostname redundant in probably most cases. I suggest you read up on the Bash manual; it'll change your world. > Are you referring to https://github.com/konstruktoid/hardening/blob/master/ubuntu.sh#L40? That will catch the using logged in, not the user (since `sudo` is used) running the script. `$HOSTNAME` only returns the hostname, not FQDN but sure, it could be used at https://github.com/konstruktoid/hardening/blob/master/ubuntu.sh#L152. > Lines 88-93, you have a for loop iterating over files to be sourced, but you're only checking 'they' exist, not what they actually are; they could be directories, links, block specials, FIFOs, etc. I recommend using -f instead. It's usually good to be more specific than just -e. > Absolutly. > This isn't too big a deal, at least how I've seen you use echo, but I would strongly recommend you ditch echo, saving it only for quick terminal stuff, as printf is more feature-rich (especially in Bash) and far more reliable and consistent. > Feel free to submit a PR with `printf` improved code. > While there are at least two naming conventions to things like variables and functions, there is one which is typically shunned, and that's the all-caps way. I actually did the same thing for a very long time, until I finally stopped being so stubborn. :P I'm glad I did, because ThisMethod is a lot easier to read. > https://google.github.io/styleguide/shell.xml#Naming_Conventions, so the variables names is in need of a touching up. > Hope this helps. Let me know if you'd be interested in some pull requests. Thanks for the comments and I'm always interested in pull requests. > BTW, this is some seriously awesome stuff! Must have taken a lot of research. Thanks!

No labels

Stale

bug

pull-request

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

starred/hardening#25

No description provided.

Rows
Columns