You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2021/07/15 09:15:05 UTC
[GitHub] [cloudstack] weizhouapache commented on a change in pull request #4852: Allow host cert renewals even if client auth strictness is false
weizhouapache commented on a change in pull request #4852:
URL: https://github.com/apache/cloudstack/pull/4852#discussion_r669602111
##########
File path: plugins/ca/root-ca/src/main/java/org/apache/cloudstack/ca/provider/RootCACustomTrustManager.java
##########
@@ -79,32 +79,36 @@ public void checkClientTrusted(final X509Certificate[] certificates, final Strin
if (LOG.isDebugEnabled()) {
printCertificateChain(certificates, s);
}
- if (!authStrictness) {
- return;
- }
- if (certificates == null || certificates.length < 1 || certificates[0] == null) {
+
+ final X509Certificate primaryClientCertificate = (certificates != null && certificates.length > 0 && certificates[0] != null) ? certificates[0] : null;
+ String exceptionMsg = "";
+
+ if (authStrictness && primaryClientCertificate == null) {
throw new CertificateException("In strict auth mode, certificate(s) are expected from client:" + clientAddress);
+ } else if (primaryClientCertificate == null) {
+ LOG.info("No certificate was received from client, but continuing since strict auth mode is disabled");
+ return;
}
- 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);
+ exceptionMsg = (Strings.isNullOrEmpty(exceptionMsg)) ? errorMsg : (exceptionMsg + ". " + 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); }
+ 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);
+ if (!allowExpiredCertificate) {
Review comment:
@Slair1 what if allowExpiredCertificate is true ? log the error and silently ignore it ?
should it be appended to exceptionMsg ?
##########
File path: plugins/ca/root-ca/src/main/java/org/apache/cloudstack/ca/provider/RootCACustomTrustManager.java
##########
@@ -79,57 +79,58 @@ public void checkClientTrusted(final X509Certificate[] certificates, final Strin
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",
+ final X509Certificate primaryClientCertificate = (certificates != null && certificates.length > 0 && certificates[0] != null) ? certificates[0] : null;
+
+ if (authStrictness) {
+ if (primaryClientCertificate == null) {
+ throw new CertificateException("In strict auth mode, certificate(s) are expected from client:" + clientAddress);
+ }
+
+ // 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); }
- }
+ 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;
+ // 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 (LOG.isDebugEnabled()) {
+ LOG.debug("Client/agent connection from ip=" + clientAddress + " has been validated and trusted.");
+ }
}
- 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)) {
+ if (primaryClientCertificate != null && activeCertMap != null && !Strings.isNullOrEmpty(clientAddress)) {
Review comment:
@Slair1 according to line 86 to 90, primaryClientCertificate is not null.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org