Oved Ourfali has posted comments on this change.

Change subject: WIP Support foreman SSL provider
......................................................................


Patch Set 3: (7 inline comments)

....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/provider/BaseProviderProxy.java
Line 74:                 );
Line 75:                 conn = (HttpsURLConnection) url.openConnection();
Line 76:                 conn.setSSLSocketFactory(ctx.getSocketFactory());
Line 77:                 conn.connect();
Line 78:                 return chain;
No.
Will check that in the future.
For now we don't expect providers that are known/trusted by the JRE.
Line 79:             } catch (SSLHandshakeException e) {
Line 80:                 return chain;
Line 81:             } catch (NoSuchAlgorithmException e) {
Line 82:                 handleException(VdcBllErrors.PROVIDER_FAILURE, e);


Line 75:                 conn = (HttpsURLConnection) url.openConnection();
Line 76:                 conn.setSSLSocketFactory(ctx.getSocketFactory());
Line 77:                 conn.connect();
Line 78:                 return chain;
Line 79:             } catch (SSLHandshakeException e) {
As the handshake will fail if the host isn't trusted, host name isn't verified, 
etc, but in this case we would also like to get the chain back, and make it 
trusted.
Line 80:                 return chain;
Line 81:             } catch (NoSuchAlgorithmException e) {
Line 82:                 handleException(VdcBllErrors.PROVIDER_FAILURE, e);
Line 83:             } catch (KeyManagementException e) {


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/provider/ExternalTrustStoreInitializer.java
Line 10: import org.ovirt.engine.core.utils.EngineLocalConfig;
Line 11: 
Line 12: public class ExternalTrustStoreInitializer {
Line 13: 
Line 14:     private static final String FILE_URL_PREFIX = "file:";
Weird... it worked well even without it....
Will change that.
Line 15: 
Line 16:     public static String getTrustStorePath() {
Line 17:         File varDir = EngineLocalConfig.getInstance().getVarDir();
Line 18:         return varDir + "/" + "external_truststore";


Line 36:                 // Passing null stream will create a new empty trust 
store
Line 37:                 trustStore.load(null, 
getTrustStorePassword().toCharArray());
Line 38:                 trustStore.store(out, 
getTrustStorePassword().toCharArray());
Line 39:             } catch (Exception e) {
Line 40:                 e.printStackTrace();
Autogenerated code. Will fix.
Line 41:             }
Line 42:             finally {
Line 43:                 if (out != null) {
Line 44:                     try {


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/provider/ImportProviderCertificateChainCommand.java
Line 80:                 ks = KeyStore.getInstance(KeyStore.getDefaultType());
Line 81:                 ks.load(in, trustStorePassword.toCharArray());
Line 82:                 out = new FileOutputStream(trustStore);
Line 83:                 // In case there is only one certificate, we insert it.
Line 84:                 // Otherwise, we need to insert the entire chain 
except the end certificate
If the user authorizes a chain with one certificate, then it is implied that he 
wants this trusted... don't want to make the UI over-complicated as I see no 
need to do that in this case.
Line 85:                 int numberOfCertificatesToInsert = chain.size() == 1 ? 
1 : chain.size() - 1;
Line 86:                 for (int certIndex = 0; certIndex < 
numberOfCertificatesToInsert; ++certIndex) {
Line 87:                     Certificate certificate = chain.get(certIndex);
Line 88:                     String alias = Guid.NewGuid().toString();


Line 96:                 
handleException(VdcBllErrors.PROVIDER_IMPORT_CERTIFICATE_CHAIN_ERROR, e);
Line 97:             } catch (IOException e) {
Line 98:                 
handleException(VdcBllErrors.PROVIDER_IMPORT_CERTIFICATE_CHAIN_ERROR, e);
Line 99:             } catch (KeyStoreException e) {
Line 100:                 
handleException(VdcBllErrors.PROVIDER_IMPORT_CERTIFICATE_CHAIN_ERROR, e);
Already answered that one. It is a matter of style, and I do see a value in 
that, allowing to see what exceptions can be expected here. Can be useful when 
maintaining the code in the future.
Line 101:             }
Line 102:             finally {
Line 103:                 if (in != null) {
Line 104:                     try {


....................................................
File 
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/EngineLocalConfig.java
Line 62:     public boolean isHttpEnabled() {
Line 63:         return getBoolean("ENGINE_HTTP_ENABLED");
Line 64:     }
Line 65: 
Line 66:     public boolean isEnableSniExtension() {
Yes. Was removed in the next patch set... but I didn't push it fast enough :-)
nice catch :-)
Line 67:         return getBoolean("ENGINE_ENABLE_SNI_EXTENSION");
Line 68:     }
Line 69: 
Line 70:     public int getHttpPort() {


--
To view, visit http://gerrit.ovirt.org/15128
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I35343409d74a4f90aae726b46781f27ce08a981a
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Oved Ourfali <oourf...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to