python-pam icon indicating copy to clipboard operation
python-pam copied to clipboard

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

Open mar10 opened this issue 2 years ago • 2 comments

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: https://github.com/mar10/wsgidav/issues/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.

mar10 avatar Sep 14 '22 19:09 mar10

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.

ElpyDE avatar Apr 08 '23 18:04 ElpyDE

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

mar10 avatar Apr 10 '23 16:04 mar10