API GW phase 0 (depends on PR #544) #551
Conversation
| @@ -108,7 +131,7 @@ func main() { | |||
| Logger.Fatal(err) | |||
| } | |||
|
|
|||
| // override the default so we can use self-signed certs on our microservices | |||
| // Override the default so we can use self-signed certs on our microservices | |||
| // and use a self-signed cert in this server | |||
| http.DefaultTransport.(*http.Transport).TLSClientConfig = &tls.Config{InsecureSkipVerify: true} | |||
There was a problem hiding this comment.
This is understandable for debug, but it should be a config, and default to false. No one should ever skip cert verification in production.
|
|
||
| rule := s.matchRule(req) | ||
| if rule == nil { | ||
| http.Error(w, "Not found", http.StatusNotFound) |
There was a problem hiding this comment.
"Not found" should be http.StatusText(http.StatusNotFound) to avoid magic strings.
| func rejectNoToken(handler http.Handler) http.HandlerFunc { | ||
| return func(w http.ResponseWriter, r *http.Request) { | ||
| w.WriteHeader(http.StatusForbidden) | ||
| func (rule *FwdRule) authorize(w http.ResponseWriter, req *http.Request) bool { |
There was a problem hiding this comment.
As-written this is acceptable but misleading. But, comments elsewhere suggest the ultimate goal is to deprecate internal TO authorization and rely exclusively on the proxy/auth service/other microservices. That would not be acceptable.
Authorization MUST check the database. If it doesn't, it's not possible to revoke tokens. For example, if an employee is fired, or if an attacker is discovered after gaining an authorization token. It must be possible to revoke access.
There was a problem hiding this comment.
By design, authorization does not check the database to avoid congestion. We use short lived access tokens.
The general approach for doing authentication in the API layer was discussed and agreed upon.
There was a problem hiding this comment.
The general approach for doing authentication in the API layer was discussed and agreed upon.
"We already agreed" is not an argument for implementing security vulnerabilities.
Moving this to the mailing list.
| body, err := ioutil.ReadAll(r.Body) | ||
| if err != nil { | ||
| Logger.Println("Error reading body: ", err.Error()) | ||
| http.Error(w, "Error reading body: "+err.Error(), http.StatusBadRequest) |
There was a problem hiding this comment.
These should log, but the error MUST NOT be returned to the client, for security. Especially SQL errors. Clients should be given an appropriate response code, and no more information.
| err = json.Unmarshal(body, &login) | ||
| if err != nil { | ||
| Logger.Println("Invalid JSON: ", err.Error()) | ||
| http.Error(w, "Invalid JSON: "+err.Error(), http.StatusBadRequest) |
There was a problem hiding this comment.
See previous comment: errors must not be returned to client.
|
|
||
| if err != nil { | ||
| Logger.Printf("%v %v Token error: %s", req.Method, req.URL.RequestURI(), err) | ||
| http.Error(w, "Forbidden", http.StatusForbidden) |
There was a problem hiding this comment.
"Forbidden" should be http.StatusText(http.StatusForbidden)
| route := rule.matchRoute(req) | ||
| if route == nil { | ||
| Logger.Printf("%v %v Route not found", req.Method, req.URL.RequestURI()) | ||
| http.Error(w, "Not found", http.StatusNotFound) |
There was a problem hiding this comment.
"Not found" should be http.StatusText(http.StatusNotFound)
| claims, ok := token.Claims.(*Claims) | ||
| if !ok { | ||
| Logger.Printf("%v %v Token valid but cannot parse claims", req.Method, req.URL.RequestURI()) | ||
| http.Error(w, "Forbidden", http.StatusForbidden) |
There was a problem hiding this comment.
"Forbidden" should be http.StatusText(http.StatusForbidden)
| method := route.Auth[req.Method] | ||
| if method == nil { | ||
| Logger.Printf("%v %v Route found but method forbidden", req.Method, req.URL.RequestURI()) | ||
| http.Error(w, "Forbidden", http.StatusForbidden) |
There was a problem hiding this comment.
"Forbidden" should be http.StatusText(http.StatusForbidden)
| if (satisfied > 0) { | ||
| Logger.Printf("%v %v Route found but required capabilitoes not satisfied. HAS %v, NEED %v", | ||
| req.Method, req.URL.RequestURI(), claims.Capabilities, method) | ||
| http.Error(w, "Forbidden", http.StatusForbidden) |
There was a problem hiding this comment.
"Forbidden" should be http.StatusText(http.StatusForbidden)
| #### Legacy TO support | ||
|
|
||
| Legacy TO authorization code requires any API call to pass a mojolicios access token in its access control headers. | ||
| Untill this code is deprecated, the Auth server and the API GW handle legacy authorization in hte following way: |
| @@ -1,6 +1,13 @@ | |||
|
|
|||
| A simple authentication server written in go that authenticates user agains the `tm_user` table and returns a jwt representing the user, incl. its API access capabilities, derived from the user's role. | |||
|
|
|||
| #### Legacy TO support | |||
|
|
|||
| Legacy TO authorization code requires any API call to pass a mojolicios access token in its access control headers. | |||
|
|
||
| Legacy TO authorization code requires any API call to pass a mojolicios access token in its access control headers. | ||
| Untill this code is deprecated, the Auth server and the API GW handle legacy authorization in hte following way: | ||
| Upon every sucessful login the auth server performs additional login against legacy TO (mojolicious app) and recieves a lagacy TO authentication token. |
|
Closed by mistake. Created PR #567. |
First operational version of API GW