OpenVPN / tap-windows6

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

initial commit, install publisher cert into Trusted publishers store #31

Closed chipitsine closed 6 years ago

chipitsine commented 7 years ago

I created some prototype for resolving https://github.com/OpenVPN/tap-windows6/issues/26 is such an approach good ?

if so, I'll wrap it with current build.py logic

this PR is not supposed to be merged

mattock commented 7 years ago

I think this approach is simple and good. I also believe this is what other people tend to do to achieve the same goal. I'll do some testing tomorrow to verify that this works as intended.

chipitsine commented 7 years ago

it works as expected. I'll add python logic and nsis hooks around this. I think we should leave default behaviour unchanged (certificate will not be installed automatically), but add a posibility to force such an installation by command line option.

the original question was related to "certutil" usage. I could not get it working using "crypt32:CertOpenStore" calls unfortunately.

mattock commented 7 years ago

In most cases people install tap-windows6 by running an OpenVPN installer. Therefore we need to add the same parameter ("install publisher certificate") to openvpn.nsi and pass that to tap-windows6.nsi.

I think certutil.exe approach is good enough as long as it's included in all supported Windows versions (Vista -> 10).

mattock commented 7 years ago

Similar certutil injection is documented in this Trac ticket.

mattock commented 7 years ago

Right now I don't have access to the driver-signing computer. Once I get acces, I will produce new tap-windows6 installer which includes this patch.

chipitsine commented 7 years ago

it might be ok for one time installer release.

give me a week, I'll add "if/else" and automate it, a bit busy right nnow

mattock commented 7 years ago

@chipitsine : Ok. Getting the new driver built and signed will take me a week anyways :smile:.

mattock commented 7 years ago

Here's a installer package based on this PR:

Based on my testing it works as expected. My Powershell test procedure was this:

# Basic uninstall
& 'C;\Program Files\TAP-Windows\Uninstall.exe' /S

# Get rid of cached publisher certificate. This command removes it
# from CurrentUser as well.
Remove-Item Cert:\LocalMachine\TrustedPublisher\5E66E0*

# This gets rid of all tap-windows6 drivers in the driver store. Script is from
# https://github.com/mattock/tap-windows-scripts
Remove-Tapwindows.ps1 -Yes

# Install
& .\tap-windows-9.21.2-I901.exe

The install log shows that the certificate is added to the TrustedPublisher store correctly, and the driver warning message box does not show up.

A few things that need to be fixed still:

We also need injection support in openvpn-build/windows-nsis/openvpn.nsi. A few ideas:

@chipitsine @selvanair : thoughts?

selvanair commented 7 years ago

Haven't tested it yet, but going by the description, all good changes except removal of all tap windows drivers from the driver store. Although this makes life simpler in many cases, I think it should be optional --- could be enabled by default. I, for example, keep many drivers in the driver store for testing purposes.

mattock commented 7 years ago

@selvanair : removing the drivers from the driver store was just for testing this PR. I've read somewhere that if the driver already exists in the driver store, then the publisher certificate check does not kick in. I have not verified that myself..

chipitsine commented 7 years ago

well, I was absent ... for few days :) now back to this PR.

previously I thought the good idea is to pick certificate right from command arguments (and pass it to installer).

now, I think it is bad idea because of https://github.com/mattock/sign-tap6 @mattock ?

so, it seems to be better to specify (multiple) certificates, that are going to be added to trusted store, separately ?

mattock commented 7 years ago

@chipitsine now that I think of it, probably the best approach would be to

Essentially, we'd have something like

> buildtap.py <other-params> --inject <certfile> --inject <certfile>

or

> buildtap.py <other-params> --inject <certfile>,<certfile>

The same --inject parameter would be added to openvpn-build and passed on to tap-windows6 installer.

Thoughts?

chipitsine commented 7 years ago

it matches my (current) thoughts.

mattock commented 7 years ago

One more thing: we need a run-time "inject" option in addition to a build-time option. This allows the user (or the OpenVPN installer) to skip publisher certificate injection, even if the certficates are included in tap-windows6 installer.

chipitsine commented 7 years ago

it is current (default) behaviour. I'm going to leave it as is

chipitsine commented 6 years ago

this is stale. need to be rewritten

rozmansi commented 6 years ago

FYI, the MSI installer I am working on installs signer's certificate in the computer's trusted publisher store. After the MSI packaging is ready, you might reconsider NSIS installer future.