aacid added inline comments. INLINE COMMENTS
> sitter wrote in pam_kwallet.c:329 > Alex did add this check in a dedicated commit > 634464255a82de55e0288f7e425e50f6c409f51d > <https://phabricator.kde.org/R107: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. > 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 REPOSITORY R107 KWallet PAM Integration REVISION DETAIL https://phabricator.kde.org/D27935 To: aacid Cc: sitter, security-team, davidedmundson, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
