feat: support storing hashed client secrets

[strowk]

Adds a support to automatically hash client secrets before storing provided that the underlying store implements new SavingClientStore interface. Client store wrapper supporting hashing returns client info wrapped into structure that implements ClientPasswordVerifier using provided hashing algo for verifying. Hasher interface is added to allow any hashing algorythm to be used instead of default bcrypt.

[drognisep]

TL;DR: Given all of the existing constraints described below, what you're expressing with your change is probably the best possible implementation given all of the existing constraints of the go-oauth2 library. There are a couple things I'd recommend:

• It might be a good idea to change the name of the Save method to something like PersistClientInfo. It's kind of confusing at first glance seeing both Set and Save as options on store.ClientStore.
  • Related to this, if Save is changing, then the name of the SavingClientStore interface may also need to change.
• I'd recommend expanding the documentation on the SavingClientStore interface a bit to explain why the interface exists, at the very least to highlight the hidden coupling in the current version of go-oauth2.
• I think the maintainers should consider a consistent write specification for any given ClientStore in the future (either using what you have defined, or something that works for most ClientStore providers), because this still necessitates further wrapping of ClientStore implementations that are not compliant with SavingClientStore, or an "opt-in" approach to being consistently writable, which is of course a leaky abstraction.
  • Also, providing additional "handlers" for Manager verification of ClientInfo would be a welcome change.

Based on the interfaces defined for any client store, it looks like it's intended to be read-only from go-oauth2's perspective. However, expressing a consistent policy of hashing secrets - a very common requirement - is something I would expect to be able to do at the go-oauth2 level, regardless of store implementation.

Looking at the structure expressed in model.go, it seems like the ClientInfo interface is intended as a simple data transmission format, so it makes sense why the interface doesn't define a means to verify itself. That is instead the role of the ClientPasswordVerifier.

Unfortunately, the way that the library treats ClientInfo returned from a ClientStore both defines these functional areas as separate concepts and conflates them in the context of verification, as seen in the Manager's verification flow, without allowing for an extension point for global policies. That means that there's some hidden coupling between the storage concerns of ClientInfo and the verification concerns of ClientPasswordVerifier.

A better approach might be to really separate those concerns in the Manager, so that storage implementations aren't required to address this hidden coupling, but that's likely a breaking change.

Another approach that is similar to what you're expressing is to proxy calls to a wrapped ClientStore such that Set calls result in the provided secret being hashed, while reads return a boxed ClientInfo that is able to verify itself using the Hasher provided to the ClientStore wrapper. The problem with that is the ClientStore interface doesn't define any consistent write method signature, so there's really no way that a pure wrapper could predict what the write call could look like. I'm assuming that's probably why you defined the SavingClientStore interface. This missing method of ClientStore means that go-oauth2 is leaving all persistence and verification details to the implementors, which takes the ability to express consistent policy away from the caller. Not the right choice in my opinion.