User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7a) Gecko/20040107 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7a) Gecko/20040107 Not all details of a cert are reported, for example the subject-alternative-name(s) in the example URL's cert are *.onnet.cc, *.kensystem.om, & *.interactiveengines.com. However, none of these aliases can be seen by the cert exam tool. When examining the cert served by https://www.kensystem.com, only the *.interactiveengines.com CN can be seen, this may confuse some folks. Reproducible: Always Steps to Reproduce:

Actually, it DOES show all of them, but a) the extension OIDs are shown in decimal, not as meaningful strings, and b) the extension values are shown in hexadecimal, not ASCII or UTF8. So, unless you can read the hex, and know the OIDs (as some of us do :-) the display is pretty opaque. So, I am changing the summary of this bug report to be slightly more accurate.

The OIDs were addressed by the fix to bug 97406. Work on the values is blocked by bug 232812, which has been awaiting review for a week.

I believe I need to use CERT_DecodeGeneralName() to implement this. That function is exported in nss.def, but it is declared in genname.h which NSS doesn't export. Nelson?

It seems that we exported CERT_DecodeGeneralName for PSM, but the PSM function that uses it has been commented out. In any case, if CERT_DecodeGeneralName is suitable for being exported, we should move its declaration from genname.h to cert.h.

Requires a newer version of NSS than is currently tagged. Largely cribbed off of nss/cmd/lib/secutil.c

Comment on attachment 144286 [details] [diff] [review] 2 of 3: Make GetPIPNSSBundleString append instead of replace Instead of doing this, perhaps you could pass in a parameter that specifies whether to replace or append, and make it default to replace?

Comment on attachment 144287 [details] [diff] [review] 3 of 3: decode subject alt names and simple strings In ProcessRawString and ProcessRawBytes, it looks like you can get into a situation (data->len == 0 or data->data[data->len-1] == '\n') where you'll emit |level| spaces without them being followed by anything. How about: static nsresult ProcessRawString(SECItem *data, int level, nsAString &text) { PRUint32 i; PRBool doIndent = PR_TRUE; ProcessIndent(level, text); for (i=0; i < data->len; i++) { if (doIndent) { ProcessIndent(level, text); doIndent = PR_FALSE; } char c = data->data[i]; if (c == '\r' && i+1 < data->len && data->data[i+1] == '\n') continue; if (c == '\n') { text.Append(PRUnichar('\n')); doIndent = PR_TRUE; continue; } if (c < ' ' || c >= 0x7f) c = '.'; text.Append(PRUnichar(c)); } text.Append(PRUnichar('\n')); return NS_OK; } Also, to aid l10n and RTL languages, instead of manually appending names etc. to CertDumpGNameOther etc., put in a %S and use PIPBundleFormatStringFromName. Looks good otherwise.

Comment on attachment 144285 [details] [diff] [review] 1 of 3: Move cert details code to nsNSSCertHelper.cpp Why did you move nsNSSCertificate::CreateTBSCertificateASN1Struct and nsNSSCertificate::CreateASN1Struct? Wouldn't it be better to expose the helper functions they need in nsNSSCertHelper.h and keep the above functions in the nsNSSCertificate.cpp file?

Comment on attachment 144285 [details] [diff] [review] 1 of 3: Move cert details code to nsNSSCertHelper.cpp Moving the functions puts all of the code related to the cert details tab in one file. That allows us to avoid exposing functions across files and gives the compiler better opportunities to make inter-function optimizations. There is no advantage to having CreateTBSCertificateASN1Struct and CreateASN1Struct in nsNSSCedrtificate.cpp

Comment on attachment 144285 [details] [diff] [review] 1 of 3: Move cert details code to nsNSSCertHelper.cpp sr=jag Could you address my questions/comments on the other two patches?

Is it too late for any patch for this to make it into Deerpark betas? It SURE would be nice of we could fully inspect certificates in 1.5, especially subject-alt names. Bug 281446 is a duplicate of this; please feel free to flag that, and/or communicate with any independent fixers there. Bug bug 259031 also appears related.

*** Bug 281446 has been marked as a duplicate of this bug. ***

Basically, this patch train got derailed by comment 8. I never found the time to go and re-audit all of the code to implement an arbitrary design change.

Ken, it is too late for Firefox 1.5. The next release will be Firefox 2.0.

I think this got fixed a while ago. When I use a trunk build and cert viewer to display the cert from the site mentioned in comment 0, cert viewer lists 4 subject alt names.