Alon Bar-Lev has posted comments on this change.
Change subject: WIP Support foreman SSL provider
......................................................................
Patch Set 2: (15 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetProviderCertificateChainQuery.java
Line 24: HostProviderProxy proxy = ((HostProviderProxy)
ProviderProxyFactory.getInstance().create(hostProvider));
Line 25:
getQueryReturnValue().setReturnValue(chainToString(proxy.getCertificateChain()));
Line 26: }
Line 27:
Line 28: private List<String> chainToString(List<X509Certificate> chain) {
Java has standard object for chain it is CertPath. I also suggest you be able
to return multiple chains.
Line 29: if (chain != null) {
Line 30: List<String> stringChain = new ArrayList<String>();
Line 31: for( X509Certificate certificate : chain ) {
Line 32: StringBuilder certStringBuilder = new StringBuilder();
Line 34: certStringBuilder.append('\n');
Line 35: stringChain.add(certStringBuilder.toString());
Line 36: }
Line 37: return stringChain;
Line 38: }
else?
Line 39: return null;
Line 40: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/host/provider/foreman/ForemanHostProviderProxy.java
Line 124: throw new VdcBLLException(VdcBllErrors.PROVIDER_FAILURE,
e.getMessage());
Line 125: }
Line 126: }
Line 127:
Line 128: private void initHttpClient(String hostUrlString) {
Why isn't the implementation is in base class?
Line 129: try {
Line 130: hostUrl = new URL(hostUrlString);
Line 131: if
(hostUrl.getProtocol().equalsIgnoreCase(HTTPS_PROTOCOL)) {
Line 132: URL trustStorePath = new URL(FILE_URL_PREFIX +
EngineLocalConfig.getInstance().getPKIExternalTrustStore());
Line 131: if
(hostUrl.getProtocol().equalsIgnoreCase(HTTPS_PROTOCOL)) {
Line 132: URL trustStorePath = new URL(FILE_URL_PREFIX +
EngineLocalConfig.getInstance().getPKIExternalTrustStore());
Line 133: String trustStorePassword =
EngineLocalConfig.getInstance().getPKIExternalTrustStorePassword();
Line 134: boolean enableSniExtension =
EngineLocalConfig.getInstance().isEnableSniExtension();
Line 135: System.setProperty("jsse.enableSNIExtension",
String.valueOf(enableSniExtension));
What do you mean "The configuration for it is not part of the engine
configuration"?
If it were not, then it should have been in its own repository entirely
disconnected from engine.
If it is within engine repository and released with engine then it is part of
engine.
Anyway, you can always install a file at
/etc/ovirt-engine/engine.conf.d/20-foreman or something with specific
configuration.
1. Changing jboss/jvm global state should not be with function.
2. Even if you are to change the jboss/jvm state, it should be in static
context.
3. I repeat (1), cannot be that you change the state of ssl for the entire jvm
in a plugin/provider, what if you break other components?
Line 136: int hostPort = hostUrl.getPort();
Line 137: if (hostPort == -1) {
Line 138: hostPort = DEFAULT_SECURED_PORT;
Line 139: }
Line 134: boolean enableSniExtension =
EngineLocalConfig.getInstance().isEnableSniExtension();
Line 135: System.setProperty("jsse.enableSNIExtension",
String.valueOf(enableSniExtension));
Line 136: int hostPort = hostUrl.getPort();
Line 137: if (hostPort == -1) {
Line 138: hostPort = DEFAULT_SECURED_PORT;
are you sure httpclient does not have these constants?
Line 139: }
Line 140: Protocol httpsProtocol = new Protocol("https", new
AuthSSLProtocolSocketFactory(null, null, trustStorePath, trustStorePassword),
hostPort);
Line 141:
httpClient.getHostConfiguration().setHost(hostUrl.getHost(), hostPort,
httpsProtocol);
Line 142: this.isSecured = true;
Line 142: this.isSecured = true;
Line 143: } else {
Line 144: int hostPort = hostUrl.getPort();
Line 145: if (hostPort == -1) {
Line 146: hostPort = DEFAULT_PORT;
same for here... I almost sure this class gets URL.
Line 147: }
Line 148:
httpClient.getHostConfiguration().setHost(hostUrl.getHost(), hostPort);
Line 149: }
Line 150:
objectMapper.configure(Feature.FAIL_ON_UNKNOWN_PROPERTIES, false);
Line 157: }
Line 158: }
Line 159:
Line 160: @Override
Line 161: public List<X509Certificate> getCertificateChain() {
Why is the implementation is not at the base class?
Line 162: if (!isSecured) {
Line 163: return null;
Line 164: } else {
Line 165: HttpsURLConnection conn = null;
Line 196: conn.connect();
Line 197: return chain;
Line 198: } catch (SSLHandshakeException e) {
Line 199: return chain;
Line 200: } catch (NoSuchAlgorithmException e) {
So catch only one entry... Exception, no?
Line 201: throw new
VdcBLLException(VdcBllErrors.PROVIDER_FAILURE, e.getMessage());
Line 202: } catch (KeyManagementException e) {
Line 203: throw new
VdcBLLException(VdcBllErrors.PROVIDER_FAILURE, e.getMessage());
Line 204: } catch (IOException e) {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/provider/ImportProviderCertificateChainCommand.java
Line 68: VdcObjectType.System,
Line 69: ActionGroup.CREATE_STORAGE_POOL));
Line 70: }
Line 71:
Line 72: private void saveChainToTrustStore(List<X509Certificate> chain) {
This probably need to be CertPath
Line 73: if (chain != null && chain.size() > 0) {
Line 74: try {
Line 75: File trustStore =
EngineLocalConfig.getInstance().getPKIExternalTrustStore();
Line 76: String trustStorePassword =
EngineLocalConfig.getInstance().getPKIExternalTrustStorePassword();
Line 80: ks = KeyStore.getInstance(KeyStore.getDefaultType());
Line 81: ks.load(in, trustStorePassword.toCharArray());
Line 82: in.close();
Line 83: OutputStream out = new FileOutputStream(trustStore);
Line 84: for (int certIndex = 0; certIndex < chain.size();
++certIndex) {
so you are to store only one chain? as the alias will conflict next chain.
BTW: you should not store the end certificate unless you want explicit trust
the end certificate and not the chain. And in this case you do not need the
chain, only the end certificate.
Line 85: X509Certificate cert = chain.get(certIndex);
Line 86: String alias = getProviderName() + "-" +
(certIndex + 1);
Line 87: ks.setCertificateEntry(alias, cert);
Line 88: }
Line 86: String alias = getProviderName() + "-" +
(certIndex + 1);
Line 87: ks.setCertificateEntry(alias, cert);
Line 88: }
Line 89: ks.store(out, trustStorePassword.toCharArray());
Line 90: out.close();
should be in finally.
Line 91: setSucceeded(true);
Line 92: } catch (NoSuchAlgorithmException e) {
Line 93: throw new
VdcBLLException(VdcBllErrors.PROVIDER_IMPORT_CERTIFICATE_CHAIN_ERROR,
e.getMessage());
Line 94: } catch (CertificateException e) {
Line 89: ks.store(out, trustStorePassword.toCharArray());
Line 90: out.close();
Line 91: setSucceeded(true);
Line 92: } catch (NoSuchAlgorithmException e) {
Line 93: throw new
VdcBLLException(VdcBllErrors.PROVIDER_IMPORT_CERTIFICATE_CHAIN_ERROR,
e.getMessage());
no need for multiple catches with same logic.
Line 94: } catch (CertificateException e) {
Line 95: throw new
VdcBLLException(VdcBllErrors.PROVIDER_IMPORT_CERTIFICATE_CHAIN_ERROR,
e.getMessage());
Line 96: } catch (IOException e) {
Line 97: throw new
VdcBLLException(VdcBllErrors.PROVIDER_IMPORT_CERTIFICATE_CHAIN_ERROR,
e.getMessage());
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/provider/ProviderProxy.java
Line 16: * Get the certificate chain of the provider.<br>
Line 17: * Useful when the provider is secured.
Line 18: * @return List of X509Certificate objects
Line 19: */
Line 20: List<X509Certificate> getCertificateChain();
This should probably need to be CertPath and receive URL.
Line 21:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/GetProviderCertificateChainParameters.java
Line 3: import javax.validation.Valid;
Line 4:
Line 5: import org.ovirt.engine.core.common.businessentities.Provider;
Line 6:
Line 7: public class GetProviderCertificateChainParameters extends
VdcQueryParametersBase {
I think these should get plain URL and have nothing to do with 'Provider'
Line 8:
Line 9: private static final long serialVersionUID = 308877238121233739L;
Line 10:
Line 11: @Valid
....................................................
File packaging/etc/pki/installCA.sh
Line 78: keytool -import -noprompt -trustcacerts -alias cacert -keypass "$PASS"
-file certs/ca.der -keystore ./.truststore -storepass "$PASS"
Line 79:
Line 80: # Generate the external truststore also trusting the CA certificate
Line 81: keytool -import -noprompt -trustcacerts -alias cacert -keypass "$PASS"
-file certs/ca.der -keystore ./.truststore_external -storepass "$PASS"
Line 82:
You do not need our ca within the trust store.
I don't think it belongs here.
I even don't think it should be at /etc/pki, better place for these are at
/var/lib/ovirt-engine as this file is not managed by administrator manually.
And no need for the '.' prefix :)
Line 83: echo " "
Line 84: echo "} Creating client certificates for oVirt..."
Line 85: enroll_certificate engine "$PASS"
"/C=${COUNTRY}/O=${ORG}/CN=${SUBJECT}"
Line 86: enroll_certificate apache "$PASS"
"/C=${COUNTRY}/O=${ORG}/CN=${SUBJECT}"
--
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: 2
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
