On Thu, 15 May 2025 19:29:07 GMT, Sean Mullan <mul...@openjdk.org> wrote:
> > > It is nice to refactor the common code for algorithm constraints checking > > > into a new class, `X509KeyManagerConstraints.java`, used by both > > > `SunX509KeyManagerImpl` and `X509KeyManagerImpl`. However, it looks like > > > a new system property, "jdk.tls.keymanager.disableConstraintsChecking", > > > is introduced, and it will affect both `SunX509KeyManagerImpl` and > > > `X509KeyManagerImpl`. Should the property be a switch for SunX509 > > > KeyManager, not a general toggle for all KeyManager? Avoiding its misuse > > > for `X509KeyManagerImpl` that may lead to disable the existing RFC > > > compliant algorithm constraints checking? It might be preferable to keep > > > the property logic in `SunX509KeyManagerImpl` (not in the common code). > > > > > > @haimaychao Thanks for looking into it! Yes, it will disable constraints > > checking for both key managers and I did it this way on purpose. I think it > > will be simpler and less confusing to the end users. This system property > > is off by default and my assumption is that if end users want to disable KM > > algorithm constraints checking they would expect it to be disabled > > system-wide. Making this toggle SunX509-specific is a trivial change if we > > have a consensus on this. > > @seanjmullan What do you think? > > Need to think about it some more, but I am kind of leaning towards it only > affecting SunX509. The main benefit of the property is to workaround any > compatibility issues where current code is not ready for the change. Any > application already using the PKIX TrustManager already has this checking > enabled/enforced. I'd agree. As I mentioned in my earlier comment, if the new system property ends up toggling behavior in both `SunX509KeyManager` and `X509KeyManagerImpl`, we could run into an unintended side effect. While we're adding compliant algorithm constraints checking to `SunX509KeyManager`, turning on the property to disable it for compatibility reasons would also disable the already-existing checking in `X509KeyManagerImpl`. The applications already relying on the stricter checks in `X509KeyManagerImpl` might lose that enforcement unintentionally. ------------- PR Comment: https://git.openjdk.org/jdk/pull/25016#issuecomment-2950663297
