183824 - Implement an alternative mechanism to ensure SOAP security

Status: RESOLVED FIXED

(Core :: XML, defect, P1)

Description

[harishd]

Comment 1

[harishd]

Attachment: Security Model Documentation, draft 1.0

Comment 2

[Mitchell Stoltz (not reading bugmail)]

Looks great. Do we have a demo?

Comment 3

[harishd]

Mitch: Not yet. I discovered a flaw in my implementation and hence rewiring my code.

Comment 4

[harishd]

Attachment: patch v1.0

Note: This is not the final patch. However, would appreciate reviews.

Comment 5

[harishd]

Attachment: patch v1.1

Please ignore patch v1.0

Comment 6

[harishd]

Mitch and I breifly discussed about the patch and Mitch brought up the point
that I should be comparing nsIURIs instead of plain uri strings. I'll work on
that but that shouldn't stop people from reviewing the patch :-)

Comment 7

[Johnny Stenback (:jst)]

A couple of general notes before I'll start looking at this line-by-line...

I see this all over:

+NS_NAMED_LITERAL_STRING(webScriptAccess, "webScriptAccess");
+const nsAString& kWebScriptAccessTag = webScriptAccess;

Why not just:

+NS_NAMED_LITERAL_STRING(kWebScriptAccessTag, "webScriptAccess");

?

The struct 'Attributes' should IMO be renamed, Attributes is IMO way too
generic, find a name that describes the struct better. And while you're at it,
make it a class and incapsulate the cleanup of the struct/class into its destructor.

+  PRBool success = status / 100 == 2;

Why help the optimizer a bit and write this as:

+  PRBool success = status == 200;

This would also make it possible to search the code for the success code 200.

- In nsSOAPSecurity::CreateAccessInfoEntry():

+    node->GetAttributes(getter_AddRefs(attrs));
+    NS_ENSURE_TRUE(attrs, NS_ERROR_UNEXPECTED);
+
+    attrs->GetNamedItemNS(kNamespace2002, kTypeAttr,
+                          getter_AddRefs(attr));
+    if (attr) 
+      attr->GetNodeValue(type);

Couldn't this be done using nsIDOMElement::GetAttributeNS()? That would
definately be faster than creating an attribute list (they're lazily created, so
evey call here would malloc to create the list).

This method also uses ToNewUnicode() in a few places w/o doing any error
checking, i.e. it doesn't deal with OOM (and FreeEntries() frees the memory that
was returned by ToNewUnicode() w/o checking that it's not null too).

- In nsSOAPSecurity::ValidateDocument():

+  nsCOMPtr<nsIDOMNode> rootNode(do_QueryInterface(rootElement));
+  NS_ENSURE_TRUE(rootNode, NS_ERROR_UNEXPECTED);

An element is always a node, therefore the error checking is unnecessary. And
since you're only using rootNode to get the children of the root element, you
don't even need rootNode, nsIDOMElement inherits nsIDOMNode, so you can call
GetCholdNodes() on rootElement directly...

I still think it would be a good idea to put a InvalidateCache() method in the
nsISOPASecurityService interface, even if there are no callers today. The
implementation of this would be on the order of 2 lines, so the cost is really
really low, and some developers migh find this extremely useful.

Fix that, and what you talked to Mitch about and I can have a closer look...

Comment 8

[Arun Ranganathan]

Harish, 

Can you explain the alternate algorithm?  What exactly does this new model
consist of?  What evangelism tasks are necessary (e.g. should documentation
exist on evangelizing SOAP services to do anything additional in order to allow
cross-domain Mozilla access?)

Comment 9

[harishd]

Arun, please take a look a the draft ( v1.0 ) in comment #1.

Comment 10

[harishd]

Attachment: patch v1.2

Addressing jst's concerns. This patch also includes error reporting ( localized
).

Comment 11

[harishd]

Comment on attachment 112325 [details] [diff] [review]
patch v1.2

Note to reviewers: 
Please ignore the changes in nsHTTPSOAPTransport.cpp. 
I was only using those changes to trigger my code.

Comment 12

[harishd]

Attachment: tar ball ( gzipped ) - Includes changes suggested by rayw.

Note: I've created a new directory called "security" under xmlextras and have
named the new files as nsIWebScriptsAccessService and nsWebScriptsAccess.* to
reflect the proposed security model. I'll post a patch once the directory/file
names are finalized.

Comment 13

[harishd]

Attachment: Tar ball.

Comment 14

[harishd]

Attachment: patch

This patch includes:
1) Build changes 
2) Code to trigger the new security.

Comment 15

[harishd]

Comment on attachment 118101 [details] [diff] [review]
patch

Adding jst's email sr=

Comment 16

[harishd]

Attachment: Tarball of new security implementation [ gzipped ]

Comment 17

[harishd]

Comment on attachment 118103 [details]
Tarball of new security implementation [ gzipped ]

Adding jst's email sr=

Comment 18

[Ray Whitmer]

Comment on attachment 118101 [details] [diff] [review]
patch

r=rayw

Comment 19

[Ray Whitmer]

Comment on attachment 118103 [details]
Tarball of new security implementation [ gzipped ]

One thing to fix, on line 128-129, change to:

if (rhs_end == rhs_begin)
  return PR_FALSE;

Comment 20

[Ray Whitmer]

Comment on attachment 118103 [details]
Tarball of new security implementation [ gzipped ]

One thing to fix, on line 128-129, change to:

if (rhs_end == rhs_begin)
  return PR_FALSE;

r=rayw

Comment 21

[harishd]

Feature landed and enabled.

Comment 22

[Matthias Versen [:Matti]]

Comment on attachment 112766 [details]
tar ball ( gzipped ) - Includes changes suggested by rayw.

clearing obsolete review requests