Skip to content

Add updateCertChain to SSL ContextImpl#3765

Closed
qiwzhang wants to merge 1 commit intoenvoyproxy:masterfrom
qiwzhang:ctx_inplace_update
Closed

Add updateCertChain to SSL ContextImpl#3765
qiwzhang wants to merge 1 commit intoenvoyproxy:masterfrom
qiwzhang:ctx_inplace_update

Conversation

@qiwzhang
Copy link
Contributor

Signed-off-by: Wayne Zhang qiwzhang@google.com

Description:

This is one of PR to support SDS dynamic secret. Please see high level design PR:#3748

In this PR, a new function updateCertChain is added to SSL ContextImpl class to update SSL ctx for certchain. It will be used when certificate is updated from SDS, the Context object will update its internal SSL_CTX with the new certificates

Risk Level: Low

Testing: Unit-test

Docs Changes: None
Release Notes: None

Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@qiwzhang
Copy link
Contributor Author

@PiotrSikora @lizan Could you help to review this?

Copy link
Contributor

@PiotrSikora PiotrSikora left a comment

Choose a reason for hiding this comment

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

You need to create new SSL_CTX and swap the old one with it. You cannot mutate SSL_CTX after SSL object was already created from it. cc @davidben FYI

You can reuse the old ContextImpl, though.

@qiwzhang
Copy link
Contributor Author

@PiotrSikora Thanks.

In that case, it is cleaner to use shared_ptr for ssl context object. Will use a new ContextImpl for a new SSL_CTX.

I have re-opened this PR: #3754

@qiwzhang
Copy link
Contributor Author

I think it is cleaner to create a new ContextImpl for each updated ContextConfig instead of using the same ContextImpl to update its internal SSL_CTX. There are many fields in ContextImpl beside ctx_, specially some callback flag. We could not just replace SSL_CTX ctx_ without replacing other fields. If we are replacing the other fields, then why not just create a new instance.

@qiwzhang qiwzhang closed this Jun 29, 2018
@qiwzhang qiwzhang deleted the ctx_inplace_update branch June 29, 2018 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants