Skip to content

[WIP] TLS: generate certificates pair dynamically (#18928)#19137

Closed
LuyaoZhong wants to merge 2 commits intoenvoyproxy:mainfrom
LuyaoZhong:tls-dynamic-cert
Closed

[WIP] TLS: generate certificates pair dynamically (#18928)#19137
LuyaoZhong wants to merge 2 commits intoenvoyproxy:mainfrom
LuyaoZhong:tls-dynamic-cert

Conversation

@LuyaoZhong
Copy link
Copy Markdown

  1. Introduce API to set root CA cert/key to enable this feature

    e.g.
    common_tls_context:
    tls_root_ca_certificate:
    cert: {"filename": "root-ca.pem"}
    private_key: {"filename": "root-ca.key"}

  2. Generate/reuse dynamic certificates pair in TLS transport socket and set SSL*

    a. if there is no corresponding cached certs, create CSR and
    create certs signed from root CA, then cache the generated
    certs to local cache looked up by host name
    b. if there is corresponding cached certs, reuse them

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

1. Introduce API to set root CA cert/key to enable this feature

   e.g.
   common_tls_context:
     tls_root_ca_certificate:
       cert: {"filename": "root-ca.pem"}
       private_key: {"filename": "root-ca.key"}

2. Generate/reuse dynamic certificates pair in TLS transport socket and set SSL*

   a. if there is no corresponding cached certs, create CSR and
      create certs signed from root CA, then cache the generated
      certs to local cache looked up by host name
   b. if there is corresponding cached certs, reuse them

Signed-off-by: Luyao Zhong <luyao.zhong@intel.com>
@repokitteh-read-only
Copy link
Copy Markdown

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: #19137 was opened by LuyaoZhong.

see: more, trace.

@LuyaoZhong LuyaoZhong changed the title TLS: generate certificates pair dynamically (#18928) [WIP] TLS: generate certificates pair dynamically (#18928) Nov 30, 2021
Copy link
Copy Markdown
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API review only

}

// [#next-free-field: 3]
message TlsRootCACertificate {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't have to be root CA, an intermediate CA works as well.

//
// Only one of *tls_certificates*, *tls_certificate_sds_secret_configs*,
// *tls_certificate_provider_instance* and *tls_root_ca_certificate* may be used.
TlsRootCACertificate tls_root_ca_certificate = 15;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thought, can this be one of CertificateProvider? I think that's an natural extension point you might want to use, though the extension mechanism itself isn't implemented yet. cc @markdroth

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for CertificateProvider. There have been other requests for that in the past (I think related to managing enormous numbers of certificates and not having a filter-chain per cert).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the status for CertificateProvider implementation? Is there any PoC code which allows me testing with my patch?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The config is defined in the .protos, but it hasn't been implemented yet.

Copy link
Copy Markdown
Author

@LuyaoZhong LuyaoZhong Dec 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you mean this should be config.core.v3.TypedExtensionConfig of CertificateProvider, right?
Is there any example which implemented such config, where am I supposed to define the TypedExtensionConfig and how do I use it in config file?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly, you don't need an API change in this file. Just implement CertificateProvider extension point, and the certificate generator should be one of the extension. If you're still unclear ping me on Slack.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got that I don't need an API change in this file. I just wanna another implemented extension point like this for reference.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The closest one is PrivateKeyMethodProvider, you may search related code to that to see how extension point is implemented.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I dived into the code and tried to implement, these names make me confused:
CertificateProviderPluginInstance, CertificateProvider, CertificateProviderPluginInstance

I need time to ramp up extension mechanism, I'd like to confirm it is CertificateProvider, if so I will create a new issue to discuss its implementation, since it will serve general usage in the future.

@phlax
Copy link
Copy Markdown
Member

phlax commented Dec 15, 2021

@LuyaoZhong is this PR WIP ? either way the PR state should reflect the title

there are some minor formatting issues, but i think this PR is waiting on feedback to be implemented before further review

/wait

@LuyaoZhong
Copy link
Copy Markdown
Author

@phlax Yes, it's WIP. I need to implementing an API extension point first, thanks for adding label.

@github-actions
Copy link
Copy Markdown

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 14, 2022
@github-actions
Copy link
Copy Markdown

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!

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

@phlax @lizan @ggreenway could you reopen this pr

@ggreenway ggreenway reopened this Mar 28, 2022
@github-actions github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Mar 29, 2022
@adisuissa
Copy link
Copy Markdown
Contributor

/wait-any

@github-actions
Copy link
Copy Markdown

github-actions bot commented May 1, 2022

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 May 1, 2022
@github-actions
Copy link
Copy Markdown

github-actions bot commented May 8, 2022

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 May 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api stale stalebot believes this issue/PR has not been touched recently waiting:any

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants