> On 2007-01-08, at 1509, Joshua Megerman wrote:
>> OK, I think I can say that I've figured out what I think the onauth
>> API
>> should be, so I'd like to get some comments on it.  I haven't done any
>> implementation yet (other than copying John's onchange function and
>> doing
>> the search & replace).  Assuming I get positive feedback, I'll move
>> forward with the implementation.
>> The onauth script will be given 4 parameters. the first three of which
>> will come from the parameters to the call_onauth function.
>>   $1 is the username. [1]
>>   $2 is the domain name or NULL if it's a system user. [1]
> i think the username and domain name should be kept together, since
> the checkpassword interface has them together. again, i can see the
> possibility for programs other than just vpopmail calling the
> "onauth" script, and i'd like to keep the interface as generic as
> possible.
OK, I can see that.  More importantly, that's the standard for other
vpopmail command-line arguments ([EMAIL PROTECTED], not user<space>domain), so
even though it's just more work, it's not unreasonable.

>>   $3 is the numeric value associated with the service type. [2]
> again, stick with words... with a synthesized "(type=nnn)" string to
> represent unknown service types.
I'm torn with this - mostly because I'm not sure that this information is
really that useful (at least right now).  Also, I think you and I are
looking at 2 different pieces of information:

I'm looking at the connection type - this is only used by vchkpw to test
against the disable flags to prevent users from authenticating via methods
that they aren't allowed to use.  In that case, there's only 4
possibilities: POP, SMTP, IMAP and WEBMAIL, which are #define integers
from 0-3.  Also, technically there is no "other", as any unknown
connection is defaulted to POP.

You're talking about the actual service, which vchkpw will set to one of
{smtp|pop3|imap|smtps|submission|imaps|pop3s|webmail|<port #>}, depending
on what port (and possibly IP) it comes in on.

Again, the question is what info should we really be passing on to the
onauth script?  And the answer is, I'm really not sure anymore :)  Unless
we're trying to move the auth logging to the onauth script (which, BTW,
would be MUCH more complicated because there's so much that needs to be
embedded in other tools as well, as opposed to the auth-before-relay
stuff), I'm not sure it's really all that useful...

What does everyone else think?

>>   $4 is the current timestamp as generated by time(NULL). [3]
> formatted as "%lu". or maybe as something else- but if you're writing
> an API, you need to explicitly state these kinds of details. "%lu"
> gives you ten digits, just one big decimal number. if you'd rather do
> something like strftime("%Y-%m-%d.%X") i wouldn't complain... but
> then the only thing my "onauth" script might do is log something, and
> that will happen through multilog- which will be adding timestamps on
> its own, so for me this is probably useless.
Yes, you're right.  This is my first real API documentation and I'm still
getting the hang of things...  Timestamp will be sent as "%lu", so that it
can be easily handled by any type of script (shell, perl, C, etc.), and
because it's unambiguous as to what it means (once clearly documented :)).

>> [1] The username and domain name are passed separately because they
>> are
>> already broken apart in order for vchkpw/vpopmaild to authenticate the
>> user, and would have to be concatenated together before calling
>> onauth,
>> and then split back apart by the onauth script.  That increases the
>> workload on both ends, so why bother.
> because other programs which may make use of the "onauth API" might
> not hold the user's identification as two separate items. i can see
> patching djb's original "checkpassword" program to call the "onauth"
> script for systems which might have both system accounts and vpopmail
> accounts.
See my above comments.  I'm not convinced that compatibility with programs
outside of vpopmail is a good argument, but compatibility with other
vpopmail programs is, so I'll make that change :)
>> [2] Except when being used to determine whether a particular
>> service is
>> authorized in vchkpw, this information is never used.  I think it
>> might be
>> good to pass a long for logging purposes, but see no reason not to
>> stick
>> to numeric codes, and let the called script deal with the
>> information as
>> it sees fit.  I'll add defines so that vpomaild and  other have
>> numeric
>> code designations as well, and they remain static unless someone
>> decides
>> to renumber them.
> just go with words, and only deal with numbers if there is no label
> for the value.
See the discussion above...

>> Other API notes:
>> The call to call_onauth should probably return the exit status of the
>> onauth script, which can be gotten after the wait(&pid) call
>> returns.  And
>> instead of returning 0 for a failed call, return -1.  I'm not sure
>> if we
>> care about that, but it's information that might be useful later on.
> personally i would go with "1" for good and "0" for bad, but i can
> see the logic here as well- just return the exit status code from wait
> () or waitpid(). as long as it's clearly documented, i can go either
> way.
Well, we run into a minor issue, but I think I can resolve it fairly
simply.    We need to return "internal" error codes if we have problems
running the script, but we need to return "external" error codes if the
script returns non-zero.  I propose that we return codes >= 256 for
"internal" errors (in the case of ENOENT or EACCES, either bit-shift them
or add 256 (probably simpler to add 256), and return "external" error
codes as-is.  Either way, 0 is a success.  What do you think?  I'd like to
keep our APIs consistant in this manner.

>> Other code notes:
>> There's a couple of minor issues I see in the onchange code, which
>> I'll
>> fix in my onauth code and you can port back to the onchange code.
>> Mostly
>> the handling of a failed exec call - you should call _exit rather than
>> return, since it's in the vfork and you don't want the vfork'd process
>> doing anything more...
> ack, why didn't i see that before... i keep telling myself not to
> write code at 2.30am. (errr... what time is it now? again?)

> good catch. thanks.
You're welcome.

> also, the parameter to wait() is not the address of "pid"... it's the
> address of the integer where the exit status will be stored. it
> doesn't matter right now, but after your discussion about returning
> the exit code, that makes sense- so i've added that.
I thought about that and figured why not reuse the existing variable, but
there's no reason not to use another one either (4 bytes of memory is
pretty cheap these days :)).

> i also fixed the error messages (so that they actually tell what the
> problem is- "unable to fork" is not the same as "unable to exec") and
> fixed the braces and indentation so that they are the way i
> originally wrote the code. (yes, i'm picky about braces and
> indentation... clean code is much easier to debug.) (and please leave
> them this way... let one little piece of the vpopmail code be shiny
> and clean and easy to read... please?)
I'd adjusted the message in my code, but I was leaving your code alone for
the moment.  I do my curly braces a little different from you, but I
figured your function could use your style, and my function would use my
style (at least until one of the other maintainers decided to reformat one
or both of our sections :)).

One last code note:  Looking at your allow_onchange variable, why not do
the if (!allow_onchange) outside call_onchange, before you call the
function (since the variable is global in scope, and you have to set it
external to the function).  That way you save yourself a function call and
the associated stack push...  Either way, I don't think that part is
needed for the onauth code anyway since it isn't going to be called from
functions that can call each other - either you have the code or your
don't, and you call it when you authenticate...

I'll wait to post the updated API until this gets a little more
commentary, but I'm already working on code to put the handler in place.

Joshua Megerman
SJGames MIB #5273 - OGRE AI Testing Division
You can't win; You can't break even; You can't even quit the game.
  - Layman's translation of the Laws of Thermodynamics

Reply via email to