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

Two suggestions for readme (PAM not thread-safe and sample) #37

Open
mar10 opened this issue Sep 14, 2022 · 2 comments
Open

Two suggestions for readme (PAM not thread-safe and sample) #37

mar10 opened this issue Sep 14, 2022 · 2 comments

Comments

@mar10
Copy link

mar10 commented Sep 14, 2022

It seems that python_pam is not thread-safe, or I am using it in a wrong way.
At least adding an RLock solved one issue for me:
mar10/wsgidav#265

Maybe you want to add a remark in the readme (or give me a hint how to use it correctly)?

Currently I create an instance p = pam.pam() on startup, then re-use it in request handlers (p.authenticate(...)).
This led to errors, that I was able to fix by wrapping it like

with lock:
    p.authenticate(...)

Should a new pam.pam() instance be created in every request-hander instead and would this impact performance?

Also, the readme says

>>> import pam
>>> p = pam.authenticate()
>>> p.authenticate('david', 'correctpassword')
True
...

which gives an exception. I assume it should be

>>> import pam
>>> p = pam
>>> p.authenticate('david', 'correctpassword')
True
...

instead.

@ElpyDE
Copy link

ElpyDE commented Apr 8, 2023

The example in the README.md should actually be (found that in some older version, probably a copy&paste mistake):

>>> import pam
>>> p = pam.pam()
>>> p.authenticate('david', 'correctpassword')
True
...

And then it becomes rather obvious that you don't want to reuse the same PamAuthenticator object (returned by pam.pam()) for different and concurrent requests - the p.code and p.reason will for sure be overwritten. I think creating a new PamAuthenticator for each request and discarding it with all it's internal states is the way to go, and I would be surprised when locking a single instance (or making a pool and take one that is free with all the organization to make that thread safe) is seriously less overhead.

@mar10
Copy link
Author

mar10 commented Apr 10, 2023

Thanks, I was just curious, since the PamAuthenticator's constructor seems to call ctype's cdll.loadLibrary() and find_library() every time.

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

No branches or pull requests

2 participants