-
Notifications
You must be signed in to change notification settings - Fork 26
Implement ssl context which was implemented in react socket-client 0.5.0 #58
Conversation
This adds the ability to parse along a secure context This also resolved issue #25 Additionally added 2 new socket options for ssl: allow-selfsigned allow-verify_peer If you set these 2 you should be able to connect to self-signed certificate enabled ssl irc servers. Obviously only use this for developing and testing, in production you should always connect properly signed ssl enabled irc server.
composer.lock
Outdated
}, | ||
{ | ||
"name": "monolog/monolog", | ||
"version": "1.17.2", | ||
"version": "1.22.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes to composer.lock
should only reflect what was changed in composer.json
(i.e. that react/socket-client
was upgraded).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point, composer is new to me.
I fixed this.
tests/ClientTest.php
Outdated
} | ||
} | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test should be modified to ensure that both of these options can now be enabled simultaneously.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am uncertain how to test this and that is why I commented it out.
The actual socket for Secure Connections is created in TcpConnecter (react/socket-client) and as far as I can see cannot be requested to check the context options.
I did find I could get the context options using:
stream_context_get_options($connection->getData('stream'))
However getData is not setup in MockConnection.
Could you help me on this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/Client.php
Outdated
if ($this->getAllowSelfSignedFlag($connection)) { | ||
$context['allow_self_signed'] = true; | ||
} | ||
//$context = array('socket' => $context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think removing this line, by commenting it out or otherwise, makes this code incorrect. See these examples of using socket context options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The context we send to react socket-client for a secure connection is different.
As far as I can see passing the socket is not valid, I should have deleted that line and did that now.
Maybe I am wrong, but if I look at SecureConnector.php at react/socket-client it is doing a completely different thing than for a regular connection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're correct, SecureConnector
appears to expect socket options specifically. However, it also appears that TcpConnector
does as well in this newer version of react/socket-client
. As such, this PR will need to update this line as well to avoid breaking non-secure connections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me see if I understand you correctly here.
We should also update the regular (non-ssl) connection processing to the new 0.5.* range and send the context directly to react.
This means that functions like getSocket() will no longer be required.
If that is what you mean I will see if I can update the regular connection process aswell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first glance, it looks like it would be possible to do so, but I think that may be better addressed in a separate PR once this one is merged unless there's compelling reason to do otherwise.
Alternatively, we could have a PR that does nothing but update react/socket-client
to ^0.5
, and this and the other prospective PR could be based against that one, so that the two can be developed concurrently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you are right and we should use a seperate PR to look at socket connections in general.
I updated code to reflect discussions
src/Client.php
Outdated
*/ | ||
protected function getAllowSelfSignedFlag(ConnectionInterface $connection) | ||
{ | ||
return (boolean) $connection->getOption('allow-selfsigned') ?: false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use allow-self-signed
instead of allow-selfsigned
here, for consistency with the socket option being set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
src/Client.php
Outdated
*/ | ||
protected function getVerifyPeerFlag(ConnectionInterface $connection) | ||
{ | ||
return (boolean) $connection->getOption('allow-verify_peer') ?: false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use allow-verify-peer
instead of allow-verify_peer
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed it as I cannot find a proper way of testing this and we might need to refactor socket connections completely due to new insights and possibilities of react socket-client
I believe this is intended to address #25 ? |
Hi elazar, Yes this also fixes #25, however that is not why I changed the code. I will look at your review and update the code. |
composer.json
Outdated
@@ -16,7 +16,7 @@ | |||
"phergie/phergie-irc-connection": "~2.0", | |||
"react/event-loop": "~0.4.0", | |||
"react/stream": "~0.4.2", | |||
"react/socket-client": "~0.4.2", | |||
"react/socket-client": "~0.5.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend changing ~0.5.0
to ^0.5
. It appears that 0.5.3 is available, while this version restriction is only allowing 0.5.1 to be installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/Client.php
Outdated
if ($this->getAllowSelfSignedFlag($connection)) { | ||
$context['allow_self_signed'] = true; | ||
} | ||
//$context = array('socket' => $context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're correct, SecureConnector
appears to expect socket options specifically. However, it also appears that TcpConnector
does as well in this newer version of react/socket-client
. As such, this PR will need to update this line as well to avoid breaking non-secure connections.
tests/ClientTest.php
Outdated
$this->client->setResolver($this->getMockResolver()); | ||
$this->client->addConnection($connection); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's sufficient for this test to simply call addConnection()
as above. So long as an exception is no longer thrown, that's all the test should need to verify. I'm skeptical that it would be worth the effort to test for lower-level conditions beyond that.
@svpernova09 @matthewtrask I'd suggest manual testing of SSL and non-SSL connections after applying this change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing w/ SSL:
2017-02-04 18:16:02 DEBUG TestPhergie!Elazar@irc.freenode.net NICK :TestPhergie []
2017-02-04 18:16:02 DEBUG TestPhergie!Elazar@irc.freenode.net :verne.freenode.net NOTICE * :*** Looking up your hostname... []
2017-02-04 18:16:02 DEBUG Attaching activity listener for connection []
2017-02-04 18:16:02 DEBUG TestPhergie!Elazar@irc.freenode.net :verne.freenode.net NOTICE * :*** Checking Ident []
2017-02-04 18:16:02 DEBUG TestPhergie!Elazar@irc.freenode.net :verne.freenode.net NOTICE * :*** Found your hostname []
2017-02-04 18:16:10 DEBUG TestPhergie!Elazar@irc.freenode.net :verne.freenode.net NOTICE * :*** No Ident response []
2017-02-04 18:16:10 DEBUG TestPhergie!Elazar@irc.freenode.net :verne.freenode.net 001 TestPhergie :Welcome to the freenode Internet Relay Chat Network TestPhergie []
2017-02-04 18:16:10 DEBUG TestPhergie!Elazar@irc.freenode.net :verne.freenode.net 002 TestPhergie :Your host is verne.freenode.net[185.30.166.37/6697], running version ircd-seven-1.1.4 []
2017-02-04 18:16:10 DEBUG TestPhergie!Elazar@irc.freenode.net :verne.freenode.net 003 TestPhergie :This server was created Sat Jan 7 2017 at 11:53:54 EST []
2017-02-04 18:16:10 DEBUG TestPhergie!Elazar@irc.freenode.net :verne.freenode.net 004 TestPhergie verne.freenode.net ircd-seven-1.1.4 DOQRSZaghilopswz CFILMPQSbcefgijklmnopqrstvz bkloveqjfI []
2017-02-04 18:16:10 DEBUG TestPhergie!Elazar@irc.freenode.net :verne.freenode.net 005 TestPhergie CHANTYPES=# EXCEPTS INVEX CHANMODES=eIbq,k,flj,CFLMPQScgimnprstz CHANLIMIT=#:120 PREFIX=(ov)@+ MAXLIST=bqeI:100 MODES=4 NETWORK=freenode STATUSMSG=@+ CALLERID=g CASEMAPPING=rfc1459 :are supported by this server []
2017-02-04 18:16:10 DEBUG TestPhergie!Elazar@irc.freenode.net :verne.freenode.net 005 TestPhergie CHARSET=ascii NICKLEN=16 CHANNELLEN=50 TOPICLEN=390 DEAF=D FNC TARGMAX=NAMES:1,LIST:1,KICK:1,WHOIS:1,PRIVMSG:4,NOTICE:4,ACCEPT:,MONITOR: EXTBAN=$,ajrxz CLIENTVER=3.0 ETRACE WHOX KNOCK :are supported by this server []
2017-02-04 18:16:10 DEBUG TestPhergie!Elazar@irc.freenode.net :verne.freenode.net 005 TestPhergie SAFELIST ELIST=CTU CPRIVMSG CNOTICE :are supported by this server []
2017-02-04 18:16:10 DEBUG TestPhergie!Elazar@irc.freenode.net :verne.freenode.net 251 TestPhergie :There are 137 users and 83932 invisible on 27 servers []
Testing without SSL
2017-02-04 18:19:53 DEBUG TestPhergie!Elazar@irc.freenode.net USER Elazar 0 * :Matthew Turland []
2017-02-04 18:19:53 DEBUG TestPhergie!Elazar@irc.freenode.net NICK :TestPhergie []
2017-02-04 18:19:53 DEBUG TestPhergie!Elazar@irc.freenode.net :niven.freenode.net NOTICE * :*** Looking up your hostname... []
2017-02-04 18:19:53 DEBUG Attaching activity listener for connection []
2017-02-04 18:19:53 DEBUG TestPhergie!Elazar@irc.freenode.net :niven.freenode.net NOTICE * :*** Checking Ident []
2017-02-04 18:19:54 DEBUG TestPhergie!Elazar@irc.freenode.net :niven.freenode.net NOTICE * :*** Found your hostname []
2017-02-04 18:19:59 DEBUG TestPhergie!Elazar@irc.freenode.net :niven.freenode.net NOTICE * :*** No Ident response []
2017-02-04 18:19:59 DEBUG TestPhergie!Elazar@irc.freenode.net :niven.freenode.net 001 TestPhergie :Welcome to the freenode Internet Relay Chat Network TestPhergie []
2017-02-04 18:19:59 DEBUG TestPhergie!Elazar@irc.freenode.net :niven.freenode.net 002 TestPhergie :Your host is niven.freenode.net[139.162.227.51/6667], running version ircd-seven-1.1.4 []
2017-02-04 18:19:59 DEBUG TestPhergie!Elazar@irc.freenode.net :niven.freenode.net 003 TestPhergie :This server was created Thu Jan 5 2017 at 04:18:05 UTC []
2017-02-04 18:19:59 DEBUG TestPhergie!Elazar@irc.freenode.net :niven.freenode.net 004 TestPhergie niven.freenode.net ircd-seven-1.1.4 DOQRSZaghilopswz CFILMPQSbcefgijklmnopqrstvz bkloveqjfI []
2017-02-04 18:19:59 DEBUG TestPhergie!Elazar@irc.freenode.net :niven.freenode.net 005 TestPhergie CHANTYPES=# EXCEPTS INVEX CHANMODES=eIbq,k,flj,CFLMPQScgimnprstz CHANLIMIT=#:120 PREFIX=(ov)@+ MAXLIST=bqeI:100 MODES=4 NETWORK=freenode STATUSMSG=@+ CALLERID=g CASEMAPPING=rfc1459 :are supported by this server []
2017-02-04 18:19:59 DEBUG TestPhergie!Elazar@irc.freenode.net :niven.freenode.net 005 TestPhergie CHARSET=ascii NICKLEN=16 CHANNELLEN=50 TOPICLEN=390 DEAF=D FNC TARGMAX=NAMES:1,LIST:1,KICK:1,WHOIS:1,PRIVMSG:4,NOTICE:4,ACCEPT:,MONITOR: EXTBAN=$,ajrxz CLIENTVER=3.0 SAFELIST ELIST=CTU CPRIVMSG :are supported by this server []
2017-02-04 18:19:59 DEBUG TestPhergie!Elazar@irc.freenode.net :niven.freenode.net 005 TestPhergie CNOTICE WHOX ETRACE KNOCK :are supported by this server []
2017-02-04 18:19:59 DEBUG TestPhergie!Elazar@irc.freenode.net :niven.freenode.net 251 TestPhergie :There are 135 users and 83941 invisible on 27 servers []
I don't see any issues.
@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? |
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. |
My private ircd is at: Found anoter one: Sorry to say that I cannot find another at this time |
@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? |
tests/ClientTest.php
Outdated
$this->client->setResolver($this->getMockResolver()); | ||
$this->client->addConnection($connection); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Github is showing this as commented out? Is there a reason there is a commented out test in the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been struggling with this specific test and asked elazar to help me with it.
Once we have figured that out this will be resolved.
tests/ClientTest.php
Outdated
$this->assertSame(Exception::ERR_CONNECTION_STATE_UNSUPPORTED, $e->getCode()); | ||
} | ||
print_r(stream_context_get_options($connection->getData('stream'))); | ||
//print_r($actual); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been struggling with this specific test and asked elazar to help me with it.
Once we have figured that out this will be resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I have been thinking about this for a while now and how to get this moving forward.
Please let me know if you think option 1 should be focused on or option 2. |
@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. |
Okay! I have a test connection for you to test. Once I have all the rework done I'll post a connection config so you can test it. |
…d be found as of yet Finally fix the commented test and make sure we can now add a ssl connection with ipv4 binding
Hi, Please use this options array to create a connection. |
*/ | ||
protected function getAllowSelfSignedFlag(ConnectionInterface $connection) | ||
{ | ||
return (boolean) $connection->getOption('allow-self-signed') ?: false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, just noticed that Phergie configuration uses camel case (e.g. serverHostname
) rather than this style (i.e. allow-self-signed
). Would you mind updating this to use camel case as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a problem, I followed the existing options as example.
See 'force-ipv4' that should have been 'forceIpv4' ?
If we go for camel case we should update them all, however this breaks api ...
just my 0.02.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point. Leave this as-is, then.
@mvangoor The library README should probably be updated with any new configuration options. |
@elazar: Fixed the README |
thanks @mvangoor! |
@mvangoor is there a chance add |
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. |
No description provided.