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

IPv6 is not working still if its enabled #140

Closed
artem-sidorenko opened this Issue Dec 3, 2016 · 7 comments

Comments

Projects
None yet
3 participants
@artem-sidorenko
Member

artem-sidorenko commented Dec 3, 2016

The default listen value blocks listening of sshd on IPv6 interfaces:

default['ssh']['listen_to']               = ['0.0.0.0']     # sshd

This default should react on the IPv6 setting and allow listening on IPv6 too

@artem-sidorenko artem-sidorenko changed the title from IPv6 is block still if its enabled to IPv6 is not working still if its enabled Dec 3, 2016

@chris-rock

This comment has been minimized.

Show comment
Hide comment
@chris-rock

chris-rock Dec 4, 2016

Member

Thanks @artem-sidorenko that is a great idea

Member

chris-rock commented Dec 4, 2016

Thanks @artem-sidorenko that is a great idea

@artem-sidorenko

This comment has been minimized.

Show comment
Hide comment
@artem-sidorenko

artem-sidorenko Dec 17, 2016

Member

Question to the handling of such defaults, which depend on another attribute with a default value.

In chef-os-hardening a problem like this is solved like:

default['network']['ipv6']['enable'] = false

default['ssh']['listen_to'] = if node['network']['ipv6']['enable']
  ['0.0.0.0', '::']
else
  ['0.0.0.0']
end

The attribute files are loaded by chef in the run list order prior to the compilation of recipes. So basically in this implementation we do have only one way to change the ['network']['ipv6']['enable'] to true and ['ssh']['listen_to'] gets the value ['0.0.0.0', '::']:

  1. if we do default['network']['ipv6']['enable']=true in the run list before this cookbook - this value gets overwritten
  2. if we do default['network']['ipv6']['enable']=true in the run list after this cookbook - default['ssh']['listen_to'] is already set to ['0.0.0.0'] and won't be changed
  3. if we do override['network']['ipv6']['enable']=true in the run list before this cookbook - it works as desired, default['ssh']['listen_to']=true
  4. if we do override['network']['ipv6']['enable']=true in the run list after this cookbook - default['ssh']['listen_to'] is already set to ['0.0.0.0'] and won't be changed

In the end, its possible to change the defaults of attributes depending on other attributes only in the 3rd case: via override['network']['ipv6']['enable']=true in the run list before this cookbook. In my eyes its a bit limited and we should consider some other implementation ways.

I see following option - we move attributes like default['ssh']['listen_to'] to the recipe files:

  • ['network']['ipv6']['enable'] would be properly evaluated within default/override precedence levels
  • default['ssh']['listen_to'] is set in the recipe in the compile phase based on node['network']['ipv6']['enable']
  • if there is a need to override ['ssh']['listen_to'], it should be done via override['ssh']['listen_to'] (and we document it this way)

@chris-rock @arlimus @atomic111 opinions?

Member

artem-sidorenko commented Dec 17, 2016

Question to the handling of such defaults, which depend on another attribute with a default value.

In chef-os-hardening a problem like this is solved like:

default['network']['ipv6']['enable'] = false

default['ssh']['listen_to'] = if node['network']['ipv6']['enable']
  ['0.0.0.0', '::']
else
  ['0.0.0.0']
end

The attribute files are loaded by chef in the run list order prior to the compilation of recipes. So basically in this implementation we do have only one way to change the ['network']['ipv6']['enable'] to true and ['ssh']['listen_to'] gets the value ['0.0.0.0', '::']:

  1. if we do default['network']['ipv6']['enable']=true in the run list before this cookbook - this value gets overwritten
  2. if we do default['network']['ipv6']['enable']=true in the run list after this cookbook - default['ssh']['listen_to'] is already set to ['0.0.0.0'] and won't be changed
  3. if we do override['network']['ipv6']['enable']=true in the run list before this cookbook - it works as desired, default['ssh']['listen_to']=true
  4. if we do override['network']['ipv6']['enable']=true in the run list after this cookbook - default['ssh']['listen_to'] is already set to ['0.0.0.0'] and won't be changed

In the end, its possible to change the defaults of attributes depending on other attributes only in the 3rd case: via override['network']['ipv6']['enable']=true in the run list before this cookbook. In my eyes its a bit limited and we should consider some other implementation ways.

I see following option - we move attributes like default['ssh']['listen_to'] to the recipe files:

  • ['network']['ipv6']['enable'] would be properly evaluated within default/override precedence levels
  • default['ssh']['listen_to'] is set in the recipe in the compile phase based on node['network']['ipv6']['enable']
  • if there is a need to override ['ssh']['listen_to'], it should be done via override['ssh']['listen_to'] (and we document it this way)

@chris-rock @arlimus @atomic111 opinions?

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

@artem-sidorenko artem-sidorenko added the bug label Dec 22, 2016

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

@artem-sidorenko artem-sidorenko self-assigned this Dec 22, 2016

@artem-sidorenko artem-sidorenko added this to the v2.0.0 milestone Dec 22, 2016

artem-sidorenko added a commit to artem-forks/chef-ssh-hardening that referenced this issue Dec 23, 2016

artem-sidorenko added a commit to artem-forks/chef-ssh-hardening that referenced this issue Dec 23, 2016

artem-sidorenko added a commit to artem-forks/chef-ssh-hardening that referenced this issue Dec 23, 2016

artem-sidorenko added a commit to artem-forks/chef-ssh-hardening that referenced this issue Dec 23, 2016

artem-sidorenko added a commit to artem-forks/chef-ssh-hardening that referenced this issue Dec 23, 2016

artem-sidorenko added a commit to artem-forks/chef-ssh-hardening that referenced this issue Jan 3, 2017

@chris-rock chris-rock closed this in #148 Jan 3, 2017

@rjhornsby

This comment has been minimized.

Show comment
Hide comment
@rjhornsby

rjhornsby Jan 4, 2017

To save someone else the head scratching that I went through, this issue's changes mean that if you need to set the listen_to ipaddress attribute, you'll need to use override not default in your attribute file, like so:

override['ssh-hardening']['ssh']['server']['listen_to'] = node['ipaddress']

This addresses the CIS and chef base/ssh compliance finding that sshd should not be listening to 0.0.0.0

rjhornsby commented Jan 4, 2017

To save someone else the head scratching that I went through, this issue's changes mean that if you need to set the listen_to ipaddress attribute, you'll need to use override not default in your attribute file, like so:

override['ssh-hardening']['ssh']['server']['listen_to'] = node['ipaddress']

This addresses the CIS and chef base/ssh compliance finding that sshd should not be listening to 0.0.0.0

@artem-sidorenko

This comment has been minimized.

Show comment
Hide comment
@artem-sidorenko

artem-sidorenko Jan 4, 2017

Member

@rjhornsby I added a notice on the override vs default. Is there something wrong with it? Do you think it makes sense to have this example in the README?

Its the common use case, but I do not think its a good idea to make it the default this way. It might break the ssh access to the box if node['ipaddress'] points to a non-management IP in multi-interface setups.

Member

artem-sidorenko commented Jan 4, 2017

@rjhornsby I added a notice on the override vs default. Is there something wrong with it? Do you think it makes sense to have this example in the README?

Its the common use case, but I do not think its a good idea to make it the default this way. It might break the ssh access to the box if node['ipaddress'] points to a non-management IP in multi-interface setups.

@rjhornsby

This comment has been minimized.

Show comment
Hide comment
@rjhornsby

rjhornsby Jan 4, 2017

I added a notice on the override vs default. Is there something wrong with it? Do you think it
makes sense to have this example in the README?

I think it might help to have the example in the readme, or maybe some way to call out which attributes are set in recipes and must use override[]?

I'm relatively new to Chef (~3 months), coming over from Puppet. I thought I read somewhere that using default[] in an attribute file was almost always correct, so I was not understanding why setting listen_to wasn't working until I tried override[].

Is it good or common practice to use override[] (instead of default[]) in the attributes file of a wrapper cookbook?

Its the common use case, but I do not think its a good idea to make it the default this way.

I agree with you. Making the cookbook default to 0.0.0.0 is the right way to do it because, as you point out, it greatly reduces the chance of breaking ssh. I think it is fine to allow cookbook consumers to modify this using attributes to address the finding if they wish, in whatever way that is most appropriate for their environment.

rjhornsby commented Jan 4, 2017

I added a notice on the override vs default. Is there something wrong with it? Do you think it
makes sense to have this example in the README?

I think it might help to have the example in the readme, or maybe some way to call out which attributes are set in recipes and must use override[]?

I'm relatively new to Chef (~3 months), coming over from Puppet. I thought I read somewhere that using default[] in an attribute file was almost always correct, so I was not understanding why setting listen_to wasn't working until I tried override[].

Is it good or common practice to use override[] (instead of default[]) in the attributes file of a wrapper cookbook?

Its the common use case, but I do not think its a good idea to make it the default this way.

I agree with you. Making the cookbook default to 0.0.0.0 is the right way to do it because, as you point out, it greatly reduces the chance of breaking ssh. I think it is fine to allow cookbook consumers to modify this using attributes to address the finding if they wish, in whatever way that is most appropriate for their environment.

@artem-sidorenko

This comment has been minimized.

Show comment
Hide comment
@artem-sidorenko

artem-sidorenko Jan 4, 2017

Member

or maybe some way to call out which attributes are set in recipes and must use override[]?

yeah, this was exactly the point of @atomic111 in the review of #148 :) However I personally do not like this idea: this adds another documentation layer and usually such things get unmaintained very fast and are often not up-to-date. What about to have a short hint on the attribute itself? Like this:

Notice: Some of attribute defaults of this cookbook are set in the recipes. You should use a higher attribute precedence level for overriding of such attributes. Such attributes are flagged with `override attribute` in the list below. Example for overriding a such attribute:

override['ssh-hardening']['ssh']['server']['listen_to'] = node['ipaddress']

* `['ssh-hardening']['ssh']['server']['listen_to']` - `override attribute` - one or more ip addresses, to which ssh-server should listen to. Default is to listen on all interfaces. It should be configured for security reasons!

Is it good or common practice to use override[] (instead of default[]) in the attributes file of a wrapper cookbook?

I see both things quite often (and you also have force_default etc). I see default[] as a common practice as it works in the most cases, however not for all. I use the default[] way for very simple scenarios (where I'm sure it will not cause any problems) and use force_default or override for all complex cases

Member

artem-sidorenko commented Jan 4, 2017

or maybe some way to call out which attributes are set in recipes and must use override[]?

yeah, this was exactly the point of @atomic111 in the review of #148 :) However I personally do not like this idea: this adds another documentation layer and usually such things get unmaintained very fast and are often not up-to-date. What about to have a short hint on the attribute itself? Like this:

Notice: Some of attribute defaults of this cookbook are set in the recipes. You should use a higher attribute precedence level for overriding of such attributes. Such attributes are flagged with `override attribute` in the list below. Example for overriding a such attribute:

override['ssh-hardening']['ssh']['server']['listen_to'] = node['ipaddress']

* `['ssh-hardening']['ssh']['server']['listen_to']` - `override attribute` - one or more ip addresses, to which ssh-server should listen to. Default is to listen on all interfaces. It should be configured for security reasons!

Is it good or common practice to use override[] (instead of default[]) in the attributes file of a wrapper cookbook?

I see both things quite often (and you also have force_default etc). I see default[] as a common practice as it works in the most cases, however not for all. I use the default[] way for very simple scenarios (where I'm sure it will not cause any problems) and use force_default or override for all complex cases

@rjhornsby

This comment has been minimized.

Show comment
Hide comment
@rjhornsby

rjhornsby Jan 4, 2017

What about to have a short hint on the attribute itself? Like this:

+1, I like it.

rjhornsby commented Jan 4, 2017

What about to have a short hint on the attribute itself? Like this:

+1, I like it.

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