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/10 17:34:50 UTC
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #6577: JWT Authorization
srijeet0406 commented on a change in pull request #6577:
URL: https://github.com/apache/trafficcontrol/pull/6577#discussion_r823957820
##########
File path: traffic_ops/traffic_ops_golang/api/api.go
##########
@@ -1029,9 +1030,35 @@ func ParseDBError(ierr error) (error, error, int) {
// GetUserFromReq returns the current user, any user error, any system error, and an error code to be returned if either error was not nil.
// This also uses the given ResponseWriter to refresh the cookie, if it was valid.
func GetUserFromReq(w http.ResponseWriter, r *http.Request, secret string) (auth.CurrentUser, error, error, int) {
- cookie, err := r.Cookie(tocookie.Name)
- if err != nil {
- return auth.CurrentUser{}, errors.New("Unauthorized, please log in."), errors.New("error getting cookie: " + err.Error()), http.StatusUnauthorized
+ var cookie *http.Cookie
+
+ if r.Header.Get("Authorization") != "" && strings.Contains(r.Header.Get("Authorization"), "Bearer") {
Review comment:
Could we define a couple of constants for these strings?
##########
File path: traffic_ops/traffic_ops_golang/api/api.go
##########
@@ -1029,9 +1030,35 @@ func ParseDBError(ierr error) (error, error, int) {
// GetUserFromReq returns the current user, any user error, any system error, and an error code to be returned if either error was not nil.
// This also uses the given ResponseWriter to refresh the cookie, if it was valid.
func GetUserFromReq(w http.ResponseWriter, r *http.Request, secret string) (auth.CurrentUser, error, error, int) {
- cookie, err := r.Cookie(tocookie.Name)
- if err != nil {
- return auth.CurrentUser{}, errors.New("Unauthorized, please log in."), errors.New("error getting cookie: " + err.Error()), http.StatusUnauthorized
+ var cookie *http.Cookie
+
+ if r.Header.Get("Authorization") != "" && strings.Contains(r.Header.Get("Authorization"), "Bearer") {
+ givenToken := r.Header.Get("Authorization")
+ tokenSplit := strings.Split(givenToken, " ")
+ if len(tokenSplit) > 1 {
+ givenToken = tokenSplit[1]
+ }
+ bearerCookie, err := getCookieFromAccessToken(givenToken, secret)
+ if err != nil {
+ return auth.CurrentUser{}, errors.New("Unauthorized, please log in."), err, http.StatusUnauthorized
+ }
+ cookie = bearerCookie
+ } else {
+ for _, givenCookie := range r.Cookies() {
+ if cookie != nil {
+ break
+ }
+ switch givenCookie.Name {
+ case "access_token":
+ bearerCookie, err := getCookieFromAccessToken(givenCookie.Value, secret)
+ if err != nil {
+ return auth.CurrentUser{}, errors.New("Unauthorized, please log in."), err, http.StatusUnauthorized
Review comment:
Same nit here re:error ^
##########
File path: traffic_ops/traffic_ops_golang/api/api.go
##########
@@ -1029,9 +1030,35 @@ func ParseDBError(ierr error) (error, error, int) {
// GetUserFromReq returns the current user, any user error, any system error, and an error code to be returned if either error was not nil.
// This also uses the given ResponseWriter to refresh the cookie, if it was valid.
func GetUserFromReq(w http.ResponseWriter, r *http.Request, secret string) (auth.CurrentUser, error, error, int) {
- cookie, err := r.Cookie(tocookie.Name)
- if err != nil {
- return auth.CurrentUser{}, errors.New("Unauthorized, please log in."), errors.New("error getting cookie: " + err.Error()), http.StatusUnauthorized
+ var cookie *http.Cookie
+
+ if r.Header.Get("Authorization") != "" && strings.Contains(r.Header.Get("Authorization"), "Bearer") {
+ givenToken := r.Header.Get("Authorization")
+ tokenSplit := strings.Split(givenToken, " ")
+ if len(tokenSplit) > 1 {
+ givenToken = tokenSplit[1]
+ }
+ bearerCookie, err := getCookieFromAccessToken(givenToken, secret)
+ if err != nil {
+ return auth.CurrentUser{}, errors.New("Unauthorized, please log in."), err, http.StatusUnauthorized
+ }
+ cookie = bearerCookie
+ } else {
+ for _, givenCookie := range r.Cookies() {
+ if cookie != nil {
+ break
+ }
+ switch givenCookie.Name {
Review comment:
Can there be a case where there is a (pointer to a)cookie in the array, but it is `nil`? If yes, this might panic in that case.
##########
File path: traffic_ops/traffic_ops_golang/api/api.go
##########
@@ -1075,6 +1102,34 @@ func GetUserFromReq(w http.ResponseWriter, r *http.Request, secret string) (auth
return user, nil, nil, http.StatusOK
}
+func getCookieFromAccessToken(bearerToken string, secret string) (*http.Cookie, error) {
+ var cookie *http.Cookie
+ claims := jwt.MapClaims{}
+ token, err := jwt.ParseWithClaims(bearerToken, claims, func(token *jwt.Token) (interface{}, error) {
+ return []byte(secret), nil
+ })
+ if err != nil {
+ return nil, fmt.Errorf("parsing claims: %w", err)
+ }
+ if !token.Valid {
+ return nil, errors.New("invalid token")
+ }
+
+ for key, val := range claims {
+ switch key {
+ case "mojoCookie":
+ if _, ok := val.(string); !ok {
+ return nil, errors.New("invalid token - mojoCookie must be a string")
+ }
+ cookie = &http.Cookie{
+ Value: val.(string),
Review comment:
Can we just use the value directly from line 1121, instead of type casting it again?
Also, `mojoCookie` could be a constant.
##########
File path: traffic_ops/traffic_ops_golang/cdni/shared.go
##########
@@ -106,6 +107,25 @@ func GetCapabilities(w http.ResponseWriter, r *http.Request) {
api.WriteRespRaw(w, r, fciCaps)
}
+func getBearerToken(r *http.Request) string {
+ if r.Header.Get("Authorization") != "" && strings.Contains(r.Header.Get("Authorization"), "Bearer") {
Review comment:
We could use the constants from my earlier comment here.
##########
File path: traffic_ops/traffic_ops_golang/cdni/shared.go
##########
@@ -106,6 +107,25 @@ func GetCapabilities(w http.ResponseWriter, r *http.Request) {
api.WriteRespRaw(w, r, fciCaps)
}
+func getBearerToken(r *http.Request) string {
+ if r.Header.Get("Authorization") != "" && strings.Contains(r.Header.Get("Authorization"), "Bearer") {
+ givenToken := r.Header.Get("Authorization")
+ if strings.Contains(givenToken, "access_token") {
+ givenToken = strings.Split(givenToken, "=")[1]
+ } else {
+ givenToken = strings.Split(givenToken, " ")[1]
Review comment:
Same comment here ^
##########
File path: traffic_ops/traffic_ops_golang/api/api.go
##########
@@ -1029,9 +1030,35 @@ func ParseDBError(ierr error) (error, error, int) {
// GetUserFromReq returns the current user, any user error, any system error, and an error code to be returned if either error was not nil.
// This also uses the given ResponseWriter to refresh the cookie, if it was valid.
func GetUserFromReq(w http.ResponseWriter, r *http.Request, secret string) (auth.CurrentUser, error, error, int) {
- cookie, err := r.Cookie(tocookie.Name)
- if err != nil {
- return auth.CurrentUser{}, errors.New("Unauthorized, please log in."), errors.New("error getting cookie: " + err.Error()), http.StatusUnauthorized
+ var cookie *http.Cookie
+
+ if r.Header.Get("Authorization") != "" && strings.Contains(r.Header.Get("Authorization"), "Bearer") {
+ givenToken := r.Header.Get("Authorization")
+ tokenSplit := strings.Split(givenToken, " ")
+ if len(tokenSplit) > 1 {
+ givenToken = tokenSplit[1]
+ }
+ bearerCookie, err := getCookieFromAccessToken(givenToken, secret)
+ if err != nil {
+ return auth.CurrentUser{}, errors.New("Unauthorized, please log in."), err, http.StatusUnauthorized
+ }
+ cookie = bearerCookie
+ } else {
+ for _, givenCookie := range r.Cookies() {
+ if cookie != nil {
+ break
+ }
+ switch givenCookie.Name {
+ case "access_token":
Review comment:
Might be good to make this into a constant
##########
File path: traffic_ops/traffic_ops_golang/cdni/shared.go
##########
@@ -106,6 +107,25 @@ func GetCapabilities(w http.ResponseWriter, r *http.Request) {
api.WriteRespRaw(w, r, fciCaps)
}
+func getBearerToken(r *http.Request) string {
+ if r.Header.Get("Authorization") != "" && strings.Contains(r.Header.Get("Authorization"), "Bearer") {
+ givenToken := r.Header.Get("Authorization")
+ if strings.Contains(givenToken, "access_token") {
+ givenToken = strings.Split(givenToken, "=")[1]
+ } else {
+ givenToken = strings.Split(givenToken, " ")[1]
+ }
+ return givenToken
+ }
+ for _, cookie := range r.Cookies() {
+ switch cookie.Name {
+ case "access_token":
Review comment:
Same comment re:constants
##########
File path: traffic_ops/traffic_ops_golang/api/api.go
##########
@@ -1029,9 +1030,35 @@ func ParseDBError(ierr error) (error, error, int) {
// GetUserFromReq returns the current user, any user error, any system error, and an error code to be returned if either error was not nil.
// This also uses the given ResponseWriter to refresh the cookie, if it was valid.
func GetUserFromReq(w http.ResponseWriter, r *http.Request, secret string) (auth.CurrentUser, error, error, int) {
- cookie, err := r.Cookie(tocookie.Name)
- if err != nil {
- return auth.CurrentUser{}, errors.New("Unauthorized, please log in."), errors.New("error getting cookie: " + err.Error()), http.StatusUnauthorized
+ var cookie *http.Cookie
+
+ if r.Header.Get("Authorization") != "" && strings.Contains(r.Header.Get("Authorization"), "Bearer") {
+ givenToken := r.Header.Get("Authorization")
+ tokenSplit := strings.Split(givenToken, " ")
+ if len(tokenSplit) > 1 {
+ givenToken = tokenSplit[1]
+ }
+ bearerCookie, err := getCookieFromAccessToken(givenToken, secret)
+ if err != nil {
+ return auth.CurrentUser{}, errors.New("Unauthorized, please log in."), err, http.StatusUnauthorized
Review comment:
nit: Errors should start with lowercase letters.
##########
File path: traffic_ops/traffic_ops_golang/api/api.go
##########
@@ -1075,6 +1102,34 @@ func GetUserFromReq(w http.ResponseWriter, r *http.Request, secret string) (auth
return user, nil, nil, http.StatusOK
}
+func getCookieFromAccessToken(bearerToken string, secret string) (*http.Cookie, error) {
+ var cookie *http.Cookie
+ claims := jwt.MapClaims{}
+ token, err := jwt.ParseWithClaims(bearerToken, claims, func(token *jwt.Token) (interface{}, error) {
+ return []byte(secret), nil
+ })
+ if err != nil {
+ return nil, fmt.Errorf("parsing claims: %w", err)
+ }
+ if !token.Valid {
Review comment:
I didn't look in the underlying functions, but can there be a case where there is no error, but the returned token pointer is `nil`?
If so, this will panic in that case.
If not, then disregard this comment :)
##########
File path: traffic_ops/traffic_ops_golang/cdni/shared.go
##########
@@ -106,6 +107,25 @@ func GetCapabilities(w http.ResponseWriter, r *http.Request) {
api.WriteRespRaw(w, r, fciCaps)
}
+func getBearerToken(r *http.Request) string {
+ if r.Header.Get("Authorization") != "" && strings.Contains(r.Header.Get("Authorization"), "Bearer") {
+ givenToken := r.Header.Get("Authorization")
+ if strings.Contains(givenToken, "access_token") {
+ givenToken = strings.Split(givenToken, "=")[1]
Review comment:
We should probably do a length check before getting the second item from the array here.
##########
File path: traffic_ops/traffic_ops_golang/cdni/shared.go
##########
@@ -120,14 +140,15 @@ func PutHostConfiguration(w http.ResponseWriter, r *http.Request) {
return
}
- if inf.Config.Cdni == nil || inf.Config.Cdni.JwtDecodingSecret == "" || inf.Config.Cdni.DCdnId == "" {
+ if inf.Config.Cdni == nil || inf.Config.Secrets[0] == "" || inf.Config.Cdni.DCdnId == "" {
Review comment:
Same comment re: length check
##########
File path: traffic_ops/traffic_ops_golang/cdni/shared.go
##########
@@ -73,14 +72,16 @@ func GetCapabilities(w http.ResponseWriter, r *http.Request) {
}
defer inf.Close()
- if inf.Config.Cdni == nil || inf.Config.Cdni.JwtDecodingSecret == "" || inf.Config.Cdni.DCdnId == "" {
+ if inf.Config.Cdni == nil || inf.Config.Secrets[0] == "" || inf.Config.Cdni.DCdnId == "" {
Review comment:
Unless it is being done elsewhere, we should probably check to make sure that `inf.Config.Secrets` has length of `>=1` before getting the first item from there.
##########
File path: traffic_ops/traffic_ops_golang/cdni/shared.go
##########
@@ -517,8 +539,20 @@ func checkBearerToken(bearerToken string, inf *api.APIInfo) (string, error) {
if dcdn != inf.Config.Cdni.DCdnId {
return "", errors.New("invalid token - incorrect dcdn")
}
+
+ if ucdn != inf.User.UCDN {
+ return "", errors.New("user ucdn did not match token ucdn")
+ }
+
if ucdn == "" {
- return "", errors.New("invalid token - empty ucdn field")
+ if inf.User.Can("ICDN:UCDN-OVERRIDE") {
+ ucdn = inf.Params["ucdn"]
+ if ucdn == "" {
+ return "", errors.New("admin level ucdn requests require a ucdn query parameter")
+ }
+ } else {
+ return "", errors.New("invalid token - empty ucdn field")
Review comment:
Should this error be something like `user doesn't have the required permission:ICDN:UCDN-OVERRIDE`?
##########
File path: traffic_ops/traffic_ops_golang/cdni/shared.go
##########
@@ -190,14 +211,15 @@ func PutConfiguration(w http.ResponseWriter, r *http.Request) {
}
defer inf.Close()
- if inf.Config.Cdni == nil || inf.Config.Cdni.JwtDecodingSecret == "" || inf.Config.Cdni.DCdnId == "" {
+ if inf.Config.Cdni == nil || inf.Config.Secrets[0] == "" || inf.Config.Cdni.DCdnId == "" {
Review comment:
Same comment re:length. We could just check it at the beginning of the function once and call it good.
--
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