ssl: add ability to configure ECDH curves.#766
ssl: add ability to configure ECDH curves.#766mattklein123 merged 1 commit intoenvoyproxy:masterfrom
Conversation
There was a problem hiding this comment.
@mattklein123 I'm not sure what are you thoughts about changing the default. Do you prefer if I leave it as-is (i.e. P-256 only)? Should I split it into another PR?
There was a problem hiding this comment.
@PiotrSikora I don't really have an opinion. TBH I'm not qualified to review TLS changes like this. Can you find one (optimally 2) Google people to review? I can review for code style, etc. but that's about it.
There was a problem hiding this comment.
@mattklein123 my question was mostly to check if changing the default curve was acceptable. Those values are sane (Google's public facing infrastructure is using those curves as well).
@davidben would you mind commenting on the curves and/or looking at this PR? Thanks!
There was a problem hiding this comment.
Yes assuming at least 2 people who know what they are talking about give it a +1 I'm fine changing the defaults.
There was a problem hiding this comment.
Curve selection LGTM. Yeah, X25519 is generally the preferred curve, but P-256 is still needed to talk to all the older clients out there.
source/common/json/config_schemas.cc
Outdated
There was a problem hiding this comment.
Q: From schema perspective, should we verify non-empty? Or do we want to allow empty string to mean no ECDH? What makes sense?
There was a problem hiding this comment.
Empty string means that SSL_CTX_set1_curves_list won't be called and that BoringSSL is going to use its default curves (currently: X25519:P-256:P-384).
Having said that, setting cipher_suites and/or ecdh_curves to an empty value probably shouldn't be allowed, since the result is most likely not what user intended to do.
There was a problem hiding this comment.
👍 let's make the schema non-empty if set then.
While there, change default from "P-256" to "X25519:P-256". Signed-off-by: Piotr Sikora <piotrsikora@google.com>
97b75d7 to
402707f
Compare
Description: messed up gpg keys for heroku on github actions hosted machines. I opened heroku/cli#1464 with heroku to start a discussion. For now I found in https://cli-assets.heroku.com/install-ubuntu.sh where they host their key. Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>
Description: messed up gpg keys for heroku on github actions hosted machines. I opened heroku/cli#1464 with heroku to start a discussion. For now I found in https://cli-assets.heroku.com/install-ubuntu.sh where they host their key. Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>
**Commit Message** This cleans up the PR template so that it explicitly says how the title as well as the description is used in the finale commit. --------- Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
**Description** This is a follow up on #766 which also required the change in pr_style_check.yaml. Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
**Description** This fixes pr_style_check.yaml which has a remaining old portion from #766. Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
While there, change default from "P-256" to "X25519:P-256".