Skip to content

Implementing authorization rotation for static files#4294

Merged
sougou merged 2 commits intovitessio:masterfrom
planetscale:abhi-auth-rotation
Nov 4, 2018
Merged

Implementing authorization rotation for static files#4294
sougou merged 2 commits intovitessio:masterfrom
planetscale:abhi-auth-rotation

Conversation

@avaidyanatha
Copy link
Copy Markdown
Contributor

Main Changes

  • ValidateHash now reloads Authorization file if given time has elapsed
  • This allows for the rotation of static authorization strings/config

Notes

  • Pass in the rotation time through the command line flag: "-mysql_auth_rotation_time"
  • The default is 5 minutes
  • Tests will return non-fatal "failed to read" errors - this is expected

Signed-off-by: avaidyanatha avaidyanatha@ucsb.edu

Signed-off-by: avaidyanatha <avaidyanatha@ucsb.edu>
Copy link
Copy Markdown
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

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

@demmer has pointed out that there exists a data race between loadConfigFromParams, which sets a.Entries and all the other functions like ValidateHash that read from it.

This race was originally introduced by installSignalHandlers, but this change aggravates it because loadFromConfig is now called every time ValidateHash is invoked. We probably need to introduce a mutex to prevent these races.

Signed-off-by: avaidyanatha <avaidyanatha@ucsb.edu>
@sougou sougou merged commit 2437ee4 into vitessio:master Nov 4, 2018
@demmer
Copy link
Copy Markdown
Member

demmer commented Nov 4, 2018

I think this PR still needs additional work to be robust and safe.

Reading through I noticed the following issues:

  1. The lastAuthRotation is accessed and modified from various threads without holding the mutex.

  2. There’s no protection against multiple threads concurrently deciding that it’s time to reload the config and racing to do it.

  3. The system calls needed to load the config are issued while holding the mutex, which would block all threads from being able to do auth verification while the refresh is happening.

I think a better pattern for this would be to rework it to safely spin up a goroutine to refresh the config asynchronously when the ttl lapses.

FWIW it might also be worth thinking about making a periodic config file refresh utility class with an interface to manage the ttl tracking and reloader goroutine management though tbh just the simple timer is probably sufficient.

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.

3 participants