search

w3c / webappsec-credential-management

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

accessing settings object from in-parallel steps? #92

Open equalsJeffH opened 7 years ago

equalsJeffH commented 7 years ago

update 18-Nov-2021: a good portion of the problems identified by this issue were fixed with merging of PR #100, although more work remains as @mikewest notes at a high level in https://github.com/w3c/webappsec-credential-management/pull/100#pullrequestreview-180682069.

I.e. someone ought to go through the spec and find further instances of "accessing settings object from in-parallel steps" (and related problems) and fix them, then we can close this issue.

Other issues that are a part of this overall effort:

Original post:

Given @bzbarsky's https://github.com/w3c/webauthn/issues/254#issue-187399591 on webauthn, wherein he says:

[a webauthn alg step] is using "current settings object's origin" in an algorithm step that is running async and hence no concept of "current settings object". Note that async steps may also be running on separate execution threads, where settings objects, and origins, aren't even accessible. Whatever work is done with settings objects and origins needs to be done before going async.

Given the above, is there an issue with credman's Request a Credential where it is says "settings’ origin..." during steps running in parallel?

Or did the initial alg step of "Let |settings| be the [=current settings object=]" effectively copy the current settings object and thus the "in parallel" steps have access to it?

bzbarsky commented 7 years ago

You can't copy a settings object, really. In particular, getting the origin of a settings object commonly involves getting the origin of a Document, so now you're touching Documents in parallel, which is a bad idea.

equalsJeffH commented 7 years ago

Ok, thx, would the below mods appropriately fix credman's Request a Credential ?

insert new step after current step 4: Let |origin| be the [=current setting object=]'s [=environment settings object/origin=].

Alter current step 5.3.2 (which is running [=in parallel=]) to be: |origin| does not [=require user mediation=]

bzbarsky commented 7 years ago

Can whether an origin requires user mediation change? Can it change between step 4 and step 5.3.2? Can there be multiple parallel sections trying to read or write the credential store here at once?

equalsJeffH commented 7 years ago

Can whether an origin requires user mediation change? Can it change between step 4 and step 5.3.2? Can there be multiple parallel sections trying to read or write the credential store here at once?

Thx, those are good questions (that I do not have answers to), but are tangential to my inquiry, I think.

Rather, I am wondering specifically about whether there is an issue with how credman's Request a Credential refers to "settings’ origin..." during steps running in parallel, and you seemed to say "yes, that's a problem" (to paraphrase).

And am also wondering if whether the approach proposed in https://github.com/w3c/webappsec-credential-management/issues/92#issuecomment-311706352 (above) would address the issue (if there indeed is one)?

thx again.

bzbarsky commented 7 years ago

and you seemed to say "yes, that's a problem"

Right, it's definitely a problem.

It may also be a problem to get the origin's "requires user mediation" boolean in parallel.

That is, the changes suggested in https://github.com/w3c/webappsec-credential-management/issues/92#issuecomment-311706352 are necessary (to address the "getting the origin in parallel") problem, but may not be sufficient in that they do not address the "getting the 'requires user mediation' boolean in parallel" problem.

equalsJeffH commented 7 years ago

That is, the changes suggested in https://github.com/w3c/webappsec-credential-management/issues/92#issuecomment-311706352 are necessary (to address the "getting the origin in parallel") problem, but may not be sufficient in that they do not address the "getting the 'requires user mediation' boolean in parallel" problem.

Great, thanks, that's what I thought you meant but wanted to be sure.

equalsJeffH commented 7 years ago

In nosing around in credman I notice that the current settings object is accessed in a few places in algorithms that are AFAICT running in parallel. For example:

Create a FederatedCredential from FederatedCredentialInit seems to be in the same boat.

battre commented 6 years ago

I have created pull request #93 but don't know how to assign @equalsJeffH as a reviewer. I hope that I have caught everything. Please check whether this works for WebAuthN. I am passing the origin to DiscoverFromExternalSource.

equalsJeffH commented 6 years ago

@battre said:

I have created pull request #93

Thanks! am reviewing.

but don't know how to assign @equalsJeffH as a reviewer

it is probably because I am not a 'contributor' to the w3c/webappsec-credential-management repo -- might someone please add me? thx.

equalsJeffH commented 6 years ago

*bump*

but don't know how to assign @equalsJeffH as a reviewer

it is probably because I am not a 'contributor' to the w3c/webappsec-credential-management repo -- might someone please add me? thx.

pretty please?

mikewest commented 6 years ago

Done! Sorry I didn't see this earlier. :(

equalsJeffH commented 6 years ago

thanks!