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

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

Closed
wants to merge 1 commit into from

Conversation

@mkolechkin
Copy link
Contributor

@mkolechkin mkolechkin commented Jan 15, 2021

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 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?

Loading

@mkolechkin
Copy link
Contributor Author

@mkolechkin mkolechkin commented Jan 15, 2021

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! :)

Loading

Copy link
Collaborator

@nickzman nickzman left a comment

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

Loading

lib/vtls/sectransp.c Outdated Show resolved Hide resolved
Loading
lib/vtls/sectransp.c Show resolved Hide resolved
Loading
lib/vtls/sectransp.c Outdated Show resolved Hide resolved
Loading
docs/TLS-CIPHERS.md Outdated Show resolved Hide resolved
Loading
docs/TLS-CIPHERS.md Outdated Show resolved Hide resolved
Loading
@mkolechkin mkolechkin requested a review from nickzman Jan 22, 2021
@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
Copy link
Collaborator

@nickzman nickzman left a comment

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.

Loading

lib/vtls/sectransp.c Outdated Show resolved Hide resolved
Loading
lib/vtls/sectransp.c Outdated Show resolved Hide resolved
Loading
lib/vtls/sectransp.c Outdated Show resolved Hide resolved
Loading
lib/vtls/sectransp.c Outdated Show resolved Hide resolved
Loading
lib/vtls/sectransp.c Outdated Show resolved Hide resolved
Loading
@mkolechkin mkolechkin requested a review from nickzman Jan 28, 2021
Copy link
Collaborator

@nickzman nickzman left a comment

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

Loading

lib/vtls/sectransp.c Outdated Show resolved Hide resolved
Loading
lib/vtls/sectransp.c Outdated Show resolved Hide resolved
Loading
lib/vtls/sectransp.c Outdated Show resolved Hide resolved
Loading
lib/vtls/sectransp.c Outdated Show resolved Hide resolved
Loading
lib/vtls/sectransp.c Outdated Show resolved Hide resolved
Loading
lib/vtls/sectransp.c Outdated Show resolved Hide resolved
Loading
@mkolechkin mkolechkin requested a review from nickzman Feb 4, 2021
@mkolechkin
Copy link
Contributor Author

@mkolechkin mkolechkin commented Feb 4, 2021

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.

Loading

Copy link
Collaborator

@nickzman nickzman left a comment

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

Loading

lib/vtls/sectransp.c Show resolved Hide resolved
Loading
@mkolechkin mkolechkin requested a review from nickzman Feb 16, 2021
Copy link
Collaborator

@nickzman nickzman left a comment

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

Loading

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 mkolechkin commented Feb 17, 2021

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.

Loading

@mkolechkin
Copy link
Contributor Author

@mkolechkin mkolechkin commented Feb 26, 2021

@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

Loading

@bagder
Copy link
Member

@bagder 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.

Loading

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

@bagder bagder commented Apr 21, 2021

Thanks!

Loading

@mkolechkin
Copy link
Contributor Author

@mkolechkin mkolechkin commented Apr 22, 2021

Thank you, Daniel and Nick!

Loading

@mkolechkin mkolechkin deleted the mkolechkin branch Jun 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants