What else is pending to merge this PR ?

As far as I understand we have reached an agreement in #322 about the use of ` optional unsigned long and the addition of ```?=null``` would avoid the need to introducing changes in the algorithm's steps.

@javifernandez since #324 was arguably merged prematurely, I wanted to make doubly sure that we have consensus on this before it's merged :)

@annevk, @martinthomson and @saschanaz, it would be great if you could confirm whether you're OK with this solution, as I think you were at one point arguing against changing the WebIDL in #322. Though note that this PR also addresses #329, and that's not possible without changing the WebIDL - but also there we haven't discussed much whether it's worth it, and in scope for the "maintenance" charter. I do personally think this would be a nice change for developer ergonomics, but arguably it's debatable whether that's in scope. Let me know what you think!

I'm not against changing Web IDL, but I still don't see the point in allowing null. The argument being optional already gives you the value space you need.

@annevk It's not so much that I think allowing null is useful for new applications (that indeed can just omit the parameter), it's rather that given the question of what to do if an existing application does pass null, returning the entire value seems safer than returning an empty Uint8Array.

It's admittedly unlikely any existing web app does so since only Safari behaves "as expected" (per this PR, not per the current spec) in that case, but perhaps not entirely impossible since the web platform tests do test for it, so someone may have gotten the idea from there (or a loose reading of the current spec), and written a Safari-only application that depends on this, perhaps.

Also note that if such code would run today on Chrome, it would silently return an empty value, potentially leading to a security issue. So this change would fix a security issue in such a web app, whereas changing Safari to return the empty value instead of the full value could actually cause a security issue instead (strictly speaking not by fault of Safari, but nevertheless it seems better to be cautious here).

I'm personally not really concerned about that. I'll defer to @martinthomson and @davidben as to whether to accept this PR as-is.

I think have a convenient way to return an untruncated value is good. Better would have been for WebCrypto to not jam Diffie-Hellman and KDFs into the same function, but I don't have a time machine. Thus, I like making it optional to mean default size.

However, the discussion in #322 (comment) suggests that undefined, not null, is the idiomatic JavaScript pattern for an optional value. Given this is not idiomatic, and given both Chromium's and the spec's long-standing behavior, I'm also not convinced by the concern that existing websites might be relying null or zero being interpreted as default.

Thus, I think idiomatic patterns should win out and we should not have the = null business. I'm also a little surprised at this being an IDL-only change... it seems we ought to have some supporting prose, but it's been a while since I'd looked carefully at the spec formulation. That does, AIUI, preserve the existing behavior of coercing null to zero, but type coercion is just deeply part of this language and trying to work around it in every function that takes a number is just going to leave a confusing, inconsistent mess.

@davidben:

and given both Chromium's and the spec's long-standing behavior

Are you concerned about changing Chromium's behavior because you're worried that someone might be relying on passing null returning an empty value? Or some other reason?

(IMHO we shouldn't be worried about "the spec's long-standing behavior" because the spec's intention on this point is debatable, as evidenced by the web platform tests.)

However, the discussion in #322 (comment) suggests that undefined, not null, is the idiomatic JavaScript pattern for an optional value.

Would you be happier if it said optional unsigned long? length = undefined, and then the spec prose said "if length is undefined or null"?

I'm also a little surprised at this being an IDL-only change... it seems we ought to have some supporting prose, but it's been a while since I'd looked carefully at the spec formulation.

The reason it's an IDL-only change is because the prose saying "if length is null" is already there.

That does, AIUI, preserve the existing behavior of coercing null to zero, but type coercion is just deeply part of this language and trying to work around it in every function that takes a number is just going to leave a confusing, inconsistent mess.

In general I would agree but I think we should make an exception for security concerns 😅

@saschanaz:

I'm not sure I understand the argument here, passing null for optional unsigned long length will behave like passing undefined, not 0.

Are you sure? https://webidl.spec.whatwg.org/#es-overloads only mentions special behavior of undefined for optional arguments, not null. See also the references in #322 (comment).

(Me quickly checking with new DOMException(null)) Ah yes, you are right, I guess I have long had a wrong understanding about optional.

I'm OK with this outcome. Like @davidben, this is a consequence of deeper structural issues, but the way this works seems fine.

I'm not 100% on the outcome for length == 0. ECDH seems to return an empty array in that case, but HKDF and PBKDF2 throw. That seems wrong, at least for ECDH.

Right, we could throw for length == 0 as well, that seems like a reasonable protection mechanism.

I'm not 100% on the outcome for length == 0. ECDH seems to return an empty array in that case, but HKDF and PBKDF2 throw. That seems wrong, at least for ECDH.

Ah, fun. I think we should treat that separately, as it's not related to how to handle the default.

I really don't like making 0 silently turn into default, but I feel less strongly about empty vs throw. TBH, I probably would have picked consistently empty over consistently throw, but shrug. I'm not opposed to trying to align that, though as it'd be a breaking change, we should get some metrics for that.

Keep in mind that, say, 1-byte or 2-byte output is also insecure, so throwing on zero is not actually meaningfully enforcing any kind of secure use of the algorithm. Indeed for ECDH, truncation at all is a bad idea. The whole output should be passed to a KDF. Given we've already gone down the truncation route, it's just a question of whether you believe "truncate to zero" is a defined operation.

If we want to throw, we should instead be seeing whether we can compatibly throw on any truncation at all.

(IMHO we shouldn't be worried about "the spec's long-standing behavior" because the spec's intention on this point is debatable, as evidenced by the web platform tests.)

We've discussed this thoroughly in #322 already, but the WPTs came long after the spec and original Chromium implementation. The spec was also originally written in large part by someone from Chromium. Whether or not the intended semantics were right (like I said, I'm extremely unhappy with how the spec handles ECDH overall), it's quite clear what the spec's intent was.

FWIW, 0 = empty seems obvious but it's a nonsensical thing to ask for, so I'm OK with throwing. A separate PR might be needed there.

Edit: I agree with David about truncation for ECDH, at any number of bytes. But I have found many uses for very short KDF outputs.

Ah, fun. I think we should treat that separately, as it's not related to how to handle the default.

Yeah, I'll make a separate PR for that.

Keep in mind that, say, 1-byte or 2-byte output is also insecure

Sure, I meant more that it could be a protection mechanism against accidentally getting an empty value instead of the full value. For example, Safari currently returns the full value when returning 0, it might be better to change that to throw rather than returning the empty value (which could theoretically cause a security issue).

If we want to throw, we should instead be seeing whether we can compatibly throw on any truncation at all.

Can we not let perfect be the enemy of good? 😅 We already know we can compatibly throw for 0, as Firefox does so, so it would seem reasonable to me to start with that.

If we want to throw, we should instead be seeing whether we can compatibly throw on any truncation at all.

Can we not let perfect be the enemy of good? 😅 We already know we can compatibly throw for 0, as Firefox does so, so it would seem reasonable to me to start with that.

The tradeoff is between trying to enforce some security criteria (which rejecting zero does a bad job of) vs having an extra case of fallibility in the function. The point of my comment isn't striving towards perfection, but that we're sacrificing continuity of semantics for a security check that doesn't actually do much. The continuity doesn't mean much, but measured against a security check that doesn't do much, this is all silly.

Since starting to throw would be a spec-incompatible change, and a breaking change for Chromium, we'd need some code to measure things. If we're measuring things anyway, we should also validate whether we can just remove this truncation behavior altogether, as it was wrong to begin with.

(I've created #351 for this)

The tradeoff is between trying to enforce some security criteria (which rejecting zero does a bad job of)

Again, the security check wouldn't be to protect web applications in general, just any code that happens to rely on WebKit returning the entire value for zero. Rejecting zero does do a good job of protecting against that 🙃

Since starting to throw would be a spec-incompatible change, and a breaking change for Chromium, we'd need some code to measure things. If we're measuring things anyway, we should also validate whether we can just remove this truncation behavior altogether, as it was wrong to begin with.

But yes, fair enough. If you do want to do some measurements that would be great, let us know what the outcome is 😊

Ah fair, yeah zero is a bit special because of this mess. :-)

@martinthomson and @saschanaz could you please explicitly approve this PR, if you do indeed approve of it (with or without #351)?

And @davidben I'll repeat this part as it was a genuine question:

Would you be happier if it said optional unsigned long? length = undefined, and then the spec prose said "if length is undefined or null"?

And perhaps if everyone else wants to weigh in on that alternative option that'd be great 😊

I really don't think that = undefined changes anything in this case.

Certainly there's no difference between = null and = undefined with prose for null. The question in my mind is whether passing in an explicit null should be interpreted as using a default.

My preference would be what was already discussed in the issue:
#322 (comment)
#322 (comment)
#322 (comment)

That is, we should just do whatever would be idiomatic on the web for an optional parameter. That seems to be plain optional unsigned long length, which I understand from the discussion to not permit null as a stand-in for an omitted value.

But if we're really set on ignoring convention for some reason, I can live with null being a stand-in. (WebCrypto is very much not a hill worth dying on. 😛) But then there should be a clear reason for it, and it doesn't seem like there is one, beyond the WPTs which were always just a mistake in the tests.

Certainly there's no difference between = null and = undefined with prose for null. The question in my mind is whether passing in an explicit null should be interpreted as using a default.

OK.

But then there should be a clear reason for it, and it doesn't seem like there is one, beyond the WPTs which were always just a mistake in the tests.

The reasons are:

• Various runtimes return the entire value for null and there's some opposition to throwing or returning an empty value: deriveBits length idl does not allow null #322 (comment)
• Returning the empty value could also cause security issues in code written for runtimes that currently return the entire value: Make deriveBits length parameter optional and nullable #345 (comment)
• So, if throwing and returning an empty value are off the table, the only option left is to return the entire value 😅

I had a thought, though, which would perhaps allow us to do this in a more idiomatic way; if your metrics (and others', if they want to check as well) confirm that supporting truncation of ECDH derived bits is not required for web compatibility. In that case, we could make it optional unsigned long length, but rather than throwing for truncating values, we could just ignore the length parameter entirely for ECDH, and return the entire value no matter what, including for null, 0 (which WebKit already does), and any other value. (This would also imply not merging #351.)

We might need to keep support for truncation in deriveKey() (e.g. if someone is deriving an AES-256 key using P-521, although it's a bad idea of course), so this would probably cause some ugliness in the deriveBits() method, as we'd have to overwrite the length with null in the case of ECDH only (and X25519 and X448, once those are added), but perhaps it's not unreasonable to have a special case for ECDH as it's indeed different than the other algorithms, as you've noted.

However, since this would require waiting for metrics (and ideally for https://github.com/WICG/webcrypto-secure-curves/ to be merged into this spec, as otherwise it would require monkey patching this text there), and would generally be a larger change than this PR, I would prefer to still merge this PR now (to resolve the ambiguity for null and make it clear that we should return the entire value there, and to make the parameter optional), and then hopefully in the future we can also return the entire value for other values of length too, and make the WebIDL more idiomatic again. Does that sound like a reasonable plan?

@davidben it'd be great if you could rubber-stamp this PR explicitly, as @martinthomson did, to avoid confusions. I think we have reached an agreement, or we are close to at least, so I wouldn't like to let too much time pass.

@annevk If I've understood it correctly you would defer on martin's opinion, but it'd be useful of you could approve the PR explicitly as well.

Thanks everybody involved on this complex discussion.

Once Martin and David approve, I'm happy to join in. My reading of David's latest comment was he still had some reservations.

I'd like to reactivate this PR, so that we can solve the ambiguity of null and undefined, meanwhile we wait for the Chrome's metric on the usage of the '0' value. @davidben do you think we could merge this as a first step ?