Re: Spec for CryptoKey.algorithm and CryptoKey.usages doesn't really make sense

On Sat, Jul 12, 2014 at 10:06 PM, Boris Zbarsky <bzbarsky@mit.edu> wrote:

> On 7/13/14, 12:10 AM, Ryan Sleevi wrote:
>
>> That is the definition of reflects - i.e. what immediately follows the
>> comma.
>>
>
> The part that follows the comma doesn't actually define the behavior
> sanely.
>
> I mean, for the [[algorithm]] case it's talking about "converting the
> interface" but there is no interface involved.


It would help if you could clarify how you arrived at that, so that we can
try to find language to clarify.

For example, all of the places that dictate how [[algorithm]] gets a value
do so in the specific context of an interface.

For example, the first instance of an algorithm-specific operation,
RSA-SSA's generateKey

https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#rsassa-pkcs1-operations

4. Let *algorithm* be a new RsaHashedKeyAlgorithm
...
11. Set the [[algorithm]] internal slot of *publicKey* to *algorithm*.
...
16. Set the [[algorithm]] internal slot to *privateKey* to *algorithm*.

So in the case of an RSA-SSA key, the internal slot is an IDL object (a
dictionary) that is of type RsaHashedKeyAlgorithm.

When the *algorithm* attribute of privateKey/publicKey is accessed,
[[algorithm]] goes through the WebIDL conversion steps to convert a
dictionary to an ES object.


>
>  1 - you can't modify what usages returns (short of overriding the getter)
>>
>
> Can't modify in what sense?  Can't modify the resulting object?  Why not,
> exactly?


In the same way that a DOMString getter returns new instances of the
DOMString.

As for why you can't modify the resulting object, how does that differ from
http://heycam.github.io/webidl/#es-attributes , which notes "Note that
attempting to assign to a property corresponding to a read only attribute
results in different behavior depending on whether the script is doing so
in strict mode. When in strict mode, such an assignment will result in a
TypeError being thrown. When not in strict mode, *the assignment attempt
will be ignored*." (emphasis mine)


>
>
>  If you still feel it is ambiguous (and this was language that seemed to
>> satisfy the TAG review
>>
>
> I can only assume people didn't read this very carefully.  :(


Inevitably


> I can't provide sample language, because I have no idea what behavior you
> want!
>
> But let's assume that [[algorithm]] is supposed to store a Web IDL
> dictionary.  Then you should probably say something like this:
>
> The [[algorithm]] internal slot stores one of the dictionaries defined by
> this specification that inherit from KeyAlgorithm.  The exact type of
> dictionary stored depends on the value in the [[type]] internal slot (note:
> assuming it does, of course).
>

[[algorithm]] varies... depending on the type of algorithm. This seems like
it's entirely up to the implementation to track the exact type, in as much
as the individual sections for creating a CryptoKey detail the value stored
within [[algorithm]] (e.g. generateKey, importKey, etc)


>  Why is that desirable behaviour?
>>
>
> Why is what desirable behavior?  We want two things to be true:
>
> 1) foo.bar == foo.bar


> tests true for our objects.  That means that a getter can't just return a
> new object every single time it's called.  This is why dictionaries aren't
> allowed as values of attributes in IDL, because they _do_ create a new
> object each time.  So if you want to return a dictionary, you want to
> convert it to an object and then cache that object somewhere.
>

Can you point to any explanation as to the "why"? I don't say so glibly,
it's just that multiple people from different organizations have clearly
different views on the "right" way to do things, and whether or not
something is really "desired".

For example, I could not find discussion that the rationale for not
exposing dictionaries is because they create new objects, and separately
heard requests that it does create new objects (from non-Googlers, to be
clear)


>
> 2) Within the constraint imposed by #1, the behavior is defined sanely.
>
> I'm quite sure whatever recommendations you got from the TAG did not
> include violating constraint #1.  It's pretty fundamental to how getters
> should behave: they should be idempotent unless something changes internal
> state between the two getter invocations.
>
> Within that constraint, you have several options:
>
> A) Return an instance of some interface.  Some people would argue that's
> the right thing here (e.g. see http://lists.w3.org/Archives/
> Public/public-script-coord/2014JanMar/0201.html) but that depends on what
> properties the return value should have.
>
> B) Return an object produced from a Web IDL dictionary, accept that a web
> page can modify it (but not your internal state!) and thus somewhat shoot
> itself in the foot, in that what it sees from that point on via the getter
> will not match the internal state.  This is the option Domenic Denicola
> would advocate, I suspect,
>
> C) Return an object produced from a Web IDL dictionary, freeze it before
> returning it.  This is in some ways a stronger readonlyness than option A,
> since it means no expandos.
>
> There are probably other variants of (C) involving sealing instead of
> freezing and setting the attributes you youself define as non-writable.


Since Web IDL declares read-only in terms of the absence of a setter (and a
possible TypeError), it seems that B is the only option that matches Web
IDL.


> As currently written, they store IDL objects
>>
>
> As currently defined, nothing says what they store.


Please qualify. As with the above example of RSA-SSA, this is documented.
If you feel it should be documented elsewhere as well, it would be good to
note.


>  But OK, storing IDL objects here is good.  Then we just need to define
> what the getters do, and make sure that everything interacting with these
> internal slots is clear on them storing IDL objects.  For example, some of
> the spec prose talks about [[usages]] being an array, which makes people
> think that it's supposed to be an IDL array (which it's not) or a JS array
> (which it's also not).  It's meant to be a sequence, as far as I can tell.
>
>
[[usages]] is supposed to be an IDL sequence, for the same logic above as
[[algorithm]] with respect to algorithm-specific construction.

That said, raised https://www.w3.org/Bugs/Public/show_bug.cgi?id=26331 on
the language to make this clearer, since the "usage intersection" language
makes this ambiguous.


>
>  Structured Clone doesn't seem incompatible with this, given that
>> structured clone algorithm itself makes use of such terminology.
>>
>
> Structured clone is not incompatible with anything; just have to define
> what it does on our members.
>
>
>  (ImageData, File, and Blob are three objects that play particularly fast
>> and lose with details, and they're the prime role models for this spec)
>>
>
> ImageData is perfectly well defined.  Structured cloning of ImageData
> explicitly creates a new one that has the same width and height and then
> structured clones the Uint8ClampedArray.  Structured cloning of
> Uint8ClampedArray is also clearly defined, so there is no problem.
>

No, it's really not, at least not by the same criteria you're holding Web
Crypto to. Which is fine, I suppose - as long as we acknowledge that no one
is really doing what you want.

In this case, ImageData.width, .height, and .resolution are all read-only
attributes. In the same way that CryptoKey.algorithm is read-only -
although perhaps even less.

That is, as far as I can tell, the .height attribute is created by a
defineOwnProperty(P, descriptor{ [[value]]: V, [[Writable]]: true,
[[Enumerable]]: true, [[Configurable]]: true}). Can't someone then replace
this property (or the default setter associated), and define some custom
steps for setting width/height? That is, your "B" option above.

It's under-specified how structured clone for the ImageData will work in
this respect. That is, rather than describing "internal properties" (as we
have), it seems to work on the exposed properties (i.e. the result of
calling Get(P) on the ImageData object), suggesting that the calling script
can replace the values and have them serialized as such.

Now, we can argue that's not a reasonable read of the spec, or not the
intent, and I agree. And however ImageData (and friends) solve this issue,
or if it's already solved through some clever reading of the spec that I've
missed, then this is what I'd like Web Crypto to do.

But it doesn't seem like there is a clear consensus yet on the answer to
this?


>
> The File/Blob spec is ... <sigh>.  Let's not copy it's problems.
>
>
So, while I'm sure these comments are borne out of genuine frustration and
real issues, it doesn't really help, and highlights what I mentioned above,
which is that various specs are in various degrees of being 'in vogue', or
that different people perceive different answers.

In an ideal world, Web Crypto will avoid all of these landmines. But I
tried to read the open issue trackers of most of the WebApps WG specs, read
through the history and discussion, when trying to come up with *this*
language, and none of these issues seemed to be presented. So naturally, on
the Web Crypto side, it's equally frustrating that there's no "right way"
to do this.