fervidus / secure_linux_cis

Apache License 2.0
16 stars 33 forks source link

Please support more distributions, like Debian 9 #18

Closed bjvrielink closed 4 years ago

bjvrielink commented 4 years ago

And before you reply 'wait, that's a lot of work', please look at https://github.com/bjvrielink/secure_linux_cis/tree/debian for an almost complete implementation I'm currently working on for debian9. It is still work in progress:

I could use some input/feedback on how I wrote the code for Debian 9 support. I tried to re-use as much code as possible, and made any class that was RedHat specific also work on Debian(9). This resulted in many 'redhat7' classes included in 'debian9.pp'. Which looks weird, but works. Any thoughts on a better way of doing this?

dan-wittenberg commented 4 years ago

Maybe some of those move to a 'common' section if we're re-using in others anyway? I'm all for it!

bryanjbelanger commented 4 years ago

Wait!

That seems like a good idea.

I’ll take a look tonight.

Thanks,

Bryan Belanger | Principal Consultant

phone: +1 (248) 613-2538 | email: bryan@fervid.us mailto:bryan@fervid.us

On Oct 29, 2019, at 12:12 PM, Daniel Wittenberg notifications@github.com wrote:

Maybe some of those move to a 'common' section if we're re-using in others anyway? I'm all for it!

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/fervidus/secure_linux_cis/issues/18?email_source=notifications&email_token=ACBKLJFZOOMWESPV7J7PU33QRBOHRA5CNFSM4JGINEE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECRDV2Y#issuecomment-547502827, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACBKLJFARDZEWHPNUTP3D5DQRBOHRANCNFSM4JGINEEQ.

bjvrielink commented 4 years ago

Keep in mind that the numbering of some of the checks are also different between distributions (see the comments in debian9.pp). Also, there is a "Distribution Independent Benchmark" that may be used for the 'common' section. Either way, it is a lot of work moving (almost) all classes to a new naming scheme. I'm not against contributing time towards that work, as long as there is a consensus before I commit to it :)

dan-wittenberg commented 4 years ago

I'm all for it assuming someone needs/wants it, which sounds like you at the very least. I think the lack of comprehensive CIS has been missing too long, so if it fills the gap!

bryanjbelanger commented 4 years ago

Created a new branch add_new_distributions. Will add a bunch of stuff today. Will have more time to clean it up week after next.

bryanjbelanger commented 4 years ago

@bjvrielink @dan-wittenberg @prolixalias @canihavethisone @prolixalias

I have pushed a new branch here: https://github.com/fervidus/secure_linux_cis/tree/add_new_distributions

This is still rough and I need suggestions.

I have all the CIS rules broken down by common, os and os major version. They use class names describing the rule vs. the CIS number. (Many rules are common after all)

I renamed the current CIS class to their rule name.

IF this path is taken we likely need to change the class array in Hiera to something we are doing already so we can pass parameters to the classes. Also need to pass boolean value.

I am looking for comments at this point.

Thanks,

Bryan

dan-wittenberg commented 4 years ago

The only thing I don't care for is the renaming, I think it finds it harder to find which one you are looking for. Usually when I get reports of non-compliance I get the number and maybe the name, but I think the numbering makes that easier and faster to see them in order as well, otherwise looks good so far!

Dan

On Sun, Nov 3, 2019 at 11:16 PM Bryan Belanger notifications@github.com wrote:

@bjvrielink https://github.com/bjvrielink @dan-wittenberg https://github.com/dan-wittenberg @prolixalias https://github.com/prolixalias @canihavethisone https://github.com/canihavethisone @prolixalias https://github.com/prolixalias

I have pushed a new branch here: https://github.com/fervidus/secure_linux_cis/tree/add_new_distributions

This is still rough and I need suggestions.

I have all the CIS rules broken down by common, os and os major version. They use class names describing the rule vs. the CIS number. (Many rules are common after all)

I renamed the current CIS class to their rule name.

IF this path is taken we likely need to change the class array in Hiera to something we are doing already so we can pass parameters to the classes. Also need to pass boolean value.

I am looking for comments at this point.

Thanks,

Bryan

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/fervidus/secure_linux_cis/issues/18?email_source=notifications&email_token=ADIIDAZ4MC7T6DGBWHVILWLQR6O2XA5CNFSM4JGINEE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC6ITGQ#issuecomment-549226906, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADIIDA7BC2EBHRFU7EQFMTTQR6O2XANCNFSM4JGINEEQ .

bryanjbelanger commented 4 years ago

The names and numbers don't match up across OSs. Any ideas how we can keep numbering without redundant classes? Maybe have a numbered shell class that 'contains' the named class?

dan-wittenberg commented 4 years ago

Yeah I like that better, so at least when you look at the different ones you can easily tell right where to go without having to go lookup something else.

On Mon, Nov 4, 2019 at 12:46 AM Bryan Belanger notifications@github.com wrote:

The names and numbers don't match up across OSs. Any ideas how we can keep numbering without redundant classes? Maybe have a numbered shell class that 'contains' the named class?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/fervidus/secure_linux_cis/issues/18?email_source=notifications&email_token=ADIIDAY3SKEL2DMPOA7V3HTQR6ZKTA5CNFSM4JGINEE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC6LN7I#issuecomment-549238525, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADIIDAYRDXF7O56SD6UF6H3QR6ZKTANCNFSM4JGINEEQ .

bjvrielink commented 4 years ago

Apologies for the lack of updates from my side the last few days. I've been busy... I've just added a few more commits to my fork, including support for Ubuntu 18.04.

canihavethisone commented 4 years ago

@bjvrielink were you at Microsoft Ignite in Orlando last week?

bjvrielink commented 4 years ago

@bjvrielink were you at Microsoft Ignite in Orlando last week?

I deny everything...

bryanjbelanger commented 4 years ago

HI @bjvrielink , what branch should I pull from to get Ubuntu stuff?

bjvrielink commented 4 years ago

Ubuntu18.04 is also in the Debian branch. Changes for Ubuntu were minimal, once Debian was done. I think it needs rebasing to this repo's master, but should be trivial.

canihavethisone commented 4 years ago

@bjvrielink were you at Microsoft Ignite in Orlando last week?

I deny everything...

I think we sat at the same table in the meals hall for lunch, maybe Wednesday I recognized your face and was trying to read your name badge but realized who you were after you left the table. Would have been great to have a chat.

bjvrielink commented 4 years ago

Having a chat indeed would have been great, but the chance of running into me in Orlando is quite low. Not only because I live in the Netherlands, but also because my profile picture is a little outdated and lacks the facial hair I currently have.

canihavethisone commented 4 years ago

Having a chat indeed would have been great, but the chance of running into me in Orlando is quite low. Not only because I live in the Netherlands, but also because my profile picture is a little outdated and lacks the facial hair I currently have.

It must have been your doppelganger. :) BTW I look exactly like Clint Eastwood. Exactly.

bryanjbelanger commented 4 years ago

@bjvrielink @dan-wittenberg @prolixalias @canihavethisone

I have pushed a new branch here: https://github.com/fervidus/secure_linux_cis/tree/add_new_distributions

Please review.

canihavethisone commented 4 years ago

@bryanjbelanger that layout conceptually looks awesome. I had a feeling that module-level hiera could be used here. I won't get to do any live testing with it for a few days however due to commitments (I like to do actual puppet runs, see if it breaks then run Nessus against the host).

One further thought is that all the shared profiles be renamed to just secure_linux_cis to make them version agnostic and avoid duplication. These base, or shared profiles would not contain any thing which is not common across all linux releases, and only uncommon settings would be in the release directories. This to me seems like the best way to refactor the module and make it ready for any/all releases. I see that you have actually done this with the new secure_linux_cis::rules classes

I have done some preliminary testing on CentOS 7 and found that NTP doesn't get processed (servers not written etc). I have not found why this is occurring, however chrony is fine.

Also as some classes are not enforced (5_5, 6_1_1, 6_1_13 and 6_1_14) they need to be commented out of the hiera until the profiles are implemented.

I will submit a PR with a few minor fixes.

bjvrielink commented 4 years ago

@bryanjbelanger great work!

With this huge change, it of course requires quite a bit testing. I'll try to make some time to help with this. However, once you're confident it's as good as the old code for RedHat/Centos7, I'd love to see a 2.0 version :)

bjvrielink commented 4 years ago

Did some testing on a Debian9 system. Made it to work (see my #25 ) Note that some older distributions like el6 require a lot of work before they can be used.

abuxton commented 4 years ago

I'd like to see the facts represent the related cis_benchmark in their names, or indeed one larger structured fact that shows pass/fail of the status of the benchmarks for reporting purposes something akin to https://github.com/ipcrm/ipcrm-demo_cis/blob/master/lib/facter/cis_redhat.rb or even

Facter.add(:secure_linux_cis_7) do
  confine :operatingsystem => 'CentOS'
  confine :operatingsystemmajrelease => '7'
 # 1.1.1.1 Ensure mounting of cramfs filesystems is disabled (Scored)
  results["1"]["1"]["1"]["1"] = {
    "title"  => "Ensure mounting of cramfs filesystems is disabled",
    "scored" => true,
    "level"  => { "server" => "1", "workstation" => "1" },
    "result" => "pass"
  }
  modprobe = Facter::Core::Execution.exec('/usr/sbin/modprobe -n -v cramfs')
  lsmod = Facter::Core::Execution.exec('/usr/sbin/lsmod | /usr/bin/grep "cramfs"')
  if ( modprobe != "install /bin/true" || lsmod != "" )
    results["1"]["1"]["1"]["1"]["result"] = "fail"
  end
bjvrielink commented 4 years ago

@abuxton At the moment the facts are more used as helper functions, not as pass/fail facts. Some facts are used by more than one benchmark item. Also, CIS is highly inconsistent in numbering of their benchmarks between distributions. 1.1.1.1 for example is about cramfs for Centos7, but about freevxfs for Debian9 (which is 1.1.1.2 for Centos7).

canihavethisone commented 4 years ago

I'm currently testing the 2.0.6 release, however a few questions/notes:

  1. How do you opt in for the level 2 settings that have been commented out? One method is for an implementer to duplicate the rules in environment or wrapping module hiera, however it would be good to have a way to opt for L1, L2 or both via a param
  2. The current version on the forge is 2.0.5, I am unsure if this is a publishing issue of if 2.0.6 didn't make it there yet
  3. the readme will need considerable rewriting with this new version, but I'm sure I am stating the obvious there.

I have not tested releases other than Centos 7 as yet, and always verify the results with Nessus, but I have to say that this refactored version 2x is looking REALLY good @bryanjbelanger - well done and a huge thank you for all the time and effort that you and other contributors have put into the is module. It's absolutely awesome.

bryanjbelanger commented 4 years ago

The L2 thing is a UX decision. I am open to suggestions. Some maybes:

1) Let users add them one by one in the include_rules array in the control repo. 2) Add something like 'All' to the profile_type parameter. 3) Something else...?

2.0.6 not packaged yet.

The README needs a rewrite. Other things in the classes need to be documented too. I am still working on SLES and Ubuntu profiles.

Also, some classes in RHEL 8 and Debian 9 need to be implemented.

Thanks,

Bryan

On Fri, Dec 27, 2019 at 8:15 PM canihavethisone notifications@github.com wrote:

I'm currently testing the 2.0.6 release, however a few questions/notes:

  1. How do you opt in for the level 2 settings that have been commented out? One method is for an implementer to duplicate the rules in environment or wrapping module hiera, however it would be good to have a way to opt for L1, L2 or both via a param
  2. The current version on the forge is 2.0.5, I am unsure if this is a publishing issue of if 2.0.6 didn't make it there yet
  3. the readme will need considerable rewriting with this new version, but I'm sure I am stating the obvious there.

I have not tested releases other than Centos 7 as yet, and always verify the results with Nessus, bu I have to say that this refactored version 2x is looking REALLY good Brian - well done and a huge thank you for all the time and effort that you and other contributors have put into the is module. It's absolutely awesome.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/fervidus/secure_linux_cis/issues/18?email_source=notifications&email_token=ACBKLJFHR6SFLOLEK4VOU23Q22SBTA5CNFSM4JGINEE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHX66AA#issuecomment-569372416, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACBKLJEBJXFNGF4VCA2KEHLQ22SBTANCNFSM4JGINEEQ .

canihavethisone commented 4 years ago

I think that the option should be exposed and not require repo (env) level hiera overrides.

edit - tested the attached files and changed my approach

My first second thought is to handle it by: i) increase the profile_type to 4 options, as per @bryanjbelanger 's suggestions. Note that level_2 will include level_1 as it is cumulative

ii) change the hiera keys to secure_linux_cis::workstation_rules_level_1: secure_linux_cis::workstation_rules_level_2: secure_linux_cis::server_rules_level_1: secure_linux_cis::server_rules_level_2:

iii) refactor the logic in init.pp that builds the $base_rules param to account for the above

I have tested the above with the attached 2 files (hiera is only updated for centos 7 so far) init.pp.txt 7.yaml.txt

canihavethisone commented 4 years ago

@bryanjbelanger, delighted to see the latest release - I hope to do some testing later this week. Re the commenting out of level 2 hardening, do you intend to separate them out and allow them to be enforced with a param set to true (as per the above post)? The only other way I can think of for an end user would be to duplicate the hiera data at the environment level and remove #'s.

bryanjbelanger commented 4 years ago

A cis_level parameter that accepts a 1 or 2 is best.

Default it in common.yaml.

Do you have an account in Puppet Community Slack?

Thanks,

Bryan

Sent from my iPhone

On Feb 16, 2020, at 4:51 AM, canihavethisone notifications@github.com wrote:



@bryanjbelangerhttps://github.com/bryanjbelanger, delighted to see the latest release - I hope to do some testing later this week. Re the commenting out of level 2 hardening, do you intend to separate then out and allow them to be enforced with a param set to true (as per the above post)? The only other way I can think of for an end user would be to duplicate the hiera data at the environment level and remove #'s.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/fervidus/secure_linux_cis/issues/18?email_source=notifications&email_token=ACBKLJDGJ2OUKCZJ2O5BB63RDED77A5CNFSM4JGINEE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEL4CGDI#issuecomment-586687245, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACBKLJFEBBYB4CDFETV2B63RDED77ANCNFSM4JGINEEQ.

bryanjbelanger commented 4 years ago

@prolixalias your thoughts?

prolixalias commented 4 years ago

@bryanjbelanger -- I got @canihavethisone 's suggestion implemented with minor modifications. Result looks like the example(s). Testing today... Once done, I'll mark #19 closed. Thanks for the input and contributions everyone!

prolixalias commented 4 years ago

A quick update on testing. This change quadrupled the number of tests. I’ve made it halfway through as of yesterday... Uncovered two level-two class issues that I’ll circle back on once the other tests are complete. Hoping to have this one wrapped up EOD tomorrow.

bjvrielink commented 4 years ago

Closing this issue