Configurable SSH Banner File #130

Merged
merged 1 commit into from Nov 8, 2016

Conversation

Projects
None yet
4 participants
@sidxz
Contributor

sidxz commented Nov 7, 2016

Path to the Banner File in template 'opensshd.conf.erb' is hard coded to '/etc/ssh/banner.txt'
Not all flavours use this. (for example, CentOS defaults to etc/issue.net)
I have made this configurable by adding an attribute.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 7, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 325fb9c on sidxz:master into 2d11a0f on dev-sec:master.

Coverage Status

Coverage remained the same at 100.0% when pulling 325fb9c on sidxz:master into 2d11a0f on dev-sec:master.

@artem-sidorenko

This comment has been minimized.

Show comment
Hide comment
@artem-sidorenko

artem-sidorenko Nov 8, 2016

Member

@sidxz thank you for PR! As different distros use different banner file, maybe we can provide reasonable defaults for each distribution? Like we do for package or service names

Member

artem-sidorenko commented Nov 8, 2016

@sidxz thank you for PR! As different distros use different banner file, maybe we can provide reasonable defaults for each distribution? Like we do for package or service names

@sidxz

This comment has been minimized.

Show comment
Hide comment
@sidxz

sidxz Nov 8, 2016

Contributor

@artem-sidorenko Thanks, that's surely a much better way to do it!
I have updated it!.

Contributor

sidxz commented Nov 8, 2016

@artem-sidorenko Thanks, that's surely a much better way to do it!
I have updated it!.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 8, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 4e1e757 on sidxz:master into 2d11a0f on dev-sec:master.

Coverage Status

Coverage remained the same at 100.0% when pulling 4e1e757 on sidxz:master into 2d11a0f on dev-sec:master.

@artem-sidorenko

This comment has been minimized.

Show comment
Hide comment
@artem-sidorenko

artem-sidorenko Nov 8, 2016

Member

@sidxz thank you!

I've checked ubuntu 16.04, centos 7 and opensuse-leap-42.1. They all have issue.net and it looks like issue.net is the common file which is used in this case for this banner. However, sshd does not evaluate the sequences which are usually present in this file, example login to some centos box with default issue.net:

$ ssh root@192.168.X.X
\S
Kernel \r on an \m

Still if it might get to a such "broken" banner, I would suggest to stick to issue.net in the defaults without any distribution differentiation:

  • issue.net is always present
  • issue.net is the most expected place for a such banner

@atomic111 any other opinion?

Member

artem-sidorenko commented Nov 8, 2016

@sidxz thank you!

I've checked ubuntu 16.04, centos 7 and opensuse-leap-42.1. They all have issue.net and it looks like issue.net is the common file which is used in this case for this banner. However, sshd does not evaluate the sequences which are usually present in this file, example login to some centos box with default issue.net:

$ ssh root@192.168.X.X
\S
Kernel \r on an \m

Still if it might get to a such "broken" banner, I would suggest to stick to issue.net in the defaults without any distribution differentiation:

  • issue.net is always present
  • issue.net is the most expected place for a such banner

@atomic111 any other opinion?

@atomic111

This comment has been minimized.

Show comment
Hide comment
@atomic111

atomic111 Nov 8, 2016

Member

this sounds good. may be we add a attribute to configure the text in the issue.net file. @artem-sidorenko and @sidxz What do think?

Member

atomic111 commented Nov 8, 2016

this sounds good. may be we add a attribute to configure the text in the issue.net file. @artem-sidorenko and @sidxz What do think?

@artem-sidorenko

This comment has been minimized.

Show comment
Hide comment
@artem-sidorenko

artem-sidorenko Nov 8, 2016

Member

@atomic111 👍

@sidxz what about to use node['ssh']['banner_content'], if its defined we manage the file node['ssh']['banner_path'] with its content?

Member

artem-sidorenko commented Nov 8, 2016

@atomic111 👍

@sidxz what about to use node['ssh']['banner_content'], if its defined we manage the file node['ssh']['banner_path'] with its content?

@sidxz

This comment has been minimized.

Show comment
Hide comment
@sidxz

sidxz Nov 8, 2016

Contributor

Thanks for replying @atomic111 and @artem-sidorenko.
I believe we are using hardening philosophy in this cookbook to enforce configuration. I am not so sure if defining the content of issue.net belongs to the hardening cookbook. (/etc/issue.net can always be written by the cookbook that installs sshd or a wrapper cookbook that wraps ssh-hardening). I think we should have this cookbook just to enforce (or not enforce) configurations. The way we (and I believe some other organizations too) handle issue.net is by configuring it dynamically by a completely separate system that generates content for issue.net depending on the physical location of the system. Declaring contents of issue.net in this cookbook will unnecessarily overwrite and rewrite this file in each chef-client run.
I believe following DRY, we can have only one attribute ['ssh']['banner'] that defaults to nil and can be overridden by the user to set a path like '/etc/issue.net'. Defaulting to 'nil' will ensure that installing the cookbook with all default options never breaks it.

Thanks

Contributor

sidxz commented Nov 8, 2016

Thanks for replying @atomic111 and @artem-sidorenko.
I believe we are using hardening philosophy in this cookbook to enforce configuration. I am not so sure if defining the content of issue.net belongs to the hardening cookbook. (/etc/issue.net can always be written by the cookbook that installs sshd or a wrapper cookbook that wraps ssh-hardening). I think we should have this cookbook just to enforce (or not enforce) configurations. The way we (and I believe some other organizations too) handle issue.net is by configuring it dynamically by a completely separate system that generates content for issue.net depending on the physical location of the system. Declaring contents of issue.net in this cookbook will unnecessarily overwrite and rewrite this file in each chef-client run.
I believe following DRY, we can have only one attribute ['ssh']['banner'] that defaults to nil and can be overridden by the user to set a path like '/etc/issue.net'. Defaulting to 'nil' will ensure that installing the cookbook with all default options never breaks it.

Thanks

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 8, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 15c35e0 on sidxz:master into 2d11a0f on dev-sec:master.

Coverage Status

Coverage remained the same at 100.0% when pulling 15c35e0 on sidxz:master into 2d11a0f on dev-sec:master.

@artem-sidorenko

This comment has been minimized.

Show comment
Hide comment
@artem-sidorenko

artem-sidorenko Nov 8, 2016

Member

@sidxz the idea of @atomic111 makes sense here as well. Example: within my org issue.net is mostly used for a usual "...your actions might be observed..."-disclaimer without any further information. In this use case this idea might be pretty useful

Anyway, lets go with your current suggestion, its the most flexible and simple in the same time;-)

could you please cleanup the commit history via squashing the commits?

Member

artem-sidorenko commented Nov 8, 2016

@sidxz the idea of @atomic111 makes sense here as well. Example: within my org issue.net is mostly used for a usual "...your actions might be observed..."-disclaimer without any further information. In this use case this idea might be pretty useful

Anyway, lets go with your current suggestion, its the most flexible and simple in the same time;-)

could you please cleanup the commit history via squashing the commits?

@artem-sidorenko

This comment has been minimized.

Show comment
Hide comment
@artem-sidorenko

artem-sidorenko Nov 8, 2016

Member

@sidxz could you also please add a line to the README.md about this attribute?

Member

artem-sidorenko commented Nov 8, 2016

@sidxz could you also please add a line to the README.md about this attribute?

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 8, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 0c07dec on sidxz:master into 2d11a0f on dev-sec:master.

Coverage Status

Coverage remained the same at 100.0% when pulling 0c07dec on sidxz:master into 2d11a0f on dev-sec:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 8, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 124efbe on sidxz:master into 5d58452 on dev-sec:master.

Coverage Status

Coverage remained the same at 100.0% when pulling 124efbe on sidxz:master into 5d58452 on dev-sec:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 8, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 124efbe on sidxz:master into 5d58452 on dev-sec:master.

Coverage Status

Coverage remained the same at 100.0% when pulling 124efbe on sidxz:master into 5d58452 on dev-sec:master.

@artem-sidorenko

This comment has been minimized.

Show comment
Hide comment
@artem-sidorenko

artem-sidorenko Nov 8, 2016

Member

@sidxz thank you!

Member

artem-sidorenko commented Nov 8, 2016

@sidxz thank you!

@artem-sidorenko artem-sidorenko merged commit a1af6df into dev-sec:master Nov 8, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@sidxz

This comment has been minimized.

Show comment
Hide comment
@sidxz

sidxz Nov 8, 2016

Contributor

@artem-sidorenko, @atomic111 Thank you!
I have done the changes :)

Contributor

sidxz commented Nov 8, 2016

@artem-sidorenko, @atomic111 Thank you!
I have done the changes :)

artem-sidorenko added a commit to artem-forks/chef-ssh-hardening that referenced this pull request Dec 22, 2016

artem-sidorenko added a commit to artem-forks/chef-ssh-hardening that referenced this pull request Dec 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment