Skip to content

Commit

Permalink
CLOUDSTACK-9993: Fixes from code review
Browse files Browse the repository at this point in the history
Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
  • Loading branch information
rohityadavcloud committed Aug 28, 2017
1 parent f5e2b88 commit f12b00e
Show file tree
Hide file tree
Showing 14 changed files with 205 additions and 158 deletions.
23 changes: 13 additions & 10 deletions agent/src/com/cloud/agent/Agent.java
Original file line number Diff line number Diff line change
Expand Up @@ -254,8 +254,10 @@ public void start() {
s_logger.info("Attempted to connect to the server, but received an unexpected exception, trying again...");
}
while (!_connection.isStartup()) {
final String host = _shell.getHost();
_shell.getBackoffAlgorithm().waitBeforeRetry();
_connection = new NioClient("Agent", _shell.getHost(), _shell.getPort(), _shell.getWorkers(), this);
_connection = new NioClient("Agent", host, _shell.getPort(), _shell.getWorkers(), this);
s_logger.info("Connecting to host:" + host);
try {
_connection.start();
} catch (final NioConnectionException e) {
Expand Down Expand Up @@ -431,9 +433,10 @@ protected void reconnect(final Link link) {
_shell.getBackoffAlgorithm().waitBeforeRetry();
}

final String host = _shell.getHost();
do {
_connection = new NioClient("Agent", _shell.getHost(), _shell.getPort(), _shell.getWorkers(), this);
s_logger.info("Reconnecting...");
_connection = new NioClient("Agent", host, _shell.getPort(), _shell.getWorkers(), this);
s_logger.info("Reconnecting to host:" + host);
try {
_connection.start();
} catch (final NioConnectionException e) {
Expand Down Expand Up @@ -493,7 +496,7 @@ protected void processRequest(final Request request, final Link link) {

for (int i = 0; i < cmds.length; i++) {
final Command cmd = cmds[i];
Answer answer = null;
Answer answer;
try {
if (cmd.getContextParam("logid") != null) {
MDC.put("logcontextid", cmd.getContextParam("logid"));
Expand Down Expand Up @@ -607,8 +610,8 @@ public Answer setupAgentKeystore(final SetupKeyStoreCommand cmd) {
if (agentFile == null) {
return new Answer(cmd, false, "Failed to find agent.properties file");
}
final String keyStoreFile = agentFile.getParent() + KeyStoreUtils.defaultKeystoreFile;
final String csrFile = agentFile.getParent() + KeyStoreUtils.defaultCsrFile;
final String keyStoreFile = agentFile.getParent() + "/" + KeyStoreUtils.defaultKeystoreFile;
final String csrFile = agentFile.getParent() + "/" + KeyStoreUtils.defaultCsrFile;

String storedPassword = _shell.getPersistentProperty(null, KeyStoreUtils.passphrasePropertyName);
if (Strings.isNullOrEmpty(storedPassword)) {
Expand Down Expand Up @@ -647,10 +650,10 @@ private Answer setupAgentCertificate(final SetupCertificateCommand cmd) {
if (agentFile == null) {
return new Answer(cmd, false, "Failed to find agent.properties file");
}
final String keyStoreFile = agentFile.getParent() + KeyStoreUtils.defaultKeystoreFile;
final String certFile = agentFile.getParent() + KeyStoreUtils.defaultCertFile;
final String privateKeyFile = agentFile.getParent() + KeyStoreUtils.defaultPrivateKeyFile;
final String caCertFile = agentFile.getParent() + KeyStoreUtils.defaultCaCertFile;
final String keyStoreFile = agentFile.getParent() + "/" + KeyStoreUtils.defaultKeystoreFile;
final String certFile = agentFile.getParent() + "/" + KeyStoreUtils.defaultCertFile;
final String privateKeyFile = agentFile.getParent() + "/" + KeyStoreUtils.defaultPrivateKeyFile;
final String caCertFile = agentFile.getParent() + "/" + KeyStoreUtils.defaultCaCertFile;

try {
FileUtils.writeStringToFile(new File(certFile), certificate, Charset.defaultCharset());
Expand Down
7 changes: 4 additions & 3 deletions agent/src/com/cloud/agent/AgentShell.java
Original file line number Diff line number Diff line change
Expand Up @@ -108,12 +108,13 @@ public String getPod() {

@Override
public String getHost() {
String[] hosts = _host.split(",");
final String[] hosts = _host.split(",");
if (_hostCounter >= hosts.length) {
_hostCounter = 0;
}
s_logger.info("Connecting to host: " + hosts[_hostCounter % hosts.length]);
return hosts[_hostCounter++ % hosts.length];
final String host = hosts[_hostCounter % hosts.length];
_hostCounter++;
return host;
}

public void setHost(final String host) {
Expand Down
2 changes: 1 addition & 1 deletion agent/src/com/cloud/agent/dao/impl/PropertiesStorage.java
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public synchronized String get(String key) {
@Override
public synchronized void persist(String key, String value) {
if (!loadFromFile(_file)) {
s_logger.warn("Failed to load changes and then write to them");
s_logger.error("Failed to load changes and then write to them");
}
_properties.setProperty(key, value);
FileOutputStream output = null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@
@APICommand(name = IssueCertificateCmd.APINAME,
description = "Issues a client certificate using configured or provided CA plugin",
responseObject = CertificateResponse.class,
requestHasSensitiveInfo = false,
responseHasSensitiveInfo = false,
requestHasSensitiveInfo = true,
responseHasSensitiveInfo = true,
since = "4.11.0",
authorized = {RoleType.Admin})
public class IssueCertificateCmd extends BaseAsyncCmd {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
@APICommand(name = RevokeCertificateCmd.APINAME,
description = "Revokes certificate using configured CA plugin",
responseObject = SuccessResponse.class,
requestHasSensitiveInfo = false,
requestHasSensitiveInfo = true,
responseHasSensitiveInfo = false,
since = "4.11.0",
authorized = {RoleType.Admin})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1095,7 +1095,6 @@ public void orchestrateStart(final String vmUuid, final Map<VirtualMachineProfil
} else {
s_logger.error("Failed to setup keystore and generate CSR for system vm: " + vm.getInstanceName());
}

}
return;
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public interface CAProvider {

/**
* Method returns capability of the plugin to participate in certificate issuance, revocation and provisioning
* @return
* @return returns true when CA provider can do certificate lifecycle tasks
*/
boolean canProvisionCertificates();

Expand All @@ -46,7 +46,7 @@ public interface CAProvider {
* @param domainNames
* @param ipAddresses
* @param validityDays
* @return
* @return returns issued certificate
*/
Certificate issueCertificate(final List<String> domainNames, final List<String> ipAddresses, final int validityDays);

Expand All @@ -56,7 +56,7 @@ public interface CAProvider {
* @param domainNames
* @param ipAddresses
* @param validityDays
* @return
* @return returns issued certificate using provided CSR and other options
*/
Certificate issueCertificate(final String csr, final List<String> domainNames, final List<String> ipAddresses, final int validityDays);

Expand All @@ -73,21 +73,21 @@ public interface CAProvider {
* @param sslContext The SSL context used while accepting a client connection
* @param remoteAddress
* @param certMap
* @return
* @return returns created SSL engine instance
* @throws GeneralSecurityException
* @throws IOException
*/
SSLEngine createSSLEngine(final SSLContext sslContext, final String remoteAddress, final Map<String, X509Certificate> certMap) throws GeneralSecurityException, IOException;

/**
* Returns the unique name of the provider
* @return
* @return returns provider name
*/
String getProviderName();

/**
* Returns description about the CA provider plugin
* @return
* @return returns description
*/
String getDescription();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
// Licensed to the Apache Software Foundation (ASF) under one
// or more contributor license agreements. See the NOTICE file
// distributed with this work for additional information
// regarding copyright ownership. The ASF licenses this file
// to you under the Apache License, Version 2.0 (the
// "License"); you may not use this file except in compliance
// with the License. You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing,
// software distributed under the License is distributed on an
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.

package org.apache.cloudstack.ca.provider;

import java.math.BigInteger;
import java.security.cert.CertificateException;
import java.security.cert.CertificateExpiredException;
import java.security.cert.CertificateNotYetValidException;
import java.security.cert.X509Certificate;
import java.util.List;
import java.util.Map;

import javax.net.ssl.X509TrustManager;

import org.apache.log4j.Logger;

import com.cloud.certificate.dao.CrlDao;
import com.google.common.base.Strings;

public final class RootCACustomTrustManager implements X509TrustManager {
private static final Logger LOG = Logger.getLogger(RootCACustomTrustManager.class);

private String clientAddress = "Unknown";
private boolean authStrictness = true;
private boolean allowExpiredCertificate = true;
private CrlDao crlDao;
private X509Certificate caCertificate;
private Map<String, X509Certificate> activeCertMap;

public RootCACustomTrustManager(final String clientAddress, final boolean authStrictness, final boolean allowExpiredCertificate, final Map<String, X509Certificate> activeCertMap, final X509Certificate caCertificate, final CrlDao crlDao) {
if (!Strings.isNullOrEmpty(clientAddress)) {
this.clientAddress = clientAddress.replace("/", "").split(":")[0];
}
this.authStrictness = authStrictness;
this.allowExpiredCertificate = allowExpiredCertificate;
this.activeCertMap = activeCertMap;
this.caCertificate = caCertificate;
this.crlDao = crlDao;
}

private void printCertificateChain(final X509Certificate[] certificates, final String s) throws CertificateException {
if (certificates == null) {
return;
}
final StringBuilder builder = new StringBuilder();
builder.append("A client/agent attempting connection from address=").append(clientAddress).append(" has presented these certificate(s):");
int counter = 1;
for (final X509Certificate certificate: certificates) {
builder.append("\nCertificate [").append(counter++).append("] :");
builder.append(String.format("\n Serial: %x", certificate.getSerialNumber()));
builder.append("\n Not Before:" + certificate.getNotBefore());
builder.append("\n Not After:" + certificate.getNotAfter());
builder.append("\n Signature Algorithm:" + certificate.getSigAlgName());
builder.append("\n Version:" + certificate.getVersion());
builder.append("\n Subject DN:" + certificate.getSubjectDN());
builder.append("\n Issuer DN:" + certificate.getIssuerDN());
builder.append("\n Alternative Names:" + certificate.getSubjectAlternativeNames());
}
LOG.debug(builder.toString());
}

@Override
public void checkClientTrusted(final X509Certificate[] certificates, final String s) throws CertificateException {
if (LOG.isDebugEnabled()) {
printCertificateChain(certificates, s);
}
if (!authStrictness) {
return;
}
if (certificates == null || certificates.length < 1 || certificates[0] == null) {
throw new CertificateException("In strict auth mode, certificate(s) are expected from client:" + clientAddress);
}
final X509Certificate primaryClientCertificate = certificates[0];

// Revocation check
final BigInteger serialNumber = primaryClientCertificate.getSerialNumber();
if (serialNumber == null || crlDao.findBySerial(serialNumber) != null) {
final String errorMsg = String.format("Client is using revoked certificate of serial=%x, subject=%s from address=%s",
primaryClientCertificate.getSerialNumber(), primaryClientCertificate.getSubjectDN(), clientAddress);
LOG.error(errorMsg);
throw new CertificateException(errorMsg);
}

// Validity check
if (!allowExpiredCertificate) {
try {
primaryClientCertificate.checkValidity();
} catch (final CertificateExpiredException | CertificateNotYetValidException e) {
final String errorMsg = String.format("Client certificate has expired with serial=%x, subject=%s from address=%s",
primaryClientCertificate.getSerialNumber(), primaryClientCertificate.getSubjectDN(), clientAddress);
LOG.error(errorMsg);
throw new CertificateException(errorMsg); }
}

// Ownership check
boolean certMatchesOwnership = false;
if (primaryClientCertificate.getSubjectAlternativeNames() != null) {
for (final List<?> list : primaryClientCertificate.getSubjectAlternativeNames()) {
if (list != null && list.size() == 2 && list.get(1) instanceof String) {
final String alternativeName = (String) list.get(1);
if (clientAddress.equals(alternativeName)) {
certMatchesOwnership = true;
}
}
}
}
if (!certMatchesOwnership) {
final String errorMsg = "Certificate ownership verification failed for client: " + clientAddress;
LOG.error(errorMsg);
throw new CertificateException(errorMsg);
}
if (activeCertMap != null && !Strings.isNullOrEmpty(clientAddress)) {
activeCertMap.put(clientAddress, primaryClientCertificate);
}
if (LOG.isDebugEnabled()) {
LOG.debug("Client/agent connection from ip=" + clientAddress + " has been validated and trusted.");
}
}

@Override
public void checkServerTrusted(X509Certificate[] x509Certificates, String s) throws CertificateException {
}

@Override
public X509Certificate[] getAcceptedIssuers() {
if (!authStrictness) {
return null;
}
return new X509Certificate[]{caCertificate};
}
}
Loading

0 comments on commit f12b00e

Please sign in to comment.