Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auth OIDC: Possibility of using Client Certificate Path at IdP and authentication #2378

Closed
joaop221 opened this issue Sep 28, 2023 · 9 comments · Fixed by #2436 or #2437
Closed

Auth OIDC: Possibility of using Client Certificate Path at IdP and authentication #2378

joaop221 opened this issue Sep 28, 2023 · 9 comments · Fixed by #2436 or #2437
Assignees
Labels
Issue type - enhancement request New feature being requested outside of original scope. Plugin - auth_oidc Status - queued / not yet started The request is clear, but the work has yet to be scheduled.
Milestone

Comments

@joaop221
Copy link
Contributor

joaop221 commented Sep 28, 2023

Greetings,

With actual config of Idp auth requires that user inputs raw certificate contents (including private key). Moodle grants security of this approach, but some organizations require that some of such contents are placed at key vault solution or file path (inclusive key vault can do this). Searching the contributions available here, I've found this implementation #2235 and the discussion about overcomplication that this implementation implies.

So I've been thinking about the possibility of specifying a default location of certificates inside IdP and authentication feature, giving admin option to describe both filenames (public and private key) that will be placed at this "default location". Of course, cert passphrase will be appreciated.

If these ideas are viable for this project, I'm at your disposal. (Some of the work is already done - PR will come soon).

@joaop221
Copy link
Contributor Author

joaop221 commented Oct 5, 2023

@weilai-irl is this feature viable?

I'd like to know if there is any process or implementation to make it possible.

@yabbondanza
Copy link

It would be really great if we have a feature like this.

@gabiaabreu
Copy link

That would be a nice and useful feature to get implemented. Any updates on this?

@frankgalindo
Copy link

Up!

I would appreciate this feature!

@weilai-irl weilai-irl self-assigned this Oct 24, 2023
@weilai-irl weilai-irl added Issue type - enhancement request New feature being requested outside of original scope. Plugin - auth_oidc Status - queued / not yet started The request is clear, but the work has yet to be scheduled. labels Oct 24, 2023
@weilai-irl weilai-irl added this to the 2023-12 milestone Oct 24, 2023
@weilai-irl
Copy link
Collaborator

Hi @joaop221

Sorry for not getting back to you sooner.

Yes, I think this feature is viable. I'll review your implementation and aim to include it in the next release.

Regards,
Lai

@joaop221
Copy link
Contributor Author

joaop221 commented Nov 3, 2023

Thank you @weilai-irl for the response.

I'm available for any help needed.

@joaop221
Copy link
Contributor Author

joaop221 commented Nov 30, 2023

Hi @weilai-irl,

During tests and reviews of this implementation, I've found some problems with client objects (local_o365) that interact with authentication methods (auth_oidc) when Certificate authentication method is used. Follow the list:

  • Cannot reload token for user profile photo update; This problem is not related to this issue (see Inconsistent photo sync issue #1871)
  • Warning during cron executions. E.g. Warning: reading undefined stdClass::clientsecret

This happens because of static method that does not verify authentication method and create an object using client secret. See below:

return new static($cfg->clientid, $cfg->clientsecret, $cfg->authendpoint, $cfg->tokenendpoint, $apptokenendpoint);

Proposed solution: be96d22

@weilai-irl
Copy link
Collaborator

Hi @joaop221,

I have reviewed your PRs and they look very good to me. They work as expected on certificates configured in file names, and for those encrypted using passphrase.

I made some small changes and created separate branches and PRs in order to comply with our release process. The changes I made are:

  • Apply passphrase logic to "Plain text" certificate source.
  • Initialise "Certificate source" configuration during upgrade.
  • Set default value for the "Certificate source" configuration in case it's missing.
  • Change the base location where certificate files are stored.
  • Update some language strings.

I'll include this item in the pre-release test in our process, so that they can be included in the next release.

FYI, support for Moodle 4.0 version of the plugin has been dropped, so this feature will not be supported in 4.0 version.

Thank you again for your contribution. I'll review other issue mentions in your latest comment separately.

Regards,
Lai

@weilai-irl
Copy link
Collaborator

Hi all,

This feature has been included in the release today for:

@joaop221 Thank you again for your contribution.

Regards,
Lai

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