Skip to content

Remove dependency on OpenSSL's OCSP API #9624

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
Merged
maskit merged 5 commits into from
Apr 28, 2023
Merged

Remove dependency on OpenSSL's OCSP API #9624

maskit merged 5 commits into from
Apr 28, 2023

Conversation

@maskit
Copy link
Member

@maskit maskit commented Apr 19, 2023

Changes:

  • Remove all boringocsp stuff
  • Copy OCSP_ functions from OpenSSL 3 (Apache License 2) and add TS_ prefix.
  • Copy ASN1 definitions for OCSP from OpenSSL 3
  • Modify code not to use functions only available on OpenSSL
  • Modify code to make it C++ (original code is C and has issues compiling as C++ code)
  • Change ERR_raise to ATS's Debug

@maskit maskit added the TLS label Apr 19, 2023
@maskit maskit added this to the 10.0.0 milestone Apr 19, 2023
@maskit maskit self-assigned this Apr 19, 2023
@bryancall bryancall requested a review from ywkaras April 24, 2023 22:15
ywkaras
ywkaras reviewed Apr 26, 2023
View reviewed changes
iocore/net/OCSPStapling.cc
#endif
#include <openssl/x509v3.h>
#include <openssl/asn1.h>
#include <openssl/asn1t.h>
Copy link
Contributor

@ywkaras ywkaras Apr 26, 2023

Choose a reason for hiding this comment

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

Is all this code copied from openssl 3.0? If not, do we need to display this license somewhere? https://www.openssl.org/source/license-openssl-ssleay.txt

Copy link
Member Author

@maskit maskit Apr 26, 2023

Choose a reason for hiding this comment

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

All code copied is from OpenSSL 3.0 or 3.1 (the both are Apache License), but we may still need some note about it. @zwoop What do you think?

Copy link
Member Author

@maskit maskit Apr 28, 2023

Choose a reason for hiding this comment

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

Added comments.

ywkaras
ywkaras reviewed Apr 26, 2023
View reviewed changes
iocore/net/OCSPStapling.cc
#define MAX_STAPLING_DER 10240

extern ClassAllocator<FetchSM> FetchSMAllocator;

Copy link
Contributor

@ywkaras ywkaras Apr 26, 2023

Choose a reason for hiding this comment

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

Why can' t the anonymous namespace start here?

Copy link
Member Author

@maskit maskit Apr 26, 2023

Choose a reason for hiding this comment

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

I don't remember the exact error, but it was probably considered as a different thing if it's in a namespace because the actual FetchSMAllocator is not in any namespace.

Copy link
Contributor

@ywkaras ywkaras Apr 27, 2023

Choose a reason for hiding this comment

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

I meant the anonymous namespace should start after that declaration, not include it.

Copy link
Member Author

@maskit maskit Apr 28, 2023

Choose a reason for hiding this comment

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

  CXX      OCSPStapling.o
OCSPStapling.cc:230:1: error: no template named 'StackTraits'; did you mean '::bssl::internal::StackTraits'?
DEFINE_STACK_OF(TS_OCSP_ONEREQ)
^
/Users/mkitajo/opt/boringssl/include/openssl/stack.h:100:31: note: expanded from macro 'DEFINE_STACK_OF'
#define DEFINE_STACK_OF(type) DEFINE_NAMED_STACK_OF(type, type)
                              ^
/Users/mkitajo/opt/boringssl/include/openssl/stack.h:94:3: note: expanded from macro 'DEFINE_NAMED_STACK_OF'
  BORINGSSL_DEFINE_STACK_TRAITS(name, type, false)
  ^
/Users/mkitajo/opt/boringssl/include/openssl/stack.h:357:10: note: expanded from macro 'BORINGSSL_DEFINE_STACK_TRAITS'
  struct StackTraits<STACK_OF(name)> {                      \
         ^
/Users/mkitajo/opt/boringssl/include/openssl/stack.h:347:8: note: '::bssl::internal::StackTraits' declared here
struct StackTraits {};
       ^
OCSPStapling.cc:230:1: error: class template specialization of 'StackTraits' not in a namespace enclosing 'internal'
DEFINE_STACK_OF(TS_OCSP_ONEREQ)
^
/Users/mkitajo/opt/boringssl/include/openssl/stack.h:100:31: note: expanded from macro 'DEFINE_STACK_OF'
#define DEFINE_STACK_OF(type) DEFINE_NAMED_STACK_OF(type, type)
                              ^
/Users/mkitajo/opt/boringssl/include/openssl/stack.h:94:3: note: expanded from macro 'DEFINE_NAMED_STACK_OF'
  BORINGSSL_DEFINE_STACK_TRAITS(name, type, false)
  ^
/Users/mkitajo/opt/boringssl/include/openssl/stack.h:357:10: note: expanded from macro 'BORINGSSL_DEFINE_STACK_TRAITS'
  struct StackTraits<STACK_OF(name)> {                      \
         ^
blah blah blah

@maskit maskit merged commit 9c3b214 into apache:master Apr 28, 2023
@maskit maskit mentioned this pull request May 2, 2023
Merged
@midchildan midchildan mentioned this pull request May 28, 2023
Merged
midchildan added a commit to midchildan/trafficserver that referenced this pull request May 28, 2023
@midchildan
Fix OCSP detection during build
41a1c1c 
The configure script fails to detect OCSP support when building ATS with
OpenSSL 3.0.

This isn't a problem in the `master` branch, which copied OpenSSL's OCSP code
into ATS itself in apache#9624. However, this remains a problem on existing releases
and downstream packages seem to be affected by it. Here's a list of the few I
checked:

- Alpine
- Debian 12
- Fedora 37
- Homebrew
- Nixpkgs

This happens because OpenSSL 3.0 made changes to its APIs that affected how ATS
detects OCSP support. ATS checks the existence of a few functions, including
`OCSP_REQ_CTX_add1_header` and `OCSP_REQ_CTX_set1_req`, by attempting to link to
them using `AC_CHECK_FUNCS`. In OpenSSL 3.0, these functions were turned into
macros making them uneligible for detection with `AC_CHECK_FUNCS`.

This change fixes that problem by instead using `AC_LANG_PROGRAM` to check that
code using the aforementioned functions compile. This approach works for OpenSSL
both before and after 3.0.
midchildan added a commit to midchildan/trafficserver that referenced this pull request May 28, 2023
@midchildan
Fix OCSP detection during build
99cea98 
The configure script fails to detect OCSP support when building ATS with
OpenSSL 3.0.

This isn't a problem in the `master` branch, which copied OpenSSL's OCSP code
into ATS itself in apache#9624. However, this remains a problem on existing releases
and downstream packages seem to be affected by it. Here's a list of the few I
checked:

- Alpine
- Debian 12
- Fedora 37
- Homebrew
- Nixpkgs

This happens because OpenSSL 3.0 made changes to its APIs that affected how ATS
detects OCSP support. ATS checks the existence of a few functions, including
`OCSP_REQ_CTX_add1_header` and `OCSP_REQ_CTX_set1_req`, by attempting to link to
them using `AC_CHECK_FUNCS`. In OpenSSL 3.0, these functions were turned into
macros making them uneligible for detection with `AC_CHECK_FUNCS`.

This change fixes that problem by instead using `AC_LANG_PROGRAM` to check that
code using the aforementioned functions compile. This approach works for OpenSSL
both before and after 3.0.
bryancall pushed a commit that referenced this pull request Jun 26, 2023
@midchildan
Fix OCSP detection during build (#9754)
e079d20 
The configure script fails to detect OCSP support when building ATS with
OpenSSL 3.0.

This isn't a problem in the `master` branch, which copied OpenSSL's OCSP code
into ATS itself in #9624. However, this remains a problem on existing releases
and downstream packages seem to be affected by it. Here's a list of the few I
checked:

- Alpine
- Debian 12
- Fedora 37
- Homebrew
- Nixpkgs

This happens because OpenSSL 3.0 made changes to its APIs that affected how ATS
detects OCSP support. ATS checks the existence of a few functions, including
`OCSP_REQ_CTX_add1_header` and `OCSP_REQ_CTX_set1_req`, by attempting to link to
them using `AC_CHECK_FUNCS`. In OpenSSL 3.0, these functions were turned into
macros making them uneligible for detection with `AC_CHECK_FUNCS`.

This change fixes that problem by instead using `AC_LANG_PROGRAM` to check that
code using the aforementioned functions compile. This approach works for OpenSSL
both before and after 3.0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ywkaras ywkaras ywkaras approved these changes

Assignees

@maskit maskit

Labels

TLS

Projects

None yet

Milestone

10.0.0

Development

Successfully merging this pull request may close these issues.

2 participants

@maskit @ywkaras