webcrypto: Throw exception on saltLength: 0 for RSA PSS sign/verify

[olegbespalov]

What?

While implementing support of RSA grafana/xk6-webcrypto#85 we've found that standard Golang's SDK doesn't provide support of the saltLength: 0, instead it always tries to generate salt with the maximum length.

This led us for the patch to WebPlatfrom tests:
https://github.com/grafana/xk6-webcrypto/blob/main/webcrypto/tests/wpt-patches/WebCryptoAPI__sign_verify__rsa.js.patch

Since some use cases could actually try to use the lengths of salt 0 we need to issue a log warning user that the actual behavior of our implementation is different.

edit(@mstoykov): After some discussion the idea is currently to throw an exception instead of just log a warning.

Why?

It improves UX and makes implementation predictable.

[vincentandreas]

hi @olegbespalov I'm interested to contribute this, but, I need time to understanding and implement this. is it okay? maybe around 2 weeks

[olegbespalov]

Hey @vincentandreas ! Sure, it's totally fine 👍

[mstoykov]

Looking at the PR I wonder if we shouldn't actually throw an exception - we are not doing what is expected, so us just logging a warning seems strange. Especially with stuff such as crypto this will have completely different results - right?

So it seems like we either have to fix it or throw an exception and maybe fix it later. Given that fixing it likely will be quite involved, I propose we just throw an exception.

Do we have any idea how common this behaviour is? Is this like a thing that is there just because it can be or is something that is used?

cc @grafana/k6-core

[oleiade]

@mstoykov I have no hard facts around it, but my main intuition as to why one would want 0-length salt is for testing purposes, and keeping the output as deterministic as possible. But I have no certainties that's the reason.

Not a strong opinion: my preference would go to throwing an exception as it clearly indicates that's both something we're unable to achieve, and it's also something that users should probably be kept from doing, until there's a hard requirement for it.

[inancgumus]

+1 for throwing an exception. It's also worth documenting this fact in our API docs.

[mstoykov]

There seems to be aggreement on this, so please @vincentandreas change the code to throw an exception in those cases and please add a test that this is what happens.

edit: I have edited the issue