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

server/rotation.go: Fix key rotation with multiple dex instances. #998

Merged
merged 1 commit into from Jul 19, 2017

Conversation

rithujohn191
Copy link
Contributor

This fixes #976.

It also ensures that it displays "keys already rotated" as an info log message instead of an error message.

@@ -15,6 +16,8 @@ import (
"github.com/coreos/dex/storage"
)

var errAlreadyRotated = errors.New("key already rotated by another instance of Dex")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

s/key/keys/g
s/another instance of Dex/another server instance/g

Copy link
Contributor

@ericchiang ericchiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@robszumski
Copy link

From the changes it looks like this was cosmetic not an actual bug? Is that true?

@ericchiang
Copy link
Contributor

ericchiang commented Jul 19, 2017

@robszumski returning nil instead of an error was causing the update to write an empty keys object to the backing storage. This was indeed a pretty bad bug if two instances of dex were attempting to rotate keys within a second or so of each other.

@rithujohn191
Copy link
Contributor Author

@robszumski Adding to that its necessary to have more than one instance of Dex to reproduce this bug. Its not an issue if the user has only one replica of Dex

mmrath pushed a commit to mmrath/dex that referenced this pull request Sep 2, 2019
server/rotation.go: Fix key rotation with multiple dex instances.
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

Successfully merging this pull request may close these issues.

Key rotation regression
3 participants