search

MovingBlocks / FacadeServer

A headless facade that acts as a game host and provides web-based administration. Automation category: Terasology Facade. See https://forum.terasology.org/threads/facadeserver-headless-server-with-web-interface.1906
Apache License 2.0
4 stars 11 forks source link

Unit test "AuthenticationHandshakeHandlerTest.testBadSignature" isn't fully deterministic and can (rarely) fail #7

Closed gianluca-nitti closed 7 years ago

gianluca-nitti commented 7 years ago

Adding this mainly as a note to myself for when I have the time to fix it.

As the title says, the unit test AuthenticationHandshakeHandlerTest.testBadSignature is not fully deterministic (it uses some randomly generated values) and can sometimes fail with the following exception:

java.lang.Exception: Unexpected exception, expected<org.terasology.web.authentication.AuthenticationFailedException> but was<java.lang.ArithmeticException>

    at org.junit.internal.runners.statements.ExpectException.evaluate(ExpectException.java:28)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
    at org.junit.runners.Suite.runChild(Suite.java:128)
    at org.junit.runners.Suite.runChild(Suite.java:27)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
    at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
    at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
    at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:51)
    at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
    at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)
Caused by: java.lang.ArithmeticException: BigInteger not invertible.
    at java.math.MutableBigInteger.modInverse(MutableBigInteger.java:2106)
    at java.math.MutableBigInteger.mutableModInverse(MutableBigInteger.java:1982)
    at java.math.BigInteger.modInverse(BigInteger.java:2905)
    at java.math.BigInteger.modPow(BigInteger.java:2501)
    at sun.security.rsa.RSACore.priCrypt(RSACore.java:150)
    at sun.security.rsa.RSACore.rsa(RSACore.java:124)
    at sun.security.rsa.RSASignature.engineSign(RSASignature.java:175)
    at java.security.Signature$Delegate.engineSign(Signature.java:1207)
    at java.security.Signature.sign(Signature.java:579)
    at org.terasology.identity.PrivateIdentityCertificate.sign(PrivateIdentityCertificate.java:71)
    at org.terasology.web.authentication.AuthenticationHandshakeHandlerTest.testBadSignature(AuthenticationHandshakeHandlerTest.java:77)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:497)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
    at org.junit.internal.runners.statements.ExpectException.evaluate(ExpectException.java:19)
    ... 22 more

I thought that the random generator was properly seeded to always produce the same numbers but looks like it isn't. This failure happened about 2 or 3 times in two months, always when I was working on completely separate classes from the one this code tests and I was just running all the tests before committing (which I do multiple times a day when I work on the server).

msteiger commented 7 years ago

Is this one still relevant?

gianluca-nitti commented 7 years ago

Yes. I did some further investigation and I found out that the non-determinism is due to the usage of CertificateGenerator, i.e. the signature byte arrays are generated by a fixed-seed RNG, but the certificates to sign (private) and verify (public) them are generated by the CertificateGenerator class thus always different.

To make the test deterministic I could make a mock object for the CertificateGenerator class which always returns the same certs.

But the test is actually showing an incorrect behavior in the code which should probably be corrected. I think the way to go is splitting this test case in multiple deterministic ones that cover both the cases that are now failing and the ones which are succeeding, then find a way to make them all pass.

gianluca-nitti commented 7 years ago

Update - I did more debugging and found out that even by providing fixed client and server certificate pairs the test can fail, and the real cause of the non-determinism of this test is the random handshake data generated by AuthenticationHandshakeHandlerImpl (the class being tested) to be signed by the client.

The good news is that I noticed that the exception is raised when signing the handshake message and not when verifying it, so the problematic part is when the test simulates a client; the server's behavior, in my understanding, is correct. It's not possible to send a "bad" handshake reply from a client to the server which would crash the server; it's a client that, given an invalid private certificate, may fail to use it to sign the handshake reply.

At this point - unless anyone has a better idea - I'm considering just removing this test method; I don't know how I could make it fully deterministic since generating random handshakes to be signed by the client (using the private certificate) is part of the expected behavior of the class being tested. I noticed that the "equivalent" code (for regular Terasology clients) in the main repository hasn't any tests.