From 0ed6b1bb18da8a8487132627241803057addf501 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Sat, 10 Dec 2016 11:19:08 -0800 Subject: [PATCH] Bans anonymous queries that aren't tied to a session. This gets us coverage of PQ creation under the existing service policy or the soon-to-be-added session policy. --- api/prepared_query_test.go | 1 + command/agent/dns_test.go | 13 +++ consul/prepared_query_endpoint.go | 12 ++- consul/prepared_query_endpoint_test.go | 127 ++++++++++++++++++------- 4 files changed, 117 insertions(+), 36 deletions(-) diff --git a/api/prepared_query_test.go b/api/prepared_query_test.go index d16f007c9d..c9adb5b2c2 100644 --- a/api/prepared_query_test.go +++ b/api/prepared_query_test.go @@ -45,6 +45,7 @@ func TestPreparedQuery(t *testing.T) { // Create a simple prepared query. def := &PreparedQueryDefinition{ + Name: "test", Service: ServiceQuery{ Service: "redis", }, diff --git a/command/agent/dns_test.go b/command/agent/dns_test.go index be9c56c2a7..8eabfa804f 100644 --- a/command/agent/dns_test.go +++ b/command/agent/dns_test.go @@ -569,6 +569,7 @@ func TestDNS_ServiceLookup(t *testing.T) { Datacenter: "dc1", Op: structs.PreparedQueryCreate, Query: &structs.PreparedQuery{ + Name: "test", Service: structs.ServiceQuery{ Service: "db", }, @@ -1021,6 +1022,7 @@ func TestDNS_ServiceLookup_ServiceAddress(t *testing.T) { Datacenter: "dc1", Op: structs.PreparedQueryCreate, Query: &structs.PreparedQuery{ + Name: "test", Service: structs.ServiceQuery{ Service: "db", }, @@ -1115,6 +1117,7 @@ func TestDNS_ServiceLookup_ServiceAddressIPV6(t *testing.T) { Datacenter: "dc1", Op: structs.PreparedQueryCreate, Query: &structs.PreparedQuery{ + Name: "test", Service: structs.ServiceQuery{ Service: "db", }, @@ -1236,6 +1239,7 @@ func TestDNS_ServiceLookup_WanAddress(t *testing.T) { Datacenter: "dc2", Op: structs.PreparedQueryCreate, Query: &structs.PreparedQuery{ + Name: "test", Service: structs.ServiceQuery{ Service: "db", }, @@ -1641,6 +1645,7 @@ func TestDNS_ServiceLookup_Dedup(t *testing.T) { Datacenter: "dc1", Op: structs.PreparedQueryCreate, Query: &structs.PreparedQuery{ + Name: "test", Service: structs.ServiceQuery{ Service: "db", }, @@ -1745,6 +1750,7 @@ func TestDNS_ServiceLookup_Dedup_SRV(t *testing.T) { Datacenter: "dc1", Op: structs.PreparedQueryCreate, Query: &structs.PreparedQuery{ + Name: "test", Service: structs.ServiceQuery{ Service: "db", }, @@ -2040,6 +2046,7 @@ func TestDNS_ServiceLookup_FilterCritical(t *testing.T) { Datacenter: "dc1", Op: structs.PreparedQueryCreate, Query: &structs.PreparedQuery{ + Name: "test", Service: structs.ServiceQuery{ Service: "db", }, @@ -2163,6 +2170,7 @@ func TestDNS_ServiceLookup_OnlyFailing(t *testing.T) { Datacenter: "dc1", Op: structs.PreparedQueryCreate, Query: &structs.PreparedQuery{ + Name: "test", Service: structs.ServiceQuery{ Service: "db", }, @@ -2283,6 +2291,7 @@ func TestDNS_ServiceLookup_OnlyPassing(t *testing.T) { Datacenter: "dc1", Op: structs.PreparedQueryCreate, Query: &structs.PreparedQuery{ + Name: "test", Service: structs.ServiceQuery{ Service: "db", OnlyPassing: true, @@ -2356,6 +2365,7 @@ func TestDNS_ServiceLookup_Randomize(t *testing.T) { Datacenter: "dc1", Op: structs.PreparedQueryCreate, Query: &structs.PreparedQuery{ + Name: "test", Service: structs.ServiceQuery{ Service: "web", }, @@ -2450,6 +2460,7 @@ func TestDNS_ServiceLookup_Truncate(t *testing.T) { Datacenter: "dc1", Op: structs.PreparedQueryCreate, Query: &structs.PreparedQuery{ + Name: "test", Service: structs.ServiceQuery{ Service: "web", }, @@ -2770,6 +2781,7 @@ func TestDNS_ServiceLookup_CNAME(t *testing.T) { Datacenter: "dc1", Op: structs.PreparedQueryCreate, Query: &structs.PreparedQuery{ + Name: "test", Service: structs.ServiceQuery{ Service: "search", }, @@ -4342,6 +4354,7 @@ func TestDNS_Compression_Query(t *testing.T) { Datacenter: "dc1", Op: structs.PreparedQueryCreate, Query: &structs.PreparedQuery{ + Name: "test", Service: structs.ServiceQuery{ Service: "db", }, diff --git a/consul/prepared_query_endpoint.go b/consul/prepared_query_endpoint.go index 47b6fe1a77..b73a4c848a 100644 --- a/consul/prepared_query_endpoint.go +++ b/consul/prepared_query_endpoint.go @@ -96,7 +96,7 @@ func (p *PreparedQuery) Apply(args *structs.PreparedQueryRequest, reply *string) // Parse the query and prep it for the state store. switch args.Op { case structs.PreparedQueryCreate, structs.PreparedQueryUpdate: - if err := parseQuery(args.Query); err != nil { + if err := parseQuery(args.Query, p.srv.config.ACLEnforceVersion8); err != nil { return fmt.Errorf("Invalid prepared query: %v", err) } @@ -125,7 +125,7 @@ func (p *PreparedQuery) Apply(args *structs.PreparedQueryRequest, reply *string) // update operation. Some of the fields are not checked or are partially // checked, as noted in the comments below. This also updates all the parsed // fields of the query. -func parseQuery(query *structs.PreparedQuery) error { +func parseQuery(query *structs.PreparedQuery, enforceVersion8 bool) error { // We skip a few fields: // - ID is checked outside this fn. // - Name is optional with no restrictions, except for uniqueness which @@ -133,10 +133,16 @@ func parseQuery(query *structs.PreparedQuery) error { // names do not overlap with IDs, which is also checked during the // transaction. Otherwise, people could "steal" queries that they don't // have proper ACL rights to change. - // - Session is optional and checked for integrity during the transaction. // - Template is checked during the transaction since that's where we // compile it. + // Anonymous queries require a session or need to be part of a template. + if enforceVersion8 { + if query.Name == "" && query.Template.Type == "" && query.Session == "" { + return fmt.Errorf("Must be bound to a session") + } + } + // Token is checked when the query is executed, but we do make sure the // user hasn't accidentally pasted-in the special redacted token name, // which if we allowed in would be super hard to debug and understand. diff --git a/consul/prepared_query_endpoint_test.go b/consul/prepared_query_endpoint_test.go index d23b627b9c..f3120892e3 100644 --- a/consul/prepared_query_endpoint_test.go +++ b/consul/prepared_query_endpoint_test.go @@ -32,6 +32,7 @@ func TestPreparedQuery_Apply(t *testing.T) { Datacenter: "dc1", Op: structs.PreparedQueryCreate, Query: &structs.PreparedQuery{ + Name: "test", Service: structs.ServiceQuery{ Service: "redis", }, @@ -515,6 +516,7 @@ func TestPreparedQuery_Apply_ForwardLeader(t *testing.T) { Datacenter: "dc1", Op: structs.PreparedQueryCreate, Query: &structs.PreparedQuery{ + Name: "test", Service: structs.ServiceQuery{ Service: "redis", }, @@ -531,53 +533,77 @@ func TestPreparedQuery_Apply_ForwardLeader(t *testing.T) { func TestPreparedQuery_parseQuery(t *testing.T) { query := &structs.PreparedQuery{} - err := parseQuery(query) - if err == nil || !strings.Contains(err.Error(), "Must provide a Service") { + err := parseQuery(query, true) + if err == nil || !strings.Contains(err.Error(), "Must be bound to a session") { t.Fatalf("bad: %v", err) } - query.Service.Service = "foo" - if err := parseQuery(query); err != nil { - t.Fatalf("err: %v", err) - } - - query.Token = redactedToken - err = parseQuery(query) - if err == nil || !strings.Contains(err.Error(), "Bad Token") { + query.Session = "adf4238a-882b-9ddc-4a9d-5b6758e4159e" + err = parseQuery(query, true) + if err == nil || !strings.Contains(err.Error(), "Must provide a Service") { t.Fatalf("bad: %v", err) } - query.Token = "adf4238a-882b-9ddc-4a9d-5b6758e4159e" - if err := parseQuery(query); err != nil { - t.Fatalf("err: %v", err) + query.Session = "" + query.Template.Type = "some-kind-of-template" + err = parseQuery(query, true) + if err == nil || !strings.Contains(err.Error(), "Must provide a Service") { + t.Fatalf("bad: %v", err) } - query.Service.Failover.NearestN = -1 - err = parseQuery(query) - if err == nil || !strings.Contains(err.Error(), "Bad NearestN") { + query.Template.Type = "" + err = parseQuery(query, false) + if err == nil || !strings.Contains(err.Error(), "Must provide a Service") { t.Fatalf("bad: %v", err) } - query.Service.Failover.NearestN = 3 - if err := parseQuery(query); err != nil { - t.Fatalf("err: %v", err) - } + // None of the rest of these care about version 8 ACL enforcement. + for _, version8 := range []bool{true, false} { + query = &structs.PreparedQuery{} + query.Session = "adf4238a-882b-9ddc-4a9d-5b6758e4159e" + query.Service.Service = "foo" + if err := parseQuery(query, version8); err != nil { + t.Fatalf("err: %v", err) + } - query.DNS.TTL = "two fortnights" - err = parseQuery(query) - if err == nil || !strings.Contains(err.Error(), "Bad DNS TTL") { - t.Fatalf("bad: %v", err) - } + query.Token = redactedToken + err = parseQuery(query, version8) + if err == nil || !strings.Contains(err.Error(), "Bad Token") { + t.Fatalf("bad: %v", err) + } - query.DNS.TTL = "-3s" - err = parseQuery(query) - if err == nil || !strings.Contains(err.Error(), "must be >=0") { - t.Fatalf("bad: %v", err) - } + query.Token = "adf4238a-882b-9ddc-4a9d-5b6758e4159e" + if err := parseQuery(query, version8); err != nil { + t.Fatalf("err: %v", err) + } - query.DNS.TTL = "3s" - if err := parseQuery(query); err != nil { - t.Fatalf("err: %v", err) + query.Service.Failover.NearestN = -1 + err = parseQuery(query, version8) + if err == nil || !strings.Contains(err.Error(), "Bad NearestN") { + t.Fatalf("bad: %v", err) + } + + query.Service.Failover.NearestN = 3 + if err := parseQuery(query, version8); err != nil { + t.Fatalf("err: %v", err) + } + + query.DNS.TTL = "two fortnights" + err = parseQuery(query, version8) + if err == nil || !strings.Contains(err.Error(), "Bad DNS TTL") { + t.Fatalf("bad: %v", err) + } + + query.DNS.TTL = "-3s" + err = parseQuery(query, version8) + if err == nil || !strings.Contains(err.Error(), "must be >=0") { + t.Fatalf("bad: %v", err) + } + + query.DNS.TTL = "3s" + if err := parseQuery(query, version8); err != nil { + t.Fatalf("err: %v", err) + } } } @@ -917,9 +943,25 @@ func TestPreparedQuery_Get(t *testing.T) { } } + // Create a session. + var session string + { + req := structs.SessionRequest{ + Datacenter: "dc1", + Op: structs.SessionCreate, + Session: structs.Session{ + Node: s1.config.NodeName, + }, + } + if err := msgpackrpc.CallWithCodec(codec, "Session.Apply", &req, &session); err != nil { + t.Fatalf("err: %v", err) + } + } + // Now update the query to take away its name. query.Op = structs.PreparedQueryUpdate query.Query.Name = "" + query.Query.Session = session if err := msgpackrpc.CallWithCodec(codec, "PreparedQuery.Apply", &query, &reply); err != nil { t.Fatalf("err: %v", err) } @@ -1170,9 +1212,25 @@ func TestPreparedQuery_List(t *testing.T) { } } + // Create a session. + var session string + { + req := structs.SessionRequest{ + Datacenter: "dc1", + Op: structs.SessionCreate, + Session: structs.Session{ + Node: s1.config.NodeName, + }, + } + if err := msgpackrpc.CallWithCodec(codec, "Session.Apply", &req, &session); err != nil { + t.Fatalf("err: %v", err) + } + } + // Now take away the query name. query.Op = structs.PreparedQueryUpdate query.Query.Name = "" + query.Query.Session = session if err := msgpackrpc.CallWithCodec(codec, "PreparedQuery.Apply", &query, &reply); err != nil { t.Fatalf("err: %v", err) } @@ -1451,6 +1509,7 @@ func TestPreparedQuery_Execute(t *testing.T) { Datacenter: "dc1", Op: structs.PreparedQueryCreate, Query: &structs.PreparedQuery{ + Name: "test", Service: structs.ServiceQuery{ Service: "foo", }, @@ -2314,6 +2373,7 @@ func TestPreparedQuery_Execute_ForwardLeader(t *testing.T) { Datacenter: "dc1", Op: structs.PreparedQueryCreate, Query: &structs.PreparedQuery{ + Name: "test", Service: structs.ServiceQuery{ Service: "redis", }, @@ -2577,6 +2637,7 @@ func (m *mockQueryServer) ForwardDC(method, dc string, args interface{}, reply i func TestPreparedQuery_queryFailover(t *testing.T) { query := &structs.PreparedQuery{ + Name: "test", Service: structs.ServiceQuery{ Failover: structs.QueryDatacenterOptions{ NearestN: 0,