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

CURLOPT_PROXY, CURLOPT_SSL_VERIFYHOST, CURLOPT_SSL_VERIFYPEER support #2424

Closed
wants to merge 5 commits into from

Conversation

nowackipawel
Copy link
Contributor

No description provided.

@nowackipawel nowackipawel changed the title CURLOPT_PROXY support CURLOPT_PROXY, CURLOPT_SSL_VERIFYHOST, CURLOPT_SSL_VERIFYPEER support Nov 22, 2019
@MGatner
Copy link
Member

MGatner commented Nov 22, 2019

It looks good to me but we’ll need some tests.

@nowackipawel
Copy link
Contributor Author

Could someone help me with this, @MGatner ? ;-) I needed it in my project so I wrote it and it is working as expected - but I will not find time for tests :(.

@MGatner
Copy link
Member

MGatner commented Nov 23, 2019

I’ll give it a shot, probably next week though.

@nowackipawel
Copy link
Contributor Author

@MGatner: that's gr8. Thank you.

@MGatner
Copy link
Member

MGatner commented Dec 10, 2019

Looking at the current tests, there isn't really a way to verify proxy & SSL behavior beyond making sure they are set and passed to CURL correctly. SSL tests already exist for the verify parameter, but I added a basic proxy one. I think this will suffice, as we don't need to test CURL itself, just the framework's interface with it.

@MGatner
Copy link
Member

MGatner commented Dec 10, 2019

Dig some digging after my test failed, and I think this actually isn't implemented correctly. All options should be applied during setCURLOptions() rather than in send(), and secure-peer has redundant value with the verify config option and should either be removed or merged with existing functionality. secure-host seems to be new content but I'm unclear on if SSL host verification is necessary apart from CURLOPT_SSL_VERIFYPEER.

This reverts commit 62babae.
@lonnieezell
Copy link
Member

@MGatner I agree - it should be handled in setCURLOptions(). I don't believe Host verification is necessary, though, I'm not 100% sure. It's definitely redundant functionality. I would say remove for now.

Is this something you want to tackle still?

@MGatner
Copy link
Member

MGatner commented Dec 29, 2019

I like the idea, especially since it doesn't look like we have a way to do this otherwise which would leave developers extending the library if they needed these. @nowackipawel Are you able to rework this a bit given the feedback above?

@MGatner
Copy link
Member

MGatner commented Jan 31, 2020

@lonnieezell If @nowackipawel is MIA I think this should be closed out. Barring his continuation, I think an implementation of these features would merit a fresh start.

@lonnieezell
Copy link
Member

@MGatner agreed. Closing.

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

3 participants