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

Send and Accept locale environment variables #167

Merged
merged 1 commit into from
Feb 2, 2017

Conversation

mikemoate
Copy link
Member

Fixes #160

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 0632a9a on mikemoate:issue_160_locale into b882cdd on dev-sec:master.

@mikemoate
Copy link
Member Author

@artem-sidorenko tests seem to fail on Oracle only, I'm not sure why, and I can't see how to view the specific failures in Travis?

@artem-sidorenko
Copy link
Member

@mikemoate you just click the failed job and can see the log: https://travis-ci.org/dev-sec/chef-ssh-hardening/jobs/196915943

I looks totally unrelated to your changes and actually looks like master should be also broken. I will have a look

@artem-sidorenko
Copy link
Member

it looks like this is related to the fix within train: inspec/train@6b5d582

We have to add oracle platform to the ssh-baseline, I'll create a PR

@mikemoate
Copy link
Member Author

OK thanks. I could see the Travis logs but not the details of the failing test cases.

Now I realise they are just very un-obvious (compared to the test passed lines which are highlighted red), maybe I'll give TravisCI some feedback :-)

@artem-sidorenko
Copy link
Member

@mikemoate I guess this color problem is related somehow to the bash-color done by inspec. Usually it looks properly

@artem-sidorenko
Copy link
Member

@mikemoate the dev-sec/ssh-baseline#80 should fix the tests

@@ -112,3 +112,6 @@ Compression yes

# http://undeadly.org/cgi?action=article&sid=20160114142733
UseRoaming <%= @node['ssh-hardening']['ssh']['client']['roaming'] ? 'yes' : 'no' %>

#Send locale environment variables
SendEnv LANG LC_* LANGUAGE
Copy link
Member

Choose a reason for hiding this comment

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

Could we have it configurable? Some attribute like ['ssh-hardening']['ssh']['client']['send_env'] as with ['LANG', 'LC_*', 'LANGUAGE'] as default in the attributes/default.rb

@@ -193,6 +193,9 @@ UseDNS <%= ((@node['ssh-hardening']['ssh']['server']['use_dns']) ? 'yes' : 'no'
#ChrootDirectory none
#ChrootDirectory /home/%u

#Accept locale environment variables
AcceptEnv LANG LC_* LANGUAGE
Copy link
Member

Choose a reason for hiding this comment

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

Could we have it configurable too? Attribute like ['ssh-hardening']['ssh']['server']['accept_env'] with ['LANG', 'LC_*', 'LANGUAGE'] as default in the attributes/default.rb would be cool

@@ -64,6 +64,11 @@
)
end

it 'accepts locale environment variables' do
expect(chef_run).to render_file('/etc/ssh/sshd_config').
with_content('AcceptEnv LANG LC_* LANGUAGE')
Copy link
Member

Choose a reason for hiding this comment

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

If you implement it as attribute, could you please add the second test: to verify the attribute change? Good example is here for allow_root_with_key attribute

@artem-sidorenko
Copy link
Member

Thank you for this PR! I've added some comments:)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a9adb7d on mikemoate:issue_160_locale into b882cdd on dev-sec:master.

@mikemoate
Copy link
Member Author

@artem-sidorenko I think this covers everything you asked for now.

@artem-sidorenko
Copy link
Member

@mikemoate looks good to me, could you please also cleanup the commit history by squashing the commits?

Use attributes to set the environment variables that ssh client should send and that ssh daemon should accept.
The primary use case here is for locale, and the default attribute value reflects this (as discussed in dev-sec#160).

Chefspec tests cover the default, custom/overriden and empty cases for the attributes.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling d5d7ea6 on mikemoate:issue_160_locale into b882cdd on dev-sec:master.

@mikemoate
Copy link
Member Author

@artem-sidorenko squashed as requested.

@artem-sidorenko artem-sidorenko merged commit 41d98b3 into dev-sec:master Feb 2, 2017
@artem-sidorenko
Copy link
Member

@mikemoate thank you!

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

Successfully merging this pull request may close these issues.

3 participants