The net.i2p.crypto.eddsa 0.3.0 bundle has bad OSGi metadata

[merks]

It's available here:

https://repo1.maven.org/maven2/net/i2p/crypto/eddsa/0.3.0/

But if we try to use it directly, its package requirements fail:

[ERROR]   Missing requirement: net.i2p.crypto.eddsa 0.3.0 requires 'java.package; sun.security.x509 0.0.0' but it could not be found
[ERROR]   Cannot satisfy dependency: org.eclipse.orbit.maven.osgi.all.feature.group 4.30.0.v20230928-1507 depends on: org.eclipse.equinox.p2.iu; net.i2p.crypto.eddsa [0.3.0,0.3.0]

It has this import:

Import-Package                          sun.security.x509

The EBR recipe rewraps it with that import optional:

Import-Package                          sun.security.x509;resolution:=optional

[merks]

@jonahgraham

FYI, this is the last/only Orbit EBR bundle that I've not excluded from aggregation:

orbit-simrel/orbit-aggregation/orbit.aggr

Lines 16 to 18 in 85f7256

	<contributions description="orbit" label="orbit">
	<repositories location="https://download.eclipse.org/tools/orbit/downloads/latest-N" description="orbit">
	<bundles name="net.i2p.crypto.eddsa" versionRange="[0.3.0.v20220506-1020]"/>

[jonahgraham]

That is awesome progress Ed!

[merks]

I'm actually doubtful now that this is bad metadata. I found another case of this here:

https://repo1.maven.org/maven2/org/conscrypt/conscrypt-openjdk-uber/2.5.2/

That package and its classes definitely exist:

But these are not considered system packages by OSGi (at least Eclipse's implementation) and so those packages do no appear in any profiles nor as a result p2's fake a.jre.javase IUs nor Justj's JRE IUs.

[merks]

@HannesWell @laeubi

I'm not sure what's going on here. I expect someone is actually using these OSGi bundles, but are they needing to do something "funky" to get them to actually work? Or maybe they just work and it's a p2 problem that one can't install them....

[laeubi]

sun.security.x509 should be part of java.base/sun.security.x509, I think the problem might be that P2 does not have a matching profile IU for that.

Tycho should derive those automatically from the running JVM (or any configured JVM profile) so main question is who gives that error message and is there a small reproducer to that?

[merks]

I think the fundamental problem is that it's not a system package. I.e.,

    String property = System.getProperty("org.osgi.framework.system.packages");
    System.out.println("org.osgi.framework.system.packages=");
    for (String systemPackage : property.split(", *"))
    {
      if (systemPackage.startsWith("sun"))
      {
        System.out.println("" + systemPackage);
      }
    }

prints out this

org.osgi.framework.system.packages=
sun.misc
sun.reflect

I can dig further into OSGi, but, unless my recollection fails me, fundamentally it's this that's being used to synthesize the p2 metadata for JustJ JREs and the a.jre.javase profiles. And it's this that makes me wonder if it works or not at runtime.

But I have a hard time to even create an example that tests it:

It's clear this package is not really intended to be used...

[merks]

Note that the a.jre.javase's packages are created by org.eclipse.equinox.p2.publisher.actions.JREAction.calculateVMPackages().

[laeubi]

You can add additional packages with org.osgi.framework.system.packages.extra framework property, I'm not famailiar with module system in detail but it is mentioned that you need --add-exports java.base/sun.security.x509=ALL-UNNAMED

Note that the a.jre.javase's packages are created by
org.eclipse.equinox.p2.publisher.actions.JREAction.calculateVMPackages().

That's why I asked where you get this error Tycho uses:
https://github.com/eclipse-tycho/tycho/blob/master/tycho-core/src/main/java/org/eclipse/tycho/core/ee/ListSystemPackages.java

[merks]

Unfortunately this is exactly what I mean about needing to do "funky" things.

If this (require package sun.security.x509 ) only works when adding system properties and/or other VM arguments, then one would need some other installable unit with touchpoints that add those things to an installation. Also one needs an installable unit that provides that package capability so that p2 resolution succeeds; in the installer there is only p2 metadata, nothing else. And then one has to be optimistic that PDE won't also need additional "funky" things to be able to launch (without validation errors) and actually work at runtime; certainly the launched JVM will need the same property/arguments.

It all sounds kind of complex and questionable. In the debugger I can see that org.eclipse.osgi.storage.Storage.calculateVMPackages() computes exactly the system packages we see, so without some "funky" stuff, it's not going to work...

It also doesn't seem to work:

[laeubi]

need additional "funky" things

I think that's the case when you use non portable stuff.

But the library is still there: https://github.com/str4d/ed25519-java so if it is useful for anyone it would make sense to probably adjust things a bit there e.g. replace by bouncy-castle

It also doesn't seem to work

How do you define "not work"? The bundle should now resolve inside OSGi as the package is provided by the system-bundle, but the system property will not change, so you need to run the code that loads something from sun.security.x509 to see if something really fails.

[merks]

That's a very good question! I found that adding -Dorg.osgi.framework.system.packages.extra=sun.security.x509 to the launch allows the package requirement to resolve and I can reflectively loaded the class so it actually does work.

[merks]

This previously removed the import:

https://github.com/eclipse-orbit/orbit/blob/3e69e7840ce739b5b2b3617cfca3532d6ed10787/google/org.conscrypt.conscrypt-openjdk-uber_2.5.1/osgi.bnd#L12

This made the import optional:

https://github.com/eclipse-orbit/orbit/blob/3e69e7840ce739b5b2b3617cfca3532d6ed10787/github/eddsa/net.i2p.crypto.eddsa_0.3.0/osgi.bnd#L11-L12

So while it's possible to make these work in principle, in practice one will have to do a significant number of "funky" things for p2 to resolve (i.e., IU with that package capability), for PDE to resolve (goodness know what), and for OSGi to resolve (i.e., system property for the extra package). The fact they were previously wrapped to remove the import or to make the import optional suggests that maybe trying to get this package to work is overkill in terms of the downstream impact.

Maybe the following is a stupid idea;

We could make a bundle that both imports and exports sun.security.x509. That makes resolution happy in general because there really is a bundle with that java.package capability. It's even the case that when the sun.security.x509 is added via the system property, that the classes are actually visible as demonstrated via reflection above. We'd not really need the system property if the package really is optional, as is suggested by the repackaging of the past. The bundle could additionally have such a touch point, but note that the property value is a comma separated list, so only one such touch point can be used across all installed bundles.

We seem to have two main choices:

1. Re-wrap the bundles, which requires building some infrastructure to strip the artifact such that we can re-wrap it.
2. Provide a bundle that makes p2, PDE, and OSGi happy, which could add the system property or could just makes the package effectively optional.

[merks]

@msohn

This decision impacts jgit so your opinion is relevant:

[laeubi]

It's even the case that when the sun.security.x509 is added via the system property, that the classes are actually visible as demonstrated via reflection above.

Please note that an equinox specific legacy mode make this possible, this is not standard OSGi if you mean that you do not need to import it!

We seem to have two main choices:

3. make it optional at the source and ask for a new release :-)

Could one not include the old item in the target via a Directory location?

[merks]

I do expect it's visible (to the class loader) only because of the package import in the MANIFEST.MF as I actually did in the example...

Yes, we can keep redistributing the already-rewrapped bundle(s) as we are doing now. But I'd very much prefer that we have freedom of action to rely only on what is currently buildable from scratch. I also prefer that we have a solution for dealing with cases where other people cannot be relied upon to do what we wish them to do. Call me pessimistic, though I prefer to think of it as realistic..

Note that this bundle is newer:

https://repo1.maven.org/maven2/org/conscrypt/conscrypt-openjdk-uber/2.5.2/

Previously we wrapped 2.5.1. So for that we have no path forward, except

• keep maintaining some parts of old Orbit and EBR and using it to build new bundles
• ask other people to do things for us and patiently hope for the best

This is why I look for a solution that I can complete ASAP without requiring anything of anyone else...

[merks]

Speaking of asking for something from someone else. How about if Tycho/m2e supports a "force" mode in the *.target that allows me to treat a artifact with an OSGi MANIFESF.MF as if it didn't have one so that I could re-wrap it directly without jumping through hoops? I suppose I'd have to do that myself too. 😜 And wait for that to percolate through the release processes... 😱 Or maybe such a thing exists already?!

[laeubi]

Wrapping is really meant as a way for adding (temporarily) things for testing or if adding metadata is impossible and in all other cases where there is "bad" (or better inappropriate) metadata it is expected to consider this a bug, fix it at the source and use the fixed library.

So there is currently no way to "force" wrapping, as wrapping is not the function that is primarily performed, it is only performed as a mean for error handling (== no OSGI manifest) therefore the other options are ignore and error.

Of course wan can also simply fork the repository, adjust what is needed an build from that source to build an older "fixed" version.

[merks]

@jonahgraham @msohn

I'll interpret silence as consent.

I intend to create this bundle to satisfy the package import with an empty package:

That will allow Orbit to consume the following two dependencies as-is directly from Maven:

Consumers can choose to add this same package import/export to their consuming bundle instead of relying on the dummy bundle. Consumers can also choose to add a system property to make the actual package from the JRE available at runtime.

I will wait until tomorrow morning for any possible objections.

[jonahgraham]

I'll interpret silence as consent.

You can have my active consent :-)

That said, I don't know what effect this will have and I look forward hearing from @msohn what effect he thinks this will have. @msohn if you want to try an integration build of the full stack with this change (i.e. all the way until EPP) let me know and I can help make sure that happens.

[merks]

I will proceed tomorrow then.

I hope to have a new pre-M2 milestone in place by the end of the day... We shall see...

I'm 99% sure that @msohn could just create an empty package and import and export it just like what I did in my dummy bundle...

[sratz]

But the library is still there: https://github.com/str4d/ed25519-java so if it is useful for anyone it would make sense to probably adjust things a bit there e.g. replace by bouncy-castle

Actually, the dependency to sun.security.x509 has already been removed in master branch:
str4d/ed25519-java@35c34a5

There just is no new release, yet.

I have asked for a new release: str4d/ed25519-java#92

[merks]

That's great. This is another good reason not to build infrastructure to strip the manifest and to re-package it...

[msohn]

We added this library to add support for ed25519 ssh keys in JGit.
Making these imports optional was a workaround to make OSGi happy and didn't cause issues in JGit's use.

I guess development of str4d/ed25519-java stalled since ed25519 was added to Java 15 with JEP339 https://openjdk.org/jeps/339

AFAICS it's also supported by bouncycastle, see
https://javadoc.io/static/org.bouncycastle/bcprov-jdk18on/1.76/org/bouncycastle/math/ec/rfc8032/Ed25519.Algorithm.html

I guess the way forward would be to bump JGit's minimum Java version to 17 (currently it's still 11) or use the bouncycastle implementation.

@twolf: what do you think ?

[merks]

I opened this issue:

google/conscrypt#1173

[merks]

The workaround is complete and is available in the pre-m2 milestone.

https://download.eclipse.org/tools/orbit/simrel/orbit-aggregation/milestone/latest

[msohn]

The workaround to export an empty sun.security.x509 package seems to work:
https://git.eclipse.org/r/c/jgit/jgit/+/204934

[merks]

Very good! Thanks for the confirmation. 👍

[tomaswolf]

We added this library to add support for ed25519 ssh keys in JGit. Making these imports optional was a workaround to make OSGi happy and didn't cause issues in JGit's use.

I guess development of str4d/ed25519-java stalled since ed25519 was added to Java 15 with JEP339 https://openjdk.org/jeps/339

AFAICS it's also supported by bouncycastle, see https://javadoc.io/static/org.bouncycastle/bcprov-jdk18on/1.76/org/bouncycastle/math/ec/rfc8032/Ed25519.Algorithm.html

I guess the way forward would be to bump JGit's minimum Java version to 17 (currently it's still 11) or use the bouncycastle implementation.

@twolf: what do you think ?

My Github handle is "tomaswolf". (Without "h"; all reasonable names were already taken when I created the account...)

IIRC the dependency to sun.security.x509 in the net.i2p bundle is a bug. It is a test dependency only, but not declared as such. That's why declaring it optional didn't cause any problems.

Apache MINA sshd should move away from this net.i2p bundle; it also has a security bug, plus an API bug that Apache MINA sshd works around. There is already an issue for it in the Apache MINA sshd bug tracker. Unfortunately it's only possible to get rid of this dependency by breaking API. Probably I should just do so (break the API) and go for a new major release Apache MINA sshd 3.0.0... (It's also one of the more convoluted areas of Apache MINA sshd -- these baroque APIs and having everything public is really taxing.)