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

Possibly missing locale handling #160

Closed
mikemoate opened this Issue Jan 13, 2017 · 8 comments

Comments

Projects
None yet
3 participants
@mikemoate
Member

mikemoate commented Jan 13, 2017

I will freely admit my knowledge of locale in Linux is limited, hopefully others are more knowledgeable :-S

Whilst investigating some locale issues on our Ubuntu servers (14.04) I noticed that the ssh_config and sshd_config created by this cookbook lack the settings for sending and accepting locale, which differs from the default ubuntu/debian behaviour.

Specifically, sshd_config on debian would have:

AcceptEnv LANG LC_*

and ssh_config on debian would have:

SendEnv LANG LC_*

Can we add these back in, should we? (it won't fix our problem, but seems like an oversight)

@artem-sidorenko

This comment has been minimized.

Show comment
Hide comment
@artem-sidorenko

artem-sidorenko Jan 14, 2017

Member

@mikemoate thank you for raising this question,

from my POV we can add the accept_env and send_env attributes, however I'm not sure how the defaults should look like

[root@b59b2373a9c0 ~]# cat /etc/*release*
CentOS Linux release 7.2.1511 (Core) 
...
[root@b59b2373a9c0 ~]# grep Env /etc/ssh/ssh_config
	SendEnv LANG LC_CTYPE LC_NUMERIC LC_TIME LC_COLLATE LC_MONETARY LC_MESSAGES
	SendEnv LC_PAPER LC_NAME LC_ADDRESS LC_TELEPHONE LC_MEASUREMENT
	SendEnv LC_IDENTIFICATION LC_ALL LANGUAGE
	SendEnv XMODIFIERS
[root@b59b2373a9c0 ~]# grep Env /etc/ssh/sshd_config 
#PermitUserEnvironment no
AcceptEnv LANG LC_CTYPE LC_NUMERIC LC_TIME LC_COLLATE LC_MONETARY LC_MESSAGES
AcceptEnv LC_PAPER LC_NAME LC_ADDRESS LC_TELEPHONE LC_MEASUREMENT
AcceptEnv LC_IDENTIFICATION LC_ALL LANGUAGE
AcceptEnv XMODIFIERS

root@7adcc13493f1:~# cat /etc/*release*
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=16.04
DISTRIB_CODENAME=xenial
...
root@7adcc13493f1:~# grep Env /etc/ssh/ssh_config
    SendEnv LANG LC_*
root@7adcc13493f1:~# grep Env /etc/ssh/sshd_config
AcceptEnv LANG LC_*

It looks like LANG LC_* LANGUAGE should be the proper default option here. What do you think?

@atomic111 @chris-rock opinions?

Member

artem-sidorenko commented Jan 14, 2017

@mikemoate thank you for raising this question,

from my POV we can add the accept_env and send_env attributes, however I'm not sure how the defaults should look like

[root@b59b2373a9c0 ~]# cat /etc/*release*
CentOS Linux release 7.2.1511 (Core) 
...
[root@b59b2373a9c0 ~]# grep Env /etc/ssh/ssh_config
	SendEnv LANG LC_CTYPE LC_NUMERIC LC_TIME LC_COLLATE LC_MONETARY LC_MESSAGES
	SendEnv LC_PAPER LC_NAME LC_ADDRESS LC_TELEPHONE LC_MEASUREMENT
	SendEnv LC_IDENTIFICATION LC_ALL LANGUAGE
	SendEnv XMODIFIERS
[root@b59b2373a9c0 ~]# grep Env /etc/ssh/sshd_config 
#PermitUserEnvironment no
AcceptEnv LANG LC_CTYPE LC_NUMERIC LC_TIME LC_COLLATE LC_MONETARY LC_MESSAGES
AcceptEnv LC_PAPER LC_NAME LC_ADDRESS LC_TELEPHONE LC_MEASUREMENT
AcceptEnv LC_IDENTIFICATION LC_ALL LANGUAGE
AcceptEnv XMODIFIERS

root@7adcc13493f1:~# cat /etc/*release*
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=16.04
DISTRIB_CODENAME=xenial
...
root@7adcc13493f1:~# grep Env /etc/ssh/ssh_config
    SendEnv LANG LC_*
root@7adcc13493f1:~# grep Env /etc/ssh/sshd_config
AcceptEnv LANG LC_*

It looks like LANG LC_* LANGUAGE should be the proper default option here. What do you think?

@atomic111 @chris-rock opinions?

@mikemoate

This comment has been minimized.

Show comment
Hide comment
@mikemoate

mikemoate Jan 16, 2017

Member

I agree with your suggestion, and am happy to work up a PR for that, but will wait for other experts to give their opinions.

Member

mikemoate commented Jan 16, 2017

I agree with your suggestion, and am happy to work up a PR for that, but will wait for other experts to give their opinions.

@artem-sidorenko

This comment has been minimized.

Show comment
Hide comment
@artem-sidorenko

artem-sidorenko Jan 26, 2017

Member

@mikemoate I think you can just start to work on the PR:) man locale is the best expert regarding this topic and LANG LC_* LANGUAGE sound totally reasonable for my 16 years of unix experience with different locale setups (english/german/russian)

Member

artem-sidorenko commented Jan 26, 2017

@mikemoate I think you can just start to work on the PR:) man locale is the best expert regarding this topic and LANG LC_* LANGUAGE sound totally reasonable for my 16 years of unix experience with different locale setups (english/german/russian)

@mikemoate

This comment has been minimized.

Show comment
Hide comment
@mikemoate

mikemoate Jan 27, 2017

Member

@artem-sidorenko OK, I wasn't sure if you intended for me to wait for the other's you had mentioned in your comment to reply?

I'll try and get something together in the next few days. Any thoughts on testing this?

Member

mikemoate commented Jan 27, 2017

@artem-sidorenko OK, I wasn't sure if you intended for me to wait for the other's you had mentioned in your comment to reply?

I'll try and get something together in the next few days. Any thoughts on testing this?

@artem-sidorenko

This comment has been minimized.

Show comment
Hide comment
@artem-sidorenko

artem-sidorenko Jan 27, 2017

Member

@mikemoate

I wasn't sure if you intended for me to wait for the other's you had mentioned in your comment to reply?

I asked them if they have any opinion, if they would have anything to say here: we would already see a response (at least this is my experience) :)

I'll try and get something together in the next few days. Any thoughts on testing this?

I would just do unit tests, something like in GH-155

Member

artem-sidorenko commented Jan 27, 2017

@mikemoate

I wasn't sure if you intended for me to wait for the other's you had mentioned in your comment to reply?

I asked them if they have any opinion, if they would have anything to say here: we would already see a response (at least this is my experience) :)

I'll try and get something together in the next few days. Any thoughts on testing this?

I would just do unit tests, something like in GH-155

@chris-rock

This comment has been minimized.

Show comment
Hide comment
@chris-rock

chris-rock Jan 27, 2017

Member

We could also set the values on ubuntu only, therefore we would not set the values if the attribute is nil

Member

chris-rock commented Jan 27, 2017

We could also set the values on ubuntu only, therefore we would not set the values if the attribute is nil

@artem-sidorenko

This comment has been minimized.

Show comment
Hide comment
@artem-sidorenko

artem-sidorenko Jan 27, 2017

Member

@chris-rock there is a similar issue on RHEL/centos too. I think we should stick here with a default distro configuration, which is to accept/send the locale settings, otherwise it might cause unexpected problems. But we should allow to disable it

P.S. was my comment a wakeup call?:D

Member

artem-sidorenko commented Jan 27, 2017

@chris-rock there is a similar issue on RHEL/centos too. I think we should stick here with a default distro configuration, which is to accept/send the locale settings, otherwise it might cause unexpected problems. But we should allow to disable it

P.S. was my comment a wakeup call?:D

@chris-rock

This comment has been minimized.

Show comment
Hide comment
@chris-rock

chris-rock Jan 27, 2017

Member

I was not sleeping @artem-sidorenko :-) All good, sounds reasonable.

Member

chris-rock commented Jan 27, 2017

I was not sleeping @artem-sidorenko :-) All good, sounds reasonable.

mikemoate pushed a commit to mikemoate/chef-ssh-hardening that referenced this issue Feb 2, 2017

Mike Moate
Send and Accept locale environment variables
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 #160).

Chefspec tests cover the default, custom/overriden and empty cases for the attributes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment