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

ListenAddress should exist #165

Merged
merged 4 commits into from
May 20, 2020
Merged

ListenAddress should exist #165

merged 4 commits into from
May 20, 2020

Conversation

micheelengronne
Copy link
Member

@micheelengronne micheelengronne commented May 19, 2020

There can be multiple ListenAddress (for instance ipv6 and ipv4 failover).

Does the exist matcher work for sshd_config ?

There can be multiple ListenAddress (for instance ipv6 and ipv4 failover).

Signed-off-by: Michée Lengronne <[email protected]>
@@ -139,7 +139,7 @@
title 'Server: Specify ListenAddress'
desc "Limit the SSH server to listen to a specific address. Don't let it listen on all interfaces to avoid logins from unexpected sources."
describe sshd_config(sshd_custom_path + '/sshd_config') do
its('ListenAddress') { should match(/.*/) }
its('ListenAddress') { should exist }
Copy link
Member

Choose a reason for hiding this comment

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

I think the test should check for not nil then

Copy link
Member

@chris-rock chris-rock May 20, 2020

Choose a reason for hiding this comment

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

its probably a bug in InSpec if the nil matches that regex. What is the value for ListenAddress that you want to check against?

Copy link
Member Author

Choose a reason for hiding this comment

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

I want to check it agaisnt multiple defined ListenAddress. My usecase is ipv6+ipv4 failover. 2 ListenAddress that together give ["0.0.0.0", "::"]

The check fails with:

expected ["0.0.0.0", "::"] to match /.*/

Copy link
Member

Choose a reason for hiding this comment

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

Got ya. I see that the control itself was probably not correctly implemented in the first place.

Should we check that it is not nil and the entries should not include "0.0.0.0" or "::"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think 'not nil' is enough. I am not sure that "0.0.0.0" and "::" should be tested. For my usecase, i use these values as my sshd is in a container so the network and firewall are handled by the runtime (Docker, podman, CRI-O...). What are your thoughts on that ? Maybe, test "0.0.0.0" and "::" as an option via an attribute ?

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 struggling with InSpec's dynamic results at this check. We may need to test for more emptiness:

describe sshd_config(sshd_custom_path + '/sshd_config') do
   its('ListenAddress') { should_not eq nil }
   its('ListenAddress') { should_not match /^\s*$/ }
   its('ListenAddress') { should_not eq [] }
 end

Copy link
Member Author

Choose a reason for hiding this comment

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

Travis CI builds are green even if they appear running. Weird bug.

Signed-off-by: Michée Lengronne <[email protected]>
Signed-off-by: Michée Lengronne <[email protected]>
Signed-off-by: Michée Lengronne <[email protected]>
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.

Great improvement @micheelengronne

@chris-rock chris-rock merged commit 0932d5f into master May 20, 2020
@chris-rock chris-rock deleted the micheelengronne-patch-4 branch May 20, 2020 16:03
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.

2 participants