search

OpenVPN / tap-windows6

Windows TAP driver (NDIS 6)
Other
788 stars 237 forks source link

Add external manifest file for tapinstall.exe #22

Closed mattock closed 7 years ago

mattock commented 7 years ago

Code-vise this PR "should just work". The manifest file has been tested (see Trac#153) so it works fine. I will create a new tap-windows6 installer and test it. Then we can consider merging this.

mattock commented 7 years ago

A tap-windows-9.21.2 installer that implements this PR is here:

I tested it quick and it seemed to work properly. The timestamp of tapinstall.exe is not explicitly updated yet.

Right now that installer is only signed with a single SHA2 user-mode authenticode certificate. If I recall correctly, the current, official installer is first signed with SHA1 during build, then a EV SHA2 signature is appended to it.. So the installer is probably not release-ready yet, but good enough for testing.

selvanair commented 7 years ago

Cant seem to connect to Trac right now, so commenting here: I've noticed the need for touching the timestamp on the exec for external manifest to work, but its hard to reproduce. Appears to matter only if the executable already exists and has been run before the manifest is copied in.

Will test this installer.

selvanair commented 7 years ago

Tested successfully on WIndows 7 64 bit and server 2008 32 bit (equal to Vista?) -- I didnt know one installer works on both 32 and 64 bit. Timestamp was not updated, tried both with an old tapinstall present before install and clean install. Works from powershell too..

Only one issue: UAC causes tapinstall.exe to be run in a separate command window which automatically closes on completion. So the user cannot see the output of the command. The pause in addtap.bat was meant for that, but doesn't help with UAC. How can we keep the window running tapinstall.exe open?

EDIT: by the way the license text shown has no line breaks -- nsis needs windows style CRNLs?

mattock commented 7 years ago

I also noticed that cmd.exe pops up second window issue. Whether that is a big issue depends on how quickly tapinstall runs. In my case it sometimes ran too quickly to see anything, and sometimes it took several seconds.

I'll have a look at the license text.

mattock commented 7 years ago

I added a commit that seems to fix the linefeeds.

I'm not sure why it was originally necessary to convert the linefeeds: Git for Windows defaults to checking out files with CRLF linefeeds, so if one builds tap-windows6 on Windows (as one must), then no linefeed conversion is necessary. Test installer here:

The installer displays the license file correctly now. The linefeeds are also correct when opening When C:\Program Files\TAP-Windows\license.txt manually with notepad.exe.

mattock commented 7 years ago

A few ways to "touch" tapinstall.exe:

selvanair commented 7 years ago

For file time stamp it may be enough to do that before packaging.?? Then the installed file will have a timestamp newer than the copy that was already present, if any. Whether this is really enough or not I don't know.

selvanair commented 7 years ago

@mattock: Here is an alternate approach in this branch. This just sets the addtap and deltap shortcuts added to the start menu to run as admin. Avoids the issue of output "lost" in the UAC cmd window and external manifest woes. But directly running addtap.bat or tapinstall.exe will not work though -- only shortcuts can be made to run as admin this way.

I cant properly test this without a setup for building the driver, but independent test of the added function shows it works on Win7 at least.

mattock commented 7 years ago

The alternate approach in https://github.com/selvanair/tap-windows6/commit/3cdd3cb4008e099774a367405444a1d940c3d81e works as intended on Windows 2012r2. Moreover, modifying addtap.bat/deltap.bat would solve the problem well enough imho. My only worry is IShellLink_Set_RunAs_flag itself: it is quite nasty piece of work for the untrained eye, and according to Wiki history it has not been updated since 2010. We might have to fix it ourselves if it happens to break.

Based on a quick search the ShellLink plug-in can serve the same purpose and as a project it seems to be semi-active at least.

Thoughts?

selvanair commented 7 years ago

@mattock: good point, at least I will not be able to fix if that IShellLink code if it breaks. A cleaner way would be to have a dll that exposes the IShellLink COM interface. I'll checkout the plugin you mentioned.

selvanair commented 7 years ago

@mattock: opened a PR that uses the dll for ShellLink that you pointed out. The PR has both commits, to be squashed if this works out.

mattock commented 7 years ago

@selvanair : I'm inclined to closing this PR without merging. The solution in PR https://github.com/OpenVPN/tap-windows6/pull/23 does not have the caveats of this external manifest, and has fixed the actual problem quite nicely for less-advanced users. More advanced users should be able to launch an elevated cmd.exe/Powershell prompt and to run tapinstall.exe manually.

Shall I close this PR?

selvanair commented 7 years ago

@mattock: Yes, this may be closed.