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.

Sign up for GitHub

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

[openssl] add feature to enable ssl3/weak-ssl-ciphers #35196

Merged
merged 4 commits into from
Dec 21, 2023
Merged

[openssl] add feature to enable ssl3/weak-ssl-ciphers #35196

merged 4 commits into from
Dec 21, 2023

Conversation

BamButz
Copy link
Contributor

@BamButz BamButz commented Nov 19, 2023 •
edited
Loading

I removed the changes made in #28690 because they are the default values (see here https://github.com/openssl/openssl/blob/master/Configure#L557).

In addition, I was in need to enable these functionalities, so I added features to enable them when needed.

  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
  • The "supports" clause reflects platforms that may be fixed by this new version
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

dg0yt
dg0yt suggested changes Nov 19, 2023
View reviewed changes
Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

Just for consistent formatting: no tabs.

ports/openssl/portfile.cmake Outdated Show resolved Hide resolved
ports/openssl/portfile.cmake Outdated Show resolved Hide resolved
@BamButz
Copy link
Contributor Author

BamButz commented Nov 19, 2023

Thanks, I've updated my code formatting settings.

@FrankXie05 FrankXie05 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Nov 20, 2023
@BamButz BamButz force-pushed the openssl/weak-ciphers branch 4 times, most recently from da32e45 to d5f40af Compare November 24, 2023 21:44
@BamButz
Copy link
Contributor Author

BamButz commented Dec 19, 2023

Is there something left to do for me? I would really appreciate this to be merged.

FrankXie05
FrankXie05 previously approved these changes Dec 20, 2023
View reviewed changes
@FrankXie05
Copy link
Contributor

FrankXie05 commented Dec 21, 2023 •
edited
Loading

All features are tested successfully in the following triplet:

  • x86-windows
  • x64-windows
  • x64-windows-stataic

@FrankXie05 FrankXie05 added the info:reviewed Pull Request changes follow basic guidelines label Dec 21, 2023
BillyONeal
BillyONeal reviewed Dec 21, 2023
View reviewed changes
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

Is it a good idea to add a feature that presumably should never be used any longer?

(I'm not necessarily opposed to adding it, but I think we at least need on record a reason why we were in a secure condition and decided to allow an insecure condition)

ports/openssl/portfile.cmake
@@ -58,6 +56,15 @@ if(NOT "tools" IN_LIST FEATURES)
vcpkg_list(APPEND CONFIGURE_OPTIONS no-apps)
endif()

if("weak-ssl-ciphers" IN_LIST FEATURES)
vcpkg_list(APPEND CONFIGURE_OPTIONS enable-weak-ssl-ciphers)
Copy link
Member

Choose a reason for hiding this comment

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

For consistency shouldn't these also set no-weak-ssl-ciphers and/or no-ssl3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no-weak-ssl-ciphers and no-ssl3 are default values, they do not need to be set.

@BillyONeal
Copy link
Member

Put another way: It seems likely to me that this feature was intentionally not added before, so I think we need a reason on record as to why we changed our minds.

@BamButz
Copy link
Contributor Author

BamButz commented Dec 21, 2023

The reason I need this is that I'm working on internal legacy software, which is not supposed to be changed to another cipher. I understand your concerns and that's why I implemented this change as a feature so you have to opt in to enable the insecure ciphers.

@BillyONeal
Copy link
Member

OK, just be aware that actively exploited attacks like CRIME and POODLE are out there, and that SSL 3 has been obsolete for 24 years, and broken for 9 years. There is pretty much no use case that would require encryption that are OK with SSL 3 anymore.

Nonetheless, it is an option exposed by upstream, so I'll merge it.

@BillyONeal BillyONeal merged commit ec05fe4 into microsoft:master Dec 21, 2023
15 checks passed
Osyotr pushed a commit to Osyotr/vcpkg that referenced this pull request Jan 23, 2024
@BamButz @Osyotr
[openssl] add feature to enable ssl3/weak-ssl-ciphers (microsoft#35196)
254fd65
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BillyONeal BillyONeal BillyONeal left review comments

@dg0yt dg0yt dg0yt requested changes

@FrankXie05 FrankXie05 FrankXie05 left review comments

Assignees

@FrankXie05 FrankXie05

Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@BamButz @FrankXie05 @BillyONeal @dg0yt