Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement Certificate Provider Framework (#19308) #19582

Closed
wants to merge 27 commits into from
Closed

Implement Certificate Provider Framework (#19308) #19582

wants to merge 27 commits into from

Conversation

LuyaoZhong
Copy link
Contributor

@LuyaoZhong LuyaoZhong commented Jan 18, 2022 •
edited

Implement Certificate Provider Framework

Constructing and managing the certificate provider instances in Bootstrap
Implement the ca_certificate_provider_instance in CertificateValidationContext
Implement mplement tls_certificate_provider_instance in CommonTlsContext

Risk Level: Low
Testing: Unit tests
Docs Changes: N/A
Release Notes: N/A
Fixes #19308

Signed-off-by: Luyao Zhong luyao.zhong@intel.com

@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #19582 was opened by LuyaoZhong.

see: more, trace.

@LuyaoZhong LuyaoZhong mentioned this pull request Jan 18, 2022
Closed
@LuyaoZhong
Copy link
Contributor Author

This PR is in WIP status, it's not ready for build and test. Please refer to #19308 and join our discussion.

@LuyaoZhong LuyaoZhong changed the title [WIP] Implement CertificateProviderPluginInstance mechanism (#19308) [WIP] implement certificate_provider_instances in bootstrap (#19308) Jan 20, 2022
@LuyaoZhong LuyaoZhong force-pushed the certificate-provider-instances branch from cf5834f to ee706f5 Compare January 20, 2022 10:20
@lizan
Copy link
Member

lizan commented Jan 24, 2022

@LuyaoZhong ping me when this is ready to review or you want an early feedback.

@snowp snowp added the waiting label Feb 1, 2022
@LuyaoZhong LuyaoZhong force-pushed the certificate-provider-instances branch from 1433757 to 5b6b6e6 Compare February 9, 2022 11:55
@LuyaoZhong
Copy link
Contributor Author

@lizan @markdroth
This PR focus on certificate providers management, including initialization of all certificate providers configured. Please help review the overall infrastructure code, particularly the CertificateProvider interfaces definition. Afterwards I will continue further implementation details.

@rojkov
Copy link
Member

rojkov commented Feb 10, 2022

To fix the CodeQL check you need to merge the main branch. Also check the other CI failures.

Please never use git --force push after a PR is set ready for review.

@LuyaoZhong
Copy link
Contributor Author

@rojkov Thanks for your kind reminder. This PR is not ready for fully reviewed, I just force pushed my code to get some early feedback.

lizan
lizan requested changes Feb 10, 2022
View reviewed changes
envoy/certificate_provider/certificate_provider_manager.h Outdated Show resolved Hide resolved
envoy/certificate_provider/certificate_provider_manager.h Outdated Show resolved Hide resolved
envoy/certificate_provider/certificate_provider_manager.h Outdated Show resolved Hide resolved
source/extensions/certificate_providers/certificate_provider.h Outdated Show resolved Hide resolved
source/extensions/certificate_providers/certificate_provider.h Outdated Show resolved Hide resolved
source/extensions/certificate_providers/default_cert_provider/config.h Outdated Show resolved Hide resolved
source/common/certificate_provider/certificate_provider_manager_impl.h Outdated Show resolved Hide resolved
source/common/certificate_provider/certificate_provider_manager_impl.h Outdated Show resolved Hide resolved
source/server/configuration_impl.cc Outdated Show resolved Hide resolved
source/common/certificate_provider/certificate_provider_manager_impl.cc Outdated Show resolved Hide resolved
@LuyaoZhong LuyaoZhong changed the title [WIP] implement certificate_provider_instances in bootstrap (#19308) implement certificate_provider_instances in bootstrap (#19308) Feb 15, 2022
@LuyaoZhong LuyaoZhong changed the title implement certificate_provider_instances in bootstrap (#19308) [WIP]implement certificate_provider_instances in bootstrap (#19308) Feb 15, 2022
@LuyaoZhong LuyaoZhong force-pushed the certificate-provider-instances branch 2 times, most recently from eab3c86 to 9be6c85 Compare February 15, 2022 13:05
@LuyaoZhong LuyaoZhong changed the title [WIP]implement certificate_provider_instances in bootstrap (#19308) implement certificate_provider_instances in bootstrap (#19308) Feb 15, 2022
Implement certificate_provider_instances in bootstrap (envoyproxy#19308)
d4eb3cb 
Introduce a CertificateProviderManager to parse the certificate_provider_instances
config and instantiate certificate providers.

Signed-off-by: Luyao Zhong <luyao.zhong@intel.com>
@LuyaoZhong
Copy link
Contributor Author

@lizan I updated the PR. I forced pushed the code again, currently it's ready for review and I remove "WIP" from PR title.

@LuyaoZhong
Copy link
Contributor Author

@ggreenway @lizan could you take a look at this PR?

@github-actions github-actions bot closed this Oct 7, 2022
@LuyaoZhong
Copy link
Contributor Author

@lizan could you reopen this PR?

As we mentioned we have a concrete example called TLS bumping to utilize this certificate provider framework, currently the PoC is submitted #23192. I think we can move forward this PR as a separate feature, polishing the API design, adding more testcases, etc, does it make sense to you?

@lizan lizan reopened this Oct 13, 2022
@github-actions github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Oct 13, 2022
Luyao Zhong added 4 commits November 3, 2022 10:28
Merge branch 'main' into certificate-provider-instances
73c06f4 
Signed-off-by: Luyao Zhong <luyao.zhong@intel.com>
update cert provider api
a1c21d2 
Signed-off-by: Luyao Zhong <luyao.zhong@intel.com>
Merge branch 'main' into certificate-provider-instances
ba78629 
Signed-off-by: Luyao Zhong <luyao.zhong@intel.com>
format fix
6361493 
Signed-off-by: Luyao Zhong <luyao.zhong@intel.com>
@LuyaoZhong LuyaoZhong marked this pull request as ready for review November 7, 2022 09:57
Luyao Zhong added 3 commits November 7, 2022 10:59
add mocks for cert provider
c3467b1 
Signed-off-by: Luyao Zhong <luyao.zhong@intel.com>
add more mocks for cert provider
7f6af44 
Signed-off-by: Luyao Zhong <luyao.zhong@intel.com>
fix format and fix test failures
814d7ec 
Signed-off-by: Luyao Zhong <luyao.zhong@intel.com>
@LuyaoZhong LuyaoZhong mentioned this pull request Nov 15, 2022
Merged
@alyssawilk
Copy link
Contributor

/wait on CI and conflicts

@phlax phlax mentioned this pull request Dec 15, 2022
Closed
@github-actions
Copy link

github-actions bot commented Jan 4, 2023

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jan 4, 2023
@github-actions
Copy link

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot closed this Jan 11, 2023
@botengyao
Copy link
Member

Is this at main branch now? Thanks!

@LuyaoZhong
Copy link
Contributor Author

@botengyao Not yet. Community would like concrete use case instead of only adding extension point. One use case is #18928 but we are still working on it.

cc @mattklein123

@vermajit
Copy link

vermajit commented Apr 5, 2023

@LuyaoZhong are you guys still pursuing this ?

@LuyaoZhong
Copy link
Contributor Author

@vermajit yes, we use it in #18928 , do you require this feature as well?

@vermajit
Copy link

vermajit commented Apr 6, 2023 via email •
edited

@LuyaoZhong
Copy link
Contributor Author

@vermajit Community would like concrete use case instead of only adding extension point. We need end user to adopt our solution and support us to merge this change, so it might take a long time.

Which feature are you interested in? Only certificate provider or the whole TLS bumping (dynamic TLS-I)?

@mattklein123 It seems many people want this feature , could we evaluate if it could be supported as a common feature by maintainer instead of end user?

@mattklein123
Copy link
Member

@mattklein123 It seems many people want this feature , could we evaluate if it could be supported as a common feature by maintainer instead of end user?

Let's come up with a concrete use case and then we can discuss.

@LuyaoZhong
Copy link
Contributor Author

LuyaoZhong commented Apr 7, 2023 •
edited

@mattklein123
One use case is TLS bumping(dynamic TLS-I/MITM), since we might want to filter the traffic when accessing external service. Envoy is a security guard in this use case. And the certificate provider here is used to get dynamic TLS certificate for downstream. Envoy can be egress gateway in cluster or just a standalone proxy.

According to the comments from other people, they would like to have similar functionalities.
#18928 (comment)
#20425
#20708

@mattklein123
Copy link
Member

I don't remember where we landed on this conversation last time, but there is a fully working use case that can be implemented in OSS we are open to that. Just adding an extension point is not very useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phlax phlax phlax left review comments

@KfreeZ KfreeZ KfreeZ left review comments

@kyessenov kyessenov kyessenov left review comments

@markdroth markdroth markdroth requested changes

@ZhenLian ZhenLian ZhenLian left review comments

@ggreenway ggreenway Awaiting requested review from ggreenway ggreenway is a code owner

@lizan lizan Awaiting requested review from lizan lizan is a code owner

Assignees

@lizan lizan

Labels
api stale stalebot believes this issue/PR has not been touched recently waiting
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement CertificateProvider mechanism