Make kwallet-pam work with pam_fscrypt
ClosedPublic

Authored by aacid on Sun, Mar 8, 6:14 PM.

Details

Summary

fscrypt encrypts folders, you can use pam_fscrypt to auto-unencrypt
some, for example your home folder.

This means that the old code of kwallet-pam would not work
since it tries to read/write the salt file at the auth step.

This change moves that from the auth step to the open step, sadly that
means we need to keep the password in (pam "secure") memory for a while more.

Test Plan

I can now login into my session and the kwallet password prompt is not shown

Diff Detail

Repository
R107 KWallet PAM Integration
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
aacid created this revision.Sun, Mar 8, 6:14 PM
Restricted Application added a project: Plasma. · View Herald TranscriptSun, Mar 8, 6:14 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
aacid requested review of this revision.Sun, Mar 8, 6:14 PM
aacid updated this revision to Diff 77232.Sun, Mar 8, 6:15 PM

don't log the password to journald ^_^

sitter added a subscriber: sitter.Fri, Mar 13, 10:51 AM
sitter added inline comments.
pam_kwallet.c
313

This can ENOMEM. Does that maybe need handling? Or will pam_set_data just fail if you give it a nullptr?

329

I wonder about this comment. Can the call sequence here be random? Can open be called before authenticate?

aacid added inline comments.Fri, Mar 13, 11:03 PM
pam_kwallet.c
313
329

That is a good question, the old code was kind of prepared for it.

I am going to say "no" open can not be called before authenticate, if you read https://pubs.opengroup.org/onlinepubs/008329799/pam_open_session.htm it says

"The pam_open_session() function opens a new session for a user previously authenticated with a call to pam_authenticate()."

But my pam knowledge is between none and i googled a little, so I would be happy if someone can google a bit more and agree/disagree with me

sitter added inline comments.Mon, Mar 16, 10:58 AM
pam_kwallet.c
329

Alex did add this check in a dedicated commit 634464255a82de55e0288f7e425e50f6c409f51d and even though I couldn't find a subsequent commit that made use of that preparation I'm sure he must have had a reason to add it. Perhaps it was this:

http://www.linux-pam.org/Linux-PAM-html/mwg-expected-of-module-overview.html#mwg-expected-of-module-overview-1

The independence of the four groups of service a module can offer means that the module should allow for the possibility that any one of these four services may legitimately be called in any order. Thus, the module writer should consider the appropriateness of performing a service without the prior success of some other part of the module.

gnome-keyring, as the most topically relevant example, doesn't explicitly have 'open called before' but its written in a very particular way. Both auth and session can unlock the keyring (and attempt a daemon start) it seems. Not really the same, but there's certainly some nuance to how modules may get called.

So, the original check for sm_open_session in our code may have been not entirely pointless, it does kinda meet that any-order expectation. It probably also wasn't entirely correct though as it didn't meet the independence expectation ^^

Removing it leaves me a bit uneasy without input from someone who knows more about pam and the two of us. At the same time it's clearly not quite right either.

How about leaving it in for 5.18 and drop it for master?
Should there be an unexpected problem we'll at least have months of theoretical testing and a shorter window from 5.19.0 to .1 to hotfix a potential regression.

aacid added inline comments.Mon, Mar 16, 10:20 PM
pam_kwallet.c
329

How about leaving it in for 5.18 and drop it for master?
Should there be an unexpected problem we'll at least have months of theoretical testing and a shorter window from 5.19.0 to .1 to hotfix a > potential regression.

Doesn't sound very convincing to me, if there's going to be a regression i'd prefer it to be this month when i still remember this code and i still use kwallet-pam and pam_fscrypt, if you delay it to 5.19.0 or something I will not totally remember this code and may not be using this any of those two either os my motivation to fix it may be pretty small.

I can try adding the code that makes pam_sm_open_session from here if you think it makes sense to have it

sitter added inline comments.Wed, Mar 18, 2:41 PM
pam_kwallet.c
329

That is a fair point but I for one cannot +1 this diff as-is. So, I would prefer the pam_sm_open_session call making a return.

Or someone else gives a +1, I don't profoundly object to the diff, but it is in my mind not suitable for a stable release right now:
Currently in 5.18 one can call session before auth and it works. With this diff that'd be no longer the case and a behavioral change in a stable release update. I have no idea if that has real life implications but I can totally imagine one of our enterprise users pulling weird stuff like that.

aacid updated this revision to Diff 77963.Wed, Mar 18, 9:44 PM

restore the code that calls pam_sm_open_session from pam_sm_authenticate if they were called "out of order"

sitter accepted this revision.Thu, Mar 19, 12:47 PM
This revision is now accepted and ready to land.Thu, Mar 19, 12:47 PM
This revision was automatically updated to reflect the committed changes.