I'd like PSM to extract the list of valid DNS names from a cert. Bob R pointed out, this requires the use of CERT_FindNSStringExtension(nssCert, SEC_OID_NS_CERT_EXT_SSL_SERVER_NAME); which can override the name found in a cert's CN. Unfortunately CERT_FindNSStringExtension is not being exported. Can we do that for 3.12?

Because CERT_VerifyCertName uses it, right? Would it be OK if we changed CERT_VerifyCertName to not call it any more, and retire function CERT_VerifyCertName instead? If not, then go ahead and write the one-line patch for whatever .def file should export it, and ask Julien to review it. :)

er, sorry, copy-n-paste error there. I meant ... and retire function CERT_FindNSStringExtension ... (Yes, I know it also has one other caller)

(In reply to comment #1) > Because CERT_VerifyCertName uses it, right? Because Bob recommended me to do so :-) > Would it be OK if we changed CERT_VerifyCertName to not call it any more, > and retire function CERT_VerifyCertName instead? Even after reading comment 2 I'm not sure what you intended to ask, sorry :-/ Are you proposing to retire function CERT_FindNSStringExtension? I don't understand why you would propose that and how that would be related to this bug. There seem to be multiple callers from NSS to function CERT_FindNSStringExtension. Why would retiring CERT_FindNSStringExtension and using CERT_VerifyCertName help me? I think it would not? I don't want to verify names, I want to extract names.

Sorry for not making my counter-proposal clear. Let me try again. I believe that the motivation for extracting DNS names from certs is to display the names in various dialogs and/or error pages, right? Isn't bug 238142 really the motivation for this RFE? The DNS names that want to be displayed are the names that may be used to satisfy the matching of server names from URLs, right? A DNS name that appears somewhere in the cert (such as in the OU) where it cannot be used to match against the hostname in a URL is not of interest, right? So, presently NSS's name matching code (CERT_VerifyCertName) includes names from an old Netscape cert extension that was never standardized, and (AFAIK) is long since forgotten by any CAs that ever may have supported if (if there ever were any). And because CERT_VerifyCertName uses that as a potential source of matching DNS names, you want to get access to that name so it can be displayed with the other potentially matching DNS names in dialogs, right? My counter-proposal is that we remove the code that calls CERT_FindNSStringExtension from CERT_VerifyCertName, so that that obsolete Netscape extension ceases to be one of the places from which a DNS host name may be taken for matching. Doing so would mean that there is no need to display that name in dialogs to satisfy bug 238142.

Nelson, thanks a lot for your more detailed comment. > I believe that the motivation for extracting DNS names from certs is to > display the names in various dialogs and/or error pages, right? > Isn't bug 238142 really the motivation for this RFE? Right, although it was done as part of bug 398718. > The DNS names that want to be displayed are the names that may be used to > satisfy the matching of server names from URLs, right? right > A DNS name that appears somewhere in the cert (such as in the OU) where > it cannot be used to match against the hostname in a URL is not of interest, > right? right > So, presently NSS's name matching code (CERT_VerifyCertName) includes names > from an old Netscape cert extension that was never standardized, and (AFAIK) > is long since forgotten by any CAs that ever may have supported if (if there > ever were any). And because CERT_VerifyCertName uses that as a potential > source of matching DNS names, you want to get access to that name so it can > be displayed with the other potentially matching DNS names in dialogs, right? Right, or more specifially, if there are no sub alt names, the Netscape cert extension would have precendence over the common name (same as NSS code does). > My counter-proposal is that we remove the code that calls > CERT_FindNSStringExtension from CERT_VerifyCertName, so that that obsolete > Netscape extension ceases to be one of the places from which a DNS host name > may be taken for matching. Doing so would mean that there is no need to > display that name in dialogs to satisfy bug 238142. Well, I'm ok with your proposal if you think it's safe to do. But isn't there a risk that we will break some sites on the internet? Is it necessary to break such sites? I'm ok with either action that achieves consistency between NSS name matching and the application's name display. In addition to the alternatives (a) export the function (b) stop using the extension in NSS there is a third one: (c) Implement a new exported function that makes NSS return a list of all allowed names it uses for hostname matching. Doing (c) would free the application from repeating the logic.

Kai, I agree that your proposal: > Implement a new exported function that makes NSS return > a list of all allowed names it uses for hostname matching. Is really the right thing to do here. What do you think the interface for such a function should look like?

P2 because PSM would use it if it existed.

Comment on attachment 286038 [details] [diff] [review] Patch v1 I'm preemptively marking this r-. Kai's alternative proposal, a function that returns all the DNS names for matching, is the right answer.

(In reply to comment #7) > What do you think the interface for such a function should look like? We want it to return a list of strings, where each can be either a fully qualified name, or DNS wildcard pattern. I think the consumer does not need to differ between them. I propose we mirror a scheme already used by NSS, I found CERT_NicknameStringsFromCertList which uses type CERTCertNicknames which represents a count and a list of string pointers. Seems right to me. Unless you object, I think we simply reuse the existing type. If you object, I propose we duplicate the type under a new name. /* * Collect the valid DNS names or patterns from the given cert. * Returns NULL on failure. * Use CERT_FreeNicknames() to free the result. * * "cert" - the certificate */ CERTCertNicknames * CERT_GetValidDNSPatternsFromCert(CERTCertificate *cert); Or if you prefer the most simple solution: /* * */ int CERT_GetValidDNSPatternsFromCert(CERTCertificate *cert, char ***patterns) { *patterns = (char**)(malloc((sizeof char*) * count)); return count; } char **patterns = NULL; int number_of_patterns = CERT_GetValidDNSPatternsFromCert(cert, &patterns); char **walk = patterns; for (int i=0; i < number_of_patterns; ++i, ++walk) { printf("%s\n", *walk); free(*walk); } free(patterns); Phrew, triple *, I bet I made a minor bug in this code, please read it as pseudo code.

Kai, I think I could live with your proposal to usurp CERTCertNicknames. Would you like to write a patch for this for NSS 3.12?

Sigh. Even if you just want to return a little bit of data, where everything is straightforward, doing it in C is still a lot of detailed hacking. We should convert NSS to C++... ;-)

Comment on attachment 290958 [details] [diff] [review] Patch v2 I think the design of this new code is basically good, but I have a number of comments, mostly about function names, comments, and one function argument. Easy Stuff. 1. This new function, CERT_GetValidDNSPatternsFromCert, is trying to match the behavior of another, existing function, namely CERT_VerifyCertName, except that it outputs the names/patterns to be used for comparisons, rather than doing the comparisons. So, I think this new function should be implemented in the same source file where CERT_VerifyCertName is implemented, namely certdb.c, and close to CERT_VerifyCertName in that file. And I think the declaration of this new function should be near the declaration of CERT_VerifyCertName in cert.h. 2. I think there may potentially be a lot of confusion about what's being returned. As you well know, even though it has type CERTCertNicknames, it's actually NOT a set of Nicknames. But most readers of this code are not going to know that, so there needs to be a clear comment, immediately before the declaration in cert.h and before the definition (in certdb.c) making that point very clear: These "nicknames" are actually the host names, host name patterns, and/or IP address strings taken from the cert for host name comparison purposes. Now for some specifics in the code. 3. This code passes the address of a SECItem to cert_GetSubjectAltNameList as an argument. cert_GetSubjectAltNameList puts data into that secitem, and that data is freed by the caller sometime later. I'm guessing that this is because it was thought that the content of that secitem had to be preserved until after cert_GetNickNamesFromGeneralNames was called and had finished, for some reason. But IINM, it's not necessary to preserve the contents of that SECITEM outside of the scope of function cert_GetSubjectAltNameList, because the contents of that SECItem is copied into the arena by CERT_DecodeAltNameExtension, which is called by cert_GetSubjectAltNameList. So, it should be enough to define and initialize SECItem subAltName inside of cert_GetSubjectAltNameList, and unconditionally call SECITEM_FreeItem on that SECItem immediately after the call to CERT_DecodeAltNameExtension. No need for the contents of that SECItem to be accessible to any code outside of cert_GetSubjectAltNameList, I believe. (But if you think this analysis is wrong, please enlighten me.) 4. New function cert_CountGeneralNames doesn't do what its name suggests. It only counts general names of two specific types, and ignores the other types of general names that are allowed to exist in a subjectAltNames extension. So it should be given a name that better explains what it does, or else some developer will be tempted to call it for the purpose of counting ALL general names. 5. Similarly, cert_GetNickNamesFromGeneralNames doesn't do what its name suggests. It doesn't get nicknames. It populates the contents of a nicknames structure with values that aren't nicknames. I think we want the function name to be completely clear about what it's doing. As a code reviewer, I've seen many developers write code that calls a function, guessing that they know what it does based solely on its name. So, let's use names that won't tempt them to use this function for the wrong purpose. Maybe "cert_GetDNSPatternsFromGeneralNames"?

Nelson, thanks a lot for your review. I agree with all your comments and think I addressed all your change requests.

The diff diff is not very easy to read, so let me summarize my changes between v2 and v3, which I'm building by looking at a graphical side by side diff in tkdiff. - moved declaration and function to the new desired location - added a comment in the header - eliminated the SECItem parameter in cert_GetSubjectAltNameList - use a temporary SECItem in cert_GetSubjectAltNameList and free it asap - renamed cert_CountGeneralNames to cert_CountDNSPatterns - rename cert_GetNickNamesFromGeneralNames to cert_GetDNSPatternsFromGeneralNames

Comment on attachment 293051 [details] [diff] [review] Patch v3 r+=nelson Nits: >+ * numberOfGeneralNames should have been obtain from cert_CountDNSPatterns, should be -> obtained >+ /* could we allocate both the buffer of pointers and the string? */ should be -> Did we

Thanks for the review Nelson. Checked in, marking fixed. Checking in certdb/cert.h; /cvsroot/mozilla/security/nss/lib/certdb/cert.h,v <-- cert.h new revision: 1.63; previous revision: 1.62 done Checking in certdb/certdb.c; /cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v <-- certdb.c new revision: 1.88; previous revision: 1.87 done Checking in nss/nss.def; /cvsroot/mozilla/security/nss/lib/nss/nss.def,v <-- nss.def new revision: 1.184; previous revision: 1.183 done