At Fri, 4 Feb 2022 17:06:53 +0000, Jacob Champion <pchamp...@vmware.com> wrote in > That works a lot better than what I had in my head. Done that way in > v4. Thanks!
Thanks! 0002: +#define PGSQL_AF_INET (AF_INET + 0) +#define PGSQL_AF_INET6 (AF_INET + 1) .. -#define PGSQL_AF_INET (AF_INET + 0) -#define PGSQL_AF_INET6 (AF_INET + 1) I feel this should be a part of 0001. (But the patches will be finally merged so maybe no need to bother moving it). > * The use of inet_aton() instead of inet_pton() is deliberate; the > * latter cannot handle alternate IPv4 notations ("numbers-and-dots"). I think we should be consistent in handling IP addresses. We have both inet_pton and inet_aton to parse IPv4 addresses. We use inet_pton in the inet type (network_in). We use inet_aton in server addresses. # Hmm. I'm surprised to see listen_addresses accepts "0x7f.1". # I think we should accept the same by network_in but it is another # issue. So, inet_aton there seems to be the right choice but the comment doesn't describe the reason for that behavior. I think we should add an explanation about the reason for the behavior, maybe something like this: > We accept alternative IPv4 address notations that are accepted by > inet_aton but not by inet_pton as server address. + * GEN_IPADD is an OCTET STRING containing an IP address in network byte + * order. + /* OK to cast from unsigned to plain char, since it's all ASCII. */ + return pq_verify_peer_name_matches_certificate_ip(conn, (const char *) addrdata, len, store_name); Aren't the two comments contradicting each other? The retruned general name looks like an octet array, which is not a subset of ASCII string. So pq_verify_peer_name_matches_certificate_ip should receive addrdata as "const unsigned char *", without casting. + if (name->type == host_type) + check_cn = false; Don't we want a concise coment for this? - if (*names_examined == 0) + if ((rc == 0) && check_cn) To me, it seems a bit hard to understand. We can set false to check_cn in the rc != 0 path in the loop on i, like this: > if (rc != 0) > + { > + /* > + * don't fall back to CN when we have a match > or have an error > + */ > + check_cn = false; > break; > + } ... > - if ((rc == 0) && check_cn) > + if (check_cn) The following existing code (CN fallback) > rc = openssl_verify_peer_name_matches_certificate_name(conn, > X509_NAME_ENTRY_get_data(X509_NAME_get_entry(subject_name, cn_index)), > first_name); is expecting that first_name has not been set when it is visited. However, with this patch, first_name can be set when the cert has any SAN of unmatching type (DNS/IPADD) and the already-set name leaks. We need to avoid that memory leak since the path can be visited multiple times from the user-program of libpq. I came up with two directions. 1. Completely ignore type-unmatching entries. first_name is not set by such entries. Such unmatching entreis doesn't increase *names_examined. 2. Avoid overwriting first_name there. I like 1, but since we don't make distinction between DNS and IPADDR in the error message emited by the caller, we would take 2? regards. -- Kyotaro Horiguchi NTT Open Source Software Center