[PATCH] MINOR: ssl: add ssl-skip-self-issued-ca global option

2020-04-22 Thread Emmanuel Hocdet

and voila:



0001-MINOR-ssl-add-ssl-skip-self-issued-ca-global-option.patch
Description: Binary data




Re: [PATCH] MINOR: ssl: skip self issued CA in cert chain for ssl_ctx

2020-04-22 Thread Emmanuel Hocdet


> Le 21 avr. 2020 à 10:58, William Lallemand  a écrit :
> 
> On Fri, Apr 03, 2020 at 10:34:12AM +0200, Emmanuel Hocdet wrote:
>> 
>>> Le 31 mars 2020 à 18:40, William Lallemand  a écrit 
>>> :
>>> 
>>> On Thu, Mar 26, 2020 at 06:29:48PM +0100, William Lallemand wrote:
>>>> 
>>>> After some thinking and discussing with people involved in this part of
>>>> HAProxy. I'm not feeling very confortable with setting this behavior by
>>>> default, on top on that the next version is an LTS so its not a good
>>>> idea to change this behavior yet. I think in most case it won't be a
>>>> problem but it would be better if it's enabled by an option in the
>>>> global section.
>>>> 
>>> 
>>> Hi Manu,
>>> 
>>> Could you take a look at this? Because I already merged your first
>>> patch, so if we don't do anything about it we may revert it before the
>>> release.
>>> 
>>> Thanks a lot!
>> 
>> Hi William,
>> 
>> It’s really safe because self Issued CA is the X509 end chain by definition,
>> but yes it change the behaviour.
>> Why not an option in global section.
>> 
>> ++
>> Manu
>> 
> Hello Manu,
> 
> I hope you are well and live well this confinement period.
> 
> Did you had time to work on the documentation patch and the global
> option?
> 

Hi William,

It’s ok, thanks. I hope is the case for all of us.

I will take time to do it.

++
Manu




Re: [PATCH] MINOR: ssl: skip self issued CA in cert chain for ssl_ctx

2020-04-03 Thread Emmanuel Hocdet


> Le 31 mars 2020 à 18:40, William Lallemand  a écrit :
> 
> On Thu, Mar 26, 2020 at 06:29:48PM +0100, William Lallemand wrote:
>> 
>> After some thinking and discussing with people involved in this part of
>> HAProxy. I'm not feeling very confortable with setting this behavior by
>> default, on top on that the next version is an LTS so its not a good
>> idea to change this behavior yet. I think in most case it won't be a
>> problem but it would be better if it's enabled by an option in the
>> global section.
>> 
> 
> Hi Manu,
> 
> Could you take a look at this? Because I already merged your first
> patch, so if we don't do anything about it we may revert it before the
> release.
> 
> Thanks a lot!

Hi William,

It’s really safe because self Issued CA is the X509 end chain by definition,
but yes it change the behaviour.
Why not an option in global section.

++
Manu




Re: [PATCH] MINOR: ssl: skip self issued CA in cert chain for ssl_ctx

2020-03-26 Thread Emmanuel Hocdet

> Le 26 mars 2020 à 14:11, Илья Шипицин  a écrit :
> 
> 
> 
> чт, 26 мар. 2020 г. в 17:27, Emmanuel Hocdet  <mailto:m...@gandi.net>>:
> 
> > Le 26 mars 2020 à 13:02, Илья Шипицин  > <mailto:chipits...@gmail.com>> a écrit :
> > 
> > RootCA is needed if you send cross certificate as well.
> > 
> > It is very rare but legitimate case
> 
> It’s only for self issued CA, it should be safe, right?
> 
> I do not know what "yes" or "no" would mean :)
> 
> by cross certificate I mean chain like that
> 
> server cert --> intermediate CA --> root CA --> cross certificate
> 
> https://knowledge.digicert.com/generalinformation/INFO2523.html 
> <https://knowledge.digicert.com/generalinformation/INFO2523.html>
> 
> root CA is self issued

self issued CA is a root CA
Subject == Issuer

In your example:

Subject: C = US, O = "thawte, Inc.", OU = Certification Services Division, OU = 
"(c) 2006 thawte, Inc. - For authorized use only", CN = thawte Primary Root CA
Issuer: C = ZA, ST = Western Cape, L = Cape Town, O = Thawte Consulting cc, OU 
= Certification Services Division, CN = Thawte Premium Server CA, emailAddress 
= premium-ser...@thawte.com <mailto:premium-ser...@thawte.com>

Re: [PATCH] MINOR: ssl: skip self issued CA in cert chain for ssl_ctx

2020-03-26 Thread Emmanuel Hocdet


> Le 26 mars 2020 à 13:02, Илья Шипицин  a écrit :
> 
> RootCA is needed if you send cross certificate as well.
> 
> It is very rare but legitimate case

It’s only for self issued CA, it should be safe, right?




Re: [PATCH] MINOR: ssl: skip self issued CA in cert chain for ssl_ctx

2020-03-25 Thread Emmanuel Hocdet

Hi,
Patch rebase from master.

> Le 6 mars 2020 à 17:06, Emmanuel Hocdet  a écrit :
> 
> Hi,
> 
> 
> Patch proposal.
> I will update the documentation if this feature is approved.
> 

++
Manu



0001-MINOR-ssl-skip-self-issued-CA-in-cert-chain-for-ssl_.patch
Description: Binary data


[PATCH] MINOR: ssl: rework add cert chain to CTX to be libssl independent

2020-03-23 Thread Emmanuel Hocdet
Hi,This patch remove #ifdef compatibility for add cert chain to CTX, goal is to simplify code.It’s an extract from "[PATCH] MINOR: ssl: skip self issued CA in cert chain for ssl_ctx » proposal.++Manu

0001-MINOR-ssl-rework-add-cert-chain-to-CTX-to-be-libssl-.patch
Description: Binary data


Re: [PATCH] CLEANUP: ssl: rename ssl_get_issuer_chain to ssl_get0_issuer_chain

2020-03-23 Thread Emmanuel Hocdet

> Le 23 mars 2020 à 15:12, William Lallemand  a écrit :
> 
> On Mon, Mar 23, 2020 at 02:50:03PM +0100, Emmanuel Hocdet wrote:
>> 
>> As discussed in #559
>> 
> 
> Can't we return directly a STACK_OF(X509)* structure instead of the
> struct issuer_chain * ?
> 
> Because I have the impression that we use the struct issuer_chain only
> to lookup and we only use the chain field of this structure.


I changed that to be able ro extract the path for « show ssl cert »:

chain = ckchs->ckch->chain;
if (chain == NULL) {
struct issuer_chain *issuer;
issuer = ssl_get0_issuer_chain(ckchs->ckch->cert);
if (issuer) {
chain = issuer->chain;
chunk_appendf(out, "Chain Filename: ");
chunk_appendf(out, "%s\n", issuer->path);
}
}

[PATCH] CLEANUP: ssl: rename ssl_get_issuer_chain to ssl_get0_issuer_chain

2020-03-23 Thread Emmanuel Hocdet

As discussed in #559



0001-CLEANUP-ssl-rename-ssl_get_issuer_chain-to-ssl_get0_.patch
Description: Binary data


Re: [PATCH] fix memory leak, issue 559

2020-03-23 Thread Emmanuel Hocdet

Hi,

This issue was introduced by #516.
find_chain must not be freed.
patch attached.

> Le 21 mars 2020 à 15:23, Илья Шипицин  a écrit :
> 
> Hello,
> 
> I attached patch that fixes memory leak, described in #559
> 

++
Manu



0001-BUG-MINOR-ssl-memory-leak-when-find_chain-is-NULL.patch
Description: Binary data


[PATCH] MINOR: ssl: skip self issued CA in cert chain for ssl_ctx

2020-03-06 Thread Emmanuel Hocdet
Hi,


Patch proposal.
I will update the documentation if this feature is approved.

++
Manu



0001-MINOR-ssl-skip-self-issued-CA-in-cert-chain-for-ssl_.patch
Description: Binary data


[PATCH] MINOR: ssl: add "ca-verify-file" directive

2020-03-04 Thread Emmanuel Hocdet
Hi,

« ca-no-names-file » renamed to « ca-verify-file »

++
Manu



0001-MINOR-ssl-add-ca-verify-file-directive.patch
Description: Binary data






[PATCH] MINOR: ssl: add "ca-no-names-file" directive

2020-03-03 Thread Emmanuel Hocdet
rebase from dev branch:(https://github.com/haproxy/haproxy/issues/404)++ManuLe 20 déc. 2019 à 17:00, Emmanuel Hocdet <m...@gandi.net> a écrit :patch update,Le 19 déc. 2019 à 17:08, Emmanuel Hocdet <m...@gandi.net> a écrit :With this proposition, ca-root-file should be rename to something like ca-end-file.Refer to https://github.com/haproxy/haproxy/issues/404 discussion.Le 19 déc. 2019 à 13:10, Emmanuel Hocdet <m...@gandi.net> a écrit :Hi,The purpose of this patch is to fix #404 and keep compatibility with actual"ca-file » directive for bind line.

0001-MINOR-ssl-add-ca-no-names-file-directive.patch
Description: Binary data


Re: [PATCH 4/4] MINOR: ssl: "show ssl cert" command should print the "Chain filename:"

2020-02-26 Thread Emmanuel Hocdet
Hi,Le 18 févr. 2020 à 17:49, Emmanuel Hocdet <m...@gandi.net> a écrit :Yes. Show the chain-filename would be very helpful.For that i think a good way would be to keep ckch->chain and ckch->issuerwith value (or NULL) from PEM/, and resolve chain and ocsp_issuerwhen needed. « show ssl cert » will be able to find the origin of chain (and ocsp_issuer)without  store a new state. The drawback(?) is that .issuer file will be loaded, in every case, if present.Patch series to do that:example:Issuer: /C=US/O=Let's Encrypt/CN=Let's Encrypt Authority X3Chain filename: /etc/haproxy/issuers/letsencryptEC.pemRebased with current dev branch.++Manu

0001-MINOR-ssl-move-find-certificate-chain-code-to-its-ow.patch
Description: Binary data


0002-MINOR-ssl-resolve-issuers-chain-later.patch
Description: Binary data


0003-MINOR-ssl-resolve-ocsp_issuer-later.patch
Description: Binary data


0004-MINOR-ssl-show-ssl-cert-command-should-print-the-Cha.patch
Description: Binary data


[PATCH 4/4] MINOR: ssl: "show ssl cert" command should print the "Chain filename:"

2020-02-18 Thread Emmanuel Hocdet
Hi,Le 18 févr. 2020 à 11:45, Emmanuel Hocdet <m...@gandi.net> a écrit :I think we will probably need more information in the "show ssl cert"output in the future so the users can debug this kind of feature easily.Yes. Show the chain-filename would be very helpful.For that i think a good way would be to keep ckch->chain and ckch->issuerwith value (or NULL) from PEM/, and resolve chain and ocsp_issuerwhen needed. « show ssl cert » will be able to find the origin of chain (and ocsp_issuer)without  store a new state. The drawback(?) is that .issuer file will be loaded, in every case, if present.Patch series to do that:example:Issuer: /C=US/O=Let's Encrypt/CN=Let's Encrypt Authority X3Chain filename: /etc/haproxy/issuers/letsencryptEC.pem++Manu

0001-MINOR-ssl-move-find-certificate-chain-code-to-its-ow.patch
Description: Binary data


0002-MINOR-ssl-resolve-issuers-chain-later.patch
Description: Binary data


0003-MINOR-ssl-resolve-ocsp_issuer-later.patch
Description: Binary data


0004-MINOR-ssl-show-ssl-cert-command-should-print-the-Cha.patch
Description: Binary data


Re: [PATCH] MINOR: ssl: add "issuers-chain-path" directive.

2020-02-18 Thread Emmanuel Hocdet


> Le 18 févr. 2020 à 14:36, William Lallemand  a écrit :
> 
> On Tue, Feb 18, 2020 at 01:58:39PM +0100, Emmanuel Hocdet wrote:
>> 
>>> Le 18 févr. 2020 à 11:45, Emmanuel Hocdet  a écrit :
>>> 
>>>> Can you add a little bit of explanation on how the discovery of the
>>>> issuer is done in the documentation?
>>>> 
>>> ok
>> 
>> 
>> documentation updated:
>> 
> 
> Thanks Manu!
> 
> Merged and pushed in master.
> 

w00t!
Thanks

Manu




Re: [PATCH] MINOR: ssl: add "issuers-chain-path" directive.

2020-02-18 Thread Emmanuel Hocdet
Le 18 févr. 2020 à 11:45, Emmanuel Hocdet <m...@gandi.net> a écrit :Can you add a little bit of explanation on how the discovery of theissuer is done in the documentation?okdocumentation updated:

0001-MINOR-ssl-add-issuers-chain-path-directive.patch
Description: Binary data


Re: [PATCH] MINOR: ssl: add "issuers-chain-path" directive.

2020-02-18 Thread Emmanuel Hocdet
Hi William

> Le 14 févr. 2020 à 15:59, William Lallemand  a écrit :
> 
> On Fri, Feb 14, 2020 at 03:25:48PM +0100, Emmanuel Hocdet wrote:
>> Hi,
>> 
>> Is there any hope that this proposal will be considered before HAproxy 2.2?
>> 
>> ++
>> Manu
> 
> Hello,
> 
> I'm ok with the feature itself. I'm still not fond of an
> "auto-discovery" based on the SKID, but I agree that's probably the most
> convenient way of doing it for the user.
> 
great news

> The way it's done we won't be able to change the issuers from the CLI
> easily, but these files don't change too often so that's not a problem
> at the moment.
> 
I agree.

> Can you add a little bit of explanation on how the discovery of the
> issuer is done in the documentation?
> 
ok

> I think we will probably need more information in the "show ssl cert"
> output in the future so the users can debug this kind of feature easily.
> 

Yes. Show the chain-filename would be very helpful.
For that i think a good way would be to keep ckch->chain and ckch->issuer
with value (or NULL) from PEM/, and resolve chain and ocsp_issuer
when needed. « show ssl cert » will be able to find the origin of chain (and 
ocsp_issuer)
without  store a new state. The drawback(?) is that .issuer file will be 
loaded, in every case, if present.
Something like i do for ‘chain’ directive POC: 
https://github.com/ehocdet/haproxy/commits/chain

++
Manu





Re: [PATCH] MINOR: ssl: add "issuers-chain-path" directive.

2020-02-14 Thread Emmanuel Hocdet
Hi,

Is there any hope that this proposal will be considered before HAproxy 2.2?

++
Manu


> Le 31 janv. 2020 à 16:06, Emmanuel Hocdet  a écrit :
> 
> 
>> Le 31 janv. 2020 à 12:22, Emmanuel Hocdet  a écrit :
> 
>> 
>> I will send a new patch for « issuers-chain-path » with corrections.
>> 
> 
> 
> 
> <0001-MINOR-ssl-add-issuers-chain-path-directive.patch>




Re: [PATCH] ssl certificates load speedup and dedup (pem/ctx)

2020-01-31 Thread Emmanuel Hocdet
Hi William,

> Le 27 janv. 2020 à 16:55, Emmanuel Hocdet  a écrit :
>> 
>> With ‘ssl crt foo.pem chain bar.pem’, or crt-list with ‘foo.pem [chain 
>> bar.pem]’,
>> deduplicate chain look like deduplicate ca-file.
>> Find ocsp_issuer with this chain doesn’t work directly, but it seems doable.
>> For CLI, reload cert when chain is updated seem also complicated, perhaps
>> less problematic than others solutions. 
>> 
> 
> Proposal for ‘chain’ parameter:
> https://github.com/ehocdet/haproxy/commits/chain 
> <https://github.com/ehocdet/haproxy/commits/chain>
> 

This approach is really too complicated to use and a source of errors.
The first patch alone « issuer-path » ( move to « issuers-chain-path ») really 
do
a better job.
For the possible reload of chain certificates (as you suggested), both are 
equivalent in complexity.
‘set ssl issuers-chain  ’ with « issuers-chain-path » should
accept  only if it's compatible with the stored issuers-chain ()
(via SKID)

I will send a new patch for « issuers-chain-path » with corrections.

++
Manu



Re: [PATCH] ssl certificates load speedup and dedup (pem/ctx)

2020-01-27 Thread Emmanuel Hocdet

Hi William,

> 
> With ‘ssl crt foo.pem chain bar.pem’, or crt-list with ‘foo.pem [chain 
> bar.pem]’,
> deduplicate chain look like deduplicate ca-file.
> Find ocsp_issuer with this chain doesn’t work directly, but it seems doable.
> For CLI, reload cert when chain is updated seem also complicated, perhaps
> less problematic than others solutions. 
> 

Proposal for ‘chain’ parameter:
https://github.com/ehocdet/haproxy/commits/chain 


++
Manu



Re: [PATCH] ssl certificates load speedup and dedup (pem/ctx)

2020-01-24 Thread Emmanuel Hocdet


> Le 24 janv. 2020 à 16:38, William Lallemand  a écrit :
> 
> On Fri, Jan 24, 2020 at 01:22:05PM +0100, Emmanuel Hocdet wrote:
>> 
>> Hi William,
>> 
> Hello Manu!
> 
>>> Le 23 janv. 2020 à 16:20, William Lallemand  a 
>>> écrit :
>>> 
>>> That's not a good idea to be able to add a new path to the list each time 
>>> this
>>> keyword is found, this is not how the configuration works in the global
>>> section, a new line with the keyword should remove the previous one.
>>> 
>> 
>> Okay, or refuse the second one.
> 
> That's really important to be able to overwrite the previous value, because 
> the
> global section could be superseded. For example in the ALOHA appliances, there
> is a global.def file which is a configuration file with default values, and
> these values can be overwritten in the haproxy.cfg.
> 
>> To be clear, 'issuer-path' can be confused, i could be named 
>> 'issuers-chain-path’.
>> 
> 
> That might be a good idea.
> 
>>>> -  PEM files into one (e.g. cat cert.pem key.pem > combined.pem). If your 
>>>> CA
>>>> +  PEM files into one (e.g. cat key.pem cert.pem > combined.pem). If your 
>>>> CA
>>> 
>>> 
>>> I'm not sure to understand why this was changed, maybe by mistake? If not 
>>> could
>>> you elaborate in the commit message, or split this part in another commit if
>>> it's not related to the feature.
>>> 
>> 
>> Indeed, is not related. It’s more optimal when PEM is parsed.
>> Key is parsed first, Cert next after rewind the BIO at begining.
>> 
> 
> Oh, that's right. We can probably update the documentation by explaining
> why it's more efficient in this order. I might be wrong but I think the
> previous order was mandatory because of the way the certificates were loaded
> before, because it wasn't using the BIO, but the high level functions of
> openssl.
> 
>> 
>> It’s not really magic, it’s based on X509, how certificate are build, and
>> and how to check(find) an issuer for a certificate (via X509_check_issued 
>> with openssl).
>> It’s what libssl do to verify certificat, find the appropriate root CA 
>> (loaded via ca-file in
>> HAproxy).
> 
> I understand the principle, but my fear is the fact that its a global context
> only referenced by IDs, and you don't specified which file are used in the
> configuration.
> 
>> For certificats generated via the same issuer, there are no reason to not 
>> complete it 
>> with the same chain. At worst, not used the functionality (include a chain 
>> who match the issuer).
>> I have experienced this for years on our multi-tenant platform. Customers 
>> upload 
>> their certificates (the chain are ignored) and we accept it if we can verify 
>> it.
>> For that we need, in addition to "ca-certificates » pkg, all intermediate 
>> certificates from
>> accepted CA (Comodo, LetsEncrypt, CAcert, Cloudflare, …).
>> When PEM file are generated for HAproxy, the chain of issuers is construct 
>> from
>> all this intermediate certificates. This guarantees to have a correct chain 
>> for all certificates,
>> and chain can be updated automatically.
>> When intermediate certificates repository is updated (by humans) we can have 
>> root
>> certificate (detected and ignored), or more than one issuer for a 
>> certificate (sha1/sha256 ...):
>> the better (valid) issuer is chosen.
>> (1) For haproxy, I didn't want to do that initially, because its’ tricky and 
>> too "magic ». Add such
>> feature without change the initial behaviour:
>> Have PEM with Key+Cert+Chain or have Key+Cert and complete the Chain (via 
>> the start of 
>> the chain - the issuer -).
>> 
> 
> Ok I understand your usage better, it completely makes sense. So the provider
> will fill itself the directory with trusted files, this is not something which
> will be filled by the customer.
> 
>>> Maybe we should use a keyword on the bind line to load it instead of this, 
>>> like
>>> `ssl crt foo.pem issuer bar.pem` and this keyword could be applied to a lot
>>> of certificates by using the default-server keyword.
>>> 
>> (default-server -> ssl-default-bind-options)
>> (‘issuer’ -> ‘issuers-chain’ to be less confused)
>> I don’t like that very much. We can have a lot of certificats with different 
>> issuer, and only crt-list
>> with this issuers-chain param could do the job. It’s very unwieldy. On the 
>> other hand it takes
>>

Re: [PATCH] ssl certificates load speedup and dedup (pem/ctx)

2020-01-24 Thread Emmanuel Hocdet
Hi Tim,

> Le 23 janv. 2020 à 17:21, Tim Düsterhus  a écrit :
> 
> Manu,
> 
> Am 21.01.20 um 12:42 schrieb Emmanuel Hocdet:
>> Patches updated, depend on "[PATCH] BUG/MINOR: ssl:
>> ssl_sock_load_pem_into_ckch is not consistent"
> 
> Out of curiosity:
> 
>> +issuer-path 
>> +  Assigns a directory to load certificate chain for issuer completion. All
>> +  files must be in PEM format. For certificates loaded with "crt" or 
>> "crt-list",
>> +  if certificate chain is not included in PEM (also commonly known as 
>> intermediate
>> +  certificate), haproxy will complete chain if issuer match the first 
>> certificate
>> +  of the chain loaded with "issuer-path". "issuer-path" directive can be set
>> +  several times.
> 
> Will HAProxy complete the chain if multiple intermediate certificates
> are required?
> 

Patch don’t do that.

> Consider this:
> 
> Root CA -> Intermediate CA -> Intermediate CB -> End Certificate
> 
Usually, Root CA should not be include in the chain.

> I configure `issuer-path` to a directory that contains the following
> certificates:
> 
> - Root CA
> - Intermediate CA
> - Intermediate CB
> 

You should have a file with:
- intermediate CB + intermediate CA

> Then I configure a `crt` pointing to a file containing only the End
> Certificate.
> 
> What will HAProxy send to the client?
> 

End Certificate + intermediate CB + intermediate CA
The same as if you have crt with  End Certificate + intermediate CB + 
intermediate CA

++
Manu





Re: [PATCH] ssl certificates load speedup and dedup (pem/ctx)

2020-01-24 Thread Emmanuel Hocdet


Hi William,

> Le 23 janv. 2020 à 16:20, William Lallemand  a écrit :
> 
> On Tue, Jan 21, 2020 at 12:42:04PM +0100, Emmanuel Hocdet wrote:
>> Hi,
>> 
>> Patches updated, depend on "[PATCH] BUG/MINOR: ssl: 
>> ssl_sock_load_pem_into_ckch is not consistent"
>> 
> 
> Hello,
> 
> It could be great to share more of the certificates in memory, but some 
> points are
> confusing in my opinion:
> 
>> +issuer-path 
>> +  Assigns a directory to load certificate chain for issuer completion. All
>> +  files must be in PEM format. For certificates loaded with "crt" or 
>> "crt-list",
>> +  if certificate chain is not included in PEM (also commonly known as 
>> intermediate
>> +  certificate), haproxy will complete chain if issuer match the first 
>> certificate
>> +  of the chain loaded with "issuer-path". "issuer-path" directive can be set
>> +  several times.
>> +
> 
> That's not a good idea to be able to add a new path to the list each time this
> keyword is found, this is not how the configuration works in the global
> section, a new line with the keyword should remove the previous one.
> 

Okay, or refuse the second one.
To be clear, 'issuer-path' can be confused, i could be named 
'issuers-chain-path’.

>> -  PEM files into one (e.g. cat cert.pem key.pem > combined.pem). If your CA
>> +  PEM files into one (e.g. cat key.pem cert.pem > combined.pem). If your CA
> 
> 
> I'm not sure to understand why this was changed, maybe by mistake? If not 
> could
> you elaborate in the commit message, or split this part in another commit if
> it's not related to the feature.
> 

Indeed, is not related. It’s more optimal when PEM is parsed.
Key is parsed first, Cert next after rewind the BIO at begining.


>> +/* Find Certificate Chain in global */
>> +if (chain == NULL) {
>> +AUTHORITY_KEYID *akid;
>> +akid = X509_get_ext_d2i(cert, NID_authority_key_identifier, 
>> NULL, NULL);
>> +if (akid) {
>> +struct issuer_chain *issuer;
>> +struct eb64_node *node;
>> +u64 hk;
>> +hk = XXH64(ASN1_STRING_get0_data(akid->keyid), 
>> ASN1_STRING_length(akid->keyid), 0);
>> +for (node = eb64_lookup(_ssl.cert_issuer_tree, 
>> hk); node; node = eb64_next(node)) {
>> +issuer = container_of(node, typeof(*issuer), 
>> node);
>> +if 
>> (X509_check_issued(sk_X509_value(issuer->chain, 0), cert) == X509_V_OK) {
>> +chain = 
>> X509_chain_up_ref(issuer->chain);
>> +break;
>> +}
>> +}
>> +AUTHORITY_KEYID_free(akid);
>> +}
>> +}
> 
> I'm not sure about this, what's bothering me is that the certificate is not
> found using a filename, but only found by looking for the SKID. So it's kind 
> of
> magic because it's not named in the configuration. It can be bothering if you
> don't want to complete the chain for some of the certificates, or if you have 
> a
> multi-tenant configuration.
> 

It’s not really magic, it’s based on X509, how certificate are build, and
and how to check(find) an issuer for a certificate (via X509_check_issued with 
openssl).
It’s what libssl do to verify certificat, find the appropriate root CA (loaded 
via ca-file in
HAproxy).
For certificats generated via the same issuer, there are no reason to not 
complete it 
with the same chain. At worst, not used the functionality (include a chain who 
match the issuer).

I have experienced this for years on our multi-tenant platform. Customers 
upload 
their certificates (the chain are ignored) and we accept it if we can verify it.
For that we need, in addition to "ca-certificates » pkg, all intermediate 
certificates from
accepted CA (Comodo, LetsEncrypt, CAcert, Cloudflare, …).
When PEM file are generated for HAproxy, the chain of issuers is construct from
all this intermediate certificates. This guarantees to have a correct chain for 
all certificates,
and chain can be updated automatically.
When intermediate certificates repository is updated (by humans) we can have 
root
certificate (detected and ignored), or more than one issuer for a certificate 
(sha1/sha256 ...):
the better (valid) issuer is chosen.
(1) For haproxy, I didn't want to do that initially, because its’ tricky and 
too "magic ». Add such
feature without change the initial behaviour:
Have PEM with Key+Cert+Chain or have Key+Cert and 

PATCH] BUG/MINOR: ssl: ocsp_issuer must be set in the right way

2020-01-23 Thread Emmanuel Hocdet

Following discussion from "[PATCH] BUG/MINOR: ssl: ssl_sock_load_pem_into_ckch 
is not consistent ».



0001-BUG-MINOR-ssl-ocsp_issuer-must-be-set-in-the-right-w.patch
Description: Binary data


Re: [PATCH] BUG/MINOR: ssl: ssl_sock_load_pem_into_ckch is not consistent

2020-01-23 Thread Emmanuel Hocdet


> Le 23 janv. 2020 à 11:19, William Lallemand  a écrit :
> 
> On Wed, Jan 22, 2020 at 05:22:51PM +0100, Emmanuel Hocdet wrote:
>> 
>>> Le 22 janv. 2020 à 15:56, William Lallemand  a 
>>> écrit :
>>> 
>> Indeed, and the case of ckch->ocsp_issuer is also problematic. 
>> 
> 
> Right.
> 
> I fixed this, thanks. 
> 

See, i think it would be cleaner to use 
ssl_sock_free_cert_ket_and_chain_contents.
My concern for ckch->ocsp_issuer is also to set it correctly (patch is coming).

Manu




Re: [PATCH] BUG/MINOR: ssl: ssl_sock_load_pem_into_ckch is not consistent

2020-01-22 Thread Emmanuel Hocdet


> Le 22 janv. 2020 à 15:56, William Lallemand  a écrit :
> 
> On Mon, Jan 20, 2020 at 05:13:13PM +0100, Emmanuel Hocdet wrote:
>> 
>> Hi,
>> 
>> Proposal to fix the issue.
>> 
> 
> The purpose at the beginning was to be able to keep a .dh / .ocsp etc. But 
> that
> probably does not make sense once you changed the private key, we should
> probably remove everything in a ckch once we load a new private key.
> 
Indeed, and the case of ckch->ocsp_issuer is also problematic. 

> Thanks, merged!
> 

Thanks

Manu




Re: [PATCH] ssl certificates load speedup and dedup (pem/ctx)

2020-01-21 Thread Emmanuel Hocdet
Hi,Patches updated, depend on "[PATCH] BUG/MINOR: ssl: ssl_sock_load_pem_into_ckch is not consistent"++ManuLe 10 avr. 2019 à 13:23, Emmanuel Hocdet <m...@gandi.net> a écrit :Hi,Updated patch serie:Fix OpenSSL < 1.0.2 compatibilty.More generic key for issuers ebtree.++Manu

0001-MINOR-ssl-add-issuer-path-directive.patch
Description: Binary data


0002-MINOR-ssl-cli-set-ssl-issuer-load-an-issuer-certific.patch
Description: Binary data


[PATCH] MINOR: ssl: accept 'verify' bind option with 'set ssl cert'

2020-01-20 Thread Emmanuel Hocdet
Hi,

A last patch for today.

++
Manu



0001-MINOR-ssl-accept-verify-bind-option-with-set-ssl-cer.patch
Description: Binary data


[PATCH] BUG/MINOR: ssl: ssl_sock_load_pem_into_ckch is not consistent

2020-01-20 Thread Emmanuel Hocdet

Hi,

Proposal to fix the issue.

++
Manu 



0001-BUG-MINOR-ssl-ssl_sock_load_pem_into_ckch-is-not-con.patch
Description: Binary data


[PATCH] BUG/MINOR: ssl: fix 3 memory leaks with set ssl cert

2020-01-20 Thread Emmanuel Hocdet

Hi,

Fix memory leaks with « set ssl cert ».

++
Manu




0001-BUG-MINOR-ssl-ssl_sock_load_ocsp_response_from_file-.patch
Description: Binary data


0002-BUG-MINOR-ssl-ssl_sock_load_issuer_file_into_ckch-me.patch
Description: Binary data


0003-BUG-MINOR-ssl-ssl_sock_load_sctl_from_file-memory-le.patch
Description: Binary data


[PATCH] MINOR: ssl: add "ca-no-names-file" directive

2019-12-20 Thread Emmanuel Hocdet
patch update,Le 19 déc. 2019 à 17:08, Emmanuel Hocdet <m...@gandi.net> a écrit :With this proposition, ca-root-file should be rename to something like ca-end-file.Refer to https://github.com/haproxy/haproxy/issues/404 discussion.Le 19 déc. 2019 à 13:10, Emmanuel Hocdet <m...@gandi.net> a écrit :Hi,The purpose of this patch is to fix #404 and keep compatibility with actual"ca-file » directive for bind line.++Manu

0001-MINOR-ssl-add-ca-no-names-file-directive.patch
Description: Binary data


Re: [PATCH] MINOR: ssl: add "ca-root-file" directive

2019-12-19 Thread Emmanuel Hocdet

With this proposition, ca-root-file should be rename to something like 
ca-end-file.
Refer to https://github.com/haproxy/haproxy/issues/404 
<https://github.com/haproxy/haproxy/issues/404> discussion.

> Le 19 déc. 2019 à 13:10, Emmanuel Hocdet  a écrit :
> 
> 
> Hi,
> 
> The purpose of this patch is to fix #404 and keep compatibility with actual
> "ca-file » directive for bind line.
> 
> ++
> Manu
> 
> <0001-MINOR-ssl-add-ca-root-file-directive.patch>
> 



[PATCH] MINOR: ssl: add "ca-root-file" directive

2019-12-19 Thread Emmanuel Hocdet

Hi,

The purpose of this patch is to fix #404 and keep compatibility with actual
"ca-file » directive for bind line.

++
Manu



0001-MINOR-ssl-add-ca-root-file-directive.patch
Description: Binary data




[PATCH] BUG/MINOR: ssl: certificate choice can be unexpected with openssl >= 1.1.1

2019-12-02 Thread Emmanuel Hocdet

Hi,

address #394

++
Manu



0001-BUG-MINOR-ssl-certificate-choice-can-be-unexpected-w.patch
Description: Binary data


[PATCH] BUG/MINOR: ssl: fix X509 compatibility for openssl < 1.1.0

2019-12-02 Thread Emmanuel Hocdet
Hi,

> Le 2 déc. 2019 à 08:12, William Lallemand  a écrit :
> 
> It seems to have break the build on centos 6, could you take a look at this
> ticket?
> 
> https://github.com/haproxy/haproxy/issues/385
> 
> 

Fix tested with openssl 1.0.1

++
Manu



0001-BUG-MINOR-ssl-fix-X509-compatibility-for-openssl-1.1.patch
Description: Binary data


PATCH] BUG/MINOR: ssl: fix SSL_CTX_set1_chain compatibility for openssl < 1.0.2

2019-11-29 Thread Emmanuel Hocdet

Hi,

A forgotten fix, comment updated.

++
Manu




0001-BUG-MINOR-ssl-fix-SSL_CTX_set1_chain-compatibility-f.patch
Description: Binary data




Re: PATCH: partially fix build if OpenSSL is built with no-deprecated option

2019-11-27 Thread Emmanuel Hocdet
Hi,

> Le 27 nov. 2019 à 03:46, Willy Tarreau  a écrit :
> 
>> @@ -5046,7 +5046,9 @@ int ssl_sock_prepare_ctx(struct bind_conf *bind_conf, 
>> struct ssl_bind_conf *ssl_
>>   NULL);
>> 
>>  if (ecdhe == NULL) {
>> +#if defined(SSL_CTX_set_ecdh_auto)
>>  (void)SSL_CTX_set_ecdh_auto(ctx, 1);
>> +#endif
>>  return cfgerr;
>>  }
>> #else
> 
> Here, in order to avoid the pollution caused by too many ifdefs, I'd
> instead put this into openssl-compat.h:
> 
> +#if !defined(SSL_CTX_set_ecdh_auto)
> +#define SSL_CTX_set_ecdh_auto(a,b) 0
> +#endif
> 
> As long as we can keep all such changes limited, we could imagine
> backporting them, that's great!
> 


To be suitable with  deprecated version of libssl* should be:
#define SSL_CTX_set_ecdh_auto(ctx, onoff)  ((onoff) != 0)
or
#define SSL_CTX_set_ecdh_auto(ctx, onoff) 1

++
Manu




Re: [PATCH] MINOR: ssl: deduplicate ca-file

2019-11-26 Thread Emmanuel Hocdet
Hi William,

> Le 22 nov. 2019 à 17:34, William Lallemand  a écrit :
> 
> Hi Manu,
> 
> I have a few questions/remarks below:
> 
>> Subject: [PATCH 1/3] MINOR: ssl: deduplicate ca-file
>> [...]
>> 
>> +static int ssl_store_load_locations_file(X509_STORE **store_ptr, char *path)
>> +{
>> +struct ebmb_node *eb;
>> +struct cafile_entry *ca_e;
>> +
>> +eb = ebst_lookup(_tree, path);
>> +if (eb)
>> +ca_e = ebmb_entry(eb, struct cafile_entry, node);
>> +else {
>> +X509_STORE *store = X509_STORE_new();
>> +if (X509_STORE_load_locations(store, path, NULL)) {
>> +int pathlen;
>> +pathlen = strlen(path);
>> +ca_e = calloc(1, sizeof(*ca_e) + pathlen + 1);
> 
> You probably want to check the return of calloc here.
> 
>> +memcpy(ca_e->path, path, pathlen + 1);
>> +ca_e->ca_store = store;
>> +ebst_insert(_tree, _e->node);
>> +} else {
>> +X509_STORE_free(store);
>> +return 0;
>> +}
>> +}
>> +*store_ptr = ca_e->ca_store;
>> +return 1;
>> +}
>> +
> 
>> Subject: [PATCH 2/3] MINOR: ssl: compute ca-list from deduplicate ca-file
>> [...]
>> 
>> +/* Duplicate ca_name is tracking with ebtree. It's simplify openssl 
>> compatibility */
>> +static STACK_OF(X509_NAME)* ssl_load_client_ca_file(char *path)
>> +{
>> [...]
>> +
>> +skn = sk_X509_NAME_new_null();
>> +/* take x509 from cafile_tree */
>> +objs = X509_STORE_get0_objects(ca_e->ca_store);
>> +for (i = 0; i < sk_X509_OBJECT_num(objs); i++) {
>> +x = X509_OBJECT_get0_X509(sk_X509_OBJECT_value(objs, 
>> i));
>> +if (!x)
>> +continue;
>> +xn = X509_get_subject_name(x);
>> +if (!xn)
>> +continue;
>> +/* Check for duplicates. */
>> +key = X509_NAME_hash(xn);
>> +for (node = eb64_lookup(_name_tree, key); node; node 
>> = eb64_next(node)) {
>> +ca_name = container_of(node, typeof(*ca_name), 
>> node);
>> +if (X509_NAME_cmp(xn, ca_name->xname) == 0)
>> +continue;
>> +}
> 
> I'm not sure this part is right. I think you wanted to skip pushing the object
> if it was already in the tree, but instead you are doing a continue in the
> inner loop.
> 

yep, introduce a ‘for' around a ‘continue’ to remplace sk_X509_NAME_find is not 
the best
thing to do… i will fix this.

> Also I'm not sure why we have to deduplicate the entries? Isn't it the job of
> openssl to do that?
> 
> 
>> +xn = X509_NAME_dup(xn);
>> +sk_X509_NAME_push(skn, xn);
>> +ca_name = calloc(1, sizeof *ca_name);
> 

Is what openssl do when used upper layer API. Openssl doesn’t refcount xname or 
dup xname in push().
Without dup xname, ca_e->ca_list  and  ca_e->ca_store will share xnames: should 
not in openssl word. 

> Same issue with calloc there.
> 
>> +ca_name->node.key = key;
>> +ca_name->xname = xn;
>> +eb64_insert(_name_tree, _name->node);
> 
ok i will adress allocs.

> 
> After digging into this part of the code, I think it's safer to split the 
> logic
> in two parts, like what has been done for the ckch.
> 
> The access to the file system should be done in the configuration parsing, so
> we don't access at all to the files in ssl_sock_prepare_ctx().
> 
> And we should only have a lookup in ssl_sock_prepare_ctx().
> 

Agree, this will be cleaner to integrate with existing code.

The initial purpose of the patch is to greatly speedup start and greatly reduce 
memory footprint when
ca-file is used in big configurations (can be unusable without).
Keep the initial patch easily to make backport should help.
What do you think?

++
Manu





Re: [PATCH] MINOR: ssl: deduplicate ca-file

2019-11-22 Thread Emmanuel Hocdet
Fix bad merge from my branch,

> Le 22 nov. 2019 à 11:35, Emmanuel Hocdet  a écrit :
> 
> 
> Patches update with compat lib-ssl and crl-file.
> Deduplicate Verify-stuff  in memory will prevent file access when updating a 
> certificate with CLI.



0001-MINOR-ssl-deduplicate-ca-file.patch
Description: Binary data


0002-MINOR-ssl-compute-ca-list-from-deduplicate-ca-file.patch
Description: Binary data


0003-MINOR-ssl-deduplicate-crl-file.patch
Description: Binary data




Re: [PATCH] MINOR: ssl: deduplicate ca-file

2019-11-22 Thread Emmanuel Hocdet
Hi,

> Le 29 oct. 2019 à 07:59, Willy Tarreau  a écrit :
> 
> Please, let's revisit this after the release. The only people able to
> have a look at this and to have an opinion on it are all busy finishing
> this release.
> 


Patches update with compat lib-ssl and crl-file.
Deduplicate Verify-stuff  in memory will prevent file access when updating a 
certificate with CLI.

++
Manu



0001-MINOR-ssl-deduplicate-ca-file.patch
Description: Binary data


0002-MINOR-ssl-compute-ca-list-from-deduplicate-ca-file.patch
Description: Binary data


0003-MINOR-ssl-deduplicate-crl-file.patch
Description: Binary data


[PATCH] BUG/MINOR: ssl: fix crt-list neg filter for openssl < 1.1.1

2019-11-06 Thread Emmanuel Hocdet
Hi,

Very difficult to trigger the bug, except with spécific test configuration like:
crt-list:
cert.pem !www.dom.tld
cert.pem *.dom.tld

If you can consider the patch.

Thank's
Manu




0001-BUG-MINOR-ssl-fix-crt-list-neg-filter-for-openssl-1..patch
Description: Binary data




[PATCH] BUG/MINOR: ssl: ssl_pkey_info_index ex_data can store a dereferenced pointer

2019-11-06 Thread Emmanuel Hocdet
Hi,

If you can consider the patch (related to CLI cert update)

Thank's
Manu



0001-BUG-MINOR-ssl-ssl_pkey_info_index-ex_data-can-store-.patch
Description: Binary data






Re: [PATCH] MINOR: ssl: deduplicate ca-file

2019-10-25 Thread Emmanuel Hocdet
Hi,

add a second patch to address ca-list case.

++
Manu

> Le 24 oct. 2019 à 12:14, Emmanuel Hocdet  a écrit :
> 
> Hi,
> 
> Little patch with big win when ca-file is used in server line.
> 
> ++
> Manu
> 
> <0001-MINOR-ssl-deduplicate-ca-file.patch>
> 


0001-MINOR-ssl-deduplicate-ca-file.patch
Description: Binary data


0002-MINOR-ssl-compute-ca-list-from-deduplicate-ca-file.patch
Description: Binary data




[PATCH] MINOR: ssl: deduplicate ca-file

2019-10-24 Thread Emmanuel Hocdet
Hi,

Little patch with big win when ca-file is used in server line.

++
Manu



0001-MINOR-ssl-deduplicate-ca-file.patch
Description: Binary data




Re: [PATCH] MINOR: connection: add "set-authority" and "authority" sample fetch

2019-09-27 Thread Emmanuel Hocdet


> Le 27 sept. 2019 à 12:23, Geoff Simmons  a écrit :
> 
> On 9/26/19 19:27, Emmanuel Hocdet wrote:
> 
>>> And I wonder if there are situations in which someone will want to
>>> specifically choose one source of truth for authority over the other.
>>> Suppose an incoming connection uses TLS with an SNI, and the peer
>>> component also sends an authority TLV via Proxy. Is a situation
>>> imaginable in which only one of them is getting it "right", for the
>>> purposes of haproxy, and the config author wants to be sure to catch
>>> that one only?
>> 
>> You can with the sample fetch from transport layer, « ssl_fc_sni » for TLS.
> 
> Then if I understand correctly:
> 
> - when you prefer the authority value from TLS, use the ssl_fc_sni fetch
> 

yes, or fix authority value with  tcp-request content set-authority ssl_fc_sni

> - if you prefer the value from the Proxy TLV, just use the authority
> fetch, since that one prefers the TLV over the value from TLS, according
> to the rules described above.
> 
> Is that right?
> 

yes

++
Manu




Re: [PATCH] MINOR: connection: add "set-authority" and "authority" sample fetch

2019-09-26 Thread Emmanuel Hocdet


> Le 26 sept. 2019 à 18:10, Geoff Simmons  a écrit :
> 
> On 9/26/19 11:43, Emmanuel Hocdet wrote:
>> 
>> Proposal reworking after playing with « authority » and look at how « src 
>> »/« dst » are working.
>> 
>> Authority » can come from transport layer (TLS), ProxyV2 TLV or « 
>> set-authority ».
>> « src/dst » is set from transport layer (TCP), overwrite by Proxy-protocol 
>> and « set-{src,dst} »
>> I propose to do the same for « authority » sample fetch:
>> pick « authority » from « set-authority, Proxy-protocol, and transport layer 
>> (in this order)
>> . It’s already what authority is in « proxy-v2-options authority"
>>  => « fc_pp_authority » disappears in favour of the generic « authority » 
>> sample fetch
> 
> Some thoughts that come to mind -- it sounds like there will be a bit of
> "magic" at work here, so will it be transparent to the user? Will users
> find that the authority field is being set and they wonder where it came
> from?
> 

I think we can. It will simplify the usage in the vast majority of cases.
Proxy-protocol is done to restore the initial context from a
connection. And should be used between trusted client/server.
typicaly:
client  --TLS (sni)--> haproxy  —TLS(with internal sni)--> haproxy 
--TLS(sni)--> backend

> And I wonder if there are situations in which someone will want to
> specifically choose one source of truth for authority over the other.
> Suppose an incoming connection uses TLS with an SNI, and the peer
> component also sends an authority TLV via Proxy. Is a situation
> imaginable in which only one of them is getting it "right", for the
> purposes of haproxy, and the config author wants to be sure to catch
> that one only?
> 

You can with the sample fetch from transport layer, « ssl_fc_sni » for TLS.

> To be honest I'm not sure, I'm still a bit of an "outsider" around here,
> and other readers of the list will have better intuitions about what's
> common and possible. So I'd be happy to be assured that this will be fine.
> 
I'm not sure me too :)
Thank’s for the report!

Manu




Re: [PATCH] MINOR: connection: add "set-authority" and "authority" sample fetch

2019-09-26 Thread Emmanuel Hocdet


Hi Tim,

> Le 26 sept. 2019 à 15:11, Tim Düsterhus  a écrit :
> 
> Manu,
> 
> Am 26.09.19 um 11:43 schrieb Emmanuel Hocdet:
>> Included my patch for that proposal. (could be split with comments from this 
>> mail)
> 
> Did you forgot to actually attach the patch? I'm not seeing anything.
> 


I see it on my side.




Re: [PATCH] MINOR: connection: add "set-authority" and "authority" sample fetch

2019-09-26 Thread Emmanuel Hocdet
Hi,Proposal reworking after playing with « authority » and look at how « src »/« dst » are working.Authority » can come from transport layer (TLS), ProxyV2 TLV or « set-authority ».« src/dst » is set from transport layer (TCP), overwrite by Proxy-protocol and « set-{src,dst} »I propose to do the same for « authority » sample fetch:pick « authority » from « set-authority, Proxy-protocol, and transport layer (in this order) . It’s already what authority is in « proxy-v2-options authority"  => « fc_pp_authority » disappears in favour of the generic « authority » sample fetchExample:listen offload       mode tcp       bind :80       bind :443 ssl crt-list /etc/haproxy/crtbindlist.cfg       server bla 127.0.0.1:8080 send-proxy-v2 proxy-v2-options authoritylisten onload       mode tcp       bind 127.0.0.1:8080 accept-proxy       acl has_authority authority -m found       tcp-request inspect-delay 5s       tcp-request content set-authority hdr(Host),lower if !has_authority       tcp-request content reject if !has_authority       server srvssl 0.0.0.0:443 ssl verify none sni authority  Note: in case of:   tcp-request connection set-authority str(authbla)   « authority » is set before  ProxyV2, and will be overwritten by TLV authority.Included my patch for that proposal. (could be split with comments from this mail)++Manu

0001-MINOR-connection-add-set-authority-and-normalize-aut.patch
Description: Binary data


[PATCH] BUG/MINOR: build: fix event ports (Solaris)

2019-09-19 Thread Emmanuel Hocdet

Hi,

Please consider this patch.
Thank’s

Manu



0001-BUG-MINOR-build-fix-event-ports-Solaris.patch
Description: Binary data


Re: [PATCH] MINOR: connection: add "set-authority" and "authority" sample fetch

2019-09-12 Thread Emmanuel Hocdet

patch update with bug fix

> Le 10 sept. 2019 à 14:19, Emmanuel Hocdet  a écrit :
> 
> 
> Hi,
> 
> Included, my first proposal for « set-authority » action, to set 
> custom "authority" sample  fetch.
> 
> Use case could be to use « sni authority » in server line.
> For "proxy-v2-options authority », authority is pick from custom
> authority (« set-authority »), ppv2 authority or ssl_fc_sni.
> Sample fetch « authority » could be do the same, but i don’t
> know if it’s a good idea.
> 
> ++
> Manu
> 
> 



0001-MINOR-connection-add-set-authority-and-authority-sam.patch
Description: Binary data



[PATCH] MINOR: connection: add "set-authority" and "authority" sample fetch

2019-09-10 Thread Emmanuel Hocdet

Hi,

Included, my first proposal for « set-authority » action, to set 
custom "authority" sample  fetch.

Use case could be to use « sni authority » in server line.
For "proxy-v2-options authority », authority is pick from custom
authority (« set-authority »), ppv2 authority or ssl_fc_sni.
Sample fetch « authority » could be do the same, but i don’t
know if it’s a good idea.

++
Manu




0001-MINOR-connection-add-set-authority-and-authority-sam.patch
Description: Binary data


Re: [PATCH] MINOR: send-proxy-v2: sends authority TLV according to TLV received

2019-09-02 Thread Emmanuel Hocdet


> Le 31 août 2019 à 12:29, Willy Tarreau  a écrit :
> 
> Hi Manu,
> 
> On Thu, Aug 29, 2019 at 03:22:11PM +0200, Emmanuel Hocdet wrote:
>> This patch follows Geoff's patch.
> 
> Thanks for this. I didn't remember we automatically copied the SNI
> into the PP. I'm suspecting that sooner or later we'll need a
> "set-authority" action to complete the set-dst and so-on. We'll see.
> 
> Now merged, thanks,
> Willy
> 
Thanks.

Yes, it’s the next step to set authority, but I wonder what is the right 
approach.
. simply, on server line: proxy-v2-authority  (like it is with sni)
  but I would prefer a more generic way.
. more generic (as you suggest): add a « set-authority » for srv_conn,
  and reused the new « proxy_authority » entry to store it.
 
With « set-authority », in usage we will need a sample fetch « authority » as
« bc_authority » ?
Use case could be « sni bc_authority ».
For PPv2 authority, we will automatically use, in the order: bc_authority, 
fc_pp_authority, ssl_fc_sni.

++
Manu




[PATCH] MINOR: send-proxy-v2: sends authority TLV according to TLV received

2019-08-29 Thread Emmanuel Hocdet

Hi,
This patch follows Geoff's patch.

++
Manu



0001-MINOR-send-proxy-v2-sends-authority-TLV-according-to.patch
Description: Binary data


Re: [PATCH] MINOR: Add the fc_pp_authority fetch -- authority TLV from PROXYv2

2019-08-27 Thread Emmanuel Hocdet
Hi Geoff,


For:
> 
> @@ -630,6 +631,17 @@ int conn_recv_proxy(struct connection *conn, int flag)
>   conn->proxy_netns = ns;
>   break;
>   }
> +
> + case PP2_TYPE_AUTHORITY: {

Is inside #ifdef USE_NS

It work for me, with the #ifdef fix.

Thanks
Manu



Re: [RFC] setting the backend SNI from the client's authority TLV, when the target address was forwarded

2019-08-27 Thread Emmanuel Hocdet


> Le 22 août 2019 à 14:40, Willy Tarreau  a écrit :
> 
> On Thu, Aug 22, 2019 at 11:36:00AM +0200, Geoff Simmons wrote:
> 
>> I suspect that there are other ways that the authority TLV can be useful
>> for haproxy besides the specific Varnish case. Someone connecting via
>> TLS, for example, might want to send the TLV to "override" the SNI for
>> certain backends.
> 
> It's definitely useful for other cases, which is why I'm interested in
> seeing it addressed properly. Right now we do emit a few fields in the
> PPv2 but we ignore them on receipt, which is a bit sad. For example, if
> you use multi-process to chain two haproxy instances, one with TLS
> offloading, passing to the next level using PPv2, you currently lose the
> SNI information. With your change it would finally work.
> 

Indeed, the receipt part of PPv2 has been postponed until needed.

With authority received from PPv2 (Geoff patch), i see use cases  (one is to 
test 
configuration like chaining tow haproxy :-) )
To generalize the usage of authority,  i will see a get_authority func in 
xprt_ops.
With a ssl connection: return the sni (aka ssl authority).
With a tcp connection: return the ppv2 authority.
So a « send-proxy-v2   proxy-v2-options authority » server line will work in 
both cases.

To override the authority, is like « sni » for server line, we could used « 
authority ».
«  send-proxy-v2   authority  hdr(Host) proxy-v2-options authority » 

For server line with ssl, i thought that « authority » could replace « sni » 
but we can have both:
«  ssl sni hdr(Host),lower  send-proxy-v2   authority  hdr(Host) 
proxy-v2-options authority »

Note:
in my test «  sni hdr(Host),lower » work but « sni var(req.host) » doesn’t  
(with http-request content set-var(req.host) hdr(host),lower )

What do you think?

Manu





Re: [RFC] setting the backend SNI from the client's authority TLV, when the target address was forwarded

2019-08-26 Thread Emmanuel Hocdet



HI  Geoff, Willy

Great to see TLS onloader continue.

> Le 22 août 2019 à 16:33, Geoff Simmons  a écrit :
> 
> On 8/22/19 14:40, Willy Tarreau wrote:
>> 
>>> I would suggest naming it something like fc_authority or
>>> fc_pp_authority, to be specific about where it came from.
> 
> Since you used fc_pp_authority in an example further down, I'll take
> that as the choice (unless somebody yells). Seems better to me, since
> just "authority" could refer to a number of things.
> 

fc_pp_authority seems ok.
(fc_)authority could refer to ssl_fc_sni for ssl connection or host header for 
http connection.


About the TLS onloader configuration. If i understand the principle of servers 
set to 0.0.0.0 and stick table:
The server configuration will look like:
   server s0 0.0.0.0:0 ssl sni fc_pp_authority
   […]
For stick part, to correctly reused TLS connection, destination IP + authority 
should be used.

Regards
Manu




[PATCH] BUG/MINOR: ssl: fix 0-RTT for BoringSSL

2019-08-07 Thread Emmanuel Hocdet
Hi,

Two patches to fix (and simplify) 0-RTT for BoringSSL.
If you can consider them.

++
Manu



0001-BUG-MINOR-ssl-fix-0-RTT-for-BoringSSL.patch
Description: Binary data


0002-MINOR-ssl-ssl_fc_has_early-should-work-for-BoringSSL.patch
Description: Binary data


Re: Recent BoringSSL build breakage

2019-08-01 Thread Emmanuel Hocdet
Hi Willy,Le 1 août 2019 à 10:07, Willy Tarreau  a écrit :Hi Manu,On Travis CI there was a fairly recent regression on BoringSSL whichhappened between 03e09f3 and a7a0f99 a day ago. It breaks on definitionof EVP_PKEY_base_id() in openssl-compat.h, which was not modified, andI guess this issue was hidden by the simultaneous breakage of libresslby latest changes.It looks like latest BoringSSL now defines this function and that thedefinition above could be removed. Could you please have a look whenyou have a moment and possibly propose a patch so that we can fix thosebuild reports (especially if you find that other places need to betouched) ?For reference, first breakage :    https://travis-ci.com/haproxy/haproxy/builds/121281529Last known good build:    https://travis-ci.com/haproxy/haproxy/builds/121258130Yep, i already built with the change.  Fix included.I'm looking at BORINGSSL_API_VERSION for compatibility evolution, but for nowis not incremented as i would expect.// BORINGSSL_API_VERSION is a positive integer that increments as BoringSSL// changes over time. The value itself is not meaningful. It will be incremented// whenever is convenient to coordinate an API change with consumers. This will// not denote any special point in development. A consumer may use this symbol in the preprocessor to temporarily build// against multiple revisions of BoringSSL at the same time. It is not// recommended to do so for longer than is necessary.#define BORINGSSL_API_VERSION 9++Manu

0001-BUILD-ssl-BoringSSL-add-EVP_PKEY_base_id.patch
Description: Binary data


Re: [PATCH] BUG/MINOR: ssl: (no) empty handshake detection for BoringSSL

2019-07-18 Thread Emmanuel Hocdet

Hi,

This patch is an update to follow the Lukas's one.

Only BoringSSL case is addressed, because i test it for BoringSSL.
It could be used by LibreSSL for "dontlognull" to work.

++
Manu




0001-BUG-MINOR-ssl-no-empty-handshake-detection-for-Borin.patch
Description: Binary data


Re: [RFC PATCH] BUG/MINOR: ssl: revert empty handshake detection in OpenSSL <= 1.0.2

2019-07-05 Thread Emmanuel Hocdet

> Le 4 juil. 2019 à 18:55, Илья Шипицин  a écrit :
> 
> can you provide some comment around code ?
> 
> I think almost nobody can read such code
> 
> чт, 4 июл. 2019 г. в 21:17, Emmanuel Hocdet  <mailto:m...@gandi.net>>:
> Hi,
> 
> This thread reminds me that with BoringSSL empty (and abort) handshake is not 
> set.
> After tests BoringSSL seems to have simpler case.
> I sent a patch to fix that.
> 
> For OpenSSL <= 1.0.2, revert is the thing to do.
> For LibreSSL, include it with BoringSSL case could be ok (with my patch).
> With time (no HB and better error report in libSSL), it seems code could 
> simply look like:
>   if (!errno)
>   conn->err_code = CO_ER_SSL_EMPTY;
>   else
>   conn->err_code = CO_ER_SSL_ABORT;
> 

Only CO_ER_SSL_EMPTY and CO_ER_SSL_ABORT  can be set for conn->err_code
(it’s the case for BoringSSL)


> ++
> Manu
> 
>> Le 4 juil. 2019 à 12:14, Lukas Tribus mailto:lu...@ltri.eu>> 
>> a écrit :
>> 
>> Hello Ilya,
>> 
>> 
>> On Mon, 1 Jul 2019 at 23:08, Илья Шипицин > <mailto:chipits...@gmail.com>> wrote:
>>> 
>>> 
>>> 
>>> вт, 2 июл. 2019 г. в 01:34, Willy Tarreau >> <mailto:w...@1wt.eu>>:
>>>> 
>>>> On Mon, Jul 01, 2019 at 10:32:29PM +0200, Lukas Tribus wrote:
>>>>> Commit 54832b97 ("BUILD: enable several LibreSSL hacks, including")
>>>>> changed empty handshake detection in OpenSSL <= 1.0.2 and LibreSSL,
>>>>> from accessing packet_length directly (not available in LibreSSL) to
>>>>> calling SSL_state() instead.
>>>> (...)
>>>> 
>>>> Thanks a lot Lukas. Just out of curiosity, do you have any idea of a
>>>> concrete user-visible issue this bug can cause ? It would help bisecting
>>>> issues later. I don't know in what case an empty handshake may happen.
>>> 
>>> 
>>> nmap scan ?
>> 
>> Ilya, just to avoid misunderstandings, I would like to have your
>> feedback on this patch so we can decide whether to commit it or work
>> on counter-proposals.
>> 
>> 
>> Thanks,
>> Lukas
> 



Re: [RFC PATCH] BUG/MINOR: ssl: revert empty handshake detection in OpenSSL <= 1.0.2

2019-07-04 Thread Emmanuel Hocdet
Hi,

This thread reminds me that with BoringSSL empty (and abort) handshake is not 
set.
After tests BoringSSL seems to have simpler case.
I sent a patch to fix that.

For OpenSSL <= 1.0.2, revert is the thing to do.
For LibreSSL, include it with BoringSSL case could be ok (with my patch).
With time (no HB and better error report in libSSL), it seems code could simply 
look like:
  if (!errno)
  conn->err_code = CO_ER_SSL_EMPTY;
  else
  conn->err_code = CO_ER_SSL_ABORT;

++
Manu

> Le 4 juil. 2019 à 12:14, Lukas Tribus  a écrit :
> 
> Hello Ilya,
> 
> 
> On Mon, 1 Jul 2019 at 23:08, Илья Шипицин  > wrote:
>> 
>> 
>> 
>> вт, 2 июл. 2019 г. в 01:34, Willy Tarreau :
>>> 
>>> On Mon, Jul 01, 2019 at 10:32:29PM +0200, Lukas Tribus wrote:
 Commit 54832b97 ("BUILD: enable several LibreSSL hacks, including")
 changed empty handshake detection in OpenSSL <= 1.0.2 and LibreSSL,
 from accessing packet_length directly (not available in LibreSSL) to
 calling SSL_state() instead.
>>> (...)
>>> 
>>> Thanks a lot Lukas. Just out of curiosity, do you have any idea of a
>>> concrete user-visible issue this bug can cause ? It would help bisecting
>>> issues later. I don't know in what case an empty handshake may happen.
>> 
>> 
>> nmap scan ?
> 
> Ilya, just to avoid misunderstandings, I would like to have your
> feedback on this patch so we can decide whether to commit it or work
> on counter-proposals.
> 
> 
> Thanks,
> Lukas



[PATCH] BUG/MINOR: ssl: empty handshake detection for BoringSSL

2019-07-04 Thread Emmanuel Hocdet
Hi,

This patch fix BoringSSL case.

++
Manu



0001-BUG-MINOR-ssl-empty-handshake-detection-for-BoringSS.patch
Description: Binary data




Re: [BUG] memory leak with treads and stick-table/peers

2019-07-01 Thread Emmanuel Hocdet
Hi,

no more leak after "BUG/MINOR: memory: Set objects size for pools in the 
per-thread cache" 

++
Manu


> Le 5 juin 2019 à 16:13, Emmanuel Hocdet  a écrit :
> 
> 
>> Le 5 juin 2019 à 16:07, Emmanuel Hocdet > <mailto:m...@gandi.net>> a écrit :
>> 
>> Hi Frederic
>> 
>>> Le 5 juin 2019 à 15:44, Frederic Lecaille >> <mailto:flecai...@haproxy.com>> a écrit :
>>> 
>>> On 6/5/19 3:06 PM, Emmanuel Hocdet wrote:
>>>> Hi,
>>> 
>>> Hi Emmanuel,
>>> 
>>>> After switched to haproxy 1.9 with threads activated, i noticed a 
>>>> significant memory leak.
>>> 
>>> Is valgrind able to expose this memory leak?
>>> 
>>>> With threads disable (and bind process omitted) leak disappear.
>>>> This seems to be related to stick-table/peers with regard to the 
>>>> (simplified) configuration.
>>> 
>>> Has this been revealed by a git bisect?
>>> 
>> 
>> Report is from production, i need to reproduce this on dev platform to debug 
>> more.
>> 
> 
> As a reminder, the leak was detected after thread enable, on 1.9.5.
> 



Re: [BUG] memory leak with treads and stick-table/peers

2019-06-05 Thread Emmanuel Hocdet

> Le 5 juin 2019 à 16:07, Emmanuel Hocdet  a écrit :
> 
> Hi Frederic
> 
>> Le 5 juin 2019 à 15:44, Frederic Lecaille > <mailto:flecai...@haproxy.com>> a écrit :
>> 
>> On 6/5/19 3:06 PM, Emmanuel Hocdet wrote:
>>> Hi,
>> 
>> Hi Emmanuel,
>> 
>>> After switched to haproxy 1.9 with threads activated, i noticed a 
>>> significant memory leak.
>> 
>> Is valgrind able to expose this memory leak?
>> 
>>> With threads disable (and bind process omitted) leak disappear.
>>> This seems to be related to stick-table/peers with regard to the 
>>> (simplified) configuration.
>> 
>> Has this been revealed by a git bisect?
>> 
> 
> Report is from production, i need to reproduce this on dev platform to debug 
> more.
> 

As a reminder, the leak was detected after thread enable, on 1.9.5.



Re: [BUG] memory leak with treads and stick-table/peers

2019-06-05 Thread Emmanuel Hocdet
Hi Frederic

> Le 5 juin 2019 à 15:44, Frederic Lecaille  a écrit :
> 
> On 6/5/19 3:06 PM, Emmanuel Hocdet wrote:
>> Hi,
> 
> Hi Emmanuel,
> 
>> After switched to haproxy 1.9 with threads activated, i noticed a 
>> significant memory leak.
> 
> Is valgrind able to expose this memory leak?
> 
>> With threads disable (and bind process omitted) leak disappear.
>> This seems to be related to stick-table/peers with regard to the 
>> (simplified) configuration.
> 
> Has this been revealed by a git bisect?
> 

Report is from production, i need to reproduce this on dev platform to debug 
more.

> I am trying to understand the impact of the peers configuration 
> simplification on your configuration because as far as I see you do not use 
> this feature.
> 
> This feature is used only if you have "table" lines in "peers" sections.
> 
> Furthermore this feature has not been backported to 1.9. This is a 2.0 
> specific feature. Or perhaps I have missed something.
> 
> This feature comes with at least this commit:
> 
>MEDIUM: stick-table: Stop handling stick-tables as proxies.
> 

It’s legacy stick-table/peers configuration:

backend ssl-back
...
stick-table type binary len 32 size 1m expire 5m peers front-peers

++
Manu



[BUG] memory leak with treads and stick-table/peers

2019-06-05 Thread Emmanuel Hocdet

Hi,

After switched to haproxy 1.9 with threads activated, i noticed a significant 
memory leak.
With threads disable (and bind process omitted) leak disappear.

This seems to be related to stick-table/peers with regard to the (simplified) 
configuration.

++
Manu


ENV:
HA-Proxy version 1.9.8-1 2019/05/15 - https://haproxy.org/
Build options :
  TARGET  = linux2628
  CPU = generic
  CC  = gcc
  CFLAGS  = -g -O2 -fPIE -fstack-protector-strong -Wformat 
-Werror=format-security -D_FORTIFY_SOURCE=2
  OPTIONS = USE_ZLIB=1 USE_REGPARM=1 USE_OPENSSL=1 USE_SYSTEMD=1 USE_PCRE=1 
USE_NS=1

Default settings :
  maxconn = 2000, bufsize = 16384, maxrewrite = 1024, maxpollevents = 200

Built with OpenSSL version : BoringSSL 7f4f41fa
OpenSSL library supports TLS extensions : yes
OpenSSL library supports SNI : yes
OpenSSL library supports : TLSv1.0 TLSv1.1 TLSv1.2 TLSv1.3
Built with network namespace support.
Built with transparent proxy support using: IP_TRANSPARENT IPV6_TRANSPARENT 
IP_FREEBIND
Built with zlib version : 1.2.8
Running on zlib version : 1.2.8
Compression algorithms supported : identity("identity"), deflate("deflate"), 
raw-deflate("deflate"), gzip("gzip")
Built with PCRE version : 8.35 2014-04-04
Running on PCRE version : 8.35 2014-04-04
PCRE library supports JIT : no (USE_PCRE_JIT not set)
Encrypted password support via crypt(3): yes
Built with multi-threading support.

Available polling systems :
  epoll : pref=300,  test result OK
   poll : pref=200,  test result OK
 select : pref=150,  test result OK
Total: 3 (3 usable), will use epoll.

Available multiplexer protocols :
(protocols marked as  cannot be specified using 'proto' keyword)
  h2 : mode=HTXside=FE|BE
  h2 : mode=HTTP   side=FE
: mode=HTXside=FE|BE
: mode=TCP|HTTP   side=FE|BE

Available filters :
[SPOE] spoe
[COMP] compression
[CACHE] cache
[TRACE] trace

minimal configuration to reproduce the leak:
global
user haproxy
group haproxy
daemon

# ssl(odd) http(even)
nbthread 2

stats socket /var/run/haproxy_L6.sock mode 660 level admin expose-fd 
listeners

log /dev/log daemon warning

# hard limit
hard-stop-after 2h
maxconn 262144

defaults
log global
log-tag "haproxy_L6"
option dontlognull
mode tcp
maxconn 10
timeout connect 500ms


timeout client 610s
retries 3
timeout server 610s

peers front-peers

peer L6_1 x.y.z.1:943

peer L6_2 x.y.z.2:943

peer L6_3 x.y.z.3:943


frontend ssl-front

bind :443  process 1/odd
bind :::443 v6only process 1/odd


tcp-request inspect-delay 5s
tcp-request content reject if !{ req_ssl_sni -m found }
default_backend ssl-back

backend ssl-back
balance roundrobin
option ssl-hello-chk

stick-table type binary len 32 size 1m expire 5m peers front-peers

acl clienthello req_ssl_hello_type 1
acl serverhello rep_ssl_hello_type 2

tcp-request inspect-delay 5s
tcp-request content accept if clienthello

tcp-response content accept if serverhello

stick on payload_lv(43,1) if clienthello
stick store-response payload_lv(43,1) if serverhello


server ssl_1 a.b.c.1:463 check send-proxy 

server ssl_2 a.b.c.2:463 check send-proxy 

server ssl_3 a.b.c.3:463 check send-proxy 

frontend http-front
bind :80  process 1/even
bind :::80 v6only process 1/even


default_backend http-back


backend http-back
balance roundrobin


server L7_1 a.b.c.1:480 check send-proxy 

server L7_2 a.b.c.1:480 check send-proxy 

server L7_3 a.b.c.3:480 check send-proxy 



[PATCH] CLEANUP: ssl: remove unneeded defined(OPENSSL_IS_BORINGSSL)

2019-06-04 Thread Emmanuel Hocdet
Hi,

Simple cleanup to limit #defined inflation.

++
Manu



0001-CLEANUP-ssl-remove-unneeded-defined-OPENSSL_IS_BORIN.patch
Description: Binary data


Re: [PATCH] ssl certificates load speedup and dedup (pem/ctx)

2019-04-10 Thread Emmanuel Hocdet

Hi,

Updated patch serie:
Fix OpenSSL < 1.0.2 compatibilty.
More generic key for issuers ebtree.

++
Manu




0001-REORG-ssl-promote-cert_key_and_chain-handling.patch
Description: Binary data


0002-MINOR-ssl-use-STACK_OF-for-chain-certs.patch
Description: Binary data


0003-MINOR-ssl-add-extra-chain-compatibility.patch
Description: Binary data


0004-MINOR-ssl-used-cert_key_and_chain-func-in-load_cert_.patch
Description: Binary data


0005-BUG-MINOR-ssl-fix-ssl_sock_load_multi_cert-init-vars.patch
Description: Binary data


0006-CLEANUP-ssl-ssl_sock_load_crt_file_into_ckch.patch
Description: Binary data


0007-MINOR-ssl-add-issuer-path-directive.patch
Description: Binary data


0008-MINOR-ssl-dedup-cert-chain-for-bind-ctx-boringssl.patch
Description: Binary data






Re: Abort on exit "libgcc_s.so.1 must be installed for pthread_cancel to work"

2019-04-10 Thread Emmanuel Hocdet


> Le 5 avr. 2019 à 13:05, William Lallemand  a écrit :
> 
> On Fri, Apr 05, 2019 at 12:55:11PM +0200, Emmanuel Hocdet wrote:
>> 
>> Hi,
>> 
>> To test deinit, i come across this:
>> 
>> #  /srv/sources/haproxy/haproxy -f /etc/haproxy/ssl.cfg -d -x 
>> /run/haproxy_ssl.sock -sf 15716
>> 
>> log on 15716 process:
>> Available polling systems :
>>  epoll : pref=300,  test result OK
>>   poll : pref=200,  test result OK
>> select : pref=150,  test result FAILED
>> Total: 3 (2 usable), will use epoll.
>> 
>> Available filters :
>>  [SPOE] spoe
>>  [COMP] compression
>>  [CACHE] cache
>>  [TRACE] trace
>> Using epoll() as the polling mechanism.
>> :GLOBAL.accept(0005)=000d from [unix:1] ALPN=
>> :GLOBAL.srvcls[adfd:]
>> :GLOBAL.clicls[adfd:]
>> :GLOBAL.closed[adfd:]
>> [WARNING] 094/124050 (15809) : Stopping frontend GLOBAL in 0 ms.
>> [WARNING] 094/124050 (15809) : Stopping frontend f-redir in 0 ms.
>> [WARNING] 094/124050 (15809) : Stopping backend redir in 0 ms.
>> [WARNING] 094/124050 (15809) : Stopping backend varnish in 0 ms.
>> [WARNING] 094/124050 (15809) : Proxy GLOBAL stopped (FE: 1 conns, BE: 1 
>> conns).
>> [WARNING] 094/124050 (15809) : Proxy f-redir stopped (FE: 0 conns, BE: 0 
>> conns).
>> [WARNING] 094/124050 (15809) : Proxy redir stopped (FE: 0 conns, BE: 0 
>> conns).
>> [WARNING] 094/124051 (15809) : Proxy varnish stopped (FE: 0 conns, BE: 0 
>> conns).
>> libgcc_s.so.1 must be installed for pthread_cancel to work
>> Aborted
>> 
>> Link with -lgcc_s fix that, and haproxy return with error code 0.
>> I think it will be not very portable…
>> 
>> ++
>> Manu
>> 
>> 
> 
> Hi Emmanuel,
> 
> This bug is caused by libpthread which is not linked with libgcc_s, so it 
> tries
> to load libgcc_s during the call to phtread_exit() but that can't work if the
> process is chroot'ed.
> 
> Your solution is the good one, but it was not working on some distributions, I
> add to do ADDLIB="-Wl,--no-as-needed -lgcc_s -Wl,--as-needed" instead.
> 
> Regards,
> 
> -- 
> William Lallemand

Hi William,

Thanks for clarifying. It’s annoying…
And --no-as-needed also depend on gcc tools version…

++
Manu





[PATCH] MINOR: ssl: Activate aes_gcm_dec converter for BoringSSL

2019-04-10 Thread Emmanuel Hocdet

Hi,

If you can consider this patch.
BoringSSL actually mimic OpenSSL 1.1.0 and have OPENSSL_VERSION_NUMBER set 
accordly.

++
Manu



0001-MINOR-ssl-Activate-aes_gcm_dec-converter-for-BoringS.patch
Description: Binary data


Re: [ANNOUNCE] haproxy-1.9.6

2019-04-09 Thread Emmanuel Hocdet

> Le 9 avr. 2019 à 09:58, Aleksandar Lazic  a écrit :
> 
> Hi Manu.
> 
> Am 05.04.2019 um 12:36 schrieb Emmanuel Hocdet:
>> Hi Aleks,
>> 
>> Thanks you to have integrate BoringSSL!
>> 
>>> Le 29 mars 2019 à 14:51, Aleksandar Lazic >> <mailto:al-hapr...@none.at>
>>> <mailto:al-hapr...@none.at <mailto:al-hapr...@none.at>>> a écrit :
>>> 
>>> Am 29.03.2019 um 14:25 schrieb Willy Tarreau:
>>>> Hi Aleks,
>>>> 
>>>> On Fri, Mar 29, 2019 at 02:09:28PM +0100, Aleksandar Lazic wrote:
>>>>> With openssl are 2 tests failed but I'm not sure because of the setup or 
>>>>> a bug.
>>>>> https://gitlab.com/aleks001/haproxy19-centos/-/jobs/186769272
>>>> 
>>>> Thank you for the quick feedback. I remember about the first one being
>>>> caused by a mismatch in the exact computed response size due to headers
>>>> encoding causing some very faint variations, though I have no idea why
>>>> I don't see it here, since I should as well, I'll have to check my regtest
>>>> script. For the second one, it looks related to the reactivation of the
>>>> HEAD method in this test which was broken in previous vtest. But I'm
>>>> seeing in your trace that you're taking it from the git repo so that
>>>> can't be that. I need to dig as well.
>>>> 
>>>>> With boringssl are 3 tests failed but I'm not sure because of the setup 
>>>>> or a
>>>>> bug.
>>>>> https://gitlab.com/aleks001/haproxy-19-boringssl/-/jobs/186780822
>>>> 
>>>> For this one I don't know, curl reports some unexpected EOFs. I don't
>>>> see why it would fail only with boringssl. Did it use to work in the
>>>> past ?
>>> 
>>> No. The tests with Boringssl always failed in one or another way.
>>> 
>> 
>> It’s strange. After quick test, it works in my environnements.
>> I need to comment "${no-htx} option http-use-htx »
>> to test with varnishtest.
> 
> I use `make reg-tests -- --use-htx` to test with htx.
> 
> https://gitlab.com/aleks001/haproxy-19-boringssl/blob/master/Dockerfile#L65 
> <https://gitlab.com/aleks001/haproxy-19-boringssl/blob/master/Dockerfile#L65>
> 
> Do you tested with or without htx?
> 

I use varnishtest, i will switch to vtest.
I do again the test with and without htx, with 1.9 and 2.0dev, and it’s ok.

>>> https://gitlab.com/aleks001/haproxy-19-boringssl/-/jobs/157743825 
>>> <https://gitlab.com/aleks001/haproxy-19-boringssl/-/jobs/157743825>
>>> https://gitlab.com/aleks001/haproxy-19-boringssl/-/jobs/157730793 
>>> <https://gitlab.com/aleks001/haproxy-19-boringssl/-/jobs/157730793>
>>> 
>>> I'm not sure if the docker setup on gitlab is the limitation or just a bug.
>>> Sorry to be so unspecific.
>>> 
>>>> Thanks,
>>>> Willy
>>> 
>>> Regards
>>> Aleks
>> 
>> ++
>> Manu
> 
> Best regards
> Aleks



Abort on exit "libgcc_s.so.1 must be installed for pthread_cancel to work"

2019-04-05 Thread Emmanuel Hocdet

Hi,

To test deinit, i come across this:

#  /srv/sources/haproxy/haproxy -f /etc/haproxy/ssl.cfg -d -x 
/run/haproxy_ssl.sock -sf 15716

log on 15716 process:
Available polling systems :
  epoll : pref=300,  test result OK
   poll : pref=200,  test result OK
 select : pref=150,  test result FAILED
Total: 3 (2 usable), will use epoll.

Available filters :
[SPOE] spoe
[COMP] compression
[CACHE] cache
[TRACE] trace
Using epoll() as the polling mechanism.
:GLOBAL.accept(0005)=000d from [unix:1] ALPN=
:GLOBAL.srvcls[adfd:]
:GLOBAL.clicls[adfd:]
:GLOBAL.closed[adfd:]
[WARNING] 094/124050 (15809) : Stopping frontend GLOBAL in 0 ms.
[WARNING] 094/124050 (15809) : Stopping frontend f-redir in 0 ms.
[WARNING] 094/124050 (15809) : Stopping backend redir in 0 ms.
[WARNING] 094/124050 (15809) : Stopping backend varnish in 0 ms.
[WARNING] 094/124050 (15809) : Proxy GLOBAL stopped (FE: 1 conns, BE: 1 conns).
[WARNING] 094/124050 (15809) : Proxy f-redir stopped (FE: 0 conns, BE: 0 conns).
[WARNING] 094/124050 (15809) : Proxy redir stopped (FE: 0 conns, BE: 0 conns).
[WARNING] 094/124051 (15809) : Proxy varnish stopped (FE: 0 conns, BE: 0 conns).
libgcc_s.so.1 must be installed for pthread_cancel to work
Aborted

Link with -lgcc_s fix that, and haproxy return with error code 0.
I think it will be not very portable…

++
Manu




Re: [ANNOUNCE] haproxy-1.9.6

2019-04-05 Thread Emmanuel Hocdet
Hi Aleks,

Thanks you to have integrate BoringSSL!

> Le 29 mars 2019 à 14:51, Aleksandar Lazic  a écrit :
> 
> Am 29.03.2019 um 14:25 schrieb Willy Tarreau:
>> Hi Aleks,
>> 
>> On Fri, Mar 29, 2019 at 02:09:28PM +0100, Aleksandar Lazic wrote:
>>> With openssl are 2 tests failed but I'm not sure because of the setup or a 
>>> bug.
>>> https://gitlab.com/aleks001/haproxy19-centos/-/jobs/186769272
>> 
>> Thank you for the quick feedback. I remember about the first one being
>> caused by a mismatch in the exact computed response size due to headers
>> encoding causing some very faint variations, though I have no idea why
>> I don't see it here, since I should as well, I'll have to check my regtest
>> script. For the second one, it looks related to the reactivation of the
>> HEAD method in this test which was broken in previous vtest. But I'm
>> seeing in your trace that you're taking it from the git repo so that
>> can't be that. I need to dig as well.
>> 
>>> With boringssl are 3 tests failed but I'm not sure because of the setup or 
>>> a bug.
>>> https://gitlab.com/aleks001/haproxy-19-boringssl/-/jobs/186780822
>> 
>> For this one I don't know, curl reports some unexpected EOFs. I don't
>> see why it would fail only with boringssl. Did it use to work in the
>> past ?
> 
> No. The tests with Boringssl always failed in one or another way.
> 

It’s strange. After quick test, it works in my environnements.
I need to comment "${no-htx} option http-use-htx »
to test with varnishtest.


> https://gitlab.com/aleks001/haproxy-19-boringssl/-/jobs/157743825 
> 
> https://gitlab.com/aleks001/haproxy-19-boringssl/-/jobs/157730793 
> 
> 
> I'm not sure if the docker setup on gitlab is the limitation or just a bug.
> Sorry to be so unspecific.
> 
>> Thanks,
>> Willy
> 
> Regards
> Aleks

++
Manu




Re: HAProxy with OpenSSL 1.1.1 breaks when TLS 1.3 KeyUpdate is used.

2019-01-22 Thread Emmanuel Hocdet


> Le 21 janv. 2019 à 19:31, Adam Langley  a écrit :
> 
> On Mon, Jan 21, 2019 at 10:16 AM Dirkjan Bussink  wrote:
>> Ah ok, I recently added support in HAProxy to handle the new 
>> SSL_CTX_set_ciphersuites option since OpenSSL handles setting TLS 1.3 
>> ciphers separate from the regular ones. Are those things that BoringSSL 
>> would also want to adopt then?
> 
> SSL_CTX_set_ciphersuites is more than a compatibility hack like adding
> a dummy #define, and the considerations are more complex. I'm not sure
> that we want to allow TLS 1.3 ciphersuite to be configured: the
> excessive number of cipher suites prior to TLS 1.3 was a problem, as
> was the excessive diversity of configurations. Also, string-based APIs
> have historically been expensive because they prevent easy static
> analysis. So we could add a dummy SSL_CTX_set_ciphersuites that always
> returns zero, but most applications would probably take that to be a
> fatal error so that wouldn't be helpful. So SSL_CTX_set_ciphersuites
> might be a case where a #ifdef is the best answer. But we'll always
> think about such things if asked.
> 

I agree, no need for SSL_CTX_set_ciphersuites. If a security issue appear on
cipher i suppose BoringSSL will evolve with a default fix.

> (If you happen to know, I would be curious who is using BoringSSL with 
> HAProxy.)
> 
We used BoringSSL in production since 1.5 year.

++
Manu




Re: HAProxy with OpenSSL 1.1.1 breaks when TLS 1.3 KeyUpdate is used.

2019-01-22 Thread Emmanuel Hocdet


> Le 21 janv. 2019 à 19:07, Dirkjan Bussink  a écrit :
> 
> Hi Manu,
> 
>> On 21 Jan 2019, at 09:49, Emmanuel Hocdet  wrote:
>> 
>> Boringssl does not have SSL_OP_NO_RENEGOTIATION and need KeyUpdate to work.
>> As workaround, SSL_OP_NO_RENEGOTIATION could be set to 0 in openssl-compat.h.
> 
> Hmm, then we will need a different #define though since we can’t rely own the 
> constant not being defined in that case to disable the logic. We would need a 
> separate way to detect this then. Is there a good example of this or should I 
> change the logic then to version checks instead? And how about LibreSSL in 
> that case?
> 
> Does BoringSSL need any of the logic in the first place? There’s not really 
> versions of it, so is the target there always current master or something 
> else? 
> 


No need to change, SSL_OP_NO_RENEGOTIATION is now in Boringssl, thanks Adam, 
and renegotiation is disabled by default.
For LibreSSL, no TLSv1.3, no SSL_OP_NO_RENEGOTIATION.

++
Manu






Re: HAProxy with OpenSSL 1.1.1 breaks when TLS 1.3 KeyUpdate is used.

2019-01-21 Thread Emmanuel Hocdet
Hi,

> Le 21 janv. 2019 à 17:06, Emeric Brun  a écrit :
> 
> Interesting, it would be good to skip the check using the same method.
> 
> We must stay careful to not put the OP_NO_RENEG flag on the client part (when 
> haproxy connects to server), because reneg from server is authorized
> but i think infocbk is called only on frontend/accept side.
> 
> so a patch which do:
> 
> #ifdef  SSL_OP_NO_RENEGOTIATION
> SSL_set_options(ctx, SSL_OP_NO_RENEGOTIATION);
> #endif
> 
> without condition during init
> 
> and adding #ifndef SSL_OP_NO_RENEGOTIATION arround the CVE check, should fix 
> the issue mentionned about keyupdate and will fix the CVE using the clean way 
> if the version
> of openssl support.
> 

Boringssl does not have SSL_OP_NO_RENEGOTIATION and need KeyUpdate to work.
As workaround, SSL_OP_NO_RENEGOTIATION could be set to 0 in openssl-compat.h.

++
Manu







Re: haproxy reload terminated with master/worker

2019-01-08 Thread Emmanuel Hocdet


> Le 8 janv. 2019 à 15:02, William Lallemand  a écrit :
> 
> On Tue, Jan 08, 2019 at 02:03:22PM +0100, Tim Düsterhus wrote:
>> Emmanuel,
>> 
>> Am 08.01.19 um 13:53 schrieb Emmanuel Hocdet:
>>> Without master/worker, haproxy reload work with an active waiting (haproxy 
>>> exec).
>>> With master/worker, kill -USR2 return immediately: Is there a way to know 
>>> when the reload is finished?
>>> 
>> 
>> Are you using systemd with -Ws? haproxy informs systemd when a reload
>> starts and when it is finished using the sd_notify protocol:
>> 
>> https://www.freedesktop.org/software/systemd/man/sd_notify.html#RELOADING=1
>> 
>> Best regards
>> Tim Düsterhus
>> 
> 
> Hi guys,
> 
> Indeed you can't do that without systemd, haproxy send a READY=1 variable with
> sd_notify once the processes are ready.
> 
> Unfortunately sd_notify has some limitation, there is no sd_notify variable
> which permits to notify that a reload failed, but you could read the errors in
> the service logs.
> 
> I have some plans for the master worker in the future, to have a synchronous
> state of the reload on the CLI, but this is not possible with the current
> architecture. We will probably need to stop reexecuting the process to achieve
> that and do a reload over the master CLI with a return code.
> 
> -- 
> William Lallemand

Thanks for quick responses.
I do not use systemd and even, it would be a shame to depend on systemd for 
that.

Yes return code for reload failed is one of the use case to reload active 
waiting.
(read the errors in the service logs is not really scripts friendly).

Reload over the master CLI with return code seem to be a good solution.
Waiting for the feature ;-)

++
Manu





haproxy reload terminated with master/worker

2019-01-08 Thread Emmanuel Hocdet
Hi,

Without master/worker, haproxy reload work with an active waiting (haproxy 
exec).
With master/worker, kill -USR2 return immediately: Is there a way to know when 
the reload is finished?

++
Manu




Re: [PATCH] ssl certificates load speedup and dedup (pem/ctx)

2019-01-08 Thread Emmanuel Hocdet


Hi Emeric,

> Le 7 janv. 2019 à 18:11, Emeric Brun  a écrit :
> 
> Hi Manu,
> 
> On 1/7/19 5:59 PM, Emmanuel Hocdet wrote:
>> It's better with patches…
>> 
>>> Le 7 janv. 2019 à 17:57, Emmanuel Hocdet >> <mailto:m...@gandi.net>> a écrit :
>>> 
>>> Hi,
>>> 
>>> Following the first patch series (included).
>>> The goal is to deduplicate common certificates in memory and in shared pem 
>>> files.
>>> 
>>> PATCH 7/8 is only for boringssl (directive to dedup certificate in memory 
>>> for ctx)
>>> Last patch should be the more interesting:
>>> [PATCH 8/8] MINOR: ssl: add "issuer-path" directive.
>>> 
>>> Certificates loaded with "crt" and "crt-list" commonly share the same
>>> intermediate certificate in PEM file. "issuer-path" is a global
>>> directive to share intermediate certificate in a directory. If
>>> certificate chain is not included in certificate PEM file, haproxy
>>> will complete chain if issuer match the first certificate of the chain
>>> stored via "issuer-path" directive. Such chains will be shared in ssl
>>> shared memory.
>>> . "issuer-path" directive can be set several times.
>>> . only sha1 key identifier is supported (rfc5280 4.2.1.2. (1))
>>> 
>>> If you want to test it, the patch series can be apply to haproxy-dev or 
>>> haproxy-1.9.
>>> 
>>> Feedbacks are welcome :)
>>> 
>>> ++
>>> Manu
>>> 
>> 
>> 
> 
> We have to double check this patches proposal because we have a pending 
> feature in roadmap which could heavily collide: to load only one time a 
> certificate per fs entry.
> 
> For us it is a mandatory feature to allow a clean "hot" update of 
> certificates. (the key to identify a certificate to update will be the path 
> on the fs, or at least, the base path)
> 
> Emeric

Interesting, i have some questions about this feature, i will wait. With fs 
entry only it can conflict with crt-list.

I think it should not heavily collide. Some conflicts with code refactoring for 
sure, issuer sharing feature should be complementary.

++
Manu






Re: [PATCH] ssl certificates load speedup and dedup (pem/ctx)

2019-01-07 Thread Emmanuel Hocdet
It's better with patches…Le 7 janv. 2019 à 17:57, Emmanuel Hocdet <m...@gandi.net> a écrit :Hi,Following the first patch series (included).The goal is to deduplicate common certificates in memory and in shared pem files.PATCH 7/8 is only for boringssl (directive to dedup certificate in memory for ctx)Last patch should be the more interesting:[PATCH 8/8] MINOR: ssl: add "issuer-path" directive.Certificates loaded with "crt" and "crt-list" commonly share the sameintermediate certificate in PEM file. "issuer-path" is a globaldirective to share intermediate certificate in a directory. Ifcertificate chain is not included in certificate PEM file, haproxywill complete chain if issuer match the first certificate of the chainstored via "issuer-path" directive. Such chains will be shared in sslshared memory.. "issuer-path" directive can be set several times.. only sha1 key identifier is supported (rfc5280 4.2.1.2. (1))If you want to test it, the patch series can be apply to haproxy-dev or haproxy-1.9.Feedbacks are welcome :)++Manu

0001-REORG-ssl-promote-cert_key_and_chain-handling.patch
Description: Binary data


0002-MINOR-ssl-use-STACK_OF-for-chain-certs.patch
Description: Binary data


0003-MINOR-ssl-SSL_CTX_set1_chain-compatibility.patch
Description: Binary data


0004-MINOR-ssl-used-cert_key_and_chain-func-in-load_cert_.patch
Description: Binary data


0005-BUG-MINOR-ssl-fix-ssl_sock_load_multi_cert-init-vars.patch
Description: Binary data


0006-CLEANUP-ssl-ssl_sock_load_crt_file_into_ckch.patch
Description: Binary data


0007-MINOR-ssl-dedup-cert-chain-for-bind-ctx-boringssl.patch
Description: Binary data


0008-MINOR-ssl-add-issuer-path-directive.patch
Description: Binary data


[PATCH] ssl certificates load speedup and dedup (pem/ctx)

2019-01-07 Thread Emmanuel Hocdet
Hi,

Following the first patch series (included).
The goal is to deduplicate common certificates in memory and in shared pem 
files.

PATCH 7/8 is only for boringssl (directive to dedup certificate in memory for 
ctx)
Last patch should be the more interesting:
[PATCH 8/8] MINOR: ssl: add "issuer-path" directive.

Certificates loaded with "crt" and "crt-list" commonly share the same
intermediate certificate in PEM file. "issuer-path" is a global
directive to share intermediate certificate in a directory. If
certificate chain is not included in certificate PEM file, haproxy
will complete chain if issuer match the first certificate of the chain
stored via "issuer-path" directive. Such chains will be shared in ssl
shared memory.
. "issuer-path" directive can be set several times.
. only sha1 key identifier is supported (rfc5280 4.2.1.2. (1))

If you want to test it, the patch series can be apply to haproxy-dev or 
haproxy-1.9.

Feedbacks are welcome :)

++
Manu

> Le 12 déc. 2018 à 12:23, Emmanuel Hocdet  a écrit :
> 
> 
> Hi,
> 
> I tried to improve the haproxy loading time with a lot of certificates, and 
> see a double file
> open for each certificate (one for private-key and one for the cert/chain).
> Multi-cert loading part have not this issue and is good candidate for sharing 
> code:
> patches is this work with factoring/cleanup/fix.
> 
> About speed: PEM file with private key in first position is far better.
> 
> If you can consider this patches?
> 



Re: [PATCH] ssl: factoring load cert/key and chains

2018-12-12 Thread Emmanuel Hocdet
Hi Julien,

> Le 12 déc. 2018 à 14:28, Julien Laffaye  a écrit :
> 
> 
> On Wed, Dec 12, 2018 at 12:24 PM Emmanuel Hocdet  <mailto:m...@gandi.net>> wrote:
> 
> Hi,
> 
> I tried to improve the haproxy loading time with a lot of certificates, and 
> see a double file
> open for each certificate (one for private-key and one for the cert/chain).
> Multi-cert loading part have not this issue and is good candidate for sharing 
> code:
> patches is this work with factoring/cleanup/fix.
> 
> About speed: PEM file with private key in first position is far better.
> 
> If you can consider this patches?
> 
> ++
> Manu
> 
> 
> Hello,
> 
> I'm curious, do you have numbers concerning the open(2) optimization ?

Not much (a few seconds), because load cert and especially load key is the 
heaviest part,
but avoid syscall for reload  it's rather good when kernel has something else 
to do.

> And also the PEM ordering, I did not know it mattered.
> 
5 to 10% speedup in my tests.

I tested with pkcs12 format (with limited encryption) but it’s slower than pem.
The best gain is with EC certificates (x2 vs RSA-2048) (load key is really the 
heaviest part)

++
Manu



[PATCH] ssl: factoring load cert/key and chains

2018-12-12 Thread Emmanuel Hocdet

Hi,

I tried to improve the haproxy loading time with a lot of certificates, and see 
a double file
open for each certificate (one for private-key and one for the cert/chain).
Multi-cert loading part have not this issue and is good candidate for sharing 
code:
patches is this work with factoring/cleanup/fix.

About speed: PEM file with private key in first position is far better.

If you can consider this patches?

++
Manu



0001-REORG-ssl-promote-cert_key_and_chain-handling.patch
Description: Binary data


0002-MINOR-ssl-use-STACK_OF-for-chain-certs.patch
Description: Binary data


0003-MINOR-ssl-SSL_CTX_set1_chain-compatibility.patch
Description: Binary data


0004-MINOR-ssl-used-cert_key_and_chain-func-in-load_cert_.patch
Description: Binary data


0005-BUG-MINOR-ssl-fix-ssl_sock_load_multi_cert-init-vars.patch
Description: Binary data


0006-CLEANUP-ssl-ssl_sock_load_crt_file_into_ckch.patch
Description: Binary data




Re: HTTP/3 | daniel.haxx.se

2018-11-12 Thread Emmanuel Hocdet

Hi Aleks,

> Le 12 nov. 2018 à 18:02, Aleksandar Lazic  a écrit :
> 
> Hi Manu.
> 
> Am 12.11.2018 um 16:19 schrieb Emmanuel Hocdet:
>> 
>> Hi,
>> 
>> The primary (major) step should be to deal with QUIC transport (over UDP).
>> At the same level as TCP for haproxy?
>> Willy should already have a little idea on it ;-)
> 
> Is then the conclusion for that that haproxy will be able to proxy/load 
> balance UDP?

The only conclusion is that haproxy should be able to proxy QUIC.
From wiki: «  QUIC, Quick UDP Internet Connections, aims to be nearly 
equivalent to an independent TCP 
<https://en.wikipedia.org/wiki/Transmission_Control_Protocol> connection »
It could be see as TCP/2, the connection part is provided by the application. 
For example,  the congestion avoidance algorithms
must be provide into the application space at both endpoints.
A very cool feature is that QUIC can support IP migration.

++
Manu





Re: HTTP/3 | daniel.haxx.se

2018-11-12 Thread Emmanuel Hocdet


Hi,

The primary (major) step should be to deal with QUIC transport (over UDP).
At the same level as TCP for haproxy?
Willy should already have a little idea on it ;-)

++
Manu

> Le 11 nov. 2018 à 20:38, Aleksandar Lazic  a écrit :
> 
> Hi.
> 
> FYI.
> 
> Oh no, that was quite fast after HTTP/2
> 
> https://daniel.haxx.se/blog/2018/11/11/http-3/
> 
> Regards
> Aleks
> 




[PATCH] MINOR: generate-certificates for BoringSSL

2018-10-03 Thread Emmanuel Hocdet

Hi,

For generate-certificates, X509V3_EXT_conf is used but it's an (very) old API
call: X509V3_EXT_nconf must be preferred. Openssl compatibility is ok
because it's inside #ifdef SSL_CTRL_SET_TLSEXT_HOSTNAME, introduce 5
years after X509V3_EXT_nconf.
(BoringSSL only have X509V3_EXT_nconf)

Christopher, if you have time to check this little patch :)

++
Manu




0001-MINOR-ssl-cleanup-old-openssl-API-call.patch
Description: Binary data


0002-MINOR-ssl-generate-certificates-for-BoringSSL.patch
Description: Binary data


Re: New TLS proposal for SNI => ESNI

2018-09-25 Thread Emmanuel Hocdet

Hi Aleks,

> Le 25 sept. 2018 à 08:05, Aleksandar Lazic  a écrit :
> 
> Hi.
> 
> Have anyone seen this?
> 
> https://www.eff.org/deeplinks/2018/09/esni-privacy-protecting-upgrade-https
> 
> It looks very interesting for higher privacy.
> 

Yep.
Also 
https://datatracker.ietf.org/meeting/102/materials/slides-102-tls-encrypted-sni-01
 

I looked in boringssl, only cloudflare version seem to have it (source not 
public).

This will be problematic to extract the eSNI in L6, even with the private key.

++
Manu



Re: [ANNOUNCE] haproxy-1.9-dev2

2018-09-18 Thread Emmanuel Hocdet

> Le 18 sept. 2018 à 11:54, Lukas Tribus  a écrit :
> 
> Hi Manu,
> 
> 
> On Fri, 14 Sep 2018 at 15:45, Emmanuel Hocdet  wrote:
>> 
>> Hi,
>> 
>> Quick test with 1.9-dev2, and i see latency (in seconds) to connect to 
>> haproxy with SSL (tcp mode).
>> It’s ok in master with 9f9b0c6a.
>> No time to investigate more for the moment.
> 
> I cannot reproduce it in a simple SSL termination + tcp mode
> configuration. There is probably something more to it.
> 
> 

perhaps with: tcp-request inspect-delay 5s

++
Manu



Re: OpenSSL and per-context option problem

2018-09-17 Thread Emmanuel Hocdet

Hi Thierry,

> Le 15 sept. 2018 à 18:06, Thierry Fournier  a écrit 
> :
> 
> Hi,
> 
> I tried to use per-context options, in order to enable HTTP2 for a short
> list of SNI. I just add lines like this:
> 
>   /certif1.pem [alpn h2,http/1.1] my-h2-host.com
>   /certif2.pem my-other-host.com
> 
> This configuration works fine on debian 8 with OpenSSL 1.0.2g, and doesn’t
> work on Ubuntu 16.04 with OpenSSL 1.0.2l.
> 
> I compile the OpenSSL debian package 1.0.2g on Ubuntu, and the feature is
> enabled.
> 
> My conclusion, is that some version of OpenSSL doesn’t support all per-context
> options. 
> 
> Do you have an opinion ?
> 

Are you sure it's not the opposite: doesn't work with 1.0.2g?

"Major changes between OpenSSL 1.0.2g and OpenSSL 1.0.2h [3 May 2016]

Modify behavior of ALPN to invoke callback after SNI/servername callback, such 
that updates to the SSL_CTX affect ALPN. »

++
Manu



Re: [ANNOUNCE] haproxy-1.9-dev2

2018-09-14 Thread Emmanuel Hocdet
Hi,

Quick test with 1.9-dev2, and i see latency (in seconds) to connect to haproxy 
with SSL (tcp mode).
It’s ok in master with 9f9b0c6a.
No time to investigate more for the moment.

++
Manu




Re: TLS 1.3 options available with OpenSSL 1.1.1

2018-09-14 Thread Emmanuel Hocdet


> Le 14 sept. 2018 à 14:01, Dirkjan Bussink  a écrit :
> 
> Hi all,
> 
>> On 14 Sep 2018, at 12:18, Emmanuel Hocdet  wrote:
>> 
>> Same deal with boringssl, TLSv <= 1.2 ciphers configuration and TLSv1.3 
>> ciphers are segregated.
>> https://boringssl.googlesource.com/boringssl/+/abbbee10ad4739648bcbf36a5ac52f23263a67dd%5E!/
> 
> This reminded me to double check with BoringSSL and LibreSSL how they handle 
> this. Neither has this new method, so I have updated the conditional used in 
> the patch to exclude these two as well.
> 

It’s not necessary, BoringSSL and LibreSSL have, at best,  
OPENSSL_VERSION_NUMBER  set to 1.1.0 for API compatibilité.

++
Manu





Re: TLS 1.3 options available with OpenSSL 1.1.1

2018-09-14 Thread Emmanuel Hocdet


Hi Emeric, Lukas, Dirkjan

> Le 14 sept. 2018 à 11:12, Emeric Brun  a écrit :
> 
> Hi Lukas, Dirkjan,
> 
> On 09/13/2018 10:17 PM, Lukas Tribus wrote:
>> Hello Dirkjan,
>> 
>> 
>> On Thu, 13 Sep 2018 at 16:44, Dirkjan Bussink  wrote:
>>> So with a new API call, does that mean adding for example a `ciphersuites`
>>> option that works similar to `ciphers` today that it accepts a string and 
>>> then
>>> calls `SSL_CTX_set_ciphersuites`?
>> 
>> Yes, that's what I'd have in mind. Sufficiently #ifdef that everything
>> still builds the same and documented of course so that people
>> understand what is what. Let's wait for Manu and Emeric's feedback
>> though, before investing your time.
> 
> I think if TLSv <= 1.2 and TLSv1.3 ciphers are handled separately, this is 
> good reason
> to add a new keyword to manage both at a same line on an haproxy 
> configuration file's line .
> 
> I've just realized that it may be the openssl's response to an issue we faced 
> on earlier version of 
> openssl1.1.1 dev branch where forcing cipher suite on a SSL_CTX broke TLSv1.2 
> handshakes if
> no TLSv1.3 ciphers were specified in this list.
> 
> Doing this, managing differently TLS <= v1.2 and 1.3 ciphers permits the user 
> to not face regression issues
> upgrading  to v1.1.1 when suites where forced in configuration because 
> openssl-1.1.1 kept default
> TVSv1.3 ciphers.
> 

Same deal with boringssl, TLSv <= 1.2 ciphers configuration and TLSv1.3 ciphers 
are segregated.
https://boringssl.googlesource.com/boringssl/+/abbbee10ad4739648bcbf36a5ac52f23263a67dd%5E!/

> So i'm convinced we need to handle this new TLSv1.3 cipher suite with a new 
> config keyword, but I
> don't know how we should name it. I think it will be a mistake to make appear 
> 1.3 in the new name because
> there is no warranty that next TLS versions will specify specific cipher 
> lists. Openssl's
> API make the choice of "ciphersuites" ... perhaps a the right choice.


Following the API name should be the right choice.

++
Manu





[PATCH] BUG/MEDIUM: ECC cert should work with TLS < v1.2 and openssl >= 1.1.1

2018-09-03 Thread Emmanuel Hocdet
Hi Lukas, Emeric

This patch fix the issue. If you can check it.
Thanks
Manu




0001-BUG-MEDIUM-ECC-cert-should-work-with-TLS-v1.2-and-op.patch
Description: Binary data




Re: BUG: ssl: regression with openssl 1.1.1 when using <= TLSv1.2

2018-09-03 Thread Emmanuel Hocdet

Hi Lukas,

> Le 2 sept. 2018 à 15:31, Lukas Tribus  a écrit :
> On Sat, 1 Sep 2018 at 20:49, Lukas Tribus  wrote:
>>> I've confirmed the change in behavior only happens with an ECC
>>> certificate, an RSA certificate is not affected.
>> 
>> Just to confirm that this is still an actual problem with current
>> haproxy and openssl 1.1.1pre9.
>> 
>> You just have to use a ECC certificate instead of a RSA certificate,
>> and it will fail with TLSv1.1 when strict-sni is enabled.
> 
> Actually the problem is worse: SNI doesn't work *at all* with ECC
> certificates in TLSv1.1 and TLSv1.0. It simply falls back to a
> matching RSA certificate or the default-certificate. Of course, if
> only the ECC certificate is there, and strict-sni is set, the
> handshake is rejected.
> 
> Same exact behavior happens with boringssl as well (not only openssl 1.1.1).
> 
> 
> Any help with this would be much appreciated.
> 


It’s in ssl_sock_switchctx_cbk for openssl 1.1.1/boringssl:
/* without TLSEXT_TYPE_signature_algorithms extension (< TLS 1.2) */
has_ecdsa_sig is not enable -> no check for ECDSA cipher -> ECC certificate for 
TLS < 1.2 can’t be selected.
strict-sni disable the default cert fallback, so handshake is rejected with no 
rsa certificate.

Certificate selection must be changed to match this case (check for ECDSA 
cipher when TLS < 1.2).
I can look at that.

++
Manu




Re: [PATCH] MINOR: ssl: BoringSSL matches OpenSSL 1.1.0

2018-07-25 Thread Emmanuel Hocdet
Le 25 juil. 2018 à 10:34, Emmanuel Hocdet <m...@gandi.net> a écrit :Hi WillyLe 24 juil. 2018 à 18:59, Willy Tarreau <w...@1wt.eu> a écrit :Hi Manu,On Mon, Jul 23, 2018 at 06:12:34PM +0200, Emmanuel Hocdet wrote:Hi Willy,This patch is necessary to build with current BoringSSL (SSL_SESSION is now opaque).BoringSSL correctly matches OpenSSL 1.1.0 since 3b2ff028 for haproxy needs.The patch revert part of haproxy 019f9b10 (openssl-compat.h).This will not break openssl/libressl compat.OK, but the chunk here seems to contradict this assertion :@@ -119,13 +114,6 @@ static inline const OCSP_CERTID *OCSP_SINGLERESP_get0_id(const OCSP_SINGLERESP *}#endif-#endif--#if (OPENSSL_VERSION_NUMBER < 0x101fL) || defined(LIBRESSL_VERSION_NUMBER)-/*- * Functions introduced in OpenSSL 1.1.0 and not yet present in LibreSSL- */-static inline pem_password_cb *SSL_CTX_get_default_passwd_cb(SSL_CTX *ctx){	return ctx->default_passwd_callback;I'm seeing that libressl will use a different code that is commonwith openssl while you seem to have targetted boringssl only. Maybe this part escaped from a larger patch that you used during development ?It’s ok because this function is inserted upper in the patch.As said, it's only a revert from 019f9b10 patches for openssl-compat.h.From:# Functions introduced in OpenSSL 1.1.0 and not yet present in LibreSSL / BoringSSL# Functions introduced in OpenSSL 1.1.0 and not yet present in LibreSSLTo:# Functions introduced in OpenSSL 1.1.0 and not yet present in LibreSSLThis patch is easier to read out of context:

0001-MINOR-ssl-BoringSSL-matches-OpenSSL-1.1.0.patch
Description: Binary data


Re: [PATCH] MINOR: ssl: BoringSSL matches OpenSSL 1.1.0

2018-07-25 Thread Emmanuel Hocdet
Hi Willy

> Le 24 juil. 2018 à 18:59, Willy Tarreau  a écrit :
> 
> Hi Manu,
> 
> On Mon, Jul 23, 2018 at 06:12:34PM +0200, Emmanuel Hocdet wrote:
>> Hi Willy,
>> 
>> This patch is necessary to build with current BoringSSL (SSL_SESSION is now 
>> opaque).
>> BoringSSL correctly matches OpenSSL 1.1.0 since 3b2ff028 for haproxy needs.
>> The patch revert part of haproxy 019f9b10 (openssl-compat.h).
>> This will not break openssl/libressl compat.
> 
> OK, but the chunk here seems to contradict this assertion :
> 
> 
> @@ -119,13 +114,6 @@ static inline const OCSP_CERTID 
> *OCSP_SINGLERESP_get0_id(const OCSP_SINGLERESP *
> }
> #endif
> 
> -#endif
> -
> -#if (OPENSSL_VERSION_NUMBER < 0x101fL) || 
> defined(LIBRESSL_VERSION_NUMBER)
> -/*
> - * Functions introduced in OpenSSL 1.1.0 and not yet present in LibreSSL
> - */
> -
> static inline pem_password_cb *SSL_CTX_get_default_passwd_cb(SSL_CTX *ctx)
> {
>   return ctx->default_passwd_callback;
> 
> I'm seeing that libressl will use a different code that is common
> with openssl while you seem to have targetted boringssl only. Maybe 
> this part escaped from a larger patch that you used during development ?
> 

It’s ok because this function is inserted upper in the patch.

As said, it's only a revert from 019f9b10 patches for openssl-compat.h.
From:
# Functions introduced in OpenSSL 1.1.0 and not yet present in LibreSSL / 
BoringSSL
# Functions introduced in OpenSSL 1.1.0 and not yet present in LibreSSL
To:
# Functions introduced in OpenSSL 1.1.0 and not yet present in LibreSSL

++
Manu




[PATCH] MINOR: ssl: BoringSSL matches OpenSSL 1.1.0

2018-07-23 Thread Emmanuel Hocdet
Hi Willy,

This patch is necessary to build with current BoringSSL (SSL_SESSION is now 
opaque).
BoringSSL correctly matches OpenSSL 1.1.0 since 3b2ff028 for haproxy needs.
The patch revert part of haproxy 019f9b10 (openssl-compat.h).
This will not break openssl/libressl compat.

Can you consider it for 1.9?
Thanks.

Manu



0001-MINOR-ssl-BoringSSL-matches-OpenSSL-1.1.0.patch
Description: Binary data




Re: [PATCH] Re: Random crash (segfault, double free, ...) with a mix of SSL + cipherlist hash

2018-06-19 Thread Emmanuel Hocdet


> Le 18 juin 2018 à 15:54, Thierry Fournier  a 
> écrit :
> 
> I don’t known. In fact it works, so it is not a bug. But, when I use the
> reservation for an ex_data slot, it returns the slot 0, and this slot is
> used for the compatibility layer and can be crush some data. I conclude
> that is a bug in Openssl. The reservation function must give a slot
> starting to 1.
> 

Indeed, it’s unexpected.

> Maybe, the recommendation is to not mix the compatibility functions
> like set_app_data() with *ex_data*() functions. But, I don’t see 
> anything about this.
> 

Neither do I...
In any case it's cleaner with  *_get_ex_new_index.

++
Manu





Re: [PATCH] Re: Random crash (segfault, double free, ...) with a mix of SSL + cipherlist hash

2018-06-18 Thread Emmanuel Hocdet

> Le 18 juin 2018 à 15:30, Thierry Fournier  a 
> écrit :
> 
> 
> 
>> On 18 Jun 2018, at 14:37, Emmanuel Hocdet  wrote:
>> 
>>> 
>>> Le 18 juin 2018 à 10:43, Thierry Fournier  a 
>>> écrit :
>>> 
>>> 
>>>> On 18 Jun 2018, at 10:33, Willy Tarreau  wrote:
>>>> 
>>>> On Sun, Jun 17, 2018 at 09:44:50PM +0200, thierry.fourn...@arpalert.org 
>>>> wrote:
>>>>> Finally, I got it ! It works with luck because we have 1 bug in Haproxy
>>>>> and 1 error (I suppose) in a OpenSSL compatibility layer.
>>>> (...)
>>>>> I join two patch. The first which fix the cipher capture must be
>>>>> backported to 1.8, for the second patch wich fix the app data
>>>>> compatibility, I dont known (at least 1.8).
>>>> 
>>>> Good job! I imagine you didn't have a funny week-end playing with this one 
>>>> :-/
>>> 
>>> 
>>> Yes, including the Friday :-) But I hope this path improve stability. If 
>>> someone
>>> have time and is interested by the subject, it may be interesting to see in 
>>> the
>>> OpenSSL code if the slot 0 used without reservation works fine, or works 
>>> because
>>> we have luck.
>>> 
>> 
>> It work find because slot 0 is natively reserved for old *_{set, 
>> get}_app_data API compatibility.
> 
> 
> Ok, thanks. So the classifcation BUG/MAJOR can be changed for BUG/MEDIUM
> because it impacts only the usage of SSL join with the cipherlist hash.
> Too late :-)
> 

I think it should not be a bug at all (second patch), and set of ex_data 
without reservation
(first patch and my patch) should be the only sources of bugs.



  1   2   3   >