Re: [EXTERNAL] Re: Re: Re: [PATCH] get BoringSSL back to the game
On Fri, Feb 04, 2022 at 11:54:05PM +0500, Илья Шипицин wrote: > > as you already suggested "best effort" support policy, it should not > require your time. > am I correct ? > Don't worry I will still review and merge patches :-) -- William Lallemand
Re: Re: Re: [PATCH] get BoringSSL back to the game
On Fri, Feb 04, 2022 at 07:46:44PM +0100, William Lallemand wrote: > > On Fri, Feb 04, 2022 at 11:02:24PM +0500, Илья Шипицин wrote: > > пт, 4 февр. 2022 г. в 19:16, William Lallemand : > > > > > On Fri, Feb 04, 2022 at 11:52:06AM +0100, William Lallemand wrote: > > > > > > > > I just tried to build with the latest boringSSL version, the problem is > > > > on our side: > > > > > > > > We are defining X509_OBJECT_get0_X509_CRL() because it does not exist in > > > > boringSSL, and inside it we are accessing the members of the X509_OBJECT > > > > and it can't work since it's opaque. > > > > > > > > We need to use the accessors instead, or find an alternative API. > > > > > > > > > > So, basically, we need to extract some X509_CRL from an X509_STORE which > > > could be done in OpenSSL with the X509_OBJECT API by using > > > X509_OBJECT_get0_X509_CRL() which is not available in boringSSL > > > > > > We are kind of stuck there because this is supposed to be the low > > > level API, now everything is opaque and we can't do much. > > > > > > The alternative would be to stop using the X509_STORE for storing the > > > CRL, and use a STACK_OF(X509_CRL)... But we use the store because it > > > could be either a X509_CRL or a X509... so it would be kind of > > > redefining a X509_STORE API... and I honestly don't want to do that. > > > > > > In my opinion there is a problem in their API because there is no > > > accessor for the X509_CRL in a X509_OBJECT. Which is kind of weird > > > because they have a X509_OBJECT_get0_X509() accessor, they probably just > > > missed it. > > > > > > > we already use "#ifdef X509_V_FLAG_CRL_CHECK" for enabling those functions. > > in theory we have 2 choices > > > > 1) adopt X509_OBJECT_get0_X509_CRL to BoringSSL in some way > > 2) exclude CRL manipulations under BoringSSL by adjusting ifdef > > I can't spend more time on this, feel free to open a bug report on their > bugtracker, this is probably better for everyone. > > You can adjust the ifdef as well. > Just to be more precise about the ifdef: this won't work with boringSSL, OpenSSL supports the CRLs, they define X509_V_FLAG_CRL_CHECK, the problem is the lack of accessor, not the support of CRL in boringSSL. -- William Lallemand
Re: Re: Re: [PATCH] get BoringSSL back to the game
as you already suggested "best effort" support policy, it should not require your time. am I correct ? пт, 4 февр. 2022 г. в 23:47, William Lallemand : > On Fri, Feb 04, 2022 at 11:02:24PM +0500, Илья Шипицин wrote: > > пт, 4 февр. 2022 г. в 19:16, William Lallemand : > > > > > On Fri, Feb 04, 2022 at 11:52:06AM +0100, William Lallemand wrote: > > > > > > > > I just tried to build with the latest boringSSL version, the problem > is > > > > on our side: > > > > > > > > We are defining X509_OBJECT_get0_X509_CRL() because it does not > exist in > > > > boringSSL, and inside it we are accessing the members of the > X509_OBJECT > > > > and it can't work since it's opaque. > > > > > > > > We need to use the accessors instead, or find an alternative API. > > > > > > > > > > So, basically, we need to extract some X509_CRL from an X509_STORE > which > > > could be done in OpenSSL with the X509_OBJECT API by using > > > X509_OBJECT_get0_X509_CRL() which is not available in boringSSL > > > > > > We are kind of stuck there because this is supposed to be the low > > > level API, now everything is opaque and we can't do much. > > > > > > The alternative would be to stop using the X509_STORE for storing the > > > CRL, and use a STACK_OF(X509_CRL)... But we use the store because it > > > could be either a X509_CRL or a X509... so it would be kind of > > > redefining a X509_STORE API... and I honestly don't want to do that. > > > > > > In my opinion there is a problem in their API because there is no > > > accessor for the X509_CRL in a X509_OBJECT. Which is kind of weird > > > because they have a X509_OBJECT_get0_X509() accessor, they probably > just > > > missed it. > > > > > > > we already use "#ifdef X509_V_FLAG_CRL_CHECK" for enabling those > functions. > > in theory we have 2 choices > > > > 1) adopt X509_OBJECT_get0_X509_CRL to BoringSSL in some way > > 2) exclude CRL manipulations under BoringSSL by adjusting ifdef > > I can't spend more time on this, feel free to open a bug report on their > bugtracker, this is probably better for everyone. > > You can adjust the ifdef as well. > > -- > William Lallemand >
Re: Re: Re: [PATCH] get BoringSSL back to the game
On Fri, Feb 04, 2022 at 11:02:24PM +0500, Илья Шипицин wrote: > пт, 4 февр. 2022 г. в 19:16, William Lallemand : > > > On Fri, Feb 04, 2022 at 11:52:06AM +0100, William Lallemand wrote: > > > > > > I just tried to build with the latest boringSSL version, the problem is > > > on our side: > > > > > > We are defining X509_OBJECT_get0_X509_CRL() because it does not exist in > > > boringSSL, and inside it we are accessing the members of the X509_OBJECT > > > and it can't work since it's opaque. > > > > > > We need to use the accessors instead, or find an alternative API. > > > > > > > So, basically, we need to extract some X509_CRL from an X509_STORE which > > could be done in OpenSSL with the X509_OBJECT API by using > > X509_OBJECT_get0_X509_CRL() which is not available in boringSSL > > > > We are kind of stuck there because this is supposed to be the low > > level API, now everything is opaque and we can't do much. > > > > The alternative would be to stop using the X509_STORE for storing the > > CRL, and use a STACK_OF(X509_CRL)... But we use the store because it > > could be either a X509_CRL or a X509... so it would be kind of > > redefining a X509_STORE API... and I honestly don't want to do that. > > > > In my opinion there is a problem in their API because there is no > > accessor for the X509_CRL in a X509_OBJECT. Which is kind of weird > > because they have a X509_OBJECT_get0_X509() accessor, they probably just > > missed it. > > > > we already use "#ifdef X509_V_FLAG_CRL_CHECK" for enabling those functions. > in theory we have 2 choices > > 1) adopt X509_OBJECT_get0_X509_CRL to BoringSSL in some way > 2) exclude CRL manipulations under BoringSSL by adjusting ifdef I can't spend more time on this, feel free to open a bug report on their bugtracker, this is probably better for everyone. You can adjust the ifdef as well. -- William Lallemand
Re: Re: [PATCH] get BoringSSL back to the game
пт, 4 февр. 2022 г. в 19:16, William Lallemand : > On Fri, Feb 04, 2022 at 11:52:06AM +0100, William Lallemand wrote: > > > > I just tried to build with the latest boringSSL version, the problem is > > on our side: > > > > We are defining X509_OBJECT_get0_X509_CRL() because it does not exist in > > boringSSL, and inside it we are accessing the members of the X509_OBJECT > > and it can't work since it's opaque. > > > > We need to use the accessors instead, or find an alternative API. > > > > So, basically, we need to extract some X509_CRL from an X509_STORE which > could be done in OpenSSL with the X509_OBJECT API by using > X509_OBJECT_get0_X509_CRL() which is not available in boringSSL > > We are kind of stuck there because this is supposed to be the low > level API, now everything is opaque and we can't do much. > > The alternative would be to stop using the X509_STORE for storing the > CRL, and use a STACK_OF(X509_CRL)... But we use the store because it > could be either a X509_CRL or a X509... so it would be kind of > redefining a X509_STORE API... and I honestly don't want to do that. > > In my opinion there is a problem in their API because there is no > accessor for the X509_CRL in a X509_OBJECT. Which is kind of weird > because they have a X509_OBJECT_get0_X509() accessor, they probably just > missed it. > we already use "#ifdef X509_V_FLAG_CRL_CHECK" for enabling those functions. in theory we have 2 choices 1) adopt X509_OBJECT_get0_X509_CRL to BoringSSL in some way 2) exclude CRL manipulations under BoringSSL by adjusting ifdef > > -- > William Lallemand >
Re: Re: [PATCH] get BoringSSL back to the game
On Fri, Feb 04, 2022 at 11:52:06AM +0100, William Lallemand wrote: > > I just tried to build with the latest boringSSL version, the problem is > on our side: > > We are defining X509_OBJECT_get0_X509_CRL() because it does not exist in > boringSSL, and inside it we are accessing the members of the X509_OBJECT > and it can't work since it's opaque. > > We need to use the accessors instead, or find an alternative API. > So, basically, we need to extract some X509_CRL from an X509_STORE which could be done in OpenSSL with the X509_OBJECT API by using X509_OBJECT_get0_X509_CRL() which is not available in boringSSL We are kind of stuck there because this is supposed to be the low level API, now everything is opaque and we can't do much. The alternative would be to stop using the X509_STORE for storing the CRL, and use a STACK_OF(X509_CRL)... But we use the store because it could be either a X509_CRL or a X509... so it would be kind of redefining a X509_STORE API... and I honestly don't want to do that. In my opinion there is a problem in their API because there is no accessor for the X509_CRL in a X509_OBJECT. Which is kind of weird because they have a X509_OBJECT_get0_X509() accessor, they probably just missed it. -- William Lallemand
Re: Re: [PATCH] get BoringSSL back to the game
On Fri, Feb 04, 2022 at 11:18:50AM +0100, William Lallemand wrote: > On Fri, Feb 04, 2022 at 09:57:25AM +0100, Remi Tricot-Le Breton wrote: > > > > > > On 02/02/2022 17:49, William Lallemand wrote: > > > > > >> Subject: [PATCH 2/7] BUILD: SSL: define X509_OBJECT for BoringSSL > > >> > > >> X509_OBJECT is opaque in BonringSSL, since we still use it, let us move > > >> it to openssl-compat.h > > >> > > >> from > > >> https://boringssl.googlesource.com/boringssl/+/refs/heads/2924/include/openssl/x509_vfy.h#120 > > > I'm not really fond of this kind of declaration, most of the time we > > > added helpers that were available in recent version of OpenSSL in this > > > file. But in this case, adding a whole structure that was removed... > > > with no guarantee that this will continue to work it's not a good idea. > > > > > > From what I get they aligned the opaque structures with the OpenSSL API, > > > so we probably will have the same problem with OpenSSL v3 without the > > > obsolete API. And we are currently in the process of porting it to > > > HAProxy. We probably need to change the code that uses X509_OBJECT. > > > So I suppose it will start to work during this portage. > > > > > X509_OBJECT and the APIs working on this structure were not marked as > > deprecated in OpenSSLv3, we are facing yet another place where BoringSSL > > seems a bit excessive in what they want to keep hidden. > > Managing BoringSSL would still be much more expensive than managing > > OpenSSLv3 if this kind of problem happens on many structures. > > > > Thanks for the clarification Rémi, I now remember having this > conversation :-) > > But when checking the commits they didn't make anything deprecated in > fact, they just made it opaque. > > https://boringssl.googlesource.com/boringssl/+/dddb60eb9700110835ff6e2b429de40a17006429 > > In this commit they pretend aligning with OpenSSL, which might be the > case, if you take a look at openssl/x509.h, they still define: > > - DEFINE_STACK_OF(X509_OBJECT) > - OPENSSL_EXPORT STACK_OF(X509_OBJECT) *X509_STORE_get0_objects(X509_STORE > *st); > > So either there is an export problem of the X509_OBJECT or we are > missing a include or something else is wrongly done. > > I'll made some test to check what's going on. > I just tried to build with the latest boringSSL version, the problem is on our side: We are defining X509_OBJECT_get0_X509_CRL() because it does not exist in boringSSL, and inside it we are accessing the members of the X509_OBJECT and it can't work since it's opaque. We need to use the accessors instead, or find an alternative API. -- William Lallemand
Re: Re: [PATCH] get BoringSSL back to the game
On Fri, Feb 04, 2022 at 09:57:25AM +0100, Remi Tricot-Le Breton wrote: > > > On 02/02/2022 17:49, William Lallemand wrote: > > > >> Subject: [PATCH 2/7] BUILD: SSL: define X509_OBJECT for BoringSSL > >> > >> X509_OBJECT is opaque in BonringSSL, since we still use it, let us move it > >> to openssl-compat.h > >> > >> from > >> https://boringssl.googlesource.com/boringssl/+/refs/heads/2924/include/openssl/x509_vfy.h#120 > > I'm not really fond of this kind of declaration, most of the time we > > added helpers that were available in recent version of OpenSSL in this > > file. But in this case, adding a whole structure that was removed... > > with no guarantee that this will continue to work it's not a good idea. > > > > From what I get they aligned the opaque structures with the OpenSSL API, > > so we probably will have the same problem with OpenSSL v3 without the > > obsolete API. And we are currently in the process of porting it to > > HAProxy. We probably need to change the code that uses X509_OBJECT. > > So I suppose it will start to work during this portage. > > > X509_OBJECT and the APIs working on this structure were not marked as > deprecated in OpenSSLv3, we are facing yet another place where BoringSSL > seems a bit excessive in what they want to keep hidden. > Managing BoringSSL would still be much more expensive than managing > OpenSSLv3 if this kind of problem happens on many structures. > Thanks for the clarification Rémi, I now remember having this conversation :-) But when checking the commits they didn't make anything deprecated in fact, they just made it opaque. https://boringssl.googlesource.com/boringssl/+/dddb60eb9700110835ff6e2b429de40a17006429 In this commit they pretend aligning with OpenSSL, which might be the case, if you take a look at openssl/x509.h, they still define: - DEFINE_STACK_OF(X509_OBJECT) - OPENSSL_EXPORT STACK_OF(X509_OBJECT) *X509_STORE_get0_objects(X509_STORE *st); So either there is an export problem of the X509_OBJECT or we are missing a include or something else is wrongly done. I'll made some test to check what's going on. -- William Lallemand
Re: [EXTERNAL] Re: [PATCH] get BoringSSL back to the game
On 02/02/2022 17:49, William Lallemand wrote: Subject: [PATCH 2/7] BUILD: SSL: define X509_OBJECT for BoringSSL X509_OBJECT is opaque in BonringSSL, since we still use it, let us move it to openssl-compat.h from https://boringssl.googlesource.com/boringssl/+/refs/heads/2924/include/openssl/x509_vfy.h#120 I'm not really fond of this kind of declaration, most of the time we added helpers that were available in recent version of OpenSSL in this file. But in this case, adding a whole structure that was removed... with no guarantee that this will continue to work it's not a good idea. From what I get they aligned the opaque structures with the OpenSSL API, so we probably will have the same problem with OpenSSL v3 without the obsolete API. And we are currently in the process of porting it to HAProxy. We probably need to change the code that uses X509_OBJECT. So I suppose it will start to work during this portage. X509_OBJECT and the APIs working on this structure were not marked as deprecated in OpenSSLv3, we are facing yet another place where BoringSSL seems a bit excessive in what they want to keep hidden. Managing BoringSSL would still be much more expensive than managing OpenSSLv3 if this kind of problem happens on many structures. Rémi
Re: [PATCH] get BoringSSL back to the game
Ilya, Adding Fred to the thread, so he can gives his opinion about the QUIC part. On Mon, Jan 31, 2022 at 10:22:01AM +0500, Илья Шипицин wrote: > 0001 .. 0003 are "pre QUIC" patches > 0007 is very simple Regarding the first patches: > Subject: [PATCH 3/7] REGTESTS: skip show_ssl_ocspresponse.vtc when BoringSSL > is used > > OCSP stapling implementation is not compatible with BoringSSL, test > is broken in BoringSSL Merged. > Subject: [PATCH 2/7] BUILD: SSL: define X509_OBJECT for BoringSSL > > X509_OBJECT is opaque in BonringSSL, since we still use it, let us move it to > openssl-compat.h > > from > https://boringssl.googlesource.com/boringssl/+/refs/heads/2924/include/openssl/x509_vfy.h#120 I'm not really fond of this kind of declaration, most of the time we added helpers that were available in recent version of OpenSSL in this file. But in this case, adding a whole structure that was removed... with no guarantee that this will continue to work it's not a good idea. >From what I get they aligned the opaque structures with the OpenSSL API, so we probably will have the same problem with OpenSSL v3 without the obsolete API. And we are currently in the process of porting it to HAProxy. We probably need to change the code that uses X509_OBJECT. So I suppose it will start to work during this portage. > Subject: [PATCH 1/7] BUILD: SSL: adjust guard for X509_get_X509_PUBKEY(x) > > BoringSSL defines that function since > https://boringssl.googlesource.com/boringssl/+/33f8d33af0dcb083610e978baad5a8b6e1cfee82 Merged. -- William Lallemand
Re: [PATCH] get BoringSSL back to the game
вт, 1 февр. 2022 г. в 15:35, William Lallemand : > On Mon, Jan 31, 2022 at 10:22:01AM +0500, Илья Шипицин wrote: > > > > Hello, > > > > Hello Ilya, > > > 0001 .. 0003 are "pre QUIC" patches > > 0004 .. 0006 are most questionable QUIC part > > 0007 is very simple > > > > > > we can discuss whether BoringSSL should be > > 1) dropped completely > > 2) supported, but no QUIC > > 3) supported for QUIC as well > > > > as for "3)" I've checked current state of QUICTLS, looks like its future > is > > not clear, it is not updated since mid december 2021, also it is not > clear > > whether OpenSSL is going to accept it or not. > > > > thanks, > > Ilya > > > BoringSSL is a burden in term of maintenance, since they only work in a > rolling release mode, we can't offer a real support in maintenancecc > branches. > > The current development team also won't have time to support fully > BoringSSL, the only library we are fully supporting is OpenSSL. > > The other libs are supported as a best effort, but not all features are > available. > > I don't mind merging some patches for improving boringSSL support, but > what's bothering me is building the master with the boringSSL HEAD in > the CI. Because API changes and can broke the build without us doing > anything. > I'm fine with best effort approach. Please merge patches that are appropriate. we'll figure out how to enable CI later. maybe "best effort" status should be documented on wiki, I guess people ask it pretty frequently. > > So if we don't want to be bothered to much we could activate the > boringSSL build weekly or something like that. But I don't want a > reminder each time I pushed that boringSSL broke something. > > Regarding the QUIC patches, I'll let the guys in charge of that decides, > but the development of QUIC in HAProxy is made with quictls currently. > > -- > William Lallemand >
Re: [PATCH] get BoringSSL back to the game
On Mon, Jan 31, 2022 at 10:22:01AM +0500, Илья Шипицин wrote: > > Hello, > Hello Ilya, > 0001 .. 0003 are "pre QUIC" patches > 0004 .. 0006 are most questionable QUIC part > 0007 is very simple > > > we can discuss whether BoringSSL should be > 1) dropped completely > 2) supported, but no QUIC > 3) supported for QUIC as well > > as for "3)" I've checked current state of QUICTLS, looks like its future is > not clear, it is not updated since mid december 2021, also it is not clear > whether OpenSSL is going to accept it or not. > > thanks, > Ilya BoringSSL is a burden in term of maintenance, since they only work in a rolling release mode, we can't offer a real support in maintenancecc branches. The current development team also won't have time to support fully BoringSSL, the only library we are fully supporting is OpenSSL. The other libs are supported as a best effort, but not all features are available. I don't mind merging some patches for improving boringSSL support, but what's bothering me is building the master with the boringSSL HEAD in the CI. Because API changes and can broke the build without us doing anything. So if we don't want to be bothered to much we could activate the boringSSL build weekly or something like that. But I don't want a reminder each time I pushed that boringSSL broke something. Regarding the QUIC patches, I'll let the guys in charge of that decides, but the development of QUIC in HAProxy is made with quictls currently. -- William Lallemand
