Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feature/plumbing: sub_repo_perms: do pumbling to thread comparing IP addresses alongside paths #64401

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

feature/plumbing: sub_repo_perms: do pumbling to thread comparing IP addresses alongside paths #64401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cmd/gitserver/internal/search/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ go_library(
"//internal/authz",
"//internal/byteutils",
"//internal/gitserver/protocol",
"//internal/requestclient",
"//internal/search/casetransform",
"//internal/search/result",
"//lib/errors",
Expand Down
4 changes: 3 additions & 1 deletion cmd/gitserver/internal/search/search.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/sourcegraph/sourcegraph/internal/api"
"github.com/sourcegraph/sourcegraph/internal/authz"
"github.com/sourcegraph/sourcegraph/internal/gitserver/protocol"
"github.com/sourcegraph/sourcegraph/internal/requestclient"
"github.com/sourcegraph/sourcegraph/internal/search/result"
"github.com/sourcegraph/sourcegraph/lib/errors"
)
Expand Down Expand Up @@ -224,8 +225,9 @@ func getSubRepoFilterFunc(ctx context.Context, checker authz.SubRepoPermissionCh
return nil
}
a := actor.FromContext(ctx)
ipSource := authz.NewRequestClientIPSource(requestclient.FromContext(ctx))
return func(filePath string) (bool, error) {
return authz.FilterActorPath(ctx, checker, a, repo, filePath)
return authz.FilterActorPath(ctx, checker, a, ipSource, repo, filePath)
}
}

Expand Down
1 change: 1 addition & 0 deletions internal/authz/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ go_library(
"//internal/collections",
"//internal/dotcom",
"//internal/extsvc",
"//internal/requestclient",
"//internal/types",
"//lib/errors",
"@com_github_prometheus_client_golang//prometheus",
Expand Down
32 changes: 18 additions & 14 deletions internal/authz/mocks_temp.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

99 changes: 75 additions & 24 deletions internal/authz/sub_repo_perms.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package authz
import (
"context"
"io/fs"
"net/netip"
"strconv"
"time"

Expand All @@ -11,6 +12,7 @@ import (

"github.com/sourcegraph/sourcegraph/internal/actor"
"github.com/sourcegraph/sourcegraph/internal/api"
"github.com/sourcegraph/sourcegraph/internal/requestclient"
"github.com/sourcegraph/sourcegraph/lib/errors"
)

Expand All @@ -26,7 +28,7 @@ type RepoContent struct {
// function is associated with a user and repository and should not be used
// beyond the lifetime of a single request. It exists to amortize the costs of
// setup when checking many files in a repository.
type FilePermissionFunc func(path string) (Perms, error)
type FilePermissionFunc func(path string, ip netip.Addr) (Perms, error)

// SubRepoPermissionChecker is the interface exposed by the SubRepoPermsClient and is
// exposed to allow consumers to mock out the client.
Expand All @@ -35,7 +37,7 @@ type SubRepoPermissionChecker interface {
// content.
//
// If the userID represents an anonymous user, ErrUnauthenticated is returned.
Permissions(ctx context.Context, userID int32, content RepoContent) (Perms, error)
Permissions(ctx context.Context, userID int32, ip netip.Addr, content RepoContent) (Perms, error)

// FilePermissionsFunc returns a FilePermissionFunc for userID in repo.
// This function should only be used during the lifetime of a request. It
Expand All @@ -62,12 +64,12 @@ var DefaultSubRepoPermsChecker SubRepoPermissionChecker = &noopPermsChecker{}

type noopPermsChecker struct{}

func (*noopPermsChecker) Permissions(_ context.Context, _ int32, _ RepoContent) (Perms, error) {
func (*noopPermsChecker) Permissions(_ context.Context, _ int32, _ netip.Addr, _ RepoContent) (Perms, error) {
return None, nil
}

func (*noopPermsChecker) FilePermissionsFunc(_ context.Context, _ int32, _ api.RepoName) (FilePermissionFunc, error) {
return func(path string) (Perms, error) {
return func(path string, ip netip.Addr) (Perms, error) {
return None, nil
}, nil
}
Expand All @@ -89,7 +91,7 @@ func (*noopPermsChecker) EnabledForRepo(_ context.Context, _ api.RepoName) (bool
//
// If the context is unauthenticated, ErrUnauthenticated is returned. If the context is
// internal, Read permissions is granted.
func ActorPermissions(ctx context.Context, s SubRepoPermissionChecker, a *actor.Actor, content RepoContent) (Perms, error) {
func ActorPermissions(ctx context.Context, s SubRepoPermissionChecker, a *actor.Actor, ipSource IPSource, content RepoContent) (Perms, error) {
// Check config here, despite checking again in the s.Permissions implementation,
// because we also make some permissions decisions here.
if doCheck, err := actorSubRepoEnabled(s, a); err != nil {
Expand All @@ -98,7 +100,12 @@ func ActorPermissions(ctx context.Context, s SubRepoPermissionChecker, a *actor.
return Read, nil
}

perms, err := s.Permissions(ctx, a.UID, content)
ip, err := ipSource.GetIP()
if err != nil {
return None, errors.Wrap(err, "getting the IP address for checking permissions")
}

perms, err := s.Permissions(ctx, a.UID, ip, content)
if err != nil {
return None, errors.Wrapf(err, "getting actor permissions for actor: %d", a.UID)
}
Expand Down Expand Up @@ -156,14 +163,18 @@ var (
}, []string{"any", "result"})
)

func canReadPaths(ctx context.Context, checker SubRepoPermissionChecker, repo api.RepoName, paths []string, any bool) (result bool, err error) {
a := actor.FromContext(ctx)
func canReadPaths(ctx context.Context, checker SubRepoPermissionChecker, a *actor.Actor, ipSource IPSource, repo api.RepoName, paths []string, any bool) (result bool, err error) {
if doCheck, err := actorSubRepoEnabled(checker, a); err != nil {
return false, err
} else if !doCheck {
return true, nil
}

ip, err := ipSource.GetIP()
if err != nil {
return false, errors.Wrap(err, "getting the IP address for checking permissions")
}

start := time.Now()
var checkPathPermsCount int
defer func() {
Expand All @@ -181,7 +192,7 @@ func canReadPaths(ctx context.Context, checker SubRepoPermissionChecker, repo ap

for _, p := range paths {
checkPathPermsCount++
perms, err := checkPathPerms(p)
perms, err := checkPathPerms(p, ip)
if err != nil {
return false, err
}
Expand All @@ -196,13 +207,13 @@ func canReadPaths(ctx context.Context, checker SubRepoPermissionChecker, repo ap
}

// CanReadAllPaths returns true if the actor can read all paths.
func CanReadAllPaths(ctx context.Context, checker SubRepoPermissionChecker, repo api.RepoName, paths []string) (bool, error) {
return canReadPaths(ctx, checker, repo, paths, false)
func CanReadAllPaths(ctx context.Context, checker SubRepoPermissionChecker, a *actor.Actor, ipSource IPSource, repo api.RepoName, paths []string) (bool, error) {
return canReadPaths(ctx, checker, a, ipSource, repo, paths, false)
}

// CanReadAnyPath returns true if the actor can read any path in the list of paths.
func CanReadAnyPath(ctx context.Context, checker SubRepoPermissionChecker, repo api.RepoName, paths []string) (bool, error) {
return canReadPaths(ctx, checker, repo, paths, true)
func CanReadAnyPath(ctx context.Context, checker SubRepoPermissionChecker, a *actor.Actor, ipSource IPSource, repo api.RepoName, paths []string) (bool, error) {
return canReadPaths(ctx, checker, a, ipSource, repo, paths, true)
}

var (
Expand All @@ -218,13 +229,18 @@ var (

// FilterActorPaths will filter the given list of paths for the given actor
// returning on paths they are allowed to read.
func FilterActorPaths(ctx context.Context, checker SubRepoPermissionChecker, a *actor.Actor, repo api.RepoName, paths []string) (_ []string, err error) {
func FilterActorPaths(ctx context.Context, checker SubRepoPermissionChecker, a *actor.Actor, ipSource IPSource, repo api.RepoName, paths []string) (_ []string, err error) {
if doCheck, err := actorSubRepoEnabled(checker, a); err != nil {
return nil, errors.Wrap(err, "checking sub-repo permissions")
} else if !doCheck {
return paths, nil
}

ip, err := ipSource.GetIP()
if err != nil {
return nil, errors.Wrap(err, "getting IP address for filtering actor paths")
}

start := time.Now()
var checkPathPermsCount int
defer func() {
Expand All @@ -240,7 +256,7 @@ func FilterActorPaths(ctx context.Context, checker SubRepoPermissionChecker, a *
filtered := make([]string, 0, len(paths))
for _, p := range paths {
checkPathPermsCount++
perms, err := checkPathPerms(p)
perms, err := checkPathPerms(p, ip)
if err != nil {
return nil, errors.Wrap(err, "checking sub-repo permissions")
}
Expand All @@ -253,27 +269,34 @@ func FilterActorPaths(ctx context.Context, checker SubRepoPermissionChecker, a *

// FilterActorPath will filter the given path for the given actor
// returning true if the path is allowed to read.
func FilterActorPath(ctx context.Context, checker SubRepoPermissionChecker, a *actor.Actor, repo api.RepoName, path string) (bool, error) {
func FilterActorPath(ctx context.Context, checker SubRepoPermissionChecker, a *actor.Actor, ipSource IPSource, repo api.RepoName, path string) (bool, error) {
if !SubRepoEnabled(checker) {
return true, nil
}
perms, err := ActorPermissions(ctx, checker, a, RepoContent{
Repo: repo,
Path: path,
})

perms, err := ActorPermissions(ctx, checker, a, ipSource,
RepoContent{
Repo: repo,
Path: path,
})
if err != nil {
return false, errors.Wrap(err, "checking sub-repo permissions")
}
return perms.Include(Read), nil
}

func FilterActorFileInfos(ctx context.Context, checker SubRepoPermissionChecker, a *actor.Actor, repo api.RepoName, fis []fs.FileInfo) (_ []fs.FileInfo, err error) {
func FilterActorFileInfos(ctx context.Context, checker SubRepoPermissionChecker, a *actor.Actor, ipSource IPSource, repo api.RepoName, fis []fs.FileInfo) (_ []fs.FileInfo, err error) {
if doCheck, err := actorSubRepoEnabled(checker, a); err != nil {
return nil, errors.Wrap(err, "checking sub-repo permissions")
} else if !doCheck {
return fis, nil
}

ip, err := ipSource.GetIP()
if err != nil {
return nil, errors.Wrap(err, "getting IP address for filtering actor paths")
}

start := time.Now()
var checkPathPermsCount int
defer func() {
Expand All @@ -291,7 +314,7 @@ func FilterActorFileInfos(ctx context.Context, checker SubRepoPermissionChecker,
filtered := make([]fs.FileInfo, 0, len(fis))
for _, fi := range fis {
checkPathPermsCount++
perms, err := checkPathPerms(fileInfoPath(fi))
perms, err := checkPathPerms(fileInfoPath(fi), ip)
if err != nil {
return nil, err
}
Expand All @@ -302,12 +325,12 @@ func FilterActorFileInfos(ctx context.Context, checker SubRepoPermissionChecker,
return filtered, nil
}

func FilterActorFileInfo(ctx context.Context, checker SubRepoPermissionChecker, a *actor.Actor, repo api.RepoName, fi fs.FileInfo) (bool, error) {
func FilterActorFileInfo(ctx context.Context, checker SubRepoPermissionChecker, a *actor.Actor, ipSource IPSource, repo api.RepoName, fi fs.FileInfo) (bool, error) {
rc := RepoContent{
Repo: repo,
Path: fileInfoPath(fi),
}
perms, err := ActorPermissions(ctx, checker, a, rc)
perms, err := ActorPermissions(ctx, checker, a, ipSource, rc)
if err != nil {
return false, errors.Wrap(err, "checking sub-repo permissions")
}
Expand All @@ -322,3 +345,31 @@ func fileInfoPath(fi fs.FileInfo) string {
}
return fi.Name()
}

type IPSource interface {
GetIP() (netip.Addr, error)
}

type clientIPSource struct {
client *requestclient.Client
}

func (c *clientIPSource) GetIP() (netip.Addr, error) {
if c.client == nil {
return netip.Addr{}, errors.New("client is nil")
}

return fakeIP, nil // TODO@ggilmore: Replace this with the real IP extraction logic from the client
}

var fakeIP = netip.MustParseAddr("127.0.0.1") // TODO@ggimore: Fake ip address used until we thread the real one through.

func NewRequestClientIPSource(client *requestclient.Client) IPSource {
return &clientIPSource{client: client}
}

type IPSourceFunc func() (netip.Addr, error)

func (f IPSourceFunc) GetIP() (netip.Addr, error) {
return f()
}
Loading
Loading