phergie / phergie-irc-client-react

IRC client library built on React
BSD 3-Clause "New" or "Revised" License
56 stars 26 forks source link

Implement ssl context which was implemented in react socket-client 0.5.0 #58

Closed mvangoor closed 7 years ago

elazar commented 7 years ago

I believe this is intended to address #25 ?

mvangoor commented 7 years ago

Hi elazar,

Yes this also fixes #25, however that is not why I changed the code. The reason is that there was no way to actually pass along any ssl context options. With this you can do so and I added 2 extra options.

I will look at your review and update the code.

elazar commented 7 years ago

@svpernova09 @matthewtrask I'd suggest manual testing of SSL and non-SSL connections after applying this change.

elazar commented 7 years ago

@svpernova09 I believe @mvangoor started this work in order to be able to use SSL with self-signed certificates. Is there any further manual testing we should do for that use case?

svpernova09 commented 7 years ago

@elazar Yes, in that case we need to find an IRC server that is using self signed cert to test this against. @mvangoor Can you share a server that is using self signed certs?

mvangoor commented 7 years ago

Hi,

The reason for adding it is not the fact that (to my knowledge) any public ircd server is serving ssl without a signed cert. But rather that I am running a private (public reachible) irc server with self-signed certificate with a connection password.

I'll see if I can find a self-signed certificate ircd out there.

mvangoor commented 7 years ago

My private ircd is at: whoever.probie.nl:7000, but requires a password to connect

Found anoter one: ircsource.baconsvin.org:6697, but the certificate has expired

Sorry to say that I cannot find another at this time

matthewtrask commented 7 years ago

@mvangoor Im sitting with @svpernova09 and we are both having a hard time trying to get it up and running for testing. Can you please provide instructions for us to test properly?

mvangoor commented 7 years ago

Hi,

I have been thinking about this for a while now and how to get this moving forward. I see 2 options currently:

Please let me know if you think option 1 should be focused on or option 2.

elazar commented 7 years ago

@mvangoor My advice would be to focus on option 1. Connection handling is one of the larger parts of the client and should probably be refactored into a separate supporting class, but I think that would be best done in its own PR apart from the work you've already done in this one.

mvangoor commented 7 years ago

Okay!

I have a test connection for you to test. I will be removing the part about "verify_peer" as I cannot test it at this time and actually do not need it.

Once I have all the rework done I'll post a connection config so you can test it.

mvangoor commented 7 years ago

Hi,

Please use this options array to create a connection. $options = array( 'serverHostname' => '***', 'serverport' => 7000, 'password' => 'Temporary_phergie_testing', 'username' => 'testphergie', 'realname' => 'Phergie Client react update ^0.5', 'nickname' => 'testphergie', 'options' => array( 'transport' => 'ssl', 'allow-self-signed' => true, ) );

elazar commented 7 years ago

@mvangoor The library README should probably be updated with any new configuration options.

mvangoor commented 7 years ago

@elazar: Fixed the README

matthewtrask commented 7 years ago

thanks @mvangoor!

0x6B386F commented 6 years ago

@mvangoor is there a chance add verify_peer and verify_peer_name? I understand, it is difficult to test that.. but this basic options are often needed, lot of servers arent configured properly. It would be nice to have support for this :) Thank you.

mvangoor commented 6 years ago

Hi 1dal,

Yes that is possible, however I have not looked at phergie in 1 year as I ended up doing other stuff.

This pull request is basically everything I needed to do to add support for self_signed option.