xds: SslContext updates handling when using system root certs #12340
Conversation
There was a problem hiding this comment.
A bit tangential, but in updateSslContextWhenReady(), are we missing a check for isUsingSystemRootCerts in the isClientSideTls() block?
} else if (isClientSideTls()) {
if (savedTrustedRoots != null || savedSpiffeTrustMap != null) {
(If that needs fixing, it can be done in a separate PR).
…ootcerts-ignore-trusted-root-updates
I have added a comment explaining. I have also renamed |
| protected final boolean isClientSideTls() { | ||
| protected final boolean isNormalTlsAndClientSide() { | ||
| // We don't do (rootCertInstance != null || isUsingSystemRootCerts) here because of how this | ||
| // method is used. With the rootCertInstance being null when using system root certs, there |
There was a problem hiding this comment.
Is that true? Regular TLS on client-side doesn't match any cases in updateSslContextWhenReady(), and I don't see any other calls to updateSslContext().
There was a problem hiding this comment.
Yes, that's what I intended and wrote in this comment, because the root cert is from the system store and cannot be updated by the watcher mechanism which is what will invoke updateSslContextWhenReady. Only if it is Mtls, when the client cert needs changing, it shall happen via updateSslContextWhenReady. The system root cert shall never get updated. The first and only update for system root certs should happen right in SslContextProviderSupplier.updateSslContext by executing callback.updateSslContext on the callback's executor.
The file watcher based update should only update client certs if using Mtls.
There was a problem hiding this comment.
I was going to do the SslContext updating changes for system root certs in the SNI PR but I thought the better of it and have done those changes in this PR now.
There was a problem hiding this comment.
As discussed, removed the special handling for system root certs in SslContextProviderSupplier, and instead making the sslContext available in the constructor of the client SslContext provider.
kannanjgithub
changed the title
…lier and handle it in
…usted-root-updates # Conflicts: # xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProvider.java
...src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderSslContextProvider.java
Show resolved
Hide resolved
| if (rootCertInstance != null | ||
| && rootCertInstance.isInitialized() | ||
| && !rootCertInstance.getInstanceName().equals(certInstanceName)) { | ||
| if (createRootCertInstance && sharedCertInstance) { |
There was a problem hiding this comment.
This condition/block can be deleted while rootCertHandle isn't being used for the isMtls() and related functions.
kannanjgithub
deleted the
systemrootcerts-ignore-trusted-root-updates
branch
…2340) Fixes the issues in SslContext not updated when using system root certs because it was using `CertProviderClientSslContextProvider` that relies on watcher updates from the certificate file watcher. This change creates a separate handler for system root certs and updates the SslContext on the `SslContextProvider` callback as soon as the provider is created.
…2340) Fixes the issues in SslContext not updated when using system root certs because it was using `CertProviderClientSslContextProvider` that relies on watcher updates from the certificate file watcher. This change creates a separate handler for system root certs and updates the SslContext on the `SslContextProvider` callback as soon as the provider is created.
No description provided.