Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should AlgorithmIdentifier be (Algorithm or DOMString)? #161

Closed
foolip opened this issue Oct 13, 2016 · 14 comments
Closed

Should AlgorithmIdentifier be (Algorithm or DOMString)? #161

foolip opened this issue Oct 13, 2016 · 14 comments
Labels

Comments

@foolip
Copy link
Member

foolip commented Oct 13, 2016

https://w3c.github.io/webcrypto/Overview.html#algorithm-dictionary

It is currently defined as (object or DOMString) with prose to handle it in https://w3c.github.io/webcrypto/Overview.html#dfn-normalize-an-algorithm

Because of "If an error occurred, return the error and terminate this algorithm" and every use of "normalizing an algorithm" is followed by "If an error occurred, return a Promise rejected with something" it looks like letting this be handled by WebIDL would be equivalent.

The trouble is that in w3c/webrtc-pc#499 AlgorithmIdentifier is used as a return value, and if the type is (object or DOMString) there would have to be prose saying whether it's the same object that passed to RTCPeerConnection.generateCertificate({ name: "something" }) or a newly created object. When using a dictionary as a return type this is unambiguous.

@ericroman920
Copy link

ericroman920 commented Oct 18, 2016

Correct, AlgorithmIdentifier cannot be used as a return type -- my suggestion for WebRTC is to define the return type as an "object".

The Web Crypto spec has the method for "normalizing an algorithm":
https://w3c.github.io/webcrypto/Overview.html#algorithm-normalization-normalize-an-algorithm

The procedure of normalizing an algorithm necessarily returns a new object, since it may involve copying bytes from BufferSources, or changing the case on algorithm names to match the case from registration.

So from this alone, I think the answer should unambiguously be that the result is a new "object", since that is what normalizing an algorithm returns.

Looking further for precedence:

The Web Crypto spec itself doesn't reflect back the passed in algorithms, except in the case of CryptoKey.algorithm -- https://w3c.github.io/webcrypto/Overview.html#dfn-CryptoKey

This is not exactly an Algorithm, but rather it is a KeyAlgorithm --https://w3c.github.io/webcrypto/Overview.html#dfn-KeyAlgorithm -- but for this analogy I think the distinctions aren't terribly important.

In the case of CryptoKey.algorithm, the spec says to create a new instance of KeyAlgorithm when assigning to it. For instance in the "import" steps for "raw" format for ECDH --https://w3c.github.io/webcrypto/Overview.html#ecdh-operations -- the spec indicates that [[algorithm]] slot is assigned a new EcKeyAlgorithm.

@foolip
Copy link
Member Author

foolip commented Oct 24, 2016

I opened this issue not only to ask about w3c/webrtc-pc#499, but also to ask why (Algorithm or DOMString) cannot be used in Web Crypto, where it's an argument type, not the type of a return value or attribute.

@mwatson2
Copy link
Collaborator

The issue is that if we define it as (Algorithm or DOMString) then WebIDL type conversion will remove any members that are not in the base Algorithm dictionary. However, many algorithms define and expect a different dictionary derived from Algorithm. Hence the steps in the normalizing an algorithm algorithm which explicitly perform WebIDL type conversion using the correct derived Dictionary type.

@mwatson2
Copy link
Collaborator

I suppose one could address this differently in the prose by saying that we should "repeat the WebIDL type conversion using the original ECMAScript value of the algorithm parameter and the xxx Dictionary type", but then the need to retain the original value past the original WebIDL type conversion (based on parameter type) is not clear until you read through the normalization algorithm.

@mwatson2
Copy link
Collaborator

@foolip Please reopen if you feel this is still a problem.

@foolip
Copy link
Member Author

foolip commented Oct 24, 2016

I can't reopen, but why can't each method take a dictionary that has all the possible members?

@mwatson2
Copy link
Collaborator

Because the members depend on the algorithm and the methods are supposed to be independent of the algorithms. That is, you should be able to add new algorithms without changing the method signatures or procedures.

@foolip
Copy link
Member Author

foolip commented Oct 24, 2016

Why is that split helpful, did it make implementations simpler? Both the algorithms and the IDL would be in the same repository, so it can't be that they really change independently of each other.

@mwatson2
Copy link
Collaborator

From a specification point of view, new algorithms can be specified without touching this specification (i.e. we have extension points to do this without monkey-patching).

@foolip
Copy link
Member Author

foolip commented Oct 24, 2016

What are those extension points used for, and couldn't other specs simply extend the relevant dictionaries? w3c/webrtc-pc#880 is trying to use IDL, so if that's a bad idea, please comment.

Unless there's something that really can't be expressed using IDL dictionaries, this all looks a bit mysterious, I'd really like to understand how it came about.

@foolip
Copy link
Member Author

foolip commented Oct 27, 2016

Ping @ericroman920 and @mwatson2, I'd like to understand this, and I'm pretty sure w3c/webrtc-pc#880 (now merged) isn't how you intended other specs to extend things.

@foolip
Copy link
Member Author

foolip commented Oct 27, 2016

Hmm, so I've looked a little bit at the implementation in Blink. Is it that depending on the name member, the input should then be treated as one of various IDL dictionaries, and the union of those dictionaries wouldn't make sense, or at least require expressing much of the requirements in prose? In particular the various length members are of different type and only some are required.

@mwatson2
Copy link
Collaborator

@foolip Right, the full type of the dictionary depends on the value of the name member and the operation being performed. The type should always be derived from Algorithm. And we have an extension point which allows other specifications to add new name values and their associated Algorithm dictionary types without modifying the method IDL.

Arguably, handling this cleanly is a missing feature in WebIDL. This was discussed at the WebIDL breakout at TPAC, but I don't find any minutes for that discussion.

It's a problem anywhere you need to specify a WebCrypto AlgorithmIdentifier because depending on the purpose and the actual algorithm you will need different members in there and if you specify AlgorithmIdentifier in your IDL those members will be stripped out (according to standard WebIDL type conversion).

One solution would be a WebIDL extension to mark a dictionary type as "extensible", such that WebIDL type conversion performs type checking and conversion only on the members specified in the base dictionary type but leaves the overall thing as an object, retaining the other members.

Another solution would have been for AlgorithmIdentifier to contain name and a params object containing the parameters. But it's a bit late for that now (although I vaguely recall it might have been like that in the very beginning).

@foolip
Copy link
Member Author

foolip commented Nov 9, 2016

Thanks @mwatson2, I think I see how this all came about now. Do you know if there are WebIDL issues filed for these ideas?

https://heycam.github.io/webidl/#idl-record was added with whatwg/webidl#180 and is somewhat related. They behave similar to dictionaries with the "always passed by value", but you'd still have to do all of the type checking in prose.

I am OK with closing this issue if it's cluttering the bug list, or leaving it open to reconsider when/if WebIDL gets the support you need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants