Skip to content

Commit

Permalink
fix: limit client IDs to 65,535
Browse files Browse the repository at this point in the history
  • Loading branch information
MikeDombo committed Mar 19, 2024
1 parent 22a4550 commit ab2aba1
Show file tree
Hide file tree
Showing 6 changed files with 16 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public void listen() {
* @param event Certificate authority configuration change event
*/
@Override
public void accept(CAConfigurationChanged event) {
public synchronized void accept(CAConfigurationChanged event) {
CAConfiguration configuration = event.getConfiguration();

try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,6 @@ public ConfigureCustomCertificateAuthority(CertificateManager certificateManager

@Override
public Void apply(CAConfiguration configuration) throws UseCaseException {
// TODO: We need to synchronize the changes that configuration has on the state of the service. There is
// a possibility that 2 threads run different use cases and change the certificate authority concurrently
// causing potential race conditions

try {
logger.info("Configuring custom certificate authority.");
// NOTE: We will pull the configureCustomCA out of the certificate Manager to here
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,6 @@ public Void apply(CAConfiguration configuration) throws UseCaseException {
// TODO: We should not be passing the entire configuration just what changed. We are just doing it for
// its convenience but eventually syncing the runtime config can be its own use case triggered by events.

// TODO: We need to synchronize the changes that configuration has on the state of the service. There is
// a possibility that 2 threads run different use cases and change the certificate authority concurrently
// causing potential race conditions

logger.info("Configuring Greengrass managed certificate authority.");

try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ public CreateIoTThingSession(ThingRegistry thingRegistry, CertificateRegistry ce
*/
@Override
public Session apply(CreateSessionDTO dto) throws AuthenticationException {
if (dto.getThingName() != null && dto.getThingName().length() > 65_535) {
throw new AuthenticationException("Thing name is too long");
}

Certificate certificate = getActiveCertificateFromRegistry(dto);
String thingName = dto.getThingName();
Thing thing = thingRegistry.getOrCreateThing(thingName);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public Session createSession(Map<String, String> credentialMap) throws Authentic
}

private Session createIotThingSession(MqttCredential mqttCredential) throws AuthenticationException {
// NOTE: We should remove calling this useCase from here, but for now it serves its purpose. We will
// NOTE: We should remove calling this useCase from here, but for now it serves its purpose. We will
// refactor this later
CreateIoTThingSession useCase = useCases.get(CreateIoTThingSession.class);
CreateSessionDTO command = new CreateSessionDTO(mqttCredential.clientId, mqttCredential.certificatePem);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import java.util.Optional;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.notNullValue;
import static org.mockito.ArgumentMatchers.any;
Expand Down Expand Up @@ -100,6 +101,15 @@ void GIVEN_credentialsWithInvalidCertificate_WHEN_createSession_THEN_throwsAuthe
Assertions.assertThrows(AuthenticationException.class, () -> mqttSessionFactory.createSession(credentialMap));
}

@Test
void GIVEN_credentialsWithLongClientId_WHEN_createSession_THEN_throwsAuthenticationException() {
AuthenticationException ex = Assertions.assertThrows(AuthenticationException.class,
() -> mqttSessionFactory.createSession(
ImmutableMap.of("certificatePem", "PEM", "clientId", new String(new byte[65536]), "username",
"", "password", "")));
assertThat(ex.getMessage(), containsString("too long"));
}

@Test
void GIVEN_credentialsWithCertificate_WHEN_createSession_AND_cloudError_THEN_throwsAuthenticationException()
throws InvalidCertificateException, CloudServiceInteractionException {
Expand Down

0 comments on commit ab2aba1

Please sign in to comment.