You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2022/03/17 21:56:04 UTC
[GitHub] [trafficcontrol] rawlinp opened a new pull request #6671: Add TO in-memory User cache option to improve performance
rawlinp opened a new pull request #6671:
URL: https://github.com/apache/trafficcontrol/pull/6671
Currently, logging in a user can take up to 4 different DB queries, and
most route handlers require at least 1 DB query to look up the user and
role of the requester.
By periodically reading users and roles from the database (using the
interval defined by `user_cache_refresh_interval_sec` in cdn.conf) and
storing them in memory, Traffic Ops can quickly check the user's data
and role without having to read from the database at request time.
This makes logins extremely efficient because they require no database
queries at request time. Additionally, overall database load is reduced
because most route handlers have reduced the number of queries they make
by 1.
From a security perspective, making logins extremely efficient is
advantageous because it reduces the effectiveness of denial of service
attacks targeting the login endpoint (which would normally make at least
1 database query to check that the user is allowed to log in). With this
feature enabled, a DoS attack against the login endpoint would not
exhaust limited database resources. Note: other endpoints are not as
susceptible to DoS attacks due to requiring a valid cookie before any
database queries are made.
The downside to enabling this feature is that updates to Users and/or
Roles are not enforced by Traffic Ops immediately when the updates are
made. If `user_cache_refresh_interval_sec` is N, it may take up to N
seconds before Traffic Ops will enforce updates to Users and/or Roles.
However, as long as this interval is sufficiently short, this should not
be a problem.
<!--
Thank you for contributing! Please be sure to read our contribution guidelines: https://github.com/apache/trafficcontrol/blob/master/CONTRIBUTING.md
If this closes or relates to an existing issue, please reference it using one of the following:
Closes: #ISSUE
Related: #ISSUE
If this PR fixes a security vulnerability, DO NOT submit! Instead, contact
the Apache Traffic Control Security Team at security@trafficcontrol.apache.org and follow the
guidelines at https://apache.org/security regarding vulnerability disclosure.
-->
<!-- **^ Add meaningful description above** --><hr/>
## Which Traffic Control components are affected by this PR?
<!-- Please delete all components from this list that are NOT affected by this PR.
Feel free to add the name of a tool or script that is affected but not on the list.
-->
- Documentation
- Traffic Ops
- Ansible
## What is the best way to verify this PR?
<!-- Please include here ALL the steps necessary to test your PR.
If your PR has tests (and most should), provide the steps needed to run the tests.
If not, please provide step-by-step instructions to test the PR manually and explain why your PR does not need tests. -->
Ensure the automated tests pass. Manual: configure `user_cache_refresh_interval_sec` in cdn.conf to 5, start TO, and change a user's role (or its role's permissions), while using that user to make requests for endpoints with the corresponding permissions that are being changed. Ensure that the changed permissions are enforced within 5 seconds of making the change via the API.
## PR submission checklist
- [x] This PR has tests <!-- If not, please delete this text and explain why this PR does not need tests. -->
- [x] This PR has documentation <!-- If not, please delete this text and explain why this PR does not need documentation. -->
- [x] This PR has a CHANGELOG.md entry <!-- A fix for a bug from an ATC release, an improvement, or a new feature should have a changelog entry. -->
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://apache.org/security) for details)
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 merged pull request #6671: Add TO in-memory User cache option to improve performance
Posted by GitBox <gi...@apache.org>.
srijeet0406 merged pull request #6671:
URL: https://github.com/apache/trafficcontrol/pull/6671
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on pull request #6671: Add TO in-memory User cache option to improve performance
Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #6671:
URL: https://github.com/apache/trafficcontrol/pull/6671#issuecomment-1075364025
```
ERROR: for to_server (<Service: to_server>, 'Build failed')
Service 'to_server' failed to build : Build failed
Error: Process completed with exit code 1.
```
However, this builds fine for me locally using docker... I can't really tell why it's failing here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #6671: Add TO in-memory User cache option to improve performance
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #6671:
URL: https://github.com/apache/trafficcontrol/pull/6671#discussion_r835555694
##########
File path: traffic_ops/traffic_ops_golang/auth/usercache.go
##########
@@ -0,0 +1,208 @@
+package auth
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import (
+ "context"
+ "database/sql"
+ "errors"
+ "sync"
+ "time"
+
+ "github.com/apache/trafficcontrol/lib/go-log"
+
+ "github.com/lib/pq"
+)
+
+const (
+ getUsersQuery = `
+ SELECT
+ u.id,
+ u.local_passwd,
+ u.role,
+ u.tenant_id,
+ u.token,
+ u.ucdn,
+ u.username
+ FROM
+ tm_user AS u
+ `
+ getRolesQuery = `
+ SELECT
+ ARRAY(SELECT rc.cap_name FROM role_capability AS rc WHERE rc.role_id=r.id) AS capabilities,
+ r.id as role,
+ r.name as role_name,
+ r.priv_level
+ FROM role r
+ `
+)
+
+type user struct {
+ CurrentUser
+ LocalPasswd *string
+ Token *string
+}
+
+type role struct {
+ Capabilities pq.StringArray
+ ID int
+ Name string
+ PrivLevel int
+}
+
+type users struct {
+ userMap map[string]user
+ usernamesByToken map[string]string
+ *sync.RWMutex
+ initialized bool
+ enabled bool // note: enabled is only written to once at startup, before serving requests, so it doesn't need synchronized access
+}
+
+var usersCache = users{RWMutex: &sync.RWMutex{}}
+
+func usersCacheIsEnabled() bool {
+ if usersCache.enabled {
+ usersCache.RLock()
+ defer usersCache.RUnlock()
+ return usersCache.initialized
+ }
+ return false
+}
+
+// getUserFromCache returns the user with the given username and a boolean indicating whether the user exists.
+func getUserFromCache(username string) (user, bool) {
+ usersCache.RLock()
+ defer usersCache.RUnlock()
+ u, exists := usersCache.userMap[username]
+ return u, exists
+}
+
+// getUserNameFromCacheByToken returns the username with the given token or an empty string if not found.
+func getUserNameFromCacheByToken(token string) string {
Review comment:
That way we don't have to check it everywhere this function gets called.
##########
File path: traffic_ops/traffic_ops_golang/auth/authorize.go
##########
@@ -197,15 +217,26 @@ func GetUserUcdn(form PasswordForm, db *sqlx.DB, ctx context.Context) (string, e
func CheckLocalUserPassword(form PasswordForm, db *sqlx.DB, ctx context.Context) (bool, error, error) {
var hashedPassword string
-
- err := db.GetContext(ctx, &hashedPassword, "SELECT local_passwd FROM tm_user WHERE username=$1", form.Username)
- if err != nil {
- if err == context.DeadlineExceeded || err == context.Canceled {
- return false, nil, err
+ if usersCacheIsEnabled() {
+ u, exists := getUserFromCache(form.Username)
+ if !exists {
+ return false, fmt.Errorf("user '%s' not found in cache", form.Username), nil
+ }
+ if u.LocalPasswd == nil {
+ return false, nil, nil
Review comment:
Should we return some kind of an error here, saying that the localpswd was `nil`?
##########
File path: traffic_ops/traffic_ops_golang/auth/usercache.go
##########
@@ -0,0 +1,208 @@
+package auth
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import (
+ "context"
+ "database/sql"
+ "errors"
+ "sync"
+ "time"
+
+ "github.com/apache/trafficcontrol/lib/go-log"
+
+ "github.com/lib/pq"
+)
+
+const (
+ getUsersQuery = `
+ SELECT
+ u.id,
+ u.local_passwd,
+ u.role,
+ u.tenant_id,
+ u.token,
+ u.ucdn,
+ u.username
+ FROM
+ tm_user AS u
+ `
+ getRolesQuery = `
+ SELECT
+ ARRAY(SELECT rc.cap_name FROM role_capability AS rc WHERE rc.role_id=r.id) AS capabilities,
+ r.id as role,
+ r.name as role_name,
+ r.priv_level
+ FROM role r
+ `
+)
+
+type user struct {
+ CurrentUser
+ LocalPasswd *string
+ Token *string
+}
+
+type role struct {
+ Capabilities pq.StringArray
+ ID int
+ Name string
+ PrivLevel int
+}
+
+type users struct {
+ userMap map[string]user
+ usernamesByToken map[string]string
+ *sync.RWMutex
+ initialized bool
+ enabled bool // note: enabled is only written to once at startup, before serving requests, so it doesn't need synchronized access
+}
+
+var usersCache = users{RWMutex: &sync.RWMutex{}}
+
+func usersCacheIsEnabled() bool {
+ if usersCache.enabled {
+ usersCache.RLock()
+ defer usersCache.RUnlock()
+ return usersCache.initialized
+ }
+ return false
+}
+
+// getUserFromCache returns the user with the given username and a boolean indicating whether the user exists.
+func getUserFromCache(username string) (user, bool) {
+ usersCache.RLock()
+ defer usersCache.RUnlock()
+ u, exists := usersCache.userMap[username]
+ return u, exists
+}
+
+// getUserNameFromCacheByToken returns the username with the given token or an empty string if not found.
+func getUserNameFromCacheByToken(token string) string {
Review comment:
just for uniformity sake, does it make sense to return a boolean here to denote whether or not the username was found, and whether or not it was = `""`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #6671: Add TO in-memory User cache option to improve performance
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #6671:
URL: https://github.com/apache/trafficcontrol/pull/6671#discussion_r835676966
##########
File path: traffic_ops/traffic_ops_golang/auth/usercache.go
##########
@@ -0,0 +1,208 @@
+package auth
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import (
+ "context"
+ "database/sql"
+ "errors"
+ "sync"
+ "time"
+
+ "github.com/apache/trafficcontrol/lib/go-log"
+
+ "github.com/lib/pq"
+)
+
+const (
+ getUsersQuery = `
+ SELECT
+ u.id,
+ u.local_passwd,
+ u.role,
+ u.tenant_id,
+ u.token,
+ u.ucdn,
+ u.username
+ FROM
+ tm_user AS u
+ `
+ getRolesQuery = `
+ SELECT
+ ARRAY(SELECT rc.cap_name FROM role_capability AS rc WHERE rc.role_id=r.id) AS capabilities,
+ r.id as role,
+ r.name as role_name,
+ r.priv_level
+ FROM role r
+ `
+)
+
+type user struct {
+ CurrentUser
+ LocalPasswd *string
+ Token *string
+}
+
+type role struct {
+ Capabilities pq.StringArray
+ ID int
+ Name string
+ PrivLevel int
+}
+
+type users struct {
+ userMap map[string]user
+ usernamesByToken map[string]string
+ *sync.RWMutex
+ initialized bool
+ enabled bool // note: enabled is only written to once at startup, before serving requests, so it doesn't need synchronized access
+}
+
+var usersCache = users{RWMutex: &sync.RWMutex{}}
+
+func usersCacheIsEnabled() bool {
+ if usersCache.enabled {
+ usersCache.RLock()
+ defer usersCache.RUnlock()
+ return usersCache.initialized
+ }
+ return false
+}
+
+// getUserFromCache returns the user with the given username and a boolean indicating whether the user exists.
+func getUserFromCache(username string) (user, bool) {
+ usersCache.RLock()
+ defer usersCache.RUnlock()
+ u, exists := usersCache.userMap[username]
+ return u, exists
+}
+
+// getUserNameFromCacheByToken returns the username with the given token or an empty string if not found.
+func getUserNameFromCacheByToken(token string) string {
Review comment:
I actually considered that approach, but this is only getting called in one place and is only for the token login, so I just left it that way. That said, I'll go ahead and do it for consistency at least.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #6671: Add TO in-memory User cache option to improve performance
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #6671:
URL: https://github.com/apache/trafficcontrol/pull/6671#discussion_r835676499
##########
File path: traffic_ops/traffic_ops_golang/auth/authorize.go
##########
@@ -197,15 +217,26 @@ func GetUserUcdn(form PasswordForm, db *sqlx.DB, ctx context.Context) (string, e
func CheckLocalUserPassword(form PasswordForm, db *sqlx.DB, ctx context.Context) (bool, error, error) {
var hashedPassword string
-
- err := db.GetContext(ctx, &hashedPassword, "SELECT local_passwd FROM tm_user WHERE username=$1", form.Username)
- if err != nil {
- if err == context.DeadlineExceeded || err == context.Canceled {
- return false, nil, err
+ if usersCacheIsEnabled() {
+ u, exists := getUserFromCache(form.Username)
+ if !exists {
+ return false, fmt.Errorf("user '%s' not found in cache", form.Username), nil
+ }
+ if u.LocalPasswd == nil {
+ return false, nil, nil
Review comment:
I don't think so, because this means the user doesn't have a local password, which is normal behavior if a user is LDAP-only. The original code (and non-usersCache path) will log this non-error, but I didn't feel like correcting that path since it means checking a different sql error and making that conditional more complex.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org