Alon Bar-Lev has posted comments on this change.
Change subject: WIP Support foreman SSL provider
......................................................................
Patch Set 1: (4 inline comments)
Create the truststore during installation or ignore if not exist?
....................................................
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();
what if store is missing?
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);
Can't it be as simple as:
httpClient = new HttpClient();
httpclient.getHostConfiguration().setHost(
hostUrl.getHost(),
hostUrl.getPort(),
new Protocol(
"myhttps",
new AuthSSLProtocolSocketFactory(null, null,
trustStorePath, trustStorePassword),
0
)
)
I mean... why do you create a new abstraction for single call of function?
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));
This is changing the global state of jre, should not be done in class.
Anyway, why do you need this?
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 do not need to change the global https state within jre, not in this class
anyway.
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 <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Mike Kolesnik <[email protected]>
Gerrit-Reviewer: Oved Ourfali <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
