Re: crypto-ISSUE-41: Clean up algorithm normalization and support checks [design for Web Crypto API]

On Apr 5, 2013, at 3:31 PM, Ryan Sleevi <sleevi@google.com> wrote:

> On Fri, Apr 5, 2013 at 12:23 PM, Richard Barnes <rbarnes@bbn.com> wrote:
>> 
>> On Apr 5, 2013, at 3:08 PM, Ryan Sleevi <sleevi@google.com> wrote:
>> 
>>> On Wed, Apr 3, 2013 at 10:03 PM, Richard Barnes <rbarnes@bbn.com> wrote:
>>>> 
>>>> On Apr 3, 2013, at 2:11 PM, Ryan Sleevi <sleevi@google.com> wrote:
>>>> 
>>>>> On Mon, Apr 1, 2013 at 8:03 AM, Web Cryptography Working Group Issue
>>>>> Tracker <sysbot+tracker@w3.org> wrote:
>>>>>> crypto-ISSUE-41: Clean up algorithm normalization and support checks [design for Web Crypto API]
>>>>>> 
>>>>>> http://www.w3.org/2012/webcrypto/track/issues/41
>>>>>> 
>>>>>> Raised by: Richard Barnes
>>>>>> On product: design for Web Crypto API
>>>>>> 
>>>>>> SUMMARY:
>>>>>> 
>>>>>> We should move checks on algorithm support to the operations (out of the normalization) and clean up language about "registered" algorithms.
>>>>>> 
>>>>>> 
>>>>>> DETAILS:
>>>>>> 
>>>>>> The "encrypt" method and others need to verify two things about algorithm identifiers:
>>>>>> (1) That the algorithm is supported by the UA
>>>>>> (2) That the algorithm supports the requested operation
>>>>>> 
>>>>>> Currently, (1) is done twice, once by algorithm normalization rules and once by the operation itself, and (2) is done in in the operation.
>>>>>> 
>>>>>> In the algorithm normalization rules:
>>>>>> """
>>>>>> If name does not contain a recognized algorithm name, throw an InvalidAlgorithmError exception and return from this algorithm.
>>>>>> """
>>>>>> 
>>>>>> In encrypt():
>>>>>> """
>>>>>> If normalizedAlgorithm does not describe a registered algorithm that supports the encrypt operation, throw a NotSupportedError and terminate the algorithm.
>>>>>> """
>>>>>> 
>>>>>> Two problems here:
>>>>>> 
>>>>>> First, checking support is not properly a part of algorithm normalization.  Whether an algorithm is supported by a UA is a separate question from whether its AlgorithmIdentifier is in a normal form.
>>>>>> 
>>>>>> Second, the check on registration status in the operation is unnecessary.  The algorithm normalization rules already check that an algorithm is "recognized" by the UA, so the only thing that needs to be checked here is the algorithms support for the operation.  UAs may also support unregistered algorithms, for experimentation or for early deployment while specification update is in process. Part of this support is knowing which operations are supported for those algorithms.
>>>>>> 
>>>>>> 
>>>>>> PROPOSAL:
>>>>>> 
>>>>>> Remove the following step from the algorithm normalization rules:
>>>>>> """
>>>>>> If name does not contain a recognized algorithm name, throw an InvalidAlgorithmError exception and return from this algorithm.
>>>>>> """
>>>>>> 
>>>>>> Make the following change to encrypt, and corresponding changes to decrypt/sign/verify/digest:
>>>>>> OLD:
>>>>>> """
>>>>>> If normalizedAlgorithm does not describe a registered algorithm that supports the encrypt operation, throw a NotSupportedError and terminate the algorithm.
>>>>>> """
>>>>>> 
>>>>>> NEW:
>>>>>> """
>>>>>> If normalizedAlgorithm does not describe a recognized algorithm, throw an AlgorithmNotSupportedError exception and terminate the algorithm.
>>>>>> 
>>>>>> If normalizedAlgorithm does not describe an algorithm that supports encryption, throw an OperationNotSupportedError exception and terminate the algorithm.
>>>>>> """
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> 
>>>>> 
>>>>> Hi Richard,
>>>>> 
>>>>> I've read this several times, and I'm still not sure I follow what
>>>>> your issue is.
>>>>> 
>>>>> It seems like you're trying to find a way to say "Look, the UA can
>>>>> support algorithms that nobody knows about and it doesn't tell anyone
>>>>> about" - is that correct?
>>>> 
>>>> Yes.  The objective is to allow UAs to support algorithms that are not registered.
>>>> 
>>>> 
>>>>>> From reading your new proposal, it seems to be doing exactly what
>>>>> you're saying you're trying NOT to do - namely, you're forcing double
>>>>> validation of the algorithm name (once during normalization, once
>>>>> during operation).
>>>>> 
>>>>> The intent was that the normalization phase addresses any/all invalid
>>>>> values, so that by definition, a 'normalized' algorithm only contains
>>>>> recognized algorithms. At that point, post-normalization, any
>>>>> operation can 'trust' that input and perform whatever secondary checks
>>>>> it needs (eg: is it supported).
>>>>> 
>>>>> It seems with your proposal that you're wanting to treat the
>>>>> normalized form as still untrusted.
>>>>> 
>>>>> Is this correct?
>>>> 
>>>> That's correct.  I was thinking that "normalization" was basically a reformatting function, not a question of support.  That led me to think that the "support" ("registered" in the current text) should be in the operation function, which is already checking for support of the specific operation.  It also seemed like there were conflicting notions of support, "recognized" vs. "registered".
>>>> 
>>>> If you would like to have the normalization do support checking as well, we could just delete the "registered" in the operation functions to clarify that there's not an additional algorithm support check going on here (since it already happened in the normalization), just an operation check.
>>>> 
>>>> So the net effect would be:
>>>> 1. No change to the algorithm normalization rules
>>>> 2. Change the operations to say change "a registered algorithm" to "an algorithm", e.g., "If normalizedAlgorithm does not describe an algorithm that supports the encrypt operation, ..."
>>>> 
>>>> I think it's slightly clearer to have all the support checking in the operations, but I could also see having a consolidated normalizeAndCheckSupport() function.
>>> 
>>> Can you explain why you think it makes more sense in the operations?
>>> 
>>> The design goal has been to separate out what information is expected
>>> to be knowable synchronously vs. what information is expected to only
>>> be resolvable asynchronously.
>> 
>> I'm not sure I understand how sync/async plays into this.  Both algorithm normalization and support checking are cheap, fast operations, and everything we're talking about takes place within the operation, which is already off the main thread.  So it seems like we're mainly talking about spec clarity and, to a lesser extent, how you modularize the code.
> 
> Support checking is two fold
> a) Support that the browser layer understands how to communicate that
> algorithm to its underlying cryptographic implementation
> b) Support within the underlying cryptographic implementation for that
> algorithm.
> 
> Algorithm Normalization handles a), because that's an immediate check.
> Operations handle b), because that's a potentially expensive check
> that may further be contingent on other parameters.
> 
> You're proposing that a) be moved to be with b). I think that's both
> unnecessary and bad for developers - we should try to fail as fast as
> possible, and there's no reason that we can't fail immediately at a.

First, the a/b distinction here is not at all in the current spec.  You seem to be assuming that (b) is in the operation right now, but the only check there is to check that the algorithm is "registered".  Does "registered" mean something different than "defined in the spec"?  

As far as moving this around, see above, "everything we're talking about takes place within the operation, which is already off the main thread."   Maybe to be clear, we should have the UA "queue a task" for the checks (2) in the below process.

--Richard



>> Having all of the checks in the operation seems to make sense because they fit into an overall sequence of checks that the UA makes before it actually goes and does crypto.  That is, it seems to me that the general form of an operation is as follows:
>> 
>> 1. Convert the algorithm to normal form [RLB: and add defaults :) ]
> 
> There has been no support for defaults - and a good deal of public
> comment (more than any other issue) suggesting otherwise. Can we put
> this issue to rest and move on?
> 
>> 2. Check that this operation is supported and permitted
>> 2.1. Is this an algorithm I support?
>> 2.2. Is this an algorithm that supports this operation?
>> 2.3. Is the algorithm well-formed?  (all required parameters present)
>> 2.4. Is this key usable with this SubtleCrypto instance?
>> 2.5. Is this algorithm supported with this key? (key.algorithm)
>> 2.6. Is this operation supported with this key? (key.usage)
>> 3. Actually do crypto processing
>> 
>> Right now, the algorithm normalization rules does (1) and (2.1).  I'm just thinking that (2.1) belongs with the other stuff in (2).  That would put it in the operation.
>> 
>> I might also be getting hung up on terminology.  To me, "normalization" is a formatting operation, not a verification operation.
>> 
>> --Richard
>

Received on Friday, 5 April 2013 22:53:55 UTC