WIP: __fail2ban new type #379

Draft
smwltr wants to merge 1 commits from smwltr:fail2ban into master
First-time contributor

Does this need anything other than a man page?

Does this need anything other than a man page?
smwltr added 1 commit 2024-05-09 12:17:41 +00:00
smwltr changed title from WIP: __fail2ban to WIP: __fail2ban new type 2024-05-09 12:37:42 +00:00
fnux requested changes 2024-05-13 05:44:47 +00:00
@ -0,0 +30,4 @@
do
echo "[$(tput setaf 6)info$(tput sgr 0)] Enabling fail2ban for $service..." >&2
cat << EOF
perl -i -pe 'BEGIN{undef $/;} s/\[$service\].*[\n]*enabled.*=.*\n/\[$service\]\n\nenabled = true\n/g' $config_file
Collaborator

I see two problems here:

  • The configuration should be generated and installed in the manifest, not in gencode-remote.
  • You should not use perl in core cdist types - we only assume POSIX sh on the remote hosts.
I see two problems here: * The configuration should be generated and installed in the manifest, not in gencode-remote. * You should not use `perl` in core cdist types - we only assume POSIX sh on the remote hosts.
@ -0,0 +31,4 @@
require=__package/epel-release __package fail2ban --state present
;;
*)
echo "Your operating system ($os) is currently untested for ${__type##*/}." >&2
Collaborator

Just say that the type does not support $os and exit.

Just say that the type does not support `$os` and exit.
Author
First-time contributor

I'll need to make a testing set up for this. It's most likely that other OSes can use the same code, but I just haven't tested it.

I'll need to make a testing set up for this. It's most likely that other OSes can use the same code, but I just haven't tested it.
Collaborator

Well, we only ship code that is known to be working :-)

Well, we only ship code that is known to be working :-)
@ -0,0 +36,4 @@
__package fail2ban --state present
;;
esac
Collaborator

You also have to make sure that the fail2ban service starts on boot.

You also have to make sure that the fail2ban service starts on boot.
Author
First-time contributor

Is that typically what we do?

Or do we expect the user to use __start_on_boot?

Maybe I should rather fail when there is no fail2ban package installed. Then this type would be only for configuring the services that you want fail2ban to monitor (jails).

I could make an opiton and default to start it. Or I could add __start_on_boot and __package as an example in the man page?

What fits more with the cdist design?

Is that typically what we do? Or do we expect the user to use `__start_on_boot`? Maybe I should rather fail when there is no fail2ban package installed. Then this type would be only for configuring the services that you want fail2ban to monitor (jails). I could make an opiton and default to start it. Or I could add `__start_on_boot` and `__package` as an example in the man page? What fits more with the cdist design?
Collaborator

We usually install and enable (i.e. start_on_boot) the service within the type. Please run both __package and __start_on_boot in your type.

We usually install and enable (i.e. start_on_boot) the service within the type. Please run both `__package` and `__start_on_boot` in your type.
Author
First-time contributor

IMO, forcing something other than the default is not my style, but if that is standard cdist style, so be it.

IMO, forcing something other than the default is not my style, but if that is standard cdist style, so be it.
@ -0,0 +1 @@
enable-services
Collaborator

I find this parameter confusing - could you rename it? e.g. jail-service ?

I find this parameter confusing - could you rename it? e.g. `jail-service` ?
Author
First-time contributor

Would it be enable-jail? I suppose we need disable-jail too. Or?

Would it be `enable-jail`? I suppose we need `disable-jail` too. Or?
Collaborator

The type could generate configuration for the services specified in --jail-service and remove anything that isn't provided. You could make --jail-service an optional_multiple parameter.

Also mind that if we need to add per-service/jail configuration, a second __fail2ban_jail service will be needed (so that every jailed service can have its own configuration).

The type could generate configuration for the services specified in `--jail-service` and remove anything that isn't provided. You could make `--jail-service` an optional_multiple parameter. Also mind that if we need to add per-service/jail configuration, a second __fail2ban_jail service will be needed (so that every jailed service can have its own configuration).
Author
First-time contributor

IIUC, that would mean that cdist team be responsible for shipping working tested config for fail2ban.

Do you think that is out of scope? Have you seen how many jails are in the default config?

That's the reason why all this type does is allow you to enable a jail that is already in the config.

Before I remove the perl code, I'd like to understand this more. If we are just dis/enabling jails from the config, I can use sed. But generating a config for fail2ban requires much more code and maintenance. It doesn't make sense to me.

WDYT?

IIUC, that would mean that cdist team be responsible for shipping working tested config for fail2ban. Do you think that is out of scope? Have you seen how many jails are in the default config? That's the reason why all this type does is allow you to enable a jail that is already in the config. Before I remove the perl code, I'd like to understand this more. If we are just dis/enabling jails from the config, I can use sed. But generating a config for fail2ban requires much more code and maintenance. It doesn't make sense to me. WDYT?
Collaborator

IT would be a generic type to deploy a file in /etc/fail2ban/jail.d/myjail.conf (on debian at least). It doesn't have to support every configuration parameter, but should allow for custom configuration.

Its simplest form could looks like:

 __fail2ban_jail myjail --config <<- EOF
[apache-overflows]
enabled  = true
port     = http,https
filter   = apache-overflows
logpath  = /var/log/apache*/*error.log
maxretry = 2
EOF

I would also run fail2ban-client --test or similar in gencode-remote before reloading the configuration.

IT would be a generic type to deploy a file in /etc/fail2ban/jail.d/myjail.conf (on debian at least). It doesn't have to support every configuration parameter, but should allow for custom configuration. Its simplest form could looks like: ``` __fail2ban_jail myjail --config <<- EOF [apache-overflows] enabled = true port = http,https filter = apache-overflows logpath = /var/log/apache*/*error.log maxretry = 2 EOF ``` I would also run ` fail2ban-client --test` or similar in gencode-remote before reloading the configuration.
Author
First-time contributor

@fnux, sorry for the n00b question: how do I update a PR from my fork? I've pushed some changes, but I don't see them here.

@fnux, sorry for the n00b question: how do I update a PR from my fork? I've pushed some changes, but I don't see them here.
Collaborator

@fnux, sorry for the n00b question: how do I update a PR from my fork? I've pushed some changes, but I don't see them here.

There's currently an issue with this gitea instance. I asked ungleich to investigate it.

> @fnux, sorry for the n00b question: how do I update a PR from my fork? I've pushed some changes, but I don't see them here. There's currently an issue with this gitea instance. I asked ungleich to investigate it.
This pull request is broken due to missing fork information.
You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.
git checkout -b smwltr-fail2ban master
git pull fail2ban

Step 2:

Merge the changes and update on Gitea.
git checkout master
git merge --no-ff smwltr-fail2ban
git push origin master
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
2 Participants
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: ungleich-public/cdist#379
No description provided.