Add OAuth2 prompting config

[eisenmann-b1]

This splits the OAuth2 prompt Authenticate at <url> and press ENTER. into an info message Authenticate at "<url>" and a prompt Press ENTER to continue.
The latter can be skipped by setting the PAM option skip_enter_prompt.

[sumit-bose]

Hi,

thank you for your patch. Can you explain a bit more why you do not like "Press Enter" message?

In general I would prefer to not handle this with PAM module options but with prompting options in sssd.conf, see
"PROMPTING CONFIGURATION SECTION" in man sssd.conf for details. Currently there is nothing for "oauth2" and adding new options here would give a much greater flexibility in configuring the prompts for OAuth2.

bye,
Sumit

[eisenmann-b1]

I think prompting the user to press ENTER, while at the same time asking them to use another device to log in to some URL is actually harmful to the user experience.
If pressed too early, the request will most likely time out before the user can log in on the other device, and if pressed too late the request might already be expired.
IMO, setting the idp_request_timeout to a higher number and disabling the ENTER prompt allows for a better experience for the user.

Regarding the prompting configuration, I agree that it makes sense to have the option there. Then I will move the option to the sssd.conf. Maybe something along the lines of:

[prompting/oauth2]
interactive: show prompt before continuing. (default: true)
interactive_prompt: the message for the interactive prompt. (default: "Press ENTER")

[eisenmann-b1]

As suggested, I removed the PAM option, and added a [prompting/oauth2] section, with options interactive (default true) and interactive_prompt (default Press ENTER to continue.).

[sumit-bose]

Hi,

thank you, the new approach looks much better now. Can you squash your initial two patches into the new one so that the changes from the first two does not have to be removed by the third? Please remove the changes to the *.pot file as well, this will be done at certain times before a new release.

Additionally, I would like to ask you for a bit of patience and wait until #8212 is merged and then rebase your patch on top of it. And @ikerexxe, the author of #8212, might also have some comments if your new options fits into the general plan for configurable prompts.

bye,
Sumit

[eisenmann-b1]

I squashed the commits and dropped the changes to the *.pot file.
Also changed the title to better reflect the changes.

Looking forward to seeing the mentioned PR merged.

[eisenmann-b1]

Rebased, since there were some conflicts from the passwordless-gdm PR that was merged.

Also, CodeFactor complained about complexity of pc_list_from_response, so I moved the string copying blocks there into a separate function.

[ondrejv2]

Does setting of interactive to false mean SSSD will periodically poll IdP to see if authenticated yet? Or will it simply wait for timeout and then try? This should be IMO clarified in the man page.

[eisenmann-b1]

Does setting of interactive to false mean SSSD will periodically poll IdP to see if authenticated yet? Or will it simply wait for timeout and then try? This should be IMO clarified in the man page.

SSSD already handles "try again" messages (like authorization_pending or slow_down).
Setting interactive does not change that.

However, if interactive is set to False, idp_request_timeout probably needs to be increased; otherwise, the retries will not have time to run, and the authentication will simply fail.

I already added a paragraph for that in sssd.conf.5.xml.

[ikerexxe]

Apart from the inline comment I'm missing tests for the newly introduced functionality. test_prompt_config.c already contains tests that validate the behaviour for prompts. Do you mind adding a new test case for oath2? In addition, try to extend test_pam_get_response_prompt_config and test_pc_list_from_response test cases

[eisenmann-b1]

Added a new test test_pc_list_add_oauth2, and adjusted test_pam_get_response_prompt_config and test_pc_list_from_response.

[ikerexxe]

LGTM! Thank you for adding the test cases

CI failures seem unrelated but I'm rerunning them just in case

[sumit-bose]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces configurable prompting for OAuth2, separating the authentication message from the interactive prompt. The changes are well-structured, including updates to configuration, documentation, and tests. A notable improvement is the refactoring of the string parsing logic in pam_sss_prompt_config.c, which enhances code clarity and maintainability. I have identified a couple of issues: a potential memory safety concern in the new helper function due to mixed-sign integer comparisons, and a potential NULL pointer dereference that could lead to a crash. I've provided specific comments and suggestions to address these points.

[sumit-bose]

Coverity is green

[eisenmann-b1]

I fixed the two problems reported by gemini.

The int/size_t issue is present in a few other places [1] as well.
Should I open a separate issue and/or PR for that?

Regarding the CodeFactor issue; Is that OK, or should the response data go into test setup & teardown?

[1]: Here are some I found:
pc_list_from_response: int-typed size argument.
eval_response: reads int32_t from buffer, calls pc_list_from_response with it.
pam_get_response_prompt_config: forces size_t into int, returned via pointer.
pam_eval_prompting_config: gets the int from pam_get_response_prompt_config and passes it on to pam_add_response.
pam_add_response: assigns int to int32_t, in a struct response_data.
struct response_data: len field is int32_t.

[sumit-bose]

Hi,

I fixed the two problems reported by gemini.

thank you.

The int/size_t issue is present in a few other places [1] as well. Should I open a separate issue and/or PR for that?

Regarding the CodeFactor issue; Is that OK, or should the response data go into test setup & teardown?

Imo, this can be ignored for the time being, at least, it does not have to be fixed in this PR.

[1]: Here are some I found:
pc_list_from_response: int-typed size argument.
eval_response: reads int32_t from buffer, calls pc_list_from_response with it.
pam_get_response_prompt_config: forces size_t into int, returned via pointer.
pam_eval_prompting_config: gets the int from pam_get_response_prompt_config and passes it on to pam_add_response.
pam_add_response: assigns int to int32_t, in a struct response_data.
struct response_data: len field is int32_t.

The int32_t types are typically used to send/receive some data and make sure the data is 4 bytes long. In general I think a switch from int to size_t is not needed in all places, but a check if the values are in a reasonable range before assigning them to variable of other types would makes sense. If you are interested, feel free to continue, but currently I do not see major issues in this area.

bye,
Sumit

[eisenmann-b1]

If you are interested, feel free to continue, but currently I do not see major issues in this area.

Alright, then I would keep this PR as is, and keep the changes focused on the prompt config.

For the typing issue I will open another PR at a later time.

[sumit-bose]

Hi,

thank you for the updates, ACK.

bye,
Sumit

[sssd-bot]

The pull request was accepted by @sumit-bose with the following PR CI status:

---

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