[rfc][icedtea-web] https probing

[Jacob Wisor]

On 09/23/2014 at 05:31 PM, Jiri Vanek wrote:
> On 09/14/2014 06:17 AM, Jacob Wisor wrote:
>> On 09/10/2014 at 11:42 AM, Jiri Vanek wrote:

Because the discussion on this patch has become quite extensive I am going to 
focus on the two most important issues only. At least for now.

[...]
>>> +        if (url.getProtocol().equalsIgnoreCase("https") &&
>>> isProbingAllowed()) {
>>> +            if (isSyncForced()) {
>>> +                while (!httpsConnections.isEmpty()) {
>>> +                    try {
>>> +                        Thread.sleep(100);
>>> +                    } catch (InterruptedException ex) {
>>> +                        throw new IOException(ex);
>>> +                    }
>>> +                }
>>
>> Polling? Really? Is this really the proper way to do it? And, do we really
>> need to offer
>
>
> Actually yes. I dont know about better way here. I will be happy if you have some.
>
> What I wonted to take care about, is situatuion when the individual https
> settings differe.   Then the first one must be disconnected, before second can
> apply for new settings.
> And so tehre is need to not accept other https conenctions until previous one
> disconnects.
>
>
>> synchronous connections?
>> Besides, what about infinite looping?
>
> Do you mean that it can deadlock here? Yes. I'm thinking the same. But The same
> deadlock will rise now too. For my pooling here, the timeout is most simple
> solution and will probably appear in some next iteration changeset

I think you cannot avoid to pay tribute to the fact that networks are inherently 
asynchronous by their very nature (I am not talking about USB here ;-) ). Having 
said that, we /can/ support synchronous connection creating but it should be 
based on asynchronous calls because ... you guessed it, of a network's inherent 
asynchronous nature.
Anyway, the Http(s)Factory should support creating HTTP /and/ HTTPS connections 
(and thus download JNLP resources) not only asynchronously but also 
concurrently. None of the protocols posses a property that prevents us from 
implementing asynchronous connection creating, forces us to support synchronous 
connection creating only (even when testing for multiple versions), nor imposes 
a limit on the full (logical) potential of a network. Besides, establishing 
connections synchronously only, is definitely not the state of the art for the 
21st century. ;-)

Also, please note that your current approach is not thread-safe, even for 
synchronous calls only! Thread-safety is actually a MUST for a Http(s)Factory. 
Simply putting the synchronized keyword in front of a method declaration is not 
fully solving the problem here nor should actually be done this way. What you 
actually want to synchronize is the access to the Http(s)Factory's internal 
state, not the access to the factory (its methods) itself. There is no need to 
block *all* other threads which want to create a connection just because one 
thread is using the factory or is waiting for the factory to complete creating a 
connection for it. I strongly advise you to rewrite the factory accordingly.

The proper way to provide a synchronous call in an asynchronous environment is 
to block or yield. And, you certainly do not accomplish this with potentially 
infinite loops making the current thread wait an arbitrary amount of time. What 
you do is; you set off the asynchronous call and then wait (Object.wait()) until 
you get notified (Object.notify()) by the asynchronous call's on complete method.

>>> [...]
>>> +        //this s intentional search by object value. equals do not work
>>> +        for (int i = 0; i < httpsConnections.size(); i++) {
>>
>> Start from the end, decrement to 0, and compare to 0. Comparing to 0 gives
>> almost always far better
>> performance on modern processors because they are heavily optimized for branch
>> prediction on 0.
>>
>>> +            URLConnection uRLConnection = httpsConnections.get(i);
>>> +            if (uRLConnection == conn) {
>>> +                httpsConnections.remove(i);
>>> +                i--;
>>
>> Besides, why not use ArrayList.removeAll(Collection) here? It is probably much
>> faster, even though
>> you have to create a Collection instance.
>
>
> I'm not sure what you mean here.

You are trying to remove all "conn" elements from the "httpsConnections" 
ArrayList the awkward way. The best way to do this is to call 
ArrayList.removeAll(Collection) instead of iterating through the ArrayList manually.

>>> [...]
>>> +        public static URLConnection openConnection(URL url, boolean ssl2,
>>> boolean insecure)
>>> throws IOException {
>>> +            try {
>>> +                if (ssl2) {
>>> +                    System.setProperty("https.protocols", "SSLv3,SSLv2Hello");
>>> +                } else {
>>> +                    System.clearProperty("https.protocols");
>>> +
>>> +                }
>>
>> I am absolutely sure we do not want to modify *system* properties for *all*
>> future connections here.
>> What about other existing and other future connections, especially connections
>> created by a JNLP
>> application?
>> And then there are SecurityManager implications. What if these or all system
>> properties are read-only?
>> So no, we do not want to modify system properties at runtime.
>
> Ok.I made investigations and you are right. This is show stopper on current
> approach.
> The issue helpcrypto is facing with jdk8 have solution based on properties to:(

This raises the question whether we should bother about this at all, since the 
SSL version of HTTPS connections can and have always been configurable by system 
properties in the JRE. :-D

> By default, no probing will be done, and  itw will work as before.
> There will be only one property - public static final String
> KEY_HTTPS_PROBINGALLOWED = "deployment.security.https.probing.allowed";

Then this property should be renamed to "deployment.security.https.probing.enabled".

> By default false, and with possible true/false
>
> Maybe probing will be possible to turn on in runtime, if https connection fails,
> on users approve.

It could. But, on the other hand who or what would turn on probing for HTTPS 
connections at runtime? The JNLP application? Certainly not. A JNLP application 
does not even control downloading resources directly.
Then perhaps the user? If so, this would require for the user to be able to 
change the desired JNLP application's JRE configuration in some kind of UI or 
command line interface provided by IcedTea-Web. And it would make tracing the 
configuration for every running JNLP application and visualizing system 
properties mandatory. This is counter intuitive and breaks JNLPRuntime's current 
concept which is to configure the JRE for a JNLP application's entire lifetime. 
Besides, there is currently no UI or infrastructure to support this in IcedTea-Web.

> Once proobing is true, then the scenario will be same as attached (same as
> always in this thread) chnageset with :
> deployment.security.https.syncforced = true

Why so? Why should creating HTTPS connections be synchronous only? There is no 
rational reason to impose such a limit. Besides, it definitely leads to 
increased latencies and possibly to decreased throughput.

> deployment.security.https.probing.always = true
> deployment.security.https.allowinsecure = ask

Jacob