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

fix: Improve ReceiveMessagesTest with log asserts #230

Merged
merged 2 commits into from
May 18, 2022
Merged

fix: Improve ReceiveMessagesTest with log asserts #230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a0efcd9
Improve ReceiveMessagesTest with log asserts
sdsantos May 9, 2022
f92a507
Handle MissingKeyException for 3rd party endpoints
sdsantos May 10, 2022
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
10 changes: 9 additions & 1 deletion lib/src/main/java/tech/relaycorp/awaladroid/messaging/IncomingMessage.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import tech.relaycorp.awaladroid.endpoint.ThirdPartyEndpoint
import tech.relaycorp.awaladroid.endpoint.UnknownFirstPartyEndpointException
import tech.relaycorp.awaladroid.endpoint.UnknownThirdPartyEndpointException
import tech.relaycorp.awaladroid.storage.persistence.PersistenceException
import tech.relaycorp.relaynet.keystores.MissingKeyException
import tech.relaycorp.relaynet.messages.InvalidMessageException
import tech.relaycorp.relaynet.messages.Parcel
import tech.relaycorp.relaynet.pki.CertificationPath
Expand Down Expand Up @@ -62,7 +63,14 @@ public class IncomingMessage internal constructor(
)

val context = Awala.getContextOrThrow()
val serviceMessage = context.endpointManager.unwrapMessagePayload(parcel)

val serviceMessage = try {
context.endpointManager.unwrapMessagePayload(parcel)
} catch (e: MissingKeyException) {
throw UnknownThirdPartyEndpointException(
"Missing third-party endpoint session keys"
)
}
if (serviceMessage.type == PDA_PATH_TYPE) {
processPDAPath(serviceMessage.content, sender, recipientEndpoint)
ack()
Expand Down
3 changes: 2 additions & 1 deletion lib/src/main/java/tech/relaycorp/awaladroid/messaging/ReceiveMessages.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import tech.relaycorp.awaladroid.GatewayException
import tech.relaycorp.awaladroid.GatewayProtocolException
import tech.relaycorp.awaladroid.common.Logging.logger
import tech.relaycorp.awaladroid.endpoint.UnknownFirstPartyEndpointException
import tech.relaycorp.awaladroid.endpoint.UnknownThirdPartyEndpointException
import tech.relaycorp.awaladroid.storage.persistence.PersistenceException
import tech.relaycorp.poweb.PoWebClient
import tech.relaycorp.relaynet.bindings.pdc.ClientBindingException
Expand Down Expand Up @@ -95,7 +96,7 @@ internal class ReceiveMessages(
} catch (exp: UnknownFirstPartyEndpointException) {
parcelCollection.disregard("Incoming parcel with invalid recipient", exp)
return@mapNotNull null
} catch (exp: UnknownFirstPartyEndpointException) {
} catch (exp: UnknownThirdPartyEndpointException) {
parcelCollection.disregard("Incoming parcel issues with invalid sender", exp)
return@mapNotNull null
} catch (exp: EnvelopedDataException) {
Expand Down
64 changes: 58 additions & 6 deletions lib/src/test/java/tech/relaycorp/awaladroid/messaging/ReceiveMessagesTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
package tech.relaycorp.awaladroid.messaging

import java.time.ZonedDateTime
import java.util.UUID
import kotlinx.coroutines.flow.collect
import kotlinx.coroutines.flow.flowOf
import kotlinx.coroutines.flow.toCollection
import kotlinx.coroutines.test.runBlockingTest
import nl.altindag.log.LogCaptor
import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
import org.junit.Test
Expand Down Expand Up @@ -35,6 +35,7 @@ internal class ReceiveMessagesTest : MockContextTestCase() {
private val subject = ReceiveMessages { pdcClient }

private val serviceMessage = ServiceMessage("type", "content".toByteArray())
private val logCaptor = LogCaptor.forClass(ParcelCollection::class.java)

@Test
fun receiveParcelSuccessfully() = runBlockingTest {
Expand Down Expand Up @@ -95,15 +96,14 @@ internal class ReceiveMessagesTest : MockContextTestCase() {
@Test
fun receiveInvalidParcel_ackedButNotDeliveredToApp() = runBlockingTest {
val invalidParcel = Parcel(
recipientAddress = UUID.randomUUID().toString(),
payload = "1234".toByteArray(),
senderCertificate = PDACertPath.PRIVATE_ENDPOINT,
creationDate = ZonedDateTime.now().plusDays(1)
recipientAddress = KeyPairSet.PRIVATE_ENDPOINT.public.privateAddress,
payload = "".toByteArray(),
senderCertificate = PDACertPath.PRIVATE_ENDPOINT
)
var ackWasCalled = false
val parcelCollection = ParcelCollection(
parcelSerialized = invalidParcel.serialize(KeyPairSet.PRIVATE_ENDPOINT.private),
trustedCertificates = emptyList(),
trustedCertificates = emptyList(), // sender won't be trusted
ack = { ackWasCalled = true }
)
val collectParcelsCall = CollectParcelsCall(Result.success(flowOf(parcelCollection)))
Expand All @@ -115,6 +115,7 @@ internal class ReceiveMessagesTest : MockContextTestCase() {
assertTrue(collectParcelsCall.wasCalled)
assertTrue(messages.isEmpty())
assertTrue(ackWasCalled)
assertTrue(logCaptor.warnLogs.contains("Invalid incoming parcel"))
}

@Test
Expand All @@ -134,6 +135,47 @@ internal class ReceiveMessagesTest : MockContextTestCase() {
assertTrue(collectParcelsCall.wasCalled)
assertTrue(messages.isEmpty())
assertTrue(ackWasCalled)
assertTrue(logCaptor.warnLogs.contains("Malformed incoming parcel"))
}

@Test
fun receiveParcelWithUnknownRecipient_ackedButNotDeliveredToApp() = runBlockingTest {
val channel = createEndpointChannel(RecipientAddressType.PUBLIC)
val parcel = buildParcel(channel)
var ackWasCalled = false
val parcelCollection = parcel.toParcelCollection { ackWasCalled = true }
val collectParcelsCall = CollectParcelsCall(Result.success(flowOf(parcelCollection)))
pdcClient = MockPDCClient(collectParcelsCall)

channel.firstPartyEndpoint.delete()

val messages = subject.receive().toCollection(mutableListOf())
Comment on lines +147 to +152
Copy link
Member

Choose a reason for hiding this comment

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

As you noticed, this is failing with the error: MissingKeyException: There is no session key for 03d6f89f597e881f275831faaa7abb92553f18fc563e53494372db18461533178.

That's because this test currently simulates a situation where the 1st party endpoint exists when we start collecting parcels, but no longer exists by the time we get the parcel. However, I think what you meant to simulate is the scenario where the 1st party endpoint didn't even exist when parcel collection started -- that is, parcel.recipientAddress != channel.firstPartyEndpoint.privateAddress is what you want to replicate.

I think the former is worth testing to make sure it's handled (I don't think I considered this scenario but it's great we came across it as it could certainly happen in real life!)

I'm not sure we can test the latter, but I could be wrong: If the 1st party endpoint didn't exist when parcel collection started (because it never existed or it was deleted), then that should/will throw an InvalidMessageException in:

awala-endpoint-android/lib/src/main/java/tech/relaycorp/awaladroid/messaging/ReceiveMessages.kt

Lines 84 to 92 in bad9585

val parcel = try {
parcelCollection.deserializeAndValidateParcel()
} catch (exp: RAMFException) {
parcelCollection.disregard("Malformed incoming parcel", exp)
return@mapNotNull null
} catch (exp: InvalidMessageException) {
parcelCollection.disregard("Invalid incoming parcel", exp)
return@mapNotNull null
}

... because the sender's PDA wouldn't have been issued by any id certificate of our 1st party endpoints.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

By "I think the former is worth testing to make sure it's handled" I mean add a catch for MissingKeyException


assertTrue(pdcClient.wasClosed)
assertTrue(collectParcelsCall.wasCalled)
assertTrue(messages.isEmpty())
assertTrue(ackWasCalled)
assertTrue(logCaptor.warnLogs.contains("Incoming parcel with invalid recipient"))
}

@Test
fun receiveParcelWithUnknownSender_ackedButNotDeliveredToApp() = runBlockingTest {
val channel = createEndpointChannel(RecipientAddressType.PUBLIC)
val parcel = buildParcel(channel)
var ackWasCalled = false
val parcelCollection = parcel.toParcelCollection { ackWasCalled = true }
val collectParcelsCall = CollectParcelsCall(Result.success(flowOf(parcelCollection)))
pdcClient = MockPDCClient(collectParcelsCall)

channel.thirdPartyEndpoint.delete()

val messages = subject.receive().toCollection(mutableListOf())

assertTrue(pdcClient.wasClosed)
assertTrue(collectParcelsCall.wasCalled)
assertTrue(messages.isEmpty())
assertTrue(ackWasCalled)
assertTrue(logCaptor.warnLogs.contains("Incoming parcel issues with invalid sender"))
}

@Test
Expand Down Expand Up @@ -168,6 +210,11 @@ internal class ReceiveMessagesTest : MockContextTestCase() {
assertTrue(pdcClient.wasClosed)
assertTrue(messages.isEmpty())
assertTrue(ackWasCalled)
assertTrue(
logCaptor.warnLogs.contains(
"Failed to decrypt parcel; sender might have used wrong key"
)
)
}

@Test
Expand Down Expand Up @@ -200,6 +247,11 @@ internal class ReceiveMessagesTest : MockContextTestCase() {
assertTrue(pdcClient.wasClosed)
assertTrue(messages.isEmpty())
assertTrue(ackWasCalled)
assertTrue(
logCaptor.warnLogs.contains(
"Incoming parcel did not encapsulate a valid service message"
)
)
}

private fun buildParcel(channel: EndpointChannel) = Parcel(
Expand Down