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

SSL in socket listener #1

Merged
merged 1 commit into from
Jan 10, 2015
Merged

Conversation

jabbera
Copy link
Contributor

@jabbera jabbera commented Jan 6, 2015

I'm dedicated to adding SSL support to media browser. This is just a proof of concept that it will work. If I work out the kinks, test with mono, etc etc would this be something you would be willing to accept? Any gotchas I should be looking out for?

@LukePulverenti
Copy link
Member

Great stuff! Yes of course, if we can get this perfected then absolutely we'll merge it in.

Fyi - you should base this off of the mono http listener, since it already supports SSL, and this library is based off of that. If you check out the readme, i originally based off of websocket-sharp. As part of reverting it back to look more like the mono code, i more or less ignored SSL. There's a good chance you can base it off that:

https://github.com/mono/mono/tree/master/mcs/class/System/System.Net

(Although you might already know this).

@jabbera
Copy link
Contributor Author

jabbera commented Jan 6, 2015

I started doing that but I ended up pulling about 30 classes in from Mono.Security fro SslServerStream and it showed no signs of slowing.

Switching to SslStream (.NET native) from SslServerStream (Mono.Security) resolved the issue. I'm not opposed to bringing in SslServerStream but before I do I wanted to confirm that you actually want potentially 30+ classes brought into this project when there seems to be a working SslStream in mono which I've linked below. (We could also bring in a reference to Mono.Security and any dependencies and use it from windows as well)

I have no idea why this functionality seems to be duplicated in Mono.Security so I'm obviously lacking some context to make an informed decision.Hoping you might be able to shed some light on the issue.

https://github.com/mono/mono/blob/effa4c07ba850bedbe1ff54b2a5df281c058ebcb/mcs/class/System/System.Net.Security/SslStream.cs

@jabbera
Copy link
Contributor Author

jabbera commented Jan 6, 2015

Actually after spending 2 minutes looking at sslstream.cs in mono I see it's built using SslServerStream. Doesn't help us in the windows world unfortunately.

@LukePulverenti
Copy link
Member

we can't use the .net SslStream?

@jabbera
Copy link
Contributor Author

jabbera commented Jan 6, 2015

We can, it's what my proof of concept does, but it's a deviation from the mono HttpListener implementation. Its a tradeoff. Use SslServerStream and bring in every dependent class (or reference Mono.Security) or just use SslStream even though it's not what the mono httplistener does.

@jabbera jabbera force-pushed the master branch 3 times, most recently from 609bb1c to 27ee266 Compare January 6, 2015 16:38
@jabbera
Copy link
Contributor Author

jabbera commented Jan 6, 2015

This would be my recommended change. What is your opinion on it?

@jabbera jabbera changed the title Proof of concept SSL in socket listener SSL in socket listener Jan 6, 2015
@LukePulverenti
Copy link
Member

We can go with your research. As long as it's unit tested i'm comfortable with it.

@jabbera
Copy link
Contributor Author

jabbera commented Jan 7, 2015

This is pretty isolated and low risk as you can see. The changes only come into play with an ssl endpoint. (I don't have a mac to test, but linux and windows are fine.)

The changes in media browser are much more involved. We need to discuss a handful of issues. If you want me to start a new thread in that project let me know, but these are the issues I've come up with so far:

Do you want both an http and https endpoint hosted? I don't know the complications of hosting 2 different endpoints, but I can imagine it's complicated. (Which to register in media connect?) Any generated urls need to know the ssl or not to generate absolute urls.

If the ssl cert's common name is issued to "home.test.com" for example, how does this impact media browser connect? I'm not privy to the implementation but could the common name not being "app.mediabrowser.tv" cause an issues?

Another connect issue is how the registration works. Does it just send ip? A full URL? etc etc.

Anything else you can think of or serious issues I need to consider?

@LukePulverenti
Copy link
Member

I think that aspect of MBS and clients will be ok because servers are always treated as url's. With connect the client gets the url from the connect api. Without connect it's up to users to manually enter or auto-discover on the lan. I had ssl in mind when designing connect so i think that will be ok.

The tricky part is going to be port authorization on windows. It will require an admin prompt the first time, and since updates don't require that i think this is only going to be enabled for new installations by default. And then i'll have to give users a way to enable it.

Out of the box we'll definitely support both http and https. We can have multiple endpoints so no big deal there.

@jabbera
Copy link
Contributor Author

jabbera commented Jan 7, 2015

Glad to hear. I usually run as an admin so I had no thought of port auth. (I hate netsh)

Is there anything to worry about with DLNA?: RegisterServerEndpoints

            var uri = new Uri(string.Format("http://{0}:{1}{2}", address, _config.Configuration.HttpServerPortNumber, descriptorURI));

PlayToManager.cs

private string GetServerAddress(IPAddress localIp)
{
return string.Format("{0}://{1}:{2}/mediabrowser",

            "http",
            localIp,
            _appHost.HttpServerPort
            );
    }

@LukePulverenti
Copy link
Member

yea those are easily changed and as long as we're supporting both for a period of time, it won't be a big deal.

@LukePulverenti
Copy link
Member

plus i think we will probably always support both and just have an option to force ssl

Allow passing in certificate file location.
@jabbera
Copy link
Contributor Author

jabbera commented Jan 8, 2015

This is updated to allow passing in of the certificate.

LukePulverenti added a commit that referenced this pull request Jan 10, 2015
@LukePulverenti LukePulverenti merged commit c177055 into MediaBrowser:master Jan 10, 2015
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.

None yet

2 participants