Re: Switch to in-memory key store in tomcat 8.5.23 fails application to load

[Nitkalya Wiriyanuparb (Ing)]

Emil,

On 15 Feb 2018, 8:52 PM +1300, Emil John , wrote:
> Context
> ---
>
> Exact tomcat version, Operating Systems, other configurations-
>
> Current Tomcat version - 8.5.15
> Operating Systems - Windows/ Linux
> Upgrading to tomcat version - 8.5.23
> Application - Java Application.
>
> I have an application with tomcat, say fooapp. I also have a custom
> keystore type, say DKS (Java by default has the JKS keystore). During start
> of my application, it loads the DKS keystore to get the certificate for the
> application. This is done using the following changes in server.xml
>
>  sslImplementationName="com.vmware.identity.tomcat.GKSAwareImpl"
> store="CERT_STORE"
> port="${bio-ssl-localhost.https.port}"
> protocol="com.vmware.identity.tomcat. GKSAwareHttp11NioProtocol"
> redirectPort="${bio-ssl-localhost.https.port}"
> scheme="https"
> secure="true"
> maxHttpHeaderSize="16384"
> 
>
> Problem
> ---
>
> The new version of tomcat has a changed code that is causing my application
> from not able to load the GKS keystore.
>
> In Tomcat 8.5.15,
> getKeyManagers() method - if ks is not null, it simply proceeds further
> doing the ks.isKeyEntry() etc..
>
> In Tomcat 8.5.23,
> getKeyManagers() method - if ks is initialized as before and create a new
> reference -
> KeyStore ks = certificate.getCertificateKeystore();
> KeyStore ksUsed = ks;
>
> After the below code, the ksUsed is getting back to JKS and fails to load
> my custom keystore type "GKS"
>
> // Switch to in-memory key store String provider =
> certificate.getCertificateKeystoreProvider(); if (provider == null) {
> ksUsed = KeyStore.getInstance(certificate.getCertificateKeystoreType()); }
> else { ksUsed =
> KeyStore.getInstance(certificate.getCertificateKeystoreType(), provider); }
> ksUsed.load(null, null); --> throws unimplmented method
>
> I am setting the provider type properly in java.security which is also used
> while loading the application.
>
> Has anybody faced similar problem?

I had a similar problem – not quite the same but close enough. I ended up 
creating another key store type that wraps around my existing types. See my 
threads here http://markmail.org/message/5vus3jpsp5secm44

Cheers,
Ing
>
> Thanks,
> Emil

Re: Questions about JSSEUtil#getKeyManagers

[Nitkalya Wiriyanuparb (Ing)]

On 24 Jan 2018, 10:19 PM +1300, Nitkalya Wiriyanuparb (Ing) 
<dekpi...@gmail.com>, wrote:

>
> On 24 Jan 2018, 9:45 PM +1300, Mark Thomas <ma...@apache.org>, wrote:
> > On 23/01/18 02:57, Nitkalya (Ing) Wiriyanuparb wrote:
> > > Hi all,
> > >
> > > I'm on Java 8 and Tomcat 8.5.26 (built from tag) moving from 7.0.41.
> > >
> > > I have a little problem with how JSSEUtil#getKeyManagers creates key
> > > managers. This essentially causes Tomcat to sometimes serves an incorrect
> > > server certificate chain during ServerHello.
> > > -Djavax.net.debug=all gave me a clue as it printed out multiple "matching
> > > alias", so I believe it's because the key manager (and key store) returned
> > > from that method doesn't contain only one key. From what I see, when
> > > switching to in-memory key store getKeyManagers creates a new key store of
> > > the configured type, calls setKeyEntry and expects the new key store to
> > > have only this one key in it.
> > >
> > > Note that we have our own implementation of the key store, but please bear
> > > with me.
> > >
> > > I'm also aware of this following bit of documentation and I suspect that
> > > the second sentence is very much related to my problem here. I'm also sure
> > > the certificateKeyAlias is set correctly and SSLHostConfigCertificate has
> > > all the expected values when I checked in debug mode.
> > >
> > > > The alias used for the server key and certificate in the keystore. If 
> > > > not
> > > specified, the first key read from the keystore will be used. The order in
> > > which keys are read from the keystore is implementation dependent.
> > >
> > > We didn't have this problem in 7.0.41 because it's doing something less
> > > complex and eventually just creates a JSSEKeyManager with the expected key
> > > alias with the key store as a delegate – see
> > > https://github.com/apache/tomcat70/blob/TOMCAT_7_0_41/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java#L563
> > >
> > > But in 8.5,
> > > https://github.com/apache/tomcat85/blob/TOMCAT_8_5_26/java/org/apache/tomcat/util/net/jsse/JSSEUtil.java#L267
> > > the identity comparison "ksUsed == ks" looks kind of weird to me as
> > > KeyStore.getInstance (at least in Oracle Java 8) always returns a new
> > > instance of KeyStore, so the checks will never be true (or will it?).
> >
> > Yes they will. As per the comment at line 255, non-PKCS#8 keystores will
> > use the original key store.
> >
> > > Ideally, I'd want to find a way to get into that if block so the end state
> > > is like in 7.0.41.
> > >
> > > As I mentioned, we have our own key store implementation and it always
> > > loads all keys it's supposed to know about so reassigning "ksUsed =
> > > KeyStore.getInstance..." doesn't make a difference for us – it actually
> > > makes it worse as without it "ksUsed == ks" would have been true.
> >
> > And there is the problem.
> >
> > Tomcat is jumping through quite a few hoops to handle various use cases:
> > - PEM encoded keys
> > - keystores with multiple keys each with their own password
> >
> > That last one is the cause of most of the trouble. Key stores allow this
> > but the KeyManagerFactory API doesn't. This is why we now always create
> > the in-memory key store. When we do this, we can't just use JKS for the
> > in-memory key store type as that creates issues like BZ 61557.
> >
> > > We technically can just modify or introduce a new key store implementation
> > > to cater for Tomcat implementation – locally patching Tomcat to remove the
> > > identity check would work for us as well.
> > >
> > > Before doing that, am I missing something obvious? is reimplementing our
> > > key store the way to go here?
> >
> > I don't think you are missing anything obvious. We could look at adding
> > (even more) configuration options to separately control the type and
> > provider for the in-memory key store (assuming using JKS here would work
> > for you) but I'm a little concerned about how complex that code is getting.
> >
> I guess that’s another option. JKS would work for us. We have our own 
> implementation of in-memory key store that would also (almost) work if Tomcat 
> let us pick a different key store type for the in-memory store. But that 
> sounds a bit yucky as it's exposing an option for internal Tomcat 
> implementation.
&g

Re: Questions about JSSEUtil#getKeyManagers

[Nitkalya Wiriyanuparb (Ing)]

On 24 Jan 2018, 9:45 PM +1300, Mark Thomas , wrote:
> On 23/01/18 02:57, Nitkalya (Ing) Wiriyanuparb wrote:
> > Hi all,
> >
> > I'm on Java 8 and Tomcat 8.5.26 (built from tag) moving from 7.0.41.
> >
> > I have a little problem with how JSSEUtil#getKeyManagers creates key
> > managers. This essentially causes Tomcat to sometimes serves an incorrect
> > server certificate chain during ServerHello.
> > -Djavax.net.debug=all gave me a clue as it printed out multiple "matching
> > alias", so I believe it's because the key manager (and key store) returned
> > from that method doesn't contain only one key. From what I see, when
> > switching to in-memory key store getKeyManagers creates a new key store of
> > the configured type, calls setKeyEntry and expects the new key store to
> > have only this one key in it.
> >
> > Note that we have our own implementation of the key store, but please bear
> > with me.
> >
> > I'm also aware of this following bit of documentation and I suspect that
> > the second sentence is very much related to my problem here. I'm also sure
> > the certificateKeyAlias is set correctly and SSLHostConfigCertificate has
> > all the expected values when I checked in debug mode.
> >
> > > The alias used for the server key and certificate in the keystore. If not
> > specified, the first key read from the keystore will be used. The order in
> > which keys are read from the keystore is implementation dependent.
> >
> > We didn't have this problem in 7.0.41 because it's doing something less
> > complex and eventually just creates a JSSEKeyManager with the expected key
> > alias with the key store as a delegate – see
> > https://github.com/apache/tomcat70/blob/TOMCAT_7_0_41/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java#L563
> >
> > But in 8.5,
> > https://github.com/apache/tomcat85/blob/TOMCAT_8_5_26/java/org/apache/tomcat/util/net/jsse/JSSEUtil.java#L267
> > the identity comparison "ksUsed == ks" looks kind of weird to me as
> > KeyStore.getInstance (at least in Oracle Java 8) always returns a new
> > instance of KeyStore, so the checks will never be true (or will it?).
>
> Yes they will. As per the comment at line 255, non-PKCS#8 keystores will
> use the original key store.
>
> > Ideally, I'd want to find a way to get into that if block so the end state
> > is like in 7.0.41.
> >
> > As I mentioned, we have our own key store implementation and it always
> > loads all keys it's supposed to know about so reassigning "ksUsed =
> > KeyStore.getInstance..." doesn't make a difference for us – it actually
> > makes it worse as without it "ksUsed == ks" would have been true.
>
> And there is the problem.
>
> Tomcat is jumping through quite a few hoops to handle various use cases:
> - PEM encoded keys
> - keystores with multiple keys each with their own password
>
> That last one is the cause of most of the trouble. Key stores allow this
> but the KeyManagerFactory API doesn't. This is why we now always create
> the in-memory key store. When we do this, we can't just use JKS for the
> in-memory key store type as that creates issues like BZ 61557.
>
> > We technically can just modify or introduce a new key store implementation
> > to cater for Tomcat implementation – locally patching Tomcat to remove the
> > identity check would work for us as well.
> >
> > Before doing that, am I missing something obvious? is reimplementing our
> > key store the way to go here?
>
> I don't think you are missing anything obvious. We could look at adding
> (even more) configuration options to separately control the type and
> provider for the in-memory key store (assuming using JKS here would work
> for you) but I'm a little concerned about how complex that code is getting.
>
I guess that’s another option. JKS would work for us. We have our own 
implementation of in-memory key store that would also (almost) work if Tomcat 
let us pick a different key store type for the in-memory store. But that sounds 
a bit yucky as it's exposing an option for internal Tomcat implementation.

> I think I'd look at modifying your key store implementation but if that
> is a lot of work, we can explore some additional configuration options
> in Tomcat.
The current easiest workaround for us is patching Tomcat internally as 
mentioned (our application stack is pretty strict so we’re sure nothing will be 
using a different key store). But if the current Tomcat implementation is here 
to stay, I would prefer doing the right thing. I’ll discuss this with my team 
and try creating another key store type for Tomcat as well.
>
> Cheers,
>
> Mark
>
> -
> To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
> For additional commands, e-mail: users-h...@tomcat.apache.org

Cheers,
Ing

Re: BeanELResolver issue – calling a varargs method with no argument

[Nitkalya Wiriyanuparb (Ing)]

On 24 Dec 2017, 3:08 AM +1300, Mark Thomas , wrote:
> On 19/12/17 13:13, Mark Thomas wrote:
> > On 18 December 2017 03:32:40 GMT+00:00, Nitkalya Wiriyanuparb 
> >  wrote:
> >
> >  >
> > > Tested with my system – everything’s awesome. I can also confirm that
> > > the edge case I mentioned is valid; calling the varargs method with an
> > > array ${actionBean.getIncludes([“something"])} doesn’t coerce
> > > correctly. I personally don’t care about this use case, so I’ll leave
> > > it to the maintainers to decide. :)
> >
> > Thanks for confirming the fix.
> >
> > Personally, edge cases not working correctly bugs me so I've added this to 
> > my TODO list.
>
> I've taken a look at this and it is a sufficiently grey area that I'm
> not intending to make any changes at this point.
>
> The problem is that ["something"] isn't an array with a single element,
> it is a List with a single element. The EL spec doesn't mention any
> coercion rules for Lists or varargs.
>
> What we have tried to implement so far is to first apply the normal Java
> rules and if they don't work coerce the parameters and try again. The
> additional coercion steps sometimes create ambiguity. In that case we fail.
>
> For this specific case we currently coerce List to a String and
> then treat it as a single argument.
>
> We could coerce the List elements individually but there is nothing in
> the EL spec to suggest that that is correct behaviour. I can see how it
> could be expected but given that a) the spec doesn't mention it; b) it
> isn't a problem for you; and c) changing it might cause a problem for
> others; I'm going to leave things as they are. We can always re-evaluate
> that choice at a later date.
>
> Mark
>
> -
> To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
> For additional commands, e-mail: users-h...@tomcat.apache.org
>

Thanks for looking into it and providing such a detailed explanation. I also 
think it’s not worth changing at this point – especially if you believe it 
might create a problem unexpectedly.

Merry Christmas & Happy New Year
Ing

Re: BeanELResolver issue – calling a varargs method with no argument

[Nitkalya Wiriyanuparb (Ing)]

On 9 Dec 2017, 3:35 AM +1300, Mark Thomas , wrote:

> On 07/12/17 15:00, Mark Thomas wrote:
> > On 07/12/17 02:52, Nitkalya (Ing) Wiriyanuparb wrote:
> > > Hello.
> > >
> > > I'm upgrading from Tomcat 7.0.30 to 8.5.24. Everything is working fine, 
> > > but
> > > I notice that BeanELResolver behaves differently when calling a bean
> > > varargs method with no argument.
> > >
> > > My action bean has a method with the signature "public String
> > > getIncludes(final String... moduleNames)". I'm calling it on one of my JSP
> > > pages like "${actionBean.getIncludes()}", but got MethodNotFoundException.
> > > Calling it with some arguments or an empty array would work – as one would
> > > expect. Please see the stacktrace below.
> > >
> > > This method call used to be okay in 7.0.30. I've tried to find something 
> > > in
> > > the EL and JSP specifications around varargs, but couldn't find anything
> > > concrete. I got the impression that it's not officially supported in the
> > > specs, but Tomcat supports it.
> > >
> > > I'm treating it as a bug as I've already created a patch for it – see
> > > below. However, I'm new to this mailing list so I thought I would ask 
> > > first.
> > >
> > > Is this a bug?
> >
> > It looks like it, yes.
> >
> > Thanks for the patch. Note it is usually best to attach patches to a
> > Bugzilla issue as that prevents them from getting mangled by e-mail clients.
> >
> > I'll take a closer look at this now.
>
> Thanks again for reporting the issue and providing a patch.
>
> I've applied your patch with a few changes and additions.
>
> The provided test cases highlighted the complexity of matching varargs
> methods, particularly because of the coercion rules that apply in EL. I
> expanded the tests significantly to try and cover all the edge cases. I
> extended the proposed patch to cover those edge cases as well.
>
> The changes will are in:
> - trunk for 9.0.3 onwards
> - 8.5.x for 8.5.25 onwards
> - 8.0.x for 8.0.49 onwards
>
> Mark
>
> -
> To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
> For additional commands, e-mail: users-h...@tomcat.apache.org
>

Thanks for the quick fix. I’ll be testing it with my system tomorrow.

I think there might be another edge case that we missed i.e. passing an array 
as a varargs. If I remember correctly the array is currently coerced into a 
string. Its string representation becomes the first element of the varargs 
instead of the array being the varargs itself. I could be wrong though – will 
double check tomorrow and report back again. However, I have no use case for 
this edge case if it’s deemed ‘unsupported’.

Cheers,
Ing