Implement notification for EV certs

RESOLVED FIXED

Status

()

RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: kaie, Assigned: kaie)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

12 years ago
As of today, the security status tracking code in PSM uses nsISecurityEventSink::onSecurityChange to notify consumers about the security state of a web page.

This function is defined as
     void onSecurityChange(in nsISupports i_Context, 
                           in unsigned long state);

The possible state values are defined nsIWebProgressListener.
Unfortunately nsIWebProgressListener is a frozen interface!

How should PSM communicate the additional EV status to the consumers?

Ideally the notification of all status information should be done with a single function call. It simplifies the life of event consumers if they receive information "secure page" and "uses EV cert" at the same time.


In the past I had considered to extend the definition of parameter "state" with additional bits. But this approach has problems. While nsISecurityEventSink is NOT frozen, the definition of states in nsIWebProgressListener is frozen. The implementation of event consumers in Firefox and SeaMonkey doesn't look for bits, they look for absolute values. It's likely that other implementations do the same. By adding new bits we confuse the existing implementation, with the risk that noone notices.


Because nsISecurityEventSink is NOT frozen, we could change the existing API to use an additional parameter and force all implementations to update their code. 


Or we could implement a new nsISecurityEventSink2 interface, with an extended onSecurityChange method. PSM could attempt to QueryInterface the listener to the latter interface, which includes notification about EV, and possibly fall back to the older API.


What should we do?
(Assignee)

Comment 1

12 years ago
Besides the decision about notification API to external consumers, there is more work that I want to do in the scope of this bug.


It is an expensive operation to make the decision whether a certificate is to be treated as EV or not.

In this initial implementation I limit the cert analysis to the cert used by the top level document of the page.

As only the document loading tracker is able to know whether a given SSL socket is the top level document of a web page, this is the place where I'm requesting the EV analysis.

In order to enable this late analysis, I'm adding a reference to the cert to each channel's security status object.
(Assignee)

Comment 2

12 years ago
Hmm, the issue is more complicated than I thought.

The security notification is not sent directly from producer (PSM) to end consumers (like browser UI). In the middle sits nsDocLoader which makes a translation to another interface

PSM calls
  nsISecurityEventSink {
    void onSecurityChange(in nsISupports i_Context, in unsigned long state);
  }

uriloader listens for that, iterates over its list of all registered web progress listeners and calls each with
  nsIWebProgressListener {
    void onSecurityChange(in nsIWebProgress aWebProgress,
                          in nsIRequest aRequest,
                          in unsigned long aState);
  }


If we were to introduce a new interface nsISecurityEventSink2, we'd have to change nsDocLoader to implement that as well.
In addition we'd have to extend nsIWebProgressListener2 with a new onSecurityChange method that carries EV state.
The new nsDocLoader::OnSecurityChange would attempt to QI each consumer to nsIWebProgressListener2 to make a decision whether to call the old or new function...
(Assignee)

Comment 3

12 years ago
I'm taking a step back to my original simple approach.

I've been told that we added new bits to frozen interfaces in the past, too.

In addition, the comment for parameter state of nsIWebProgressListener::onSecurityChange explicitly says:

  "Any undefined bits are reserved for future use."


I'm therefore going to declare it a bug if existing consumers check for absolute values (instead for bits).

I'll attach a patch that implements the simple approach.
(Assignee)

Comment 5

12 years ago
Created attachment 270667 [details] [diff] [review]
ui hack for testing purposes

This patch is NOT meant for check in, I only use it for testing.
It is an example how browser could catch the new event and bring up better UI for it.

It also changes handling of existing states to look for bits set (instead of looking for absolute values).
(Assignee)

Updated

12 years ago
Blocks: 374336
(Assignee)

Comment 6

12 years ago
Adding Benjamin to cc list because the proposed patch adds a new state flag to nsIWebProgressListener(2) in uriloader.
Status: NEW → ASSIGNED
(Assignee)

Comment 7

12 years ago
Created attachment 270668 [details] [diff] [review]
stub for nsNSSSocketInfo::GetIsExtendedValidation

I'm attaching this little helper patch that allows you to get Patch v1 compiled.

Patch v1 requires the implementation of nsNSSSocketInfo::GetIsExtendedValidation.

At this time, no working implementation is available. We will focus on getting that done in bug 374336. So for the time being and while testing interfaces, feel free to use this stub.
(Assignee)

Comment 8

12 years ago
(In reply to comment #3)
> I'm therefore going to declare it a bug if existing consumers check for
> absolute values (instead for bits).

I filed bug 386681 to get that fixed in the places I found.
(Assignee)

Updated

12 years ago
Depends on: 386681
(Assignee)

Updated

11 years ago
Blocks: 383183
(Assignee)

Comment 9

11 years ago
Comment on attachment 270665 [details] [diff] [review]
Patch v1

Bob, can you please review?

Boris, can you please r/sr the change in uriloader?
Attachment #270665 - Flags: superreview?(bzbarsky)
Attachment #270665 - Flags: review?(rrelyea)
(Assignee)

Comment 10

11 years ago
Comment on attachment 270668 [details] [diff] [review]
stub for nsNSSSocketInfo::GetIsExtendedValidation

Bob, can you please review this stub as well?
Attachment #270668 - Flags: review?(rrelyea)
(Assignee)

Updated

11 years ago
Attachment #270667 - Attachment is obsolete: true
Why are we putting the bit on nsIWebProgressListener2 if we're going to send the notification to implementations of nsIWebProgressListener?
(Assignee)

Comment 12

11 years ago
(In reply to comment #11)
> Why are we putting the bit on nsIWebProgressListener2 if we're going to send
> the notification to implementations of nsIWebProgressListener?

Because nsIWebProgressListener is frozen. However, the API mentions the possibility to add more flags in the future.
Frozen doesn't mean you can't add flag bits to it if the API provides for such.  We do that all the time.  In particular, see rev 1.17 of nsIWebProgressListener.idl.

I'd prefer that we put the flag on that interface, and if desired added a comment that that flag only appears in Gecko 1.9 and later.

Comment 14

11 years ago
Comment on attachment 270668 [details] [diff] [review]
stub for nsNSSSocketInfo::GetIsExtendedValidation

r+ = rrelyea.
Attachment #270668 - Flags: review?(rrelyea) → review+

Comment 15

11 years ago
Comment on attachment 270665 [details] [diff] [review]
Patch v1

r+ on the psm part of this patch, assuming the stub is checked in first. and bzb's interface issues are addressed.
Attachment #270665 - Flags: review?(rrelyea) → review+
(Assignee)

Comment 16

11 years ago
Created attachment 275445 [details] [diff] [review]
Patch v2

Differences from Patch v1:

- moved flag to existing idl file
- renamed flag to make it more obvious this is related to identity
- now includes the stub, which I had attached separately.

Carrying forward r=rrelyea

Boris, does this look ok?
Attachment #270665 - Attachment is obsolete: true
Attachment #270668 - Attachment is obsolete: true
Attachment #275445 - Flags: superreview?(bzbarsky)
Attachment #275445 - Flags: review+
Attachment #270665 - Flags: superreview?(bzbarsky)
Comment on attachment 275445 [details] [diff] [review]
Patch v2

sr=bzbarsky
Attachment #275445 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Updated

11 years ago
Flags: blocking1.9?
(Assignee)

Updated

11 years ago
Attachment #275445 - Flags: approval1.9?
(Assignee)

Updated

11 years ago
Flags: blocking1.9?
(Assignee)

Updated

11 years ago
Attachment #275445 - Flags: approval1.9?
(Assignee)

Comment 18

11 years ago
Created attachment 276552 [details] [diff] [review]
Patch v2 with merging conflicts fixed

I'm in merging hell. This patch slightly collides with the work I'm doing in bug 327181.

This is a new patch that still works correctly, has code just slightly moved, but is free of conflict with my patch from bug 327181.

I'd appreciate being able to land this on trunk soon, thanks in advance for considering to approve :-)
Attachment #275445 - Attachment is obsolete: true
Attachment #276552 - Flags: superreview+
Attachment #276552 - Flags: review+
Attachment #276552 - Flags: approval1.9?
Comment on attachment 276552 [details] [diff] [review]
Patch v2 with merging conflicts fixed

a=bzbarsky.  Yay finally having PSM in the approval queries!
Attachment #276552 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 20

11 years ago
fixed on trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.