Oved Ourfali has posted comments on this change.

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


Patch Set 1: (4 inline comments)

Thank you very much for your review.
See comments inline.
Also, as for the creation of the trust store, I'd want to create it.
Where is the code responsible for creating the system truststore we use today?

Oved

....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/host/provider/foreman/ForemanHostProviderProxy.java
Line 47:         this.hostProvider = hostProvider;
Line 48:         try {
Line 49:             URL hostUrl = new URL(hostProvider.getUrl());
Line 50:             if 
(hostUrl.getProtocol().equalsIgnoreCase(HTTPS_PROTOCOL)) {
Line 51:                 String trustStorePath = FILE_URL_PREFIX + 
EngineLocalConfig.getInstance().getPKIExternalTrustStore();
I'd want to create it on setup/upgrade. so such a scenario shouldn't happen, 
but, obviously if missing then SSL communication won't work here.
Line 52:                 String trustStorePassword = 
EngineLocalConfig.getInstance().getPKIExternalTrustStorePassword();
Line 53:                 httpClient = new SecuredHostHttpClient(hostUrl, new 
URL(trustStorePath), trustStorePassword, false);
Line 54:             } else {
Line 55:                 httpClient = new HttpClient();


Line 49:             URL hostUrl = new URL(hostProvider.getUrl());
Line 50:             if 
(hostUrl.getProtocol().equalsIgnoreCase(HTTPS_PROTOCOL)) {
Line 51:                 String trustStorePath = FILE_URL_PREFIX + 
EngineLocalConfig.getInstance().getPKIExternalTrustStore();
Line 52:                 String trustStorePassword = 
EngineLocalConfig.getInstance().getPKIExternalTrustStorePassword();
Line 53:                 httpClient = new SecuredHostHttpClient(hostUrl, new 
URL(trustStorePath), trustStorePassword, false);
It looked both cleaner to me, and it might be useful in other cases as well 
(thought of moving it to utils or something).
Also, it is not the only thing I do in SecuredHostHttpClient
Line 54:             } else {
Line 55:                 httpClient = new HttpClient();
Line 56:             }
Line 57:             objectMapper.configure(Feature.FAIL_ON_UNKNOWN_PROPERTIES, 
false);


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/host/provider/foreman/SecuredHostHttpClient.java
Line 10: 
Line 11:     private static final int DEFAULT_SECURED_PORT = 443;
Line 12:     public SecuredHostHttpClient(URL hostUrl, URL trustStorePath, 
String trustStorePassword, boolean enableSniExtension) {
Line 13:         super();
Line 14:         System.setProperty ("jsse.enableSNIExtension", 
String.valueOf(enableSniExtension));
I'm aware of that.
However, without doing that I always failed on the error:
javax.net.ssl.SSLProtocolException: handshake alert: unrecognized_name

Still haven't found another way to overcome that one.
Line 15:         int hostPort = hostUrl.getPort();
Line 16:         if (hostPort == -1) {
Line 17:             hostPort = DEFAULT_SECURED_PORT;
Line 18:         }


Line 16:         if (hostPort == -1) {
Line 17:             hostPort = DEFAULT_SECURED_PORT;
Line 18:         }
Line 19:         Protocol httpsProtocol = new Protocol("https", new 
AuthSSLProtocolSocketFactory(null, null, trustStorePath, trustStorePassword),  
hostPort);
Line 20:         Protocol.registerProtocol("https", httpsProtocol);
You're right, but, "https" is just an ID when doing the Protocol registration. 
I can put a more specific ID here, that would prevent that:
        Protocol httpsProtocol = new Protocol("https", new 
AuthSSLProtocolSocketFactory(null, null, trustStorePath, trustStorePassword),  
hostPort);
        Protocol.registerProtocol(hostUrl.toString(), httpsProtocol);

So it wouldn't change the https protocol, but just change it specifically for 
this endpoint.
Testing that now, to see if it indeed works.
Line 21:         getHostConfiguration().setHost(hostUrl.getHost(), 
hostUrl.getPort(), httpsProtocol);
Line 22:     }
Line 23: 


--
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: 1
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>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to