xds: Keep saved certs and roots after SslContext update - #13085
Draft
Nicolas-EM wants to merge 1 commit into
Draft
Nicolas-EM wants to merge 1 commit into
Nicolas-EM wants to merge 1 commit into
Conversation
Fixes grpc#13058. CertProviderSslContextProvider cleared the saved key, cert chain and trusted roots after every SslContext build. When the identity cert and the CA roots come from separate certificate provider instances, the roots provider usually does not send another update, so the next identity cert rotation found no roots and did not rebuild the SslContext. New connections kept the old, possibly expired, identity cert. The same problem occurred with a single shared provider instance when only the identity cert changed. The saved identity credentials and trust roots are now kept as the latest known values. An update from either provider rebuilds the SslContext with the latest values from both. This generalizes the fix in grpc#12340, which kept the roots only when using system root certs. As a result, a root-only update now also rebuilds the SslContext. When a shared provider instance updates the cert and the roots in the same refresh, the SslContext is built twice, and the first build briefly uses the new identity cert with the old roots.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #13058.
CertProviderSslContextProvidercleared the saved key, cert chain and trusted roots after everySslContextbuild (clearKeysAndCerts()). When the identity cert and the CA roots come from separate certificate provider instances (for example, twofile_watcherinstances with different refresh intervals), the roots provider usually does not send another update. The next identity cert rotation then found no roots and did not rebuild theSslContext, so new connections kept the old, possibly expired, identity cert. The same problem occurred with a single shared provider instance when only the identity cert changed.This change removes
clearKeysAndCerts(). The saved identity credentials and trust roots are now kept as the latest known values. An update from either provider rebuilds theSslContextwith the latest values from both, as discussed in the issue. This generalizes #12340, which kept the roots only when using system root certs.Behavior changes:
SslContext. Before, it did not, because the key had been cleared.SslContextis built twice. The first build briefly uses the new identity cert with the old roots. The next update immediately replaces it.Tests:
saved*fields after a build.*UpdateOnlytests fail without the change inCertProviderSslContextProvider.