From 7e39d0935d133ac2f59058e640e4c9f39b8a068b Mon Sep 17 00:00:00 2001 From: Ivan Kozlovic Date: Mon, 27 Aug 2018 14:43:32 -0600 Subject: [PATCH] Fixed crash related to route permissions after allow/deny feature This is an issue in master only, not in any public release. The issue is that permissions should be assigned as-is for the route perms because Publish/Subscribe could be nil, so trying to dereference Publish.Allow/Deny or Subscribe.Allow/Deny could crash. The code checking for permissions correctly check if Publish/Subscribe is nil or not. This was introduced with PR #725 Signed-off-by: Ivan Kozlovic --- server/opts.go | 10 +--- server/route.go | 11 ++-- test/routes_test.go | 127 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 132 insertions(+), 16 deletions(-) diff --git a/server/opts.go b/server/opts.go index a520b62f..05ed57a2 100644 --- a/server/opts.go +++ b/server/opts.go @@ -395,14 +395,8 @@ func parseCluster(cm map[string]interface{}, opts *Options) error { // The parsing sets Import into Publish and Export into Subscribe, convert // accordingly. opts.Cluster.Permissions = &RoutePermissions{ - Import: &SubjectPermission{ - Allow: auth.defaultPermissions.Publish.Allow, - Deny: auth.defaultPermissions.Publish.Deny, - }, - Export: &SubjectPermission{ - Allow: auth.defaultPermissions.Subscribe.Allow, - Deny: auth.defaultPermissions.Subscribe.Deny, - }, + Import: auth.defaultPermissions.Publish, + Export: auth.defaultPermissions.Subscribe, } } case "routes": diff --git a/server/route.go b/server/route.go index c70e8792..e7afa97d 100644 --- a/server/route.go +++ b/server/route.go @@ -498,14 +498,9 @@ func (c *client) setRoutePermissions(perms *RoutePermissions) { // The Import permission is mapped to Publish // and Export permission is mapped to Subscribe. // For meaning of Import/Export, see canImport and canExport. - p := &Permissions{} - p.Publish = &SubjectPermission{ - Allow: perms.Import.Allow, - Deny: perms.Import.Deny, - } - p.Subscribe = &SubjectPermission{ - Allow: perms.Export.Allow, - Deny: perms.Export.Deny, + p := &Permissions{ + Publish: perms.Import, + Subscribe: perms.Export, } c.setPermissions(p) } diff --git a/test/routes_test.go b/test/routes_test.go index 9c7662c7..8d8e5e39 100644 --- a/test/routes_test.go +++ b/test/routes_test.go @@ -18,6 +18,7 @@ import ( "fmt" "io/ioutil" "net" + "os" "runtime" "strconv" "strings" @@ -1050,4 +1051,130 @@ func TestRouteBasicPermissions(t *testing.T) { t.Fatal("Message should not have been received") case <-time.After(100 * time.Millisecond): } + + nca.Close() + ncb.Close() + + srvA.Shutdown() + srvB.Shutdown() + + optsA.Cluster.Permissions.Export = nil + srvA = RunServer(optsA) + defer srvA.Shutdown() + + srvB = RunServer(optsB) + defer srvB.Shutdown() + + checkClusterFormed(t, srvA, srvB) + + nca, err = nats.Connect(fmt.Sprintf("nats://127.0.0.1:%d", optsA.Port)) + if err != nil { + t.Fatalf("Error on connect: %v", err) + } + defer nca.Close() + // Subscribe on "bar" which is not imported + if _, err := nca.Subscribe("bar", cb); err != nil { + t.Fatalf("Error on subscribe: %v", err) + } + if err := checkExpectedSubs(1, srvA); err != nil { + t.Fatal(err.Error()) + } + + // Publish from B, should not be received + ncb, err = nats.Connect(fmt.Sprintf("nats://127.0.0.1:%d", optsB.Port)) + if err != nil { + t.Fatalf("Error on connect: %v", err) + } + defer ncb.Close() + if err := ncb.Publish("bar", []byte("hello")); err != nil { + t.Fatalf("Error on publish: %v", err) + } + select { + case <-ch: + t.Fatal("Message should not have been received") + case <-time.After(100 * time.Millisecond): + //ok + } + // Subscribe on "baz" on B + if _, err := ncb.Subscribe("baz", cb); err != nil { + t.Fatalf("Error on subscribe: %v", err) + } + if err := checkExpectedSubs(1, srvB); err != nil { + t.Fatal(err.Error()) + } + if err := checkExpectedSubs(2, srvA); err != nil { + t.Fatal(err.Error()) + } + // Publish from A, since there is no export restriction, message should be received. + if err := nca.Publish("baz", []byte("hello")); err != nil { + t.Fatalf("Error on publish: %v", err) + } + select { + case <-ch: + // ok + case <-time.After(250 * time.Millisecond): + t.Fatal("Message should have been received") + } +} + +func createConfFile(t *testing.T, content []byte) string { + t.Helper() + conf, err := ioutil.TempFile("", "") + if err != nil { + t.Fatalf("Error creating conf file: %v", err) + } + fName := conf.Name() + conf.Close() + if err := ioutil.WriteFile(fName, content, 0666); err != nil { + os.Remove(fName) + t.Fatalf("Error writing conf file: %v", err) + } + return fName +} + +func TestRoutesOnlyImportOrExport(t *testing.T) { + contents := []string{ + `import: "foo"`, + `import: { + allow: "foo" + }`, + `import: { + deny: "foo" + }`, + `import: { + allow: "foo" + deny: "foo" + }`, + `export: "foo"`, + `export: { + allow: "foo" + }`, + `export: { + deny: "foo" + }`, + `export: { + allow: "foo" + deny: "foo" + }`, + } + f := func(c string) { + cf := createConfFile(t, []byte(fmt.Sprintf(` + cluster { + port: -1 + authorization { + user: ivan + password: pwd + permissions { + %s + } + } + } + `, c))) + defer os.Remove(cf) + s, _ := RunServerWithConfig(cf) + s.Shutdown() + } + for _, c := range contents { + f(c) + } }