search

jakartaee / cdi

CDI specification
Apache License 2.0
214 stars 78 forks source link

Remove support for Security Manager #831

Open arjantijms opened 2 weeks ago

arjantijms commented 2 weeks ago

According to the Jakarta EE 11 plan, we should have removed all references to the Java SE security manager APIs -> "Remove all use of SecurityManager" (emphasis mine). In CDI we forgot to do this.

As it is purely an implementation detail, I believe we can (and should) do this in a service release of the CDI API.

See also https://github.com/jakartaee/cdi/pull/830

cc @ivargrimstad @edburns

Azquelt commented 2 weeks ago

I'll add the analysis I put on the discussion on the PR:

  1. We have no spec requirements for SecurityManager
  2. Our API classes do not extend, implement or reference in their method signatures any SecurityManager-related classes
    • In the worst case, I think this means that an implementation could use their own versions of these API classes with all mention of SecurityManager removed and still pass the TCK and signature tests.
  3. The only methods we do call within our API classes are System.getSecurityManager and AccessController.doPrivileged. These methods will still be present in Java 24 (at least according to JEP 486, section Advice to maintainers of libraries that support the Security Manager).

Given that there is currently no plan to remove the methods we do still use, I don't see any reason to change the code in the API jar that has already been released, particularly as nothing would prevent us from doing a patch release at some point in the future if this did become an actual problem for anyone.

arjantijms commented 2 weeks ago

Given that there is currently no plan to remove the methods we do still use,

Can you eleborate on that? The plan AFAIK was firmly in place. We just forgot to act on it for CDI 4.1, right?

I don't see any reason to change the code in the API jar that has already been released

Would that be even possible? Maven central artefacts are immutable. I don't think we should change anything in a jar that was already released. We could easily do a 4.1.1. I just did two of such releases for the Faces API, and that was really no big deal at all.

Azquelt commented 2 weeks ago

Can you eleborate on that? The plan AFAIK was firmly in place. We just forgot to act on it for CDI 4.1, right?

There is no plan to remove the two methods that we use (System.getSecurityManager and AccessController.doPrivileged) from Java. Hence, there is no need to remove our code that calls them.

We could easily do a 4.1.1.

Sorry, I was meaning that I see no reason to do a patch release. As a user, I would generally consider a patch release to be bug fixes only and essentially a replacement for the previous release. I do not see a reason to do a 4.1.1 release. I don't deny that it would be easy, but I don't think we should be changing anything unnecessarily in a patch release.

arjantijms commented 2 weeks ago

Hence, there is no need to remove our code that calls them.

I still don't understand, sorry for persisting here.

I meant; didn't we all agree on the Jakarta EE 11 plan, where we all committed to remove all references to the security manager in our APIs?

Ladicek commented 1 week ago

Well clearly many specifications (it's not just CDI, but also REST and others) didn't get the memo. I know I didn't. Communication is hard.

Apparently, the REST people agree with the opinions stated here -- this is an OK change for the next release, but there's no need to do a micro release of the current APIs just to remove references to the security manager. Honestly -- the security manager APIs are not being removed yet, and there's no public timeline for that, so it's just not a big deal.

manovotn commented 1 week ago

Well clearly many specifications (it's not just CDI, but also https://github.com/jakartaee/rest/issues/1262 and others) didn't get the memo. I know I didn't. Communication is hard.

It is hard indeed. I think it might have easily been the case of this decision coming late in our cycle when we had the release basically done. While I recall it being discussed during early stages of EE 11 development, I also remember that there was no decision at the time (possibly because nobody was sure if SM is going to stop working with 17 or not really).

As for maintenance release of 4.1, I am with Ladislav and Andrew - I don't think this is worth the hassle. I am not saying that it would be difficult, it's just that the presence or absence of this code has no effect on users consuming CDI APIs, regardless of what supported JDK version they pick.

IMO the important part is that we removed SM references for future CDI versions, where the actual removal of SM happens.

arjantijms commented 1 week ago

and there's no public timeline for that, so it's just not a big deal.

Isn't that re-discussing something that was already decided? I mean, if it wasn't a big deal, why did we put it in the EE 11 plan as a major item?