Skip to content

Commit 0dd2a51

Browse files
author
Nelson Ochoa
authored
fix: return CA which was used to issue updated certificates (#134)
1 parent d8d0bf8 commit 0dd2a51

File tree

7 files changed

+82
-44
lines changed

7 files changed

+82
-44
lines changed

Diff for: ‎src/main/java/com/aws/greengrass/clientdevices/auth/CertificateManager.java

+8-20
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@
5050
import java.util.List;
5151
import java.util.Map;
5252
import java.util.concurrent.ConcurrentHashMap;
53-
import java.util.function.Consumer;
53+
import java.util.function.BiConsumer;
5454
import javax.inject.Inject;
5555

5656
public class CertificateManager {
@@ -138,17 +138,6 @@ public List<String> getCACertificates() throws KeyStoreException, IOException, C
138138
return Collections.singletonList(CertificateHelper.toPem(certificateStore.getCACertificate()));
139139
}
140140

141-
/**
142-
* Returns the full CA chain.
143-
*
144-
* @throws KeyStoreException when failing to read certificates from key store.
145-
*
146-
* @deprecated start using the certificateStore.getCaPassphrase directly
147-
*/
148-
private X509Certificate[] getX509CACertificates() throws KeyStoreException {
149-
return certificateStore.getCaCertificateChain();
150-
}
151-
152141
public String getCaPassPhrase() {
153142
return certificateStore.getCaPassphrase();
154143
}
@@ -174,24 +163,23 @@ public void subscribeToCertificateUpdates(GetCertificateRequest getCertificateRe
174163
getCertificateRequest.getCertificateRequestOptions().getCertificateType();
175164
// TODO: Should be configurable
176165
KeyPair keyPair = CertificateStore.newRSAKeyPair(4096);
177-
X509Certificate[] caCertificates = getX509CACertificates();
178166

179167
if (certificateType.equals(GetCertificateRequestOptions.CertificateType.SERVER)) {
180-
Consumer<X509Certificate> consumer = (t) -> {
168+
BiConsumer<X509Certificate, X509Certificate[]> consumer = (serverCert, caCertificates) -> {
181169
CertificateUpdateEvent certificateUpdateEvent =
182-
new CertificateUpdateEvent(keyPair, t, caCertificates);
170+
new CertificateUpdateEvent(keyPair, serverCert, caCertificates);
183171
getCertificateRequest.getCertificateUpdateConsumer().accept(certificateUpdateEvent);
184172
};
185173
subscribeToServerCertificateUpdatesNoCSR(getCertificateRequest, keyPair.getPublic(), consumer);
186174
} else if (certificateType.equals(GetCertificateRequestOptions.CertificateType.CLIENT)) {
187-
Consumer<X509Certificate[]> consumer = (t) -> {
175+
BiConsumer<X509Certificate, X509Certificate[]> consumer = (clientCert, caCertificates) -> {
188176
CertificateUpdateEvent certificateUpdateEvent =
189-
new CertificateUpdateEvent(keyPair, t[0], caCertificates);
177+
new CertificateUpdateEvent(keyPair, clientCert, caCertificates);
190178
getCertificateRequest.getCertificateUpdateConsumer().accept(certificateUpdateEvent);
191179
};
192180
subscribeToClientCertificateUpdatesNoCSR(getCertificateRequest, keyPair.getPublic(), consumer);
193181
}
194-
} catch (NoSuchAlgorithmException | KeyStoreException e) {
182+
} catch (NoSuchAlgorithmException e) {
195183
throw new CertificateGenerationException(e);
196184
}
197185
}
@@ -210,7 +198,7 @@ public void unsubscribeFromCertificateUpdates(GetCertificateRequest getCertifica
210198

211199
private void subscribeToServerCertificateUpdatesNoCSR(@NonNull GetCertificateRequest certificateRequest,
212200
@NonNull PublicKey publicKey,
213-
@NonNull Consumer<X509Certificate> cb)
201+
@NonNull BiConsumer<X509Certificate, X509Certificate[]> cb)
214202
throws CertificateGenerationException {
215203
CertificateGenerator certificateGenerator =
216204
new ServerCertificateGenerator(
@@ -236,7 +224,7 @@ private void subscribeToServerCertificateUpdatesNoCSR(@NonNull GetCertificateReq
236224

237225
private void subscribeToClientCertificateUpdatesNoCSR(@NonNull GetCertificateRequest certificateRequest,
238226
@NonNull PublicKey publicKey,
239-
@NonNull Consumer<X509Certificate[]> cb)
227+
@NonNull BiConsumer<X509Certificate, X509Certificate[]> cb)
240228
throws CertificateGenerationException {
241229
CertificateGenerator certificateGenerator =
242230
new ClientCertificateGenerator(

Diff for: ‎src/main/java/com/aws/greengrass/clientdevices/auth/certificate/ClientCertificateGenerator.java

+4-6
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,13 @@
2121
import java.time.Instant;
2222
import java.util.Date;
2323
import java.util.List;
24-
import java.util.function.Consumer;
24+
import java.util.function.BiConsumer;
2525
import java.util.function.Supplier;
2626

2727
public class ClientCertificateGenerator extends CertificateGenerator {
2828
private static final Logger logger = LogManager.getLogger(ClientCertificateGenerator.class);
2929

30-
private final Consumer<X509Certificate[]> callback;
30+
private final BiConsumer<X509Certificate, X509Certificate[]> callback;
3131

3232
/**
3333
* Constructor.
@@ -41,7 +41,7 @@ public class ClientCertificateGenerator extends CertificateGenerator {
4141
*/
4242
public ClientCertificateGenerator(X500Name subject,
4343
PublicKey publicKey,
44-
Consumer<X509Certificate[]> callback,
44+
BiConsumer<X509Certificate, X509Certificate[]> callback,
4545
CertificateStore certificateStore,
4646
CertificatesConfig certificatesConfig,
4747
Clock clock) {
@@ -77,9 +77,7 @@ public synchronized void generateCertificate(Supplier<List<String>> connectivity
7777
.kv("certExpiry", getExpiryTime())
7878
.log("New client certificate generated");
7979

80-
X509Certificate caCertificate = certificateStore.getCACertificate();
81-
X509Certificate[] chain = {certificate, caCertificate};
82-
callback.accept(chain);
80+
callback.accept(certificate, certificateStore.getCaCertificateChain());
8381
} catch (NoSuchAlgorithmException | OperatorCreationException | CertificateException | IOException
8482
| KeyStoreException e) {
8583
throw new CertificateGenerationException(e);

Diff for: ‎src/main/java/com/aws/greengrass/clientdevices/auth/certificate/ServerCertificateGenerator.java

+4-4
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,12 @@
2222
import java.util.ArrayList;
2323
import java.util.Date;
2424
import java.util.List;
25-
import java.util.function.Consumer;
25+
import java.util.function.BiConsumer;
2626
import java.util.function.Supplier;
2727

2828
public class ServerCertificateGenerator extends CertificateGenerator {
2929
private static final Logger logger = LogManager.getLogger(ServerCertificateGenerator.class);
30-
private final Consumer<X509Certificate> callback;
30+
private final BiConsumer<X509Certificate, X509Certificate[]> callback;
3131

3232
/**
3333
* Constructor.
@@ -41,7 +41,7 @@ public class ServerCertificateGenerator extends CertificateGenerator {
4141
*/
4242
public ServerCertificateGenerator(X500Name subject,
4343
PublicKey publicKey,
44-
Consumer<X509Certificate> callback,
44+
BiConsumer<X509Certificate, X509Certificate[]> callback,
4545
CertificateStore certificateStore,
4646
CertificatesConfig certificatesConfig,
4747
Clock clock) {
@@ -91,6 +91,6 @@ public synchronized void generateCertificate(Supplier<List<String>> connectivity
9191
.kv("certExpiry", getExpiryTime())
9292
.log("New server certificate generated");
9393

94-
callback.accept(certificate);
94+
callback.accept(certificate, certificateStore.getCaCertificateChain());
9595
}
9696
}

Diff for: ‎src/test/java/com/aws/greengrass/clientdevices/auth/CertificateManagerTest.java

+50-1
Original file line numberDiff line numberDiff line change
@@ -10,26 +10,30 @@
1010
import com.aws.greengrass.clientdevices.auth.api.GetCertificateRequest;
1111
import com.aws.greengrass.clientdevices.auth.api.GetCertificateRequestOptions;
1212
import com.aws.greengrass.clientdevices.auth.certificate.CertificateExpiryMonitor;
13+
import com.aws.greengrass.clientdevices.auth.certificate.CertificateGenerator;
1314
import com.aws.greengrass.clientdevices.auth.certificate.CertificateHelper;
1415
import com.aws.greengrass.clientdevices.auth.certificate.CertificateStore;
1516
import com.aws.greengrass.clientdevices.auth.certificate.CertificatesConfig;
1617
import com.aws.greengrass.clientdevices.auth.configuration.CDAConfiguration;
1718
import com.aws.greengrass.clientdevices.auth.connectivity.CISShadowMonitor;
1819
import com.aws.greengrass.clientdevices.auth.connectivity.ConnectivityInformation;
1920
import com.aws.greengrass.clientdevices.auth.exception.CertificateGenerationException;
21+
import com.aws.greengrass.clientdevices.auth.helpers.CertificateTestHelpers;
2022
import com.aws.greengrass.config.Topics;
2123
import com.aws.greengrass.dependency.Context;
2224
import com.aws.greengrass.security.SecurityService;
2325
import com.aws.greengrass.testcommons.testutilities.GGExtension;
2426
import com.aws.greengrass.testcommons.testutilities.TestUtils;
2527
import com.aws.greengrass.util.GreengrassServiceClientFactory;
2628
import com.aws.greengrass.util.Pair;
29+
import org.bouncycastle.operator.OperatorCreationException;
2730
import org.junit.jupiter.api.AfterEach;
2831
import org.junit.jupiter.api.Assertions;
2932
import org.junit.jupiter.api.BeforeEach;
3033
import org.junit.jupiter.api.Test;
3134
import org.junit.jupiter.api.extension.ExtendWith;
3235
import org.junit.jupiter.api.io.TempDir;
36+
import org.mockito.ArgumentCaptor;
3337
import org.mockito.Mock;
3438
import org.mockito.junit.jupiter.MockitoExtension;
3539

@@ -45,13 +49,15 @@
4549
import java.security.cert.X509Certificate;
4650
import java.time.Clock;
4751
import java.time.Instant;
52+
import java.util.ArrayList;
4853
import java.util.Arrays;
4954
import java.util.Date;
5055
import java.util.List;
5156
import java.util.concurrent.CompletableFuture;
5257
import java.util.concurrent.ExecutionException;
5358
import java.util.concurrent.TimeUnit;
5459
import java.util.concurrent.TimeoutException;
60+
import java.util.concurrent.atomic.AtomicReference;
5561
import java.util.function.Consumer;
5662
import java.util.stream.Stream;
5763

@@ -63,6 +69,7 @@
6369
import static org.junit.jupiter.api.Assertions.assertEquals;
6470
import static org.junit.jupiter.api.Assertions.assertNotEquals;
6571
import static org.mockito.Mockito.reset;
72+
import static org.mockito.Mockito.verify;
6673
import static org.mockito.Mockito.when;
6774

6875
@ExtendWith({MockitoExtension.class, GGExtension.class})
@@ -86,10 +93,13 @@ public class CertificateManagerTest {
8693
Path tmpPath;
8794

8895
private CertificateManager certificateManager;
96+
private CertificateStore certificateStore;
8997

9098
@BeforeEach
9199
void beforeEach() throws KeyStoreException {
92-
certificateManager = new CertificateManager(new CertificateStore(tmpPath, new DomainEvents()),
100+
certificateStore = new CertificateStore(tmpPath, new DomainEvents());
101+
102+
certificateManager = new CertificateManager(certificateStore,
93103
mockConnectivityInformation, mockCertExpiryMonitor, mockShadowMonitor,
94104
Clock.systemUTC(), clientFactoryMock, securityServiceMock);
95105

@@ -237,6 +247,45 @@ void GIVEN_clientCertRequest_WHEN_clientCertificateIsGenerated_THEN_canSuccessfu
237247
certificateManager.subscribeToCertificateUpdates(certificateRequest);
238248
}
239249

250+
@Test
251+
void GIVEN_caChain_WHEN_caChainChanges_THEN_subscribersGetLatestValues() throws NoSuchAlgorithmException,
252+
CertificateException, OperatorCreationException, IOException, CertificateGenerationException,
253+
KeyStoreException {
254+
AtomicReference<CertificateUpdateEvent> eventRef = new AtomicReference<>();
255+
256+
Pair<CompletableFuture<Void>, Consumer<CertificateUpdateEvent>> asyncCall =
257+
TestUtils.asyncAssertOnConsumer(eventRef::set, 1);
258+
GetCertificateRequestOptions requestOptions = new GetCertificateRequestOptions();
259+
requestOptions.setCertificateType(GetCertificateRequestOptions.CertificateType.CLIENT);
260+
GetCertificateRequest request =
261+
new GetCertificateRequest("com.aws.clients.Plugin", requestOptions, asyncCall.getRight());
262+
263+
KeyPair caAKeys = CertificateStore.newRSAKeyPair(2048);
264+
X509Certificate caA = CertificateTestHelpers.createRootCertificateAuthority("Root A", caAKeys);
265+
266+
KeyPair caBKeys = CertificateStore.newRSAKeyPair(2048);
267+
X509Certificate caB = CertificateTestHelpers.createRootCertificateAuthority("Root B", caBKeys);
268+
269+
270+
certificateStore.setCaPrivateKey(caAKeys.getPrivate());
271+
certificateStore.setCaCertificateChain(caA);
272+
certificateManager.subscribeToCertificateUpdates(request);
273+
274+
assertEquals(1, eventRef.get().getCaCertificates().length);
275+
assertEquals(CertificateHelper.toPem(caA), CertificateHelper.toPem( eventRef.get().getCaCertificates()[0]));
276+
277+
ArgumentCaptor<CertificateGenerator> generator = ArgumentCaptor.forClass(CertificateGenerator.class);
278+
verify(mockCertExpiryMonitor).addToMonitor(generator.capture());
279+
280+
certificateStore.setCaPrivateKey(caBKeys.getPrivate());
281+
certificateStore.setCaCertificateChain(caB);
282+
283+
// This part below just simulates the expiry monitor triggering expired certificates after the ca had changed
284+
generator.getValue().generateCertificate(ArrayList::new, "testing");
285+
assertEquals(1, eventRef.get().getCaCertificates().length);
286+
assertEquals(CertificateHelper.toPem(caB), CertificateHelper.toPem(eventRef.get().getCaCertificates()[0]));
287+
}
288+
240289
@Test
241290
void GIVEN_nullRequest_WHEN_subscribeToCertificateUpdates_THEN_throwsNPE() {
242291
Assertions.assertThrows(NullPointerException.class, () ->

Diff for: ‎src/test/java/com/aws/greengrass/clientdevices/auth/certificate/CertificateExpiryMonitorTest.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,7 @@ void GIVEN_certs_added_to_monitor_WHEN_certs_not_near_expiration_THEN_no_certs_a
183183
private CertificateGenerator monitorNewServerCert(Clock clock)
184184
throws NoSuchAlgorithmException, CertificateGenerationException {
185185
return monitorNewCert(key -> new ServerCertificateGenerator(
186-
SUBJECT, key, cert -> {
186+
SUBJECT, key, (cert, caChain) -> {
187187
}, certificateStore, certificatesConfig, clock));
188188
}
189189

@@ -199,7 +199,7 @@ private CertificateGenerator monitorNewServerCert(Clock clock)
199199
private CertificateGenerator monitorNewClientCert(Clock clock)
200200
throws NoSuchAlgorithmException, CertificateGenerationException {
201201
return monitorNewCert(key -> new ClientCertificateGenerator(
202-
SUBJECT, key, cert -> {
202+
SUBJECT, key, (cert, caChain) -> {
203203
}, certificateStore, certificatesConfig, clock));
204204
}
205205

Diff for: ‎src/test/java/com/aws/greengrass/clientdevices/auth/certificate/ClientCertificateGeneratorTest.java

+6-6
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929
import java.security.cert.X509Certificate;
3030
import java.time.Clock;
3131
import java.util.Collections;
32-
import java.util.function.Consumer;
32+
import java.util.function.BiConsumer;
3333

3434
import static org.hamcrest.MatcherAssert.assertThat;
3535
import static org.hamcrest.Matchers.is;
@@ -44,7 +44,7 @@ public class ClientCertificateGeneratorTest {
4444
= "CN=testCNC\\=USST\\=WashingtonL\\=SeattleO\\=Amazon.com Inc.OU\\=Amazon Web Services";
4545

4646
@Mock
47-
private Consumer<X509Certificate[]> mockCallback;
47+
private BiConsumer<X509Certificate, X509Certificate[]> mockCallback;
4848

4949
private PublicKey publicKey;
5050
private Topics configurationTopics;
@@ -81,7 +81,7 @@ public void GIVEN_ClientCertificateGenerator_WHEN_generateCertificate_THEN_certi
8181
assertThat(new KeyPurposeId(generatedCert.getExtendedKeyUsage().get(0)), is(KeyPurposeId.id_kp_clientAuth));
8282
assertThat(generatedCert.getPublicKey(), is(publicKey));
8383
verify(mockCallback, times(1))
84-
.accept(new X509Certificate[]{generatedCert, certificateStore.getCACertificate()});
84+
.accept(generatedCert, certificateStore.getCaCertificateChain());
8585

8686
certificateGenerator.generateCertificate(Collections::emptyList, "test");
8787
X509Certificate secondGeneratedCert = certificateGenerator.getCertificate();
@@ -90,7 +90,7 @@ public void GIVEN_ClientCertificateGenerator_WHEN_generateCertificate_THEN_certi
9090

9191
@Test
9292
void GIVEN_ClientCertificateGenerator_WHEN_rotation_disabled_THEN_only_initial_certificate_generated()
93-
throws KeyStoreException, CertificateGenerationException {
93+
throws CertificateGenerationException {
9494
configurationTopics.lookup(CertificatesConfig.PATH_DISABLE_CERTIFICATE_ROTATION).withValue(true);
9595

9696
// initial cert generation and some rotation attempts
@@ -99,7 +99,7 @@ void GIVEN_ClientCertificateGenerator_WHEN_rotation_disabled_THEN_only_initial_c
9999
certificateGenerator.generateCertificate(Collections::emptyList, "test");
100100

101101
// only the initial cert is generated, no rotation occurs
102-
verify(mockCallback, times(1)).accept(new X509Certificate[]{
103-
certificateGenerator.getCertificate(), certificateStore.getCACertificate()});
102+
verify(mockCallback, times(1)).accept(
103+
certificateGenerator.getCertificate(), certificateStore.getCaCertificateChain());
104104
}
105105
}

Diff for: ‎src/test/java/com/aws/greengrass/clientdevices/auth/certificate/ServerCertificateGeneratorTest.java

+8-5
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@
3131
import java.util.Collection;
3232
import java.util.Collections;
3333
import java.util.List;
34-
import java.util.function.Consumer;
34+
import java.util.function.BiConsumer;
3535

3636
import static org.hamcrest.MatcherAssert.assertThat;
3737
import static org.hamcrest.Matchers.is;
@@ -47,11 +47,12 @@ public class ServerCertificateGeneratorTest {
4747
= "CN=testCNC\\=USST\\=WashingtonL\\=SeattleO\\=Amazon.com Inc.OU\\=Amazon Web Services";
4848

4949
@Mock
50-
private Consumer<X509Certificate> mockCallback;
50+
private BiConsumer<X509Certificate, X509Certificate[]> mockCallback;
5151

5252
private PublicKey publicKey;
5353
private Topics configurationTopics;
5454
private CertificateGenerator certificateGenerator;
55+
private CertificateStore certificateStore;
5556

5657
@TempDir
5758
Path tmpPath;
@@ -60,7 +61,7 @@ public class ServerCertificateGeneratorTest {
6061
void setup() throws KeyStoreException, NoSuchAlgorithmException {
6162
X500Name subject = new X500Name(SUBJECT_PRINCIPAL);
6263
publicKey = CertificateStore.newRSAKeyPair().getPublic();
63-
CertificateStore certificateStore = new CertificateStore(tmpPath, new DomainEvents());
64+
certificateStore = new CertificateStore(tmpPath, new DomainEvents());
6465
certificateStore.update(TEST_PASSPHRASE, CertificateStore.CAType.RSA_2048);
6566
configurationTopics = Topics.of(new Context(), KernelConfigResolver.CONFIGURATION_CONFIG_KEY, null);
6667
CertificatesConfig certificatesConfig = new CertificatesConfig(configurationTopics);
@@ -82,7 +83,8 @@ void GIVEN_ServerCertificateGenerator_WHEN_generateCertificate_THEN_certificate_
8283
assertThat(generatedCert.getSubjectX500Principal().getName(), is(SUBJECT_PRINCIPAL));
8384
assertThat(generatedCert.getExtendedKeyUsage().get(0), is(KeyPurposeId.id_kp_serverAuth.getId()));
8485
assertThat(generatedCert.getPublicKey(), is(publicKey));
85-
verify(mockCallback, times(1)).accept(generatedCert);
86+
verify(mockCallback, times(1)).accept(
87+
generatedCert, certificateStore.getCaCertificateChain());
8688

8789
certificateGenerator.generateCertificate(Collections::emptyList, "test");
8890
X509Certificate secondGeneratedCert = certificateGenerator.getCertificate();
@@ -115,6 +117,7 @@ void GIVEN_ServerCertificateGenerator_WHEN_rotation_disabled_THEN_only_initial_c
115117
certificateGenerator.generateCertificate(Collections::emptyList, "test");
116118

117119
// only the initial cert is generated, no rotation occurs
118-
verify(mockCallback, times(1)).accept(certificateGenerator.getCertificate());
120+
verify(mockCallback, times(1)).accept(
121+
certificateGenerator.getCertificate(), certificateStore.getCaCertificateChain());
119122
}
120123
}

0 commit comments

Comments
 (0)