robinvdvleuten / php-nntp

Client for communicating with servers throught the Network News Transfer Protocol (NNTP) protocol.
MIT License
39 stars 12 forks source link

Forgive me if this is wrong #20

Closed warlord0 closed 8 years ago

warlord0 commented 8 years ago

I'm kinda new to github and version control.

I pulled the project to use in Laravel and thought I'd add a LIST NEWSGROUPS command.

I also found that it wouldn't connect to my usenet host as it used a wildcard certificate. This seemed fairly straight forward to fix by setting the ssl peer_name to match the host name I was using and then the CN given in the cert matched the CN of my host.

robinvdvleuten commented 8 years ago

Hi there @warlord0! Thanks for your PR and in base it all looks good to me. The only concern that I have with the ListNewsGroupsCommand class is that it only lists newsgroups, but the actual NNTP command is only a LIST with a keyword (https://tools.ietf.org/html/rfc3977#section-7.6.1). So a better command would be a ListCommand class and a keyword passed to the constructor. It also misses some php unit tests.

Can you please provide me some more information about your SSL issue? Or a page of an usenet provider with some more details? Then I can look more thoroughly what went wrong there.

Cheers!

warlord0 commented 8 years ago

You're quite right of course. I'm even unsure as how much use the LIST NEWSGROUPS is as it return a massive amount of data. Struggling to find a more practical approach that my provider supports - like LIST ACTIVE, but that returns nothing. I definitely need to play some more.

The SSL issue I had was with newsreader60.eweka.nl:563

If I specify the host as above, by the time it get through to the sll connection it fails telling me that the CN=81.171.92.205 and fails to match the certificate CN=*.eweka.nl

So by setting the peer_name the wildcard match is successful.

HTH

ctrl-f5 commented 8 years ago

FYI @warlord0 I also implemented the LIST command, PR is pending: https://github.com/robinvdvleuten/php-nntp/pull/23

robinvdvleuten commented 8 years ago

@warlord0 @ctrl-f5 I've merged PR #23 instead of this one, cause it seems like a more robust solution. Thanks both for your contributions!

warlord0 commented 8 years ago

:+1: