Skip to content

Commit

Permalink
Limit certificates in rejected store to a constant value (currently 5)
Browse files Browse the repository at this point in the history
  • Loading branch information
@mrsuciu
mrsuciu committed Jul 31, 2024
1 parent 2aec785 commit c967726
Show file tree
Hide file tree
Showing 4 changed files with 209 additions and 29 deletions.
69 changes: 68 additions & 1 deletion Stack/Opc.Ua.Core/Security/Certificates/CertificateValidator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
using System.Collections.Generic;
using System.Diagnostics;
using System.Globalization;
using System.Linq;
using System.Security.Cryptography;
using System.Security.Cryptography.X509Certificates;
using System.Text;
Expand Down Expand Up @@ -691,8 +692,60 @@ private void SaveCertificates(X509Certificate2Collection certificateChain)
ICertificateStore store = m_rejectedCertificateStore.OpenStore();
try
{
var certs = store.Enumerate().GetAwaiter().GetResult();
int existingCertsCount = certs.Count;
var certsThumbprints = new HashSet<string>(certs.Cast<X509Certificate2>().Select(c => c.Thumbprint));

// Do not add existing certificates from the chain
var newChainCerts = certificateChain.Cast<X509Certificate2>().Where(c => !certsThumbprints.Contains(c.Thumbprint)).ToArray();
int newChainCertsCount = newChainCerts.Length;
// Nothing to add
if (newChainCertsCount == 0)
{
Utils.LogTrace("The entire rejected certificate chain is allready present in {0}", m_rejectedCertificateStore);
return;
}

X509Certificate2Collection certsToRemove = null;
X509Certificate2Collection certsToAdd = new X509Certificate2Collection();

// If newChainCerts is multiple of kMaxRejectedCerts + r
// all the existing certificates need to be cleared and only last kMaxRejectedCerts will be added
if (newChainCertsCount >= kMaxRejectedCerts)
{
certsToRemove = certs;

int startIdx = newChainCertsCount - kMaxRejectedCerts;
for (int i = startIdx; i < newChainCertsCount; i++)
{
certsToAdd.Add(newChainCerts[i]);
}
}
else
{
certs.AddRange(newChainCerts);

int startIdx = certs.Count - kMaxRejectedCerts;

if (startIdx > 0)
{
if (startIdx <= existingCertsCount)
{
certsToRemove = new X509Certificate2Collection();
for (int i = 0; i < startIdx; i++)
{
certsToRemove.Add(certs[i]);
}
}
}
for (int i = 0; i < newChainCertsCount; i++)
{
certsToAdd.Add(newChainCerts[i]);
}
}

bool leafCertificate = true;
foreach (var certificate in certificateChain)
foreach (var certificate in certsToAdd)
{
try
{
Expand All @@ -709,6 +762,16 @@ private void SaveCertificates(X509Certificate2Collection certificateChain)
Utils.LogCertificate(aex.Message, certificate);
}
}

if (certsToRemove != null)
{
// Remove the older identified certificates
foreach (var certificate in certsToRemove)
{
store.Delete(certificate.Thumbprint);
}
}
var newcerts = store.Enumerate().GetAwaiter().GetResult();
}
finally
{
Expand Down Expand Up @@ -1780,6 +1843,10 @@ private enum ProtectFlags
private ushort m_minimumCertificateKeySize;
private bool m_useValidatedCertificates;
#endregion

#region Constants
private const int kMaxRejectedCerts = 5;
#endregion
}

#region CertificateValidationEventArgs Class
Expand Down
10 changes: 1 addition & 9 deletions Stack/Opc.Ua.Core/Security/Certificates/DirectoryCertificateStore.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,14 +174,6 @@ public Task Add(X509Certificate2 certificate, string password = null)
{
byte[] data = null;

// check for certificate file.
Entry entry = Find(certificate.Thumbprint);

if (entry != null)
{
throw new ArgumentException("A certificate with the same thumbprint is already in the store.");
}

bool writePrivateKey = !NoPrivateKeys && certificate.HasPrivateKey;
if (writePrivateKey)
{
Expand Down Expand Up @@ -744,7 +736,7 @@ private IDictionary<string, Entry> Load(string thumbprint)
bool incompleteSearch = false;

// check for public keys.
foreach (FileInfo file in m_certificateSubdir.GetFiles("*.der"))
foreach (FileInfo file in m_certificateSubdir.GetFiles("*.der").OrderBy(f => f.CreationTime))
{
try
{
Expand Down
35 changes: 16 additions & 19 deletions Stack/Opc.Ua.Core/Security/Certificates/X509CertificateStore/X509CertificateStore.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,29 +140,26 @@ public Task Add(X509Certificate2 certificate, string password = null)
using (X509Store store = new X509Store(m_storeName, m_storeLocation))
{
store.Open(OpenFlags.ReadWrite);
if (!store.Certificates.Contains(certificate))
if (certificate.HasPrivateKey && !m_noPrivateKeys)
{
if (certificate.HasPrivateKey && !m_noPrivateKeys)
{
// X509Store needs a persisted private key
var persistedCertificate = X509Utils.CreateCopyWithPrivateKey(certificate, true);
store.Add(persistedCertificate);
}
else if (certificate.HasPrivateKey && m_noPrivateKeys)
{
// ensure no private key is added to store
using (var publicKey = new X509Certificate2(certificate.RawData))
{
store.Add(publicKey);
}
}
else
// X509Store needs a persisted private key
var persistedCertificate = X509Utils.CreateCopyWithPrivateKey(certificate, true);
store.Add(persistedCertificate);
}
else if (certificate.HasPrivateKey && m_noPrivateKeys)
{
// ensure no private key is added to store
using (var publicKey = new X509Certificate2(certificate.RawData))
{
store.Add(certificate);
store.Add(publicKey);
}

Utils.LogCertificate("Added certificate to X509Store {0}.", certificate, store.Name);
}
else
{
store.Add(certificate);
}

Utils.LogCertificate("Added certificate to X509Store {0}.", certificate, store.Name);
}

return Task.CompletedTask;
Expand Down
124 changes: 124 additions & 0 deletions Tests/Opc.Ua.Core.Tests/Security/Certificates/CertificateValidatorTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
using System.Diagnostics;
using System.IO;
using System.Linq;
using System.Runtime.ConstrainedExecution;
using System.Security.Cryptography;
using System.Security.Cryptography.X509Certificates;
using System.Text;
Expand Down Expand Up @@ -1235,6 +1236,129 @@ public void CertificateValidatorAssignableFromAppConfig()
});
}

/// <summary>
/// Verify rejected store rotates certificates at 5.
/// </summary>
[Theory]
[TestCase(3,1)]
[TestCase(3,2)]
[TestCase(3,9)]
[TestCase(3,34)]
[TestCase(5,1)]
[TestCase(5,15)]
[TestCase(5,23)]
public void VerifyRejectedRotates(int nrOfexistingCerts, int nrOfAddedCerts)
{
const int kMaxRotate = 5;
// verify cert with issuer chain
using (var validator = TemporaryCertValidator.Create(true))
{
var certValidator = validator.Update();

X509Certificate2Collection existingRejected = new X509Certificate2Collection();
X509Certificate2Collection addRejectedChain = new X509Certificate2Collection();

var app = m_goodApplicationTestSet[0];
// create self signed app certs as existing in Rejected folder
for (int i = 0; i < nrOfexistingCerts; i++)
{
var subject = app.Subject;
var appCert = CertificateFactory.CreateCertificate(
app.ApplicationUri,
app.ApplicationName,
subject,
app.DomainNames)
.CreateForRSA();
existingRejected.Add(appCert);
}

foreach (var cert in existingRejected)
{
var serviceResultException = Assert.Throws<ServiceResultException>(() => certValidator.Validate(new X509Certificate2(cert)));
Assert.AreEqual((StatusCode)StatusCodes.BadCertificateUntrusted, (StatusCode)serviceResultException.StatusCode, serviceResultException.Message);
}
Assert.AreEqual(existingRejected.Count, validator.RejectedStore.Enumerate().GetAwaiter().GetResult().Count);

// create self signed app certs as chain to add
for (int i = 0; i < nrOfAddedCerts; i++)
{
var subject = app.Subject;
var appCert = CertificateFactory.CreateCertificate(
app.ApplicationUri,
app.ApplicationName,
subject,
app.DomainNames)
.CreateForRSA();
addRejectedChain.Add(appCert);
}

foreach (var cert in addRejectedChain)
{
var serviceResultException = Assert.Throws<ServiceResultException>(() => certValidator.Validate(new X509Certificate2(cert)));
Assert.AreEqual((StatusCode)StatusCodes.BadCertificateUntrusted, (StatusCode)serviceResultException.StatusCode, serviceResultException.Message);
}

var rejectedCerts = validator.RejectedStore.Enumerate().GetAwaiter().GetResult();

int totalCerts = nrOfexistingCerts + nrOfAddedCerts;


if (totalCerts <= kMaxRotate)
{
Assert.AreEqual(totalCerts, rejectedCerts.Count);
foreach (var cer in existingRejected)
{
Assert.IsTrue(rejectedCerts.Contains(cer));
}
foreach (var cer in addRejectedChain)
{
Assert.IsTrue(rejectedCerts.Contains(cer));
}
}
else if (totalCerts > kMaxRotate)
{
Assert.AreEqual(kMaxRotate, rejectedCerts.Count);
int idx = totalCerts - kMaxRotate;

if (idx < nrOfexistingCerts)
{
// These should have rotated out
for (int i = 0; i < idx; i++)
{
Assert.IsTrue(!rejectedCerts.Contains(existingRejected[i]));
}
// These should still exist
for (int i = idx; i < nrOfexistingCerts; i++)
{
Assert.IsTrue(rejectedCerts.Contains(existingRejected[i]));
}
for (int i = 0; i < idx; i++)
{
Assert.IsTrue(rejectedCerts.Contains(addRejectedChain[i]));
}
}
else // idx bytes from newly AddedCerts
{
// All existing should have rotated out
for (int i = 0; i < nrOfexistingCerts; i++)
{
Assert.IsTrue(!rejectedCerts.Contains(existingRejected[i]));
}
// These newly added should not exist
for (int i = 0; i < nrOfAddedCerts - kMaxRotate; i++)
{
Assert.IsTrue(!rejectedCerts.Contains(addRejectedChain[i]));
}
// These newly added should exist
for (int i = idx; i < nrOfAddedCerts - idx; i++)
{
Assert.IsTrue(rejectedCerts.Contains(addRejectedChain[i]));
}
}
}
}
}

#region tests with missing revocation list when revocation is enforced
/// <summary>
/// Certificate chain with revoced certificate,
Expand Down

0 comments on commit c967726

Please sign in to comment.