tlog icon indicating copy to clipboard operation
tlog copied to clipboard

pam_env should be marked as 'optional'

Open trevor-vaughan opened this issue 5 years ago • 9 comments

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

Fixes #201

trevor-vaughan avatar Jul 17 '18 15:07 trevor-vaughan

Coverage Status

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

coveralls avatar Jul 17 '18 16:07 coveralls

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.

spbnick avatar Aug 22 '18 12:08 spbnick

@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?

trevor-vaughan avatar Aug 22 '18 15:08 trevor-vaughan

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.

spbnick avatar Aug 22 '18 15:08 spbnick

@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.

trevor-vaughan avatar Aug 22 '18 15:08 trevor-vaughan

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.

spbnick avatar Aug 22 '18 15:08 spbnick

@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.

trevor-vaughan avatar Aug 22 '18 15:08 trevor-vaughan

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.

spbnick avatar Aug 22 '18 16:08 spbnick

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

trevor-vaughan avatar Aug 22 '18 16:08 trevor-vaughan