Passwordless-gdm

[ikerexxe]

This is the implementation for the so called passwordless-gdm feature. The design page for this feature is available at SSSD/sssd.io#79.

The original patches were reviewed at ikerexxe#9 and ikerexxe#12 by Justin and Sumit. I've had done some minor modifications to those patches:

• Adapt the code to avoid merge/rebase conflicts.
• Simplified the prompt tuning commits. Previously there were 4 commits: one introducing prompt tuning for EIdP, one adding prompt tuning for smartcard, another commit for passkey and a final one removing all prompt tuning and hardcoding the values. Currently, only the hardcoded values remain.
• Add an additional commit, the last one, which fixes CI failures related to password change on expiration for samba (and probably AD).

As a reminder you can use https://copr.fedorainfracloud.org/coprs/ipedrosa/passwordles-gdm/ for testing and update sssd, authselect, mutter, gdm and gnome-shell packages.

authselect brings a new feature called with-switchable-auth that you should enable to use this feature. In addition, you should add the following configuration to sssd.conf:

[pam]
pam_json_services = gdm-switchable-auth

Known limitations:

• If EIdP and passkey are enabled at the same time for a given user, from the two of them only EIdP will be advertised (other methods aren't affected). Changing it will affect performance, so it's been decided to leave it as it is.
• Since PAM: Add passkey preflight operation #7983 isn't merged, SSSD is unable to propagate the PIN request and PIN attempts left information to GDM. This is temporary and once this PR or the other one are merged, I'll update the PR to make these functionality available.

[alexey-tikhonov]

@ikerexxe , how does it play together with #8185?

[ikerexxe]

@ikerexxe , how does it play together with #8185?

@alexey-tikhonov we discovered that passkey with no PIN and LDAP server wasn't working at all (not even in master branch) while developing this feature. The first commit from that PR is also present in this branch, so once one of them is merged the other PR will remove it. That's all the relation there is between one and the other.

[ikerexxe]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces the passwordless-gdm feature, enabling a more modern authentication experience. The changes are substantial, involving significant refactoring of the Kerberos authentication flow to better separate pre-authentication and authentication steps. A new JSON-based protocol is implemented for communication with GDM, handled by the new pamsrv_json.c file. While the refactoring improves code structure, I've identified a couple of critical security vulnerabilities related to buffer over-reads when parsing data from other SSSD components. These must be addressed. I've also noted an issue with error handling in the new JSON parsing logic that could mask problems and make debugging difficult.

[alexey-tikhonov]

@ikerexxe , if SSSD is updated to a version that includes those patches, but GDM and authselect aren't, does old functionality works seamlessly?

[alexey-tikhonov]

Is there a patch that contains RNs?

[ikerexxe]

@ikerexxe , if SSSD is updated to a version that includes those patches, but GDM and authselect aren't, does old functionality works seamlessly?

I was testing this the other day. If any package with this functionality is missing then the old functionality works without any user interaction.

Is there a patch that contains RNs?

Responder: new option pam_json_services contains a new configuration option RN and man: clarify and fix pam_json_services compilation contains the feature RN.

As a side note, except for four checks that were in red, CI was in green until I submitted the latest changes. I checked the failing checks and they were not related to these changes.

[alexey-tikhonov]

Responder: new option pam_json_services contains a new configuration option RN and man: clarify and fix pam_json_services compilation contains the feature RN.

Try to imagine how it will read in an automatically generated notes.

I would re-phrase

Add pam_json_services option to enable JSON protocol ...

to something like

A new option `pam_json_services` is now available to configure ...

re:

:feature: unified passwordless login in the GUI. SSSD now supports a
rich authentication selection interface. Users can login with
smartcards, passkey, External IdPs and passwords directly within the
graphical user interface.

As a nitpick: it should start with a capital letter.

I guess option is described in the man page, so I would reference in the style "please see <this> man page description for additional information and examples".

Also please add :packaging: RN - this is a note for maintainers:

• what are dependencies; what versions of those dependencies will be required
• what happens if reqs aren't met
• is there any new ./configure options

etc

[ikerexxe]

Rebased on top of master once #8176 has been merged.

I would re-phrase

Add pam_json_services option to enable JSON protocol ...

to something like

A new option `pam_json_services` is now available to configure ...

Done

Also please add :packaging: RN - this is a note for maintainers:

Added a new RN for :packaging: in commit man: clarify and fix pam_json_services compilation

[justin-stephenson]

system tests in PRCI seemed to be stuck, so I am re-running CI jobs

[alexey-tikhonov]

system tests in PRCI seemed to be stuck, so I am re-running CI jobs

It won't help until CI is fixed.

[alexey-tikhonov]

I'm removing 'backport-to' label as automation will add a little value here.
Please open backport PR manually for discussion after this PR is merged.
But I personally think it is not a good fit for sssd-2-9 LTM branch.

[justin-stephenson]

I am ready to approve but first would like to see full PRCI runs for fedora-43 and fedora-44. Based on Slack discussion there is out of disk space events occurring on f43 + f44 as a result of a GDM bug. If I understand correctly SSSD/sssd-ci-containers#155 will workaround this

[alexey-tikhonov]

Added a new RN for :packaging: in commit man: clarify and fix pam_json_services compilation

Thank you. Please don't forget to replace link to SSSD/sssd.io#79 with a link to actual page once doc PR is merged.

[alexey-tikhonov]

@ikerexxe, please rebase.

[ikerexxe]

I am ready to approve but first would like to see full PRCI runs for fedora-43 and fedora-44. Based on Slack discussion there is out of disk space events occurring on f43 + f44 as a result of a GDM bug. If I understand correctly SSSD/sssd-ci-containers#155 will workaround this

Rerun and everything is green except for system/centos-10 test, which has a known issue in the test_feature__presence test case.

@ikerexxe, please rebase.

Why? If you are asking this because #8150 was merged, those tests don't run in PRCI (at least not now): @pytest.mark.builtwith(client=["gdm", "passkey", "vfido"])

[alexey-tikhonov]

@ikerexxe, please rebase.

Why? If you are asking this because #8150 was merged,

That was my idea.

those tests don't run in PRCI (at least not now): @pytest.mark.builtwith(client=["gdm", "passkey", "vfido"])

:(

[justin-stephenson]

Thank you for your large effort on this feature.

[alexey-tikhonov]

Covscan reported issues:

krb5_child.c:
707done:
708    talloc_free(token);
709    talloc_free(pin);
     
CID 639182: (#1 of 1): Uninitialized pointer read (UNINIT)
5. uninit_use_in_call: Using uninitialized value chl when calling krb5_responder_otp_challenge_free.
710    krb5_responder_otp_challenge_free(ctx, rctx, chl);
711    return ret;

pamsrv_json.c:
255    oauth2_link_prompt = talloc_strdup(tmp_ctx, OAUTH2_LINK_PROMPT);
      notnull: At condition oauth2_init_prompt == NULL, the value of oauth2_init_prompt cannot be NULL.
      dead_error_condition: The condition oauth2_init_prompt == NULL cannot be true.
256    if (oauth2_init_prompt == NULL) {
     
CID 639181: (#1 of 1): Logically dead code (DEADCODE)
dead_error_begin: Execution cannot reach this statement: ret = 12;.
257        ret = ENOMEM;
258        goto done;
259    }

[sssd-bot]

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

---

🟢 CodeFactor (success)
🟢 CodeQL (success)
🟢 osh-diff-scan:fedora-rawhide-x86_64:upstream (success)
🟢 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) (success)
🟢 ci / intgcheck (fedora-44) (success)
🟢 ci / prepare (success)
🟢 ci / system (centos-10) (success)
🟢 ci / system (fedora-41) (success)
🟢 ci / system (fedora-42) (success)
🟢 ci / system (fedora-43) (success)
🟢 ci / system (fedora-44) (success)
➖ 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.