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

Some cleanups #25

Merged
merged 11 commits into from
Aug 23, 2019
Merged

Some cleanups #25

merged 11 commits into from
Aug 23, 2019

Conversation

omasanori
Copy link
Contributor

@omasanori omasanori commented Aug 19, 2019

Besides trivial "add one blank line here for consistency" and "the ordering seems dirty a bit" ones, this PR does the following:

  1. Check if EC APIs are available or not, instead of assuming that OpenSSL 1.1.0 or later is always fine.
  2. Use NEVERBLEED_ prefix for feature detection macros to avoid future collisions.
  3. Do not rely on indirect #includes

I believe these changes will improve maintainability and compatibility with LibreSSL. However, the branch is constructed on a step-by-step basis, so please feel free to point out inappropriate ones. I am ready to exclude them.

Compiles with:

  • OpenSSL 1.1.1 (opaque RSA_METHOD, EC APIs available, RSA_ shim unneeded)
  • OpenSSL 1.1.0k (opaque RSA_METHOD, EC APIs available, RSA_ shim unneeded)
  • OpenSSL 1.1.0k (opaque RSA_METHOD, EC APIs unavailable, RSA_ shim unneeded)
  • OpenSSL 1.0.2k (not opaque RSA_METHOD, EC APIs unavaliable, RSA_ shim needed)
  • LibreSSL 2.9.1 (not opaque RSA_METHOD, EC APIs available, RSA_ shim unneeded)
  • LibreSSL 2.8.3 (not opaque RSA_METHOD, EC APIs unavailable, RSA_ shim unneeded)
  • LibreSSL 2.6.5 (not opaque RSA_METHOD, EC APIs unavailable, RSA_ shim needed)

Fixes #19

@omasanori
Copy link
Contributor Author

omasanori commented Aug 19, 2019

I marked this as WIP because it has not yet tested enough, but it is OK to review and comment on changes of course! 😃

Signed-off-by: Masanori Ogino <masanori.ogino@gmail.com>
Signed-off-by: Masanori Ogino <masanori.ogino@gmail.com>
Signed-off-by: Masanori Ogino <masanori.ogino@gmail.com>
Even OpenSSL >= 1.1.0 may be built without EC APIs due to footprint or
patent problems, and LibreSSL >= 2.9.1 provides the EC APIs while it
may not contain some of newer RSA APIs.

Signed-off-by: Masanori Ogino <masanori.ogino@gmail.com>
Signed-off-by: Masanori Ogino <masanori.ogino@gmail.com>
Signed-off-by: Masanori Ogino <masanori.ogino@gmail.com>
Signed-off-by: Masanori Ogino <masanori.ogino@gmail.com>
It is possible that macros with OPENSSL_ prefix will collide with
OpenSSL's usage. The NEVERBLEED_ prefix will not for sure.

Also, descriptive names will prevent from reusing them in inappropriate
ways, like OPENSSL_1_1_API has been used for two different purpose: 1)
to check if RSA_METHOD is opaque and 2) to check if the new unified EC
APIs are available.

Signed-off-by: Masanori Ogino <masanori.ogino@gmail.com>
OPENSSL_NO_EC is also lacked in pre-EC-era OpenSSL, not only LibreSSL.

Signed-off-by: Masanori Ogino <masanori.ogino@gmail.com>
Signed-off-by: Masanori Ogino <masanori.ogino@gmail.com>
Signed-off-by: Masanori Ogino <masanori.ogino@gmail.com>
@omasanori omasanori marked this pull request as ready for review August 19, 2019 09:44
@omasanori
Copy link
Contributor Author

Now it is (mostly) ready! Just I am too lazy to bring up a CentOS box for testing...

@omasanori
Copy link
Contributor Author

Built successfully with CentOS 7.

Copy link
Member

@kazuho kazuho 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 the PR. Looks good to me. Please let me know when you think it is ready for merge.

@omasanori
Copy link
Contributor Author

omasanori commented Aug 20, 2019

Thank you for your prompt review, @kazuho!

Tested on CentOS 7 using test.c and curl, with the following settings:

  • System OpenSSL 1.0.2k with RSA: succeeded
  • System OpenSSL 1.0.2k with ECDSA: failed to load cert (as intended)
  • LibreSSL 2.9.2 with RSA: succeeded
  • LibreSSL 2.9.2 with ECDSA: succeeded

Since proposed changes affect significantly to OpenSSL without EC support (as it failed to build test.c without the changes) and LibreSSL 2.9+ (as it becomes ECDSA enabled), these settings cover concerned situations. I guess it is ready for merge now.

@kazuho kazuho merged commit 8297d16 into h2o:master Aug 23, 2019
@kazuho
Copy link
Member

kazuho commented Aug 23, 2019

Thank you very much for all your efforts, and the matrix of tests that you have covered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Supporting ECDSA with LibreSSL
2 participants