search

jitsi / otr4j

Off-The-Record messaging encryption written in pure Java
Apache License 2.0
30 stars 77 forks source link

Remove bc #5

Closed ibauersachs closed 4 years ago

cobratbq commented 4 years ago

@ibauersachs could you have a second look. I've resolved your suggestions and merged origin/master.

Note that I've copied the two asUnsignedByteArray functions with a remark in the javadoc that they originate from BC 1.65 which applies a MIT-style license. Someone of you guys should check if that's okay.

BC is now a test-scoped dependency for the cross-implementation comparison of DH, DSA, AES implementations.

I have not done client-to-client interop testing.

cobratbq commented 4 years ago

Fixes #4

ibauersachs commented 4 years ago

I'm confused. The test fail locally on Java 8 (jdk8u252-b09_x64, Adopt on Windows as well as Ubuntu). This makes sense with the explantion that NONEwithDSAinP1363Format is only available in Java 9+, but why do they run successfully on Github!?

java.lang.IllegalStateException: DSA signature algorithm is not available.

    at net.java.otr4j.crypto.OtrCryptoEngineImpl.sign(OtrCryptoEngineImpl.java:262)
    at net.java.otr4j.crypto.OtrCryptoEngineImplTest.testDSASigningInterop(OtrCryptoEngineImplTest.java:45)
    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:498)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
    at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
    at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
    at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
    at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
    at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
    at org.junit.runners.Suite.runChild(Suite.java:128)
    at org.junit.runners.Suite.runChild(Suite.java:27)
    at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
    at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
    at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
    at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
    at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
    at com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:33)
    at com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:230)
    at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:58)
Caused by: java.security.NoSuchAlgorithmException: NONEwithDSAinP1363Format Signature not available
    at java.security.Signature.getInstance(Signature.java:258)
    at net.java.otr4j.crypto.OtrCryptoEngineImpl.sign(OtrCryptoEngineImpl.java:257)
    ... 35 more

Exception in thread "Thread-3" java.lang.IllegalStateException: DSA signature algorithm is not available.
    at net.java.otr4j.crypto.OtrCryptoEngineImpl.sign(OtrCryptoEngineImpl.java:262)
    at net.java.otr4j.session.AuthContextImpl$MessageFactoryImpl.getRevealSignatureMessage(AuthContextImpl.java:127)
    at net.java.otr4j.session.AuthContextImpl.handleDHKeyMessage(AuthContextImpl.java:665)
    at net.java.otr4j.session.AuthContextImpl.handleReceivingMessage(AuthContextImpl.java:457)
    at net.java.otr4j.session.SessionImpl.transformReceiving(SessionImpl.java:465)
    at net.java.otr4j.session.DummyClient$MessageProcessor.process(DummyClient.java:126)
    at net.java.otr4j.session.DummyClient$MessageProcessor.run(DummyClient.java:147)
    at java.lang.Thread.run(Thread.java:748)
Caused by: java.security.NoSuchAlgorithmException: NONEwithDSAinP1363Format Signature not available
    at java.security.Signature.getInstance(Signature.java:258)
    at net.java.otr4j.crypto.OtrCryptoEngineImpl.sign(OtrCryptoEngineImpl.java:257)
    ... 7 more

Process finished with exit code -1
ibauersachs commented 4 years ago

Great. GitHub uses Zulu by default (see actions/setup-java#13), and Azul apparently decided to backport NONEwithDSAinP1363Format and others. While that's in general a nice addition to JDK8, it's entirely different from any other JDK8u build. 🤬

joschi commented 4 years ago

@ibauersachs joschi/setup-jdk is a drop-in replacement for actions/setup-java using AdoptOpenJDK instead of Zulu Community.

Maybe that helps.

cobratbq commented 4 years ago

@ibauersachs funny ... I used Zulu JDK 8 locally. I was already suspicious of the inP1363Format support, but figured it couldn't hurt and at least some JDK8 flavour made it available. I could not find clearly from which version on it was introduced. I'm guessing JDK 9 IIUC. @joschi that is interesting ... good to know such an alternative exists.

cobratbq commented 4 years ago

@ibauersachs what about doing a trial load of all necessary algorithms in a static initializer in OtrCryptoEngineImpl? That way, you know at class-load time whether or not all necessary support is available. I did this in the OTRv4 branch. It seems to be necessary to do this here too. We can throw an unchecked exception to prevent a partially functional class from being used.

ibauersachs commented 4 years ago

NONEwithDSAinP1363Format isn't available in BouncyCastle. So unless we'd agree to make otr4j Java 11+ only, we need to implement our own p1363 conversion.

ibauersachs commented 4 years ago

@joschi Thanks for the hint, but unfortunately your action doesn't seem to work on Windows runners. See e.g. https://github.com/dnsjava/dnsjava/actions/runs/165492259

joschi commented 4 years ago

Thanks for the hint, but unfortunately your action doesn't seem to work on Windows runners.

@ibauersachs The 32-bit Intel-compatible architecture is called x32 (to be in line with x64). See https://github.com/joschi/setup-jdk/pull/6 for an example.

In order not to hijack this PR it would be awesome if you could report your problems with joschi/setup-jdk at https://github.com/joschi/setup-jdk/issues/ if you want to use it. 😃

cobratbq commented 4 years ago

NONEwithDSAinP1363Format isn't available in BouncyCastle. So unless we'd agree to make otr4j Java 11+ only, we need to implement our own p1363 conversion.

IIUC inP1363Format is introduced in JDK9. But in any case, the "obvious" alternative is to convert to P1363 format. The format is simple enough, but the default format is ASN.1 (serialized in DER ... or something ... can't remember exactly) and AFAICT there is no ASN.1 parser publicly available in the JDK. I need to investigate further.

It's actually interesting to hear that BC does not provide this. Maybe it's an oversight.

ibauersachs commented 4 years ago

NONEwithDSAinP1363Format isn't available in BouncyCastle. So unless we'd agree to make otr4j Java 11+ only, we need to implement our own p1363 conversion.

IIUC inP1363Format is introduced in JDK9. But in any case, the "obvious" alternative is to convert to P1363 format. The format is simple enough, but the default format is ASN.1 (serialized in DER ... or something ... can't remember exactly) and AFAICT there is no ASN.1 parser publicly available in the JDK. I need to investigate further.

dnsjava has one of these conversions, but not sure how easy it is to rip out. Wasn't the original code before this PR in P1363 format?

It's actually interesting to hear that BC does not provide this. Maybe it's an oversight.

cobratbq commented 4 years ago

dnsjava has one of these conversions, but not sure how easy it is to rip out. Wasn't the original code before this PR in P1363 format?

Yes it was. The resulting signature provided by BouncyCastle consists of the individual components. The signature generated by JCA (without P1363) is a DER ASN.1-encoded binary-blob. So ... yeah ... the "easy" solution is to continue using BouncyCastle :-P

cobratbq commented 4 years ago

dnsjava has one of these conversions, but not sure how easy it is to rip out. Wasn't the original code before this PR in P1363 format?

@ibauersachs I've written conversion functions specific for DSA signature content. It should work for many JDKs and versions now.

ibauersachs commented 4 years ago

@cobratbq Do you think this is in a state that can be merged and released as 0.22?

cobratbq commented 4 years ago

Do you think this is in a state that can be merged and released as 0.22?

@ibauersachs actually, give me a bit of time. It bugs me that we have hardly anything for testing this. These changes are managable, but any significant change on this code base is an issue.

cobratbq commented 4 years ago

Okay, I'm not sure if I understand. You can review if you want to be certain, maybe you might have some feedback. I don't quite understand because you hinted at there being some urgency to the change, but now that you have everything to get rid of BC ... it's just sitting there.

As for the state ... given the low amount of tests we don't have as much certainty as we might want. This, however, is most easily fixed by switching to the in-progress OTRv4 work. But then we need to - at the very least - strip the API-level access to protocol version 4 to avoid accidentally stumbling into that control flow.

ibauersachs commented 4 years ago

Okay, I'm not sure if I understand. You can review if you want to be certain, maybe you might have some feedback. I don't quite understand because you hinted at there being some urgency to the change, but now that you have everything to get rid of BC ... it's just sitting there.

I'm sorry, I think you're referring to my wording soon. That was meant as a relativization, not as I need that by some fixed date. And I just haven't had time to work on Jitsi recently.

As for the state ... given the low amount of tests we don't have as much certainty as we might want. This, however, is most easily fixed by switching to the in-progress OTRv4 work. But then we need to - at the very least - strip the API-level access to protocol version 4 to avoid accidentally stumbling into that control flow.

I'll merge it and will release 0.22 as soon as time permits. We can deal with bugs as they appear.

cobratbq commented 4 years ago

I'm sorry, I think you're referring to my wording soon. That was meant as a relativization, not as I need that by some fixed date. And I just haven't had time to work on Jitsi recently.

Yes, you're right. Sorry, I indeed misrepresented your statement there.