[rfc][icedtea-web] https probing

[Jiri Vanek]

Unluckily this fix patch is not helping obscure servers to do exists. It really fixes bugs.

First part of fix is delivered to be able  handle SSLv2 handshake, Those servers do exists, and have 
no reason nor need to update or patch or whatever. We are wrong by not allowing it right now.
See   System.setProperty("https.protocols", "SSLv3,SSLv2Hello"); in code.

Second "fix" to skip certificate check - yes it is workaround abut servers fault. But imho necessary 
workaround. From time to time nearly each server  corrupt/*outdate* (read not refresh in time ) 
theirs certificares . Yes it is always flaw of admins, but if I insists on usage of javaws/applet in 
this period before admins fix? Then I'm screwed with itw now. Unluckily in intranet environment (all 
https bugs I had were actually reported form intranet) this is really common. As another changeset I 
will probably add warning for "untrusted https forced" or similar.

Nevertheless, proprietary javaws/plugin must be doing something similar, because they do not have 
similar https issues. So we *must* handle them (not necessary by this patch, but I have not found 
another way in weeks of investigating). Maybe it is messing the concept a bit, but if it is not 
fixed, then people will always say "it is working with proprietary one" and so get rid of itw.


As for naming suggetstions:
I would vote for

deployment.security.https.probing.alowed
deployment.security.https.syncforced";
deployment.security.https.probing.always";
deployment.security.https.allowunsecure";

the  last one  have nothing to do with probing only. It skips certificate check during each https 
connection (unles proobing.always is true, and another resourcs have valid certificate)
The second have nothing to do with probing at all. It is disabling parallel https connections at 
all. Again, useful when https resources are coming from differently "valid" or "handshaked" 
locations on network.
Initial https.* only namespace was intentional - it all affects https communication as complex. Not 
only probing.

And as for typos...eh, yes, sorry, thankyou :( Will be fixed.

J.




On 08/07/2014 09:37 PM, Jacob Wisor wrote:
> On 08/07/2014 07:25 PM, Jiri Vanek wrote:
>> Hi!
>>
>> this patch *should* solve most of the issues ITW have with various broken or old
>> https servers. Should, because I have no page where to reproduce. My only
>> tracker is https://bugzilla.redhat.com/show_bug.cgi?id=1089130 and small
>> attached program which I wrote used to tests various requests to known broken
>> https server (to get files).
>
> Err, I am having trouble understanding the reasoning behind this. Only because a bunch of obscure
> broken HTTPS servers that violate the protocol host some possibly even more obscure applications
> IcedTea-Web should weaken its SSL handshake protocol? :-o This gives me the creeps and sounds right
> silly. If this is the case, then I am absolutely _AGAINST_ this patch.
>
> If IcedTea-Web is unable to properly interoprate with broken HTTPS servers then those servers should
> get updated, patched, fixed, what ever. If somebody, despite knowing better and against all common
> sense /wants/ to interoperate with IcedTea-Web on broken HTTPS servers, and they require a change to
> IcedTea-Web to make it work, then they should probably save this patch for themselves instead of
> deteriorating the quality of IcedTea-Web for everybody else. Or, thus perhaps endangering others by
> configuring to insecure settings.
>
> Nevertheless, I have commented on some of the obvious stuff.
>
>> The patch do following:
>>
>> Wrap all url.openconnections and connection.dieconet by wrapper.
>>
>> This wrapper do following:
>> if connection is not not https, it acts as before (except only synchronized
>> disconnect)
>> if it is https, it locks other (https) connections and  tryes server:
>>   -  if it is not using old handsake or invalid certificate
>>   - save those settings
>>   - all other https connections then uses those settings (unless overwritten)
>>   - except https openconnection and all disconnect connection  all connectins
>> are parallel  as we were used to (unless overwritten)
>>   - the probing connection is reused (if successful)
>>
>>
>> The default schema is not working in (rare) cases when resources are in
>> differently "broken" https servers. To control this flaws there are 4
>> properties, which deserves 4 checkboxes in itw-setting (another patch onc ethis
>> is agreed)
>>
>> +    public static final String KEY_HTTPS_PROBINGALOWED =
>> "deployment.security.https.proobingalowed";
>>
>> By default true, probing is allowed.  If set to false, then itw will behave as
>> before, except synchronized disconnect
>
> Ah, check spelling please ;-)
> KEY_HTTPS_PROBINGALLOWED = "deployment.security.https.probingallowed"
>
> Btw, the property name should probably be split into another namespace, like this:
> deployment.security.https.probing.allowed
>
>>
>> +    public static final String KEY_HTTPS_SYNCFORCED =
>> "deployment.security.https.syncforced";
>>
>> by default false. Once true this will disable our excelent paralelism for https
>> downloads. Have sense only with  probe always
>
> The property namespace naming should probably consistent to above:
>
> deployment.security.https.probing.syncforced
>
>> +    public static final String KEY_HTTPS_PROBEALWAYS =
>> "deployment.security.https.probealways";
>  >
>> when this is true,  each connection will do its own probing. So each can have
>> different https settings. Have sense moreover onl with syncforced, otherwise
>> exisitng conections will overwite each other and fail.
>
> Property namespace naming:
>
> deployment.security.https.probing.always
>
>> +    public static final String KEY_HTTPS_ALOWUNSECURE =
>> "deployment.security.https.allowunsecure";
>>
>> This is actually creating security hole. SO by default false. Once in
>> itw-settings, it will need strong warning for user. It disables certificate
>> verification.
>
> Property namespace naming:
>
> KEY_HTTPS_INSECUREALLOWED = "deployment.security.https.probing.insecureallowed"
>
>
>> hmm..During writing of this email I have realized that I can use same
>> synchronized trick as for open connection (synchronize once https is recognized,
>> but let http be). The patch is updated. But I have not tested. I guess I will
>> test it during next iteration. This is justnote for reviwer that if you will
>> encounter any flaw in above description, this is probably cause. And so above
>> "synchronize ddisconnect" is now valod only for https.
>
> I am also and perhaps foremost against this patch because it promotes broken systems and/or
> software. It sends out the kind of message that it is okay to keep on running broken insecure
> outdated systems instead of actually promoting "update for security" kind of message. This bad,
> really bad...
>
> Jacob
>