[rfc][icedtea-web] https probing
Jiri Vanek
jvanek at redhat.com
Tue Sep 23 15:31:13 UTC 2014
On 09/14/2014 06:17 AM, Jacob Wisor wrote:
> On 09/10/2014 at 11:42 AM, Jiri Vanek wrote:
>> ping?
>
> Unfortunately, because tip has change in the meantime in regards to this patch I could not actually
> do runtime tests. However, I have commented on the code. Please, can you post a patch based on the
> current tip?
>
Hi.
The attached patch is only adaptation of previous one to head and with fixed minors and with fixes
you suggested.
According to yours previous hints, the approach needs revisiting. See inline., and especially bottom.
TY for the inputs!
>> -------- Original Message --------
>> Subject: Re: [rfc][icedtea-web] https probing
>> Date: Wed, 03 Sep 2014 16:04:25 +0200
>> From: Jiri Vanek <jvanek at redhat.com>
>> To: Jacob Wisor <gitne at gmx.de>, distro-pkg-dev at openjdk.java.net
>>
>> On 08/26/2014 09:51 AM, Jacob Wisor wrote:
>>> On 08/26/2014 09:16 AM, Jiri Vanek wrote:
>>>> ping?
>>>>
>>>> On 08/20/2014 08:30 PM, Jiri Vanek wrote:
>>>>> On 08/19/2014 11:25 PM, Jacob Wisor wrote:
>>>>>> On 08/18/2014 08:46 PM, Omair Majid wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>>
>>>>>>> * Jacob Wisor <gitne at gmx.de> [2014-08-11 12:12]:
>>>>>>>> On 08/08/2014 10:37 AM, Jiri Vanek wrote:
>>>>>>>>> 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.
>>>>>>>>
>>>>>>>> Oh yes they do need fixing. SSLv2 is insecure and has been revoked by the
>>>>>>>> IETF, period. Nobody should be using it. Even SSL 3.0 is deemed by the IETF
>>>>>>>> as historic (https://datatracker.ietf.org/doc/rfc6101) although it is
>>>>>>>> largely identical to TLS 1.0. Nevertheless, nobody should be using it
>>>>>>>> either. Just one of many reasons is that it does not even support such a
>>>>>>>> common hash algorithm as SHA1 (which by the way has been deprecated by NIST
>>>>>>>> in favor of SHA256 too). Everybody should really upgrade to at least TLS
>>>>>>>> 1.0, even though possible security leaks have been discovered in TLS 1.0 on
>>>>>>>> specific configuration settings permutations.
>>>>>>>>
>>>>>>>> DO NOT use SSL anymore and DO NOT promote them in your software. Upgrade to
>>>>>>>> TLS.
>>>>>>>
>>>>>>> This isn't SSv2, though. It's a SSLv2 hello packet wrapping an SSLv3
>>>>>>> packet: http://bugs.java.com/bugdatabase/view_bug.do?bug_id=4915862
>>>>>>>
>>>>>>> It's actually part of the TLS 1.0 spec:
>>>>>>> https://www.ietf.org/rfc/rfc2246.txt, Appendix E.
>>>>>>
>>>>>> This part describes backward compatibility of TLS 1.0 clients to SSL 3.0 and
>>>>>> SSL 2.0 servers (and
>>>>>> vice versa).
>>>>>>
>>>>>> "TLS 1.0 clients that support SSL Version 2.0 servers must send SSL
>>>>>> Version 2.0 client hello messages [SSL2]. TLS servers should accept
>>>>>> either client hello format if they wish to support SSL 2.0 clients on
>>>>>> the same connection port. The only deviations from the Version 2.0
>>>>>> specification are the ability to specify a version with a value of
>>>>>> three and the support for more ciphering types in the CipherSpec."
>>>>>>
>>>>>> Currently, IcedTea-Web is a TLS 1.0 or SSL 3.0 client. According to this
>>>>>> paragraph, TLS 1.0 clients
>>>>>> send SSL 2.0 client hello messages in order to connect to SSL 2.0 servers,
>>>>>> that is, to negotiate a
>>>>>> SSL 2.0 connection. So no, SSL 2.0 servers do not upgrade automagically to
>>>>>> TLS 1.0 when they receive
>>>>>> SSL 2.0 client hello messages from TLS 1.0 clients. Pure old SSL 2.0 servers
>>>>>> will never speak
>>>>>> anything later than SSL 2.0.
>>>>>>
>>>>>> One reason behind this paragraph was for TLS 1.0 clients to allow probing SSL
>>>>>> 2.0 servers with
>>>>>> cipher specifications in SSL 2.0 and those introduced in TLS 1.0. In
>>>>>> practice, SSL 2.0 servers will
>>>>>> _never_ negotiate to a cipher specification introduced since TLS 1.0 because
>>>>>> they were simply not
>>>>>> built for that.
>>>>>>
>>>>>> Another reason for this paragraph back in 1999 (!) was to allow implementors
>>>>>> of TLS 1.0 to ease
>>>>>> transition from SSL to TLS and to give them the option to merge TLS into
>>>>>> their existing SSL
>>>>>> libraries (or as much as possible build on top of them). Again, this has
>>>>>> nothing to do with
>>>>>> automagically upgrading SSL 2.0 servers to TLS. It is just a description of
>>>>>> how TLS 1.0 clients
>>>>>> could fallback to SSL 2.0, if they wish to.
>>>>>> And, I am most certain nobody should want this, unless one has no clue what
>>>>>> transport security is
>>>>>> all about or is a complete lunatic.
>>>>>>
>>>>>> The next paragraph says it all:
>>>>>>
>>>>>> " Warning: The ability to send Version 2.0 client hello messages will be
>>>>>> phased out with all due haste. Implementors should make every
>>>>>> effort to move forward as quickly as possible. Version 3.0
>>>>>> provides better mechanisms for moving to newer versions."
>>>>>>
>>>>>> So, the Java API should have got rid of the option to fallback to SSL 2.0
>>>>>> years before. It's a shame
>>>>>> that J2SE still supports SSL 2.0 connections in 2014. Why? Because this has
>>>>>> nothing to do with
>>>>>> compatibility but with *security*.
>>>>>>
>>>>>
>>>>> Ok. Thank you for patience with me. (really!)
>>>>>
>>>>> So there is another approach.
>>>>>
>>>>> Now the ssl2 is tested as last attempt, and if it is possible to connect like
>>>>> that, then the user is
>>>>> warned and error is thrown (which leads to skipping resource from that
>>>>>
>>>>>
>>>>> The not-checking certificate now can be allowed or forbidden by properties. By
>>>>> default it asks user
>>>>> by every such invalid certs its found. How to deal with human intraction is
>>>>> todo.
>
> This property needs to bo documented, especially for administrators.
Sure. The properties definitions support description long for time. It is jsut null in all cases :(
And new documentation generator will atuomatically include it in man pages...
If there is need to higlighti somehow, and current chngelog html+itwebsettings list or generated
manpage is not enough, maybe we can add some "since version" to definition?
Anyway it seems that tehy will be abandoned because of properties flaw you discovered.
>
> Does IcedTea-Web ask for /every/ invalid or unverifiable certificate in the certificate chain or
> just the first highest one in the chain? I am asking because the verification process can actually
> stop and assume the users answer or the property setting for all certificates beneath the first
> highest invalid or unverifiable certificate in the chain.
I think it stops on highest one.
>
>>>>> The reason for this message is to get verbose logical error emsssage, not only
>>>>> "itw do not work again"
>
> I understand. It's always nice to have sane and useful error messages. :-)
>
>> diff -r 6061af9d5eb8 netx/net/sourceforge/jnlp/cache/CacheUtil.java
>> --- a/netx/net/sourceforge/jnlp/cache/CacheUtil.java Wed Sep 03 14:45:40 2014 +0200
>> +++ b/netx/net/sourceforge/jnlp/cache/CacheUtil.java Wed Sep 03 16:02:36 2014 +0200
>> [...]
>> @@ -99,11 +99,9 @@
>> } else {
>> try {
>> // this is what URLClassLoader does
>> - URLConnection conn = location.openConnection();
>> + URLConnection conn =
>> ConnectionFactory.getConnectionFactory().openConnection(location);
>
> Please make "conn" final.
done
>
>> result = conn.getPermission();
>> - if (conn instanceof HttpURLConnection) {
>> - ((HttpURLConnection) conn).disconnect();
>> - }
>> + ConnectionFactory.getConnectionFactory().disconnect(conn);
>
> Formatting.
done
>
>> [...]
>> diff -r 6061af9d5eb8 netx/net/sourceforge/jnlp/cache/ResourceTracker.java
>> --- a/netx/net/sourceforge/jnlp/cache/ResourceTracker.java Wed Sep 03 14:45:40 2014 +0200
>> +++ b/netx/net/sourceforge/jnlp/cache/ResourceTracker.java Wed Sep 03 16:02:36 2014 +0200
>> @@ -56,6 +56,7 @@
>> import net.sourceforge.jnlp.event.DownloadEvent;
>> import net.sourceforge.jnlp.event.DownloadListener;
>> import net.sourceforge.jnlp.runtime.JNLPRuntime;
>> +import net.sourceforge.jnlp.security.ConnectionFactory;
>> import net.sourceforge.jnlp.util.HttpUtils;
>> import net.sourceforge.jnlp.util.UrlUtils;
>> import net.sourceforge.jnlp.util.WeakList;
>> @@ -641,7 +642,7 @@
>> try {
>> // create out second in case in does not exist
>> URL realLocation = resource.getDownloadLocation();
>
> "realLocation" can be final here too.
done
>
>> - con = realLocation.openConnection();
>> + con = ConnectionFactory.getConnectionFactory().openConnection(realLocation);
>> con.addRequestProperty("Accept-Encoding", "pack200-gzip, gzip");
>>
>> con.connect();
>> @@ -698,8 +699,7 @@
>> out.close();
>>
>> // explicitly close the URLConnection.
>> - if (con instanceof HttpURLConnection)
>> - ((HttpURLConnection) con).disconnect();
>> + ConnectionFactory.getConnectionFactory().disconnect(con);
>
> Is it safe to assume that "con" will never be null or ConnectionFactory.disconnect() will accept null?
Yes. And it will not, then it is probably healthy to follow npe to current place of catching.
>
>> if (packgz || gzip) {
>> // TODO why not set this otherwise?
>> @@ -811,7 +811,7 @@
>> }
>>
>> resource.setDownloadLocation(finalLocation);
>> - connection = finalLocation.openConnection(); // this won't change so should be
>> okay unsynchronized
>> + connection =
>> ConnectionFactory.getConnectionFactory().openConnection(finalLocation); // this won't change so
>> should be okay not-synchronized
>> connection.addRequestProperty("Accept-Encoding", "pack200-gzip, gzip");
>>
>> size = connection.getContentLength();
>> @@ -855,9 +855,7 @@
>> resource.fireDownloadEvent(); // fire CONNECTED
>>
>> // explicitly close the URLConnection.
>> - if (connection instanceof HttpURLConnection) {
>> - ((HttpURLConnection) connection).disconnect();
>> - }
>> + ConnectionFactory.getConnectionFactory().disconnect(connection);
>> } catch (Exception ex) {
>> OutputController.getLogger().log(ex);
>> resource.changeStatus(EnumSet.noneOf(Resource.Status.class), EnumSet.of(ERROR));
>> @@ -915,7 +913,7 @@
>> */
>> static CodeWithRedirect getUrlResponseCodeWithRedirectonResult(URL url, Map<String, String>
>> requestProperties, RequestMethods requestMethod) throws IOException {
>> CodeWithRedirect result = new CodeWithRedirect();
>> - URLConnection connection = url.openConnection();
>> + URLConnection connection = ConnectionFactory.getConnectionFactory().openConnection(url);
>
> "connection" should probably be final here too.
done
>
>> for (Map.Entry<String, String> property : requestProperties.entrySet()) {
>> connection.addRequestProperty(property.getKey(), property.getValue());
>> @@ -946,9 +944,7 @@
>> if (possibleRedirect != null && possibleRedirect.trim().length() > 0) {
>> result.URL = new URL(possibleRedirect);
>> }
>> - if (connection instanceof HttpURLConnection) {
>> - ((HttpURLConnection) connection).disconnect();
>> - }
>> + ConnectionFactory.getConnectionFactory().disconnect(connection);
>>
>> return result;
>>
>> diff -r 6061af9d5eb8 netx/net/sourceforge/jnlp/config/Defaults.java
>> --- a/netx/net/sourceforge/jnlp/config/Defaults.java Wed Sep 03 14:45:40 2014 +0200
>> +++ b/netx/net/sourceforge/jnlp/config/Defaults.java Wed Sep 03 16:02:36 2014 +0200
>> @@ -46,6 +46,7 @@
>>
>> import net.sourceforge.jnlp.ShortcutDesc;
>> import net.sourceforge.jnlp.runtime.JNLPProxySelector;
>> +import net.sourceforge.jnlp.security.ConnectionFactory;
>>
>> /**
>> * This class stores the default configuration
>> @@ -434,6 +435,27 @@
>> DeploymentConfiguration.KEY_ENABLE_MANIFEST_ATTRIBUTES_CHECK,
>> BasicValueValidators.getBooleanValidator(),
>> String.valueOf(true)
>> + },
>> + //https
>> + {
>> + DeploymentConfiguration.KEY_HTTPS_PROBINGALLOWED,
>> + BasicValueValidators.getBooleanValidator(),
>> + String.valueOf(true)
>> + },
>> + {
>> + DeploymentConfiguration.KEY_HTTPS_SYNCFORCED,
>> + BasicValueValidators.getBooleanValidator(),
>> + String.valueOf(false)
>> + },
>> + {
>> + DeploymentConfiguration.KEY_HTTPS_PROBEALWAYS,
>> + BasicValueValidators.getBooleanValidator(),
>> + String.valueOf(false)
>> + },
>> + {
>> + DeploymentConfiguration.KEY_HTTPS_ALLOWINSECURE,
>> + ConnectionFactory.InsecureStates.validator,
>> + ConnectionFactory.InsecureStates.ASK.toString()
>> }
>> };
>>
>> diff -r 6061af9d5eb8 netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java
>> --- a/netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java Wed Sep 03 14:45:40 2014 +0200
>> +++ b/netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java Wed Sep 03 16:02:36 2014 +0200
>> @@ -13,7 +13,6 @@
>> // You should have received a copy of the GNU Lesser General Public
>> // License along with this library; if not, write to the Free Software
>> // Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
>> -
>> package net.sourceforge.jnlp.config;
>>
>> import static net.sourceforge.jnlp.runtime.Translator.R;
>> @@ -66,28 +65,32 @@
...
>
> It is good of you to fix javadoc comments, but then please do it thoroughly. Please turn this
> comment into a complete sentence. ;-)
This was accidental format-all button. Reverted.
>
>> public static final String KEY_USER_LOCKS_DIR = "deployment.user.locksdir";
>> /**
>> * The netx_running file is used to indicate if any instances of netx are
>> @@ -124,8 +129,9 @@
>> /*
>> * Security and access control
>> */
>> -
>> - /** Boolean. Only show security prompts to user if true */
>> + /**
>> + * Boolean. Only show security prompts to user if true
>> + */
>
> Same goes here about javadoc. "Boolean." is certainly not a good brief description of
> "KEY_SECURITY_PROMPT_USER". Please keep in mind that javadoc assumes all text up to the first dot to
> be a brief description.
This was accidental format-all button. Reverted..
>
>> public static final String KEY_SECURITY_PROMPT_USER = "deployment.security.askgrantdialog.show";
>>
>> //enum of AppletSecurityLevel in result
>> @@ -133,23 +139,35 @@
>>
>> public static final String KEY_SECURITY_TRUSTED_POLICY = "deployment.security.trusted.policy";
>>
>> - /** Boolean. Only give AWTPermission("showWindowWithoutWarningBanner") if true */
>> + /**
>> + * Boolean. Only give AWTPermission("showWindowWithoutWarningBanner") if
>> + * true
>> + */
>
> Javadoc, see above.
same...
>
>> public static final String KEY_SECURITY_ALLOW_HIDE_WINDOW_WARNING =
>> "deployment.security.sandbox.awtwarningwindow";
>>
>> - /** Boolean. Only prompt user for granting any JNLP permissions if true */
>> + /**
>> + * Boolean. Only prompt user for granting any JNLP permissions if true
>> + */
>
> Javadoc, see above.
same...
>
>> public static final String KEY_SECURITY_PROMPT_USER_FOR_JNLP =
>> "deployment.security.sandbox.jnlp.enhanced";
>>
>> - /** Boolean. Only install the custom authenticator if true */
>> + /**
>> + * Boolean. Only install the custom authenticator if true
>> + */
>
> Javadoc, see above.
same...
>
>> public static final String KEY_SECURITY_INSTALL_AUTHENTICATOR =
>> "deployment.security.authenticator";
>>
>> /*
>> * Networking
>> */
>> -
>> - /** the proxy type. possible values are {@code JNLPProxySelector.PROXY_TYPE_*} */
>> + /**
>> + * the proxy type. possible values are
>> + * {@code JNLPProxySelector.PROXY_TYPE_*}
>> + */
>> public static final String KEY_PROXY_TYPE = "deployment.proxy.type";
>>
>> - /** Boolean. If true, the http host/port should be used for https and ftp as well */
>> + /**
>> + * Boolean. If true, the http host/port should be used for https and ftp as
>> + * well
>> + */
>
> Javadoc, see above.
same...
>
>> public static final String KEY_PROXY_SAME = "deployment.proxy.same";
>>
>> public static final String KEY_PROXY_AUTO_CONFIG_URL = "deployment.proxy.auto.config.url";
>> @@ -173,30 +191,23 @@
>> public static final String KEY_ENABLE_LOGGING_TOFILE = "deployment.log.file";
>> public static final String KEY_ENABLE_LOGGING_TOSTREAMS = "deployment.log.stdstreams";
>> public static final String KEY_ENABLE_LOGGING_TOSYSTEMLOG = "deployment.log.system";
>> -
>> +
>> /*
>> * manifest check
>> */
>> public static final String KEY_ENABLE_MANIFEST_ATTRIBUTES_CHECK =
>> "deployment.manifest.attributes.check";
>>
>> /**
>> - * Console initial status.
>> - * One of CONSOLE_* values
>> - * See declaration above:
>> - * CONSOLE_HIDE = "HIDE";
>> - * CONSOLE_SHOW = "SHOW";
>> - * CONSOLE_DISABLE = "DISABLE";
>> - * CONSOLE_SHOW_PLUGIN = "SHOW_PLUGIN_ONLY";
>> - * CONSOLE_SHOW_JAVAWS = "SHOW_JAVAWS_ONLY";
>> + * Console initial status. One of CONSOLE_* values See declaration above:
>> + * CONSOLE_HIDE = "HIDE"; CONSOLE_SHOW = "SHOW"; CONSOLE_DISABLE =
>> + * "DISABLE"; CONSOLE_SHOW_PLUGIN = "SHOW_PLUGIN_ONLY"; CONSOLE_SHOW_JAVAWS
>> + * = "SHOW_JAVAWS_ONLY";
>> */
>
> You should be probably using {@link} doc tags here.
>
same
>> public static final String KEY_CONSOLE_STARTUP_MODE = "deployment.console.startup.mode";
>> -
>> -
>>
>> /*
>> * Desktop Integration
>> */
>> -
>> public static final String KEY_JNLP_ASSOCIATIONS = "deployment.javaws.associations";
>> public static final String KEY_CREATE_DESKTOP_SHORTCUT = "deployment.javaws.shortcut";
>>
>> @@ -209,8 +220,17 @@
>> /*
>> * JVM arguments for plugin
>> */
>> - public static final String KEY_PLUGIN_JVM_ARGUMENTS= "deployment.plugin.jvm.arguments";
>> - public static final String KEY_JRE_DIR= "deployment.jre.dir";
>> + public static final String KEY_PLUGIN_JVM_ARGUMENTS = "deployment.plugin.jvm.arguments";
>> + public static final String KEY_JRE_DIR = "deployment.jre.dir";
>> +
>> + /*
>> + * https settings
>> + */
>> + public static final String KEY_HTTPS_PROBINGALLOWED =
>> "deployment.security.https.probing.allowed";
>> + public static final String KEY_HTTPS_SYNCFORCED = "deployment.security.https.syncforced";
>> + public static final String KEY_HTTPS_PROBEALWAYS = "deployment.security.https.probing.always";
>> + public static final String KEY_HTTPS_ALLOWINSECURE = "deployment.security.https.allowinsecure";
>> +
>> private ConfigurationException loadingException = null;
>>
>> public void setLoadingException(ConfigurationException ex) {
>> @@ -224,27 +244,37 @@
>> public void resetToDefaults() {
>> currentConfiguration = Defaults.getDefaults();
>> }
>> -
>>
>> public enum ConfigType {
>> +
>> System, User
>> }
>>
>> - /** is it mandatory to load the system properties? */
>> + /**
>> + * is it mandatory to load the system properties?
>> + */
>> private boolean systemPropertiesMandatory = false;
>>
>> - /** The system's subdirResult deployment.config file */
>> + /**
>> + * The system's subdirResult deployment.config file
>> + */
>
> "deployment.config" should probably be put into a {@code} doc tag to prevent terminating the brief
> description on "deployment.".
>
>> private File systemPropertiesFile = null;
>> - /** The user's subdirResult deployment.config file */
>> + /**
>> + * The user's subdirResult deployment.config file
>> + */
>
> Same goes here for "deployment.config".
>
same. (sorry for that noise)
>> private File userPropertiesFile = null;
>> -
>> +
>> /*default user file*/
>> public static final File USER_DEPLOYMENT_PROPERTIES_FILE = new File(Defaults.USER_CONFIG_HOME
>> + File.separator + DEPLOYMENT_PROPERTIES);
>>
>> - /** the current deployment properties */
>> + /**
>> + * the current deployment properties
>> + */
>> private Map<String, Setting<String>> currentConfiguration;
>>
>> - /** the deployment properties that cannot be changed */
>> + /**
>> + * the deployment properties that cannot be changed
>> + */
>> private Map<String, Setting<String>> unchangeableConfiguration;
>>
>> public DeploymentConfiguration() {
>> @@ -254,7 +284,8 @@
>>
>> /**
>> * Initialize this deployment configuration by reading configuration files.
>> - * Generally, it will try to continue and ignore errors it finds (such as file not found).
>> + * Generally, it will try to continue and ignore errors it finds (such as
>> + * file not found).
>> *
>> * @throws ConfigurationException if it encounters a fatal error.
>> */
>> @@ -266,18 +297,19 @@
>> return new File(Defaults.USER_CONFIG_HOME + File.separator + APPLET_TRUST_SETTINGS);
>> }
>>
>> - public static File getAppletTrustGlobalSettingsPath() {
>> - return new File(File.separator + "etc" + File.separator + ".java" + File.separator
>> + public static File getAppletTrustGlobalSettingsPath() {
>> + return new File(File.separator + "etc" + File.separator + ".java" + File.separator
>> + "deployment" + File.separator + APPLET_TRUST_SETTINGS);
>
> This is definitely not okay on Windows and probably not okay on Mac OS either but it seems
> acceptable for now.
removed - the files definitios changed since this time.
>
>> -
>> +
>> }
>>
>> /**
>> * Initialize this deployment configuration by reading configuration files.
>> - * Generally, it will try to continue and ignore errors it finds (such as file not found).
>> + * Generally, it will try to continue and ignore errors it finds (such as
>> + * file not found).
>> *
>> - * @param fixIssues If true, fix issues that are discovered when reading configuration by
>> - * resorting to the default values
>> + * @param fixIssues If true, fix issues that are discovered when reading
>> + * configuration by resorting to the default values
>> * @throws ConfigurationException if it encounters a fatal error.
>> */
>> public void load(boolean fixIssues) throws ConfigurationException {
>> @@ -303,7 +335,6 @@
>> * First, try to read the system's subdirResult deployment.config file to find if
>> * there is a system-level deployment.poperties file
>> */
>> -
>> if (systemConfigFile != null) {
>> if (loadSystemConfiguration(systemConfigFile)) {
>> OutputController.getLogger().log("System level " + DEPLOYMENT_CONFIG_FILE + " is
>> mandatory: " + systemPropertiesMandatory);
>> @@ -431,9 +462,9 @@
>>
>> /**
...snip...
>> + * @author jvanek
>
> Please replace with full name and e-mail address.
>
>> + */
>> +public class ConnectionFactory {
>
> Do we really want to allow sub-classes?
>
>> +
>> + public static enum InsecureStates {
>
> Naming: What are insecure states? ... axis of evil ...? :-D
kept for now. See below (otherwise yes, something like that :)) )
>
>> +
>> + TRUE, FALSE, ASK;
>
> Are "TRUE", "FALSE", and "ASK" really insecure? What makes them insecure?
>
>> +
>> + public static final ValueValidator validator = new ValueValidator() {
>
> Shouldn't this custom validator extend BasicValidator?
Not necessarily. kept for now. See below.
>
>> +
>> + @Override
>> + public void validate(Object value) throws IllegalArgumentException {
>
> Please make "value" final.
done
>
>> + if (value instanceof InsecureStates) {
>
> It's a pity ValueValidator cannot be parametrized. :-( This is a perfect example for making use of
> generics. But, lets keep it as it is for now.
Agree.
>
>> + return;
>> + }
>> + if (value instanceof String) {
>> + try {
>> + fromString((String) value);
>> + } catch (RuntimeException ex) {
>> + throw new IllegalArgumentException(ex);
>> + }
>> + }
>> + throw new IllegalArgumentException("Expected InsecureStates type or string of one
>> of " + getPossibleValues());
>
> Get this InsecureStates' name via reflection for the exception message instead of hard coding it
> into the string: InsecureStates.class.getName(). This will lead to a compile time error whenever the
> enum's name changes (for what ever reason), so the enum won't escape renaming in the string.
> Besides, this exception message should probably be localized.
Not fixed, as it will probably flow away. See below
>
>> + }
>> +
>> + @Override
>> + public String getPossibleValues() {
>> + return TRUE.toString() + " " + FALSE.toString() + " " + ASK.toString();
>> + }
>> + };
>
> Besides, the validator's definition should not be located at
> ConnectionFactory.InsecureStates.validator's initialization. Put its implementation into a named
> nested class instead of an anonymous one.
Not fixed, as it will probably flow away. See below
>
>> +
>> + public static InsecureStates fromString(String s) {
>
> Please make "s" final.
> And what about NPE? Should a NPE just break the program while incorrectly formatted strings just
> throw a RuntimeException? Seems to me like an arbitrary decision.
>
>> + if (s.trim().equalsIgnoreCase("true")) {
>> + return TRUE;
>> + }
>> + if (s.trim().equalsIgnoreCase("false")) {
>> + return FALSE;
>> + }
>> + if (s.trim().equalsIgnoreCase("ask")) {
>> + return ASK;
>> + }
>
> You know, you could use the new switch control statement here. ;-)
>
>> + throw new RuntimeException("unknown value of " + s + " for " +
>> DeploymentConfiguration.KEY_HTTPS_ALLOWINSECURE);
>
> This exception message should probably be localized too.
Not fixed, as it will probably flow away. See below
>
>> + }
>> +
>> + }
>> + private final List<URLConnection> httpsConnections = new ArrayList<>();
>> +
>> + private static class ConnectionFactoryHolder {
>> +
>> + //https://en.wikipedia.org/wiki/Double-checked_locking#Usage_in_Java
>> + //https://en.wikipedia.org/wiki/Initialization_on_demand_holder_idiom
>> + private static volatile ConnectionFactory INSTANCE = new ConnectionFactory();
>> + }
>> +
>> + public static boolean isProbingAllowed() {
>> + return getBooleanProperty(DeploymentConfiguration.KEY_HTTPS_PROBINGALLOWED);
>> + }
>> +
>> + public static boolean isSyncForced() {
>> + return !getBooleanProperty(DeploymentConfiguration.KEY_HTTPS_SYNCFORCED);
>
> Why negate the result here?
Not fixed, as it will probably flow away. See below
>
>> + }
>> +
>> + public static boolean isProbeAlways() {
>> + return getBooleanProperty(DeploymentConfiguration.KEY_HTTPS_PROBEALWAYS);
>> + }
>> +
>> + public static InsecureStates getInsecureStatus() {
>> + return
>> InsecureStates.fromString(JNLPRuntime.getConfiguration().getProperty(DeploymentConfiguration.KEY_HTTPS_ALLOWINSECURE));
>>
>> + }
>> +
>> + public static boolean isInsecurePossible() {
>
> Do you see now why I insist on meaingful naming? What does it mean when "insecure is possible"
> (isInsecurePossible() == true)? Insecure certificates, insecure connections? Does this method detect
> whether insecure connections are possible to hosts? What does it mean?
I see it since beggining. Thenaming is curent days more important thenalgotihm itself. I got
harm-locked (harm is less then dead ,isnt it?) a bit on this. Not fixed here as it will proabbly
flow away.
>
>> + InsecureStates i = getInsecureStatus();
>> + if (i.equals(InsecureStates.TRUE) || i.equals(InsecureStates.ASK)) {
>> + return true;
>> + }
>> + return false;
>> + }
>> +
>> + public static boolean getBooleanProperty(String s) {
>> + String s3 = JNLPRuntime.getConfiguration().getProperty(s);
>
> What does "s3" mean? Anyway, please make it final.
>
fixed.
>> + if (s3 == null || s3.trim().isEmpty()) {
>> + return false;
>> + } else {
>
> No need for "else:. ;-)
I have heard that the else is a bit better. I kept it witrh else. But otherwise Idont have personal
prefferences.
>
>> + return Boolean.valueOf(s3);
>> + }
>> +
>> + }
>> +
>> + public static ConnectionFactory getConnectionFactory() {
>> + return ConnectionFactoryHolder.INSTANCE;
>> + }
>> +
>> + public URLConnection openConnection(URL url) throws IOException {
>> + OutputController.getLogger().log("Connecting " + url.toExternalForm());
>
> "Connecting *to* ..."
ok.
>
>> + 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.
> synchronious 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 solutionand will roably appear in some next
iterationchangeset
>
>> + }
>> + return openHttpsConnection(url);
>> + } else {
>> + OutputController.getLogger().log("done " + url.toExternalForm());
>> + return url.openConnection();
>> + }
>> + }
>> +
>> + private synchronized URLConnection openHttpsConnection(URL url) throws IOException {
>> + URLConnection conn = null;
>> + //yes,by default we assume that once initialized https settings are valid for all resources
>> + //note the combination of async and try every time have no sense (it *may* change the
>> opened connections => error
>> + OutputController.getLogger().log("Probing " + url.toExternalForm());
>> + if (HttpsSettings.getHttpsSettings() == null || isProbeAlways()) {
>> + HttpsProbeResult pr = HttpsSettings.initHttpsSettings(url);
>> + if (pr.settings != null && pr.conn != null) {
>> + conn = pr.conn;
>> + }
>> + } else {
>> + conn = HttpsSettings.getHttpsSettings().openConnection(url);
>> + }
>> + httpsConnections.add(conn);
>
> "conn" can still be null and be added to "httpsConnections". This is probably not intended.
This is now sanitized.
>
>> + OutputController.getLogger().log("done " + url.toExternalForm());
>> + return conn;
>
> Same here, because "conn" can be null, null may be returned.
nope. The null connection s correctly returned.
>
>> + }
>> +
>> + public void disconnect(URLConnection conn) {
>> + if (conn instanceof HttpsURLConnection) {
>> + disconnectHttps((HttpsURLConnection) conn);
>> + } else {
>> + if (conn instanceof HttpURLConnection) {
>> + ((HttpURLConnection) conn).disconnect();
>> + }
>> + }
>> + }
>> +
>> + public synchronized void disconnectHttps(HttpsURLConnection conn) {
>> + conn.disconnect();
>
> Possible NPE.
Which should be ok. But no null shold pass the instanceof chkc.
Method changed to private.
>
>> + //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 predicion 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 meanhere.
>
>> + }
>> +
>> + }
>> + }
>> +
>> + private static class HttpsProbeResult {
>> +
>> + URLConnection conn;
>> + HttpsSettings settings;
>> + }
>> +
>> + private static class HttpsSettings {
>> +
>> + private static HttpsSettings INSTANCE;
>> + private static SSLContext ctxOrig;
>> +
>> + @Override
>> + public String toString() {
>> + return "SSL 2.0: " + ssl2 + ", insecure " + insecure;
>> + }
>> +
>> + public static HttpsSettings getHttpsSettings() {
>> + return INSTANCE;
>> + }
>> +
>> + private static HttpsProbeResult initHttpsSettings(URL initUrl) throws IOException {
>> + try {
>> + if (ctxOrig == null) {
>> + ctxOrig = SSLContext.getDefault();
>> + }
>> + HttpsProbeResult pr = new HttpsProbeResult();
>> + try {
>> + pr.settings = new HttpsSettings(false, false);
>> + pr.conn = pr.settings.openConnection(initUrl);
>> + INSTANCE = pr.settings;
>> + return pr;
>> + } catch (IOException e) {
>> + OutputController.getLogger().log(e);
>> + }
>> + if (isInsecurePossible()) {
>> + try {
>> + OutputController.getLogger().log("Probing " + initUrl.toExternalForm() +
>> " without certificate check.");
>> + pr.settings = new HttpsSettings(false, true);
>> + pr.conn = pr.settings.openConnection(initUrl);
>> + if (getInsecureStatus().equals(InsecureStates.ASK)) {
>> + //TODO dialog for not trusted cert dialog
>> + }
>> + INSTANCE = pr.settings;
>> + return pr;
>> + } catch (IOException e) {
>> + OutputController.getLogger().log(e);
>> + }
>> + }
>> + HttpURLConnection conn = null;
>> + try {
>> + OutputController.getLogger().log("Probing " + initUrl.toExternalForm() + "
>> with SSL 2.0 handshake.");
>> + conn = (HttpURLConnection) HttpsSettings.openConnection(initUrl, true, false);
>> + String s = "The " + initUrl.toExternalForm() + " was accessible only by
>> insecure SSL 2.0 handshake. This is forbidden. Contact its administrator.";
>> + OutputController.getLogger().log(OutputController.Level.WARNING_ALL, s);
>> + throw new RuntimeException(s);
>> + } catch (IOException e) {
>> + OutputController.getLogger().log(e);
>> + } finally {
>> + if (conn != null) {
>> + conn.disconnect();
>> + }
>> + }
>> + conn = null;
>> + if (isInsecurePossible()) {
>> + try {
>> + OutputController.getLogger().log("Probing " + initUrl.toExternalForm() +
>> " with SSL 2.0 and without certificate check.");
>> + conn = (HttpURLConnection) HttpsSettings.openConnection(initUrl, true,
>> true);
>> + String s = "The " + initUrl.toExternalForm() + " was accessible only by
>> insecure SSL 2.0 handshake and with unchecked certificate. The SSL 2.0 handshake is forbidden.
>> Contact its administrator.";
>> + OutputController.getLogger().log(OutputController.Level.WARNING_ALL, s);
>> + throw new RuntimeException(s);
>> + } catch (IOException e) {
>> + OutputController.getLogger().log(e);
>> + } finally {
>> + if (conn != null) {
>> + conn.disconnect();
>> + }
>> + }
>> + }
>> + throw new IOException("All probing methods on " + initUrl.toExternalForm() +
>> "failed.");
>> + } catch (NoSuchAlgorithmException ex) {
>> + throw new IOException(ex);
>> + }
>> + }
>> +
>> + private final boolean ssl2;
>> + private final boolean insecure;
>> +
>> + private HttpsSettings(boolean ssl2, boolean insecure) {
>> + this.ssl2 = ssl2;
>> + this.insecure = insecure;
>> + }
>> +
>> + private URLConnection openConnection(URL url) throws IOException {
>> + return openConnection(url, ssl2, insecure);
>> + }
>> +
>> + 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 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.
Theissue helpcrypto is facing with jdk8 have solution based on properties to:(
>
>> + if (insecure) {
>> + // configure the SSLContext without a TrustManager
>> + SSLContext ctx = SSLContext.getInstance("TLS");
>> + ctx.init(new KeyManager[0], new TrustManager[]{new DummyTrustManager()}, new
>> SecureRandom());
>> + SSLContext.setDefault(ctx);
>> + } else {
>> + SSLContext.setDefault(ctxOrig);
>> + }
>> + HttpsURLConnection conn = (HttpsURLConnection) url.openConnection();
>> + if (insecure) {
>> + conn.setHostnameVerifier(new HostnameVerifier() {
>> + @Override
>> + public boolean verify(String arg0, SSLSession arg1) {
>> + return true;
>> + }
>> + });
>> + }
>> +
>> + return conn;
>> + } catch (KeyManagementException | NoSuchAlgorithmException ex) {
>> + throw new IOException(ex);
>> + }
>> + }
>> +
>> + private static class DummyTrustManager implements X509TrustManager {
>> +
>> + public DummyTrustManager() {
>> + }
>> +
>> + @Override
>> + public void checkClientTrusted(X509Certificate[] arg0, String arg1) throws
>> CertificateException {
>> + }
>> +
>> + @Override
>> + public void checkServerTrusted(X509Certificate[] arg0, String arg1) throws
>> CertificateException {
>> + }
>> +
>> + @Override
>> + public X509Certificate[] getAcceptedIssuers() {
>> + return null;
>> + }
>> + }
>> + }
>> +
>> +}
>
> So much reviewing for now. I'll get back to it when you have a patch based on the current tip ready.
>
Thank you very much!
Well the issue with proeprties is show stopper.
I have not yet touch the paper with pen on fixing it, As I would like to know your opinion.
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";
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.
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
deployment.security.https.probing.always = true
deployment.security.https.allowinsecure = ask
set
but the mian difference will be, that in disconnectHttps method,the https will be cleaned to default
state.
What do you think?
Until you stop me, I will work on solution like this.
Thank you for hints!
J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: httpsSolution_06.patch
Type: text/x-patch
Size: 30535 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140923/e516e939/httpsSolution_06-0001.patch>
More information about the distro-pkg-dev
mailing list