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

feat[totp]: add user TOTP configuration check #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Nithe14
Copy link

@Nithe14 Nithe14 commented Apr 5, 2024

Description

The code now checks if the user has TOTP configured. If so, the input is split by the last index of the '/' character, instead of every occurrence of the character. The #14 issue should be resolved in this branch. If there is no TOTP configured for the user, the logic remains the same as in the current master branch.

This code introduces two additional configuration options: realmapiusername and realmapiassword, which are required for the IsTOTPConfigured function to run flawlessly. These configurations describe a Keycloak service user that should be present in the realm and has at least the realm-management->view-users role assigned. This user is used to call the Keycloak admin API and check if the SSH user has TOTP configured.

Notes:

  • There must be a Keycloak API user with realm-management->view-users role assigned if TOTP is configured.
  • The config.toml should not be readable by non-admin users of the system (for example permissions should be set to 640) to prevent compromise Keycloak API user.
  • Usernames must be unique within the Keycloak realm.
  • There is a new dependency (gocloak)

Doubts:

  • Not sure if the IsTOTPConfigured function should be in a separeted package file
  • The configuration has changed, so the documentation should be updated (I can update the README if you want)

Fixes #14

Type of change

  • Breaking change (new feature that would cause existing config to break TOTP)
  • Breaking change (new dependency - gocloak)
  • This change requires a documentation update

How Has This Been Tested?

  • Running existing tests

This was tested by configuring PAM with this module to use a Keycloak instance. A bunch of users were tested with TOPT configured as well as with no TOTP configuration. Multiple occurrences of '/' were added to users passwords to test if splitting works correctly.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
    • I can update the README if you want me to
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have rebased my branch to include the latest changes from master

@kha7iq
Copy link
Owner

kha7iq commented Apr 8, 2024

Thank you for your work @Nithe14
Give me some time to review it , i am away for holidays.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Authentication will fail if passwords include one or more "/"
2 participants