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.

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

Upgrading Conscrypt to 2.5.1 #7342

Merged
merged 2 commits into from Sep 10, 2020
Merged

Upgrading Conscrypt to 2.5.1 #7342

merged 2 commits into from Sep 10, 2020

Conversation

veblush
Copy link
Contributor

@veblush veblush commented Aug 20, 2020

This PR has two things;

…es in the ALTS code. (cl/308901367)"

This reverts commit a7bca23.
@veblush
Copy link
Contributor Author

veblush commented Aug 20, 2020

Both MacOS and Windows failed with

io.grpc.alts.internal.AesGcmAeadCrypterTest > getConscrypt_worksWhenConscryptIsAvailable FAILED
    expected not to be: null
        at io.grpc.alts.internal.AesGcmAeadCrypterTest.getConscrypt_worksWhenConscryptIsAvailable(AesGcmAeadCrypterTest.java:32)

but why?

@ejona86
Copy link
Member

ejona86 commented Aug 24, 2020

We need to be careful and make sure not to squash this when merging.

Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

@voidzcy, we'll probably also want to revert #6962 , or maybe change the wording to say "use Conscrypt 2.5.0 or later because of BLAH"

@ejona86
Copy link
Member

ejona86 commented Aug 24, 2020

Looking at the Windows build, where we can download the test.xml, I see:

Aug 20, 2020 9:45:34 AM io.grpc.alts.internal.AesGcmAeadCrypter getConscrypt
WARNING: Could not load Conscrypt. Will try slower JDK implementation. This may be because the JDK is older than Java 7 update 121 or Java 8 update 111. If so, please update
java.lang.SecurityException: JCE cannot authenticate the provider Conscrypt
	at javax.crypto.Cipher.getInstance(Cipher.java:657)
	at io.grpc.alts.internal.AesGcmAeadCrypter.getConscrypt(AesGcmAeadCrypter.java:134)
	at io.grpc.alts.internal.AesGcmAeadCrypter.<clinit>(AesGcmAeadCrypter.java:43)
	at io.grpc.alts.internal.AesGcmAeadCrypterTest.getConscrypt_worksWhenConscryptIsAvailable(AesGcmAeadCrypterTest.java:32)
	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: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.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.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.runTestClass(JUnitTestClassExecutor.java:110)
	at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.execute(JUnitTestClassExecutor.java:58)
	at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.execute(JUnitTestClassExecutor.java:38)
	at org.gradle.api.internal.tasks.testing.junit.AbstractJUnitTestClassProcessor.processTestClass(AbstractJUnitTestClassProcessor.java:62)
	at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.processTestClass(SuiteTestClassProcessor.java:51)
	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.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
	at org.gradle.internal.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:33)
	at org.gradle.internal.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:94)
	at com.sun.proxy.$Proxy2.processTestClass(Unknown Source)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.processTestClass(TestWorker.java:119)
	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.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
	at org.gradle.internal.remote.internal.hub.MessageHubBackedObjectConnection$DispatchWrapper.dispatch(MessageHubBackedObjectConnection.java:182)
	at org.gradle.internal.remote.internal.hub.MessageHubBackedObjectConnection$DispatchWrapper.dispatch(MessageHubBackedObjectConnection.java:164)
	at org.gradle.internal.remote.internal.hub.MessageHub$Handler.run(MessageHub.java:414)
	at org.gradle.internal.concurrent.ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:64)
	at org.gradle.internal.concurrent.ManagedExecutorImpl$1.run(ManagedExecutorImpl.java:48)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at org.gradle.internal.concurrent.ThreadFactoryImpl$ManagedThreadRunnable.run(ThreadFactoryImpl.java:56)
	at java.lang.Thread.run(Thread.java:748)
Caused by: java.util.jar.JarException: file:/C:/Users/builder/.gradle/caches/modules-2/files-2.1/org.conscrypt/conscrypt-openjdk-uber/2.5.0/c95f61a8010c0107012a7dfad62aef6ad5f791aa/conscrypt-openjdk-uber-2.5.0.jar has unsigned entries - org/conscrypt/AbstractConscryptEngine.class
	at javax.crypto.JarVerifier.verifySingleJar(JarVerifier.java:502)
	at javax.crypto.JarVerifier.verifyJars(JarVerifier.java:363)
	at javax.crypto.JarVerifier.verify(JarVerifier.java:289)
	at javax.crypto.JceSecurity.verifyProviderJar(JceSecurity.java:164)
	at javax.crypto.JceSecurity.getVerificationResult(JceSecurity.java:190)
	at javax.crypto.Cipher.getInstance(Cipher.java:653)
	... 50 more

@ejona86
Copy link
Member

ejona86 commented Aug 24, 2020

It looks like the conscrypt-openjdk-uber.jar is broken. I don't see signatures in the MANIFEST.MF. I do see them in 2.2.1, but they are missing in 2.4.0 and 2.5.0. It looks like the new release system is broken.

I don't see an issue filed for Conscrypt, so I don't know whether this means very few people upgraded Conscrypt to 2.4.0+ or very few people are using Oracle's JRE since they became a PITA to use. It's unclear if we should consider this a blocker for upgrading Conscrypt.

@veblush
Copy link
Contributor Author

veblush commented Aug 24, 2020

Eric, thank you for the investigation. So this is caused by the Conscrypt JAR which was not correctly signed on Windows and Mac. But Android and Linux were done correctly. Right?

BTW, I'll keep the commits.

@veblush
Copy link
Contributor Author

veblush commented Aug 24, 2020

@prbprbprb Can you check this? If uber package is actually broken, it probably needs the new release to fix it.

@veblush
Copy link
Contributor Author

veblush commented Aug 24, 2020

One thing that I can do is splitting this PR to revert #7049 only first and bump the Conscrypt to 2.5.0 or later. From this, at least we can benefit the recent optimizations on Linux by upgrading Conscrypt to 2.5.0 separately.

@voidzcy
Copy link
Contributor

voidzcy commented Aug 24, 2020

Link to #6684. That can be closed once this is merged.

@ejona86
Copy link
Member

ejona86 commented Aug 24, 2020

So this is caused by the Conscrypt JAR which was not correctly signed on Windows and Mac. But Android and Linux were done correctly.

No. All of them are broken. What happened is we are using OpenJDK on Linux, which is fine with this. If we used Oracle JRE on Linux it would have failed there too. I'm not 100% confident about that, but that's what it's been like in the past when this happened.

From this, at least we can benefit the recent optimizations on Linux by upgrading Conscrypt to 2.5.0 separately.

Nope. I think it is busted on Linux as well, if you use Oracle JRE.

@prbprbprb
Copy link

Sorry for the slow update! Yes, indeed it looks all the artifacts for 2.4.0 and 2.5.0 are completely unsigned (despite re-assuring output from Gradle when publishing them). It's a little surprising that nobody noticed before and we'll certainly add something to our release checklist to prevent this happening again.

Looking at a fix now.

@kruton FYI

@ejona86
Copy link
Member

ejona86 commented Sep 8, 2020

@prbprbprb, Gradle supports class signing in META-INF? I don't see any documentation for that. The most common signing for gradle is creating asc files for the artifacts, which is required for Maven Central. So the JAR is signed, but not the class files 😄

@prbprbprb
Copy link

Gradle supports class signing in META-INF?

Nope.

Historically Conscrypt has done this with a hook which uses jarsigner to sign the jars right before the .asc openpgp signatures get generated, but it got disconnected and broken when we migrated to a new Maven plugin for release 2.4.0. Should be fixed by google/conscrypt#887 and as soon as that lands I'll push a 2.5.1 release with it.

@veblush
Copy link
Contributor Author

veblush commented Sep 9, 2020

@prbprbprb Thanks! Once 2.5.1 shows up on the maven repo, I'll update this PR to have 2.5.1 and see whether it passes the tests!

@prbprbprb
Copy link

Conscrypt 2.5.1 is built for openjdk and currently going through the Maven Central release process. Android and Uberjar builds will follow later this evening, but you should at least be able to pick this one up soon.

(And yes, I double-checked all the jars had signature data in their META-INF before hitting "release" ;)

@veblush veblush changed the title Upgrading Conscrypt to 2.5.0 Upgrading Conscrypt to 2.5.1 Sep 10, 2020
@ejona86
Copy link
Member

ejona86 commented Sep 10, 2020

2.5.1 is passing all the tests! Merging!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants