Skip to content

Commit 65d0bb8

Browse files
ejona86ejona86
authored
xds: Enable deprecation warnings
The security code referenced fields removed from gRFC A29 before it was finalized. Note that this fixes a bug in CommonTlsContextUtil where CombinedValidationContext was not checked. I believe this was the only location with such a bug as I audited all non-test usages of has/getValidationContext() and confirmed they have have a corresponding has/getCombinedValidationContext().
1 parent f79ab2f commit 65d0bb8

16 files changed

+116
-253
lines changed

Diff for: ‎xds/build.gradle

-2
Original file line numberDiff line numberDiff line change
@@ -133,8 +133,6 @@ tasks.named("checkstyleThirdparty").configure {
133133

134134
tasks.named("compileJava").configure {
135135
it.options.compilerArgs += [
136-
// TODO: remove
137-
"-Xlint:-deprecation",
138136
// only has AutoValue annotation processor
139137
"-Xlint:-processing",
140138
]

Diff for: ‎xds/src/main/java/io/grpc/xds/ClusterResolverLoadBalancer.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -673,7 +673,7 @@ public Status onResult2(final ResolutionResult resolutionResult) {
673673
resolutionResult.getAddressesOrError();
674674
if (addressesOrError.hasValue()) {
675675
backoffPolicy = null; // reset backoff sequence if succeeded
676-
for (EquivalentAddressGroup eag : resolutionResult.getAddresses()) {
676+
for (EquivalentAddressGroup eag : addressesOrError.getValue()) {
677677
// No weight attribute is attached, all endpoint-level LB policy should be able
678678
// to handle such it.
679679
String localityName = localityName(LOGICAL_DNS_CLUSTER_LOCALITY);

Diff for: ‎xds/src/main/java/io/grpc/xds/RbacFilter.java

+7-2
Original file line numberDiff line numberDiff line change
@@ -276,8 +276,13 @@ private static Matcher parsePrincipal(Principal principal) {
276276
return createSourceIpMatcher(principal.getDirectRemoteIp());
277277
case REMOTE_IP:
278278
return createSourceIpMatcher(principal.getRemoteIp());
279-
case SOURCE_IP:
280-
return createSourceIpMatcher(principal.getSourceIp());
279+
case SOURCE_IP: {
280+
// gRFC A41 has identical handling of source_ip as remote_ip and direct_remote_ip and
281+
// pre-dates the deprecation.
282+
@SuppressWarnings("deprecation")
283+
CidrRange sourceIp = principal.getSourceIp();
284+
return createSourceIpMatcher(sourceIp);
285+
}
281286
case HEADER:
282287
return parseHeaderMatcher(principal.getHeader());
283288
case NOT_ID:

Diff for: ‎xds/src/main/java/io/grpc/xds/XdsClusterResource.java

+3-20
Original file line numberDiff line numberDiff line change
@@ -450,15 +450,6 @@ static void validateCommonTlsContext(
450450
throw new ResourceInvalidException(
451451
"common-tls-context with validation_context_sds_secret_config is not supported");
452452
}
453-
if (commonTlsContext.hasValidationContextCertificateProvider()) {
454-
throw new ResourceInvalidException(
455-
"common-tls-context with validation_context_certificate_provider is not supported");
456-
}
457-
if (commonTlsContext.hasValidationContextCertificateProviderInstance()) {
458-
throw new ResourceInvalidException(
459-
"common-tls-context with validation_context_certificate_provider_instance is not"
460-
+ " supported");
461-
}
462453
String certInstanceName = getIdentityCertInstanceName(commonTlsContext);
463454
if (certInstanceName == null) {
464455
if (server) {
@@ -473,10 +464,6 @@ static void validateCommonTlsContext(
473464
throw new ResourceInvalidException(
474465
"tls_certificate_provider_instance is unset");
475466
}
476-
if (commonTlsContext.hasTlsCertificateCertificateProvider()) {
477-
throw new ResourceInvalidException(
478-
"tls_certificate_provider_instance is unset");
479-
}
480467
} else if (certProviderInstances == null || !certProviderInstances.contains(certInstanceName)) {
481468
throw new ResourceInvalidException(
482469
"CertificateProvider instance name '" + certInstanceName
@@ -505,7 +492,9 @@ static void validateCommonTlsContext(
505492
.getDefaultValidationContext();
506493
}
507494
if (certificateValidationContext != null) {
508-
if (certificateValidationContext.getMatchSubjectAltNamesCount() > 0 && server) {
495+
@SuppressWarnings("deprecation") // gRFC A29 predates match_typed_subject_alt_names
496+
int matchSubjectAltNamesCount = certificateValidationContext.getMatchSubjectAltNamesCount();
497+
if (matchSubjectAltNamesCount > 0 && server) {
509498
throw new ResourceInvalidException(
510499
"match_subject_alt_names only allowed in upstream_tls_context");
511500
}
@@ -536,8 +525,6 @@ static void validateCommonTlsContext(
536525
private static String getIdentityCertInstanceName(CommonTlsContext commonTlsContext) {
537526
if (commonTlsContext.hasTlsCertificateProviderInstance()) {
538527
return commonTlsContext.getTlsCertificateProviderInstance().getInstanceName();
539-
} else if (commonTlsContext.hasTlsCertificateCertificateProviderInstance()) {
540-
return commonTlsContext.getTlsCertificateCertificateProviderInstance().getInstanceName();
541528
}
542529
return null;
543530
}
@@ -556,10 +543,6 @@ private static String getRootCertInstanceName(CommonTlsContext commonTlsContext)
556543
.hasCaCertificateProviderInstance()) {
557544
return combinedCertificateValidationContext.getDefaultValidationContext()
558545
.getCaCertificateProviderInstance().getInstanceName();
559-
} else if (combinedCertificateValidationContext
560-
.hasValidationContextCertificateProviderInstance()) {
561-
return combinedCertificateValidationContext
562-
.getValidationContextCertificateProviderInstance().getInstanceName();
563546
}
564547
}
565548
return null;

Diff for: ‎xds/src/main/java/io/grpc/xds/XdsRouteConfigureResource.java

+1-2
Original file line numberDiff line numberDiff line change
@@ -451,8 +451,7 @@ static StructOrError<RouteAction> parseRouteAction(
451451
config.getHeader();
452452
Pattern regEx = null;
453453
String regExSubstitute = null;
454-
if (headerCfg.hasRegexRewrite() && headerCfg.getRegexRewrite().hasPattern()
455-
&& headerCfg.getRegexRewrite().getPattern().hasGoogleRe2()) {
454+
if (headerCfg.hasRegexRewrite() && headerCfg.getRegexRewrite().hasPattern()) {
456455
regEx = Pattern.compile(headerCfg.getRegexRewrite().getPattern().getRegex());
457456
regExSubstitute = headerCfg.getRegexRewrite().getSubstitution();
458457
}

Diff for: ‎xds/src/main/java/io/grpc/xds/internal/MatcherParser.java

+13-4
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,12 @@ public static Matchers.HeaderMatcher parseHeaderMatcher(
2626
io.envoyproxy.envoy.config.route.v3.HeaderMatcher proto) {
2727
switch (proto.getHeaderMatchSpecifierCase()) {
2828
case EXACT_MATCH:
29+
@SuppressWarnings("deprecation") // gRFC A63: support indefinitely
30+
String exactMatch = proto.getExactMatch();
2931
return Matchers.HeaderMatcher.forExactValue(
30-
proto.getName(), proto.getExactMatch(), proto.getInvertMatch());
32+
proto.getName(), exactMatch, proto.getInvertMatch());
3133
case SAFE_REGEX_MATCH:
34+
@SuppressWarnings("deprecation") // gRFC A63: support indefinitely
3235
String rawPattern = proto.getSafeRegexMatch().getRegex();
3336
Pattern safeRegExMatch;
3437
try {
@@ -49,14 +52,20 @@ public static Matchers.HeaderMatcher parseHeaderMatcher(
4952
return Matchers.HeaderMatcher.forPresent(
5053
proto.getName(), proto.getPresentMatch(), proto.getInvertMatch());
5154
case PREFIX_MATCH:
55+
@SuppressWarnings("deprecation") // gRFC A63: support indefinitely
56+
String prefixMatch = proto.getPrefixMatch();
5257
return Matchers.HeaderMatcher.forPrefix(
53-
proto.getName(), proto.getPrefixMatch(), proto.getInvertMatch());
58+
proto.getName(), prefixMatch, proto.getInvertMatch());
5459
case SUFFIX_MATCH:
60+
@SuppressWarnings("deprecation") // gRFC A63: support indefinitely
61+
String suffixMatch = proto.getSuffixMatch();
5562
return Matchers.HeaderMatcher.forSuffix(
56-
proto.getName(), proto.getSuffixMatch(), proto.getInvertMatch());
63+
proto.getName(), suffixMatch, proto.getInvertMatch());
5764
case CONTAINS_MATCH:
65+
@SuppressWarnings("deprecation") // gRFC A63: support indefinitely
66+
String containsMatch = proto.getContainsMatch();
5867
return Matchers.HeaderMatcher.forContains(
59-
proto.getName(), proto.getContainsMatch(), proto.getInvertMatch());
68+
proto.getName(), containsMatch, proto.getInvertMatch());
6069
case STRING_MATCH:
6170
return Matchers.HeaderMatcher.forString(
6271
proto.getName(), parseStringMatcher(proto.getStringMatch()), proto.getInvertMatch());

Diff for: ‎xds/src/main/java/io/grpc/xds/internal/security/CommonTlsContextUtil.java

+4-17
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818

1919
import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CertificateProviderPluginInstance;
2020
import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CommonTlsContext;
21-
import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CommonTlsContext.CombinedCertificateValidationContext;
2221

2322
/** Class for utility functions for {@link CommonTlsContext}. */
2423
public final class CommonTlsContextUtil {
@@ -29,30 +28,18 @@ public static boolean hasCertProviderInstance(CommonTlsContext commonTlsContext)
2928
if (commonTlsContext == null) {
3029
return false;
3130
}
32-
return hasIdentityCertificateProviderInstance(commonTlsContext)
33-
|| hasCertProviderValidationContext(commonTlsContext);
34-
}
35-
36-
private static boolean hasCertProviderValidationContext(CommonTlsContext commonTlsContext) {
37-
if (commonTlsContext.hasCombinedValidationContext()) {
38-
CombinedCertificateValidationContext combinedCertificateValidationContext =
39-
commonTlsContext.getCombinedValidationContext();
40-
return combinedCertificateValidationContext.hasValidationContextCertificateProviderInstance();
41-
}
42-
return hasValidationProviderInstance(commonTlsContext);
43-
}
44-
45-
private static boolean hasIdentityCertificateProviderInstance(CommonTlsContext commonTlsContext) {
4631
return commonTlsContext.hasTlsCertificateProviderInstance()
47-
|| commonTlsContext.hasTlsCertificateCertificateProviderInstance();
32+
|| hasValidationProviderInstance(commonTlsContext);
4833
}
4934

5035
private static boolean hasValidationProviderInstance(CommonTlsContext commonTlsContext) {
5136
if (commonTlsContext.hasValidationContext() && commonTlsContext.getValidationContext()
5237
.hasCaCertificateProviderInstance()) {
5338
return true;
5439
}
55-
return commonTlsContext.hasValidationContextCertificateProviderInstance();
40+
return commonTlsContext.hasCombinedValidationContext()
41+
&& commonTlsContext.getCombinedValidationContext().getDefaultValidationContext()
42+
.hasCaCertificateProviderInstance();
5643
}
5744

5845
/**

Diff for: ‎xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderSslContextProvider.java

-11
Original file line numberDiff line numberDiff line change
@@ -99,8 +99,6 @@ protected static CertificateProviderInstance getCertProviderInstance(
9999
CommonTlsContext commonTlsContext) {
100100
if (commonTlsContext.hasTlsCertificateProviderInstance()) {
101101
return CommonTlsContextUtil.convert(commonTlsContext.getTlsCertificateProviderInstance());
102-
} else if (commonTlsContext.hasTlsCertificateCertificateProviderInstance()) {
103-
return commonTlsContext.getTlsCertificateCertificateProviderInstance();
104102
}
105103
return null;
106104
}
@@ -128,15 +126,6 @@ protected static CommonTlsContext.CertificateProviderInstance getRootCertProvide
128126
if (certValidationContext != null && certValidationContext.hasCaCertificateProviderInstance()) {
129127
return CommonTlsContextUtil.convert(certValidationContext.getCaCertificateProviderInstance());
130128
}
131-
if (commonTlsContext.hasCombinedValidationContext()) {
132-
CommonTlsContext.CombinedCertificateValidationContext combinedValidationContext =
133-
commonTlsContext.getCombinedValidationContext();
134-
if (combinedValidationContext.hasValidationContextCertificateProviderInstance()) {
135-
return combinedValidationContext.getValidationContextCertificateProviderInstance();
136-
}
137-
} else if (commonTlsContext.hasValidationContextCertificateProviderInstance()) {
138-
return commonTlsContext.getValidationContextCertificateProviderInstance();
139-
}
140129
return null;
141130
}
142131

Diff for: ‎xds/src/main/java/io/grpc/xds/internal/security/trust/XdsX509TrustManager.java

+1
Original file line numberDiff line numberDiff line change
@@ -207,6 +207,7 @@ void verifySubjectAltNameInChain(X509Certificate[] peerCertChain) throws Certifi
207207
if (certContext == null) {
208208
return;
209209
}
210+
@SuppressWarnings("deprecation") // gRFC A29 predates match_typed_subject_alt_names
210211
List<StringMatcher> verifyList = certContext.getMatchSubjectAltNamesList();
211212
if (verifyList.isEmpty()) {
212213
return;

Diff for: ‎xds/src/test/java/io/grpc/xds/FilterChainMatchingProtocolNegotiatorsTest.java

+1-2
Original file line numberDiff line numberDiff line change
@@ -1125,7 +1125,6 @@ public void filterChain_5stepMatch() throws Exception {
11251125
}
11261126

11271127
@Test
1128-
@SuppressWarnings("deprecation")
11291128
public void filterChainMatch_unsupportedMatchers() throws Exception {
11301129
EnvoyServerProtoData.DownstreamTlsContext tlsContext1 =
11311130
CommonTlsContextTestsUtil.buildTestInternalDownstreamTlsContext("CERT1", "ROOTCA");
@@ -1194,7 +1193,7 @@ public void filterChainMatch_unsupportedMatchers() throws Exception {
11941193
assertThat(sslSet.get()).isEqualTo(defaultFilterChain.sslContextProviderSupplier());
11951194
assertThat(routingSettable.get()).isEqualTo(noopConfig);
11961195
assertThat(sslSet.get().getTlsContext().getCommonTlsContext()
1197-
.getTlsCertificateCertificateProviderInstance()
1196+
.getTlsCertificateProviderInstance()
11981197
.getCertificateName()).isEqualTo("CERT3");
11991198
}
12001199

0 commit comments

Comments
 (0)