Decouple marshalling/un-marshalling from socket I/O

[pprindeville]

It's precursory to separate marshalling functions from I/O if
event-driven I/O is to be done, or if multiple Tacacs+ sessions
can be active at the same time (you don't want to be polling on
one socket and miss a timeout on another).

[pprindeville]

Looking at magic.c, it's not clear how magic() gets provided when openssl-devel is installed.

[pprindeville]

Any comments on this one?

[daveolson53]

Philip Prindeville [email protected] wrote:

Any comments on this one?

I'd looked it over quickly after you first posted, and didn't see significant issues.

I spent some more time looking at it today.

My understanding on your reintroduction of magic() for the openssl case
is that it is wrong. There should be no calls to magic() when openssl
is used, they are all ifdef'ed out. Configuring and compiling with
openssl confirms that for me. So I think commit
e0ad5da is unneeded and incorrect.

I don't think commit b754e6d is all
that worthwhile. I'd made a similar change, but ultimately, the
change to just call the init code with pthread_once() and the
magic_initialized check seems more than enough to me.

What use case do you have for using the linker spec to do the
initialization at startup or library load? It's not wrong,
but why bother? Especially since most people will end up not
using magic() at all?

I suspect that commit 96417ab that
changes to use magic() with || instead of && is also incorrect, unless
you are going to change all the other incidences of that check. I've
not worked through things to see if the change itself is good or not.

For the main change, marshalling/unmarshalling in commit
dc23e6e, I'm unhappy about adding
yet another member of struct areply that needs to be freed by the
caller. Those kinds of things make it really easy to have memory
leaks, or incorrect data use. I'd really prefer a cleanup function
in libtac that can always be called, and hide the cleanup there.

Also, as I commented on an earlier change, this changes the libtac API
and API, which means the version needs to be bumped. So I think this
change, if it's going to be made, needs to be pulled at the same time as
the other ABI and API changes.

Comments in the Changelog should also be made, making the incompatible
change clear.

I'm not a fan of adding assert()'s, either.

You've added an unconditional call to magic(); it needs to be wrapped
in the OPENSSL check and use the inline method to call
RAND_pseudo_bytes() instead if OPENSSL is defined.

Other than that ;), I think the basic ideas are good.

I can't say I really fully reviewed all the changes (because the
changes are so extensive), and I just don't have the time to go through
it all and make sure that there isn't something else wrong. I'll
rely on your testing for that, and perhaps to see the code that
relies on these changes.

Dave Olson
[email protected]

[pprindeville]

My understanding on your reintroduction of magic() for the openssl case is that it is wrong. There should be no calls to magic() when openssl is used, they are all ifdef'ed out. Configuring and compiling with openssl confirms that for me. So I think commit e0ad5da is unneeded and incorrect.

I tried building with openssl-devel on Fedora 24 and there were unresolved references to magic(). This function is clearly called by tac_acct_send(), tac_authen_send(), and tac_author_send(). When you build against OpenSSL, I don't see how a magic() function gets provided.

I don't think commit b754e6d is all that worthwhile. I'd made a similar change, but ultimately, the change to just call the init code with pthread_once() and the magic_initialized check seems more than enough to me.

I've worked in a lot of Embedded linux environments where the gcc had solid support, so any sort of __start-based initialization worked great, but the pthread library was buggy or incomplete. Also, using pthread to handle a simple initialization problem (and possibly suck in a 120K library as a dependency) seems heavy handed, especially for embedded environments with limited memory.

Your experiences may vary.

What use case do you have for using the linker spec to do the initialization at startup or library load? It's not wrong, but why bother? Especially since most people will end up not using magic() at all?

Because it doesn't introduce any additional requirements.

I suspect that commit 96417ab that changes to use magic() with || instead of && is also incorrect, unless you are going to change all the other incidences of that check. I've not worked through things to see if the change itself is good or not.

If you grep for it, you'll mostly find:

libtac/lib/authen_s.c:#if defined(HAVE_OPENSSL_MD5_H) && defined(HAVE_LIBCRYPTO)
libtac/lib/cont_s.c:#if defined(HAVE_OPENSSL_MD5_H) && defined(HAVE_LIBCRYPTO)
libtac/lib/crypt.c:#if defined(HAVE_OPENSSL_MD5_H) && defined(HAVE_LIBCRYPTO)
libtac/lib/header.c:#if defined(HAVE_OPENSSL_RAND_H) && defined(HAVE_LIBCRYPTO)
libtac/lib/header.c:#if defined(HAVE_OPENSSL_RAND_H) && defined(HAVE_LIBCRYPTO)
...

And from my Boolean Algebra classes, I remember that ! (a && b) is the same as !a || !b, so if we wanted to invert the test #if defined(HAVE_OPENSSL_RAND_H) && defined(HAVE_LIBCRYPTO) that would yield #if !defined(HAVE_OPENSSL_RAND_H) || !defined(HAVE_LIBCRYPTO).

Or am I missing something obvious?

For the main change, marshalling/unmarshalling in commit dc23e6e, I'm unhappy about adding yet another member of struct areply that needs to be freed by the caller. Those kinds of things make it really easy to have memory leaks, or incorrect data use. I'd really prefer a cleanup function in libtac that can always be called, and hide the cleanup there. We can add destructors as a separate commit.

Any other issues with the changes or just that?

Also, as I commented on an earlier change, this changes the libtac API and API, which means the version needs to be bumped. So I think this change, if it's going to be made, needs to be pulled at the same time as the other ABI and API changes.

That's fine. But there are a lot of changes still. See the previous summary I posted somewhere...

Comments in the Changelog should also be made, making the incompatible change clear.

Ah, yes, good point. Wasn't sure if git was set up to auto-populate the Changelog of if that needed to be done manually.

I'm not a fan of adding assert()'s, either.

Okay, not sure what a better way to handle this would be: no attribute, etc. string should be more than 255 characters since there's no way to represent its length. Calling the library with strings that violate this constraint is a logic violation. How would you prefer to handle it?

You've added an unconditional call to magic(); it needs to be wrapped in the OPENSSL check and use the inline method to call RAND_pseudo_bytes() instead if OPENSSL is defined.

Ah, right. I didn't see that these changes had crept into _tac_req_header(). Can we just move the conditional logic from setting session_id in _tac_req_header() to magic() and always have:

session_id = magic();

to contain the PRNG logic in one place?

Other than that ;), I think the basic ideas are good.

Oh, good. ;-)

I can't say I really fully reviewed all the changes (because the changes are so extensive), and I just don't have the time to go through it all and make sure that there isn't something else wrong. I'll
rely on your testing for that, and perhaps to see the code that relies on these changes.

I had tested the code pre-porting. I'm on the road this week, but will test it more this weekend and incorporate your feedback.

[pprindeville]

Ah, right. I didn't see that these changes had crept into _tac_req_header(). Can we just move the conditional logic from setting session_id in _tac_req_header() to magic() and always have [...] to contain the PRNG logic in one place?

Okay, made this change.

[daveolson53]

Philip Prindeville [email protected] wrote:

My understanding on your reintroduction of magic() for the openssl case is
that it is wrong. There should be no calls to magic() when openssl is used,
they are all ifdef'ed out. Configuring and compiling with openssl confirms
that for me. So I think commit e0ad5da is unneeded and incorrect.

I tried building with openssl-devel on Fedora 24 and there were unresolved
references to magic(). This function is clearly called by tac_acct_send(),
tac_authen_send(), and tac_author_send(). When you build against OpenSSL, I
don't see how a magic() function gets provided.

As I said, it doesn't. There are no calls to magic() when using
openssl. If you wind up with calls, ifdefs are missing.

I don't think commit b754e6d is all that worthwhile. I'd made a similar
change, but ultimately, the change to just call the init code with
pthread_once() and the magic_initialized check seems more than enough to
me.

I've worked in a lot of Embedded linux environments where the gcc had solid
support, so any sort of __start-based initialization worked great, but the
pthread library was buggy or incomplete. Also, using pthread to handle a simple
initialization problem (and possibly suck in a 120K library as a dependency)
seems heavy handed, especially for embedded environments with limited memory.

Your experiences may vary.

Those days are long in the past. Yes, I've seen those also.
Nobody trying to use pthreads has pthread code that horrible
any more.

And if somebody writes their own pthread library that is that
bad, then tough luck.

What use case do you have for using the linker spec to do the
initialization at startup or library load? It's not wrong, but why bother?
Especially since most people will end up not using magic() at all?

Because it doesn't introduce any additional requirements.

Sure it does, that linker magic doesn't work anywhere (and I
think is gcc-specific).

I suspect that commit 96417ab that changes to use magic() with || instead
of && is also incorrect, unless you are going to change all the other
incidences of that check. I've not worked through things to see if the
change itself is good or not.

If you grep for it, you'll mostly find:

libtac/lib/authen_s.c:#if defined(HAVE_OPENSSL_MD5_H) && defined(HAVE_LIBCRYPTO)
libtac/lib/cont_s.c:#if defined(HAVE_OPENSSL_MD5_H) && defined(HAVE_LIBCRYPTO)
libtac/lib/crypt.c:#if defined(HAVE_OPENSSL_MD5_H) && defined(HAVE_LIBCRYPTO)
libtac/lib/header.c:#if defined(HAVE_OPENSSL_RAND_H) && defined(HAVE_LIBCRYPTO)
libtac/lib/header.c:#if defined(HAVE_OPENSSL_RAND_H) && defined(HAVE_LIBCRYPTO)
...

And from my Boolean Algebra classes, I remember that ! (a && b) is the same as
!a || !b, so if we wanted to invert the test #if defined(HAVE_OPENSSL_RAND_H) &
& defined(HAVE_LIBCRYPTO) that would yield #if !defined(HAVE_OPENSSL_RAND_H) ||
!defined(HAVE_LIBCRYPTO).

Or am I missing something obvious?

I'm just saying that having the ifdef different in one place is
a bad idea. There's no reason for it, so don't.

For the main change, marshalling/unmarshalling in commit dc23e6e, I'm
unhappy about adding yet another member of struct areply that needs to be
freed by the caller. Those kinds of things make it really easy to have
memory leaks, or incorrect data use. I'd really prefer a cleanup function
in libtac that can always be called, and hide the cleanup there. We can add
destructors as a separate commit.

Any other issues with the changes or just that?

nothing else that I specifically noticed.

Also, as I commented on an earlier change, this changes the libtac API and
API, which means the version needs to be bumped. So I think this change, if
it's going to be made, needs to be pulled at the same time as the other ABI
and API changes.

That's fine. But there are a lot of changes still. See the previous summary I
posted somewhere...

I know. I just think you need to flag them in the commit message,
and have them all applied at once.

Comments in the Changelog should also be made, making the incompatible
change clear.

Ah, yes, good point. Wasn't sure if git was set up to auto-populate the
Changelog of if that needed to be done manually.

I'm not a fan of adding assert()'s, either.

Okay, not sure what a better way to handle this would be: no attribute, etc.
string should be more than 255 characters since there's no way to represent its
length. Calling the library with strings that violate this constraint is a
logic violation. How would you prefer to handle it?

Add runtime checks, and error out. assert's that aren't compiled
in are worthless.

You've added an unconditional call to magic(); it needs to be wrapped in
the OPENSSL check and use the inline method to call RAND_pseudo_bytes()
instead if OPENSSL is defined.

Ah, right. I didn't see that these changes had crept into _tac_req_header().
Can we just move the conditional logic from setting session_id in
_tac_req_header() to magic() and always have:

session_id = magic();

to contain the PRNG logic in one place?

I'm fine with it; I think it's cleaner. I didn't review the change that
did this, and I don't know why it was done the way it was. It also
avoids an API change.

Other than that ;), I think the basic ideas are good.

Oh, good. ;-)

I can't say I really fully reviewed all the changes (because the changes
are so extensive), and I just don't have the time to go through it all and
make sure that there isn't something else wrong. I'll
rely on your testing for that, and perhaps to see the code that relies on
these changes.

I had tested the code pre-porting. I'm on the road this week, but will test it
more this weekend and incorporate your feedback.

Dave Olson
[email protected]

[pprindeville]

Those days are long in the past. Yes, I've seen those also. Nobody trying to use pthreads has pthread code that horrible any more.

My experience is that gcc support has been at least that solid, and since initialization is handled by glibc and gcc jointly, and the two are extremely tightly coupled, it's never been a problem.

Using it with uClibc and eglibc has been equally solid.

What if you're linking to a project that uses pthreads, and there are side-effects of the way that the magic initialization happens before the rest of your package is really to have any pthreads calls happen?

I know. I just think you need to flag them in the commit message, and have them all applied at once.

I don't think there are any changes in this round that would break the API. I've tried to make things remain backward compatible.

The ChangeLog gets updated atomically with a summary of the changes when a new version gets bumped, right?

[daveolson53]

Philip Prindeville [email protected] wrote:

Those days are long in the past. Yes, I've seen those also. Nobody trying
to use pthreads has pthread code that horrible any more.

My experience is that gcc support has been at least that solid, and since
initialization is handled by glibc and gcc jointly, and the two are extremely
tightly coupled, it's never been a problem.

Using it with uClibc and eglibc has been equally solid.

What if you're linking to a project that uses pthreads, and there are
side-effects of the way that the magic initialization happens before the rest
of your package is really to have any pthreads calls happen?

That seems to me extremely unlikely. I'll leave it up to the
maintainers. I don't think your change is likely to cause new
problems, I just think it's pointless.

I know. I just think you need to flag them in the commit message, and have
them all applied at once.

I don't think there are any changes in this round that would break the API.
I've tried to make things remain backward compatible.

The API changes, because now the caller has to free the new field
if set. The ABI change is more important in this case, since the API
change "just" causes a memory leak.

The ChangeLog gets updated atomically with a summary of the changes when a new
version gets bumped, right?

I don't think so, but more importantly, even if it does, the content for
things like ABI and API breakage isn't automatically derived.

Dave Olson
[email protected]

[pprindeville]

Fix 1-line regression in tac_cont_send() where th->type wasn't being set.

[pprindeville]

I'll rely on your testing for that, and perhaps to see the code that relies on these changes.

Hi @daveolson53,

Was doing some of that testing last night. That's where I noticed that there were issues with the MD5 encryption on longer messages (with more than 16 bytes of payload) and that a GETPASS response wasn't being handled correctly by tacc.

So I fixed both of those.

Now I'm testing something else but I can't tell if the problem is a configuration issue (on the server) or the code is generating an ill-formed packet with the wrong values, or I'm just not invoking it on the command line with the correct args.

Got a few minutes to discuss this offline (via email or Skype-IM) so we don't clutter up the PR?

I'm drafting an email to the tac_plus mailing list (for the Tacacs+ server that I'm using) to see if there's anything obviously wrong in the configuration. I might put you in blind-copy.

Thanks

[daveolson53]

Philip Prindeville [email protected] wrote:

I'll rely on your testing for that, and perhaps to see the code that relies
on these changes.

Hi @daveolson53,

Was doing some of that testing last night. That's where I noticed that there
were issues with the MD5 encryption on longer messages (with more than 16 bytes
of payload) and that a GETPASS response wasn't being handled correctly by tacc.

So I fixed both of those.

Now I'm testing something else but I can't tell if the problem is a
configuration issue (on the server) or the code is generating an ill-formed
packet with the wrong values, or I'm just not invoking it on the command line
with the correct args.

Got a few minutes to discuss this offline (via email or Skype-IM) so we don't
clutter up the PR?

Sure. I rarely use skype, so I can't say for sure it will work this
time; my account is dave.olson1506 ; I'll need to move to a privacy
room to talk, so don't be surprised if I'm quiet when I answer.

My direct email is [email protected]

I'm drafting an email to the tac_plus mailing list (for the Tacacs+ server that
I'm using) to see if there's anything obviously wrong in the configuration. I
might put you in blind-copy.

Dave Olson
[email protected]

[pprindeville]

If anyone is interested, the post that I used on the initializer stuff for MSC/VC/gcc came from
attribute((constructor)) equivalent in VC? on stackoverflow.

[pprindeville]

By the way, if anyone is wondering how I'm testing, I'm using tacc.c and testing against the tac_plus driver. I've taken Heasley's (shrubbery.net) version F4.0.4.29a and layered various fixes and RHEL/Centos/Fedora packaging fixes.

Have a look at my fork of Heasley's tac_plus.

[pprindeville]

Rebased to master today.

[pprindeville]

Merge PR #80 before this one.