From c0f5756ae97dcfd425d41d555688113ba4b2b46a Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 5 Apr 2018 12:58:06 +0200 Subject: [PATCH 01/18] WIP grpc test --- internal/service/deploy/.gitignore | 1 + internal/service/deploy/deploy.go | 3 +++ internal/service/deploy/deploy_test.go | 26 ++++++++++++++++++++++++++ 3 files changed, 30 insertions(+) create mode 100644 internal/service/deploy/.gitignore create mode 100644 internal/service/deploy/deploy.go create mode 100644 internal/service/deploy/deploy_test.go diff --git a/internal/service/deploy/.gitignore b/internal/service/deploy/.gitignore new file mode 100644 index 000000000..ace1063ab --- /dev/null +++ b/internal/service/deploy/.gitignore @@ -0,0 +1 @@ +/testdata diff --git a/internal/service/deploy/deploy.go b/internal/service/deploy/deploy.go new file mode 100644 index 000000000..342401766 --- /dev/null +++ b/internal/service/deploy/deploy.go @@ -0,0 +1,3 @@ +package deploy + +type server struct{} diff --git a/internal/service/deploy/deploy_test.go b/internal/service/deploy/deploy_test.go new file mode 100644 index 000000000..18f2483b0 --- /dev/null +++ b/internal/service/deploy/deploy_test.go @@ -0,0 +1,26 @@ +package deploy + +import ( + "net" + "testing" + + pb "gitlab.com/gitlab-org/gitlab-pages-proto/go" + "google.golang.org/grpc" +) + +func runDeployServer(t *testing.T) (*grpc.Server, string) { + grpcServer := grpc.NewServer() + + serverSocketPath := "testdata/grpc.socket" + listener, err := net.Listen("unix", serverSocketPath) + + if err != nil { + t.Fatal(err) + } + + pb.RegisterDeployServiceServer(grpcServer, &server{}) + + go grpcServer.Serve(listener) + + return grpcServer, serverSocketPath +} -- GitLab From c356b730338d76a45c81612d5f051dcb567f61c7 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 5 Apr 2018 13:43:36 +0200 Subject: [PATCH 02/18] Add rpc call in test --- internal/service/deploy/deploy.go | 11 +++++++ internal/service/deploy/deploy_test.go | 43 ++++++++++++++++++++++++-- 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/internal/service/deploy/deploy.go b/internal/service/deploy/deploy.go index 342401766..ad2dbdb28 100644 --- a/internal/service/deploy/deploy.go +++ b/internal/service/deploy/deploy.go @@ -1,3 +1,14 @@ package deploy +import ( + "context" + + "github.com/golang/protobuf/ptypes/empty" + pb "gitlab.com/gitlab-org/gitlab-pages-proto/go" +) + type server struct{} + +func (*server) DeleteSite(context.Context, *pb.DeleteSiteRequest) (*empty.Empty, error) { + return &empty.Empty{}, nil +} diff --git a/internal/service/deploy/deploy_test.go b/internal/service/deploy/deploy_test.go index 18f2483b0..38adef56d 100644 --- a/internal/service/deploy/deploy_test.go +++ b/internal/service/deploy/deploy_test.go @@ -1,17 +1,39 @@ package deploy import ( + "context" "net" "testing" + "time" + "github.com/stretchr/testify/require" pb "gitlab.com/gitlab-org/gitlab-pages-proto/go" "google.golang.org/grpc" ) -func runDeployServer(t *testing.T) (*grpc.Server, string) { +var ( + serverSocketPath = "testdata/grpc.socket" +) + +func TestDeleteSite(t *testing.T) { + s := runDeployServer(t) + defer s.Stop() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + client, conn := newDeployClient(t) + defer conn.Close() + + req := &pb.DeleteSiteRequest{} + + _, err := client.DeleteSite(ctx, req) + require.NoError(t, err) +} + +func runDeployServer(t *testing.T) *grpc.Server { grpcServer := grpc.NewServer() - serverSocketPath := "testdata/grpc.socket" listener, err := net.Listen("unix", serverSocketPath) if err != nil { @@ -22,5 +44,20 @@ func runDeployServer(t *testing.T) (*grpc.Server, string) { go grpcServer.Serve(listener) - return grpcServer, serverSocketPath + return grpcServer +} + +func newDeployClient(t *testing.T) (pb.DeployServiceClient, *grpc.ClientConn) { + connOpts := []grpc.DialOption{ + grpc.WithInsecure(), + grpc.WithDialer(func(addr string, timeout time.Duration) (net.Conn, error) { + return net.DialTimeout("unix", addr, timeout) + }), + } + conn, err := grpc.Dial(serverSocketPath, connOpts...) + if err != nil { + t.Fatal(err) + } + + return pb.NewDeployServiceClient(conn), conn } -- GitLab From 5b1432b4053d5a266b858f9da9019e3b3931631c Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 5 Apr 2018 14:45:13 +0200 Subject: [PATCH 03/18] Add testdata --- internal/service/deploy/testdata/.gitkeep | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 internal/service/deploy/testdata/.gitkeep diff --git a/internal/service/deploy/testdata/.gitkeep b/internal/service/deploy/testdata/.gitkeep new file mode 100644 index 000000000..e69de29bb -- GitLab From b2b42a3c185d5853d0f40f92d0672f0d063af00b Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 5 Apr 2018 14:49:32 +0200 Subject: [PATCH 04/18] Use other import --- internal/service/deploy/deploy.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/service/deploy/deploy.go b/internal/service/deploy/deploy.go index ad2dbdb28..2d78ca104 100644 --- a/internal/service/deploy/deploy.go +++ b/internal/service/deploy/deploy.go @@ -1,10 +1,9 @@ package deploy import ( - "context" - "github.com/golang/protobuf/ptypes/empty" pb "gitlab.com/gitlab-org/gitlab-pages-proto/go" + "golang.org/x/net/context" ) type server struct{} -- GitLab From 0a0c80a8859e11f9953fc2b968e9e4204f1a7db8 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 5 Apr 2018 15:24:49 +0200 Subject: [PATCH 05/18] Add path traversal tests --- internal/service/deploy/deploy.go | 25 +++++++++-- internal/service/deploy/deploy_test.go | 60 +++++++++++++++++++++++--- 2 files changed, 77 insertions(+), 8 deletions(-) diff --git a/internal/service/deploy/deploy.go b/internal/service/deploy/deploy.go index 2d78ca104..9f996b1a9 100644 --- a/internal/service/deploy/deploy.go +++ b/internal/service/deploy/deploy.go @@ -1,13 +1,32 @@ package deploy import ( + "os" + "path" + "regexp" + "github.com/golang/protobuf/ptypes/empty" pb "gitlab.com/gitlab-org/gitlab-pages-proto/go" "golang.org/x/net/context" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) -type server struct{} +type server struct { + rootDir string +} + +var traversalRegex = regexp.MustCompile(`(^\.\./)|(/\.\./)|(/\.\.$)`) + +func (s *server) DeleteSite(ctx context.Context, req *pb.DeleteSiteRequest) (*empty.Empty, error) { + if req.Path == "" { + return nil, status.Errorf(codes.InvalidArgument, "path empty") + } + + if traversalRegex.MatchString(req.Path) { + return nil, status.Errorf(codes.InvalidArgument, "invalid path: %q", req.Path) + } -func (*server) DeleteSite(context.Context, *pb.DeleteSiteRequest) (*empty.Empty, error) { - return &empty.Empty{}, nil + siteDir := path.Join(s.rootDir, req.Path) + return &empty.Empty{}, os.RemoveAll(siteDir) } diff --git a/internal/service/deploy/deploy_test.go b/internal/service/deploy/deploy_test.go index 38adef56d..c71ab3b69 100644 --- a/internal/service/deploy/deploy_test.go +++ b/internal/service/deploy/deploy_test.go @@ -2,20 +2,33 @@ package deploy import ( "context" + "io/ioutil" "net" + "os" + "path" "testing" "time" "github.com/stretchr/testify/require" pb "gitlab.com/gitlab-org/gitlab-pages-proto/go" "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) -var ( +const ( serverSocketPath = "testdata/grpc.socket" + testRootDir = "testdata/root" ) func TestDeleteSite(t *testing.T) { + sitePath := "foo/bar" + testSiteDir := path.Join(testRootDir, sitePath) + require.NoError(t, os.RemoveAll(testSiteDir)) + require.NoError(t, os.MkdirAll(testSiteDir, 0755)) + + require.NoError(t, ioutil.WriteFile(path.Join(testSiteDir, "hello"), []byte("world"), 0644)) + s := runDeployServer(t) defer s.Stop() @@ -25,10 +38,47 @@ func TestDeleteSite(t *testing.T) { client, conn := newDeployClient(t) defer conn.Close() - req := &pb.DeleteSiteRequest{} - - _, err := client.DeleteSite(ctx, req) + _, err := client.DeleteSite(ctx, &pb.DeleteSiteRequest{Path: sitePath}) require.NoError(t, err) + + _, err = os.Stat(testSiteDir) + require.True(t, os.IsNotExist(err), "directory should have been removed") + + _, err = os.Stat(testRootDir) + require.NoError(t, err, "root directory should still exist") +} + +func TestDeleteSiteFail(t *testing.T) { + s := runDeployServer(t) + defer s.Stop() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + client, conn := newDeployClient(t) + defer conn.Close() + + testCases := []struct { + desc string + path string + code codes.Code + }{ + {desc: "empty path", path: "", code: codes.InvalidArgument}, + {desc: "traversal beginning", path: "../foo", code: codes.InvalidArgument}, + {desc: "traversal middle", path: "bar/../foo", code: codes.InvalidArgument}, + {desc: "traversal end", path: "foo/bar/..", code: codes.InvalidArgument}, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + req := &pb.DeleteSiteRequest{Path: tc.path} + + _, err := client.DeleteSite(ctx, req) + st, ok := status.FromError(err) + require.True(t, ok, "error has a grpc status") + require.Equal(t, tc.code, st.Code(), "grpc code") + }) + } } func runDeployServer(t *testing.T) *grpc.Server { @@ -40,7 +90,7 @@ func runDeployServer(t *testing.T) *grpc.Server { t.Fatal(err) } - pb.RegisterDeployServiceServer(grpcServer, &server{}) + pb.RegisterDeployServiceServer(grpcServer, &server{rootDir: testRootDir}) go grpcServer.Serve(listener) -- GitLab From 4481a058c4a8dc320faa8d064dafa7382fd5d3c6 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 5 Apr 2018 18:05:17 +0200 Subject: [PATCH 06/18] Better sanity check --- internal/service/deploy/deploy_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/service/deploy/deploy_test.go b/internal/service/deploy/deploy_test.go index c71ab3b69..912eb548d 100644 --- a/internal/service/deploy/deploy_test.go +++ b/internal/service/deploy/deploy_test.go @@ -44,8 +44,8 @@ func TestDeleteSite(t *testing.T) { _, err = os.Stat(testSiteDir) require.True(t, os.IsNotExist(err), "directory should have been removed") - _, err = os.Stat(testRootDir) - require.NoError(t, err, "root directory should still exist") + _, err = os.Stat(path.Dir(testSiteDir)) + require.NoError(t, err, "parent directory should still exist") } func TestDeleteSiteFail(t *testing.T) { -- GitLab From e3847952f1c42104615c779568dc556dbe1d877a Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 17 May 2018 11:15:16 +0200 Subject: [PATCH 07/18] Disallow path starting with '.' --- internal/service/deploy/deploy.go | 5 +++++ internal/service/deploy/deploy_test.go | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/internal/service/deploy/deploy.go b/internal/service/deploy/deploy.go index 9f996b1a9..fad863cf3 100644 --- a/internal/service/deploy/deploy.go +++ b/internal/service/deploy/deploy.go @@ -4,6 +4,7 @@ import ( "os" "path" "regexp" + "strings" "github.com/golang/protobuf/ptypes/empty" pb "gitlab.com/gitlab-org/gitlab-pages-proto/go" @@ -27,6 +28,10 @@ func (s *server) DeleteSite(ctx context.Context, req *pb.DeleteSiteRequest) (*em return nil, status.Errorf(codes.InvalidArgument, "invalid path: %q", req.Path) } + if strings.HasPrefix(req.Path, ".") { + return nil, status.Errorf(codes.InvalidArgument, "invalid path: %q", req.Path) + } + siteDir := path.Join(s.rootDir, req.Path) return &empty.Empty{}, os.RemoveAll(siteDir) } diff --git a/internal/service/deploy/deploy_test.go b/internal/service/deploy/deploy_test.go index 912eb548d..5a206e56c 100644 --- a/internal/service/deploy/deploy_test.go +++ b/internal/service/deploy/deploy_test.go @@ -67,6 +67,7 @@ func TestDeleteSiteFail(t *testing.T) { {desc: "traversal beginning", path: "../foo", code: codes.InvalidArgument}, {desc: "traversal middle", path: "bar/../foo", code: codes.InvalidArgument}, {desc: "traversal end", path: "foo/bar/..", code: codes.InvalidArgument}, + {desc: "path starting with period", path: ".foo/bar/baz", code: codes.InvalidArgument}, } for _, tc := range testCases { @@ -76,7 +77,7 @@ func TestDeleteSiteFail(t *testing.T) { _, err := client.DeleteSite(ctx, req) st, ok := status.FromError(err) require.True(t, ok, "error has a grpc status") - require.Equal(t, tc.code, st.Code(), "grpc code") + require.Equal(t, tc.code, st.Code(), "unexpected grpc code") }) } } -- GitLab From 711b02a93b27b114a1fa1caeb88d254763aefd0c Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 17 May 2018 11:20:38 +0200 Subject: [PATCH 08/18] Factor out admin unix setup --- admin_test.go | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/admin_test.go b/admin_test.go index 7ceaa82aa..b09399718 100644 --- a/admin_test.go +++ b/admin_test.go @@ -27,14 +27,9 @@ var ( ) func TestAdminUnixPermissions(t *testing.T) { - socketPath := "admin.socket" - // Use "../../" because the pages executable cd's into shared/pages - adminArgs := append(adminSecretArgs, "-admin-unix-listener", "../../"+socketPath) - teardown := RunPagesProcessWithoutWait(t, *pagesBinary, listeners, "", adminArgs...) + socketPath, teardown := startAdminUnix(t) defer teardown() - waitHTTP2RoundTripUnix(t, socketPath) - st, err := os.Stat(socketPath) require.NoError(t, err) expectedMode := os.FileMode(0777) @@ -42,14 +37,9 @@ func TestAdminUnixPermissions(t *testing.T) { } func TestAdminHealthCheckUnix(t *testing.T) { - socketPath := "admin.socket" - // Use "../../" because the pages executable cd's into shared/pages - adminArgs := append(adminSecretArgs, "-admin-unix-listener", "../../"+socketPath) - teardown := RunPagesProcessWithoutWait(t, *pagesBinary, listeners, "", adminArgs...) + socketPath, teardown := startAdminUnix(t) defer teardown() - waitHTTP2RoundTripUnix(t, socketPath) - testCases := []struct { desc string dialOpt grpc.DialOption @@ -144,6 +134,21 @@ func TestAdminHealthCheckHTTPS(t *testing.T) { } } +func startAdminUnix(t *testing.T) (socketPath string, teardown func()) { + socketPath = "admin.socket" + // Use "../../" because the pages executable cd's into shared/pages + adminArgs := append(adminSecretArgs, "-admin-unix-listener", "../../"+socketPath) + teardown = RunPagesProcessWithoutWait(t, *pagesBinary, listeners, "", adminArgs...) + + if err := waitHTTP2RoundTripUnix(socketPath); err != nil { + teardown() + t.Fatal(err) + return "", nil + } + + return socketPath, teardown +} + func newAddr() string { s := httptest.NewServer(http.NotFoundHandler()) s.Close() @@ -176,17 +181,17 @@ func grpcUnixDialOpt() grpc.DialOption { }) } -func waitHTTP2RoundTripUnix(t *testing.T, socketPath string) { +func waitHTTP2RoundTripUnix(socketPath string) error { var err error for start := time.Now(); time.Since(start) < 5*time.Second; time.Sleep(100 * time.Millisecond) { err = roundtripHTTP2Unix(socketPath) if err == nil { - return + return nil } } - t.Fatal(err) + return err } func roundtripHTTP2Unix(socketPath string) error { -- GitLab From ea1e2b3cfe6554b2910bd2949894a99ca9d130a5 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 17 May 2018 11:44:18 +0200 Subject: [PATCH 09/18] End to end test for DeleteSite --- admin_test.go | 41 +++++++++++++++++++++++++++++++ internal/admin/server.go | 9 +++++++ internal/service/deploy/deploy.go | 4 +++ 3 files changed, 54 insertions(+) diff --git a/admin_test.go b/admin_test.go index b09399718..e100ebff3 100644 --- a/admin_test.go +++ b/admin_test.go @@ -3,15 +3,19 @@ package main import ( "context" "crypto/tls" + "io/ioutil" "net" "net/http" "net/http/httptest" "os" + "path" + "path/filepath" "testing" "time" "github.com/stretchr/testify/require" gitalyauth "gitlab.com/gitlab-org/gitaly/auth" + pb "gitlab.com/gitlab-org/gitlab-pages-proto/go" "golang.org/x/net/http2" "google.golang.org/grpc" "google.golang.org/grpc/codes" @@ -134,6 +138,43 @@ func TestAdminHealthCheckHTTPS(t *testing.T) { } } +func TestAdminDeleteSite(t *testing.T) { + socketPath, teardown := startAdminUnix(t) + defer teardown() + + deleteName := "group/project-123-to-be-deleted" + deleteDir, err := filepath.Abs(path.Join(*pagesRoot, deleteName)) + require.NoError(t, err) + + deletePublic := path.Join(deleteDir, "public") + require.NoError(t, os.MkdirAll(deletePublic, 0755)) + require.NoError(t, ioutil.WriteFile(path.Join(deletePublic, "index.html"), nil, 0644)) + + _, err = os.Stat(deleteDir) + require.NoError(t, err, "sanity check: expected directory to exist after setup") + + connOpts := []grpc.DialOption{ + grpc.WithInsecure(), + grpcUnixDialOpt(), + grpc.WithPerRPCCredentials(gitalyauth.RPCCredentials(adminToken)), + } + + conn, err := grpc.Dial(socketPath, connOpts...) + require.NoError(t, err, "dial") + defer conn.Close() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + client := pb.NewDeployServiceClient(conn) + req := &pb.DeleteSiteRequest{Path: deleteName} + _, err = client.DeleteSite(ctx, req) + require.NoError(t, err) + + _, err = os.Stat(deleteDir) + require.True(t, os.IsNotExist(err), "expected directory to be removed") +} + func startAdminUnix(t *testing.T) (socketPath string, teardown func()) { socketPath = "admin.socket" // Use "../../" because the pages executable cd's into shared/pages diff --git a/internal/admin/server.go b/internal/admin/server.go index e7c7cfe6a..c3b3740f6 100644 --- a/internal/admin/server.go +++ b/internal/admin/server.go @@ -2,6 +2,7 @@ package admin import ( "crypto/tls" + "os" grpc_middleware "github.com/grpc-ecosystem/go-grpc-middleware" grpc_auth "github.com/grpc-ecosystem/go-grpc-middleware/auth" @@ -9,6 +10,8 @@ import ( grpc_recovery "github.com/grpc-ecosystem/go-grpc-middleware/recovery" grpc_prometheus "github.com/grpc-ecosystem/go-grpc-prometheus" log "github.com/sirupsen/logrus" + pb "gitlab.com/gitlab-org/gitlab-pages-proto/go" + "gitlab.com/gitlab-org/gitlab-pages/internal/service/deploy" "google.golang.org/grpc" "google.golang.org/grpc/credentials" "google.golang.org/grpc/health" @@ -59,5 +62,11 @@ func serverOpts(secret string) []grpc.ServerOption { } func registerServices(g *grpc.Server) { + wd, err := os.Getwd() + if err != nil { + panic(err) + } + + pb.RegisterDeployServiceServer(g, deploy.NewServer(wd)) healthpb.RegisterHealthServer(g, health.NewServer()) } diff --git a/internal/service/deploy/deploy.go b/internal/service/deploy/deploy.go index fad863cf3..64aa8c54f 100644 --- a/internal/service/deploy/deploy.go +++ b/internal/service/deploy/deploy.go @@ -17,6 +17,10 @@ type server struct { rootDir string } +func NewServer(rootDir string) *server { + return &server{rootDir: rootDir} +} + var traversalRegex = regexp.MustCompile(`(^\.\./)|(/\.\./)|(/\.\.$)`) func (s *server) DeleteSite(ctx context.Context, req *pb.DeleteSiteRequest) (*empty.Empty, error) { -- GitLab From 097b6d87fdbadd62b6ecd30aca854885f93b29b3 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 17 May 2018 11:47:49 +0200 Subject: [PATCH 10/18] More realistic example --- internal/service/deploy/deploy_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/service/deploy/deploy_test.go b/internal/service/deploy/deploy_test.go index 5a206e56c..09609e94f 100644 --- a/internal/service/deploy/deploy_test.go +++ b/internal/service/deploy/deploy_test.go @@ -67,7 +67,7 @@ func TestDeleteSiteFail(t *testing.T) { {desc: "traversal beginning", path: "../foo", code: codes.InvalidArgument}, {desc: "traversal middle", path: "bar/../foo", code: codes.InvalidArgument}, {desc: "traversal end", path: "foo/bar/..", code: codes.InvalidArgument}, - {desc: "path starting with period", path: ".foo/bar/baz", code: codes.InvalidArgument}, + {desc: "path starting with period", path: ".foo/bar", code: codes.InvalidArgument}, } for _, tc := range testCases { -- GitLab From 733cce24a4aa18df51a16871f427ae9aeb9f8796 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 17 May 2018 12:03:58 +0200 Subject: [PATCH 11/18] go vet --- internal/service/deploy/deploy.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/service/deploy/deploy.go b/internal/service/deploy/deploy.go index 64aa8c54f..a6db96ae0 100644 --- a/internal/service/deploy/deploy.go +++ b/internal/service/deploy/deploy.go @@ -17,7 +17,8 @@ type server struct { rootDir string } -func NewServer(rootDir string) *server { +// NewServer returns a new deploy service server. +func NewServer(rootDir string) pb.DeployServiceServer { return &server{rootDir: rootDir} } -- GitLab From 46a14eea1e03adbf82c58434d0292ffafceeb47e Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 17 May 2018 16:16:13 +0200 Subject: [PATCH 12/18] Add existince checks to DeleteSite --- internal/service/deploy/deploy.go | 8 ++++++++ internal/service/deploy/deploy_test.go | 20 +++++++++++++++----- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/internal/service/deploy/deploy.go b/internal/service/deploy/deploy.go index a6db96ae0..a34fabd24 100644 --- a/internal/service/deploy/deploy.go +++ b/internal/service/deploy/deploy.go @@ -38,5 +38,13 @@ func (s *server) DeleteSite(ctx context.Context, req *pb.DeleteSiteRequest) (*em } siteDir := path.Join(s.rootDir, req.Path) + st, err := os.Stat(siteDir) + if err != nil { + return nil, status.Errorf(codes.FailedPrecondition, "request.Path: %v", err) + } + if !st.IsDir() { + return nil, status.Errorf(codes.FailedPrecondition, "not a directory: %q", req.Path) + } + return &empty.Empty{}, os.RemoveAll(siteDir) } diff --git a/internal/service/deploy/deploy_test.go b/internal/service/deploy/deploy_test.go index 09609e94f..774b060ae 100644 --- a/internal/service/deploy/deploy_test.go +++ b/internal/service/deploy/deploy_test.go @@ -22,11 +22,7 @@ const ( ) func TestDeleteSite(t *testing.T) { - sitePath := "foo/bar" - testSiteDir := path.Join(testRootDir, sitePath) - require.NoError(t, os.RemoveAll(testSiteDir)) - require.NoError(t, os.MkdirAll(testSiteDir, 0755)) - + sitePath, testSiteDir := setupTestSite(t) require.NoError(t, ioutil.WriteFile(path.Join(testSiteDir, "hello"), []byte("world"), 0644)) s := runDeployServer(t) @@ -48,7 +44,19 @@ func TestDeleteSite(t *testing.T) { require.NoError(t, err, "parent directory should still exist") } +func setupTestSite(t *testing.T) (sitePath string, testSiteDir string) { + sitePath = "foo/bar" + testSiteDir = path.Join(testRootDir, sitePath) + require.NoError(t, os.RemoveAll(testSiteDir)) + require.NoError(t, os.MkdirAll(testSiteDir, 0755)) + + return sitePath, testSiteDir +} + func TestDeleteSiteFail(t *testing.T) { + sitePath, testSiteDir := setupTestSite(t) + require.NoError(t, ioutil.WriteFile(path.Join(testSiteDir, "hello"), []byte("world"), 0644)) + s := runDeployServer(t) defer s.Stop() @@ -68,6 +76,8 @@ func TestDeleteSiteFail(t *testing.T) { {desc: "traversal middle", path: "bar/../foo", code: codes.InvalidArgument}, {desc: "traversal end", path: "foo/bar/..", code: codes.InvalidArgument}, {desc: "path starting with period", path: ".foo/bar", code: codes.InvalidArgument}, + {desc: "directory does not exist", path: "does/not/exist", code: codes.FailedPrecondition}, + {desc: "path is a file not a directory", path: path.Join(sitePath, "hello"), code: codes.FailedPrecondition}, } for _, tc := range testCases { -- GitLab From bfdff41e27ba8d98061a739a62251731ad8a173f Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 17 May 2018 16:16:36 +0200 Subject: [PATCH 13/18] Circumvent "undefined" working directory --- daemon.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/daemon.go b/daemon.go index 2b3cc8c6c..d062f70d0 100644 --- a/daemon.go +++ b/daemon.go @@ -26,6 +26,14 @@ func daemonMain() { return } + if len(os.Args) != 2 { + fatal(fmt.Errorf("usage: %s WORKING_DIRECTORY", daemonRunProgram)) + } + + if err := os.Chdir(os.Args[1]); err != nil { + fatal(err) + } + log.WithFields(log.Fields{ "uid": syscall.Getuid(), "gid": syscall.Getgid(), @@ -128,6 +136,7 @@ func chrootDaemon(cmd *exec.Cmd) (*jail.Jail, error) { // Update command to use chroot cmd.SysProcAttr.Chroot = chroot.Path() cmd.Path = tempExecutablePath + cmd.Args = append(cmd.Args, "/") cmd.Dir = "/" if err := chroot.Build(); err != nil { @@ -189,7 +198,8 @@ func jailDaemon(cmd *exec.Cmd) (*jail.Jail, error) { // Update command to use chroot cmd.SysProcAttr.Chroot = cage.Path() cmd.Path = "/gitlab-pages" - cmd.Dir = pagesRootInChroot + cmd.Args = append(cmd.Args, pagesRootInChroot) + cmd.Dir = "/" err = cage.Build() if err != nil { -- GitLab From 5e7ec66dd6239c3761379a56a443d5a4102c991d Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 17 May 2018 16:34:17 +0200 Subject: [PATCH 14/18] Revert "Circumvent "undefined" working directory" This reverts commit bfdff41e27ba8d98061a739a62251731ad8a173f. --- daemon.go | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/daemon.go b/daemon.go index d062f70d0..2b3cc8c6c 100644 --- a/daemon.go +++ b/daemon.go @@ -26,14 +26,6 @@ func daemonMain() { return } - if len(os.Args) != 2 { - fatal(fmt.Errorf("usage: %s WORKING_DIRECTORY", daemonRunProgram)) - } - - if err := os.Chdir(os.Args[1]); err != nil { - fatal(err) - } - log.WithFields(log.Fields{ "uid": syscall.Getuid(), "gid": syscall.Getgid(), @@ -136,7 +128,6 @@ func chrootDaemon(cmd *exec.Cmd) (*jail.Jail, error) { // Update command to use chroot cmd.SysProcAttr.Chroot = chroot.Path() cmd.Path = tempExecutablePath - cmd.Args = append(cmd.Args, "/") cmd.Dir = "/" if err := chroot.Build(); err != nil { @@ -198,8 +189,7 @@ func jailDaemon(cmd *exec.Cmd) (*jail.Jail, error) { // Update command to use chroot cmd.SysProcAttr.Chroot = cage.Path() cmd.Path = "/gitlab-pages" - cmd.Args = append(cmd.Args, pagesRootInChroot) - cmd.Dir = "/" + cmd.Dir = pagesRootInChroot err = cage.Build() if err != nil { -- GitLab From d8db91f5aba973e0a651e5231befc0f5d542293f Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 17 May 2018 16:51:08 +0200 Subject: [PATCH 15/18] Assume pages root is working dir --- internal/service/deploy/deploy.go | 42 ++++++++++++++------------ internal/service/deploy/deploy_test.go | 24 +++++++++++++-- 2 files changed, 44 insertions(+), 22 deletions(-) diff --git a/internal/service/deploy/deploy.go b/internal/service/deploy/deploy.go index a34fabd24..936c28079 100644 --- a/internal/service/deploy/deploy.go +++ b/internal/service/deploy/deploy.go @@ -2,7 +2,6 @@ package deploy import ( "os" - "path" "regexp" "strings" @@ -13,32 +12,21 @@ import ( "google.golang.org/grpc/status" ) -type server struct { - rootDir string -} +type server struct{} // NewServer returns a new deploy service server. -func NewServer(rootDir string) pb.DeployServiceServer { - return &server{rootDir: rootDir} +func NewServer() pb.DeployServiceServer { + return &server{} } var traversalRegex = regexp.MustCompile(`(^\.\./)|(/\.\./)|(/\.\.$)`) func (s *server) DeleteSite(ctx context.Context, req *pb.DeleteSiteRequest) (*empty.Empty, error) { - if req.Path == "" { - return nil, status.Errorf(codes.InvalidArgument, "path empty") - } - - if traversalRegex.MatchString(req.Path) { - return nil, status.Errorf(codes.InvalidArgument, "invalid path: %q", req.Path) + if err := validatePath(req.Path); err != nil { + return nil, err } - if strings.HasPrefix(req.Path, ".") { - return nil, status.Errorf(codes.InvalidArgument, "invalid path: %q", req.Path) - } - - siteDir := path.Join(s.rootDir, req.Path) - st, err := os.Stat(siteDir) + st, err := os.Stat(req.Path) if err != nil { return nil, status.Errorf(codes.FailedPrecondition, "request.Path: %v", err) } @@ -46,5 +34,21 @@ func (s *server) DeleteSite(ctx context.Context, req *pb.DeleteSiteRequest) (*em return nil, status.Errorf(codes.FailedPrecondition, "not a directory: %q", req.Path) } - return &empty.Empty{}, os.RemoveAll(siteDir) + return &empty.Empty{}, os.RemoveAll(req.Path) +} + +func validatePath(requestPath string) error { + if requestPath == "" { + return status.Errorf(codes.InvalidArgument, "path empty") + } + + if traversalRegex.MatchString(requestPath) { + return status.Errorf(codes.InvalidArgument, "invalid path: %q", requestPath) + } + + if strings.HasPrefix(requestPath, ".") || strings.HasPrefix(requestPath, "/") { + return status.Errorf(codes.InvalidArgument, "invalid path: %q", requestPath) + } + + return nil } diff --git a/internal/service/deploy/deploy_test.go b/internal/service/deploy/deploy_test.go index 774b060ae..b86ba57f0 100644 --- a/internal/service/deploy/deploy_test.go +++ b/internal/service/deploy/deploy_test.go @@ -6,6 +6,8 @@ import ( "net" "os" "path" + "path/filepath" + "sync" "testing" "time" @@ -17,11 +19,23 @@ import ( ) const ( - serverSocketPath = "testdata/grpc.socket" + serverSocketPath = "../grpc.socket" testRootDir = "testdata/root" ) +var ( + cdRootOnce sync.Once +) + +func cdRoot(t *testing.T) { + cdRootOnce.Do(func() { + require.NoError(t, os.Chdir(testRootDir)) + }) +} + func TestDeleteSite(t *testing.T) { + cdRoot(t) + sitePath, testSiteDir := setupTestSite(t) require.NoError(t, ioutil.WriteFile(path.Join(testSiteDir, "hello"), []byte("world"), 0644)) @@ -46,7 +60,8 @@ func TestDeleteSite(t *testing.T) { func setupTestSite(t *testing.T) (sitePath string, testSiteDir string) { sitePath = "foo/bar" - testSiteDir = path.Join(testRootDir, sitePath) + testSiteDir, err := filepath.Abs(sitePath) + require.NoError(t, err) require.NoError(t, os.RemoveAll(testSiteDir)) require.NoError(t, os.MkdirAll(testSiteDir, 0755)) @@ -54,6 +69,8 @@ func setupTestSite(t *testing.T) (sitePath string, testSiteDir string) { } func TestDeleteSiteFail(t *testing.T) { + cdRoot(t) + sitePath, testSiteDir := setupTestSite(t) require.NoError(t, ioutil.WriteFile(path.Join(testSiteDir, "hello"), []byte("world"), 0644)) @@ -76,6 +93,7 @@ func TestDeleteSiteFail(t *testing.T) { {desc: "traversal middle", path: "bar/../foo", code: codes.InvalidArgument}, {desc: "traversal end", path: "foo/bar/..", code: codes.InvalidArgument}, {desc: "path starting with period", path: ".foo/bar", code: codes.InvalidArgument}, + {desc: "path starting with slash", path: "/foo/bar", code: codes.InvalidArgument}, {desc: "directory does not exist", path: "does/not/exist", code: codes.FailedPrecondition}, {desc: "path is a file not a directory", path: path.Join(sitePath, "hello"), code: codes.FailedPrecondition}, } @@ -101,7 +119,7 @@ func runDeployServer(t *testing.T) *grpc.Server { t.Fatal(err) } - pb.RegisterDeployServiceServer(grpcServer, &server{rootDir: testRootDir}) + pb.RegisterDeployServiceServer(grpcServer, NewServer()) go grpcServer.Serve(listener) -- GitLab From 774dc541b402433f0a2251c8220425fc33545c75 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 17 May 2018 17:08:37 +0200 Subject: [PATCH 16/18] Document why we chdir --- internal/service/deploy/deploy_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/service/deploy/deploy_test.go b/internal/service/deploy/deploy_test.go index b86ba57f0..967c9c472 100644 --- a/internal/service/deploy/deploy_test.go +++ b/internal/service/deploy/deploy_test.go @@ -27,6 +27,12 @@ var ( cdRootOnce sync.Once ) +// cdRoot changes the working directory of the test executable. We are +// forced to assume that the pages root directory is the current working +// directory. When running pages with chroot+bind mount, os.Getwd() +// resolves to a garbage "(undefined)" vaule. So in turn, the tests for +// this package must execute with the pages root as the working +// directory. func cdRoot(t *testing.T) { cdRootOnce.Do(func() { require.NoError(t, os.Chdir(testRootDir)) -- GitLab From 4a0c3a92d2e60d58a07d238e6e79a11614f8595e Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 17 May 2018 17:10:13 +0200 Subject: [PATCH 17/18] Fix NewServer invocation --- internal/admin/server.go | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/internal/admin/server.go b/internal/admin/server.go index c3b3740f6..ce1a331e3 100644 --- a/internal/admin/server.go +++ b/internal/admin/server.go @@ -2,7 +2,6 @@ package admin import ( "crypto/tls" - "os" grpc_middleware "github.com/grpc-ecosystem/go-grpc-middleware" grpc_auth "github.com/grpc-ecosystem/go-grpc-middleware/auth" @@ -62,11 +61,6 @@ func serverOpts(secret string) []grpc.ServerOption { } func registerServices(g *grpc.Server) { - wd, err := os.Getwd() - if err != nil { - panic(err) - } - - pb.RegisterDeployServiceServer(g, deploy.NewServer(wd)) + pb.RegisterDeployServiceServer(g, deploy.NewServer()) healthpb.RegisterHealthServer(g, health.NewServer()) } -- GitLab From bc511610bd5aa309b40d92d6a7c87fe3b4a3f24e Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 23 May 2018 13:36:43 +0200 Subject: [PATCH 18/18] Ban leading tilde --- internal/service/deploy/deploy.go | 2 +- internal/service/deploy/deploy_test.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/service/deploy/deploy.go b/internal/service/deploy/deploy.go index 936c28079..ac0e2452f 100644 --- a/internal/service/deploy/deploy.go +++ b/internal/service/deploy/deploy.go @@ -46,7 +46,7 @@ func validatePath(requestPath string) error { return status.Errorf(codes.InvalidArgument, "invalid path: %q", requestPath) } - if strings.HasPrefix(requestPath, ".") || strings.HasPrefix(requestPath, "/") { + if strings.IndexAny(requestPath, "./~") == 0 { return status.Errorf(codes.InvalidArgument, "invalid path: %q", requestPath) } diff --git a/internal/service/deploy/deploy_test.go b/internal/service/deploy/deploy_test.go index 967c9c472..156e333c8 100644 --- a/internal/service/deploy/deploy_test.go +++ b/internal/service/deploy/deploy_test.go @@ -100,6 +100,7 @@ func TestDeleteSiteFail(t *testing.T) { {desc: "traversal end", path: "foo/bar/..", code: codes.InvalidArgument}, {desc: "path starting with period", path: ".foo/bar", code: codes.InvalidArgument}, {desc: "path starting with slash", path: "/foo/bar", code: codes.InvalidArgument}, + {desc: "path starting with tilde", path: "~/foo/bar", code: codes.InvalidArgument}, {desc: "directory does not exist", path: "does/not/exist", code: codes.FailedPrecondition}, {desc: "path is a file not a directory", path: path.Join(sitePath, "hello"), code: codes.FailedPrecondition}, } -- GitLab