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.

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

Added Nuitka artifacts via gh-actions #330

Open
wants to merge 1 commit into
base: build
Choose a base branch
from

Conversation

sommerf-lf
Copy link

As our company's AV did always flag the pyinstaller version (as also noted in releases text and several discussions) and it doesn't quite fit our environment to run venvs barebone, I decided to add Nuitka compilation. (Nuitka converts python code to C/C++ and then compiles it, which makes the release standalone.)
I'd appreciate it, if you would give me feedback and if possible merge my changes into the main repo. I was not quite sure what the relationship between main and build is, but as the workflow files are in build, I worked on this branch. (It should be able to be merged into main without any conflicts.)

My changes:

  • setup of nuitka.yml workflow
  • modification of emailproxy.py to ignore version checks when running as Nuitka
  • tested Nuitka artifact (only windows testing was possible, but should also work on macos; I tested the default behaviour used in our use case)

see for artifacts: https://github.com/sommerf-lf/email-oauth2-proxy/releases/tag/build-1fcb1933

image

@simonrob
Copy link
Owner

Thanks for the PR. It's a simple enough thing to switch to a different packaging system, but I originally chose PyInstaller as it seemed to be the most popular choice in other projects. This popularity of course can lead to issues (e.g., the virus scanner false positives), but since I don't use the pre-built versions myself I didn't really see this as a major problem.

As it happens, I did some work a while ago to make the proxy compatible with Nuitka builds, and the instructions are already in the readme. Can you explain why the version check change is needed? I didn't run into any issues here in my own testing.

@sommerf-lf
Copy link
Author

You're absolutely right! It's just that many users might be discouraged if they need to compile things themselves. Also it's very very unfortunate that many AI-based antivirus programs tend to flag PyInstaller by default.

I'm planning to review the version checking again, probably next week when I have a bit more time. You might be right that the changes aren't necessary anymore. A few months ago, when I was using Nuitka, it had issues with these checks. I noticed you've since switched from pkg_resources to packaging, which might have addressed the earlier problems. I'll look into this and keep you updated on what I find.

Thanks a lot for the excellent tool, by the way!

But would you generally be open to the idea of publishing the Nuitka release as well?

@bwbug
Copy link

bwbug commented Feb 17, 2025

@simonrob This has unfortunately become a major issue that is preventing many users from updating past v2024-9-12. Hopefully, this PR can be merged before any critical bug fixes or security updates are released for email-oauth2-proxy.

@simonrob
Copy link
Owner

I'm happy to switch if it turns out Nuitka packaging is preferable. Could you and/or any other commenters confirm that this option definitely doesn't trigger virus warnings?

At the moment, rather than merging this PR, I'd probably just make the changes in the existing build script to use Nuitka instead since I'm not convinced the other changes are needed.

In addition, if PyInstaller is causing significant problems, as noted above, you can already build this yourself – this has been possible since 50d07c9 (July 2024) which made changes specifically to support Nuitka.

@bwbug
Copy link

bwbug commented Feb 18, 2025

I have confirmed that the emailproxy.exe binary in the emailproxy-2024-11-11_nuitka-Windows.zip file linked by @sommerf-lf can be successfully downloaded on a Windows 11 system, and comes up clear when scanned with Windows Defender.

In contrast, I should clarify that the issue with the official (PyInstaller) version of the 2024-11-11 release is not merely that it triggers "warnings", but rather that it cannot even be downloaded from GitHub without being blocked or immediately quarantined/deleted by Windows Defender and Windows Advanced Threat Protection.

At the moment, rather than merging this PR, I'd probably just make the changes in the existing build script to use Nuitka instead since I'm not convinced the other changes are needed.

Since this is beyond my personal expertise, it would be helpful to see a response from @sommerf-lf to confirm that the two of you have come to a meeting of the minds on the proposed solution.

@sommerf-lf
Copy link
Author

Well talking from my experience (@bwbug, I am no security expert either but have some years experience in deploying python executibles on systems without a python env on it) pyinstaller is often (nearly always) detected as a false positive on windows environments, mainly by Windows Defender itself. Running other AV ourselves (which I wouldn't like to disclose openly, but nothing from VirusTotal's list) pyinstaller is also flagged every time. Nuitka on the other hand offered better compatibility (with the tradeoff of sometimes beeing more hard to get to work if importing nieche libraries).
I apppended the VirusTotal evaluations:

  • PyInstaller (7/60):
    image

  • Nuitka (13/60):
    image

But I think the biggest positive/differentiating factor is Microsoft Defender not flagging Nuitka, as in most environments Microsoft Defender is still working in the background even when using another managed service on top (which should outweight the numerically larger number of detections)

We can see Nuitka is no saints either (even worse on VIrusTotal by numbers), but in the past years it worked better in all environments I had access to. Also I suggest to just add Nuitka to the releases and still keep the notice about Please note that the pre-built executables provided here are packaged automatically ... and let the users (which should probably all be developers or sysadmins, if they need this tool) decide for themselves which to choose.

(For interested ones maybe have a look at this small article: https://medium.com/@markhank/how-to-stop-your-python-programs-being-seen-as-malware-bfd7eb407a7; they also mention that Windows Defender should be the primary goal)
(Will now have a look at pkg_resources vs packaging, but I guess the is_nuitka_compiled function really was unnecessary)

@simonrob
Copy link
Owner

Thanks for the detailed summary. Like I said, I'm happy to switch if it's a better option. I don't think I'll keep both Nuitka and PyInstaller though - I can imagine this leading to questions about which one to use, and potential interoperability issues. There are also PyInstaller-specific things in the proxy at the moment that I'd be happy to remove.

Let me know about the other changes and I'll take a look when I next get chance.

@bwbug
Copy link

bwbug commented Feb 20, 2025

@simonrob As a possible alternative, it might be worthwhile trying the approach outlined in the article that @sommerf-lf linked. Here is the relevant section:

The great news it that Microsoft are awesome at dealing with false positives for malware reports.

They have a website set up which allows you to report false positives: https://www.microsoft.com/en-us/wdsi/filesubmission

You can submit your file (you need a Microsoft Account) along with a short note explaining why you think it’s a false positive. I typically say something like:

Good morning,

My file (attached) has been incorrectly identified as malware by Windows Defender. The identified detection is Trojan:Win32/Wacatac.C!ml.

It is common for files compiled from python using pyinstaller to be incorrectly identified as malware in this way.

This is innocous software which does not contain malware. The source code is available at: https://github.com/hankhank10/findmyplane-client

I would be grateful if you would review and remove this detection.

The first time I submitted this sort of request I fully expected it to disappear into a black hole and for me to never hear back. Far from it. Microsoft are on it when it comes to this.

As soon as you submit you get a tracker id by email which you can use to track your case in real time as they analyse it. They have an actual human review your program and revert to you, in my experience within a few hours. Even on the weekend. Assuming they have deemed it clean it will be whitelisted and the detection removed immediately (although it might take 24 hours for the whitelist to roll out to all users as they update their Windows Defender).

 

@simonrob If you have the time and inclination to perform the above ritual after each new release, then it may be feasible to stay with the PyInstaller builds for Windows.

The best way to test if this would work as advertised by the author of the article, would be to go ahead and submit a false-positive report for the emailproxy-2024-11-11_pyinstaller-Windows.zip file.

What do you think?

@sommerf-lf
Copy link
Author

I had a brief look again at what I did when I earlier (April 2023; on commit 779e70e) made a quickly thrown together way to run using Nuitka. I had to remove the usages of pkg_resources. But this is not necessary anymore, since Nutika seems to play nicely with packaging. (so I removed the 2nd unnecessary python change to check if running Nuitka)
To not further complicate this, I still suggest to offer both, with a sentence of something like the following

Please note that the pre-built executables provided here are packaged automatically directly from the source using PyInstaller and alternatively Nuitka. It is possible that virus scanners will raise false positive warnings about these files. These false positives are common with packaged/compiled python distributables. The PyInstaller and Nuitka builds have the same features and just have different tools for packaging. Pick whichever you AV seems to favor (Windows Defender seems to dislike PyInstaller; For more information look at this discussion). Other significantly relevant differences are not known of here.
If it helps you are welcome to report this problem to the various virus scanning providers (e.g., see #265).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants