search

w3c / webappsec-credential-management

WebAppSec Credential Management
https://w3c.github.io/webappsec-credential-management/
Other
50 stars 38 forks source link

CREDENTIAL: Generic matching algorithm #4

Open mikewest opened 9 years ago

mikewest commented 9 years ago

From @adrianhopebailie on April 22, 2015 23:25

This change updates the model for Credential to include an isMatch() function hook which is called during processing of the Credential request algorithm. The hook can be overridden by developers designing their own Credential classes in script therefor making the API flexible and extensible through ECMAScript.

Resolves #274

Copied from original issue: w3c/webappsec/pull/292

mikewest commented 9 years ago

Before I skim in more detail, would you mind rebasing on top of master? This patch doesn't cleanly apply locally.

mikewest commented 9 years ago

(Also, thanks for taking a stab at this!)

mikewest commented 9 years ago

From @adrianhopebailie on April 23, 2015 9:16

Before I skim in more detail, would you mind rebasing on top of master? This patch doesn't cleanly apply locally.

@mikewest Not sure why, I mistakenly committed my .gitignore but have reverted that. The changes are only in index.src.html and were on top of the latest from master.

My git-fu is pretty basic, any suggestions on how to fix this?

mikewest commented 9 years ago 
git add remote w3c https://github.com/w3c/webappsec.git
git fetch w3c
git rebase w3c/master
[resolve conflicts]

Something like the above should do the trick! :)

mikewest commented 9 years ago

From @adrianhopebailie on April 23, 2015 10:43

Done.

Mixing and matching betwen the GUI tool and command line is a mistake! Already had w3c as a remote just hadn't done a rebase (thought Git for Mac did this automatically when you did a sync - clearly not).

This was my first adventure into writing WebIDL definitions so there may be some tweaks required however I think it get's the genera; gist across.

I'll add an example of using a custom credential (defined in script) later today as I think that will truly illustrate the reasons for the change.

mikewest commented 9 years ago

Thanks for the PR! I've left a number of comments.

I'm fine with the central suggestion of exposing the matching algorithm, as that seems to allow some extensibility.

However, I don't like the fact that this makes the use cases the document actually intends to address more complex. Especially in the case of PasswordCredentials, the call is simply unintuitive, and will just add boilerplate to web developers' code. I'd also like to avoid the extra level of nesting for FederatedCredentials.

It's also not clear to me that filter is the right way to frame the thing that you're suggesting, as I don't understand how that verb makes sense for credential types that aren't stored locally in the browser's credential store. If I have to go out to a third-party site to grab data, I'm not filtering anymore. I'm doing something else. I couldn't come up with a term for that which made sense, and settled on the ultra-generic "options".

Does "filtering" fit into @dlongley's modeling of the world? It's not clear that it does.

mikewest commented 9 years ago

From @adrianhopebailie on April 27, 2015 20:23

@mikewest I've been poring over this and I have come to conclusion that it won't work...

The issue is that trying to create something extensible to the developer means creating a callback that could potentially open a security hole since the code is defined and executed against the DOM.

A Credential type that allows something like this will need to be explicitly designed with this in mind and be able to handle the callback having potentially malicious code. Putting this in the base class is asking for trouble.

mikewest commented 9 years ago

What's the threat model you're worried about? If an attacker can define an arbitrary callback, they can already execute arbitrary code in your origin, can't they?

mikewest commented 9 years ago

From @adrianhopebailie on April 28, 2015 14:41

Again, I may be showing my ignorance wrt the differentiation between the execution context of code behind a browser API vs that defined on the DOM within client scripts.

My proposal was for the site to provide a script that defines a custom Credential type and in so doing provide the execution logic for the isMatch() function. That means there is nothing stopping a rogue script from injecting logic that will be executed within what should be a secure context or am I wrong?

I like the idea of the extensibility you get from encapsulating the filtering logic in that function but don't really understand enough about the line between the two execution contexts within the browser to add anything more to this proposal.