Skip to content

Commit

Permalink
Support async ICertificateStore CRL interface methods (#1666)
Browse files Browse the repository at this point in the history 
- integrate 'LoadPrivateKey' in the interface to allow to implement cert stores which require a passcode to load/store certificates
Change of behavior:
- Introduce a 'NoPrivateKeys' flag to reduce the risk of leaking a private key to a unsecure store.
  - CertificateStoreIdentifier opens 'NoPrivateKey' stores
  - CertificateTrustList opens 'NoPrivateKey' stores
  - CertificateIdentifier opens store with private keys for application or GDS authority certificates
- CertificateTrustList reuses a store if it is not disposed. A user can just close the store to keep the list of certificates in memory.
- DirectoryStore `Load` does not read private keys anymore, only file names for delete. Reading all private keys caused false errors, e.g. here: #1670 

Co-authored-by: mheege-abb <marcus.heege@de.abb.com>
Co-authored-by: mheege-abb <85437567+mheege-abb@users.noreply.github.com>
  • Loading branch information
@mregen @mheege-abb
3 people committed Jan 25, 2022
1 parent 3fe9495 commit 5cbffcf
Show file tree
Hide file tree
Showing 23 changed files with 496 additions and 420 deletions.
26 changes: 13 additions & 13 deletions Libraries/Opc.Ua.Gds.Server.Common/CertificateGroup.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -371,11 +371,6 @@ public virtual async Task<X509Certificate2> LoadSigningKeyAsync(X509Certificate2
throw new ServiceResultException(StatusCodes.BadCertificateInvalid, "Cannot find issuer certificate in store.");
}

if (!certCA.HasPrivateKey)
{
throw new ServiceResultException(StatusCodes.BadCertificateInvalid, "Issuer certificate has no private key, cannot revoke certificate.");
}

CertificateIdentifier certCAIdentifier = new CertificateIdentifier(certCA) {
StorePath = storePath,
StoreType = CertificateStoreIdentifier.DetermineStoreType(storePath)
Expand All @@ -387,21 +382,26 @@ public virtual async Task<X509Certificate2> LoadSigningKeyAsync(X509Certificate2
throw new ServiceResultException(StatusCodes.BadCertificateInvalid, "Failed to load issuer private key. Is the password correct?");
}

List<X509CRL> certCACrl = store.EnumerateCRLs(certCA, false);
if (!certCAWithPrivateKey.HasPrivateKey)
{
throw new ServiceResultException(StatusCodes.BadCertificateInvalid, "Issuer certificate has no private key, cannot revoke certificate.");
}

var certCACrl = await store.EnumerateCRLs(certCA, false).ConfigureAwait(false);

var certificateCollection = new X509Certificate2Collection() { };
var certificateCollection = new X509Certificate2Collection();
if (!isCACert)
{
certificateCollection.Add(certificate);
}
updatedCRL = CertificateFactory.RevokeCertificate(certCAWithPrivateKey, certCACrl, certificateCollection);

store.AddCRL(updatedCRL);
await store.AddCRL(updatedCRL).ConfigureAwait(false);

// delete outdated CRLs from store
foreach (X509CRL caCrl in certCACrl)
{
store.DeleteCRL(caCrl);
await store.DeleteCRL(caCrl).ConfigureAwait(false);
}
store.Close();
}
Expand Down Expand Up @@ -433,18 +433,18 @@ protected async Task UpdateAuthorityCertInTrustedList()
}

// delete existing CRL in trusted list
foreach (var crl in trustedStore.EnumerateCRLs(certificate, false))
foreach (var crl in await trustedStore.EnumerateCRLs(certificate, false).ConfigureAwait(false))
{
if (crl.VerifySignature(certificate, false))
{
trustedStore.DeleteCRL(crl);
await trustedStore.DeleteCRL(crl).ConfigureAwait(false);
}
}

// copy latest CRL to trusted list
foreach (var crl in authorityStore.EnumerateCRLs(certificate, true))
foreach (var crl in await authorityStore.EnumerateCRLs(certificate, true).ConfigureAwait(false))
{
trustedStore.AddCRL(crl);
await trustedStore.AddCRL(crl).ConfigureAwait(false);
}
}
}
Expand Down
7 changes: 1 addition & 6 deletions Libraries/Opc.Ua.Security.Certificates/X509Crl/X509Crl.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -374,12 +374,7 @@ public class X509CRLCollection : List<X509CRL>
}
set
{
if (value == null)
{
throw new ArgumentNullException(nameof(value));
}

base[index] = value;
base[index] = value ?? throw new ArgumentNullException(nameof(value));
}
}

Expand Down
2 changes: 1 addition & 1 deletion Libraries/Opc.Ua.Server/Configuration/ConfigurationNodeManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,7 @@ public void HasApplicationSecureAdminAccess(ISystemContext context)
{
try
{
using (ICertificateStore appStore = CertificateStoreIdentifier.OpenStore(certificateGroup.ApplicationCertificate.StorePath))
using (ICertificateStore appStore = certificateGroup.ApplicationCertificate.OpenStore())
{
Utils.LogCertificate(Utils.TraceMasks.Security, "Delete application certificate: ", certificateGroup.ApplicationCertificate.Certificate);
appStore.Delete(certificateGroup.ApplicationCertificate.Thumbprint).Wait();
Expand Down
56 changes: 28 additions & 28 deletions Libraries/Opc.Ua.Server/Configuration/TrustList.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
using System.Collections.Generic;
using System.IO;
using System.Security.Cryptography.X509Certificates;
using System.Threading.Tasks;
using Opc.Ua.Security.Certificates;

namespace Opc.Ua.Server
Expand Down Expand Up @@ -139,7 +140,7 @@ public TrustList(Opc.Ua.TrustListState node, string trustedListPath, string issu
{
if ((masks & TrustListMasks.TrustedCertificates) != 0)
{
X509Certificate2Collection certificates = store.Enumerate().Result;
X509Certificate2Collection certificates = store.Enumerate().GetAwaiter().GetResult();
foreach (var certificate in certificates)
{
trustList.TrustedCertificates.Add(certificate.RawData);
Expand All @@ -148,7 +149,7 @@ public TrustList(Opc.Ua.TrustListState node, string trustedListPath, string issu

if ((masks & TrustListMasks.TrustedCrls) != 0)
{
foreach (var crl in store.EnumerateCRLs())
foreach (var crl in store.EnumerateCRLs().GetAwaiter().GetResult())
{
trustList.TrustedCrls.Add(crl.RawData);
}
Expand All @@ -159,7 +160,7 @@ public TrustList(Opc.Ua.TrustListState node, string trustedListPath, string issu
{
if ((masks & TrustListMasks.IssuerCertificates) != 0)
{
X509Certificate2Collection certificates = store.Enumerate().Result;
X509Certificate2Collection certificates = store.Enumerate().GetAwaiter().GetResult();
foreach (var certificate in certificates)
{
trustList.IssuerCertificates.Add(certificate.RawData);
Expand All @@ -168,7 +169,7 @@ public TrustList(Opc.Ua.TrustListState node, string trustedListPath, string issu

if ((masks & TrustListMasks.IssuerCrls) != 0)
{
foreach (var crl in store.EnumerateCRLs())
foreach (var crl in store.EnumerateCRLs().GetAwaiter().GetResult())
{
trustList.IssuerCrls.Add(crl.RawData);
}
Expand Down Expand Up @@ -319,9 +320,9 @@ public TrustList(Opc.Ua.TrustListState node, string trustedListPath, string issu
TrustListMasks masks = (TrustListMasks)trustList.SpecifiedLists;

X509Certificate2Collection issuerCertificates = null;
List<X509CRL> issuerCrls = null;
X509CRLCollection issuerCrls = null;
X509Certificate2Collection trustedCertificates = null;
List<X509CRL> trustedCrls = null;
X509CRLCollection trustedCrls = null;

// test integrity of all CRLs
if ((masks & TrustListMasks.IssuerCertificates) != 0)
Expand All @@ -334,7 +335,7 @@ public TrustList(Opc.Ua.TrustListState node, string trustedListPath, string issu
}
if ((masks & TrustListMasks.IssuerCrls) != 0)
{
issuerCrls = new List<X509CRL>();
issuerCrls = new X509CRLCollection();
foreach (var crl in trustList.IssuerCrls)
{
issuerCrls.Add(new X509CRL(crl));
Expand All @@ -350,7 +351,7 @@ public TrustList(Opc.Ua.TrustListState node, string trustedListPath, string issu
}
if ((masks & TrustListMasks.TrustedCrls) != 0)
{
trustedCrls = new List<X509CRL>();
trustedCrls = new X509CRLCollection();
foreach (var crl in trustList.TrustedCrls)
{
trustedCrls.Add(new X509CRL(crl));
Expand All @@ -362,28 +363,28 @@ public TrustList(Opc.Ua.TrustListState node, string trustedListPath, string issu
TrustListMasks updateMasks = TrustListMasks.None;
if ((masks & TrustListMasks.IssuerCertificates) != 0)
{
if (UpdateStoreCertificates(m_issuerStorePath, issuerCertificates))
if (UpdateStoreCertificates(m_issuerStorePath, issuerCertificates).GetAwaiter().GetResult())
{
updateMasks |= TrustListMasks.IssuerCertificates;
}
}
if ((masks & TrustListMasks.IssuerCrls) != 0)
{
if (UpdateStoreCrls(m_issuerStorePath, issuerCrls))
if (UpdateStoreCrls(m_issuerStorePath, issuerCrls).GetAwaiter().GetResult())
{
updateMasks |= TrustListMasks.IssuerCrls;
}
}
if ((masks & TrustListMasks.TrustedCertificates) != 0)
{
if (UpdateStoreCertificates(m_trustedStorePath, trustedCertificates))
if (UpdateStoreCertificates(m_trustedStorePath, trustedCertificates).GetAwaiter().GetResult())
{
updateMasks |= TrustListMasks.TrustedCertificates;
}
}
if ((masks & TrustListMasks.TrustedCrls) != 0)
{
if (UpdateStoreCrls(m_trustedStorePath, trustedCrls))
if (UpdateStoreCrls(m_trustedStorePath, trustedCrls).GetAwaiter().GetResult())
{
updateMasks |= TrustListMasks.TrustedCrls;
}
Expand Down Expand Up @@ -451,7 +452,7 @@ public TrustList(Opc.Ua.TrustListState node, string trustedListPath, string issu
{
if (cert != null)
{
store.Add(cert).Wait();
store.Add(cert).GetAwaiter().GetResult();
}
}

Expand Down Expand Up @@ -485,16 +486,16 @@ public TrustList(Opc.Ua.TrustListState node, string trustedListPath, string issu

using (ICertificateStore store = CertificateStoreIdentifier.OpenStore(isTrustedCertificate ? m_trustedStorePath : m_issuerStorePath))
{
var certCollection = store.FindByThumbprint(thumbprint).Result;
var certCollection = store.FindByThumbprint(thumbprint).GetAwaiter().GetResult();

if (certCollection.Count == 0)
{
return StatusCodes.BadInvalidArgument;
}

// delete all CRLs signed by cert
var crlsToDelete = new List<X509CRL>();
foreach (var crl in store.EnumerateCRLs())
var crlsToDelete = new X509CRLCollection();
foreach (var crl in store.EnumerateCRLs().GetAwaiter().GetResult())
{
foreach (var cert in certCollection)
{
Expand All @@ -507,14 +508,14 @@ public TrustList(Opc.Ua.TrustListState node, string trustedListPath, string issu
}
}

if (!store.Delete(thumbprint).Result)
if (!store.Delete(thumbprint).GetAwaiter().GetResult())
{
return StatusCodes.BadInvalidArgument;
}

foreach (var crl in crlsToDelete)
{
if (!store.DeleteCRL(crl))
if (!store.DeleteCRL(crl).GetAwaiter().GetResult())
{
// intentionally ignore errors, try best effort
Utils.LogError("RemoveCertificate: Failed to delete CRL {0}.", crl.ToString());
Expand Down Expand Up @@ -562,21 +563,21 @@ TrustListDataType trustList
return trustList;
}

private bool UpdateStoreCrls(
private async Task<bool> UpdateStoreCrls(
string storePath,
IList<X509CRL> updatedCrls)
X509CRLCollection updatedCrls)
{
bool result = true;
try
{
using (ICertificateStore store = CertificateStoreIdentifier.OpenStore(storePath))
{
var storeCrls = store.EnumerateCRLs();
var storeCrls = await store.EnumerateCRLs().ConfigureAwait(false);
foreach (var crl in storeCrls)
{
if (!updatedCrls.Contains(crl))
{
if (!store.DeleteCRL(crl))
if (!await store.DeleteCRL(crl).ConfigureAwait(false))
{
result = false;
}
Expand All @@ -588,7 +589,7 @@ TrustListDataType trustList
}
foreach (var crl in updatedCrls)
{
store.AddCRL(crl);
await store.AddCRL(crl).ConfigureAwait(false);
}
}
}
Expand All @@ -599,7 +600,7 @@ TrustListDataType trustList
return result;
}

private bool UpdateStoreCertificates(
private async Task<bool> UpdateStoreCertificates(
string storePath,
X509Certificate2Collection updatedCerts)
{
Expand All @@ -608,12 +609,12 @@ TrustListDataType trustList
{
using (ICertificateStore store = CertificateStoreIdentifier.OpenStore(storePath))
{
var storeCerts = store.Enumerate().Result;
var storeCerts = await store.Enumerate().ConfigureAwait(false);
foreach (var cert in storeCerts)
{
if (!updatedCerts.Contains(cert))
{
if (!store.Delete(cert.Thumbprint).Result)
if (!await store.Delete(cert.Thumbprint).ConfigureAwait(false))
{
result = false;
}
Expand All @@ -625,7 +626,7 @@ TrustListDataType trustList
}
foreach (var cert in updatedCerts)
{
store.Add(cert).Wait();
await store.Add(cert).ConfigureAwait(false);
}
}
}
Expand All @@ -636,7 +637,6 @@ TrustListDataType trustList
return result;
}


private void HasSecureReadAccess(ISystemContext context)
{
if (m_readAccess != null)
Expand Down
6 changes: 5 additions & 1 deletion Stack/Opc.Ua.Core/Schema/ApplicationConfiguration.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2768,7 +2768,11 @@ public CertificateTrustList()
/// <summary>
/// Sets private members to default values.
/// </summary>
private void Initialize() => m_trustedCertificates = new CertificateIdentifierCollection();
private void Initialize()
{
m_lock = new object();
m_trustedCertificates = new CertificateIdentifierCollection();
}

/// <summary>
/// Initializes the object during deserialization.
Expand Down
4 changes: 2 additions & 2 deletions Stack/Opc.Ua.Core/Security/Certificates/CertificateFactory.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ public static ICertificateBuilder CreateCertificate(string subjectName)
/// </summary>
public static X509CRL RevokeCertificate(
X509Certificate2 issuerCertificate,
List<X509CRL> issuerCrls,
X509CRLCollection issuerCrls,
X509Certificate2Collection revokedCertificates
)
{
Expand All @@ -224,7 +224,7 @@ X509Certificate2Collection revokedCertificates
/// </remarks>
public static X509CRL RevokeCertificate(
X509Certificate2 issuerCertificate,
List<X509CRL> issuerCrls,
X509CRLCollection issuerCrls,
X509Certificate2Collection revokedCertificates,
DateTime thisUpdate,
DateTime nextUpdate
Expand Down

0 comments on commit 5cbffcf

Please sign in to comment.