Ensure that unset Context.Name only allowed on base route

If Context.Name is not set, the acceess controller may allow an unintended
request through. By only allowing a request to proceed without a name on the
base route, we provide some protection if future bugs forget to set the context
properly.
This commit is contained in:
Stephen J Day 2014-12-18 17:20:35 -08:00
parent e50fcc0ab9
commit b1f36c3fe5
3 changed files with 55 additions and 30 deletions

78
app.go
View file

@ -172,36 +172,56 @@ func (app *App) authorized(w http.ResponseWriter, r *http.Request, context *Cont
} }
var accessRecords []auth.Access var accessRecords []auth.Access
resource := auth.Resource{
Type: "repository",
Name: context.Name,
}
switch r.Method { if context.Name != "" {
case "GET", "HEAD": resource := auth.Resource{
accessRecords = append(accessRecords, Type: "repository",
auth.Access{ Name: context.Name,
Resource: resource, }
Action: "pull",
}) switch r.Method {
case "POST", "PUT", "PATCH": case "GET", "HEAD":
accessRecords = append(accessRecords, accessRecords = append(accessRecords,
auth.Access{ auth.Access{
Resource: resource, Resource: resource,
Action: "pull", Action: "pull",
}, })
auth.Access{ case "POST", "PUT", "PATCH":
Resource: resource, accessRecords = append(accessRecords,
Action: "push", auth.Access{
}) Resource: resource,
case "DELETE": Action: "pull",
// DELETE access requires full admin rights, which is represented },
// as "*". This may not be ideal. auth.Access{
accessRecords = append(accessRecords, Resource: resource,
auth.Access{ Action: "push",
Resource: resource, })
Action: "*", case "DELETE":
}) // DELETE access requires full admin rights, which is represented
// as "*". This may not be ideal.
accessRecords = append(accessRecords,
auth.Access{
Resource: resource,
Action: "*",
})
}
} else {
// Only allow the name not to be set on the base route.
route := mux.CurrentRoute(r)
if route == nil || route.GetName() != v2.RouteNameBase {
// For this to be properly secured, context.Name must always be set
// for a resource that may make a modification. The only condition
// under which name is not set and we still allow access is when the
// base route is accessed. This section prevents us from making that
// mistake elsewhere in the code, allowing any operation to proceed.
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusForbidden)
var errs v2.Errors
errs.Push(v2.ErrorCodeUnauthorized)
serveJSON(w, errs)
}
} }
if err := app.accessController.Authorized(r, accessRecords...); err != nil { if err := app.accessController.Authorized(r, accessRecords...); err != nil {

View file

@ -177,6 +177,11 @@ func TestNewApp(t *testing.T) {
t.Fatalf("unexpected content-type: %v != %v", req.Header.Get("Content-Type"), "application/json") t.Fatalf("unexpected content-type: %v != %v", req.Header.Get("Content-Type"), "application/json")
} }
expectedAuthHeader := "Bearer realm=\"realm-test\",service=\"service-test\""
if req.Header.Get("Authorization") != expectedAuthHeader {
t.Fatalf("unexpected authorization header: %q != %q", req.Header.Get("Authorization"), expectedAuthHeader)
}
var errs v2.Errors var errs v2.Errors
dec := json.NewDecoder(req.Body) dec := json.NewDecoder(req.Body)
if err := dec.Decode(&errs); err != nil { if err := dec.Decode(&errs); err != nil {

View file

@ -51,7 +51,7 @@ func (ac *accessController) Authorized(req *http.Request, accessRecords ...auth.
if len(accessRecords) > 0 { if len(accessRecords) > 0 {
var scopes []string var scopes []string
for _, access := range accessRecords { for _, access := range accessRecords {
scopes = append(scopes, fmt.Sprintf("%s:%s:%s", access.Type, access.Resource, access.Action)) scopes = append(scopes, fmt.Sprintf("%s:%s:%s", access.Type, access.Resource.Name, access.Action))
} }
challenge.scope = strings.Join(scopes, " ") challenge.scope = strings.Join(scopes, " ")
} }