jws.UnregisterSigner does not remove the underlying signer from the global signers map

[penkovski]

Describe the bug

jws.UnregisterSigner does not remove the underlying signer implementation from the global signers map.
It removes the jws.SignerFactory, but not the underlying jws.Signer responsible for a given registered algorithm.
We can see it in the following code snippet from UnregisterSigner in jsw/signer.go, where only the SignerFactory
is deleted.

func UnregisterSigner(alg jwa.SignatureAlgorithm) {
	muSignerDB.Lock()
	delete(signerDB, alg)
	muSignerDB.Unlock()
}

The problem is that if you call UnregisterSigner and UnregisterSignatureAlgorithm, and then you want to register a different signer implementation for the same algorithm with RegisterSigner and RegisterSignatureAlgorithm, under the hood you will still use the previous signer implementation, because unregister does not remove it, and when you register again, it ignores the newly provided signer implementation by the new factory.

This happens because makeSigner does not use the algorithm factory to create a signer object, as it checks that the signers global map already contains an object registered for that algorithm.

The fix is very simple and is just one line of code: delete(signers, alg)

func UnregisterSigner(alg jwa.SignatureAlgorithm) {
	muSignerDB.Lock()
	delete(signerDB, alg)
+	delete(signers, alg)
	muSignerDB.Unlock()
}

Why are we having a problem with that and I'm wondering how nobody has had that problem before?

We use an external signing service and we register a custom signing algorithm with a specific implementation which under the hood calls another service (Hashicorp Vault) to produce the signature. For that we've created a small adapter to align the API client with the jws.Signer interface. Everything works correctly, until we have to write unit tests, where the signer implementation is a mock object, and is different for every test case.

When we run go test for multiple test cases where each case does UnregisterSigner/RegisterSigner to provide it's own mock implementation in order to test the different external responses, it breaks because the signer implementation remains the same for all tests.

Please attach the output of go version

go version go1.21.4 darwin/arm64

Expected behavior

I would expect that when you UnregisterSigner and then RegisterSigner again, the newly registered signer to be used for signing, and not a previous leftover one.

[lestrrat]

Why are we having a problem with that and I'm wondering how nobody has had that problem before?

The sad truth is that people ask for features that the developers don't really use, they are asked for feedback for an implementation and they just kind of handwave it without much testing.

So yeah, this probably got overlooked because nobody was registering/unregistering multiple times in a program.

Anyways, please check #1017 and let me know if this fixes the problem for you.

[penkovski]

Wow, that was very fast! Thank you for the great library, the fix looks good to me and resolves the issue.

[lestrrat]

Cool, I'll merge the fix after some cleaning up, but for the time being, please reference the commit hash as we'd like to wait for a few days before releasing fixes and wait for the dust to settle.

[lestrrat]

fixed by #1017