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 SSLCERT/SSLKEY/KEYPASSWD curlopts #217

Closed
wants to merge 4 commits into from
Closed

Add support for SSLCERT/SSLKEY/KEYPASSWD curlopts #217

wants to merge 4 commits into from

Conversation

mway
Copy link

@mway mway commented Sep 16, 2017

this addresses cases where a client cert is needed for authorization.

additionally, instead of type aliases (via using declarations) for types that are effectively strings, use mixins to make their use unambiguous (i.e. wrt overloads).

certainly open to any feedback, functional or cosmetic. if the implementation looks good, i'll add tests. i got bored and added them. 😉

@mway
Copy link
Author

mway commented Sep 16, 2017

not sure why clang is failing on travis yet. both gcc4.8 and clang3.8 look OK on my end, but i'm also building with Bazel (not cmake). will look into it.

@mway
Copy link
Author

mway commented Sep 16, 2017

ok, things look pretty good on my end, let me know what you think! looking into proxy test failures i'm seeing (connection errors), not sure if it's my network. edit: appears to be due to my machine/network, travis says it's fine.

@mway
Copy link
Author

mway commented Sep 17, 2017

  • added StringHolder tests
  • explicit StringHolder ctors, added std::initializer_list<std::string> ctor
  • updated derivative classes (Url, SslCert, SslKey, SslKeyPass) to pull in base ctors

note that using base::base syntax was added in gcc4.8 (travis says clang is fine), so this would actually establish that version as the minimum GCC version (alternatively, they could all be explicitly declared). see the travis failure for gcc4.7.

@mway
Copy link
Author

mway commented Sep 17, 2017

  • added additional ctors to cpr::Url to permit implicit conversion from std::string and const char*, for backwards compatibility

@mway
Copy link
Author

mway commented Sep 23, 2017

hey @whoshuu, let me know what you think whenever you have a sec!

@whoshuu
Copy link
Collaborator

whoshuu commented Oct 27, 2017

Hey @mway! There's a bunch of good stuff in here! One can say there's almost too much good stuff 👍.

That said, it would be much easier to get all of this in piecemeal, especially the pieces that aren't exactly dependent on each other. Here are the distinct chunks I can see being split off from this:

  • unit test sorting
  • StringHolder and tests
  • Cert/Key/Password stuff - this can have the StringHolder changes if you want, but I can review just the non StringHolder stuff and have merging of this PR be depending on the StringHolder PR
  • removing explicit Url construction in the tests - I have thoughts on this I'll share when a separate PR is up

As for the GCC issue, let's move that discussion into a new PR with just the StringHolder changes. I think I need time to research GCC version usage to get a feel for how setting a 4.8 minimum will affect library users. My current feeling is that the library's primary obligation is to be as useful to as many people as possible without compromising on design. Having to add extra explicit constructors to some internal library classes feels like a small price to pay for larger compiler compatibility, especially because the interface is unaffected by that extra work.

Again, these initial thoughts aren't set in stone and I'll happily open up the discussion more broadly on a focused PR with just the StringHolder changes.

Also, if you rebase this PR you'll see that the only travis failures are the ones caused by the using-declaration constructor inheritance changes.

@COM8
Copy link
Member

COM8 commented Jun 10, 2020

@mway I’m closing this, but could you create a separate PR for all the StringHolder stuff?

All the SSL related stuff has been added in #276 .

@COM8 COM8 closed this Jun 10, 2020
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.

3 participants