Skip to content

Commit 1735ee5

Browse files
Zgoda91Zgoda91
committed
Allowing file watcher empty identity certs
1 parent 4f521cf commit 1735ee5

File tree

4 files changed

+130
-46
lines changed

4 files changed

+130
-46
lines changed

‎xds/src/main/java/io/grpc/xds/internal/security/certprovider/FileWatcherCertificateProvider.java‎

Lines changed: 25 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -73,10 +73,12 @@ final class FileWatcherCertificateProvider extends CertificateProvider implement
7373
this.scheduledExecutorService =
7474
checkNotNull(scheduledExecutorService, "scheduledExecutorService");
7575
this.timeProvider = checkNotNull(timeProvider, "timeProvider");
76-
this.certFile = Paths.get(checkNotNull(certFile, "certFile"));
77-
this.keyFile = Paths.get(checkNotNull(keyFile, "keyFile"));
78-
checkArgument((trustFile != null || spiffeTrustMapFile != null),
79-
"either trustFile or spiffeTrustMapFile must be present");
76+
checkArgument(((certFile != null) == (keyFile != null)),
77+
"certFile and keyFile must be both set or both unset");
78+
this.certFile = certFile == null ? null : Paths.get(certFile);
79+
this.keyFile = keyFile == null ? null : Paths.get(keyFile);
80+
checkArgument((trustFile != null || spiffeTrustMapFile != null || keyFile != null),
81+
"must be watching either root or identity certificates");
8082
if (spiffeTrustMapFile != null) {
8183
this.spiffeTrustMapFile = Paths.get(spiffeTrustMapFile);
8284
this.trustFile = null;
@@ -113,23 +115,26 @@ private synchronized void scheduleNextRefreshCertificate(long delayInSeconds) {
113115
void checkAndReloadCertificates() {
114116
try {
115117
try {
116-
FileTime currentCertTime = Files.getLastModifiedTime(certFile);
117-
FileTime currentKeyTime = Files.getLastModifiedTime(keyFile);
118-
if (!currentCertTime.equals(lastModifiedTimeCert)
119-
&& !currentKeyTime.equals(lastModifiedTimeKey)) {
120-
byte[] certFileContents = Files.readAllBytes(certFile);
121-
byte[] keyFileContents = Files.readAllBytes(keyFile);
122-
FileTime currentCertTime2 = Files.getLastModifiedTime(certFile);
123-
FileTime currentKeyTime2 = Files.getLastModifiedTime(keyFile);
124-
if (currentCertTime2.equals(currentCertTime) && currentKeyTime2.equals(currentKeyTime)) {
125-
try (ByteArrayInputStream certStream = new ByteArrayInputStream(certFileContents);
126-
ByteArrayInputStream keyStream = new ByteArrayInputStream(keyFileContents)) {
127-
PrivateKey privateKey = CertificateUtils.getPrivateKey(keyStream);
128-
X509Certificate[] certs = CertificateUtils.toX509Certificates(certStream);
129-
getWatcher().updateCertificate(privateKey, Arrays.asList(certs));
118+
if (certFile != null && keyFile != null) {
119+
FileTime currentCertTime = Files.getLastModifiedTime(certFile);
120+
FileTime currentKeyTime = Files.getLastModifiedTime(keyFile);
121+
if (!currentCertTime.equals(lastModifiedTimeCert)
122+
&& !currentKeyTime.equals(lastModifiedTimeKey)) {
123+
byte[] certFileContents = Files.readAllBytes(certFile);
124+
byte[] keyFileContents = Files.readAllBytes(keyFile);
125+
FileTime currentCertTime2 = Files.getLastModifiedTime(certFile);
126+
FileTime currentKeyTime2 = Files.getLastModifiedTime(keyFile);
127+
if (currentCertTime2.equals(currentCertTime)
128+
&& currentKeyTime2.equals(currentKeyTime)) {
129+
try (ByteArrayInputStream certStream = new ByteArrayInputStream(certFileContents);
130+
ByteArrayInputStream keyStream = new ByteArrayInputStream(keyFileContents)) {
131+
PrivateKey privateKey = CertificateUtils.getPrivateKey(keyStream);
132+
X509Certificate[] certs = CertificateUtils.toX509Certificates(certStream);
133+
getWatcher().updateCertificate(privateKey, Arrays.asList(certs));
134+
}
135+
lastModifiedTimeCert = currentCertTime;
136+
lastModifiedTimeKey = currentKeyTime;
130137
}
131-
lastModifiedTimeCert = currentCertTime;
132-
lastModifiedTimeKey = currentKeyTime;
133138
}
134139
}
135140
} catch (Throwable t) {

‎xds/src/main/java/io/grpc/xds/internal/security/certprovider/FileWatcherCertificateProviderProvider.java‎

Lines changed: 15 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
package io.grpc.xds.internal.security.certprovider;
1818

1919
import static com.google.common.base.Preconditions.checkArgument;
20-
import static com.google.common.base.Preconditions.checkNotNull;
2120

2221
import com.google.common.annotations.VisibleForTesting;
2322
import com.google.common.util.concurrent.ThreadFactoryBuilder;
@@ -94,30 +93,27 @@ public CertificateProvider createCertificateProvider(
9493
timeProvider);
9594
}
9695

97-
private static String checkForNullAndGet(Map<String, ?> map, String key) {
98-
return checkNotNull(JsonUtil.getString(map, key), "'" + key + "' is required in the config");
99-
}
100-
10196
private static Config validateAndTranslateConfig(Object config) {
10297
checkArgument(config instanceof Map, "Only Map supported for config");
10398
@SuppressWarnings("unchecked") Map<String, ?> map = (Map<String, ?>)config;
10499

105100
Config configObj = new Config();
106-
configObj.certFile = checkForNullAndGet(map, CERT_FILE_KEY);
107-
configObj.keyFile = checkForNullAndGet(map, KEY_FILE_KEY);
108-
if (enableSpiffe) {
109-
if (!map.containsKey(ROOT_FILE_KEY) && !map.containsKey(SPIFFE_TRUST_MAP_FILE_KEY)) {
110-
throw new NullPointerException(
111-
String.format("either '%s' or '%s' is required in the config",
112-
ROOT_FILE_KEY, SPIFFE_TRUST_MAP_FILE_KEY));
113-
}
114-
if (map.containsKey(SPIFFE_TRUST_MAP_FILE_KEY)) {
115-
configObj.spiffeTrustMapFile = JsonUtil.getString(map, SPIFFE_TRUST_MAP_FILE_KEY);
116-
} else {
117-
configObj.rootFile = JsonUtil.getString(map, ROOT_FILE_KEY);
118-
}
101+
configObj.certFile = JsonUtil.getString(map, CERT_FILE_KEY);
102+
configObj.keyFile = JsonUtil.getString(map, KEY_FILE_KEY);
103+
if ((configObj.certFile != null) != (configObj.keyFile != null)) {
104+
throw new NullPointerException(
105+
String.format("'%s' and '%s' must be both set or both unset",
106+
CERT_FILE_KEY, KEY_FILE_KEY));
107+
}
108+
if (!map.containsKey(ROOT_FILE_KEY)
109+
&& !map.containsKey(CERT_FILE_KEY)
110+
&& (!enableSpiffe || !map.containsKey(SPIFFE_TRUST_MAP_FILE_KEY))) {
111+
throw new NullPointerException("must be watching either root or identity certificates");
112+
}
113+
if (enableSpiffe && map.containsKey(SPIFFE_TRUST_MAP_FILE_KEY)) {
114+
configObj.spiffeTrustMapFile = JsonUtil.getString(map, SPIFFE_TRUST_MAP_FILE_KEY);
119115
} else {
120-
configObj.rootFile = checkForNullAndGet(map, ROOT_FILE_KEY);
116+
configObj.rootFile = JsonUtil.getString(map, ROOT_FILE_KEY);
121117
}
122118
String refreshIntervalString = JsonUtil.getString(map, REFRESH_INTERVAL_KEY);
123119
if (refreshIntervalString != null) {

‎xds/src/test/java/io/grpc/xds/internal/security/certprovider/FileWatcherCertificateProviderProviderTest.java‎

Lines changed: 61 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,9 @@ public void createProvider_missingCert_expectException() throws IOException {
206206
provider.createCertificateProvider(map, distWatcher, true);
207207
fail("exception expected");
208208
} catch (NullPointerException npe) {
209-
assertThat(npe).hasMessageThat().isEqualTo("'certificate_file' is required in the config");
209+
assertThat(npe)
210+
.hasMessageThat()
211+
.isEqualTo("'certificate_file' and 'private_key_file' must be both set or both unset");
210212
}
211213
}
212214

@@ -220,24 +222,69 @@ public void createProvider_missingKey_expectException() throws IOException {
220222
provider.createCertificateProvider(map, distWatcher, true);
221223
fail("exception expected");
222224
} catch (NullPointerException npe) {
223-
assertThat(npe).hasMessageThat().isEqualTo("'private_key_file' is required in the config");
225+
assertThat(npe)
226+
.hasMessageThat()
227+
.isEqualTo("'certificate_file' and 'private_key_file' must be both set or both unset");
224228
}
225229
}
226230

227231
@Test
228-
public void createProvider_missingRoot_expectException() throws IOException {
229-
String expectedMessage = enableSpiffe ? "either 'ca_certificate_file' or "
230-
+ "'spiffe_trust_bundle_map_file' is required in the config"
231-
: "'ca_certificate_file' is required in the config";
232+
public void createProvider_missingRootAndSpiffeConfig() throws IOException {
232233
CertificateProvider.DistributorWatcher distWatcher =
233234
new CertificateProvider.DistributorWatcher();
234235
@SuppressWarnings("unchecked")
235236
Map<String, ?> map = (Map<String, ?>) JsonParser.parse(MISSING_ROOT_AND_SPIFFE_CONFIG);
237+
ScheduledExecutorService mockService = mock(ScheduledExecutorService.class);
238+
when(scheduledExecutorServiceFactory.create()).thenReturn(mockService);
239+
provider.createCertificateProvider(map, distWatcher, true);
240+
verify(fileWatcherCertificateProviderFactory, times(1))
241+
.create(
242+
eq(distWatcher),
243+
eq(true),
244+
eq("/var/run/gke-spiffe/certs/certificates.pem"),
245+
eq("/var/run/gke-spiffe/certs/private_key.pem"),
246+
eq(null),
247+
eq(null),
248+
eq(600L),
249+
eq(mockService),
250+
eq(timeProvider));
251+
}
252+
253+
@Test
254+
public void createProvider_missingCertAndKeyConfig() throws IOException {
255+
CertificateProvider.DistributorWatcher distWatcher =
256+
new CertificateProvider.DistributorWatcher();
257+
@SuppressWarnings("unchecked")
258+
Map<String, ?> map = (Map<String, ?>) JsonParser.parse(MISSING_CERT_AND_KEY_CONFIG);
259+
ScheduledExecutorService mockService = mock(ScheduledExecutorService.class);
260+
when(scheduledExecutorServiceFactory.create()).thenReturn(mockService);
261+
provider.createCertificateProvider(map, distWatcher, true);
262+
verify(fileWatcherCertificateProviderFactory, times(1))
263+
.create(
264+
eq(distWatcher),
265+
eq(true),
266+
eq(null),
267+
eq(null),
268+
eq("/var/run/gke-spiffe/certs/ca_certificates.pem"),
269+
eq(null),
270+
eq(600L),
271+
eq(mockService),
272+
eq(timeProvider));
273+
}
274+
275+
@Test
276+
public void createProvider_emptyConfig_expectException() throws IOException {
277+
CertificateProvider.DistributorWatcher distWatcher =
278+
new CertificateProvider.DistributorWatcher();
279+
@SuppressWarnings("unchecked")
280+
Map<String, ?> map = (Map<String, ?>) JsonParser.parse(EMPTY_CONFIG);
236281
try {
237282
provider.createCertificateProvider(map, distWatcher, true);
238283
fail("exception expected");
239284
} catch (NullPointerException npe) {
240-
assertThat(npe).hasMessageThat().isEqualTo(expectedMessage);
285+
assertThat(npe)
286+
.hasMessageThat()
287+
.isEqualTo("must be watching either root or identity certificates");
241288
}
242289
}
243290

@@ -292,6 +339,13 @@ public void createProvider_missingRoot_expectException() throws IOException {
292339
+ " \"private_key_file\": \"/var/run/gke-spiffe/certs/private_key.pem\""
293340
+ " }";
294341

342+
private static final String MISSING_CERT_AND_KEY_CONFIG =
343+
"{\n"
344+
+ " \"ca_certificate_file\": \"/var/run/gke-spiffe/certs/ca_certificates.pem\""
345+
+ " }";
346+
347+
private static final String EMPTY_CONFIG = "{\n}";
348+
295349
private static final String ZERO_REFRESH_INTERVAL =
296350
"{\n"
297351
+ " \"certificate_file\": \"/var/run/gke-spiffe/certs/certificates2.pem\","

‎xds/src/test/java/io/grpc/xds/internal/security/certprovider/FileWatcherCertificateProviderTest.java‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@
2525
import static io.grpc.xds.internal.security.CommonTlsContextTestsUtil.SERVER_1_PEM_FILE;
2626
import static io.grpc.xds.internal.security.CommonTlsContextTestsUtil.SPIFFE_TRUST_MAP_1_FILE;
2727
import static java.nio.file.StandardCopyOption.REPLACE_EXISTING;
28+
import static org.junit.Assert.assertEquals;
29+
import static org.junit.Assert.assertThrows;
2830
import static org.mockito.ArgumentMatchers.any;
2931
import static org.mockito.ArgumentMatchers.eq;
3032
import static org.mockito.Mockito.doReturn;
@@ -357,6 +359,33 @@ public void getCertificate_missingRootFile() throws IOException, InterruptedExce
357359
verifyWatcherErrorUpdates(Status.Code.UNKNOWN, NoSuchFileException.class, 1, 0, "root.pem");
358360
}
359361

362+
@Test
363+
public void illegalConstructorArguments_MissingPrivateKeyWhileCertChainPresent()
364+
throws IllegalArgumentException {
365+
Exception ex = assertThrows(
366+
IllegalArgumentException.class,
367+
() -> new FileWatcherCertificateProvider(
368+
watcher, true, null, keyFile, rootFile, null, 600L, timeService, timeProvider));
369+
370+
String expectedMsg = "certFile and keyFile must be both set or both unset";
371+
String actualMsg = ex.getMessage();
372+
373+
assertEquals(expectedMsg, actualMsg);
374+
}
375+
376+
@Test
377+
public void illegalConstructorArguments_NoFilesToWatch() throws IllegalArgumentException {
378+
Exception ex = assertThrows(
379+
IllegalArgumentException.class,
380+
() -> new FileWatcherCertificateProvider(
381+
watcher, true, null, null, null, null, 600L, timeService, timeProvider));
382+
383+
String expectedMsg = "must be watching either root or identity certificates";
384+
String actualMsg = ex.getMessage();
385+
386+
assertEquals(expectedMsg, actualMsg);
387+
}
388+
360389
private void commonErrorTest(
361390
String certFile,
362391
String keyFile,

0 commit comments

Comments
 (0)