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

Reduce exposure to the deprecated for-removal javax.security.cert APIs #1128

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

eirbjo
Copy link
Contributor

@eirbjo eirbjo commented Apr 15, 2023

Efforts are underway in the OpenJDK project to remove the long deprecated-for-removal classes in the package javax.security.cert. These classes were introduced for backwards compatibility concerns with the unbundled JSSE release for JDK 1.2/1.3, but their use have been discouraged since they were introduced.

It would be good to update Conscrypt to reduce and contain dependencies on these archaic APIs.

See https://bugs.openjdk.org/browse/JDK-8227024 and the corresponding CSR https://bugs.openjdk.org/browse/JDK-8227395

Changes:

  • Adding default method ConscryptSession.getPeerCertificateChain which throws an UnsupportedOperationException like Java 15.
  • Update ActiveSession, ExternalSession, Java7ExtendedSSLSession, SessionSnapshot, SSLNullSession to remove the getPeerCertificateChain implementations.
  • Remove the now unsed SSLUtils.toCertificateChain utility method
  • Remove the SSLSessionTest.test_SSLSession_getPeerCertificateChain test method
  • Update SSLSocketVersionCompatibilityTest to not call HandshakeCompletedEvent.getPeerCertificateChain and to not assert on the results obtain by calling this method.

@prbprbprb
Copy link
Collaborator

prbprbprb commented Apr 16, 2023

Thanks for this! It's been on our radar for a while but unfortunately isn't as simple as this PR.

I think for OpenJDK and standalone Android builds, it's probably fine to simply drop support for the getPeerCertificateChain() API at a release boundary. Caveat emptor etc.

However, the Android platform hasn't officially deprecated this API yet, there's just a note in the docs not to use it for new apps. We might be able to get some metrics to see if any apps actually use this in reality. If so, then the best we can probably do is officially deprecate it and add logging at an Android SDK boundary (and we're too late for Android 14) and then remove it at the next one. In the meantime we'll have to support it.

And of course it's worse than that. Thanks to the magic of Mainline, any change we make here will get shipped to all Mainline devices running Android 11 or greater and a few devices running Android 10, and we absolutely can't just drop support on those devices if it's in active use, so we'll need to gate it at runtime.

Soooooo..... Needs a bit of thinking through, but I think at the next release we can start making getPeerCertificateChain() throw on Android (standalone) and OpenJDK and move most of the support to the Android platform build. Might need to keep stashing the javax X509Certificate chains in the various session classes but it moves us in the right direction.

I'm guessing you guys aren't terribly interested in doing the Android Platform work? Happy to review it if you are though!

@prbprbprb
Copy link
Collaborator

prbprbprb commented Apr 16, 2023

and we're too late for Android 14

Actually that might not be true, I'll check and update this thread tomorrow.

@prbprbprb prbprbprb self-requested a review April 16, 2023 17:47
@prbprbprb prbprbprb self-assigned this Apr 16, 2023
@eirbjo
Copy link
Contributor Author

eirbjo commented Apr 16, 2023

Thanks for this! It's been on our radar for a while but unfortunately isn't as simple as this PR.

I had a feeling this could be the case :-)

I'm reaching out to various projects (BouncyCastle, Tomcat, Netty, Vert.x, Underrow, etc), and I'm doing it mainly in the form of PRs because I think that makes the discussion more focused, practical and actionable. So if this PR isn't merged as-is, I'm still happy if it can help bring the situation forward. (We seem to be in a bit of a stalemate situation, where OpenJDK is waiting for the ecosystem to reduce their exposure to these APIs and the ecosystem seems not sufficiently motivated to do much until OpenJDK actually ships a JDK with the removal. This is not very glamorous work on either side of the fence. It's easier to do nothing..

Considering this, I think you can understand I'm very excited about your response and involvement. Thanks!

I think for OpenJDK and standalone Android builds, it's probably fine to simply drop support for the getPeerCertificateChain() API at a release boundary. Caveat emptor etc.

Am I right that you are saying that OpenJDK has your/Conscrypt's blessing/support to go ahead and remove this method with the related packages in some not-so-far future release? That would be a great thing to bring home to the OpenJDK discussion.

Soooooo..... Needs a bit of thinking through, but I think at the next release we can start making getPeerCertificateChain() throw on Android (standalone) and OpenJDK and move most of the support to the Android platform build. Might need to keep stashing the javax X509Certificate chains in the various session classes but it moves us in the right direction.

OpenJDK's introduction of a default SSLSession.getPeerCertificateChain throwing UnsupportedOperationException in Java 15 seems to have become a key enabler for projects dropping dependencies on this API:

https://bugs.openjdk.org/browse/JDK-8241039

It would probably be good to have Android align in behaviour here.

I'm guessing you guys aren't terribly interested in doing the Android Platform work? Happy to review it if you are though!

Let me be honest: I have very limited experience with Android, especially on the system side, so I don't even think I understand the terms "standalone Android builds", "Mainline" etc.

If you can point me to the relevant code bases, I would be happy to take a look, but don't keep your hopes to high that will produce production ready code :-)

Again, thanks for your response and willingness to investigate and discuss this matter. Highly appreciated!

@prbprbprb
Copy link
Collaborator

prbprbprb commented Apr 16, 2023

Am I right that you are saying that OpenJDK has your/Conscrypt's blessing/support to go ahead and remove this method with the related packages in some not-so-far future release?

TBH we've never assumed that upstream OpenJDK would worry about us when making breaking changes. :) I don't mean that in a negative way, just that your priorities are not the same as ours and so it's up to us to react as needed.

I think we're all on the same page that these older classes and APIs need to go, but our problem is that we need to continue supporting a wide ecosystem, so it probably helps to clarify some of the terminology you asked about:

Conscrypt exists in multiple niches, but the 3 main ones are:

  • As an OpenJDK library, shipped via Maven or built from source, commonly used with Jetty and okhttp and expected to work as a drop-in replacement alongside the Sun Providers. The last public release supports Java 7 onwards, the next one (which is embarrassingly late) will support 8+.
  • As a standalone Android library, i.e. one which developers can bundle with their app and is expected to work as a drop-in replacement for the version that ships with the Android platform. This allows developers to ship an up-to-date Conscrypt with their app even if the target device is running an old version of Android. The last public release will work with apps targeting Android 3 and later. The next release will support Android 5 onwards.
  • In the Android platform itself. Android does not ship any of the Sun Providers and their functionality is provided by a combination of Conscrypt and Bouncy Castle Providers, plus some other complexity. This provides the implementations of many of the public APIs which Android imports from OpenJDK. As of the Android 14 beta releases currently shipping, those public APIs are a mix of Java 8, Java 11 and Java 17 but are converging on 17 over time. Project Mainline modularises a bunch of Android system components, including Conscrypt, allowing new versions to be shipped to older devices without needing to upgrade the whole OS. The support window in this case is effectively Android 11 onwards, i.e. the version of Conscrypt that ships with Android 14 will also ship (as a binary module) to those older releases, so as you can imagine API stability and app compatibility take up a lot of our thinking.

It would probably be good to have Android align in behaviour here.

Totally agreed, Android just tends to move slower in order not to break apps using public APIs from the SDK without warning.

If you can point me to the relevant code bases, I would be happy to take a look, but don't keep your hopes to high that will produce production ready code :-)

Yeah, I totally get that's not your job. :)

But FYI Conscrypt standalone and for OpenJDK build directly from the Github source. Differences are handled by the Platform.java in android/ and openjdk/ and multiple Gradle sub-projects.

Conscrypt in the Android platform builds from the /external/conscrypt tree of the Android Open Source Project. That is essentially the same as the Github source but there's an additional step which repackages the source into a different namespace and builds it using the Android build system. However given Conscrypt is a core part of the OS there's a bunch of complexity around development and testing... e.g. because it provides the backing for many public APIs, the tests are spread around the place. Android platform specific code is in platform/. If you genuinely want to work on that then please feel free to DM me for more detailed instructions.

@eirbjo
Copy link
Contributor Author

eirbjo commented Apr 17, 2023

Yeah, I totally get that's not your job. :)

To be clear: I'm not affiliated with Oracle or any other OpenJDK vendor, I'm simply an individual contributor to the project. As an individual, it might actually be easier for me to reach out and contribute to projects in the ecosystem because of reduced organizational and legal concerns. (IANAL)

@prbprbprb
Copy link
Collaborator

Yeah, I really meant "role" there, i.e. we don't expect upstream to solve our problems for us, but we won't complain if you do.

Anyway, seems like the SDK for Android 14 / API level 34 is past the point where we can officially deprecate those APIs, so we'll need to support them on Android for at last a year, more like two depending on usage. And I'm pursuing a couple of other angles to try and determine usage (if any)... e.g. errorprone warnings in Android Studio and metric collection.

@prbprbprb
Copy link
Collaborator

Oh, also I can see a way through this which means that only the Android platform build is implementing/exposed to those APIs without it being too fugly. I think it makes sense to drop some of the Java 7 workarounds first though, and probably makes sense to get the next public release done before dropping the Java 7 stuff.

@eirbjo
Copy link
Contributor Author

eirbjo commented Apr 20, 2023

Oh, also I can see a way through this which means that only the Android platform build is implementing/exposed to those APIs without it being too fugly.

Sounds promising! I'm not sure if/how Android factors into considerations to remove these APIs at the OpenJDK. It seems that Android Platform is sort-of, sort-of-not related to OpenJDK. But I'm very happy if you can reduce / remove your exposure to these APIs in your OpenJDK code.

@prbprbprb
Copy link
Collaborator

It seems that Android Platform is sort-of, sort-of-not related to OpenJDK

Yeah. Android's runtime virtual machine is all its own, but a significant amount of its Java core libraries come from OpenJDK.

There are a bunch of complexity and exceptions, one is that Android doesn't ship any of the Sun security Providers, so Conscrypt is the default implementation of the APIs we're discussing here (in turn implemented on top of ye olde javax.security.X509Certificate from OpenJDK). So until they're dropped from Android's SDK, we have to keep a working implementation.

As nobody should be using these anyway, I'm trying to accelerate the process of getting them dropped from the SDK.

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

Successfully merging this pull request may close these issues.

2 participants