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

Consider making the deriveBits length argument optional #329

Closed
twiss opened this issue Nov 1, 2022 · 10 comments · Fixed by #345
Closed

Consider making the deriveBits length argument optional #329

twiss opened this issue Nov 1, 2022 · 10 comments · Fixed by #345

Comments

@twiss
Copy link
Member

twiss commented Nov 1, 2022

For developer ergonomics, it would be nice not to have to specify the length parameter when calling deriveBits() for ECDH key derivation, as you typically want all bits anyway. Issue #322 is about making the argument nullable, which seems to have been originally intended, however, it would be even nicer to be able to leave it off entirely. This would require changing the IDL, of course - but personally I think we should do that for #322 anyway.

@martinthomson, @annevk, @saschanaz, and @panva - any thoughts?

@panva
Copy link
Member

panva commented Nov 1, 2022

I'm all for fixing the spec with #324 regardless of additional ergonomics improvement changes.

Improving ergonomics can be done but would make more sense to do across the board, not with just this one thing.

I would propose to open a thread where suggestions can be collected. I know I for sure could come up with a few.

@twiss
Copy link
Member Author

twiss commented Nov 1, 2022

I would actually prefer to keep one issue per improvement, so that we can consider and discuss them individually.

I also should note - although that may argue against this very issue as well - that the charter of the Web Application Security WG calls for "maintenance" of the Web Crypto API. In my mind, that raises the bar for such changes a bit. If we have to change the IDL in #322 anyway, I think it makes sense to make this change as well, but I'm not sure it makes sense to overhaul the API across the board. Nevertheless, feel free to open issues for any ergonomics improvements you have in mind, imo :)

@panva
Copy link
Member

panva commented Nov 1, 2022

This very issue is imho not maintenance, #324 is. Moving on with an optional argument that we need to fix first is only going to delay the fix itself. It will also require all implementers to adopt to be usable, where as the #324 fix needs no action from all but two implementors to already exhibit.

@twiss
Copy link
Member Author

twiss commented Nov 1, 2022

Yes, I agree :) This issue is meant to be addressed after #322 (if at all), not before it.

@panva
Copy link
Member

panva commented Nov 1, 2022

👍

@annevk
Copy link
Member

annevk commented Nov 1, 2022

The current proposed resolution to #324 needs action from all three browsers, no? Leaving the IDL alone would certainly result in less churn there.

@panva
Copy link
Member

panva commented Nov 1, 2022

On the surface webkit is already exhibiting the required null behaviours. So are all non-browser implementors.

Changing the IDL isn't forcing anyone to make any change, it may however prompt Blink and Gecko to fix their implementations and exhibit the null behaviours we have had WPTs for for years now.

@annevk
Copy link
Member

annevk commented Nov 1, 2022

I think it would require WebKit to change how it handles 0 as well (this would be required either way, but not counting this seems wrong).

@panva
Copy link
Member

panva commented Nov 1, 2022

#324 does not touch the 0 behaviour and fixing it (0) in webkit is inconsequential given how unimportant that behaviour is (it is also not covered by WPTs and its implementations vary even more).

@twiss
Copy link
Member Author

twiss commented Nov 1, 2022

Sorry, but can we have that discussion in #322, instead? 😅

For this issue, instead, it's obvious that all implementations would need to change. Do you think that's worth it?

(Personally I think it's a bit silly to require the , null, but it's true that changing this doesn't really fall under maintenance, so we can also just leave it as-is. Or, we can also postpone this discussion until after we land on a resolution for #322.)

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

Successfully merging a pull request may close this issue.

3 participants