497172 - Add default OCSP responder for globalsign

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement)

Description

[Johnathan Nightingale [:johnath]]

Globalsign has a live OCSP responder now, and would like to make use of the default OCSP responder mechanism implemented by bug 485052.

The data structure that controls this is here:

http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSCallbacks.cpp#1058

So we just need that information (issuer name, issuer key ID) for each cert (including intermediates) that Globalsign wants to have OCSP checking for using this mechanism.

Comment 1

[Steve Roylance]

The Intermediate certificates are not OCSP enabled.

The end untity certifiactes are issued by "GlobalSign Extended Validation CA"
The (SKI) is "34 b1 f9 c9 8c 6b 35 44 cc 08 69 0a ee e3 a3 b9 5c bf 16 e0" 

The format is therefore

 /* GlobalSign */
{
"CN=GlobalSign Extended Validation CA,O=GlobalSign,OU=Extended Validation CA",
nsnull, "to be added by engineering team in Japan", nsnull,
"http://evssl-ocsp.globalsign.com/responder "
},

Engineering need to convert the SKI to the relevant form.

Comment 2

[Masakazu Asano]

Here is a KeyID of issuer in base64

"NLH5yYxrNUTMCGkK7uOjuVy/FuA="

Masakazu Asano
Tech Div. 
GlobalSign K.K.

Comment 3

[Steve Roylance]

Here's the final logic for the code which amends the URL.

/* GlobalSign */
{
"CN=GlobalSign Extended Validation CA,O=GlobalSign,OU=Extended Validation CA",
nsnull, "NLH5yYxrNUTMCGkK7uOjuVy/FuA=", nsnull,
"http://ocsp.globalsign.com"
}

Many thanks for a swift implementation.

Comment 4

[Steve Roylance]

A final review with the team suggests that in readiness for a separate OCSP responder for EV Code signing we should change the URL slightly

/* GlobalSign */
{
"CN=GlobalSign Extended Validation CA,O=GlobalSign,OU=Extended Validation CA",
nsnull, "NLH5yYxrNUTMCGkK7uOjuVy/FuA=", nsnull,
"http://ocsp.globalsign.com/ExtendedSSL"
}

Many thanks for a swift implementation.

Comment 5

[Kai Engert [:KaiE:]]

Who wants to produce the code patch?
Then I can review it.

Comment 6

[Hideki Itoyama]

Attachment: Added GlobalSign to the data structure

Comment 7

[Steve Roylance]

Attachment: OCSP amendment

Comment 8

[Steve Roylance]

The Patch has been submitted.  Previous datastructure had a missing ,

Comment 9

[Kai Engert [:KaiE:]]

Comment on attachment 382695 [details] [diff] [review]
OCSP amendment

r=kaie

Comment 10

[Johnathan Nightingale [:johnath]]

kaie - did your review include testing? Do we know that this change will resolve the issue?

Note to drivers - I know that RC1 is already spinning. If there is an opportunity for this to land as part of a hypothetical second RC, it is a code change but extremely low-risk, since it just adds another entry to a list of known sources for certificate validation. Even if it were found to be incorrect, it would fail back to the behaviour we currently have in the product. The reward is that certificates from another EV-approved CA are properly checked for revocation and show EV UI again.  This is the CA that issues the certificate for addons.mozilla.org, among others.

Comment 11

[Kai Engert [:KaiE:]]

(In reply to comment #10)
> kaie - did your review include testing? Do we know that this change will
> resolve the issue?

No

Comment 12

[Kai Engert [:KaiE:]]

Now I've been able to test it.

Unfortunately, this patch is still not sufficient to make GlobalSign sites (e.g. addons.mozilla.org) appear with green EV status.

Suspected reason: because OCSP checking for the intermediate certs doesn't work.

abbreviations:
- globalsign fake ocsp patch from this bug: gs-ocsp-patch
- a patch I used for testing that drops the requirement to have revocation info
  on intermediate certs: ignore-chain-patch

Test results for addons.mozilla.org for current ff 3.5 (mozilla-1.9.1) with or without patches:

no patches: no green
ignore-chain-patch : no green
gs-ocsp-patch: no green
gs-ocsp-patch + ignore-chain-patch : green

This tells me, the patch in this patch improves the situation slightly, because we do get an OCSP result for the server cert, but it's not sufficient, because by our policy we require info for the chain, too.

Comment 13

[Hideki Itoyama]

Attachment: Created patch to add GlobalSign OCSP

Comment 14

[Kai Engert [:KaiE:]]

Comment on attachment 385730 [details] [diff] [review]
Created patch to add GlobalSign OCSP

r=kaie

I don't mind adding this patch, but would have prefered to test first.

Comment 15

[Hideki Itoyama]

Attachment: Updated OCSP URL for GlobalSign.

Comment 16

[Johnathan Nightingale [:johnath]]

I applied this patch to my 3.5 source tree and I still see no EV for addons.mozilla.org (I was using globalsign.com, but that cert now has an AIA field).  Did this work in your own tests? Has something changed?

Comment 17

[Eddy Nigg (StartCom)]

The AIA extension of the addons.mozilla.org server certificate doesn't contain an OCSP URI.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Eddy, surely you remember bug 485052 and bug 477244.  This is all about 
making OCSP usable from certs from certain CAs that have OCSP responders
but did not put their OCSP URLs in their certs.

Comment 19

[Masakazu Asano]

(In reply to comment #16)
> I applied this patch to my 3.5 source tree and I still see no EV for
> addons.mozilla.org (I was using globalsign.com, but that cert now has an AIA
> field).  Did this work in your own tests? Has something changed?

We have tested and it works for addons.mozilla.org.
I'm wondering what is the difference between our build and yours....

Followings are information of 3.5 source that we are using;
  Downloaded from : ftp://ftp.mozilla.org/pub/firefox/releases/3.5/source/ 
  Downloaded file name : firefox-3.5-source.tar.bz2 
  Last update : 2009/06/29 16:26:00
  Extracted Folder name : mozilla-1.9.1

Can you please let me know how did you build yours?

Comment 20

[Samuel Sidler (old account; do not CC)]

Can this land on trunk so we can consider it for 1.9.1.1? kaie, johnath? Not blocking anyway, but we'll definitely approve a patch.

Comment 21

[Steve Roylance]

Attachment: Final debugged Patch

This final patch has the " " added :-)

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 388763 [details] [diff] [review]
Final debugged Patch

Steve, I want to double-check something with you.  

The diffs between the patch in 
attachment 385730 [details] [diff] [review] ("Created patch to add GlobalSign OCSP") and 
attachment 386710 [details] [diff] [review] ("Updated OCSP URL for GlobalSign.") 
include these changes to the URLs:

-+    "http://ocsp.globalsign.com/ExtendedSSLCACross"
++    "http://ocsp.globalsign.com/ExtendedSSL"

-+    "http://ocsp.globalsign.com/ExtendedSSLCA"
++    "http://ocsp.globalsign.com/ExtendedSSL"

However, the differences between
attachment 385730 [details] [diff] [review] ("Created patch to add GlobalSign OCSP") and 
attachment 388763 [details] [diff] [review] (your "Final debugged Patch") 
do not include those changes, and instead include only this change 
(shortened here to avoid line wrap):

-+    "O=GlobalSign,OU=Extended ValidationCA",
++    "O=GlobalSign,OU=Extended Validation CA",

My question is: are the changes in attachment 386710 [details] [diff] [review] no longer wanted?

Comment 23

[Steve Roylance]

Hi Nelson,   Thanks for checking.  Yes the initial patch was correct apart from the space in the CA name.  The 2nd attachment was a red herring.   The third and final attachment is correct and so is 99% the same as the first.  Sorry for any confusion.

Comment 24

[Rob Stradling]

> +    "CN=GlobalSign Root CA,O=GlobalSign nv-sa,OU=Root CA,C=BE",

Steve, the O and OU in this string are the "wrong way round" compared to their order in the GlobalSign Root CA certificate.  This isn't a problem for the current implementation of MyAlternateOCSPAIAInfoCallback(), since CERT_CompareName() doesn't care about the order of the attributes.
However, for the sake of clarity (and just in case this behaviour ever changes), might I suggest that you put them the "right way round" instead?
e.g.
+    "CN=GlobalSign Root CA,OU=Root CA,O=GlobalSign nv-sa,C=BE",

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

> CERT_CompareName() doesn't care about the order of the attributes.

It requires RDNs to be in the same order in both names.  It will not find
two names in different orders to match, e.g.
   CN=GlobalSign Root CA,OU=Root CA,O=GlobalSign nv-sa,C=BE
   CN=GlobalSign Root CA,O=GlobalSign nv-sa,OU=Root CA,C=BE
will not match.

It is possible (though quite rare in practice) for a single RDN to contain 
multiple ATAV pairs.  Since RDNs are SETs, the order of the multiple ATAVs 
in a "multi-valued RDN" are not supposed to matter, so when comparing two 
multi-valued RDNs, the order of the ATAVs in the RDN do not matter.  When 
shown as a string, the ATAVs in a multi-valued RDN are separated by '+' 
rather than by ',' (which separates RDNs).

In the example names given above, there are no multi-valued RDNs, so this 
factoid about multi-valued RDNs is not relevant to them.

Comment 26

[Johnathan Nightingale [:johnath]]

Landed on mozilla-central for baking, though baking is really more of a technicality for this kind of change which is purely metadata.

http://hg.mozilla.org/mozilla-central/rev/ea87b5fe8a94

Asking for approval to land this on the 1.9.1 branch as soon as it opens up.

Steve - based on Rob's comment 24, and my own inspection, I updated the issuer name for that cert (after verifying that correct behaviour was preserved). If there was a reason why you wanted it different (not sure why that would be, but I'm open to the possibility!) let me know, but I have assumed it was an oversight.

Comment 27

[Johnathan Nightingale [:johnath]]

Comment on attachment 388763 [details] [diff] [review]
Final debugged Patch

Per comments above, requesting approval to land this on 1.9.1.2. There is negligible risk here, and the upside is that we fix validation of certificates for this CA.

Comment 28

[Steve Roylance]

Thanks Johnathan for your help.   And thanks to Rob for his initial feedback.  Yes this would have been an oversight.

Comment 29

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 388763 [details] [diff] [review]
Final debugged Patch

a1912=beltzner

Comment 30

[Johnathan Nightingale [:johnath]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ebdd03c0c753

This has landed on the 1.9.1 stream, should be testable in tomorrow's nightly 1.9.1 builds, and should appear in Firefox 3.5.2 when it is released.

Comment 31

[juan becerra [:juanb]]

Verified using information in comment #12 with:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 (.NET CLR 3.5.30729) As well as 3.5.2 on Linux and Mac.

3.5 shows the identity button blue, whereas 3.5.2 shows it green for https://addons.mozilla.org/ with the extended validation information.