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

Remove global mutable SigningMethod registry #419

Open
ash2k opened this issue Nov 21, 2024 · 0 comments
Open

Remove global mutable SigningMethod registry #419

ash2k opened this issue Nov 21, 2024 · 0 comments

Comments

@ash2k
Copy link

ash2k commented Nov 21, 2024

In v6 (#335) I suggest to remove the global mutable SigningMethod registry. Instead of it we could have a global immutable registry for built-in signing methods and WithSigningMethods(...SigningMethod) option for the Parser (i.e. ParserOption).

The existing WithValidMethods() can be removed too. If it's not set then any registered method is accepted. The behavior remains the same - if you want to use certain signing methods only, you need to specify them using the proposed WithSigningMethods(). The benefits are:

  • no need to register a custom singing method in the registry. Instead it is specified per-Parser.
  • no chance of accidentally changing the behavior (making it less strict - not good!) of parsers that don't have WithValidMethods() specified.
  • Less fragile code. You could depend on a signing method being registered in some other package "far away" in the program from where you use it. If it stops being registered over there, you get a subtle breakage over here - "benefits" of global mutable state. Third-party libraries can change behavior of your program when you import their packages.
  • RegisterSigningMethod() doesn't check if it's overwriting an existing registration. This can lead to undetected "bad" implementations being substituted in place of the good ones. A third party package can mess with standard registered implementations.
  • etc, etc, etc - all the drawbacks of global mutable state.

One extra benefit: if you have many implementation that are just types (e.g. structs) but they are not used anywhere - they will be trimmed from the final binary by the linker. This does not happen when you have an init() { RegisterSigningMethod(MyImpl{}) } because the type is referenced and an instance is retained in the global map. Result is more bloated binary and bigger RAM footprint.


With WithSigningMethods(...SigningMethod) parser configuration is localized and nothing can change it. The only way to change it is to actually reconfigure the parser explicitly where it is constructed. I think this is a really big deal for security-sensitive code, such as what this library is for.


Some background. I'm trying to use HMAC + SHA3 512. I have to register the custom-made signing method implementation, ensure that the package with the registration is imported where I use the signing method, specify WithValidMethods("MY ID OF THE METHOD") when constructing the parser. This is so much hassle and brittleness vs just using WithSigningMethods(MySHA3HMAC{}).

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

No branches or pull requests

1 participant