pam_env should be marked as 'optional'

[trevor-vaughan]

We don't want the session stack of our PAM configuration to fail if
pam_env fails for some reason.

Fixes #201

[coveralls]

Coverage remained the same at 38.743% when pulling df889cc on trevor-vaughan:pam_env-fix into 16f9a71 on Scribery:master.

[spbnick]

Could you please describe a situation where this would be useful, and having "required" would break things? I haven't thought this through or researched, was just copying Debian setup, where reading /etc/default/locale is required, and would appreciate your POV.

[trevor-vaughan]

@spbnick Though it should be extremely rare, required means that an error in the underlying library could cause you to never login while optional means that if the underlying library fails, continue on and just log an error/warning.

So, in reality, should this ever cause a problem with this particular call...probably not, but it could so why not be safe?

[spbnick]

Thank you, @trevor-vaughan. Well, in the case of tlog-rec, at the moment it might result in tlog-rec producing a warning about seeing ASCII, and assuming it's UTF-8, or refusing to work if for some reason something else is set. In the future it might result in the I/O being converted to UTF-8 from the wrong encoding and being (partially) unsearchable.

[trevor-vaughan]

@spbnick I guess the question is, should either of those situations cause a login to completely fail? If the answer is, yes, tlog probably needs its own PAM module.

[spbnick]

I think the right fix is for Fedora/RHEL to provide the locale environment via environment variables (as Debian does), instead of relying on the program being started at login to be a shell, knowing about /etc/locale.conf, and being able to interpret it.

I think I understand your concern with keeping logins working at all times, but I would prefer the suggested workaround to break as soon as possible, producing a relevant error message ("cannot read /etc/locale.conf", or such), instead of leading to tlog-rec-session not working and producing an error message which would need to be researched, or worse, working incorrectly.

[trevor-vaughan]

@spbnick Ok, in that case, you need to update your instructions such that the root user can jump around this condition since you'll break that user's logins as well if this fails for some reason.

[spbnick]

I understand your apprehension, but Debian doesn't have a problem with requiring pam_env for root, so I wouldn't either, at least for the time being. I would really like to have this issue raised with Fedora folks.

[trevor-vaughan]

Sure, link me over to the issue and I'll +1 it.