Implementation of accepting TLS connections

alexcrichton commented 8 years ago

These are a few commits which lead up to the conclusion of accepting server-side TLS connections. There's a bit more here than I initially anticipated, so a close eye would definitely be appreciated! The highlights are:

TLS streams can call AcceptSecurityContext instead of just InitializeSecurityContextW to kick off the process of accepting a stream.
CertStore, CertContext, and CtlContext were all exposed to expose management of keys and such.
Builders in a few locations got arguments to each of these new contexts.
The contexts were expanded with a suite of inspection/creation methods to manage them
A test was added which spins up a server, accepts a connection, communicates a little, and then exits.

The hard part about accepting connections on the server side is that we need the client to trust our custom certificate. Currently the only way I know to do this is to add a certificate to the global trusted root certificate store, so that's what this recommends! I added a script test/create.sh to create a custom CA, sign a certificate for localhost, and then delete the CA key. That way those who want to run tests can add the custom CA. Each test should also be able to detect whether our custom CA is in the system store, and also provides the option to opt-out via environment variables.

It's my intention that the tests run on AppVeyor, but I haven't tested that much, so I'll probably need some tweaks here and there for that.

alexcrichton commented 8 years ago

Ah and this includes the refactoring in https://github.com/steffengy/schannel-rs/pull/5 of a MidHandshakeTlsStream

alexcrichton commented 8 years ago

Oh hey looks like @steffengy your suggestion worked! CI is now passing.

sfackler commented 8 years ago

So to make sure I understand it, the plan for trust roots is for each person to generate a CA, have it sign a certificate, delete the CA private key, install the CA certificate in the user's trust store, and then use the certificate in tests? That seems okay, though i'm a bit worried about adding to the users's trust root, even if we are disposing of the private key.

A possible alternative is to have each test create an ephemeral CA with an expiry of like 1 minute, install it and then uninstall when the test's done.

sfackler commented 8 years ago

I should note that I don't think it's okay to have a git-tracked CA that people are supposed to install.

steffengy commented 8 years ago

Also for creating the certificates it might be advisable to just use powershell? Is there any reason, we're not simply doing:

$cert = New-SelfSignedCertificate -DnsName localhost -CertStoreLocation cert:Localmachine\My
Export-Certificate -Cert $cert -FilePath localhost.cer -Type CERT
$pwd = ConvertTo-SecureString -String "foobar" -Force –AsPlainText
Export-PfxCertificate -Cert $cert -FilePath localhost.pfx -Password $pwd
Import-PfxCertificate -FilePath localhost.pfx -CertStoreLocation cert:\LocalMachine\Root -Password $pwd

in combination with these changes (GIST) to the PR?

(That should ofcourse also work for the user's personal store, for appveyor we'll still need the machine store)

alexcrichton commented 8 years ago

@sfackler yeah it was actually my intention to check in all the certificates to git and contributors who wanted to run these tests would add the certificate to their trusted root store. I figured it's not that much of a risk as the private key is deleted immediately for the CA, so it can only ever sign one key (and that's just for localhost).

That being said I wouldn't mind creating ephemeral keys, I just couldn't figure out how to put something into the root trust store. I wouldn't mind trying it out though, means we may not need to check in any keys at all! You wouldn't happen to know the API off-hand to do that though would you?

@steffengy yeah I'd be fine with that although I'd prefer to make API calls directly rather than user power shell itself (just so a vanilla cargo test works), but it'd all boil down to the same thing I think. Do you not need elevated permissions or dialog clicks to add something to the system certificate store?

alexcrichton commented 8 years ago

@sfackler I've left the debug statements in for now but if you'd prefer I remove them all just wanted to confirm and I'll do so.

steffengy commented 8 years ago

@alexcrichton You need elevated permissions (UAC) for the system certificate store and somehow a dialog pops up for the user (Root-) certificate store, as we've seen with the appveyor tests.

Using the API directly should show the same behavior, which might make a vanilla cargo test difficult to implement that way (Except if you rely on elevated permissions). I'll do some investigating.

steffengy commented 8 years ago

@alexcrichton https://github.com/steffengy/schannel-rs/commit/1a0c4edb43494987841f8f09b8ae8ef3e42f795e

With these changes (requires some cleaning up), a vanilla cargo test works. (elevated) It works the following way:

If no certificate exists in the test folder, it creates a self-signed one and stores it there.
If a certificate exists in the test folder, it searches for it in the System Root LocalMachine store,
checks the validity (if it has expired yet) and either
- uses the stored one (if found and valid)
- or creates a new one, as if none exists.

The certificate is valid for 5 minutes, which should (currently) be enough to run all tests.

But we probably should land this first, other stuff can be improved later.

sfackler commented 8 years ago

I figured it's not that much of a risk as the private key is deleted immediately for the CA, so it can only ever sign one key (and that's just for localhost).

Whatever random person last regenerated the CA claims that they deleted the private key.

If elevated permissions are required to do the ephemeral thing, then a long-lived per-developer CA seems fine for now. I'd ideally like to get rid of the openssl stuff and generate them with native windows APIs (dogfood!) but that's not super necessary.

alexcrichton commented 8 years ago

Thanks for the commit @steffengy! I'll take that strategy and wrap it all up in some APIs as well.

steffengy commented 8 years ago

@alexcrichton Since we cannot bypass the elevation, the short validity may be counter-productive.

Probably setting the validity to one year is the best option, should not be a high security risk, since it's all locally generated. 962f7f928beefeb6fb6cf48d6e7d58bebd0498f0 should do that.

This would only require elevation on the first run, and probably is indeed the best option. (@sfackler also seems to prefer this)

alexcrichton commented 8 years ago

Ok, I've pushed an updated using @steffengy's strategy. I opted to have certificates valid for a day as it basically just means that once a day if you run tests you have to click a dialog twice (once to delete, once to add), and it's somewhat less dangerous than a year.

I added an API or two along the way, but we still deal with creating the self-signed certificate in a raw fashion. Outstanding questions are:

Do we want to just remove CertStore::memory? I don't think the final version is going to end up using it.
Do we want to just not expose CtlContext for now?

sfackler commented 8 years ago

Let's leave CtlContext private until we have a chance to add in signing, yeah, and same with Memory.

alexcrichton commented 8 years ago

Ok, left both of those private and I just removed the bits in the README

alexcrichton commented 8 years ago

Made the password on pkcs12 optional and added an enum argument to add_cert

sfackler commented 8 years ago

Woo!

steffengy / schannel-rs

Implementation of accepting TLS connections #7