Hi Martins,
----- Original Message -----
> [...]
> > Yep & will definitely help with getting a pmcd.feature.* metric in place
> > too, as that PMDA code uses those libpcp pmServerFeature interfaces too.
>
> OK, This is done.
>
*nod* - this branch is looking good. I agree with everything you've done;
specific comments / answers-to-questions inline below...
diff --git a/build/rpm/pcp.spec.in b/build/rpm/pcp.spec.in
index 41e0faa..b37127c 100644
--- a/build/rpm/pcp.spec.in
+++ b/build/rpm/pcp.spec.in
@@ -2425,6 +2425,7 @@ fi
%post
PCP_PMNS_DIR=@pcp_var_dir@/pmns
PCP_LOG_DIR=@pcp_log_dir@
+PCP_CONFIG_DIR=@pcp_var_dir@/config
I'd do a "PCP_NSSDB_DIR=@pcp_var_dir@/config/nssdb" there, but not
sure if this is needed in the end...
# restore saved configs, if any
test -s "$PCP_LOG_DIR/configs.sh" && source "$PCP_LOG_DIR/configs.sh"
rm -f $PCP_LOG_DIR/configs.sh
@@ -2457,6 +2458,8 @@ chown -R pcp:pcp "$PCP_LOG_DIR/pmcd" 2>/dev/null
chown -R pcp:pcp "$PCP_LOG_DIR/pmlogger" 2>/dev/null
chown -R pcp:pcp "$PCP_LOG_DIR/pmie" 2>/dev/null
chown -R pcp:pcp "$PCP_LOG_DIR/pmproxy" 2>/dev/null
+mkdir -p -m 755 "$PCP_CONFIG_DIR/nssdb"
This mkdir should be replaced by a $PCP_NSSDB_DIR in /etc/pcp.conf,
and install it in the top-level GNUmakefile (see pmchart config dir
there for an example).
diff --git a/man/man1/pcpintro.1 b/man/man1/pcpintro.1
index 16efae5..cfad7fe 100644
--- a/man/man1/pcpintro.1
+++ b/man/man1/pcpintro.1
@@ -856,6 +856,19 @@ As the environment usually is not checked again, the only
safe
strategy is to ensure all PCP-related environment variables are
set before the first call into any of the PCP libraries.
.TP
+.B PCP_ALLOW_BAD_CERT_DOMAIN
+When set, allow clients to accept certificates with mismatched
+domain names with no prompt when they are sent by pmcd or other
("pmcd" should be bold - see line 712 as an example)
+server components.
+See
+.B PCP_SECURE_SOCKETS.
+.TP
+.B PCP_ALLOW_SERVER_SELF_CERT
+When set, allow clients to accept self-signed certificates with
+no prompt when they are sent by pmcd or other server components.
Ditto.
@@ -933,6 +946,13 @@ method by default. Use
to override the default, most usually setting the value to the empty
string (for the older database methods).
.TP
+.B PCP_SECURE_DB_PATH
+When set, this variable specifies an alternate certficate database
+path for client tools. Similar to the action of the -C option for
... of the
.B \-C
option for ...
+.BR pmcd (3)
+and
+.BR pmproxy (3).
+.TP
These are in man section 1 (not 3).
diff --git a/src/include/pcp/impl.h b/src/include/pcp/impl.h
index 34efb2e..37f39f5 100644
--- a/src/include/pcp/impl.h
+++ b/src/include/pcp/impl.h
@@ -592,7 +592,7 @@ PCP_CALL extern void __pmConnectGetPorts(pmHostSpec *);
/*
* SSL/TLS/IPv6 support via NSS/NSPR.
*/
-PCP_CALL extern int __pmSecureServerSetup(const char *, const char *);
+PCP_CALL extern int __pmSecureServerSetup(const char *, const char *, const
char *);
Ah, thats an ABI breaking change, we'll need to tackle that in
a different way (keeping the original name, and adding a new API
with the extra parameter). Internally in libpcp, the old API is
able to just call the new API.
diff --git a/src/include/pcp/pmapi.h b/src/include/pcp/pmapi.h
index 82bbb6c..11be473 100644
--- a/src/include/pcp/pmapi.h
+++ b/src/include/pcp/pmapi.h
@@ -194,6 +194,7 @@ typedef struct pmDesc {
#define PM_ERR_LOGCHANGESEM (-PM_ERR_BASE-60) /* The semantics of a
metric has changed in an archive */
#define PM_ERR_LOGCHANGEINDOM (-PM_ERR_BASE-61) /* The instance domain of a
metric has changed in an archive */
#define PM_ERR_LOGCHANGEUNITS (-PM_ERR_BASE-62) /* The units of a metric
have changed in an archive */
+#define PM_ERR_NEEDCLIENTCERT (-PM_ERR_BASE-63) /* PMCD requires a client
certificate */
(when you get to QA, this will need an update to qa/006.out)
diff --git a/src/libpcp/src/auxserver.c b/src/libpcp/src/auxserver.c
index b7f75b1..01fccc9 100644
--- a/src/libpcp/src/auxserver.c
+++ b/src/libpcp/src/auxserver.c
@@ -865,10 +865,11 @@ __pmServerRequestPortString(int fd, char *buffer, size_t
sz)
#if !defined(HAVE_SECURE_SOCKETS)
int
-__pmSecureServerSetup(const char *db, const char *passwd)
+__pmSecureServerSetup(const char *db, const char *passwd, const char
*cert_nickname)
{
So, these might become..
int __pmSecureServerCertificateSetup(const char *db, const char *passwd, const
char *cert_nickname)
{
[...]
}
int __pmSecureServerSetup(const char *db, const char *passwd)
{
return __pmSecureServerCertificateSetup(db, passwd,
SECURE_SERVER_CERTIFICATE);
}
diff --git a/src/libpcp/src/connect.c b/src/libpcp/src/connect.c
index 0386396..2548868 100644
--- a/src/libpcp/src/connect.c
+++ b/src/libpcp/src/connect.c
@@ -103,6 +103,18 @@ check_feature_flags(int ctxflags, int features)
*/
pduflags |= PDU_FLAG_CREDS_REQD;
+ if (features & PDU_FLAG_CERT_REQD){
+ /*
+ * This is a mandatory connection feature - pmcd must be
+ * sent a trusted certificate.
+ */
+ pduflags |= PDU_FLAG_CERT_REQD;
+ if( !(ctxflags & PM_CTXFLAG_SECURE) ){
+ /* PMCD requires a client cert, but we are not even setup for
secure connections */
+ return PM_ERR_NEEDCLIENTCERT;
+ }
Whitespace a bit inconsistent with the rest of that file, in a couple
of spots there. Comments tend to go for fitting-within-80-columns in
libpcp too (like the first one above). There's a few other cases of
those inconsistencies sprinkled around the patch, be nice to get 'em
fixed up pre-merge.
Logic is exactly what I was expecting to see, glad that worked out as
anticipated.
@@ -292,7 +312,19 @@ __pmSecureServerInit(void)
else
pathSpecified = 1;
- secsts = NSS_Init(secure_server.database_path);
+ /*
+ * pmproxy acts as both a client and server. Since the
+ * server init path happens first, the db previously
+ * got opened readonly. Instead try to open RW.
+ * Any downside to doing this by default?
+ * Should this be conditional on something?
I guess the only downside might be failing if write access is
not available on the DB (and if we don't need write, just read
access, shouldn't fail) ... ?
diff --git a/src/pmcd/src/dopdus.c b/src/pmcd/src/dopdus.c
+
+ /* Not sure if this will ever be hit. All cases checked during handshake?
*/
I'd definitely keep this safeguard, but maybe put into a separate
CheckCertificateRequired() routine ala CheckAccountAccess().
@@ -660,6 +669,11 @@ 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 for remote connections only*/
+ if( !__pmSockAddrIsLoopBack(cp->addr) &&
!__pmSockAddrIsUnix(cp->addr)){
+ cp->pduInfo.features |= PDU_FLAG_CERT_REQD;
I think there's no harm setting the protocol bit for all connections
(local and remote) saying that certs are required by this pmcd - but
just not enforce for local: connections. Keeps the pmcd code simpler
here and the protocol message the same for all clients.
@@ -203,6 +208,7 @@ VerifyClient(ClientInfo *cp, __pmPDU *pb)
__pmPDUHdr *header = (__pmPDUHdr *)pb;
__pmHashCtl attrs = { 0 }; /* TODO */
__pmCred *credlist;
+ unsigned int toggle_cert_required=0;
/* first check that this is a credentials PDU */
if (header->type != PDU_CREDS)
@@ -219,9 +225,32 @@ VerifyClient(ClientInfo *cp, __pmPDU *pb)
break;
}
}
+
if (credlist != NULL)
free(credlist);
+ /*
+ * Enforce PDU_FLAG_CERT_REQD for remote connections
+ *
+ * Not sure if this will ever be hit. Will all cases be caught during
handshake? See connect.c
+ */
+ if( ( ( cp->server_features & PDU_FLAG_CERT_REQD ) && ( (flags &
PDU_FLAG_SECURE) == 0) )){
+ if( !__pmSockAddrIsLoopBack(cp->addr) && !__pmSockAddrIsUnix(cp->addr)){
+ return PM_ERR_NEEDCLIENTCERT;
+ }
+ }
Quirky use of whitespace there again - to answer the question though,
yes, definitely enforce this here, as with pmcd.
+ /*
+ * If the server advertises PDU_FLAG_CERT_REQD, add it to flags
+ * so we can setup the connection properly with the client.
+ * The client should have errored out in the initial handshake if it
+ * didn't support secure connections, so we should only end up
+ * here if both client and server support this.
+ */
+
+ if( (cp->server_features & PDU_FLAG_CERT_REQD) )
+ flags |= PDU_FLAG_CERT_REQD;
This conditional is partially shared with the code above (same
initial if test) - some code refactoring possibilities there.
@@ -238,11 +267,24 @@ VerifyClient(ClientInfo *cp, __pmPDU *pb)
if (sts >= 0 && flags)
sts = __pmSecureClientHandshake(cp->pmcd_fd,
flags | PDU_FLAG_NO_NSS_INIT,
- hostname, &attrs);
+ cp->pmcd_hostname, &attrs);
> 5. I believe there was a typo in the __pmSecureClientHandshake call for
> pmproxy. As far as I could tell, the hostname passed in here should be
> of the remote pmcd, not localhost. I believe all other uses of this
> function behave that way. It seems to work either way but I think the
> only use of this is by the SSL libraries to do hostname verification on
> the certificate. If the local hostname is not needed, I believe the
> whole local hostname generation code can go as well.
(Yep, I think you're correct there - all of the above.)
+
+/* This is a private libpcp function. OK to expose ? Copied for now */
+__pmPDUInfo
+__ntohpmPDUInfo(__pmPDUInfo info)
+{
+ unsigned int x;
+
+ x = ntohl(*(unsigned int *)&info);
+ info = *(__pmPDUInfo *)&x;
+
+ return info;
+}
Ah, OK - see below.
sts = __pmGetPDU(cp->pmcd_fd, ANY_SIZE, 0, &pb);
+
+ /*
+ * We need to know if the pmcd has PDU_FLAG_CERT_REQD so we can
+ * setup our own secure connection with the client. Need to intercept
+ * the first message from the pmcd. See __pmConnectHandshake
+ * discussion in connect.c. This code happens before VerifyClient
+ * above.
+ */
+
+ if( (!cp->status.allowed) && (sts == PDU_ERROR) ){
+ int version;
+ int challenge;
+ __pmPDUInfo pduinfo;
+ unsigned int server_features;
+ int lsts;
+ version = __pmDecodeXtendError(pb, &lsts, &challenge);
+ if( version >= 0 && version == PDU_VERSION2 && lsts >=0 ){
+ pduinfo = __ntohpmPDUInfo(*(__pmPDUInfo *)&challenge);
+ server_features = pduinfo.features;
+ if( server_features & PDU_FLAG_CERT_REQD ){
+ /* Add as a server feature */
+ cp->server_features |= PDU_FLAG_CERT_REQD;
+ }
+ }
+ }
Hmm, this should all be wrapped up in a helper function - maybe
called ServerFeatures() or something like that? Oh, this is the
call site for that libpcp internal function - a new libpcp API to
extract this info cleanly from a PDU would be best I think. Some
name like __pmSecureServerConnectionFeatures() perhaps? (or some
better name if you can think of one)
> 6. Finally, pmproxy needs to intercept the initial extended error pdu
> that pmcd sends to the pmclient. I needed to use an internal libpcp
> function here. For testing, just copied the function. Not sure what
> the policy was on exporting this. Or if this should be wrapped as a new
> exportable function from libpcp. If that would require a new pcp major
> version, etc.
So, doesn't need a new major version - the two new symbols need to
be added to a "PCP_3.15" in src/libpcp/src/exports, update impl.h,
and you are done there.
Just noticed there's a couple of odd commits on this branch - can
you add correct attribution there? (or squash 'em via rebase if
they're yours?). Then its just final docs & QA & its all done!
commit f3c391d1e2e009f80e23e33c88285d503f03152c
Author: Cloud User <centos@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
Date: Tue May 3 10:34:55 2016 +0000
Allow pmproxy to pass through the CERT_REQD feature to clients
Author: Cloud User <centos@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
Date: Thu Apr 14 19:45:25 2016 +0000
Pre-allow self signed server certificates in secure connections
By setting PCP_SERVER_SELF_CERT, a client can instruct libpcp
to accept a self signed server certificate on its behalf.
cheers.
--
Nathan
|