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

lib/vtls/sectransp.c: Specify cipher name for Mac Secure Transport back-end #6464

Closed
wants to merge 1 commit into from
Closed

lib/vtls/sectransp.c: Specify cipher name for Mac Secure Transport back-end #6464

wants to merge 1 commit into from

Conversation

mkolechkin
Copy link
Contributor

@mkolechkin mkolechkin commented Jan 15, 2021 •
edited

Add parser for CURLOPT_SSL_CIPHER_LIST option for Secure Transport (ST)
back-end. Similar to NNS and GSKit back-ends, new code parses string
value and configures ST library to use those ciphers for communication.

Create cipher spec data structure and initialize array of specs with
cipher number, names and flag. That array allows to add/modify ciphers
in one place instead of several functions in code. The flag is binary
value for backward compatibility with an existing wrapper
implementation. It can be extended later for other purposes like
support of specific TLS version.

Code change required some analysis of macOS SDK versions, we need to
know when one or another value of enum in macOS SDK was introduced,
and some other data. I created the TLS-CIPHERS.md file to summarize
that data I found and to add some more information about ciphers
could useful for others. That document is not completely ready yet
and I just want your opinion on it.

@bagder bagder added the TLS label Jan 15, 2021
@bagder
Copy link
Member

bagder commented Jan 15, 2021

Isn't the cipher names and info better put in CIPHERS.md where we have the name info for other TLS backends?

@mkolechkin
Copy link
Contributor Author

mkolechkin commented Jan 15, 2021 •
edited

Isn't the cipher names and info better put in CIPHERS.md

Yes, I just want to finish the table first and then merge it to the existing file. It is more convenient for me right now. The new table has more information and I prefer to send it as a new file. If reviewers agree the information is useful and format is good enough, etc, I'll definitely merge it into the CIPHERS.md

Question. I did not work on wide tables in .MD format before. Looks it is not very nice to scroll it right/left on the github. Do you see better way to present that information? Maybe split it to several tables or use some HTML tables instead?
I'll read more about tables in MD files. Maybe I can reduce font size or something like that.

where we have the name info for other TLS backends?

I'll add references.

Thank you for review! :)

nickzman
nickzman requested changes Jan 18, 2021
View reviewed 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.

Thank you for your contribution. I've got a few changes I'd like you to make.

lib/vtls/sectransp.c Outdated Show resolved Hide resolved
lib/vtls/sectransp.c Show resolved Hide resolved
lib/vtls/sectransp.c Outdated Show resolved Hide resolved
docs/TLS-CIPHERS.md Outdated Show resolved Hide resolved
docs/TLS-CIPHERS.md Outdated Show resolved Hide resolved
@mkolechkin mkolechkin changed the title lib/vtls/sectransp.c: Specify cipher name for Mac Secure Transport wrapper lib/vtls/sectransp.c: Specify cipher name for Mac Secure Transport back-end Jan 22, 2021
nickzman
nickzman requested changes Jan 23, 2021
View reviewed 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.

Good; the latest changes are no longer generating a failing grade when I try them on howsmyssl.com's analyzer API. I found some typos in the code.

lib/vtls/sectransp.c Outdated Show resolved Hide resolved
lib/vtls/sectransp.c Outdated Show resolved Hide resolved
lib/vtls/sectransp.c Outdated Show resolved Hide resolved
lib/vtls/sectransp.c Outdated Show resolved Hide resolved
lib/vtls/sectransp.c Outdated Show resolved Hide resolved
nickzman
nickzman requested changes Jan 31, 2021
View reviewed 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.

You're very close; you just have typos and a security flaw to cover now. Again, thanks for your work on this so far.

lib/vtls/sectransp.c Outdated Show resolved Hide resolved
lib/vtls/sectransp.c Outdated Show resolved Hide resolved
lib/vtls/sectransp.c Outdated Show resolved Hide resolved
lib/vtls/sectransp.c Outdated Show resolved Hide resolved
lib/vtls/sectransp.c Outdated Show resolved Hide resolved
lib/vtls/sectransp.c Outdated Show resolved Hide resolved
@mkolechkin
Copy link
Contributor Author

I made several commits, probably I better summarize them here.

I updated PR based on comments, commit cc7f8f7 addresses most of them.
Then I thought a little bit and made changes: removed ssl_name and slightly modified error handling: fail on error instead of switch to default ciphers. Please take a look. If those changes are not reasonable, I can bring back ssl_name and default ciphers behavior on error. Commit b0fb321
The last commit cb84af2 was to fix iOS code guards. I looked at old SDKs (https://sskaje.me/ios-sdk-archive/) and found last cipher constants added to macOS SDK 10.15, they introduced to iOS SDK 13 (I have it on my machine and checked difference with v 11 available on the site I mentioned above). Also corrected iOS SDK guards for cipher constants as they were before my change

Please take a look.

nickzman
nickzman requested changes Feb 16, 2021
View reviewed 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.

I made one more pass through this, and found one more typo in the comments. Otherwise, this is looking good.

lib/vtls/sectransp.c Show resolved Hide resolved
nickzman
nickzman approved these changes Feb 17, 2021
View reviewed 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.

All right, this looks good. Can you squash this please? @bagder , do you want to review?

@mkolechkin
SSL/TLS: Specify cipher name for macOS/iOS Secure Transport back-end
6e5dc08 
Add parser for CURLOPT_SSL_CIPHER_LIST option for Secure Transport (ST)
back-end. Similar to NSS and GSKit back-ends, new code parses string
value and configures ST library to use those ciphers for communication.
Create cipher spec data structure and initialize the array of specs with
cipher number, name, alias, and 'weak' flag.
Mark triple-DES ciphers as 'weak', and exclude them from the default
ciphers list.

PR curl#6464

Fix some comments. Fix iOS defines.
@mkolechkin
Copy link
Contributor Author

mkolechkin commented Feb 17, 2021 •
edited

All right, this looks good. Can you squash this please?

Thank you for your help!
I hope I squashed it right, did it the first time.

@mkolechkin
Copy link
Contributor Author

@bagder @nickzman
Hi Daniel and Nick,
Do I need to do something more for this change to be merged into the master branch?
Thank you
Mike

@bagder bagder added the feature-window A merge of this requires an open feature window label Mar 29, 2021
@bagder
Copy link
Member

bagder commented Mar 29, 2021

I'm ready to merge once the feature window opens again. If all goes well, that's on Monday April 5th.

@bagder bagder closed this in dd2bb48 Apr 21, 2021
@bagder
Copy link
Member

bagder commented Apr 21, 2021

Thanks!

@mkolechkin
Copy link
Contributor Author

Thank you, Daniel and Nick!

@mkolechkin mkolechkin deleted the mkolechkin branch June 12, 2021 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickzman nickzman nickzman approved these changes

Assignees
No one assigned
Labels
feature-window A merge of this requires an open feature window TLS
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mkolechkin @bagder @nickzman