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 using system root trust ca for TLS server authentication for XDS #11470

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Implement using system root trust ca for TLS server authentication for XDS #11470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
5433ebb
If TlsContext specifies to use System root certs, don't set the trust…
kannanjgithub Jul 30, 2024
c836168
Merge branch 'grpc:master' into xdsusesystemroot
kannanjgithub Aug 6, 2024
240ce31
.
kannanjgithub Aug 6, 2024
3c02531
Locally comment out setting the cacert root in the trust manager for …
kannanjgithub Aug 6, 2024
64e6cef
override system root trust store with added testca.
kannanjgithub Aug 12, 2024
dfbc812
Merge remote-tracking branch 'origin/xdsusesystemroot' into xdsusesys…
kannanjgithub Aug 12, 2024
29a4fbf
More changes needed in source code and test code for system root certs.
kannanjgithub Aug 13, 2024
1eca009
Read cacert from resources in test.
kannanjgithub Aug 13, 2024
4ac8dd6
Fix style
kannanjgithub Aug 13, 2024
c7bc172
Fix warning
kannanjgithub Aug 13, 2024
8feb398
Root cert insance set via ssl context provider should take precedence…
kannanjgithub Aug 19, 2024
39b07c3
Fix comments.
kannanjgithub Aug 28, 2024
b968088
change methods isMtls etc.
kannanjgithub Aug 28, 2024
8acab09
rename methods
kannanjgithub Aug 28, 2024
5c68d01
rename methods
kannanjgithub Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file added testing/src/main/resources/certs/cacerts-testca.jks
View file
Open in desktop
Binary file not shown.
10 changes: 6 additions & 4 deletions xds/src/main/java/io/grpc/xds/EnvoyServerProtoData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

package io.grpc.xds;

import static com.google.common.base.Preconditions.checkNotNull;

import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
Expand All @@ -41,13 +43,13 @@ private EnvoyServerProtoData() {
}

public abstract static class BaseTlsContext {
@Nullable protected final CommonTlsContext commonTlsContext;
protected final CommonTlsContext commonTlsContext;

protected BaseTlsContext(@Nullable CommonTlsContext commonTlsContext) {
this.commonTlsContext = commonTlsContext;
protected BaseTlsContext(CommonTlsContext commonTlsContext) {
this.commonTlsContext = checkNotNull(commonTlsContext, "commonTlsContext cannot be null.");
}

@Nullable public CommonTlsContext getCommonTlsContext() {
public CommonTlsContext getCommonTlsContext() {
return commonTlsContext;
}

Expand Down
6 changes: 4 additions & 2 deletions xds/src/main/java/io/grpc/xds/XdsClusterResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
import io.grpc.xds.XdsClusterResource.CdsUpdate;
import io.grpc.xds.client.XdsClient.ResourceUpdate;
import io.grpc.xds.client.XdsResourceType;
import io.grpc.xds.internal.security.CommonTlsContextUtil;
import java.util.List;
import java.util.Locale;
import java.util.Set;
Expand Down Expand Up @@ -430,9 +431,10 @@ static void validateCommonTlsContext(
}
String rootCaInstanceName = getRootCertInstanceName(commonTlsContext);
if (rootCaInstanceName == null) {
if (!server) {
if (!server && !CommonTlsContextUtil.isUsingSystemRootCerts(commonTlsContext)) {
throw new ResourceInvalidException(
"ca_certificate_provider_instance is required in upstream-tls-context");
"ca_certificate_provider_instance or system_root_certs is required in "
+ "upstream-tls-context");
}
} else {
if (certProviderInstances == null || !certProviderInstances.contains(rootCaInstanceName)) {
Expand Down
18 changes: 4 additions & 14 deletions xds/src/main/java/io/grpc/xds/internal/security/ClientSslContextProviderFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@

package io.grpc.xds.internal.security;

import static com.google.common.base.Preconditions.checkNotNull;

import io.grpc.xds.EnvoyServerProtoData.UpstreamTlsContext;
import io.grpc.xds.client.Bootstrapper.BootstrapInfo;
import io.grpc.xds.internal.security.ReferenceCountingMap.ValueFactory;
Expand All @@ -44,17 +42,9 @@ final class ClientSslContextProviderFactory
/** Creates an SslContextProvider from the given UpstreamTlsContext. */
@Override
public SslContextProvider create(UpstreamTlsContext upstreamTlsContext) {
checkNotNull(upstreamTlsContext, "upstreamTlsContext");
checkNotNull(
upstreamTlsContext.getCommonTlsContext(),
"upstreamTlsContext should have CommonTlsContext");
if (CommonTlsContextUtil.hasCertProviderInstance(
Copy link
Member

Choose a reason for hiding this comment

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

We should move this check into certProviderClientSslContextProviderFactory.getProvider(). That makes it more obvious what is going on and avoids deleting testProviderForClient_rootInstanceNull_expectError(). It'd be fine to move the checkNotNull checks to getProvider() as well.

(Although it looks like upstreamTlsContext.getCommonTlsContext() can never be null (except in tests), because proto won't return a null message, so UpstreamTlsContext.fromEnvoyProtoUpstreamTlsContext() will never pass null to the UpstreamTlsContext constructor. I see BaseTlsContext has the field mark @Nullable, but it looks to never be null. We could potentially add a checkNotNull to BaseTlsContext instead of checking for null here.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

upstreamTlsContext.getCommonTlsContext())) {
return certProviderClientSslContextProviderFactory.getProvider(
upstreamTlsContext,
bootstrapInfo.node().toEnvoyProtoNode(),
bootstrapInfo.certProviders());
}
throw new UnsupportedOperationException("Unsupported configurations in UpstreamTlsContext!");
return certProviderClientSslContextProviderFactory.getProvider(
upstreamTlsContext,
bootstrapInfo.node().toEnvoyProtoNode(),
bootstrapInfo.certProviders());
}
}
13 changes: 12 additions & 1 deletion xds/src/main/java/io/grpc/xds/internal/security/CommonTlsContextUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public final class CommonTlsContextUtil {

private CommonTlsContextUtil() {}

static boolean hasCertProviderInstance(CommonTlsContext commonTlsContext) {
public static boolean hasCertProviderInstance(CommonTlsContext commonTlsContext) {
if (commonTlsContext == null) {
return false;
}
Expand Down Expand Up @@ -65,4 +65,15 @@ public static CommonTlsContext.CertificateProviderInstance convert(
.setInstanceName(pluginInstance.getInstanceName())
.setCertificateName(pluginInstance.getCertificateName()).build();
}

public static boolean isUsingSystemRootCerts(CommonTlsContext commonTlsContext) {
if (commonTlsContext.hasCombinedValidationContext()) {
return commonTlsContext.getCombinedValidationContext().getDefaultValidationContext()
.hasSystemRootCerts();
}
if (commonTlsContext.hasValidationContext()) {
return commonTlsContext.getValidationContext().hasSystemRootCerts();
}
return false;
}
}
19 changes: 10 additions & 9 deletions ...java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@

package io.grpc.xds.internal.security.certprovider;

import static com.google.common.base.Preconditions.checkNotNull;

import io.envoyproxy.envoy.config.core.v3.Node;
import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CertificateValidationContext;
import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CommonTlsContext;
Expand Down Expand Up @@ -46,7 +44,7 @@ final class CertProviderClientSslContextProvider extends CertProviderSslContextP
node,
certProviders,
certInstance,
checkNotNull(rootCertInstance, "Client SSL requires rootCertInstance"),
Copy link
Member

Choose a reason for hiding this comment

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

CertProviderSslContextProvider.isMtls() can return the wrong value when rootCertInstance is null but the system root certs should be used. I don't know what that is used for, but have you check through the class to make sure the null is handled correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are usages of isMtls() /isClientSideTls()/isServerSideTls() for updating the SslContext.
The usage if isMtls() in server cert provider for setting the trust manager with the ca root certs won't be impacted because the change is only on the client side. In any case I think if (savedTrustedRoots != null) would do the same work as the isMtls() check here.

The usage of isMtls() in client cert provider for setting the keyManager is impacted by the change, and is rather done better with the check if (savedKey != null) instead of if (isMtls()).

The usages in the base class CertProviderSslContextProvider of isMtls() / isClientSideTls() / isServerSideTls() is very confusing to me, since both the client side and server side providers inherit from it (i.e. since a connection has a Xds client and a Xds server, will calling any of these methods on either party should return the same result, it doesn't look like it). Each of these usages also have a further nested check based on savedKey or savedTrustedRoots, I think the inner if conditions can just be used by themselves without the check first for sMtls() / isClientSideTls() / isServerSideTls(), unless those checks achieve anything meaningful.

Copy link
Contributor Author

@kannanjgithub kannanjgithub Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For the usages in the base class CertProviderSslContextProvider that I mentioned above can we use only the inner if conditions nested under each of the isMtls() / isClientSideTls() / isServerSideTls() calls?

Copy link
Member

Choose a reason for hiding this comment

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

won't be impacted because the change is only on the client side

Well, that's assuming we notice the issue if/when we add server support.

It looks like we should get rid of isMtls() and instead have new methods to check if keys or cert validation is configured, to be used as necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized the trio of methods isMtls() / isClientSideTls() / isServerSideTls() help do 2 things -

  1. When either the key or trusted roots get updated, instead of just calling updateSslContext with that value, it is used to first check whether either or both of key / trusted roots as applicable is not null before calling updateSslContext.
  2. And in the client side, if Mtls it will add client certificate to the context, and on the server side if Mtls it will add trusted roots to the context.

I think the easy way and the only thing needed is to augment isMtls() to

protected final boolean isMtls() {
    return certInstance != null && (rootCertInstance != null || isUsingSystemRootCerts;
  }

I have also introduced a test for Mtls with system roots for the client: XdsSecurityClientServerTest::tlsClientServer_useSystemRootCerts_requireClientAuth.

rootCertInstance,
staticCertValidationContext,
upstreamTlsContext,
certificateProviderStore);
Expand All @@ -56,12 +54,15 @@ final class CertProviderClientSslContextProvider extends CertProviderSslContextP
protected final SslContextBuilder getSslContextBuilder(
CertificateValidationContext certificateValidationContextdationContext)
throws CertStoreException {
SslContextBuilder sslContextBuilder =
GrpcSslContexts.forClient()
.trustManager(
new XdsTrustManagerFactory(
savedTrustedRoots.toArray(new X509Certificate[0]),
certificateValidationContextdationContext));
SslContextBuilder sslContextBuilder = GrpcSslContexts.forClient();
// Null rootCertInstance implies hasSystemRootCerts because of the check in
// {@link CertProviderClientSslContextProviderFactory}.
if (rootCertInstance != null) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's have a comment describing that null rootCertInstance implies hasSystemRootCerts because of ClientSslContextProviderFactory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

sslContextBuilder.trustManager(
new XdsTrustManagerFactory(
savedTrustedRoots.toArray(new X509Certificate[0]),
certificateValidationContextdationContext));
}
if (isMtls()) {
sslContextBuilder.keyManager(savedKey, savedCertChain);
}
Expand Down
21 changes: 13 additions & 8 deletions .../grpc/xds/internal/security/certprovider/CertProviderClientSslContextProviderFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import io.grpc.Internal;
import io.grpc.xds.EnvoyServerProtoData.UpstreamTlsContext;
import io.grpc.xds.client.Bootstrapper.CertificateProviderInfo;
import io.grpc.xds.internal.security.CommonTlsContextUtil;
import io.grpc.xds.internal.security.SslContextProvider;
import java.util.Map;
import javax.annotation.Nullable;
Expand Down Expand Up @@ -64,13 +65,17 @@ public SslContextProvider getProvider(
= CertProviderSslContextProvider.getRootCertProviderInstance(commonTlsContext);
CommonTlsContext.CertificateProviderInstance certInstance
= CertProviderSslContextProvider.getCertProviderInstance(commonTlsContext);
return new CertProviderClientSslContextProvider(
node,
certProviders,
certInstance,
rootCertInstance,
staticCertValidationContext,
upstreamTlsContext,
certificateProviderStore);
if (CommonTlsContextUtil.hasCertProviderInstance(upstreamTlsContext.getCommonTlsContext())
|| CommonTlsContextUtil.isUsingSystemRootCerts(upstreamTlsContext.getCommonTlsContext())) {
return new CertProviderClientSslContextProvider(
node,
certProviders,
certInstance,
rootCertInstance,
staticCertValidationContext,
upstreamTlsContext,
certificateProviderStore);
}
throw new UnsupportedOperationException("Unsupported configurations in UpstreamTlsContext!");
}
}
9 changes: 6 additions & 3 deletions .../main/java/io/grpc/xds/internal/security/certprovider/CertProviderSslContextProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,11 @@ abstract class CertProviderSslContextProvider extends DynamicSslContextProvider
@Nullable private final CertificateProviderStore.Handle certHandle;
@Nullable private final CertificateProviderStore.Handle rootCertHandle;
@Nullable private final CertificateProviderInstance certInstance;
@Nullable private final CertificateProviderInstance rootCertInstance;
@Nullable protected final CertificateProviderInstance rootCertInstance;
@Nullable protected PrivateKey savedKey;
@Nullable protected List<X509Certificate> savedCertChain;
@Nullable protected List<X509Certificate> savedTrustedRoots;
private final boolean isUsingSystemRootCerts;

protected CertProviderSslContextProvider(
Node node,
Expand Down Expand Up @@ -83,6 +84,8 @@ protected CertProviderSslContextProvider(
} else {
rootCertHandle = null;
}
this.isUsingSystemRootCerts = CommonTlsContextUtil.isUsingSystemRootCerts(
tlsContext.getCommonTlsContext());
}

private static CertificateProviderInfo getCertProviderConfig(
Expand Down Expand Up @@ -151,7 +154,7 @@ public final void updateTrustedRoots(List<X509Certificate> trustedRoots) {

private void updateSslContextWhenReady() {
if (isMtls()) {
if (savedKey != null && savedTrustedRoots != null) {
if (savedKey != null && (savedTrustedRoots != null || isUsingSystemRootCerts)) {
updateSslContext();
clearKeysAndCerts();
}
Expand All @@ -175,7 +178,7 @@ private void clearKeysAndCerts() {
}

protected final boolean isMtls() {
return certInstance != null && rootCertInstance != null;
return certInstance != null && (rootCertInstance != null || isUsingSystemRootCerts);
}

protected final boolean isClientSideTls() {
Expand Down
41 changes: 38 additions & 3 deletions xds/src/test/java/io/grpc/xds/GrpcXdsClientImplDataTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2503,7 +2503,8 @@ public void validateCommonTlsContext_validationContext() throws ResourceInvalidE
.setValidationContext(CertificateValidationContext.getDefaultInstance())
.build();
thrown.expect(ResourceInvalidException.class);
thrown.expectMessage("ca_certificate_provider_instance is required in upstream-tls-context");
thrown.expectMessage("ca_certificate_provider_instance or system_root_certs is required "
+ "in upstream-tls-context");
XdsClusterResource.validateCommonTlsContext(commonTlsContext, null, false);
}

Expand Down Expand Up @@ -2613,6 +2614,38 @@ public void validateCommonTlsContext_validationContextProviderInstance()
.validateCommonTlsContext(commonTlsContext, ImmutableSet.of("name1", "name2"), false);
}

@Test
public void validateCommonTlsContext_combinedValidationContextSystemRootCerts()
throws ResourceInvalidException {
CommonTlsContext commonTlsContext = CommonTlsContext.newBuilder()
.setCombinedValidationContext(
CommonTlsContext.CombinedCertificateValidationContext.newBuilder()
.setDefaultValidationContext(
CertificateValidationContext.newBuilder()
.setSystemRootCerts(
CertificateValidationContext.SystemRootCerts.newBuilder().build())
.build()
)
.build())
.build();
XdsClusterResource
.validateCommonTlsContext(commonTlsContext, ImmutableSet.of(), false);
}

@Test
public void validateCommonTlsContext_validationContextSystemRootCerts()
throws ResourceInvalidException {
CommonTlsContext commonTlsContext = CommonTlsContext.newBuilder()
.setValidationContext(
CertificateValidationContext.newBuilder()
.setSystemRootCerts(
CertificateValidationContext.SystemRootCerts.newBuilder().build())
.build())
.build();
XdsClusterResource
.validateCommonTlsContext(commonTlsContext, ImmutableSet.of(), false);
}

@Test
@SuppressWarnings("deprecation")
public void validateCommonTlsContext_validationContextProviderInstance_absentInBootstrapFile()
Expand Down Expand Up @@ -2674,7 +2707,8 @@ public void validateCommonTlsContext_combinedValidationContext_isRequiredForClie
CommonTlsContext commonTlsContext = CommonTlsContext.newBuilder()
.build();
thrown.expect(ResourceInvalidException.class);
thrown.expectMessage("ca_certificate_provider_instance is required in upstream-tls-context");
thrown.expectMessage("ca_certificate_provider_instance or system_root_certs is required "
+ "in upstream-tls-context");
XdsClusterResource.validateCommonTlsContext(commonTlsContext, null, false);
}

Expand All @@ -2687,7 +2721,8 @@ public void validateCommonTlsContext_combinedValidationContextWithoutCertProvide
.build();
thrown.expect(ResourceInvalidException.class);
thrown.expectMessage(
"ca_certificate_provider_instance is required in upstream-tls-context");
"ca_certificate_provider_instance or system_root_certs is required in "
+ "upstream-tls-context");
XdsClusterResource.validateCommonTlsContext(commonTlsContext, null, false);
}

Expand Down
3 changes: 2 additions & 1 deletion xds/src/test/java/io/grpc/xds/GrpcXdsClientImplTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2218,7 +2218,8 @@ public void cdsResponseErrorHandling_badUpstreamTlsContext() {
String errorMsg = "CDS response Cluster 'cluster.googleapis.com' validation error: "
+ "Cluster cluster.googleapis.com: malformed UpstreamTlsContext: "
+ "io.grpc.xds.client.XdsResourceType$ResourceInvalidException: "
+ "ca_certificate_provider_instance is required in upstream-tls-context";
+ "ca_certificate_provider_instance or system_root_certs is required in "
+ "upstream-tls-context";
call.verifyRequestNack(CDS, CDS_RESOURCE, "", "0000", NODE, ImmutableList.of(errorMsg));
verify(cdsResourceWatcher).onError(errorCaptor.capture());
verifyStatusWithNodeId(errorCaptor.getValue(), Code.UNAVAILABLE, errorMsg);
Expand Down
Loading