envoy
envoy copied to clipboard
Add support for intermediate CA as trusted_ca
Add support for intermediate CA as trusted_ca with X509_V_FLAG_PARTIAL_CHAIN
Risk Level: Low Testing: unit tests Docs Changes: ssl Release Notes: yes Fixes #19326
Signed-off-by: Luyao Zhong [email protected]
Before proceeding with this, we need to conclude discussion in the issue to make sure this won't have any negative side effects.
/wait
Yeah this sounds good to me, as a quick review, please add release note as behavior change.
@ggreenway @lizan
I just tried to get the CI results (I thought it should not break any tests). Unfortunately, the cert verification test fail, I need to do some investigation.
/wait
@ggreenway @lizan @dnephin
Without X509_V_FLAG_PARTIAL_CHAIN
, intermediate ca cert will not be treated as trust-anchors, it requires the trust-ca being a cert chain which containning root ca cert. max_verify_depth
is config for the max number of intermediate certificates in chain that are parsed during verification. So it expects a failure when setting max_verify_depth
no longer than root ca cert depth.
https://github.com/envoyproxy/envoy/blob/18212bb6395af308d895f75352f82df522b038b4/test/extensions/transport_sockets/tls/cert_validator/default_validator_integration_test.cc#L68-L73
After adding X509_V_FLAG_PARTIAL_CHAIN
as a default flag, this test can not pass. Because the intermediate ca cert can be treated as trust-anchor with this flag, the verification will succeed.
Except above, I don't think there are other negative side effects. Two options to enable this feature,
- making this flag as default, and refine the testcases a little bit
- do not set
X509_V_FLAG_PARTIAL_CHAIN
ifmax_verify_depth
is specified
which one do you prefer?
- sounds reasonable to me.
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/)
.
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/)
.
/retest
@adisuissa @ggreenway this looks ready for another round. Thanks!
Retrying Azure Pipelines: Retried failed jobs in: envoy-presubmit
/wait
Before proceeding with this, we need to conclude discussion in the issue to make sure this won't have any negative side effects.
What's the general mood on having this become part of main?
Thanks for the update, I think it LGTM
This is probably ready for @ggreenway review again since the test was fixed https://github.com/envoyproxy/envoy/pull/22350#discussion_r952527040
/retest
as there is no current CI result.
Retrying Azure Pipelines:
It seems this is blocked on CodeQL, merge main should unblock CI, @LuyaoZhong
seems like CI is stuck and this needs a merge from main @LuyaoZhong
/wait
/wait
This probably need maintainer feedback on this https://github.com/envoyproxy/envoy/pull/22350#discussion_r1001224029 before merge.
This looks good overall. I think we should add a runtime flag (default enabled) to disable the new behavior in the off chance this causes an issue for someone. Other than that, I think this looks good.
@ggreenway done
oh, wait, we should add test for runtime flag is false.
@soulxu For this feature, I don't think we need test for runtime flag is false, since the old code path(other than the code path that the runtime flag controls) was running for a long time and already tested before.
oh, wait, we should add test for runtime flag is false.
@soulxu For this feature, I don't think we need test for runtime flag is false, since the old code path(other than the code path that the runtime flag controls) was running for a long time and already tested before.
we should keep the old test to ensure there is no regression in the future
oh, wait, we should add test for runtime flag is false.
@soulxu For this feature, I don't think we need test for runtime flag is false, since the old code path(other than the code path that the runtime flag controls) was running for a long time and already tested before.
we should keep the old test to ensure there is no regression in the future
Sorry, what regression we might have? How to avoid regression by keeping old test?
oh, wait, we should add test for runtime flag is false.
@soulxu For this feature, I don't think we need test for runtime flag is false, since the old code path(other than the code path that the runtime flag controls) was running for a long time and already tested before.
we should keep the old test to ensure there is no regression in the future
Sorry, what regression we might have? How to avoid regression by keeping old test?
Sorry, I may not describe it clearly. We use runtime flag for switch two logic. You add the tests for the new logic, but the old logic still needs to keep here. But if there is no test for the old logic, it could happen regression if someone missed change the code. Just like, you already test the new code today and the code works well, but it doesn't mean you can remove those tests for the new code, you still need to keep the tests for it in case we won't break the code in the future. In the day we deprecate the runtime flag, then we can remove the old logic/code and the old tests.
Sorry, I may not describe it clearly. We use runtime flag for switch two logic. You add the tests for the new logic, but the old logic still needs to keep here. But if there is no test for the old logic, it could happen regression if someone missed change the code. Just like, you already test the new code today and the code works well, but it doesn't mean you can remove those tests for the new code, you still need to keep the tests for it in case we won't break the code in the future. In the day we deprecate the runtime flag, then we can remove the old logic/code and the old tests.
@soulxu Got it. Thanks.
/retest
Retrying Azure Pipelines: Retried failed jobs in: envoy-presubmit