[rfc][icedtea-web] https probing

[Jacob Wisor]

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