pam
pam copied to clipboard
msteinert
Allow to define confdir
PAM has a pam_start_confdir() which allows to define the configuration directory where all services are located. This is useful to define your own service on tests in particular, so that you can control your stack and be independant of the host when running them. Allow defining this configuration directory, with functional options. This keeps then the current package API while avoiding proliferation of function namse, which would be doubled due to Start() and StartFunc() flavors.
Add tests to cover this.
Ah, it seems the CI is using an old version of pam, indeed, the API was introduced on 2020-03-06. (and so, is available in ubuntu 20.10+). What’s the policy of your project in term of OS requirements?
Hi Didier, thanks for the contribution!
I like where you're going with this, I'm concerned about support for this new[^1] PAM feature. It seems unlikely that pam_start_confdir() will make it into many LTS distributions that are still supported, e.g. Ubuntu 16.04, CentOS-7, etc. Do you have any thoughts on how to manage compatibility? Maybe with build tags?
Using an optional config struct is a clever idea. I'm not opposed to adding new functions, especially if they mirror the underlying PAM APIs.
[^1]: It looks like this was added in 2020. I realize that isn't all that new in the software world, but for an old API like PAM that's bleeding edge stuff.
You’re welcome! Indeed, you are right about compatibility, this is bleeding edge for PAM and we may miss the target for many LTSes or other long-supported-term distros. :)
Let me think about it, build tags is obviously one way to do it, I wonder if we can get something with symbol lookup when building it. Let me sleep on this and see how it goes.
Ah, it seems the CI is using an old version of pam, indeed, the API was introduced on 2020-03-06. (and so, is available in ubuntu 20.10+). What’s the policy of your project in term of OS requirements?
I honestly haven't thought about this too much. I don't think the PAM API has changed much over the years. I would probably like to keep compatibility with the major LTS releases.
Another possibility would be to tag a new major version and then branch for supporting 1.x. I need to think about that one a bit.
I looked at a little bit more this afternoon and I came up with something.
Basically, we can weaken up the symbol avaibility and check if it’s there. If it’s not and we still try to call WithConfDir() option, we return an error (I think it’s better for the caller to check the avaibility if the caller is using the option and be explicit in erroring out rather than fallback to pam_start() silently.
I adapted the tests to pass on both locally and in CI. Fortunately, there is a fix in Go in 1.18 for this symbol checking and you updated the build matrix in the last couple of days. So it will only work in 1.18+ (it still work in 1.17- if you don’t use the option).
I think this way keeps the backward compability and introduce the needed checks. Tell me what you think about it.
hey, did have a chance to have a look at the PR by any chance? I think that the optional loading in the last commit is the best path forward right now.
Sorry it took me so long to get back to this. I was traveling for work and then it slipped my mind.
This is looking pretty good, but I think it might be confusing if Start() silently falls back pam_start() when a confdir is requested. I would prefer to have a separate function like libpam, which panics when the symbol isn't available. That would make it easy enough for a user to setup a fallback and it would keep this wrapper code simple. What do you think? Would you be willing to make that change then we can get it merged?
I realize you made an effort to combine the constructors, but in this case I think something simpler that closely follows the underlying API is more appropriate.
Hey no worry! This is why I’m pinging you :)
I’m happy to do the change and add an API, but just before, I want to hilight that the current proposed behavior is not the one you described. There is no silent fallback as I agree this would be surprising for the user. What happens is, if you call Start() with WithConfDir() and the symbol is not present, you get back an error (and the pam conversation is not started). This one is: option WithConfDir() was used, but the pam version on the system is not recent enough.
So, no fallback, getting an error from Start(). With this information, do you still prefer a separate API?
Hi Didier!
Yes, I think I still prefer the separate APIs. My reasoning: Keep this wrapper simple and aligned with the underlying PAM APIs. Building high-level APIs on top to suit your particular taste should be straightforward.
Thanks for your patience.
And here we go! I have thus created the additional API. Compared to previous version, we lose thus the Func flavor with conf directory. Tell me if you think it’s necessary, but this one is not really aligned with PAM APIs, which is why I didn’t add it.
I have rebased and pushed back, tell me know what you think.
The Start()/StartFunc() design was inspired by http.Handler()/http.HandlerFunc() interface from the standard library. In this case I don't think it's strictly necessary and I don't want to hold up your PR any longer. It's easy enough to add in the future if somebody wants it.
Thanks again for the contribution and your patience with my requests.