Support for clients' secret rotation

[simonbrynych]

Is your feature request related to a problem? Please describe.

Currently it's not possible to simultaneously rotate secrets of individual clients.

Describe the solution you'd like

That would be great to set two passwords while both of them are valid in that time. That allows user to smoothly rotate secret without any outage.

&fosite.DefaultClient{
  ID:           ID,
  Secret:       [originalSecret, newSecret],
  RedirectURIs:  redirectURIs,
  ResponseTypes: responses,
  GrantTypes:    grants,
  Scopes:        scopes,
}

[aeneasr]

The OAuth2 "compliant" (or rather OpenID Connect) way to do this is to create a copy of the client with a new password and update the client ID and the client secret in the OAuth2 consumers. This is because OpenID Connect Dynamic Client Registration currently does not have a spec for secret rotation and the API design more or less prohibits secret rotation.

Alternative would be using JWT-based auth for clients (private_key_jwt) which supports rotation out of the box.

[aeneasr]

Nevertheless - I completely understand the feature and think it's a good idea to support that!

[simonbrynych]

Great. Thanks! We want to automate entire process so having two active secrets is preferred way for us. What you think about a way how to get this feature done? Can we kindly ask you to add this functionality?

[aeneasr]

That's not how open source works ;) But you can come up with an idea how of to implement it, discuss it in this issue, and then execute on the plan in a PR

[simonbrynych]

No problem :), we will prepare an idea PR and we are gonna share that with you. ;]

[rplnt]

@aeneasr Hi, we have looked into this and have the following proposal:

1. Create new interface that would expand on fosite.Client, adding just one extra method returning secondary hashed secret. E.g.:

type ClientWithSecretRotation interface {
	Client
	// GetHashedSecretPrevious returns the previous hashed secret as it is stored in the store.
	GetHashedSecretPrevious() []byte
}

2. In fosite.AuthenticateClient (as well as in fosite.NewIntrospectionRequest) check first secret, but on failure try to convert to the new interface to see if secondary secret is available and check that as well. E.g.:

func (f *Fosite) checkClientSecret(ctx context.Context, client Client, clientSecret []byte) error {
	var err error
	err = f.Hasher.Compare(ctx, client.GetHashedSecret(), []byte(clientSecret))
	if err == nil {
		return nil
	}
	cc, ok := client.(ClientWithSecretRotation)
	if !ok {
		return err
	}

	return f.Hasher.Compare(ctx, cc.GetHashedSecretPrevious(), []byte(clientSecret))
}

This seems like a simple approach that won't break custom clients as it just adds a new interface and behavior for existing clients doesn't change. Having it separated like this also allows for instrumentation on the GetHashedSecret* methods to monitor what secrets are still in use.
Another considered option was to add split this functionality completely - have a method returning a list of secrets in which case the original secret would be ignored, but usage is less straight-forward in my opinion (although the naming could be clearer).

Is this something that would be an acceptable approach?

There are some open questions:

• naming: from my small research secret rotation for pairs seems to be oftentimes named current/previous or primary/secondary (the new method would probably have to have a suffix); what is prefferred? the interface name isn't ideal either
• should the DefaultClient be extended to support this interface (I would guess yes to test the code paths)?

[aeneasr]

0Awesome, this sounds like a great proposal! I think the naming could be GetRotatedHashes()! ClientWithSecretRotation sounds like a good naming choice :)

We could also think of making return a slice of slice of bytes so GetRotatedHashes() [][]byte which would allow more than one password in the rotation!

Looking forward to the contrib!

[aeneasr]

Closing because merged!