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

essay: differentiate redhat/debian, add extra conditions #44

Merged
merged 24 commits into from
Feb 8, 2017

Conversation

juju4
Copy link
Contributor

@juju4 juju4 commented Sep 18, 2016

  • differentiate test for redhat and debian
  • Trying to add some test dependent on environment variable to handle context
  • add tests: entropy, rsyslog, auditd

some tests should be able to handle multiple acceptable results like sysctl or syslog server (rsyslog here but could be syslog-ng or else)
Some maybe dependent on environment like auditd is not possible inside a container (docker or lxc) but it does not seem trivial to have node context (http://jakshi.com/blog/2014/05/12/accessing-chef-attributes-in-serverspec-tests/; test-kitchen/test-kitchen#174)

Feel free to had comment.
Also have a hardening profile like https://github.com/juju4/ansible-harden

@atomic111
Copy link
Member

@juju4 thanks for your PR. at first we have to fix the testing issue or maybe you want to fix this

@juju4
Copy link
Contributor Author

juju4 commented Oct 10, 2016

at this point, checking seems to fail to dependency on higher ruby release.
It's not something that I change so don't see why it fails.

I'm more looking into a way to have different group of acceptable conditions first (some sysctl or rsyslog/syslog-ng)

@juju4
Copy link
Contributor Author

juju4 commented Dec 19, 2016

from rvm/rvm#3583, seems a rubygems issue
updating seems the way to go: https://stackoverflow.com/questions/35103008/error-while-installing-rack
tried that and also pin rack for ruby<=2.2.2 but travis still fails.
on my ruby 2.3 test box, it's ok.

can you have a look?

Thanks

@atomic111
Copy link
Member

@juju4 thank you for the great work. you can remove the ruby 1.9.3 support. have a look at https://github.com/dev-sec/tests-ssh-hardening/blob/master/Gemfile. then it should work as you mentioned,

@juju4
Copy link
Contributor Author

juju4 commented Dec 19, 2016

Thanks for the hint. still unrelated issue
https://travis-ci.org/juju4/tests-os-hardening/jobs/185094285#L227

$ bundle exec rake
rake aborted!
NoMethodError: undefined method `last_comment' for #<Rake::Application:0x00000002bede38>
/home/travis/build/juju4/tests-os-hardening/vendor/bundle/ruby/2.3.0/gems/rubocop-0.36.0/lib/rubocop/rake_task.rb:24:in `initialize'

@atomic111
Copy link
Member

atomic111 commented Dec 19, 2016

@juju4 i tested the stuff and got the following gemfile

# encoding: utf-8

source 'https://rubygems.org'

gem 'rack', '~> 1.6.4'
gem 'rake'
gem 'inspec', '~> 1'
gem 'rubocop', '~> 0.44.0'
gem 'highline', '~> 1.6.0'

group :tools do
  gem 'github_changelog_generator', '~> 1.12.0'
end

now it is working and got pretty much rubocop errors. can you fix this, please

@juju4
Copy link
Contributor Author

juju4 commented Dec 19, 2016

with lighter Gemfile, got expected rubocop warning with ruby 2.3
https://travis-ci.org/juju4/tests-os-hardening/jobs/185148546#L214

but same error on rack for older ruby
https://travis-ci.org/juju4/tests-os-hardening/jobs/185148545#L176

I don't understand why so much difference with https://github.com/dev-sec/ssh-baseline (name change today?)

@atomic111
Copy link
Member

@juju4 i will tests this in the next days

Copy link
Member

@chris-rock chris-rock left a comment

Choose a reason for hiding this comment

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

Hi @juju4 thank you for all the work and the help to make our profile better. We are following best practices and I would like to see some references for additional security like bettercrypto.org etc.

In addition I would like to understand why the profile has not worked for you for debian or redhat. We are testing it continuously with our Chef/Ansible/Puppet implementation.

Could you please also share the difficulties with container environments. I'd like to understand the issues a little bit better.

@@ -18,6 +18,27 @@
# author: Dominik Richter
# author: Patrick Muench

if ENV['login_defs_umask']
Copy link
Member

Choose a reason for hiding this comment

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

@juju4 we should use InSpec attributes here:

val_user = attribute('user', default: 'alice', description: 'An identification for the user')
val_password = attribute('password', description: 'A value for the password')

See http://inspec.io/docs/reference/profiles/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that the docs/code are new since the time I tried. great addition!

it { should_not be_readable.by('other') }
end
describe file('/etc/shadow'), :if => os.family == 'redhat' do
Copy link
Member

Choose a reason for hiding this comment

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

InSpec does not support the Serverspec/RSpec if conditions :if => os.family == 'redhat'. Instead I would prefer to set a variable before and validate the value eg.

if os.redhat?
    shadow_group = 'root'
else if os.debian?
    shadow_group = 'shadow'
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. will review that

end
describe file('/etc/shadow'), :if => os.family == 'debian' do
its('group') { should eq 'shadow' }
it { should be_readable.by('group') }
Copy link
Member

Choose a reason for hiding this comment

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

Can please reference the debian hardening guide where you got this from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's more my testing experience on Ubuntu...
also see here https://help.ubuntu.com/community/FilePermissions

it { should be_readable.by('owner') }
it { should be_readable.by('group') }
it { should be_readable.by('other') }
end
describe file('/etc/login.defs'), :if => os.family == 'redhat' do
Copy link
Member

Choose a reason for hiding this comment

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

Why should this checked on redhat only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because unless you are not using different values, they are default and not needed/present in login.defs of ubuntu
http://manpages.ubuntu.com/manpages/trusty/man5/login.defs.5.html

SYS_UID_MAX (number), SYS_UID_MIN (number)
Range of user IDs used for the creation of system users by useradd
or newusers.

       The default value for SYS_UID_MIN (resp.  SYS_UID_MAX) is 101
       (resp.  UID_MIN-1).

which results in the same range 101-999 (except 100)

that something that is valid for other config files like ssh.
when the value is default, do we want to push it also?
issues

  • clutter config files
  • default can change depending on version (especially true with ssh)
  • more explicit

end
# describe login_defs, :if => os.family == 'debian' do
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain this step please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see above

@@ -142,9 +179,24 @@
'/usr/lib/pt_chown', # pseudo-tty, needed?
'/usr/lib/eject/dmcrypt-get-device',
'/usr/lib/mc/cons.saver' # midnight commander screensaver
# # from Ubuntu xenial
Copy link
Member

Choose a reason for hiding this comment

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

Could you please explain why those are commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

want more testings/comments

]

output = command('find / -perm -4000 -o -perm -2000 -type f ! -path \'/proc/*\' -print 2>/dev/null | grep -v \'^find:\'')
output = command('find / -perm -4000 -o -perm -2000 -type f ! -path \'/proc/*\' ! -path \'/var/lib/lxd/containers/*\' -print 2>/dev/null | grep -v \'^find:\'')
Copy link
Member

Choose a reason for hiding this comment

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

What happens with lxc here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this directory contains root of the multiple lxc containers active on the system = will repeat totally or not usual system

example

# find / -perm -4000 -o -perm -2000 -type f ! -path '/proc/*' -print 2>/dev/null | grep -v '^find:'
/var/lib/lxd/containers/default-ubuntu-1604-1481820784/rootfs/usr/bin/ssh-agent
/var/lib/lxd/containers/default-ubuntu-1604-1481820784/rootfs/usr/bin/expiry
/var/lib/lxd/containers/default-ubuntu-1604-1481820784/rootfs/usr/bin/mlocate
/var/lib/lxd/containers/default-ubuntu-1604-1481820784/rootfs/usr/bin/screen
/var/lib/lxd/containers/default-ubuntu-1604-1481820784/rootfs/usr/bin/bsd-write
/var/lib/lxd/containers/default-ubuntu-1604-1481820784/rootfs/usr/bin/chage
/var/lib/lxd/containers/default-ubuntu-1604-1481820784/rootfs/usr/bin/wall
/var/lib/lxd/containers/default-ubuntu-1604-1481820784/rootfs/usr/bin/crontab
...

ideally an option depending on what's your perspective on container: audit them from host with the host, from host with separate config, or inside the container.

@@ -65,3 +65,38 @@
it { should_not be_installed }
end
end

## can also be syslog-ng...
control 'package-07' do
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure why we should enforce a specific logging environment. Syslog is only one way to achieve logging

Copy link
Member

Choose a reason for hiding this comment

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

Can you please reference the hardening guide, where this is specified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

clearly I don't want enforce a specific solution but on the contrary, have a control validated if other tools are used

  • default is rsyslog
  • syslog-ng should be possible
    or an option to disable the control or an attribute. will do the latter

should probably include auditd (already done) and systemd-journald for recent systems.

describe kernel_parameter('net.ipv4.conf.all.forwarding') do
its(:value) { should eq 0 }
end
# end
Copy link
Member

Choose a reason for hiding this comment

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

we can add a only_if condition to the control block for servers where forwarding is required:

control 'sysctl-01' do
    impact 1.0
    title 'IPv4 Forwarding'
    desc "If you're not intending for your system to forward traffic between interfaces, or if you only have a single interface, the forwarding function must be disable."
    describe kernel_parameter('net.ipv4.ip_forward') do
      its(:value) { should eq 0 }
    end
    describe kernel_parameter('net.ipv4.conf.all.forwarding') do
      its(:value) { should eq 0 }
    end
    only_if { sysctl_forwarding == true }
end

@chris-rock
Copy link
Member

@juju4 cool project https://github.com/juju4/ansible-harden Thanks for mentioning it

@chris-rock
Copy link
Member

@juju4 Please rebase on latest master, that fixes the test issues.

@juju4
Copy link
Contributor Author

juju4 commented Dec 21, 2016

rubocop offenses left but does not seem different from upstream.

@chris-rock
Copy link
Member

@juju4 latest master is all green: https://travis-ci.org/dev-sec/linux-baseline

@juju4
Copy link
Contributor Author

juju4 commented Dec 22, 2016

done with rubocop...
while I'm 100% for good conventions, I find some a bit unusual, strange or not very clear messages like discussed there
houndci/hound#310
rubocop/rubocop#1730
rubocop/rubocop#1708

@atomic111
Copy link
Member

@juju4 thanks for adding all the stuff. great work!!!

@atomic111 atomic111 merged commit 50e28b5 into dev-sec:master Feb 8, 2017
@chris-rock
Copy link
Member

@artem-sidorenko put up a PR to implement this in #52

@artem-sidorenko
Copy link
Member

@chris-rock @atomic111 as soon as we get chef-os-hardening fully running with CI tests, I would wish that we run each PR of linux-baseline with chef-os-hardening (manually?)

And/or another suggestion, what about having in the CI of linux-baseline a job with master branch of chef-os-hardenung? (and maybe a similar thing for ssh-baseline/chef-ssh-hardening). Via this way we would have a cross testing, so problems with some changes like in this PR would be visible directly

@chris-rock
Copy link
Member

I like that idea. We should just make sure that this will be an optional cross check. A failure does not mean it cannot be merged. Lets open another ticket for that

artem-sidorenko added a commit to artem-forks/linux-baseline that referenced this pull request May 10, 2017
in order to match the defaults of all mainstream distros

Some of settings are removed, as the defaults of distros are different,
based on the intention of author [1] they are also not really important here

[1]: dev-sec#44 (comment)
artem-sidorenko added a commit to artem-forks/linux-baseline that referenced this pull request May 10, 2017
in order to match the defaults of all mainstream distros

Some of settings are removed, as the defaults of distros are different,
based on the intention of author [1] they are also not really important here

[1]: dev-sec#44 (comment)

Signed-off-by: Artem Sidorenko <[email protected]>
artem-sidorenko added a commit to artem-forks/linux-baseline that referenced this pull request May 10, 2017
in order to match the defaults of all mainstream distros

Some of settings are removed, as the defaults of distros are different,
based on the intention of author [1] they are also not really important here

[1]: dev-sec#44 (comment)

Signed-off-by: Artem Sidorenko <[email protected]>
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.

4 participants