-
-
Notifications
You must be signed in to change notification settings - Fork 268
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.
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
support Wails with its use of reflection on types #849
Comments
I opened a PR with a draft implementation for this proposal at #850 |
After thinking further about it, I believe the impact of leaking |
Any news on this? It would be a huge win. |
Thank you for opening this issue and raising a PR. I agree this would be a useful feature and indeed I planned for it some time ago as well. I will look into this soon, once @lu4p's #889 is finished, given that it should resolve a number of existing bugs and it is touching very related code. In fact I expect the "map" subcommand will be able to reuse much of the same code. |
Amazing @mvdan ! Thanks so much! |
Thank you @mvdan for taking the time to answer this.
I'd like to point out that in fact the PR is not introducing any new code at all, it just moves around existing code. Part of the |
Thanks for the update. Perhaps hold off on spending too much time on the PR, because I'm not yet convinced about how to design this feature. The way you designed the CLI interface and the JSON format with https://pkg.go.dev/golang.org/x/tools/go/types/objectpath#Path is perfectly fine, but I need to think about it a bit more to convince myself whether it's the right approach or if there's a better way. For example, my initial instinct is that this could live as part of |
Sure, the PR was always meant as a PoC and I do not plan on ever pushing substantial updates. I just solved the misleadingly large conflicts, as they made the changeset hard to assess. The diffing engine does not deal very well with code factorization.
On a more philosophical note, although they look similar, the purpose of "map"-like functionality is not to reverse obfuscation but to perform it, matching exactly the way the build command does it. As for the actual interface, personally I have no preference as long as the necessary functionality is there. Should it be an interactive command like reverse instead of printing a full mapping? That would be fine too. Perhaps an importable package exposing a Go API? Fine as well.
That's right. The main difference is that "map"-like functionality requires knowing in addition which objects (packages/types/methods) are actually obfuscated at build time, and which ones aren't. Hence why I factored out all related logic from
|
In light of the work that's being done in #889 I feel like I need to be a bit more specific here. What wails needs is the ability to predict at compile time, for any given package, public type, method or field, which strings will be reported by Go's reflection methods. Because wails allows developers to generate bindings for arbitrarily complex and possibly cross-package type hierarchies, requiring them to manually mark all those types as reflected is a hassle and does not compose well (what if they want to depend on an external package that is oblivious to garble?). Instead, wails can leverage its own code generator to deal transparently with obfuscated names, as long as the information discussed above is made available |
If that is all you need, in theory with #889 merged, you can simply rely on Go's As you describe in your original post, the fact that the original strings are reachable via reflection can be considered as less powerful obfuscation, but for Go programs to work in general, reflection cannot provide obfuscated names when using garble. I don't think garble as a general tool can work in any other way.
To be clear, you absolutely cannot match the obfuscation of |
That's extremely clear, and the whole reason why I wanted garble itself to provide the required data instead of working around it somehow.
Exactly. Poor word choice on my part. I meant performing obfuscation of names, as in going from a name to its obfuscated version, whereas
Indeed that's all we need, and #889 was a big step up. However, it still reports obfuscated strings for package paths and type names, which are accessible through reflection as well. The reason why we need those is to be able to match So:
BTW, there is a comment at #889 that mentions ORM usage. Although their problem seems to be with reflection detection, i.e. not directly related, reflection-based ORMs will likely require unobfuscated type names to function correctly. |
That's a fair point. #889 focused on fields because those are almost always the main aspect of reflection, for example for encodings. But as you say, package paths and type names are reachable by reflection as well. So for any types which we can detect to be used with reflection, we should also support deobfuscating their type name and package path just like we do with any struct fields underneath them. I prefer this solution over the others, at least as a start, because I'd rather have garble work out of the box for the vast majority of users, even when they make use of reflection. If someone prefers stronger obfuscation where all names are obfuscated, regardless of reflection, we can always provide that as an option in the future - perhaps paired with a I'll take a look this weekend when I get some time; it would be a matter of adding more test cases involving type names and package paths, check that they currently fail, and then tweak the logic to record the original and obfuscated strings for those too, if you want to take a look before I do. |
This is amazing @mvdan and we really appreciate your time on this 🙏 |
Just like we support fetching original type, field, and method names via reflection to ensure that no Go package is broken by garble, do the same for package names and import paths, which are reachable too. This means we can remove the test function printfWithoutPackage, which in hindsight should have been a clue as we were working around a bug in garble. Updates burrowers#849.
I have just sent a PR, if you would like to give it a go: #904 We already supported deobfuscating type names, just like we did for method names and field names. We were missing package names and import paths; those should both work with that PR. |
Thank you so much @mvdan for you efforts. I tested #904 via cloning and installing (via The tip of the branch is commit
After issuing I still see obfuscated type names and package paths at runtime. I see unobfuscated method names, hence I guess reflection detection must be working fine. How to reproduceI am working on macOS 12.6.6. Checkout my fork fbbdev/wails, branch garble-tests. The branch's got some minimal changes on top of wails v3's current tip to make it report reflected names in production mode. Cd to folder v3/examples/binding. Ensure go/garble caches are empty. Invoke If everything goes well, a test application window will open up. Ignore this; we are interested in terminal output. Expected output:
The output I see:
The involved types are
The output is produced at Type names and package paths are fetched by calling Method names are accessed through I hope this can give you enough pointers to debug the issue. If you need any assistance, or wish me to do some specific testing/debugging, feel free to ask. |
Related to the discussion that's going on at #904, specifically here. I can see how package path deobfuscation could reveal too much, as @lu4p suggests there. It would be useful here to know how widespread is the use of package paths from reflection over the entire Go ecosystem. I am under the impression (but I might be completely wrong) that Wails' case is pretty niche, and that in most common cases it would be bad design to rely upon knowledge of exact package paths (as opposed to just their uniqueness, which garble still guarantees). The use of type names — on the other hand — is arguably very widespread (see ORMs), and they can be deobfuscated selectively without doing too much damage. Once type name reporting is fixed (i.e. they are reliably deobfuscated), wails could do without package path deobfuscation, and rely instead upon some help from garble which is perhaps much more reasonable than the previously proposed We could do with a
How would you see something like that? |
Rationale
At present, garble provides a
reverse
subcommand that enables consumers to map obfuscated identifiers back to their plain counterparts as needed.There are cases where this is not enough for devtools that might want to offer support for garbled builds. This is especially true for tools that provide interoperation between go and other languages (more on a concrete use case below).
What such tools might need is actually the ability to map plain identifiers to their obfuscated counterparts – i.e. perform obfuscation – matching exactly the behaviour of the installed garble binary. This is not an easy problem especially considering that garble has some sophisticated logic for deciding which objects get obfuscated, and which do not.
Proposal
I propose therefore the addition of a new subcommand, tentatively named
garble map
, with the following usage:The command should output a JSON object with one key per obfuscated package – including all packages given on the command line and their transitive dependencies, kinda like
go list -deps
– with the following shape (but other approaches will do as long as they provide equivalent information):Here,
"TYPES_OBJECT_PATH_n": "OBFUSCATED_NAME_n"
represents one entry for each obfuscated, externally visiblego/types.Object
in the package. Keys are object paths as computed by the facilities atgolang.org/x/tools/go/types/objectpath
. Objects that are not obfuscated, or don't have a path (i.e. they are not part of the package's public API) should not appear in the map.Security considerations
Accidentally leaking the output of
garble map
would defeat obfuscationfor all exported APIsfor all public functions and public/private type names, so it carries a higher risk thangarble reverse
. Consumers should take extra care when integratinggarble map
with their development workflows.That risk can be mitigated by feeding the output of
garble map
directly into other tools through shell pipelines, without writing it to the filesystem.Concrete use case
Recently I have been working as an independent contributor on a full rewrite of the binding generator for Wails v3.
Wails is a project that enables developers to write desktop apps using Go and web technologies. Like many similar projects, it aims to support seamless interoperation between the Go backend and the JS frontend.
To this end, they provide an RPC system based upon
encoding/json
and a binding generator that parses Go code and outputs JS glue code. Specifically, developers register a list of named types as RPC "Services"; a hash is then computed for each method based on package path, type name and method name, and used to identify methods in remote call requests.This obviously breaks down for garbled builds. Project owner @leaanthony sought advice some time ago at #826 and @lu4p suggested using the
reflect.ValueOf
trick; that however is not enough in this case, because it is only going to stop obfuscation of fields, not of the type name or package path.There are workarounds, but they all involve increased boilerplate and upkeep efforts for application developers, and/or degraded obfuscation.
Not to speak of the fact that the
reflect.ValueOf
trick cannot cross package boundaries, and therefore hurts composability a lot. People who may need or want to use complex type hierarchies in their RPC API are going to experience severe limitations.From Wails' perspective, the best option is clearly to provide transparent support for garbled builds, which requires some cooperation on garble's part. I devised and implemented the
garble map
approach described above as a minimal means for them to achieve this end.It's true that the generated JS glue code could be used (although not without some effort) to partly reverse obfuscation, but Wails users would be made well aware of that and advised to obfuscate JS code as well (which they probably already do).
The text was updated successfully, but these errors were encountered: