Skip to content

Conversation

@sumit-bose
Copy link
Contributor

This patch used the new error code ERR_CHECK_NEXT_AUTH_TYPE while
processing different authentication types instead of EAGAIN because
EAGAIN might have side effects when returned to the callers.

Resolves: #8108

@sumit-bose sumit-bose marked this pull request as draft November 7, 2025 18:38
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the Kerberos authentication child process to use a new, specific error code ERR_CHECK_NEXT_AUTH_TYPE instead of the generic EAGAIN when an authentication method is not applicable. This is a good improvement as it avoids ambiguity and potential side effects from misinterpreting EAGAIN. The changes are implemented consistently across the affected functions, and the new error code is correctly defined and handled. The logic for trying subsequent authentication methods is preserved and now clearer. I have reviewed the changes and found no issues.

Copy link
Contributor

@thalman thalman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks goos, thanks @sumit-bose

@alexey-tikhonov
Copy link
Member

@sumit-bose, should this go to sssd-2-9 as well?

@sumit-bose
Copy link
Contributor Author

@sumit-bose, should this go to sssd-2-9 as well?

Hi,

the issue was cause by the EAGAIN check from #7995 which was backported to sssd-2-9 and sssd-2-11, so yes.

bye,
Sumit

@alexey-tikhonov
Copy link
Member

@shridhargadekar, @sumit-bose, is this something to account for in #8032?

@sumit-bose
Copy link
Contributor Author

@shridhargadekar, @sumit-bose, is this something to account for in #8032?

Hi,

I think not because it si not related to SSS_AUTHTOK_TYPE_PAM_STACKED tested in #8032. As you can see in the issue you need enforced 2FA and krb5_store_password_if_offline = True to trigger the issue. Not sure if we can currently handle 2FA in tests?

bye,
Sumit

@alexey-tikhonov
Copy link
Member

Not sure if we can currently handle 2FA in tests?

I think @spoore1 or maybe @krishnavema or @ikerexxe should know.

Copy link
Contributor

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ikerexxe
Copy link
Contributor

I think @spoore1 or maybe @krishnavema or @ikerexxe should know.

No idea, I never played with 2FA

@alexey-tikhonov alexey-tikhonov added the coverity Trigger a coverity scan label Nov 21, 2025
This error code should be used if another authentication type should be
checked.

Resolves: SSSD#8108
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Reviewed-by: Tomáš Halman <thalman@redhat.com>
This patch used the new error code ERR_CHECK_NEXT_AUTH_TYPE while
processing different authentication types instead of EAGAIN because
EAGAIN might have side effects when returned to the callers.

Resolves: SSSD#8108
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Reviewed-by: Tomáš Halman <thalman@redhat.com>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Reviewed-by: Tomáš Halman <thalman@redhat.com>
@sssd-bot
Copy link

The pull request was accepted by @ikerexxe with the following PR CI status:


🟢 CodeFactor (success)
🟢 CodeQL (success)
NEUTRAL osh-diff-scan:fedora-rawhide-x86_64:upstream (neutral)
🟢 rpm-build:centos-stream-10-x86_64:upstream (success)
🟢 rpm-build:fedora-41-x86_64:upstream (success)
🟢 rpm-build:fedora-42-x86_64:upstream (success)
🟢 rpm-build:fedora-43-x86_64:upstream (success)
🟢 rpm-build:fedora-rawhide-x86_64:upstream (success)
🟢 Analyze (target) / cppcheck (success)
🟢 Build / freebsd (success)
🟢 Build / make-distcheck (success)
🟢 ci / intgcheck (centos-10) (success)
🟢 ci / intgcheck (fedora-41) (success)
🟢 ci / intgcheck (fedora-42) (success)
🔴 ci / intgcheck (fedora-43) (failure)
🟢 ci / intgcheck (fedora-44) (success)
🟢 ci / prepare (success)
🔴 ci / system (centos-10) (failure)
🟢 ci / system (fedora-41) (success)
🟢 ci / system (fedora-42) (success)
🔴 ci / system (fedora-43) (failure)
🔴 ci / system (fedora-44) (failure)
➖ Coverity scan / coverity (skipped)
🟢 Static code analysis / codeql (success)
🟢 Static code analysis / pre-commit (success)
🟢 Static code analysis / python-system-tests (success)


There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging.

@sssd-bot sssd-bot force-pushed the otp_offine_password branch from afa50be to 89db64f Compare November 24, 2025 09:05
@ikerexxe ikerexxe merged commit da82d1d into SSSD:master Nov 24, 2025
11 of 16 checks passed
@ikerexxe ikerexxe mentioned this pull request Nov 24, 2025
@alexey-tikhonov alexey-tikhonov removed the coverity Trigger a coverity scan label Nov 24, 2025
@alexey-tikhonov
Copy link
Member

Note: Covscan was green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

After I log in offline with a cached password hash, sssd stays offline forever because my account requires MFA

5 participants