all lists on lists.proxmox.com
 help / color / mirror / Atom feed
From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Hannes Laimer <h.laimer@proxmox.com>, pve-devel@lists.proxmox.com
Subject: Re: [PATCH pve-access-control 1/1] pam: fork for PAM authentication to isolate SIGCHLD handler
Date: Thu, 12 Mar 2026 10:44:31 +0100	[thread overview]
Message-ID: <1773308530.j96g4rur71.astroid@yuna.none> (raw)
In-Reply-To: <20260304134649.82272-3-h.laimer@proxmox.com>

On March 4, 2026 2:46 pm, Hannes Laimer wrote:
> PAM modules can temporarily override $SIG{CHLD}, causing SIGCHLDs from
> RESTEnvironment worker processes to be lost. Run the PAM interaction in
> a subprocess via PVE::Tools::run_fork to contain any signal handler
> manipulation to the child.
> 
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>

Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Tested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

this patch alone fixes the reproducer for me, tracing the signals also
clearly shows that the PAM unix_chkpwd children are now decoupled
properly from our worker handling..

> ---
>  src/PVE/Auth/PAM.pm | 74 +++++++++++++++++++++++++--------------------
>  1 file changed, 42 insertions(+), 32 deletions(-)
> 
> diff --git a/src/PVE/Auth/PAM.pm b/src/PVE/Auth/PAM.pm
> index 3aacfc0..8586da5 100755
> --- a/src/PVE/Auth/PAM.pm
> +++ b/src/PVE/Auth/PAM.pm
> @@ -27,45 +27,55 @@ sub authenticate_user {
>      # user (www-data) need to be able to read /etc/passwd /etc/shadow
>      die "no password\n" if !$password;
>  
> -    my $pamh = Authen::PAM->new(
> -        'proxmox-ve-auth',
> -        $username,
> -        sub {
> -            my @res;
> -            while (@_) {
> -                my $msg_type = shift;
> -                my $msg = shift;
> -                push @res, (0, $password);
> -            }
> -            push @res, 0;
> -            return @res;
> -        },
> -    );
> -
> -    if (!ref($pamh)) {
> -        my $err = $pamh->pam_strerror($pamh);
> -        die "error during PAM init: $err";
> +    # PAM modules may temporarily override $SIG{CHLD}, causing SIGCHLDs from
> +    # RESTEnvironment workers to be lost. Running the PAM interaction in a fork
> +    # isolates any such handler manipulation from the parent process.
> +    my $client_ip;
> +    if (my $rpcenv = PVE::RPCEnvironment::get()) {
> +        $client_ip = $rpcenv->get_client_ip();
>      }
>  
> -    if (my $rpcenv = PVE::RPCEnvironment::get()) {
> -        if (my $ip = $rpcenv->get_client_ip()) {
> -            $pamh->pam_set_item(PAM_RHOST(), $ip);
> +    PVE::Tools::run_fork(sub {
> +        my $pamh = Authen::PAM->new(
> +            'proxmox-ve-auth',
> +            $username,
> +            sub {
> +                my @res;
> +                while (@_) {
> +                    my $msg_type = shift;
> +                    my $msg = shift;
> +                    push @res, (0, $password);
> +                }
> +                push @res, 0;
> +                return @res;
> +            },
> +        );
> +
> +        if (!ref($pamh)) {
> +            my $err = $pamh->pam_strerror($pamh);
> +            die "error during PAM init: $err";
>          }
> -    }
>  
> -    my $res;
> +        if ($client_ip) {
> +            $pamh->pam_set_item(PAM_RHOST(), $client_ip);
> +        }
>  
> -    if (($res = $pamh->pam_authenticate(0)) != PAM_SUCCESS) {
> -        my $err = $pamh->pam_strerror($res);
> -        die "$err\n";
> -    }
> +        my $res;
>  
> -    if (($res = $pamh->pam_acct_mgmt(0)) != PAM_SUCCESS) {
> -        my $err = $pamh->pam_strerror($res);
> -        die "$err\n";
> -    }
> +        if (($res = $pamh->pam_authenticate(0)) != PAM_SUCCESS) {
> +            my $err = $pamh->pam_strerror($res);
> +            die "$err\n";
> +        }
> +
> +        if (($res = $pamh->pam_acct_mgmt(0)) != PAM_SUCCESS) {
> +            my $err = $pamh->pam_strerror($res);
> +            die "$err\n";
> +        }
> +
> +        $pamh = 0; # call destructor
>  
> -    $pamh = 0; # call destructor
> +        return 1;
> +    });
>  
>      return 1;
>  }
> -- 
> 2.47.3
> 
> 
> 
> 
> 
> 




  reply	other threads:[~2026-03-12  9:45 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-03-04 13:46 [PATCH access-control/common 0/2] address probblem with SIGCHLD handler being temporarily overwritten Hannes Laimer
2026-03-04 13:46 ` [PATCH pve-common 1/1] RESTEnvironment: periodically reap workers as SIGCHLD fallback Hannes Laimer
2026-03-12  9:48   ` Fabian Grünbichler
2026-03-04 13:46 ` [PATCH pve-access-control 1/1] pam: fork for PAM authentication to isolate SIGCHLD handler Hannes Laimer
2026-03-12  9:44   ` Fabian Grünbichler [this message]
2026-03-06 17:16 ` [PATCH access-control/common 0/2] address probblem with SIGCHLD handler being temporarily overwritten Stefan Hanreich
2026-03-11 14:56 ` Michael Köppl

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1773308530.j96g4rur71.astroid@yuna.none \
    --to=f.gruenbichler@proxmox.com \
    --cc=h.laimer@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal