You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "maskit (via GitHub)" <gi...@apache.org> on 2023/04/19 21:09:51 UTC

[GitHub] [trafficserver] maskit opened a new pull request, #9624: Remove dependency on OpenSSL's OCSP API

maskit opened a new pull request, #9624:
URL: https://github.com/apache/trafficserver/pull/9624

   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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficserver] maskit commented on a diff in pull request #9624: Remove dependency on OpenSSL's OCSP API

Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9624:
URL: https://github.com/apache/trafficserver/pull/9624#discussion_r1180678309


##########
iocore/net/OCSPStapling.cc:
##########
@@ -21,16 +21,10 @@
 
 #include "P_OCSPStapling.h"
 
-#if TS_USE_TLS_OCSP
-
-#include <openssl/bio.h>
 #include <openssl/ssl.h>
-
-#if TS_HAS_BORINGOCSP
-#include <boringocsp/ocsp.h>
-#else
-#include <openssl/ocsp.h>
-#endif
+#include <openssl/x509v3.h>
+#include <openssl/asn1.h>
+#include <openssl/asn1t.h>

Review Comment:
   Added comments.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficserver] maskit commented on a diff in pull request #9624: Remove dependency on OpenSSL's OCSP API

Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9624:
URL: https://github.com/apache/trafficserver/pull/9624#discussion_r1178384209


##########
iocore/net/OCSPStapling.cc:
##########
@@ -21,16 +21,10 @@
 
 #include "P_OCSPStapling.h"
 
-#if TS_USE_TLS_OCSP
-
-#include <openssl/bio.h>
 #include <openssl/ssl.h>
-
-#if TS_HAS_BORINGOCSP
-#include <boringocsp/ocsp.h>
-#else
-#include <openssl/ocsp.h>
-#endif
+#include <openssl/x509v3.h>
+#include <openssl/asn1.h>
+#include <openssl/asn1t.h>

Review Comment:
   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? 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficserver] ywkaras commented on a diff in pull request #9624: Remove dependency on OpenSSL's OCSP API

Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on code in PR #9624:
URL: https://github.com/apache/trafficserver/pull/9624#discussion_r1178379148


##########
iocore/net/OCSPStapling.cc:
##########
@@ -46,13 +40,217 @@
 
 extern ClassAllocator<FetchSM> FetchSMAllocator;
 

Review Comment:
   Why can' t the anonymous namespace start here?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficserver] maskit commented on a diff in pull request #9624: Remove dependency on OpenSSL's OCSP API

Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9624:
URL: https://github.com/apache/trafficserver/pull/9624#discussion_r1180663052


##########
iocore/net/OCSPStapling.cc:
##########
@@ -46,13 +40,217 @@
 
 extern ClassAllocator<FetchSM> FetchSMAllocator;
 

Review Comment:
   ```
     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
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficserver] ywkaras commented on a diff in pull request #9624: Remove dependency on OpenSSL's OCSP API

Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on code in PR #9624:
URL: https://github.com/apache/trafficserver/pull/9624#discussion_r1178319035


##########
iocore/net/OCSPStapling.cc:
##########
@@ -21,16 +21,10 @@
 
 #include "P_OCSPStapling.h"
 
-#if TS_USE_TLS_OCSP
-
-#include <openssl/bio.h>
 #include <openssl/ssl.h>
-
-#if TS_HAS_BORINGOCSP
-#include <boringocsp/ocsp.h>
-#else
-#include <openssl/ocsp.h>
-#endif
+#include <openssl/x509v3.h>
+#include <openssl/asn1.h>
+#include <openssl/asn1t.h>

Review Comment:
   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



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficserver] maskit merged pull request #9624: Remove dependency on OpenSSL's OCSP API

Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit merged PR #9624:
URL: https://github.com/apache/trafficserver/pull/9624


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficserver] ywkaras commented on a diff in pull request #9624: Remove dependency on OpenSSL's OCSP API

Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on code in PR #9624:
URL: https://github.com/apache/trafficserver/pull/9624#discussion_r1178502434


##########
iocore/net/OCSPStapling.cc:
##########
@@ -46,13 +40,217 @@
 
 extern ClassAllocator<FetchSM> FetchSMAllocator;
 

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



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficserver] maskit commented on a diff in pull request #9624: Remove dependency on OpenSSL's OCSP API

Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9624:
URL: https://github.com/apache/trafficserver/pull/9624#discussion_r1178386254


##########
iocore/net/OCSPStapling.cc:
##########
@@ -46,13 +40,217 @@
 
 extern ClassAllocator<FetchSM> FetchSMAllocator;
 

Review Comment:
   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.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org