On Fri, Mar 05, 2021 at 04:01:38PM -0500, Andrew Dunstan wrote: > Yeah, fixed this, added a bit more docco, and got rid of some bitrot.
I had a look at this patch. What you have here looks in good shape, and I have come comments. > + This option is probably best used in conjunction with a username map. > + The comparison is done with the <literal>DN</literal> in RFC2253 > format. You should add a link to the RFC, using https://tools.ietf.org/html/ as a base like the other parts of the docs. > /* Make sure we have received a username in the certificate */ > - if (port->peer_cn == NULL || > - strlen(port->peer_cn) <= 0) > + peer_username = port->hba->clientcertname == clientCertCN ? > port->peer_cn : port->peer_dn; Should have some parenthesis for clarity. But, couldn't you just use a switch based on ClientCertName to select the data you wish to use for the match? If a new option is added then it is impossible to miss that peer_username needs a value as a compiler would warn on that. > - (errmsg("certificate validation (clientcert=verify-full) failed for > user \"%s\": CN mismatch", > + (errmsg("certificate validation (clientcert=verify-full) failed for > user \"%s\": CN/DN mismatch", It would be cleaner to show in this LOG that it is either a CN or a DN mismatch, but not both of them. And you can make the difference with hba->clientcertname for that. > + len = X509_NAME_get_text_by_NID(x509name, NID_commonName, NULL, 0); > if (len != -1) > { > - char *peer_cn; I think that you should keep this local declaration here. > + /* use commas instead of slashes */ > + X509_NAME_print_ex(bio, x509name, 0, XN_FLAG_RFC2253); The reason is not obvious for the reader here (aka that commas are required as slashes are common when printing the DN, quoting upthread). Hence, wouldn't it be better to add a comment explaining that here? > + BIO_get_mem_ptr(bio, &bio_buf); BIO_get_mem_ptr() (BIO_ctrl() in the OpenSSL code) returns a status code. I think that we had better check after it. > + peer_dn = MemoryContextAlloc(TopMemoryContext, bio_buf->length + 1); > + memcpy(peer_dn, bio_buf->data, bio_buf->length); > + peer_dn[bio_buf->length] = '\0'; > + if (bio_buf->length != strlen(peer_dn)) > + { > + ereport(COMMERROR, > + (errcode(ERRCODE_PROTOCOL_VIOLATION), > + errmsg("SSL certificate's distinguished name contains > embedded null"))); > + BIO_free(bio); > + pfree(peer_dn); > + pfree(port->peer_cn); > + port->peer_cn = NULL; > + return -1; > + } > + > + BIO_free(bio); You could just do one BIO_free() once the memcpy() is done, no? > @@ -121,7 +121,7 @@ secure_open_server(Port *port) > > ereport(DEBUG2, > (errmsg_internal("SSL connection from \"%s\"", > - port->peer_cn ? port->peer_cn : "(anonymous)"))); > + port->peer_dn ? port->peer_dn : "(anonymous)"))); Could it be better for debugging to show both the CN and DN if both are available? > -} ClientCertMode; > +} ClientCertMode; > + > +typedef enum ClientCertName > +{ > + clientCertCN, > + clientCertDN > +} ClientCertName; Missing some indentation stuff here. > +# correct client cert using whole DN > +my $dn_connstr = $common_connstr; > +$dn_connstr =~ s/certdb/certdb_dn/; I would use a separate variable rather than enforcing an update of the existing $common_connstr created a couple of lines above. > +# same thing but with a regex > +$dn_connstr =~ s/certdb_dn/certdb_dn_re/; Same here. This depends on the last variable assignment, which itself depends on the assignment of $common_connstr. -- Michael
signature.asc
Description: PGP signature
