types used for reflection in importers cause breakage

[ezequielaguilera1993]

What version of Garble and Go are you using?

$ garble version
I try in two ways (both versions produce the same break in the execution)
    1. compiled all the code in master branch with go build, and then I use the executable.
    2. mvdan.cc/garble v0.10.1

    Build settings:
          -buildmode exe
           -compiler gc
         CGO_ENABLED 1
              GOARCH arm64
               GOOS darwin



$ go version
go version go1.20.3 darwin/arm64

What environment are you running Garble on?

(work computer)

(work computer)

go env Output

$ go env
(work computer)

What did you do?

garble -literals -tiny -seed=random build

I made a code that lists the containers within Docker to remove them, but the obfuscated code doesn't work (the non-obfuscated one does).

(go mod)
go 1.20
require (
	github.com/docker/docker v24.0.5+incompatible
	github.com/docker/go-connections v0.4.0
	github.com/opencontainers/image-spec v1.0.2
	github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8
)


(code)
import (
	"context"
	"fmt"
	"github.com/docker/docker/api/types"
	"github.com/docker/docker/api/types/container"
	"github.com/docker/docker/client"
	"github.com/docker/go-connections/nat"
	v1 "github.com/opencontainers/image-spec/specs-go/v1"
	"github.com/pkg/browser"
	"net/http"
	"os"
	"os/exec"
	"path/filepath"
	"runtime"
	"time"
)

func tryToRemoveContainers() {
	containers, err := cli.ContainerList(context.Background(), types.ContainerListOptions{All: true})
	if err != nil {
		println("Failed to list containers:", err)
		return
	}

	names := []string{"container1", "container2"} // Hardcoded container names
	println("tryToRemoveContainers: 1")

	for _, iContainer := range containers {
		println("tryToRemoveContainers: 2")

		for _, name := range names {
			println("tryToRemoveContainers: 2.6")
			if iContainer.Names[0] == "/"+name {
				println("tryToRemoveContainers: 3")

				fmt.Printf("Removing iContainer: %s\n", iContainer.ID)
				err := cli.ContainerRemove(context.Background(), iContainer.ID, types.ContainerRemoveOptions{
					Force: true,
				})
				if err != nil {
					println("tryToRemoveContainers: 4")

					println("Failed to remove iContainer:" + iContainer.ID + " _" + err.Error())
					fmt.Printf("Failed to remove iContainer %s: %s\n", iContainer.ID, err)
                                         println("tryToRemoveContainers: 5")
				}
			}
		}
	}
}

What did you expect to see?

Finish the code and print this
tryToRemoveContainers: 1
tryToRemoveContainers: 2
tryToRemoveContainers: 2.6
tryToRemoveContainers: 3
tryToRemoveContainers: 4
tryToRemoveContainers: 5

What did you see instead?

tryToRemoveContainers: 1
tryToRemoveContainers: 2
tryToRemoveContainers: 2.6
break in this line

if iContainer.Names[0] == "/"+name {

[lu4p]

Your code can't produce your expected output.

Are you sure that 4 and 5 are expected?

Are you sure iContainer.Names[0] == "/"+name is the right condition?

Have you tried without literals?

[ezequiel-aguilera-uala]

Example to replicate the error

The previous example its bad, I create a new code (not pseucode, this really works) to replicate the error.
Its a "portable" example. With docker + docker pull hello-world you should execute fine.

package main

import (
	"context"
	"github.com/docker/docker/api/types"
	"github.com/docker/docker/api/types/container"
	"github.com/docker/docker/client"
)

var (
	ctx = context.Background()
	cli *client.Client
)

const (
	helloWorld = "helloWorld"
)

func main() {
	cli, _ = client.NewClientWithOpts(client.FromEnv)
	removeHelloWorldContainer()
	upOfficialHelloWorldImage()
	println("success!")

}

func removeHelloWorldContainer() {

	containers, err := cli.ContainerList(context.Background(), types.ContainerListOptions{All: true})
	if err != nil {
		println("Failed to list containers:", err)
		panic("")
	}

	for _, c := range containers {
		println("print before c.Names[0]")
		if c.Names[0] == "/"+helloWorld {
			println("print after c.Names[0]")
			err := cli.ContainerRemove(context.Background(), c.ID, types.ContainerRemoveOptions{
				Force: true,
			})
			if err != nil {
				println(err.Error())
				panic("")
			}
		}
	}
}

func upOfficialHelloWorldImage() {
	helloWorldContainer, err := cli.ContainerCreate(ctx, &container.Config{
		Image: "hello-world",
	}, nil, nil,
		nil, helloWorld)
	if err != nil {
		println(err)
		panic("")
	}

	// Start PostgresSQL container
	if err := cli.ContainerStart(ctx, helloWorldContainer.ID, types.ContainerStartOptions{}); err != nil {
		println(err)
		panic("")
	}
}

Executed commands

This its my makefile. I use a compiled from master branch. (because the last version can't handle protopuf)

go install mvdan.cc/garble@master
go: downloading mvdan.cc/garble v0.10.2-0.20230802092912-23c864185568

very-very-obfuscate:
	garble -literals -tiny -seed=random build -o ./very-very-obfuscate

obfuscate:
	garble build -o ./obfuscate

go-build:
	go build -o ./go-build

LOGS

very-very-obfuscate (logs)

print before c.Names[0]

obfuscate (logs)

print before c.Names[0]
panic: runtime error: index out of range [0] with length 0

goroutine 1 [running]:
main.lK6_r51DQK1L()
	kl51J83N.go:2 +0x22c
main.main()
	u6mKGLcgWT.go:1 +0x6c

go-build (logs)

print before c.Names[0]
print after c.Names[0]
success!

[ezequiel-aguilera-uala]

As it looks, the regular compilation works fine, but any compilation done by garble fails to execute the code properly. Could it be that something needs further refinement in the protobuf patch in master branch?

[ezequiel-aguilera-uala]

I don't think I fully understand how the service works in depth to be able to generate a PR, but I'd be happy to help with anything I can.

[ezequiel-aguilera-uala]

@mvdan hello, I already added info to the issue.

[pagran]

Try add to your code: var _ = reflect.TypeOf(types.Container{})

I think name obfuscation breaks docker response deserialisation

[lu4p]

To fix this we could either:
a) run a full pass of reflection detection on all packages before any obfuscation
or
b) also blacklist structs with struct tags by default

[lu4p]

I think a) would be the optimal solution.

Couldn't we run a preemptive go build -toolexec to gather information, which also caches a build for typechecking?

[mvdan]

Option B would make the obfuscator "give up" on many Go types out there, and it's still not guaranteed to be a full fix. So I don't think it's a good idea.

We have discussed option A before, I think. Perhaps it was on Slack, because I can't find it. I also worry about discussing it here, because we could get somewhat deep in the detail in the comments here.

My thoughts are that it can work, but it's very expensive. Suppose we have the main packages M1 and M2, which both import the library package P. Right now, if we do garble build M1 and garble build M2, we only obfuscate P once. With option A, there would be two significant drawbacks:

1. The M1 and M2 builds would both obfuscate and build package P differently, since the process to detect the use of reflection would use different sets of packages (M1 + P versus M2 + P). Even more than that, garble build P would lead to yet another different obfuscation and build of P for the same reason. This leads to slower builds and larger build caches.

2. Slower start of an obfuscation, since the first step would be to collect all the reflect usage information. This would be a larger upfront cost than you think: remember that it relies on go/types and go/ssa, and you can't load SSA from disk, so on large projects a garble build might spend entire minutes building SSA before the first Go package is obfuscated and built.

[mvdan]

Also: option B would never work with a command like #369, because it would be impossible to know how downstreams or importers would use the exposed types with reflection. I think that's another reason to not go down that route.

[mvdan]

Retitling the issue. To reiterate, this is a known limitation, and this is why the README suggests using reflect.TypeOf in one of the listed caveats. This is not a new bug, and it has a workaround.

I'm happy to leave the issue open for discussion, but see my last two comments - I don't think this has a good or easy solution that will "just work" for most users.