-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
fix #3102: add authentication cache #3161
Conversation
@@ -94,7 +94,7 @@ func (p *Parser) ParseStatement() (Statement, error) { | |||
case ALTER: | |||
return p.parseAlterStatement() | |||
case SET: | |||
return p.parseSetStatement() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this because the only use of SET
in our system is for setting a password?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the name of the function to match the pattern we use for all the other parse...
functions.
+1, makes sense to me. We'll want this cherry-picked to the 0.9.1 branch too. If there is already a test for altering and dropping a user, and ensuring he or she can't still authenticate (to ensure cache is cleared), we should be good. |
@@ -980,11 +984,18 @@ func (s *Store) Authenticate(username, password string) (ui *UserInfo, err error | |||
return ErrUserNotFound | |||
} | |||
|
|||
// Check the local auth cache first. | |||
if p, ok := s.authCache[username]; ok && p == password { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should change this. if it's ok
but p != password
we should return an authentication error rather than doing the bcrypt. Means that someone could easily DDOS an InfluxDB server with bad auth info
@@ -96,6 +96,9 @@ type Store struct { | |||
// The amount of time without an apply before sending a heartbeat. | |||
CommitTimeout time.Duration | |||
|
|||
// Authentication cache. | |||
authCache map[string]string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically speaking, I think this also needs synchronization. The code which checks the cache could run in a different goroutine than that which process DROP
or `ALTER'.
b57412a
to
cf86b5e
Compare
+1, thanks for the tests. I presume you will cherry-pick this over to the 0.9.1 branch. |
|
||
// GetHashPasswordFn returns the current password hashing function. | ||
func (s *Store) GetHashPasswordFn() HashPasswordFn { | ||
s.mu.Lock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could probably use a read-lock here.
821202f
to
e0a472b
Compare
+1, as long as the calls to |
fix #3102: add authentication cache
Cherry-picked to 0.9.1 |
No description provided.