Re: [PR] Add verfied chain
On Mon, May 18, 2020 at 08:09:36PM +0200, William Dauchy wrote: > Hi Arjen, > > On Mon, May 18, 2020 at 6:02 PM Arjen Nienhuis wrote: > > I used PKCS7 because I did not know how to parse concatenated blobs. > > Mathilde, how did we planned to use it? :) > > > I think you should use SSL_get_peer_cert_chain because: > > - BoringSSL has no SSL_get0_verified_chain. > > - For debugging having all the certs is better. Especially if the chain > > is not valid. > > - In theory it's not always possible to do OCSP with the verified chain. > > OCSP is part of finding a valid chain. OpenSSL could choose a cert chain > > that doesn't pass OCSP while an other chain exists that can pass OCSP. > > Thank you for your feedbacks. > Do you want to handle the changes? Otherwise we can handle them and > mention you as the original proposition in the commit message. As you > wish. Hello guys, I just wanted to notify you that we are soon reaching the release of the 2.2 version. If the patches are ready before the end of the month, I'm okay with taking them for this branch since this is not a major change. -- William Lallemand
Re: [PR] Add verfied chain
Hi Arjen, On Mon, May 18, 2020 at 6:02 PM Arjen Nienhuis wrote: > I used PKCS7 because I did not know how to parse concatenated blobs. Mathilde, how did we planned to use it? :) > I think you should use SSL_get_peer_cert_chain because: > - BoringSSL has no SSL_get0_verified_chain. > - For debugging having all the certs is better. Especially if the chain > is not valid. > - In theory it's not always possible to do OCSP with the verified chain. > OCSP is part of finding a valid chain. OpenSSL could choose a cert chain > that doesn't pass OCSP while an other chain exists that can pass OCSP. Thank you for your feedbacks. Do you want to handle the changes? Otherwise we can handle them and mention you as the original proposition in the commit message. As you wish. -- William
Re: [PR] Add verfied chain
On 18-05-2020 15:57, William Lallemand wrote: On Mon, May 18, 2020 at 02:49:28PM +0200, William Dauchy wrote: Hello William L., On Wed, Dec 4, 2019 at 4:24 PM PR Bot wrote: Patch title(s): MINOR: add fetch 'ssl_c_verified_chain' Merge branch 'master' of https://github.com/haproxy/haproxy Link: https://github.com/haproxy/haproxy/pull/396 Edit locally: wget https://github.com/haproxy/haproxy/pull/396.patch && vi 396.patch Apply locally: curl https://github.com/haproxy/haproxy/pull/396.patch | git am - We were wondering if you add the time to have a look at this one? In fact we have a similar need and Mathilde started to work on a very similar patch, see https://github.com/ShimmerGlass/haproxy/commit/c63116fe7048320abc41709e4d1b25513da91f57 difference being, Mathilde simply concatenated the certs, and the patch from Arjen, uses PKCS7. Is there any specific reason to use PKCS7? note: it also refers to https://github.com/haproxy/haproxy/issues/297 Best, Hello, I suppose it was put in a PKCS7 container to be able to distinguish each DER part of the chain easily? So It can be used by an external tool. I'm not sure of what is done with the result of this. The two patches seem to have different approches, Arjen's one is using a SSL_get0_verified_chain() and Mathild's one is using SSL_get_peer_cert_chain(). I'm not sure what approach is the best, I suppose that SSL_get_peer_cert_chain() is better if we want to have the chain event if it wasn't verified and it could be completed with the ssl_c_verify sample fetch if we need this information! I will be grateful if a .vtc test file is also provided with sample fetches patches, it's difficult to test every sample fetches nowadays. There is already a vtc for client auth which is available here: https://git.haproxy.org/?p=haproxy.git;a=blob;f=reg-tests/ssl/ssl_client_auth.vtc Thanks! Hi, I used PKCS7 because I did not know how to parse concatenated blobs. I think you should use SSL_get_peer_cert_chain because: - BoringSSL has no SSL_get0_verified_chain. - For debugging having all the certs is better. Especially if the chain is not valid. - In theory it's not always possible to do OCSP with the verified chain. OCSP is part of finding a valid chain. OpenSSL could choose a cert chain that doesn't pass OCSP while an other chain exists that can pass OCSP. Groeten, Arjen
Re: [PR] Add verfied chain
Hi All, On 5/18/20 4:32 PM, William Dauchy wrote: > On Mon, May 18, 2020 at 3:58 PM William Lallemand > wrote: >> I suppose it was put in a PKCS7 container to be able to distinguish each >> DER part of the chain easily? So It can be used by an external tool. I'm not >> sure of what is done with the result of this. >> >> The two patches seem to have different approches, Arjen's one is >> using a SSL_get0_verified_chain() and Mathild's one is using >> SSL_get_peer_cert_chain(). I'm not sure what approach is the best, I >> suppose that SSL_get_peer_cert_chain() is better if we want to have the >> chain event if it wasn't verified and it could be completed with the >> ssl_c_verify sample fetch if we need this information! >> >> I will be grateful if a .vtc test file is also provided with sample >> fetches patches, it's difficult to test every sample fetches nowadays. >> >> There is already a vtc for client auth which is available here: >> https://git.haproxy.org/?p=haproxy.git;a=blob;f=reg-tests/ssl/ssl_client_auth.vtc > > Thanks for the feedbacks. I believe we will send our proposition soon. > According to openssl's doc about SSL_get_peer_cert_chain, SSL_get0_verified_chain: NOTES If the session is resumed peers do not send certificates so a NULL pointer is returned by these functions. It would be great if the ssl_c_ 's documentations precise if those information won't return something on resumed sessions. R, Emeric
Re: [PR] Add verfied chain
On Mon, May 18, 2020 at 3:58 PM William Lallemand wrote: > I suppose it was put in a PKCS7 container to be able to distinguish each > DER part of the chain easily? So It can be used by an external tool. I'm not > sure of what is done with the result of this. > > The two patches seem to have different approches, Arjen's one is > using a SSL_get0_verified_chain() and Mathild's one is using > SSL_get_peer_cert_chain(). I'm not sure what approach is the best, I > suppose that SSL_get_peer_cert_chain() is better if we want to have the > chain event if it wasn't verified and it could be completed with the > ssl_c_verify sample fetch if we need this information! > > I will be grateful if a .vtc test file is also provided with sample > fetches patches, it's difficult to test every sample fetches nowadays. > > There is already a vtc for client auth which is available here: > https://git.haproxy.org/?p=haproxy.git;a=blob;f=reg-tests/ssl/ssl_client_auth.vtc Thanks for the feedbacks. I believe we will send our proposition soon. -- William
Re: [PR] Add verfied chain
On Mon, May 18, 2020 at 02:49:28PM +0200, William Dauchy wrote: > Hello William L., > > On Wed, Dec 4, 2019 at 4:24 PM PR Bot wrote: > > Patch title(s): > >MINOR: add fetch 'ssl_c_verified_chain' > >Merge branch 'master' of https://github.com/haproxy/haproxy > > Link: > >https://github.com/haproxy/haproxy/pull/396 > > Edit locally: > >wget https://github.com/haproxy/haproxy/pull/396.patch && vi 396.patch > > Apply locally: > >curl https://github.com/haproxy/haproxy/pull/396.patch | git am - > > We were wondering if you add the time to have a look at this one? > In fact we have a similar need and Mathilde started to work on a very > similar patch, see > https://github.com/ShimmerGlass/haproxy/commit/c63116fe7048320abc41709e4d1b25513da91f57 > difference being, Mathilde simply concatenated the certs, and the > patch from Arjen, uses PKCS7. Is there any specific reason to use > PKCS7? > > note: it also refers to https://github.com/haproxy/haproxy/issues/297 > > Best, Hello, I suppose it was put in a PKCS7 container to be able to distinguish each DER part of the chain easily? So It can be used by an external tool. I'm not sure of what is done with the result of this. The two patches seem to have different approches, Arjen's one is using a SSL_get0_verified_chain() and Mathild's one is using SSL_get_peer_cert_chain(). I'm not sure what approach is the best, I suppose that SSL_get_peer_cert_chain() is better if we want to have the chain event if it wasn't verified and it could be completed with the ssl_c_verify sample fetch if we need this information! I will be grateful if a .vtc test file is also provided with sample fetches patches, it's difficult to test every sample fetches nowadays. There is already a vtc for client auth which is available here: https://git.haproxy.org/?p=haproxy.git;a=blob;f=reg-tests/ssl/ssl_client_auth.vtc Thanks! -- William Lallemand
Re: [PR] Add verfied chain
Hello William L., On Wed, Dec 4, 2019 at 4:24 PM PR Bot wrote: > Patch title(s): >MINOR: add fetch 'ssl_c_verified_chain' >Merge branch 'master' of https://github.com/haproxy/haproxy > Link: >https://github.com/haproxy/haproxy/pull/396 > Edit locally: >wget https://github.com/haproxy/haproxy/pull/396.patch && vi 396.patch > Apply locally: >curl https://github.com/haproxy/haproxy/pull/396.patch | git am - We were wondering if you add the time to have a look at this one? In fact we have a similar need and Mathilde started to work on a very similar patch, see https://github.com/ShimmerGlass/haproxy/commit/c63116fe7048320abc41709e4d1b25513da91f57 difference being, Mathilde simply concatenated the certs, and the patch from Arjen, uses PKCS7. Is there any specific reason to use PKCS7? note: it also refers to https://github.com/haproxy/haproxy/issues/297 Best, -- William
[PR] Add verfied chain
Dear list! Author: Arjen Nienhuis Number of patches: 2 This is an automated relay of the Github pull request: Add verfied chain Patch title(s): MINOR: add fetch 'ssl_c_verified_chain' Merge branch 'master' of https://github.com/haproxy/haproxy Link: https://github.com/haproxy/haproxy/pull/396 Edit locally: wget https://github.com/haproxy/haproxy/pull/396.patch && vi 396.patch Apply locally: curl https://github.com/haproxy/haproxy/pull/396.patch | git am - Description: Instructions: This github pull request will be closed automatically; patch should be reviewed on the haproxy mailing list (haproxy@formilux.org). Everyone is invited to comment, even the patch's author. Please keep the author and list CCed in replies. Please note that in absence of any response this pull request will be lost.
