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

add support for DER and P12 certs (#2411) #2413

Merged
merged 4 commits into from Mar 10, 2021

Conversation

razvanphp
Copy link
Contributor

@razvanphp razvanphp commented Dec 1, 2019

@Nyholm
Copy link
Member

Nyholm commented Sep 22, 2020

I think this makes sense. Could you add a test for it?

Also, it needs a rebase.

@Nyholm
Copy link
Member

Nyholm commented Oct 11, 2020

Friendly ping

@razvanphp
Copy link
Contributor Author

ok, I will try to do that.

@GrahamCampbell
Copy link
Member

I think this could be added to the 7.3.0 milestone (for November??? release).

@maksimovic
Copy link

Any idea when this will make its way to the release? We rely on Guzzle and are in need of P12 cert support pretty urgently, so I'm checking in here to see what our options are. Thanks.

@Nyholm
Copy link
Member

Nyholm commented Nov 13, 2020

I need a test for this PR before we can review and merge it.
After merge we are usually pretty quick to release it.

We haven't heard from the author for about a year. @maksimovic feel free to open a new PR with this fix and a test. That will probably be the quickest way to get this released.

@Nyholm
Copy link
Member

Nyholm commented Nov 13, 2020

Ooops. @razvanphp answered a month ago.. What is the status? Do you need help from @maksimovic?

@razvanphp
Copy link
Contributor Author

I'm on it 🙂 didn't work on the project that needed this for some time...

@razvanphp
Copy link
Contributor Author

Rebased, tests added, thank you!

@Nyholm
Copy link
Member

Nyholm commented Nov 13, 2020

Thank you

@maksimovic Could you give this a review and make sure that it works?

@razvanphp
Copy link
Contributor Author

@maksimovic ping :)

@maksimovic
Copy link

Unfortunately, we opted out of using Guzzle because we're using an older version it, but can't upgrade it quickly since it's the vital part of our stack and also includes bringing some other dependencies to a more recent version, which then have their dependencies requiring version bump, so it turned out to be way more work that we anticipated.

@razvanphp
Copy link
Contributor Author

@Nyholm what do you think? can we merge this tho? I've already tested it in our application, also the tests are included in the PR.

@nuernbergerA
Copy link

Hey @Nyholm we need p12 support in our application too.

Is there still anything blocking this from being merged?

@Nyholm
Copy link
Member

Nyholm commented Mar 10, 2021

@nuernbergerA Have you tested this patch in your app? Does it work as expected?

@nuernbergerA
Copy link

@Nyholm yes it works.

But if there is any reason you want to reject the PR I'm fine to include the curl key.

'curl' => [
    CURLOPT_SSLCERTTYPE => 'P12',
],

@Nyholm
Copy link
Member

Nyholm commented Mar 10, 2021

No. I don't want to reject it. I want to be comfortable that merging this fixes the issue and does not break anything.

I'm not familiar with the feature but since many people request this, I encourage people to verify the patch and not only bump the thread.

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

@Nyholm Nyholm merged commit 9687c73 into guzzle:master Mar 10, 2021
@GrahamCampbell GrahamCampbell added this to the 7.3.0 milestone Mar 22, 2021
@razvanphp razvanphp deleted the add-p12-cert-support branch December 4, 2023 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for P12 cert format via CURLOPT_SSLCERTTYPE
5 participants