You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2020/10/27 15:58:07 UTC

[GitHub] [trafficserver] randall opened a new pull request #7298: Adds OCSP support for BoringSSL

randall opened a new pull request #7298:
URL: https://github.com/apache/trafficserver/pull/7298


   


----------------------------------------------------------------
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.

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

[GitHub] [trafficserver] randall merged pull request #7298: Adds OCSP support for BoringSSL

Posted by GitBox <gi...@apache.org>.
randall merged pull request #7298:
URL: https://github.com/apache/trafficserver/pull/7298


   


-- 
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] randall commented on pull request #7298: Adds OCSP support for BoringSSL

Posted by GitBox <gi...@apache.org>.
randall commented on pull request #7298:
URL: https://github.com/apache/trafficserver/pull/7298#issuecomment-717401372


   [approve ci clang-analyzer]


----------------------------------------------------------------
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.

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

[GitHub] [trafficserver] randall commented on a change in pull request #7298: Adds OCSP support for BoringSSL

Posted by GitBox <gi...@apache.org>.
randall commented on a change in pull request #7298:
URL: https://github.com/apache/trafficserver/pull/7298#discussion_r673283950



##########
File path: iocore/net/OCSPStapling.cc
##########
@@ -20,11 +20,18 @@
  */
 
 #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"

Review comment:
       Good call! Fixed.




-- 
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] randall removed a comment on pull request #7298: Adds OCSP support for BoringSSL

Posted by GitBox <gi...@apache.org>.
randall removed a comment on pull request #7298:
URL: https://github.com/apache/trafficserver/pull/7298#issuecomment-776114892






----------------------------------------------------------------
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.

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

[GitHub] [trafficserver] randall commented on pull request #7298: Adds OCSP support for BoringSSL

Posted by GitBox <gi...@apache.org>.
randall commented on pull request #7298:
URL: https://github.com/apache/trafficserver/pull/7298#issuecomment-775387039


   [approve ci autest]


----------------------------------------------------------------
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.

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

[GitHub] [trafficserver] shinrich commented on pull request #7298: Adds OCSP support for BoringSSL

Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7298:
URL: https://github.com/apache/trafficserver/pull/7298#issuecomment-718274568


   I agree that keeping ocsp4boring out of our source tree seems preferable.  And rather user it as a separately installed package.


----------------------------------------------------------------
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.

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

[GitHub] [trafficserver] github-actions[bot] removed a comment on pull request #7298: Adds OCSP support for BoringSSL

Posted by GitBox <gi...@apache.org>.
github-actions[bot] removed a comment on pull request #7298:
URL: https://github.com/apache/trafficserver/pull/7298#issuecomment-864211349


   This pull request has been automatically marked as stale because it has not had recent activity. Marking it stale to flag it for further consideration by the community.


-- 
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] bryancall removed a comment on pull request #7298: Adds OCSP support for BoringSSL

Posted by GitBox <gi...@apache.org>.
bryancall removed a comment on pull request #7298:
URL: https://github.com/apache/trafficserver/pull/7298#issuecomment-769455492


   @randall Can you please rebase?


-- 
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] zwoop commented on pull request #7298: Adds OCSP support for BoringSSL

Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7298:
URL: https://github.com/apache/trafficserver/pull/7298#issuecomment-883738267


   Cherry-picked to v9.1.x branch.


-- 
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] shinrich commented on a change in pull request #7298: Adds OCSP support for BoringSSL

Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7298:
URL: https://github.com/apache/trafficserver/pull/7298#discussion_r673274246



##########
File path: iocore/net/OCSPStapling.cc
##########
@@ -20,11 +20,18 @@
  */
 
 #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"

Review comment:
       Are we assuming boringocsp/ocsp.h is in our tree?  Or why are we using double quotes instead of angle brackets here?  Not impacting functionality, but maybe confusing from the maintenance perspective.




-- 
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] bryancall removed a comment on pull request #7298: Adds OCSP support for BoringSSL

Posted by GitBox <gi...@apache.org>.
bryancall removed a comment on pull request #7298:
URL: https://github.com/apache/trafficserver/pull/7298#issuecomment-769455492


   @randall Can you please rebase?


-- 
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] randall commented on pull request #7298: Adds OCSP support for BoringSSL

Posted by GitBox <gi...@apache.org>.
randall commented on pull request #7298:
URL: https://github.com/apache/trafficserver/pull/7298#issuecomment-717401190


   [approve ci autest]


----------------------------------------------------------------
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.

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

[GitHub] [trafficserver] randall removed a comment on pull request #7298: Adds OCSP support for BoringSSL

Posted by GitBox <gi...@apache.org>.
randall removed a comment on pull request #7298:
URL: https://github.com/apache/trafficserver/pull/7298#issuecomment-717401372






----------------------------------------------------------------
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.

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

[GitHub] [trafficserver] maskit commented on pull request #7298: Adds OCSP support for BoringSSL

Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7298:
URL: https://github.com/apache/trafficserver/pull/7298#issuecomment-767324171


   Some of functions that this PR adds are going to be deprecated on OpenSSL 3.0. ATS uses three of those, and will need to use some replacements.
   https://github.com/openssl/openssl/blob/master/doc/man3/OCSP_sendreq_new.pod
   
   > Deprecated since OpenSSL 3.0, can be hidden entirely by defining OPENSSL_API_COMPAT with a suitable version value, see openssl_user_macros(7):
   ```
   int OCSP_REQ_CTX_i2d(OCSP_REQ_CT *rctx, const ASN1_ITEM *it, ASN1_VALUE *req);
   int OCSP_REQ_CTX_add1_header(OCSP_REQ_CT *rctx,
                                const char *name, const char *value);
   void OCSP_REQ_CTX_free(OSSL_HTTP_REQ_CTX *rctx);
   void OCSP_set_max_response_length(OCSP_REQ_CT *rctx,
                                     unsigned long len);
   int OCSP_REQ_CTX_set1_req(OSSL_HTTP_REQ_CTX *rctx, const OCSP_REQUEST *req);
   ```


----------------------------------------------------------------
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.

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

[GitHub] [trafficserver] randall commented on pull request #7298: Adds OCSP support for BoringSSL

Posted by GitBox <gi...@apache.org>.
randall commented on pull request #7298:
URL: https://github.com/apache/trafficserver/pull/7298#issuecomment-868665578


   Randall is still working on this.


-- 
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] randall commented on pull request #7298: Adds OCSP support for BoringSSL

Posted by GitBox <gi...@apache.org>.
randall commented on pull request #7298:
URL: https://github.com/apache/trafficserver/pull/7298#issuecomment-717404270


   > 
   > License - OpenSSL 3.0 and the code on their master branch are licensed under AL2. Although I didn't check whether the current license is compatible with AL2, AL2 version would be safer option in terms of licensing. The code on master may be unstable so I'm also ok with using non-AL2 version for now as long as the license is compatible.
   > https://www.openssl.org/source/license.html
   
   The APIs/underlying structures have changed enough to make this not a trivial thing to do. This is not the first openssl code to be added to the project. It looks like parts of BIO_fastopen.cc is copied from the project into our tree.
   
   ```
   commit d02e88eb0db871a14279da28af819b5f19a945ac
   Author: Leif Hedstrom <le...@ogre.com>
   Date:   Fri Feb 17 21:54:45 2017 -0700
   
       Added the OpenSSL license
   
   ```


----------------------------------------------------------------
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.

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

[GitHub] [trafficserver] randall commented on pull request #7298: Adds OCSP support for BoringSSL

Posted by GitBox <gi...@apache.org>.
randall commented on pull request #7298:
URL: https://github.com/apache/trafficserver/pull/7298#issuecomment-720618056


   Seems reasonable to move the ocsp4boring code out of tree. I'll follow up with a change to do this.


----------------------------------------------------------------
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.

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

[GitHub] [trafficserver] maskit commented on pull request #7298: Adds OCSP support for BoringSSL

Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7298:
URL: https://github.com/apache/trafficserver/pull/7298#issuecomment-717400185


   Two comments for now.
   
   Maintenance - Having a snapshot of OpenSSL source code sounds a bit scary because it could bring security issues. I think we should have a very easy way to update the snapshot instantly. If there are modifications against the original code we should probably keep them as patch files as well.
   
   License - OpenSSL 3.0 and the code on their master branch are licensed under AL2. Although I didn't check whether the current license is compatible with AL2, AL2 version would be safer option in terms of licensing. The code on master may be unstable so I'm also ok with using non-AL2 version for now as long as the license is compatible.
   https://www.openssl.org/source/license.html


----------------------------------------------------------------
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.

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

[GitHub] [trafficserver] randall commented on pull request #7298: Adds OCSP support for BoringSSL

Posted by GitBox <gi...@apache.org>.
randall commented on pull request #7298:
URL: https://github.com/apache/trafficserver/pull/7298#issuecomment-717412860


   > Maintenance - Having a snapshot of OpenSSL source code sounds a bit scary because it could bring security issues. I think we should have a very easy way to update the snapshot instantly. If there are modifications against the original code we should probably keep them as patch files as well.
   
   Personally, I'd like the library "ocsp4boring" to not be checked into the tree itself and rather that it was its own project that other projects could easily build against (like nginx). 


----------------------------------------------------------------
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.

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

[GitHub] [trafficserver] github-actions[bot] removed a comment on pull request #7298: Adds OCSP support for BoringSSL

Posted by GitBox <gi...@apache.org>.
github-actions[bot] removed a comment on pull request #7298:
URL: https://github.com/apache/trafficserver/pull/7298#issuecomment-864211349


   This pull request has been automatically marked as stale because it has not had recent activity. Marking it stale to flag it for further consideration by the community.


-- 
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] randall commented on pull request #7298: Adds OCSP support for BoringSSL

Posted by GitBox <gi...@apache.org>.
randall commented on pull request #7298:
URL: https://github.com/apache/trafficserver/pull/7298#issuecomment-775477296


   [approve ci autest]
   


----------------------------------------------------------------
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.

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

[GitHub] [trafficserver] randall commented on pull request #7298: Adds OCSP support for BoringSSL

Posted by GitBox <gi...@apache.org>.
randall commented on pull request #7298:
URL: https://github.com/apache/trafficserver/pull/7298#issuecomment-776114892


   [approve ci autest]
   1 similar comment
   


----------------------------------------------------------------
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.

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

[GitHub] [trafficserver] bryancall commented on pull request #7298: Adds OCSP support for BoringSSL

Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #7298:
URL: https://github.com/apache/trafficserver/pull/7298#issuecomment-769455492


   @randall Can you please rebase?


----------------------------------------------------------------
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.

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

[GitHub] [trafficserver] github-actions[bot] commented on pull request #7298: Adds OCSP support for BoringSSL

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7298:
URL: https://github.com/apache/trafficserver/pull/7298#issuecomment-864211349


   This pull request has been automatically marked as stale because it has not had recent activity. Marking it stale to flag it for further consideration by the community.


-- 
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.

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