Be smarter about which methods can have their names changed

[mvdan]

Changing all method names will break some programs, as method names are necessary to implement interfaces.

On the otherhand, not touching any method names exposes far too much information. It's likely that only a minority of methods actually implement interfaces.

Of course, this is a difficult problem to solve, because for a library package it's impossible to tell which methods are meant to implement which interfaces. Perhaps it's only a package that imports our library which uses a type as an interface with methods, since a type doesn't have to be upfront about what interfaces it implements.

We implemented a conservative version of this algorithm for https://github.com/mvdan/unparam, so perhaps we can reuse it.

Right now, we grable method names only if they are unexported, which is a reasonable shortcut until we have a better mechanism.

[capnspacehook]

We implemented a conservative version of this algorithm for https://github.com/mvdan/unparam, so perhaps we can reuse it.

What do you mean by saying the algorithm is conservative?

I think that interfaces defined in packages that we are actively garbling (ie in GOPRIVATE) and methods also in the same packages can be safely garbled, no?

[mvdan]

unparam can only suggest unused parameters if those are safe to remove. A parameter isn't safe to remove if it's useful to implement an interface.

For better or worse, implementing interfaces in Go is entirely implicit, not explicit. So you could have something like https://play.golang.org/p/L3h9n74N6ax. Both unparam and garble work on a package-by-package basis, so when you're looking at the foo sub-package, you don't know that a dependent might require an exported method to satisfy an arbitrary interface.

unparam takes a form of middle ground, if I recall correctly. We only consider that methods need to implement interfaces which are used explicitly. For example, when a variable of a type is assigned to an interface type, or passed as an interface parameter, or otherwise converted to an interface type. Take a look at https://github.com/mvdan/unparam/blob/b37ab49443f75cf3121a1c153ca02be64ecdec07/check/check.go#L399-L411 and the places where we call it.

garble has a very similar situation. Unfortunately, I think we have a bigger problem here. unparam only complains about unused parameters, which are pretty rare, so the chances of it breaking such implicit interface implementations are slim. However, garble will attempt to rename all methods, so the playground example I gave above could break for sure.

[mvdan]

I think that interfaces defined in packages that we are actively garbling (ie in GOPRIVATE) and methods also in the same packages can be safely garbled, no?

To answer your question - it depends. If both the interface and methods are unexported, and the interface isn't used for anything else, they could be garbled together. Otherwise, it's possible you're breaking something, like in the situation in the playground.

The problem is that we rely on the package that defines a name to provide a salt for hashing the name. Interface method names are global, not package-scoped, so the same kind of hashing just doesn't work.

Maybe I should rethink the entire feature. Maybe exported method names should be garbled with a truly global salt, not a per-package one.

[capnspacehook]

I think a global salt would be beneficial for sure if it would let us garble more exported method names, which are pretty common in Go. It may also be helpful to change Garble's logic to gather all the imported/dependent packages and build a list of their interfaces so we can more intelligently decide if it's safe to garble exported methods.

[mvdan]

A global salt does solve the scenario I shared in the playground, which I think will be pretty common. The only problem remaining is interfaces which are garbled where the types implementing the interface are not, and vice versa. Imagine one defining a String() string method while fmt.Stringer isn't garbled, or imagine one defining an interface implemented by some types in std which are not being garbled.

One possible way to fix this would be to always garble everything, so all method names would be garbled with the global salt. I would like to avoid that, though, because it sets a bad precedent. I would like to imagine that, in the future, we could have build caching which would mean we could avoid re-building any packages outside GOPRIVATE.

It may also be helpful to change Garble's logic to gather all the imported/dependent packages and build a list of their interfaces so we can more intelligently decide if it's safe to garble exported methods.

You mean look at all the packages in a build at once? We could do that (i.e. whole-program analysis), but it would be pretty expensive. It would also be pretty unfortunate, because we would lose the deterministic build property on a per-package basis. If garble builds the same package with the same set of dependencies, the output should always be the same, no matter what packages end up importing the original package.

If you mean just looking at a package's dependencies and all the interfaces they define, that would indeed be possible. That would be along the lines of the analysis that unparam does. We wouldn't cover the playground scenario, but I think that's OK - we could just rely on the developer to provide extra hints, like with reflect.

[mvdan]

If you mean just looking at a package's dependencies and all the interfaces they define, that would indeed be possible. That would be along the lines of the analysis that unparam does.

Here's this idea, but developed a bit further:

1. For each package we're garbling, recursively look at the package and all of its imports via go/types.
2. For each exported interface we come across, add each of its method names to a "method name blacklist".
3. When garbling methods, only garble those which are not in the blacklist.
4. To garble exported method names, use a global salt instead of a per-package one.

This would be very conservative, but I think it would be a good starting point. If the interface were to be defined in an importer package (like in the playground link), I think point 4 would save us; both methods would be hashed with the same global salt. The only extra rule we'd have to enforce is that a non-garbled package can't import a garbled package. I think this is a rule/limitation we have already, just not well documented.

This strategy could be improved in the future, with ideas such as:

A) Build the blacklist via method name + method signature, instead of just the name, as those are the rules for interfaces
B) Don't add interface methods to the blacklist if the package where the interface is defined will be garbled (since we can just garble those methods too, with the global salt)
C) Always garble methods which use parameters of unexported types, since those will almost never be usable by other packages. Such methods can be considered unexported in practice.

[mvdan]

B) Don't add interface methods to the blacklist if the package where the interface is defined will be garbled (since we can just garble those methods too, with the global salt)

Mental note: this can break if an interface is defined in a private package, but satisfied by types in dependencies which are public. We would obfuscate the method name in the interface, but not the implementer types, so the program would fail to compile.

[mvdan]

Assuming that soon enough we will be obfuscating all packages always (#276 (comment)), this gets a lot easier. We simply need to obfuscate method names in a way that won't change across packages, similar to what is being done for #310.

For example, we could use the function signature to hash the name with; that way, func (...) fooBar() would result in a different hash than func (...) fooBar(n int), for example. This would still be fine for interfaces, as they require the name and signature to match.

[pmaene]

First of, I've been following this work for a while and I'm very impressed by the progress that's been made! Thank you! I'm sorry for digging up such an old issue, but I was curious whether obfuscating interface methods is still on your roadmap?

[mvdan]

No need to apologise :) Obfuscating most interface methods is still on the roadmap, roughly following #3 (comment). Right now we can't quite do that, because we don't support obfuscating all packages yet. #193 is one of the blockers, but there are still some third party packages that we fail to obfuscate as well, for which #240 should help.

We will also have to worry about reflection, because of https://pkg.go.dev/reflect#Value.MethodByName. However, that should be very similar to what we already do with exported fields, so I'm optimistically not too worried.

[pmaene]

Thanks a lot for the quick response! Great to hear that this is still on your mind, and we'll be the first to try this out once it lands ;).