fix: fix cannot login more than 1 user #64
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The original design is wrong: pam_open_session() should only be called inside child process. Move it into childModifier (after fork()), and execve manually to insert environ into the child process. The internal API is also changed to adapt the new code structure. The Pam.cpp is merged into Auth.cpp, and some change happened in Display.cpp
|
Hi @calsys456. Thanks for your PR. I'm waiting for a linuxdeepin member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Reviewer's GuideRefactors PAM handling so that pam_open_session is executed only in the child process, inlines the former Pam wrapper into Auth, and adjusts session/login lifecycle management across Auth, Display, and UserSession to correctly handle multiple concurrent logins and PAM environments. Sequence diagram for the new multi-user login and session flowsequenceDiagram
actor User
participant Display
participant Auth
participant UserSession
participant PAM
participant Logind
participant VirtualTerminal
User->>Display: requestLogin(user, password)
Display->>Display: findOrCreateAuth(user)
Display->>Auth: authenticate(secret)
Auth->>PAM: pam_start(service ddm, user, conv)
Auth->>PAM: pam_authenticate(handle, 0)
PAM-->>Auth: PAM_SUCCESS
Auth->>PAM: pam_acct_mgmt(handle, 0)
PAM-->>Auth: PAM_SUCCESS
Auth->>Auth: authenticated = true
Auth-->>Display: true
Display->>Auth: openSession(command, env, cookie)
Auth->>UserSession: setProcessEnvironment(env)
Auth->>UserSession: start(command, type, cookie)
UserSession-->>Auth: processStarted
Auth->>Auth: sessionOpened = true
par Parent_process
Auth->>VirtualTerminal: path(tty)
Auth->>Auth: sessionProcessId = m_session.processId()
Auth->>PAM: pam_handle still valid
Auth->>Logind: GetSessionByPID(sessionProcessId)
Logind-->>Auth: sessionObjectPath
Auth->>Logind: read property Id
Logind-->>Auth: xdgSessionId
Auth->>Auth: xdgSessionId stored
Auth-->>Display: xdgSessionId
Display->>Display: auths.append(auth)
and Child_process_fork
UserSession->>Auth: openSessionInternal(processEnvironment)
Auth->>PAM: pam_setcred(handle, PAM_ESTABLISH_CRED)
Auth->>PAM: pam_set_item(PAM_TTY, VirtualTerminal.path(tty))
Auth->>PAM: pam_set_item(PAM_XDISPLAY, DISPLAY)
Auth->>PAM: pam_putenv(handle, env vars)
Auth->>PAM: pam_open_session(handle, 0)
PAM-->>Auth: PAM_SUCCESS
Auth-->>UserSession: char** pamEnv
UserSession->>VirtualTerminal: jumpToVt(tty, false)
UserSession->>UserSession: setupNamespacesAndUser()
UserSession->>UserSession: setupStdioAndLogs()
UserSession->>UserSession: execve(program, args, pamEnv)
end
UserSession-->>Auth: userProcessFinished(status)
Auth-->>Display: userProcessFinished(status)
Display->>Display: auths.remove(auth)
Display->>Auth: delete
Auth->>PAM: pam_close_session(handle, 0)
Auth->>PAM: pam_setcred(handle, PAM_DELETE_CRED)
Auth->>PAM: pam_end(handle, ret)
PAM-->>Auth: cleanup complete
Class diagram for updated authentication and session managementclassDiagram
class Auth {
+bool authenticated
+bool sessionOpened
+QString user
+QString display
+int type
+int tty
+int xdgSessionId
+qint64 sessionProcessId
+int sessionId
+Auth(QObject* parent, QString user)
+~Auth()
+bool authenticate(QByteArray secret)
+int openSession(QString command, QProcessEnvironment env, QByteArray cookie)
+bool closeSession()
+char** openSessionInternal(QProcessEnvironment sessionEnv)
+void userProcessFinished(int status)
-UserSession* m_session
-AuthPrivate* d
}
class AuthPrivate {
+AuthPrivate(Auth* parent)
+~AuthPrivate()
+pam_handle_t* handle
+const char** secretPtr
+pam_conv conv
+int ret
}
class UserSession {
+UserSession(Auth* parent)
+void start(QString command, Display_DisplayServerType type, QByteArray cookie)
+void stop()
-void childModifier()
}
class Display {
+Display(QObject* parent)
+~Display()
+void stop()
+void login(QLocalSocket* socket, QString user, QString password)
+void logout(QLocalSocket* socket, int id)
+void handleSocket(QLocalSocket* socket)
-QList~Auth*~ auths
-SocketServer* m_socketServer
}
class VirtualTerminal {
+static int setUpNewVt()
+static QString path(int vt)
+static void jumpToVt(int vt, bool nowait)
+static void setVtSignalHandler(void* handler, void* data)
}
class OrgFreedesktopLogin1ManagerInterface {
+QDBusObjectPath GetSessionByPID(uint pid)
+void TerminateSession(QString id)
}
class OrgFreedesktopLogin1SessionInterface {
+QVariant property(QString name)
}
Display "1" o-- "*" Auth
Auth "1" o-- "1" UserSession
Auth "1" *-- "1" AuthPrivate
UserSession --> Auth
Auth ..> OrgFreedesktopLogin1ManagerInterface
Auth ..> OrgFreedesktopLogin1SessionInterface
Auth ..> VirtualTerminal
UserSession ..> VirtualTerminal
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 7 issues, and left some high level feedback:
- In Display::logout you modify
auths(removeAll/delete) while iterating it withfor (Auth *auth : std::as_const(auths)), which is undefined for Qt containers; consider iterating by index or using an iterator-based loop where you adjust the iterator after removal. - In Display::stop you delete
Authobjects inside the loop but leave their dangling pointers inauths; it would be safer to clear or rebuild the list after deletion (e.g. callauths.clear()once done, or remove each pointer as you delete it) to avoid accidental reuse.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In Display::logout you modify `auths` (removeAll/delete) while iterating it with `for (Auth *auth : std::as_const(auths))`, which is undefined for Qt containers; consider iterating by index or using an iterator-based loop where you adjust the iterator after removal.
- In Display::stop you delete `Auth` objects inside the loop but leave their dangling pointers in `auths`; it would be safer to clear or rebuild the list after deletion (e.g. call `auths.clear()` once done, or remove each pointer as you delete it) to avoid accidental reuse.
## Individual Comments
### Comment 1
<location> `src/daemon/Auth.cpp:125-134` </location>
<code_context>
+ static int converse(int num_msg,
</code_context>
<issue_to_address>
**issue (bug_risk):** The PAM conversation handler uses an unsafe cast that breaks const-correctness and type safety for the secret pointer.
In `converse`, `secret_ptr` is passed as `static_cast<void *>(secretPtr)` with `secretPtr` declared as `const char **`, but later cast back to `char **` and dereferenced:
```c++
resp[i]->resp = strdup(*static_cast<char **>(secret_ptr));
```
This breaks const-correctness and is undefined behavior because you treat a `const char *` as `char *`. Please align the types instead of casting through `char **` (e.g., make the stored pointer non-const in a controlled way, or wrap the secret in a struct and pass a pointer to that struct) so the PAM callback can access it without UB.
</issue_to_address>
### Comment 2
<location> `src/daemon/Auth.cpp:205-214` </location>
<code_context>
}
Auth::~Auth() {
- stop();
+ if (m_session->state() != QProcess::NotRunning)
+ m_session->stop();
+ if (sessionOpened)
+ closeSession();
+ // write logout to utmp/wtmp
+ if (sessionProcessId > 0)
+ utmpLogout(std::to_string(tty).c_str(), display, sessionProcessId);
+ if (d->handle) {
+ d->ret = pam_end(d->handle, d->ret);
+ if (d->ret != PAM_SUCCESS)
+ qWarning() << "[Auth] end:" << pam_strerror(d->handle, d->ret);
</code_context>
<issue_to_address>
**issue (bug_risk):** Using the PAM handle after pam_end() is called can be unsafe when logging errors.
In the destructor, `pam_end(d->handle, d->ret)` may free or invalidate `d->handle`, so using it in `pam_strerror(d->handle, d->ret)` is not guaranteed safe. Capture `d->ret` before calling `pam_end`, then use `pam_strerror(nullptr, ret)` (supported by Linux-PAM) or log the numeric code instead. Also set `d->handle = nullptr` after `pam_end` to prevent accidental reuse.
</issue_to_address>
### Comment 3
<location> `src/daemon/Auth.cpp:252-259` </location>
<code_context>
- void Auth::startUserProcess(const QString &command, const QByteArray &cookie) {
- Q_ASSERT(!m_env.isEmpty());
- QProcessEnvironment env = m_env;
+ int Auth::openSession(const QString &command,
+ QProcessEnvironment env,
+ const QByteArray &cookie) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Ignoring the result of waitForStarted() can lead to utmp/logind state being updated for a session that never started.
Here you call:
```c++
m_session->start(command, type, cookie);
m_session->waitForStarted();
sessionOpened = true;
sessionProcessId = m_session->processId();
utmpLogin(..., sessionProcessId, true);
```
If `waitForStarted()` fails, `processId()` may be invalid (0 or -1), yet `sessionOpened` is set, utmp/wtmp is updated, and later code will try to close a session that never started. Please check the return value of `waitForStarted()` and/or `m_session->state() == QProcess::Running` and abort early (e.g., return a non‑positive `xdgSessionId`) when startup fails.
</issue_to_address>
### Comment 4
<location> `src/daemon/UserSession.cpp:132-141` </location>
<code_context>
- QString sessionType = processEnvironment().value(QStringLiteral("XDG_SESSION_TYPE"));
- const bool waylandUserSession = sessionType == QLatin1String("wayland");
+ // Open session
+ Auth *auth = qobject_cast<Auth *>(parent());
+ char **env = auth->openSessionInternal(processEnvironment());
+ if (!env) {
</code_context>
<issue_to_address>
**🚨 issue (security):** execve() failures are not handled, which can leave the child running unexpected code with modified privileges.
In `childModifier` the direct `execve` call:
```c++
execve(qPrintable(program), c_args.data(), env);
```
means that on failure the function returns and the child continues in `childModifier`/QProcess internals, potentially after user, namespaces, FDs, or PAM session state have already been changed. This can leave a semi-privileged helper executing an unintended path.
Please handle failure explicitly and terminate the child, for example:
```c++
execve(...);
perror("execve");
_exit(1);
```
so that no further code runs in a misconfigured child process.
</issue_to_address>
### Comment 5
<location> `src/daemon/Auth.cpp:174` </location>
<code_context>
// Auth implementation //
/////////////////////////
+ class AuthPrivate : public QObject {
+ Q_OBJECT
+ public:
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the new PAM integration by making AuthPrivate a plain struct, passing it directly to the PAM conversation, and replacing error-handling macros with small helper functions to make control flow clearer.
You can keep all the new PAM behavior while reducing complexity and indirection quite a bit.
### 1. Drop `QObject` + heap `secretPtr` in `AuthPrivate`
`AuthPrivate` doesn’t use signals/slots and only holds PAM-related state. You can make it a plain struct, store the secret directly, and pass the whole struct as `appdata_ptr` to the conversation function.
```cpp
// Before
class AuthPrivate : public QObject {
Q_OBJECT
public:
AuthPrivate(Auth *parent)
: QObject(parent)
, secretPtr(new const char *)
, conv({ converse, static_cast<void *>(secretPtr) }) {
*secretPtr = nullptr;
}
~AuthPrivate() {
delete secretPtr;
}
pam_handle_t *handle{ nullptr };
const char **secretPtr{};
pam_conv conv{};
int ret{};
};
```
```cpp
// After (no QObject, no heap pointer)
struct AuthPrivate {
pam_handle_t *handle = nullptr;
pam_conv conv{};
QByteArray secret; // owns password bytes
int ret = 0;
AuthPrivate() {
conv.conv = &converse;
conv.appdata_ptr = this;
}
};
```
Then in `Auth`:
```cpp
Auth::Auth(QObject *parent, QString user)
: QObject(parent)
, user(std::move(user))
, m_session(new UserSession(this))
, d(new AuthPrivate{})
{
// unchanged signal connection
}
```
### 2. Simplify `converse()` to use `AuthPrivate` directly
Instead of `void *secret_ptr` → `char **` → pointer into `QByteArray`, just use the `AuthPrivate` and its `QByteArray secret`.
```cpp
// Before
static int converse(int num_msg,
const struct pam_message **msg,
struct pam_response **resp,
void *secret_ptr) {
...
case PAM_PROMPT_ECHO_OFF:
resp[i]->resp = strdup(*static_cast<char **>(secret_ptr));
...
}
```
```cpp
// After
static int converse(int num_msg,
const struct pam_message **msg,
struct pam_response **resp,
void *appdata_ptr)
{
auto *d = static_cast<AuthPrivate *>(appdata_ptr);
*resp = static_cast<pam_response *>(
calloc(num_msg, sizeof(struct pam_response)));
if (!*resp)
return PAM_BUF_ERR;
for (int i = 0; i < num_msg; ++i) {
switch (msg[i]->msg_style) {
case PAM_PROMPT_ECHO_OFF:
// use owned QByteArray
resp[i]->resp = ::strdup(d->secret.constData());
resp[i]->resp_retcode = 0;
break;
...
}
}
return PAM_SUCCESS;
}
```
And in `authenticate()`:
```cpp
bool Auth::authenticate(const QByteArray &secret)
{
Q_ASSERT(!user.isEmpty());
qDebug() << "[Auth] Starting...";
d->ret = pam_start("ddm", user.toLocal8Bit().constData(), &d->conv, &d->handle);
if (!checkAuthRet("start"))
return false;
qDebug() << "[Auth] Authenticating user" << user;
d->secret = secret; // store on d
d->ret = pam_authenticate(d->handle, 0);
d->secret.clear(); // clear after use
if (!checkAuthRet("authenticate"))
return false;
d->ret = pam_acct_mgmt(d->handle, 0);
if (!checkAuthRet("acct_mgmt"))
return false;
authenticated = true;
return true;
}
```
### 3. Replace macros with small helpers
The `CHECK_RET_*` macros hide control flow and depend on mutable `d->ret`. You can preserve the same behavior with small helpers; this makes it easier to see what each call does and avoids macro pitfalls.
```cpp
// Before
#define CHECK_RET_AUTH \
if (d->ret != PAM_SUCCESS) { \
qWarning() << "[Auth] Authenticate:" << pam_strerror(d->handle, d->ret); \
utmpLogin(std::to_string(tty).c_str(), display, user, 0, false); \
return false; \
}
```
```cpp
// After
bool Auth::checkAuthRet(const char *context)
{
if (d->ret == PAM_SUCCESS)
return true;
qWarning() << "[Auth]" << context << ":" << pam_strerror(d->handle, d->ret);
utmpLogin(std::to_string(tty).c_str(), display, user, 0, false);
return false;
}
```
Use similarly focused helpers for `closeSession` and `openSessionInternal`:
```cpp
bool Auth::checkCloseRet(const char *context)
{
if (d->ret == PAM_SUCCESS)
return true;
qWarning() << "[Auth] closeSession(" << context << "):"
<< pam_strerror(d->handle, d->ret);
return false;
}
bool Auth::checkOpenRet(const char *context)
{
if (d->ret == PAM_SUCCESS)
return true;
qWarning() << "[Auth] openSessionInternal(" << context << "):"
<< pam_strerror(d->handle, d->ret);
return false;
}
```
Usage example in `closeSession()`:
```cpp
bool Auth::closeSession()
{
if (!sessionOpened) {
qWarning() << "[Auth] closeSession: Session didn't opened.";
return true;
}
qDebug() << "[Auth] Closing session for user" << user;
d->ret = pam_close_session(d->handle, 0);
if (!checkCloseRet("close_session"))
return false;
sessionOpened = false;
d->ret = pam_setcred(d->handle, PAM_DELETE_CRED);
if (!checkCloseRet("setcred_delete"))
return false;
qDebug() << "[Auth] Session closed.";
return true;
}
```
### 4. Keep `d->ret` but limit its scope per operation
If you don’t want to fully remove `d->ret` yet, the helpers above at least make each logical operation’s use explicit and keep error handling localized:
- `checkAuthRet` used only in `authenticate`.
- `checkOpenRet` used only in `openSessionInternal`.
- `checkCloseRet` used only in `closeSession`.
This keeps current behavior intact while reducing indirection (`QObject`, heap pointer, `void *` gymnastics) and hiding of control flow (macros), making the PAM integration easier to follow and maintain.
</issue_to_address>
### Comment 6
<location> `src/daemon/Auth.h:23` </location>
<code_context>
- /** Indicates whether the Auth is active (PAM module started) */
- bool active{ false };
+ /** Indicates whether authenticated (authenticate() is called and succeed) */
+ bool authenticated{ false };
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider hiding internal state flags and PAM helper methods behind private members and accessors to keep the Auth public API small and cohesive.
You can keep the new behavior while reducing API complexity by tightening encapsulation and pushing the “state machine” and PAM details behind the class boundary.
### 1. Hide raw state flags from the public API
Instead of exposing `authenticated`, `sessionOpened`, `xdgSessionId`, and `sessionProcessId` as mutable public fields, keep them in `AuthPrivate` (or private members) and expose read-only accessors. This keeps the protocol internal while still allowing callers to query state.
**Header:**
```cpp
class Auth : public QObject {
Q_OBJECT
public:
Auth(QObject *parent, QString user);
~Auth();
bool isAuthenticated() const;
bool isSessionOpened() const;
int xdgSessionId() const; // Positive ⇒ valid
qint64 sessionProcessId() const; // Positive ⇒ valid
// ... rest unchanged ...
private:
UserSession *m_session{ nullptr };
AuthPrivate *d{ nullptr };
};
```
**Impl (`Auth.cpp`):**
```cpp
// in AuthPrivate:
struct AuthPrivate {
bool authenticated = false;
bool sessionOpened = false;
int xdgSessionId = 0;
qint64 sessionProcessId = -1;
};
// in Auth methods:
bool Auth::isAuthenticated() const { return d->authenticated; }
bool Auth::isSessionOpened() const { return d->sessionOpened; }
int Auth::xdgSessionId() const { return d->xdgSessionId; }
qint64 Auth::sessionProcessId() const{ return d->sessionProcessId; }
```
Then update `Auth` internals and `Display.cpp` (or other users) to use the accessors instead of touching the fields directly. This keeps the state machine (authenticate → openSession → closeSession) fully under `Auth`’s control and lets you change the protocol later without breaking callers.
### 2. Keep `openSessionInternal` out of the public surface
`openSessionInternal` is clearly a low-level PAM helper. You can maintain current behavior while not advertising it as part of the public API.
If only `UserSession` needs it:
**Header:**
```cpp
class Auth : public QObject {
Q_OBJECT
public:
// ... existing public API (authenticate, openSession, closeSession) ...
Q_SIGNALS:
void userProcessFinished(int status);
private:
friend class UserSession; // if you really must let UserSession call it
// Low-level PAM helper, not part of public API
char **openSessionInternal(const QProcessEnvironment &sessionEnv);
UserSession *m_session{ nullptr };
AuthPrivate *d{ nullptr };
};
```
This reduces surface area: general callers only see `authenticate`, `openSession(...)`, and `closeSession()`. Only `UserSession` (which already “knows” about PAM/process details) can call the low-level helper. If you later want a cleaner split, you can move `openSessionInternal` into `AuthPrivate` without further API breakage.
If you prefer to avoid `friend`, you can instead add a small, higher-level method tailored for `UserSession` while keeping the PAM specifics inside `Auth`:
```cpp
// in Auth.h
public:
// Higher-level interface for child setup
bool setupPamEnvironmentForChild(char ***envp,
const QProcessEnvironment &sessionEnv);
// in Auth.cpp
bool Auth::setupPamEnvironmentForChild(char ***envp,
const QProcessEnvironment &sessionEnv)
{
char **list = openSessionInternal(sessionEnv);
if (!list)
return false;
*envp = list;
return true;
}
```
Then `openSessionInternal` can be `private`, and `UserSession` calls `setupPamEnvironmentForChild(...)` instead of dealing with PAM directly.
These two changes keep all new capabilities (combined openSession, session PID, PAM env handling) but present a much smaller, cohesive public API.
</issue_to_address>
### Comment 7
<location> `src/daemon/UserSession.cpp:130` </location>
<code_context>
}
void UserSession::childModifier() {
- // Session type
- QString sessionType = processEnvironment().value(QStringLiteral("XDG_SESSION_TYPE"));
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the new PAM session handling, VT setup, and execve logic from childModifier() into small helper functions so the method becomes a clear orchestration of steps.
The added responsibilities in `childModifier()` (PAM session + env handling + manual `execve`) do increase its complexity. You can keep all behavior but make the function easier to read and maintain by extracting a few focused helpers.
For example:
1. **Isolate PAM session + env handling**
```c++
char **UserSession::openPamSessionEnv()
{
auto *auth = qobject_cast<Auth *>(parent());
Q_ASSERT(auth);
char **env = auth->openSessionInternal(processEnvironment());
if (!env) {
qCritical() << "Failed to open PAM session in user session process";
_exit(1);
}
return env;
}
```
Then at the top of `childModifier()`:
```c++
void UserSession::childModifier()
{
Auth *auth = qobject_cast<Auth *>(parent());
Q_ASSERT(auth);
char **env = openPamSessionEnv();
// ...
}
```
2. **Extract VT preparation**
```c++
void UserSession::prepareSessionVt(Auth *auth)
{
if (auth->type == Display::X11) {
return;
}
const QString ttyString = VirtualTerminal::path(auth->tty);
const int vtFd = ::open(qPrintable(ttyString), O_RDWR | O_NOCTTY);
bool takeControl = false;
if (vtFd > 0) {
dup2(vtFd, STDIN_FILENO);
::close(vtFd);
takeControl = true;
} else {
qWarning().nospace() << "Unable to open " << ttyString
<< " for wayland session (" << strerror(errno)
<< ") – falling back to current stdin";
}
if (takeControl) {
if (ioctl(STDIN_FILENO, TIOCSCTTY, 0) == -1) {
qCritical().nospace() << "Failed to take control of " << ttyString
<< " (" << QFileInfo(ttyString).owner()
<< "): " << strerror(errno);
_exit(1);
}
if (ioctl(STDIN_FILENO, KDSKBMODE, K_OFF) == -1) {
qCritical().nospace() << "Failed to set keyboard mode to K_OFF";
_exit(1);
}
}
VirtualTerminal::jumpToVt(auth->tty, false);
}
```
Then in `childModifier()`:
```c++
void UserSession::childModifier()
{
Auth *auth = qobject_cast<Auth *>(parent());
Q_ASSERT(auth);
char **env = openPamSessionEnv();
prepareSessionVt(auth);
// existing namespace + user switching + logging code...
}
```
3. **Separate exec + argv/env building**
```c++
static void execWithPamEnv(const QString &program,
const QStringList &args,
char **env)
{
QList<char *> c_args;
c_args.push_back(strdup(qPrintable(program)));
for (const QString &arg : args) {
c_args.push_back(strdup(qPrintable(arg)));
}
c_args.push_back(nullptr);
execve(qPrintable(program), c_args.data(), env);
// If execve returns, it's an error
qCritical() << "execve failed:" << strerror(errno);
_exit(1);
}
```
And at the end of `childModifier()`:
```c++
// after stdio redirection
execWithPamEnv(program(), arguments(), env);
```
These small helpers keep the behavior you introduced (PAM session env, VT leak decision based on `auth->type`, manual `execve`), but reduce `childModifier()` to a readable orchestration of clearly named steps, without reverting any of your changes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
calsys456
force-pushed
the
master
branch
2 times, most recently
from
4f11c2e to
f93cebc
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug where multiple users could not log in concurrently by restructuring PAM session management. The key change is moving pam_open_session() calls from the parent process into the child process (after fork()), which is the correct PAM usage pattern for multi-user scenarios.
Changes:
- Merged Pam.cpp/Pam.h into Auth.cpp/Auth.h, making Auth directly handle PAM lifecycle
- Modified UserSession to call
openSessionInternal()inchildModifier()(post-fork) and manuallyexecve()with PAM environment - Updated Display to delete Auth instances on session end/logout and use Auth for both logind session management and process launching
- Enhanced systemd service capabilities (added CAP_AUDIT_* and CAP_SYS_RESOURCE) and removed some security hardening options
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| src/daemon/Auth.h | Updated interface: removed stop(), added openSessionInternal() for child process, changed openSession() to launch process and return session ID |
| src/daemon/Auth.cpp | Merged PAM conversation and lifecycle from Pam.cpp, restructured to support fork-then-open-session pattern |
| src/daemon/UserSession.h | Removed unnecessary forward declarations and cached PID field |
| src/daemon/UserSession.cpp | Added PAM session opening in childModifier, manual execve with PAM environment |
| src/daemon/Display.h | Removed userProcessFinished slot |
| src/daemon/Display.cpp | Simplified Auth lifecycle: delete on logout/session-end, integrated session opening into login flow |
| src/daemon/Pam.h | Deleted (functionality moved to Auth) |
| src/daemon/Pam.cpp | Deleted (functionality moved to Auth) |
| src/daemon/CMakeLists.txt | Removed Pam.cpp from build |
| services/ddm.service.in | Added audit capabilities, removed ProtectKernelTunables and RestrictSUIDSGID hardening |
| REUSE.toml | Removed Pam.cpp/Pam.h from license annotations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zccrs
requested changes
Jan 14, 2026
calsys456
force-pushed
the
master
branch
4 times, most recently
from
a6c2e3f to
1b8049c
Compare
zccrs
approved these changes
Jan 14, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: calsys456, zccrs The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The original design is wrong: pam_open_session() should only be called inside child process. Move it into childModifier (after fork()), and execve manually to insert environ into the child process.
The internal API is also changed to adapt the new code structure. The Pam.cpp is merged into Auth.cpp, and some change happened in Display.cpp
Summary by Sourcery
Rework authentication and session handling to open PAM sessions only in the child process, integrate PAM directly into Auth, and adjust display/session management accordingly.
Bug Fixes:
Enhancements:
Build:
Documentation: