Issue with `normalizeAlgorithm()` universally uppercasing algorithm names

[Billyzou0741326]

The comment here isn't true for RSASSA-PKCS1-v1_5 (note the lower case v in the canonical algorithm name):

https://github.com/grafana/xk6-webcrypto/blob/f68083cf1c7c8565e8d742f6f43034eac788dcd4/webcrypto/algorithm.go#L148-L151

RSASSA-PKCS1-v1_5 isn't currently supported, so there's not necessarily a problem with the current implementation.

But nonetheless, seeing how there are usages of normalizeAlgorithm() in validating against user input (in SubtleCrypto.Encrypt and SubtleCrypto.Decrypt) as well as selecting the right SignerVerifier, KeyGenerator, KeyImporter, etc (as I'm typing this out, I realized it's used pretty much everywhere), it's definitely an error to uppercase the algorithm name in the case of RSASSA-PKCS1-v1_5 in those use cases.

To keep the current behavior backward compatible while paving the way for future RSA implementation, perhaps it's good to make a special case for RSASSA-PKCS1-v1_5 in normalizeAlgorithm()?

Edit: I'm bringing this issue up because I have a use case of signing & verifying with RSA, and am looking to close the gap in the RSA implementation.

Edit 2: The change that would fix this is as simple as:

    // Algorithm identifiers are always upper cased.
    // A registered algorithm provided in lower case format, should
    // be considered valid.
    algorithm.Name = strings.ToUpper(algorithm.Name)
    // Except for RSASSA-PKCS1-v1_5, it's not upper cased.
    if algorithm.Name == strings.ToUpper(RSASsaPkcs1v15) {
        algorithm.Name = RSASsaPkcs1v15
    }

This keeps the backward compatibility of testing upper case for all algorithm names, while still using the correct canonical algorithm name for RSASSA-PKCS1-v1_5. Thoughts on this approach?

[mstoykov]

Hi @Billyzou0741326 , thank you for reporting this 🙇

Checking this it seems you are correct and that the correct way to test for this is actually to have a list of supported algorithms and their canonical names that we match against case insensitively.

Your proposed solution also will work for this case, but I wonder if this doesn't happen in other places as well. Maybe just updating it in normalizeAlgorithm is enough 🤷 .

Given the fact this algorithm isn't currently supported I don't know whether we should fix it now as part of its implementation. But I guess we can at least apply this smaller patch

cc @olegbespalov @oleiade Are there any plans on working in RSA soon ™ ?

[Billyzou0741326]

I could be wrong, but I think as far as algorithm name matching, it's mostly done up front in normalizedAlgorithm(), and the result of that is passed down to be used elsewhere. So, having the patch applied in normalizedAlgorithm() should be sufficient.

Having done some preliminary testing on rsa implementation (mostly for my immediate needs), the small snippet of patch that I mentioned above should solve the issue for RSASSA-PKCS1-v1_5 which again, is only relevant if the implementation is there.

Let me know if the patch is worth applying. If so, I can send a PR

[olegbespalov]

I have a local branch where I faced the same issue, and the current way of fixing it (I haven't included Webapi tests yet) is also just making an exception. I don't have a strong opinion, but wouldn't it make sense to fix this without implementing the support of RSA (which is currently missing)?

[oleiade]

I'm okay with introducing a custom behavior for it indeed 👍🏻

Not a strong opinion: we should probably wait and do that in the RSA implementation though?

For reference, the implementation of the algorithm normalization was particularly painful at the time, and the uppercase constraint was probably a small shortcut I took at the time, that makes no sense in hindsight 🙇🏻

[Billyzou0741326]

Indeed. I was bringing this up mainly because I ran into it during the implementation of that particular algorithm. Agreed that it makes sense to make the change only when it's needed (e.g. when RSA is eventually implemented).

[Billyzou0741326]

Another remark about a different alternative to making an exception for RSASSA-PKCS1-v1_5, is to incorporate something similar to https://pkg.go.dev/net/http#Header, where the matching of http header names should be case-insensitive.

Something like:

type AlgorithmName map[string]string

func (h AlgorithmName) Get(key string) string

The only difference is that, instead of mapping from http header name to http header values, the map would map from algorithm name (case insensitive) to its canonical form.

Just pointing out another option - kind of echoing back with @mstoykov mentioned:

to have a list of supported algorithms and their canonical names that we match against case insensitively