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

password change functionality #50

Merged
merged 19 commits into from
Aug 26, 2024
Merged

password change functionality #50

merged 19 commits into from
Aug 26, 2024

Conversation

zarganum
Copy link
Contributor

@zarganum zarganum commented Aug 23, 2024

The three new calls change_password(), set_password() and set_password_using_ccache() allow changing the passwords for the credential owner and/or for the other principals (if enough privileges are obtained).

The primary use case is to handle the user password expiration and/or requirement to change on the first logon.
This is achieved by:

  1. Performing get_init_creds_password() with the principal.
  2. Catching the Krb5Error exception with error code of KRB5KDC_ERR_KEY_EXP.
  3. Obtaining the Service Ticket for kadmin/changepw using get_init_creds_password() and the expired principal.
  4. Using the obtained ST creds to call e.g. change_password().
  5. Analyzing the integer and string results returned for any issues like password policy violations.

All three new functions return 3-value tuples: (result_code, result_code_string, server_response)
The non-zero result code means error.
The server response may contain additional information about password policy violations or other errors.

Prompter functionality does not work when password requires changing (with MIT k5test it always complains the new password is too short, even if policy minimal length is set to 1). Anyhow, prompter is a library wrapper for the same functionality that is available directly with this PR.

Known issues:

  • It was not possible to complete all required steps for k5test with Heimdal password services. Apart from kadmind(8), Heimdal uses a separate kpasswdd(8) daemon. Some attempts to deal with Heimdal are recorded in the commits but removed from the final code. Each new attempt introduced a new error and that was just endless...

Open for comments.

@zarganum
Copy link
Contributor Author

re: MIT KV5M_DATA: I spotted MIT code marks all krb5_data structures with this magic constant. I am not sure if MIT checks or fails when the constant is missing (it appears that it does not). When the krb5_data struct is an automatic (stack frame) var, it should be marked as well. So I modified pykrb5_set_krb5_data() in 5232ecc.

Heimdal does not use magic markers, this is MIT specific.

@zarganum
Copy link
Contributor Author

zarganum commented Aug 23, 2024

re: Heimdal testing: current k5test does not support full password changing flow.

Several attempts were made to run kadmind with kpasswdd but it still fails. From my understanding, after starting the kadmind, the principal kadmin/changepw key should be extracted from DB, put into separate keytab and then used by kpasswdd. Or am I missing the whole point here?

UPD: when start_kadmind=True, Heimdal prompts user for something during start (e.g. pytest execution pauses until I press ENTER twice). It may impact all of the above but I did not investigate.

also: there is a typo in realm.py

@zarganum
Copy link
Contributor Author

re: prompts: I was under impression that the expired password can be changed using callback prompts of get_init_creds_password() even without this PR.

But it does not work.
I tracked the submission of prompt values from Python to library and execution of get_init_creds_password() at least to the certain point where it ensures the two values are non-empty and identical. What happens next is always a "password too short" response, even if the minimal policy length is set to 1. Yes, MIT k5test complains that the password should be at least 1 character length... Well, this just makes zero sense.

But after collecting prompts, get_init_creds_password() anyway calls krb5_change_password() which I expose directly with this PR and it works at least with MIT. There is no need for prompts and callbacks, especially useful for web backends.

Copy link
Owner

@jborean93 jborean93 left a comment

Choose a reason for hiding this comment

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

Thanks for the in depth implementation here. I've done a single passthrough and added some initial comments.

setup.py Outdated Show resolved Hide resolved
src/krb5/_change_password.pyi Outdated Show resolved Hide resolved
src/krb5/_change_password.pyx Outdated Show resolved Hide resolved
src/krb5/_change_password.pyx Outdated Show resolved Hide resolved
src/krb5/_change_password.pyx Outdated Show resolved Hide resolved
src/krb5/_change_password.pyx Outdated Show resolved Hide resolved
@zarganum
Copy link
Contributor Author

zarganum commented Aug 26, 2024

CI failed the new tests on the original 0.6.0 code until I changed the version metadata in f823923. It looks like pip downloaded and used the original 0.6.0 version otherwise.

src/krb5/_set_password.pyi Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
@jborean93
Copy link
Owner

Thanks for working on this.

@jborean93 jborean93 merged commit 4cbfe7d into jborean93:main Aug 26, 2024
31 checks passed
@zarganum zarganum deleted the password branch August 26, 2024 19:52
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.

2 participants