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

[openssl] support fips build feature #30916

Merged
merged 12 commits into from
Apr 21, 2023
Merged

[openssl] support fips build feature #30916

merged 12 commits into from
Apr 21, 2023

Conversation

lbermes
Copy link
Contributor

@lbermes lbermes commented Apr 17, 2023

I only implemented and tested it for windows as I have no option to do this under linux.
I'm new with changes here but I like to learn and to support this project

  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
    (no checksum changes)
  • The "supports" clause reflects platforms that may be fixed by this new version
    (I would like to mark the feature as only available for Windows but did not found or understood the syntax how todo this)
  • Any fixed CI baseline entries are removed from that file.
    (nothing changed)
  • Any patches that are no longer applied are deleted from the port's directory.
    (nothing changed)
  • 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.

I only implement and tested it for windows as I have no option to do this under linux
@lbermes
Copy link
Contributor Author

lbermes commented Apr 17, 2023

Added "supports" to the feature as I only tested it for windows

@Adela0814 Adela0814 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Apr 17, 2023
@lbermes
Copy link
Contributor Author

lbermes commented Apr 17, 2023

@lbermes please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

ports/openssl/portfile.cmake Outdated Show resolved Hide resolved
ports/openssl/portfile.cmake Show resolved Hide resolved
@Adela0814 Adela0814 changed the title Openssl optional fips build feature [openssl] support fips build feature Apr 18, 2023
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.

This PR shouldn't be merged with a windows-only change, now that windows and !windows finally share most of the openssl setup. But I will need a few more days to look into a general solution.

ports/openssl/portfile.cmake Show resolved Hide resolved
@@ -20,6 +20,10 @@
}
],
"features": {
"fips": {
"description": "Enable fips",
"supports": "windows"
Copy link
Contributor

Choose a reason for hiding this comment

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

supports is to match upstream's position. We should help you to have all platforms supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this can also work under linux so removing "supports" would already fit but I had no option to test this

Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove supports field, I have tested fips on linux and osx.

ports/openssl/portfile.cmake Show resolved Hide resolved
Removed the not needed FEATURE FIPS
Initialize INSTALL_FIPS as empty
@lbermes lbermes requested a review from Adela0814 April 19, 2023 06:24
@@ -20,6 +20,10 @@
}
],
"features": {
"fips": {
"description": "Enable fips",
"supports": "windows"
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove supports field, I have tested fips on linux and osx.

@lbermes lbermes requested a review from Adela0814 April 19, 2023 08:28
Adela0814
Adela0814 previously approved these changes Apr 19, 2023
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.

I tested the PR.
AFAIU fips support is only available with dynamic linkage. (There is no build error, but the feature will simply not be installed.) The feature should be declared with:

"supports": "!static"

@BillyONeal BillyONeal merged commit 6ffa0fc into microsoft:master Apr 21, 2023
@BillyONeal
Copy link
Member

Thanks!

@lbermes
Copy link
Contributor Author

lbermes commented Apr 22, 2023

Thank you for your support

@lbermes lbermes deleted the openssl-enable-fips-build branch April 22, 2023 05:58
@lbermes
Copy link
Contributor Author

lbermes commented Apr 26, 2023

I had the time to rebuild all based on the merge and see now an issue under windows
warning: The following files are placed in C:\GitHub\vcpkg\packages\openssl_x64-windows:

C:\GitHub\vcpkg\packages\openssl_x64-windows\fipsmodule.cnf

warning: Files cannot be present in those directories.
error: Found 1 post-build check problem(s). To submit these ports to curated catalogs, please first correct the portfile: C:\GitHub\vcpkg\ports\openssl\portfile.cmake

What would be the right/best way to merge a fix.
Should I simply create a new pull request with the fix?

@BillyONeal
Copy link
Member

Should I simply create a new pull request with the fix?

Yes please

@dg0yt
Copy link
Contributor

dg0yt commented Apr 26, 2023

While we are at fips: Should it become a default feature? (Is it a default feature upstream?)

@BillyONeal
Copy link
Member

While we are at fips: Should it become a default feature? (Is it a default feature upstream?)

I don't think so; normally FIPS only certifies distribution in binary forms, not in source forms. Moreover, I don't know about OpenSSL specifically, but elsewhere I've seen a FIPS mode that usually implies turning things off (that have not been certified) rather than on.

@guenterbe
Copy link

After getting the latest updates from github and rebuilding openssl, I can't find any fips.dll on Windows or fips.so on Linux.
Isn't a "vcpkg install openssl" sufficient? Are there some undocumented (or not found by me) command line switches?

@lbermes
Copy link
Contributor Author

lbermes commented Apr 28, 2023

Hi, please use the feature fips.
I added this first only for windows but the see the comment of [Adela0814]
It was requested that I remove this and so it now is a feature for all platforms.

@lbermes
Copy link
Contributor Author

lbermes commented Apr 28, 2023

"vcpkg install openssl[fips]" or with the tools "vcpkg install openssl[tools,fips]"

@lbermes
Copy link
Contributor Author

lbermes commented Apr 28, 2023

But I wonder how this influenced the linux build. I did not adjusted the linux port file.

@guenterbe
Copy link

@lbermes I have never needed the [features] in the past and was therefore not aware of them (must have missed them in the docs). Thanks very much!

@daschuer daschuer mentioned this pull request Jun 1, 2023
fwcd pushed a commit to fwcd/vcpkg that referenced this pull request Aug 21, 2023
* Added option to enable fips build for openssl

I only implement and tested it for windows as I have no option to do this under linux

* Adjusted port version

* Updated to support only for Windows

* Corrected Formatting

* Corrected SHA for vcpkg x-add-version --all

* Changes requested by review

Removed the not needed FEATURE FIPS
Initialize INSTALL_FIPS as empty

* Corrected SHA for vcpkg x-add-version --all

* Removed Windows only support

* Corrected SHA for vcpkg x-add-version --all

* Added "supports": "!static"

As the provider conecpts needs dynamic linkage

* Corrected SHA for vcpkg x-add-version --all
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

5 participants