Skip to content

Commit

Permalink
Merge pull request #103751 from jaylim-crl/backport23.1-101864-103070…
Browse files Browse the repository at this point in the history
…-103479-103487-103550-103625

release-23.1: ccl/sqlproxyccl: add CIDR Ranges and Private Endpoints ACL support
  • Loading branch information
jaylim-crl authored May 23, 2023
2 parents 4ae616d + 768d496 commit 65c2fd9
Show file tree
Hide file tree
Showing 28 changed files with 2,478 additions and 510 deletions.
2 changes: 1 addition & 1 deletion pkg/ccl/sqlproxyccl/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@ go_test(
"//pkg/testutils",
"//pkg/testutils/datapathutils",
"//pkg/testutils/serverutils",
"//pkg/testutils/skip",
"//pkg/testutils/sqlutils",
"//pkg/testutils/testcluster",
"//pkg/util/leaktest",
Expand All @@ -118,6 +117,7 @@ go_test(
"@com_github_jackc_pgproto3_v2//:pgproto3",
"@com_github_jackc_pgx_v4//:pgx",
"@com_github_pires_go_proxyproto//:go-proxyproto",
"@com_github_pires_go_proxyproto//tlvparse",
"@com_github_stretchr_testify//assert",
"@com_github_stretchr_testify//require",
"@in_gopkg_yaml_v3//:yaml_v3",
Expand Down
14 changes: 14 additions & 0 deletions pkg/ccl/sqlproxyccl/acl/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -6,38 +6,52 @@ go_library(
srcs = [
"access_control.go",
"allowlist.go",
"cidr_ranges.go",
"denylist.go",
"file.go",
"private_endpoints.go",
"watcher.go",
],
importpath = "github.com/cockroachdb/cockroach/pkg/ccl/sqlproxyccl/acl",
visibility = ["//visibility:public"],
deps = [
"//pkg/ccl/sqlproxyccl/tenant",
"//pkg/roachpb",
"//pkg/util/log",
"//pkg/util/metric",
"//pkg/util/syncutil",
"//pkg/util/timeutil",
"@com_github_cockroachdb_errors//:errors",
"@com_github_google_btree//:btree",
"@com_github_pires_go_proxyproto//:go-proxyproto",
"@com_github_pires_go_proxyproto//tlvparse",
"@in_gopkg_yaml_v2//:yaml_v2",
],
)

go_test(
name = "acl_test",
srcs = [
"cidr_ranges_test.go",
"file_test.go",
"private_endpoints_test.go",
"watcher_test.go",
],
args = ["-test.timeout=295s"],
embed = [":acl"],
deps = [
"//pkg/ccl/sqlproxyccl/tenant",
"//pkg/ccl/sqlproxyccl/tenantdirsvr",
"//pkg/roachpb",
"//pkg/testutils",
"//pkg/util/leaktest",
"//pkg/util/metric",
"//pkg/util/stop",
"//pkg/util/timeutil",
"@com_github_cockroachdb_errors//:errors",
"@com_github_google_btree//:btree",
"@com_github_pires_go_proxyproto//:go-proxyproto",
"@com_github_pires_go_proxyproto//tlvparse",
"@com_github_stretchr_testify//require",
"@in_gopkg_yaml_v2//:yaml_v2",
],
Expand Down
26 changes: 21 additions & 5 deletions pkg/ccl/sqlproxyccl/acl/access_control.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,30 @@

package acl

import "github.com/cockroachdb/cockroach/pkg/util/timeutil"
import (
"context"

// ConnectionTags contains connection properties to match against the denylist.
"github.com/cockroachdb/cockroach/pkg/roachpb"
)

// ConnectionTags contains connection properties to match against the ACLs.
type ConnectionTags struct {
IP string
Cluster string
// IP corresponds to the address of the connection.
IP string
// TODO(jaylim-crl): We're using TenantID to identify tenant for now. Once
// all the directory cache APIs are using TenantNames, we should modify this
// to use that too.
TenantID roachpb.TenantID
// EndpointID corresponds to the identifier of the private connection, if
// one was used. This will be an empty string if the connection is from
// the public internet. It is assumed that the connection is from a private
// network if the PROXY headers include cloud provider endpoint identifiers.
EndpointID string
}

type AccessController interface {
CheckConnection(ConnectionTags, timeutil.TimeSource) error
// CheckConnection is used to indicate whether the given connection is
// allowed to maintain a connection with the proxy. This returns an error
// if the connection should be blocked, or nil otherwise.
CheckConnection(context.Context, ConnectionTags) error
}
27 changes: 20 additions & 7 deletions pkg/ccl/sqlproxyccl/acl/allowlist.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,23 +9,28 @@
package acl

import (
"context"
"net"

"github.com/cockroachdb/cockroach/pkg/util/timeutil"
"github.com/cockroachdb/errors"
"gopkg.in/yaml.v2"
)

type AllowlistFile struct {
Seq int64 `yaml:"SequenceNumber"`
Allowlist map[string]AllowEntry `yaml:"allowlist"`
}

// Allowlist represents the current IP Allowlist,
// which maps cluster IDs to a list of allowed IP ranges.
// Allowlist represents the current IP Allowlist, which maps tenant IDs to a
// list of allowed IP ranges.
type Allowlist struct {
entries map[string]AllowEntry
}

var _ AccessController = &Allowlist{}
var _ yaml.Unmarshaler = &Allowlist{}

// UnmarshalYAML implements the yaml.Unmarshaler interface.
func (al *Allowlist) UnmarshalYAML(unmarshal func(interface{}) error) error {
var f AllowlistFile
if err := unmarshal(&f); err != nil {
Expand All @@ -35,10 +40,14 @@ func (al *Allowlist) UnmarshalYAML(unmarshal func(interface{}) error) error {
return nil
}

func (al *Allowlist) CheckConnection(
connection ConnectionTags, timeSource timeutil.TimeSource,
) error {
entry, ok := al.entries[connection.Cluster]
// CheckConnection implements the AccessController interface.
//
// TODO(jaylim-crl): Call LookupTenant and return nil if the cluster has no
// public connectivity. This ACL shouldn't be applied. We would need to do this
// eventually once we move IP allowlist entries into the tenant object. Don't
// need to do this now as we don't need anything from the tenant metadata.
func (al *Allowlist) CheckConnection(ctx context.Context, connection ConnectionTags) error {
entry, ok := al.entries[connection.TenantID.String()]
if !ok {
// No allowlist entry, allow all traffic
return nil
Expand All @@ -62,8 +71,12 @@ type AllowEntry struct {
ips []*net.IPNet
}

var _ yaml.Unmarshaler = &AllowEntry{}

// This custom unmarshal code converts each string IP address into a *net.IPNet.
// If it cannot be parsed, it is currently ignored and not added to the AllowEntry.
//
// UnmarshalYAML implements the yaml.Unmarshaler interface.
func (e *AllowEntry) UnmarshalYAML(unmarshal func(interface{}) error) error {
var raw struct {
IPs []string `yaml:"ips"`
Expand Down
68 changes: 68 additions & 0 deletions pkg/ccl/sqlproxyccl/acl/cidr_ranges.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
// Copyright 2023 The Cockroach Authors.
//
// Licensed as a CockroachDB Enterprise file under the Cockroach Community
// License (the "License"); you may not use this file except in compliance with
// the License. You may obtain a copy of the License at
//
// https://github.com/cockroachdb/cockroach/blob/master/licenses/CCL.txt

package acl

import (
"context"
"net"

"github.com/cockroachdb/errors"
)

// CIDRRanges represents the controller used to manage ACL rules for public
// connections. It rejects connections if:
// 1. the cluster does not allow public connections, or
// 2. none of the allowed_cidr_ranges entries match the incoming connection's
// IP.
type CIDRRanges struct {
LookupTenantFn lookupTenantFunc
}

var _ AccessController = &CIDRRanges{}

// CheckConnection implements the AccessController interface.
func (p *CIDRRanges) CheckConnection(ctx context.Context, conn ConnectionTags) error {
tenantObj, err := p.LookupTenantFn(ctx, conn.TenantID)
if err != nil {
return err
}

// Private connections. This ACL is only responsible for public CIDR ranges.
if conn.EndpointID != "" {
return nil
}

// Cluster allows public connections, so we'll check allowed CIDR ranges.
if tenantObj.AllowPublicConn() {
ip := net.ParseIP(conn.IP)
if ip == nil {
return errors.Newf("could not parse IP address: '%s'", conn.IP)
}
for _, cidrRange := range tenantObj.AllowedCIDRRanges {
// It is assumed that all public CIDR ranges are valid, so the
// tenant directory server will have to enforce that.
_, ipNetwork, err := net.ParseCIDR(cidrRange)
if err != nil {
return err
}
// A matching CIDR range was found.
if ipNetwork.Contains(ip) {
return nil
}
}
}

// By default, connections are rejected if the cluster does not allow public
// connections, or if no ranges match the connection's IP.
return errors.Newf(
"connection to '%s' denied: cluster does not allow public connections from IP %s",
conn.TenantID.String(),
conn.IP,
)
}
140 changes: 140 additions & 0 deletions pkg/ccl/sqlproxyccl/acl/cidr_ranges_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
// Copyright 2023 The Cockroach Authors.
//
// Licensed as a CockroachDB Enterprise file under the Cockroach Community
// License (the "License"); you may not use this file except in compliance with
// the License. You may obtain a copy of the License at
//
// https://github.com/cockroachdb/cockroach/blob/master/licenses/CCL.txt

package acl_test

import (
"context"
"testing"

"github.com/cockroachdb/cockroach/pkg/ccl/sqlproxyccl/acl"
"github.com/cockroachdb/cockroach/pkg/ccl/sqlproxyccl/tenant"
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/util/leaktest"
"github.com/cockroachdb/errors"
"github.com/stretchr/testify/require"
)

func TestCIDRRanges(t *testing.T) {
defer leaktest.AfterTest(t)()
ctx := context.Background()

tenantID := roachpb.MustMakeTenantID(42)
makeConn := func(endpoint string) acl.ConnectionTags {
return acl.ConnectionTags{
IP: "127.0.0.1",
TenantID: tenantID,
EndpointID: endpoint,
}
}

t.Run("lookup error", func(t *testing.T) {
p := &acl.CIDRRanges{
LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
return nil, errors.New("foo")
},
}
err := p.CheckConnection(ctx, makeConn(""))
require.EqualError(t, err, "foo")
})

// Private connection should allow, despite not having any CIDR ranges.
t.Run("private connection", func(t *testing.T) {
p := &acl.CIDRRanges{
LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
return &tenant.Tenant{
ConnectivityType: tenant.ALLOW_PUBLIC_ONLY,
}, nil
},
}
err := p.CheckConnection(ctx, makeConn("foo"))
require.NoError(t, err)
})

// CIDR ranges do not match.
t.Run("bad public connection", func(t *testing.T) {
p := &acl.CIDRRanges{
LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
return &tenant.Tenant{
ConnectivityType: tenant.ALLOW_ALL,
AllowedCIDRRanges: []string{"127.0.0.0/32", "10.0.0.8/16"},
}, nil
},
}
err := p.CheckConnection(ctx, makeConn(""))
require.EqualError(t, err, "connection to '42' denied: cluster does not allow public connections from IP 127.0.0.1")
})

t.Run("default behavior if no entries", func(t *testing.T) {
p := &acl.CIDRRanges{
LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
return &tenant.Tenant{
ConnectivityType: tenant.ALLOW_ALL,
AllowedCIDRRanges: []string{},
}, nil
},
}
err := p.CheckConnection(ctx, makeConn(""))
require.EqualError(t, err, "connection to '42' denied: cluster does not allow public connections from IP 127.0.0.1")
})

t.Run("good public connection", func(t *testing.T) {
p := &acl.CIDRRanges{
LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
return &tenant.Tenant{
ConnectivityType: tenant.ALLOW_ALL,
AllowedCIDRRanges: []string{"0.0.0.0/0"},
}, nil
},
}
err := p.CheckConnection(ctx, makeConn(""))
require.NoError(t, err)
})

t.Run("disallow public connections", func(t *testing.T) {
p := &acl.CIDRRanges{
LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
return &tenant.Tenant{
ConnectivityType: tenant.ALLOW_PRIVATE_ONLY,
AllowedCIDRRanges: []string{"127.0.0.1/32"},
}, nil
},
}
err := p.CheckConnection(ctx, makeConn(""))
require.EqualError(t, err, "connection to '42' denied: cluster does not allow public connections from IP 127.0.0.1")
})

t.Run("could not parse connection IP", func(t *testing.T) {
p := &acl.CIDRRanges{
LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
return &tenant.Tenant{
ConnectivityType: tenant.ALLOW_ALL,
AllowedCIDRRanges: []string{"127.0.0.1/32"},
}, nil
},
}
err := p.CheckConnection(ctx, acl.ConnectionTags{
IP: "invalid-value",
TenantID: tenantID,
})
require.EqualError(t, err, "could not parse IP address: 'invalid-value'")
})

t.Run("could not parse CIDR range", func(t *testing.T) {
p := &acl.CIDRRanges{
LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
return &tenant.Tenant{
ConnectivityType: tenant.ALLOW_ALL,
AllowedCIDRRanges: []string{"127.0.0.1"},
}, nil
},
}
err := p.CheckConnection(ctx, makeConn(""))
require.EqualError(t, err, "invalid CIDR address: 127.0.0.1")
})
}
Loading

0 comments on commit 65c2fd9

Please sign in to comment.