Need an easier way to add password validation logic

[drognisep]

Maybe I'm missing something, but with the latest version the only way I've found to add password verification logic and to avoid storing client secrets in the clear is to wrap the client store and return a different representation that implements ClientPasswordVerifier for calls to GetByID.

I feel like there should be an easier way to do this. Since the expected return value of the store's GetByID method is an interface, couldn't a config option be added to provide a store-wide verifier function and return a ClientInfo impl struct that uses it?

This is also complicated by the fact that the data column seems to be what is actually populating values. I'm not sure why that duplication is necessary when the column data could simply be copied over instead of introducing the reflection overhead of the JSON decoder.

All in all, I wish it were easier to do the "right" thing and not store secrets in the clear.

[vgarvardt]

Hi, @drognisep.

I did not work with this library for a while and not fully understand what do you mean. Can you please elaborate? Is it something postgres-backend related?

[drognisep]

Sure, I can elaborate, @vgarvardt.

Model initialization

The default structure for client storage is a table with what would appear to be meta columns with very similar details as what is stored in the data column as JSONB. Here's the CREATE TABLE template.

func (s *ClientStore) initTable() error {
	return s.adapter.Exec(context.Background(), fmt.Sprintf(`
CREATE TABLE IF NOT EXISTS %[1]s (
  id     TEXT  NOT NULL,
  secret TEXT  NOT NULL,
  domain TEXT  NOT NULL,
  data   JSONB NOT NULL,
  CONSTRAINT %[1]s_pkey PRIMARY KEY (id)
);
`, s.tableName))
}

There is a helper function which converts JSON []bytes to a Client struct which contains all of the meta columns with the addition of UserID.

One question above is why the JSONB type is needed in this case since that column could be replaced with a column to represent UserID and eliminate the reflection overhead associated with the stdlib json package. I could see JSONB being more flexible if the schema changes, but the table definition could also just be migrated to the new version by the end user.

Client store

Here's how I'm setting up the adapter, token store, and client store.

	pgUrl := fmt.Sprintf("postgres://%s:%s@%s:%d/%s", util.PgUserVar, util.PgPassVar, util.PgHostVar, util.PgPortVar, util.PgDbVar)
	pgxConn, err := pgx.Connect(ctx, pgUrl)
	if err != nil {
		log.Fatalf("Failed to establish DB connection: %v\n", err)
	}
	manager := manage.NewDefaultManager()

	adapter := pgx4adapter.NewConn(pgxConn)
	tokenStore, err := pg.NewTokenStore(adapter, pg.WithTokenStoreGCInterval(time.Minute))
	if err != nil {
		log.Fatalf("Failed to init token store: %v\n", err)
	}
	defer tokenStore.Close()

	clientStore, err := pg.NewClientStore(adapter)
	if err != nil {
		log.Fatalf("Failed to init client store: %v\n", err)
	}

	manager.MapTokenStorage(tokenStore)
	manager.MapClientStorage(security.NewClientStoreWrapper(clientStore))

Note that your client store's GetByID returns an implementation of oauth2.ClientInfo to satisfy the interface, but there's no way to specify anything other than plain text storage with the provided options. There's also no real point in trying to provide a custom option since that logic is hard coded and nothing in the returned ClientStore is actually accessible.

To provide non-plain-text storage logic, the returned oauth2.ClientInfo must also implement oauth2.ClientPasswordVerifier with a simple function to translate what's given from the storage layer to a representation that can be compared 1:1 to the parameter passed from the web layer.

There's no way to provide such a function or to specify that plain text storage should not be used based on what I've seen in your code.

Workaround

To work around this limitation I basically had to deep dive into the internals of this lib, understand how the result of GetByID is coming back and wrap your result in a value that implements oauth2.ClientPasswordVerifier just to add the translation/verification function. This seems like a lot more work than should be necessary to ensure secrets aren't stored in the clear.

package security

import (
	"context"

	"github.com/go-oauth2/oauth2/v4"
	"github.com/go-oauth2/oauth2/v4/errors"
	pg "github.com/vgarvardt/go-oauth2-pg/v4"
	"golang.org/x/crypto/bcrypt"
)

var _ oauth2.ClientInfo = (*VerifyingClientInfo)(nil)
var _ oauth2.ClientPasswordVerifier = (*VerifyingClientInfo)(nil)

type VerifyingClientInfo struct {
	ID     string
	Secret string
	Domain string
	UserID string
}

func NewVerifyingClientInfo(info oauth2.ClientInfo) *VerifyingClientInfo {
	if info == nil {
		return nil
	}
	return &VerifyingClientInfo{
		ID:     info.GetID(),
		Secret: info.GetSecret(),
		Domain: info.GetDomain(),
		UserID: info.GetUserID(),
	}
}

func (v *VerifyingClientInfo) VerifyPassword(pass string) bool {
	if pass == "" {
		return false
	}
	err := bcrypt.CompareHashAndPassword([]byte(v.Secret), []byte(pass))
	return err == nil
}

func (v *VerifyingClientInfo) GetID() string {
	return v.ID
}

func (v *VerifyingClientInfo) GetSecret() string {
	return v.Secret
}

func (v *VerifyingClientInfo) GetDomain() string {
	return v.Domain
}

func (v *VerifyingClientInfo) GetUserID() string {
	return v.UserID
}

var _ oauth2.ClientStore = (*ClientStoreWrapper)(nil)

type ClientStoreWrapper struct {
	*pg.ClientStore
}

func NewClientStoreWrapper(store *pg.ClientStore) *ClientStoreWrapper {
	return &ClientStoreWrapper{
		ClientStore: store,
	}
}

func (w *ClientStoreWrapper) GetByID(ctx context.Context, id string) (oauth2.ClientInfo, error) {
	info, err := w.ClientStore.GetByID(ctx, id)
	if err != nil {
		return nil, err
	}
	rval := NewVerifyingClientInfo(info)
	if rval == nil {
		return nil, errors.ErrInvalidClient
	}
	return rval, nil
}

Possible Solution

The result of GetByID is an interface, which means you could return one implementing type or potentially many, depending on the options passed to NewClientStore. Here's what I would do if I had time to tackle this.

1. Add a field in your client store representing a function that would satisfy oauth2.ClientPasswordVerifier.
2. Add a new option that takes an implementing function and stores it in the client store field.
3. If the verification function in the client store is non-nil, wrap the current representation in a struct that has a method that uses that function and implements oauth2.ClientPasswordVerifier. Otherwise, return the current representation.

This could be taken a step further by actually requiring a verification function to be more secure by default. In which case you could add an option to opt out of this behavior instead.

[strowk]

Hey, I have made this lib - https://github.com/strowk/go-oauth2-hashed

This is based on your example, @drognisep , but supports adding other hashing mechanisms and should work potentially with any store, though I have only checked with Postgres

[drognisep]

Hey, I have made this lib - https://github.com/strowk/go-oauth2-hashed

This is based on your example, @drognisep , but supports adding other hashing mechanisms and should work potentially with any store, though I have only checked with Postgres

I checked out your lib, @strowk, and it looks pretty great! :D

I really like the introduction of the Hasher interface for flexibility's sake. The provided bcrypt implementation is nice, but we could plug scrypt in there too for tighter/better control over the difficulty of the hashing.

One possible improvement that sticks out to me is where you're checking whether client info is nil in the NewClientInfoWithHash function. I don't think a nil ClientInfo really represents a valid state, especially considering how often it's referenced to implement the interface. Personally, I think the violation of such a fundamental constraint is panic-worthy (and it'll end up panicking anyway unless we check for nil). While it's usually advisable to avoid panicking, I still use in in cases like this where there's really no way this can represent a valid operating state.

It might also be nice to be able to specify a cost for the given BcryptHasher other than the hard-coded bcrypt.DefaultCost. ;)

Great job, and thanks for putting this together!

@vgarvardt do you think this is something that could be incorporated into go-oauth2-pg? Password/secret hashing is certainly a common requirement, so I think the thought that @strowk put into his wrapper could certainly help here. Thoughts?

[strowk]

Hey @drognisep , I forgot to comment on this, but after some thought I have actually made this into PR here - go-oauth2/oauth2#284

If this is accepted, then stores just need to implement SavingClientStore (which has Save method in addition to GetByID) to be compatible with the hashing wrapper, which users can reference directly.

[drognisep]

Sounds good, @strowk. The implementation might need to be a little different at that level, but I'll check the PR. Thanks!