diff --git a/CoseHandler.Tests/CoseX509ThumbprintTests.cs b/CoseHandler.Tests/CoseX509ThumbprintTests.cs index a466e231..76d51113 100644 --- a/CoseHandler.Tests/CoseX509ThumbprintTests.cs +++ b/CoseHandler.Tests/CoseX509ThumbprintTests.cs @@ -50,4 +50,67 @@ public void ConstructThumbprintWithUnsupportedAlgo() { _ = new CoseX509Thumprint(SelfSignedCert1, HashAlgorithmName.SHA3_512); } + + /// + /// Validates that a single instance can safely call + /// from multiple threads concurrently without + /// throwing . + /// Regression test for https://github.com/microsoft/CoseSignTool/issues/191. + /// + [TestMethod] + public void ConcurrentMatchShouldNotThrow() + { + // Arrange — one shared thumbprint, many threads calling Match + CoseX509Thumprint thumbprint = new(SelfSignedCert1); + int degreeOfParallelism = Environment.ProcessorCount * 2; + int iterationsPerThread = 50; + + // Act & Assert — hammer Match() from many threads at once + Action concurrentAction = () => + { + Parallel.For(0, degreeOfParallelism * iterationsPerThread, new ParallelOptions { MaxDegreeOfParallelism = degreeOfParallelism }, i => + { + // Alternate between matching and non-matching certs + if (i % 2 == 0) + { + thumbprint.Match(SelfSignedCert1).Should().BeTrue(); + } + else + { + thumbprint.Match(SelfSignedCert2).Should().BeFalse(); + } + }); + }; + + concurrentAction.Should().NotThrow(); + } + + /// + /// Validates that concurrent construction of instances + /// with different hash algorithms is thread-safe. + /// Regression test for https://github.com/microsoft/CoseSignTool/issues/191. + /// + [TestMethod] + public void ConcurrentConstructionAndMatchShouldNotThrow() + { + // Arrange + HashAlgorithmName[] algorithms = new[] { HashAlgorithmName.SHA256, HashAlgorithmName.SHA384, HashAlgorithmName.SHA512 }; + int degreeOfParallelism = Environment.ProcessorCount * 2; + int iterationsPerThread = 30; + + // Act & Assert — create thumbprints and match from many threads + Action concurrentAction = () => + { + Parallel.For(0, degreeOfParallelism * iterationsPerThread, new ParallelOptions { MaxDegreeOfParallelism = degreeOfParallelism }, i => + { + HashAlgorithmName algo = algorithms[i % algorithms.Length]; + CoseX509Thumprint thumbprint = new(SelfSignedCert1, algo); + + thumbprint.Match(SelfSignedCert1).Should().BeTrue(); + thumbprint.Match(SelfSignedCert2).Should().BeFalse(); + }); + }; + + concurrentAction.Should().NotThrow(); + } } \ No newline at end of file diff --git a/CoseIndirectSignature.Tests/IndirectSignatureFactoryTests.cs b/CoseIndirectSignature.Tests/IndirectSignatureFactoryTests.cs index 53b4498b..5e17fce4 100644 --- a/CoseIndirectSignature.Tests/IndirectSignatureFactoryTests.cs +++ b/CoseIndirectSignature.Tests/IndirectSignatureFactoryTests.cs @@ -502,4 +502,81 @@ public async Task TestCreateIndirectSignatureAsyncWithoutPayloadLocation() indirectSignature.TryGetPayloadHashAlgorithm(out CoseHashAlgorithm? algo).Should().BeTrue(); algo!.Should().Be(CoseHashAlgorithm.SHA256); } + + /// + /// Validates that a single instance can safely produce + /// indirect signatures from multiple threads concurrently without throwing + /// + /// ("Concurrent operations from multiple threads on this type are not supported."). + /// Regression test for https://github.com/microsoft/CoseSignTool/issues/191. + /// + [Test] + public void ConcurrentHashComputationShouldNotThrow() + { + // Arrange — one shared factory, many threads + ICoseSigningKeyProvider coseSigningKeyProvider = TestUtils.SetupMockSigningKeyProvider(); + using IndirectSignatureFactory factory = new(); + int degreeOfParallelism = Environment.ProcessorCount * 2; + int iterationsPerThread = 20; + + // Act — hammer the factory from many threads at once + Action concurrentAction = () => + { + Parallel.For(0, degreeOfParallelism * iterationsPerThread, new ParallelOptions { MaxDegreeOfParallelism = degreeOfParallelism }, i => + { + byte[] payload = new byte[128]; + Random.Shared.NextBytes(payload); + + CoseSign1Message result = factory.CreateIndirectSignature( + payload, + coseSigningKeyProvider, + "application/test.concurrent"); + + // Verify each result is valid + result.IsIndirectSignature().Should().BeTrue(); + result.SignatureMatches(payload).Should().BeTrue(); + }); + }; + + // Assert — no CryptographicException from concurrent ComputeHash + concurrentAction.Should().NotThrow(); + } + + /// + /// Validates that the async indirect signature creation path is also safe under + /// concurrent usage from multiple threads. + /// Regression test for https://github.com/microsoft/CoseSignTool/issues/191. + /// + [Test] + public async Task ConcurrentAsyncHashComputationShouldNotThrow() + { + // Arrange — one shared factory, many concurrent tasks + ICoseSigningKeyProvider coseSigningKeyProvider = TestUtils.SetupMockSigningKeyProvider(); + using IndirectSignatureFactory factory = new(); + int concurrentTasks = Environment.ProcessorCount * 4; + + // Act — launch many concurrent async operations + Task[] tasks = new Task[concurrentTasks]; + for (int i = 0; i < concurrentTasks; i++) + { + tasks[i] = Task.Run(async () => + { + byte[] payload = new byte[128]; + Random.Shared.NextBytes(payload); + + CoseSign1Message result = await factory.CreateIndirectSignatureAsync( + payload, + coseSigningKeyProvider, + "application/test.concurrent.async"); + + // Verify each result is valid + result.IsIndirectSignature().Should().BeTrue(); + result.SignatureMatches(payload).Should().BeTrue(); + }); + } + + // Assert — all tasks complete without CryptographicException + Func awaitAll = async () => await Task.WhenAll(tasks); + await awaitAll.Should().NotThrowAsync(); + } } diff --git a/CoseIndirectSignature/CoseHashV.cs b/CoseIndirectSignature/CoseHashV.cs index 64912c35..30b137df 100644 --- a/CoseIndirectSignature/CoseHashV.cs +++ b/CoseIndirectSignature/CoseHashV.cs @@ -39,7 +39,7 @@ public byte[] HashValue } // sanity check the length of the hash against the specified algorithm to be sure we're not allowing a mismatch. - HashAlgorithm algo = IndirectSignatureFactory.GetHashAlgorithmFromCoseHashAlgorithm(Algorithm); + using HashAlgorithm algo = IndirectSignatureFactory.GetHashAlgorithmFromCoseHashAlgorithm(Algorithm); if (value.Length != (algo.HashSize / 8)) { throw new ArgumentOutOfRangeException(nameof(value), @$"The hash value length of {value.Length} did not match the CoseHashAlgorithm {Algorithm} required length of {algo.HashSize / 8}"); diff --git a/CoseIndirectSignature/IndirectSignatureFactory.CoseHashEnvelope.cs b/CoseIndirectSignature/IndirectSignatureFactory.CoseHashEnvelope.cs index f3428969..73f75b4a 100644 --- a/CoseIndirectSignature/IndirectSignatureFactory.CoseHashEnvelope.cs +++ b/CoseIndirectSignature/IndirectSignatureFactory.CoseHashEnvelope.cs @@ -39,9 +39,10 @@ private object CreateIndirectSignatureWithChecksInternalCoseHashEnvelopeFormat( if (!payloadHashed) { + using HashAlgorithm hasher = this.CreateHashAlgorithm(); hash = streamPayload != null - ? InternalHashAlgorithm.ComputeHash(streamPayload) - : InternalHashAlgorithm.ComputeHash(bytePayload!.Value.ToArray()); + ? hasher.ComputeHash(streamPayload) + : hasher.ComputeHash(bytePayload!.Value.ToArray()); } else { @@ -100,9 +101,10 @@ private async Task CreateIndirectSignatureWithChecksInternalCoseHashEnve // Note: HashAlgorithm.ComputeHashAsync is not available in netstandard2.0 // For better async support in the future, consider targeting net6.0+ where // ComputeHashAsync is available on specific hash algorithm implementations + using HashAlgorithm hasher = this.CreateHashAlgorithm(); hash = streamPayload != null - ? InternalHashAlgorithm.ComputeHash(streamPayload) - : InternalHashAlgorithm.ComputeHash(bytePayload!.Value.ToArray()); + ? hasher.ComputeHash(streamPayload) + : hasher.ComputeHash(bytePayload!.Value.ToArray()); } else { diff --git a/CoseIndirectSignature/IndirectSignatureFactory.Direct.cs b/CoseIndirectSignature/IndirectSignatureFactory.Direct.cs index 202756a9..35041101 100644 --- a/CoseIndirectSignature/IndirectSignatureFactory.Direct.cs +++ b/CoseIndirectSignature/IndirectSignatureFactory.Direct.cs @@ -35,9 +35,10 @@ private object CreateIndirectSignatureWithChecksInternalDirectFormat( string extendedContentType; if (!payloadHashed) { + using HashAlgorithm hasher = this.CreateHashAlgorithm(); hash = streamPayload != null - ? InternalHashAlgorithm.ComputeHash(streamPayload) - : InternalHashAlgorithm.ComputeHash(bytePayload!.Value.ToArray()); + ? hasher.ComputeHash(streamPayload) + : hasher.ComputeHash(bytePayload!.Value.ToArray()); extendedContentType = ExtendContentTypeDirect(contentType, HashAlgorithmName); } else @@ -118,9 +119,10 @@ private async Task CreateIndirectSignatureWithChecksInternalDirectFormat string extendedContentType; if (!payloadHashed) { + using HashAlgorithm hasher = this.CreateHashAlgorithm(); hash = streamPayload != null - ? InternalHashAlgorithm.ComputeHash(streamPayload) - : InternalHashAlgorithm.ComputeHash(bytePayload!.Value.ToArray()); + ? hasher.ComputeHash(streamPayload) + : hasher.ComputeHash(bytePayload!.Value.ToArray()); extendedContentType = ExtendContentTypeDirect(contentType, HashAlgorithmName); } else diff --git a/CoseIndirectSignature/IndirectSignatureFactory.cs b/CoseIndirectSignature/IndirectSignatureFactory.cs index 220a390e..974fce8d 100644 --- a/CoseIndirectSignature/IndirectSignatureFactory.cs +++ b/CoseIndirectSignature/IndirectSignatureFactory.cs @@ -36,22 +36,32 @@ public enum IndirectSignatureVersion CoseHashEnvelope } - private readonly HashAlgorithm InternalHashAlgorithm; private readonly uint HashLength; private readonly CoseHashAlgorithm InternalCoseHashAlgorithm; private readonly HashAlgorithmName InternalHashAlgorithmName; private readonly ICoseSign1MessageFactory InternalMessageFactory; /// - /// The HashAlgorithm this factory is using. + /// Creates a new instance matching the algorithm this factory is configured with. + /// Each access returns a new instance. Callers are responsible for disposing the returned instance. /// - public HashAlgorithm HashAlgorithm => InternalHashAlgorithm; + public HashAlgorithm HashAlgorithm => CreateHashAlgorithm(); /// - /// The HashAlgorightmName this factory is using. + /// The HashAlgorithmName this factory is using. /// public HashAlgorithmName HashAlgorithmName => InternalHashAlgorithmName; + /// + /// Creates a new instance that is safe for use on the calling thread. + /// + /// A new instance. Callers are responsible for disposing this instance. + private HashAlgorithm CreateHashAlgorithm() + { + return CoseSign1MessageIndirectSignatureExtensions.CreateHashAlgorithmFromName(InternalHashAlgorithmName) + ?? throw new InvalidOperationException($"Failed to create HashAlgorithm for {InternalHashAlgorithmName}"); + } + /// /// The CoseSign1 Message Factory this factory is using. /// @@ -81,21 +91,49 @@ public IndirectSignatureFactory() : this(HashAlgorithmName.SHA256) public IndirectSignatureFactory(HashAlgorithmName hashAlgorithmName, ICoseSign1MessageFactory coseSign1MessageFactory) { InternalHashAlgorithmName = hashAlgorithmName; - InternalHashAlgorithm = CoseSign1MessageIndirectSignatureExtensions.CreateHashAlgorithmFromName(hashAlgorithmName) ?? throw new ArgumentOutOfRangeException(nameof(hashAlgorithmName), $"hashAlgorithmName[{hashAlgorithmName}] could not be instantiated into a valid HashAlgorithm"); InternalMessageFactory = coseSign1MessageFactory; - HashLength = (uint)InternalHashAlgorithm.HashSize / 8; - InternalCoseHashAlgorithm = GetCoseHashAlgorithmFromHashAlgorithm(InternalHashAlgorithm); + InternalCoseHashAlgorithm = GetCoseHashAlgorithmFromName(hashAlgorithmName); + HashLength = GetHashLengthFromName(hashAlgorithmName); } - private CoseHashAlgorithm GetCoseHashAlgorithmFromHashAlgorithm(HashAlgorithm algorithm) + private static CoseHashAlgorithm GetCoseHashAlgorithmFromName(HashAlgorithmName algorithmName) { - return algorithm switch + if (algorithmName == HashAlgorithmName.SHA256) { - SHA256 => CoseHashAlgorithm.SHA256, - SHA384 => CoseHashAlgorithm.SHA384, - SHA512 => CoseHashAlgorithm.SHA512, - _ => throw new ArgumentException($@"No mapping for hash algorithm {algorithm.GetType().FullName} to any {nameof(CoseHashAlgorithm)}") - }; + return CoseHashAlgorithm.SHA256; + } + + if (algorithmName == HashAlgorithmName.SHA384) + { + return CoseHashAlgorithm.SHA384; + } + + if (algorithmName == HashAlgorithmName.SHA512) + { + return CoseHashAlgorithm.SHA512; + } + + throw new ArgumentOutOfRangeException(nameof(algorithmName), $"No mapping for hash algorithm {algorithmName} to any {nameof(CoseHashAlgorithm)}"); + } + + private static uint GetHashLengthFromName(HashAlgorithmName algorithmName) + { + if (algorithmName == HashAlgorithmName.SHA256) + { + return 32; + } + + if (algorithmName == HashAlgorithmName.SHA384) + { + return 48; + } + + if (algorithmName == HashAlgorithmName.SHA512) + { + return 64; + } + + throw new ArgumentOutOfRangeException(nameof(algorithmName), $"Unsupported hash algorithm: {algorithmName}"); } /// @@ -314,21 +352,13 @@ internal static bool HashMatches(CoseHashAlgorithm hashAlgorithm, ReadOnlyMemory { 64, HashAlgorithmName.SHA512 } }); - private bool DisposedValue; /// - /// Dispose pattern implementation + /// Dispose pattern implementation. No unmanaged resources to release; + /// HashAlgorithm instances are now created and disposed per-operation. /// /// True if called from Dispose() private void Dispose(bool disposing) { - if (!DisposedValue) - { - if (disposing) - { - HashAlgorithm.Dispose(); - } - DisposedValue = true; - } } /// diff --git a/CoseSign1.Certificates/CoseX509Thumbprint.cs b/CoseSign1.Certificates/CoseX509Thumbprint.cs index 96a3c69b..84567f70 100644 --- a/CoseSign1.Certificates/CoseX509Thumbprint.cs +++ b/CoseSign1.Certificates/CoseX509Thumbprint.cs @@ -23,11 +23,6 @@ public class CoseX509Thumprint { -44, HashAlgorithmName.SHA512 } }; - /// - /// Hash algorithm instance used to compute thumbprints - /// - private HashAlgorithm? Hasher { get; set; } - /// /// Gets the HashId used in the CBOR/COSE representation of the x5t header /// @@ -48,8 +43,9 @@ private CoseX509Thumprint() { } /// The certificate to create a thumbprint for. public CoseX509Thumprint(X509Certificate2 cert) { - BuildHasher(GetHashID(HashAlgorithmName.SHA256.Name ?? "SHA256")); - Thumbprint = Hasher?.ComputeHash(cert.RawData); + SetHashId(GetHashID(HashAlgorithmName.SHA256.Name ?? "SHA256")); + using HashAlgorithm hasher = this.CreateHashAlgorithm(); + Thumbprint = hasher.ComputeHash(cert.RawData); } /// @@ -58,9 +54,10 @@ public CoseX509Thumprint(X509Certificate2 cert) /// The certificate to create a thumbprint for. public CoseX509Thumprint(X509Certificate2 cert, HashAlgorithmName hashAlgorithm) { - BuildHasher(GetHashID(hashAlgorithm.Name + SetHashId(GetHashID(hashAlgorithm.Name ?? throw new CryptographicException(nameof(hashAlgorithm), "The supplied hash algorithm name was not recognized."))); - Thumbprint = Hasher?.ComputeHash(cert.RawData); + using HashAlgorithm hasher = this.CreateHashAlgorithm(); + Thumbprint = hasher.ComputeHash(cert.RawData); } #region Public Methods @@ -75,8 +72,8 @@ public CoseX509Thumprint(X509Certificate2 cert, HashAlgorithmName hashAlgorithm) /// public bool Match(X509Certificate2 certificate) { - return Thumbprint.ToArray().SequenceEqual(Hasher?.ComputeHash(certificate.RawData) - ?? throw new InvalidOperationException($"The current {nameof(CoseX509Thumprint)} object is not yet initialized.")); + using HashAlgorithm hasher = this.CreateHashAlgorithm(); + return Thumbprint.ToArray().SequenceEqual(hasher.ComputeHash(certificate.RawData)); } /// @@ -108,7 +105,7 @@ public static CoseX509Thumprint Deserialize(CborReader reader) } int hashId = reader.ReadInt32(); - result.BuildHasher(hashId); + result.SetHashId(hashId); if (reader.PeekState() != CborReaderState.ByteString) { @@ -149,22 +146,35 @@ private static int GetHashID(string algorithmName) return data.Key; } - // Sets HashID and returns the value for Hasher. - private void BuildHasher(int coseHashAlgorithmId) + // Sets HashID and validates the algorithm is supported. + private void SetHashId(int coseHashAlgorithmId) { - if (!HashAlgorithmToCoseValues.TryGetValue(coseHashAlgorithmId, out HashAlgorithmName algName)) + if (!HashAlgorithmToCoseValues.TryGetValue(coseHashAlgorithmId, out _)) { throw new CoseX509FormatException($"Unsupported thumbprint hash algorithm value of {coseHashAlgorithmId}"); } HashId = coseHashAlgorithmId; + } + + /// + /// Creates a new instance based on the stored . + /// Each call returns a fresh instance that is safe for use on the calling thread. + /// Callers are responsible for disposing the returned instance. + /// + private HashAlgorithm CreateHashAlgorithm() + { + if (!HashAlgorithmToCoseValues.TryGetValue(HashId, out HashAlgorithmName algName)) + { + throw new CoseX509FormatException($"Unsupported thumbprint hash algorithm value of {HashId}"); + } // HashAlgorithmName values are not constants, so we can't use an actual switch here. - Hasher = + return algName == HashAlgorithmName.SHA256 ? SHA256.Create() : algName == HashAlgorithmName.SHA384 ? SHA384.Create() : algName == HashAlgorithmName.SHA512 ? SHA512.Create() : - throw new CoseX509FormatException($"Unsupported thumbprint hash algorithm value of {coseHashAlgorithmId}"); + throw new CoseX509FormatException($"Unsupported thumbprint hash algorithm value of {HashId}"); } #endregion