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

Jump to bottom

Secure Transport: no more "darwinssl" #3619

Closed
wants to merge 1 commit into from
Closed

Secure Transport: no more "darwinssl" #3619

wants to merge 1 commit into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Feb 26, 2019

Everyone calls it Secure Transport, now we do too.

(After previous discusssions with @nickzman and @danielgustafsson.)

@bagder bagder added the TLS label Feb 26, 2019
@bagder
Copy link
Member Author

bagder commented Feb 26, 2019

The red builds look unrelated. I'll just let it sit here for a while longer before merge in case someone wants to object or comment.

nickzman
nickzman requested changes Feb 27, 2019
View changes
Copy link
Member

@nickzman nickzman left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. I have some changes.

configure.ac Outdated Show resolved Hide resolved
lib/vtls/sectransp.c Outdated Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
@bagder
Secure Transport: no more "darwinssl"
43e1c72 
Everyone calls it Secure Transport, now we do too.

Reviewed-by: Nick Zitzmann

Closes #3619
@bagder bagder requested a review from nickzman February 27, 2019 06:43
@bagder
Copy link
Member Author

bagder commented Feb 27, 2019

Thanks @nickzman, I believe I addressed the nits you identified.

nickzman
nickzman requested changes Feb 28, 2019
View changes
Copy link
Member

@nickzman nickzman left a comment

Choose a reason for hiding this comment

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

Almost! The code itself builds and runs fine. There's a discrepancy between what the configure script advertises it takes and what it actually takes.

configure.ac Show resolved Hide resolved
configure.ac Show resolved Hide resolved
@bagder bagder closed this in 76a9c3c Feb 28, 2019
@bagder bagder deleted the bagder/secure-transport branch February 28, 2019 07:48
@webmaster128
Copy link
Contributor

Is it possible that this change broke the root cmake file? In https://github.com/curl/curl/blob/curl-7_64_1/CMakeLists.txt#L344 the old name is still used.

I am experiencing a new crash in my application because curl_version_info_data.ssl_version is nullptr since I upgraded from 7.63.0 to 7.64.1. I set CMAKE_USE_DARWINSSL to ON, which worked so far.

@bagder
Copy link
Member Author

bagder commented Mar 29, 2019

Ah, I bet we have no CI tests for that build combo! 👎 Can you file this as a new issue (or PR if you have the time)?

@webmaster128 webmaster128 mentioned this pull request Apr 5, 2019
Closed
@webmaster128 webmaster128 mentioned this pull request Apr 12, 2019
Closed
@lock lock bot locked as resolved and limited conversation to collaborators Jun 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nickzman nickzman nickzman requested changes

Assignees
No one assigned
Labels
TLS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bagder @webmaster128 @nickzman