Hi Martins,
----- Original Message -----
> On 4/15/16 3:33 PM, Martins Innus wrote:
> >
> > Thanks. I found a few missed corner cases in the third patch. I'll
> > clean those up and send something new next week.
> >
>
> OK, I think I cleaned some of this up and would appreciate another set
> of eyes. Only new commits pasted below.
>
I've looked more closely - I think the concept is great - some specific notes
and ideas follow...
> Same notes from the last round except cleaned up the test for local
> connections.
>
> New additions:
>
> 1. Support for pmproxy to negotiate client certificates. Again done
> with an environment variable. I think it would probably be ideal if
> pmproxy could pass through from pmcd that it needs a client certificate,
> but since pmproxy negotiates first with the client I don't think this is
> possible, correct?
It's possible, I think - the pmcd connection is a multi-stage process (esp.
via pmproxy) and we can definitely communicate that pmcd requires a client
cert to the client.
I think what's needed here is to add a feature flag to the protocol, so that
pmcd advertises it (as with PDU_FLAG_CREDS_REQD) in the first PDU it sends.
This flag is set early in pmcd/client connection... e.g. around here:
diff --git a/src/pmcd/src/pmcd.c b/src/pmcd/src/pmcd.c
index 7d292e4..f129f6e 100644
--- a/src/pmcd/src/pmcd.c
+++ b/src/pmcd/src/pmcd.c
@@ -660,6 +660,8 @@ CheckNewClient(__pmFdSet * fdset, int rfd, int family)
cp->pduInfo.features |= PDU_FLAG_COMPRESS;
if (__pmServerHasFeature(PM_SERVER_FEATURE_AUTH)) /*optional*/
cp->pduInfo.features |= PDU_FLAG_AUTH;
+ if (__pmServerHasFeature(PM_SERVER_FEATURE_CERT_REQD)) /*required*/
+ cp->pduInfo.features |= PDU_FLAG_CERT_REQD;
if (__pmServerHasFeature(PM_SERVER_FEATURE_CREDS_REQD)) /*required*/
cp->pduInfo.features |= PDU_FLAG_CREDS_REQD;
if (__pmServerHasFeature(PM_SERVER_FEATURE_CONTAINERS))
> 2. Add PCP_ALLOW_BAD_CERT_DOMAIN on the client side to again provide a
> non-interactive way to deal with this existing prompt.
>
This & all the ideas here look fine to me conceptually. There is a convention
used in libpcp & pmcd/pmproxy (__pmServerFeature) to aid server code sharing,
that looks appropriate to follow here. It'll help with exposing the feature
via a pmcd.features.* metric too (handy when in connection debugging mode).
In fact, as in the above pseudo-patch, auditing use of PDU_FLAG_CREDS_REQD and
PM_SERVER_FEATURE_CREDS_REQD macros throughout libpcp/pmcd should find alot of
similar places to those needed for a (client-side) CERT_REQD?
A quick audit of those suggests - there should probably be a command line option
to pmcd for this, pmproxy might not need to know anything about it?, and there's
that pmcd.feature metric we should add. That will help with QA - the tests can
(if they need to) query support using pmcd.feature.cert_required (or whatever we
end up choosing for a name) - see the tests using qa/common.secure as examples.
In terms of docs, the PCPIntro(1) man page would warrant an update with the new
environment variables (esp. those influencing client behaviour), and a tutorial-
style discussion in lab.secure.html would be excellent.
BTW, looks like there's an unused variable that crept into pmproxy in the series
of commits ("hostName").
> Finally, I noticed that pmproxy by default uses /etc/pki/nssdb for both
> client and server connections. Since this directory is usually owned by
> root and pmproxy usually runs as the pcp user, it can't store supplied
> certificates there. So i went looking for an existing directory I could
> pass in with the "-C" option to pmproxy. The best I could find was
> /var/lib/pcp/tmp. Any thoughts on a better place? Maybe create a
> pmproxy directory under /var/lib/pcp/config?
There is precedent for /var/lib/pcp/config directories to be pcp:pcp owned, so
I'd suggest that's the better option (tmp sounds a bit too temporary for this,
to me anyway).
Does it need to be accessible/writable by other daemons like pmcd too? (pmwebd
someday, perhaps?) - if so, maybe a generic spot like /var/lib/pcp/config/nssdb
would suit? Not sure.
cheers.
--
Nathan
|