Hi Dave,
----- Original Message -----
> The following have been pushed to the brolley/dev branch of the pcpfans
> repository. These changes implement PMCD host access control for unix
> domain sockets. Details in the commit logs.
Nice work, thanks for taking on that unix: access control suggestion -
the code there looks good to me.
Just reviewed the rest of the changes, and I have some small concerns,
some discoveries, and some more suggestions. ;) Nothing so big anymore
though, I hope!
1. QA
Some modified QA tests will now fail for when run against non-latest
sources, as they unconditionally expect "local:" in the output. Ken's
expressed a need to run new QA with older sources in the past, we need
to make this work (anyone doing QA testing gets instant priority over
anyone else in my book ;) ... except maybe people working on security
issues, and then only grudgingly)
I don't think this is tricky though; I'd recommend keeping the output
as local: in the cases where we've now got "localhost" -> "local:" in
the output, and in the tests translate forward (sed: s/.../local:/g)
2. Hostnames
So, this is more of a concern to me - we've talked about this before,
and I remain worried. :) We now have a number of tools that would've
printed the hostname (via the now removed gethostname calls), instead
will print "local:". Some IRC discussion happened about this one and
I'm pretty sure its going to bite people if we don't tackle it.
So, looking about in libpcp, I discovered pmGetContextHostName today!
I don't remember ever seeing this before but Happy Days!, it means we
don't have to add to the PMAPI to solve our problems! (We do need to
make many more tools use it though). We should add a "pmcd.hostname"
metric pmFetch() in there (like pmlogger does now) and start to use
the result. For hosts without that metric, and which *are* local (ie
where "sts = ctxp->c_pmcd->pc_hosts[0].name" would give a path or the
"localhost" string), we should fallback to the gethostname route.
This way we solve my both concerns about hostnames and Franks cunning
plan to get more accurate hostname reporting in the clients (ie. over
ssh tunnelled connections and so on). Win, win!
In other discoveries resulting from libpcp spelunking today, I think
__pmSetClientId is broken wrt IPV6, right? Some of the comments do
not look too promising, and its not using new network APIs.
3. Possible buglet?
Noticed this pattern... (auxconnect.c, secureconnect.c)
else if (addr->sockaddr.raw.sa_family == AF_UNIX) {
/* Simply truncate the path in the address to the length of the mask. */
i = strlen(mask->sockaddr.local.sun_path);
addr->sockaddr.local.sun_path[i] = '\0';
... are we guaranteed that the mask string is longer than the string
we're truncating? Else, that looks like a buffer overrun.
4. Cleanup
I think the new impl.h routine you've added for pmcd to do some
extra AF_UNIX setup might not be needed - can we do that work in
__pmServerAddNewClients? I think so...? It already has a little
bit of AF_UNIX-specific skullduggery, and the NewClient callback
happens almost immediately after the point you've changed within
pmcd (and, importantly, before the access checks - they're actually
done in the code that pmcd plugs into that NewClient callback).
cheers.
--
Nathan
|