From 3a5b83ec7a8b8116510d70a478762d4118ba8d66 Mon Sep 17 00:00:00 2001 From: Ben McCallum Date: Mon, 31 Jan 2022 11:03:38 +0100 Subject: [PATCH 1/3] Implement IDisposal for RequestValidator Relates to #466 Turns out this may just be for correctness, as these algos may not hold onto any unmanaged resources. https://github.com/dotnet/aspnetcore/issues/32871 --- src/Twilio/Security/RequestValidator.cs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/Twilio/Security/RequestValidator.cs b/src/Twilio/Security/RequestValidator.cs index aa94ada48..abe6da79a 100644 --- a/src/Twilio/Security/RequestValidator.cs +++ b/src/Twilio/Security/RequestValidator.cs @@ -9,7 +9,7 @@ namespace Twilio.Security /// /// Twilio request validator /// - public class RequestValidator + public class RequestValidator : IDisposable { private readonly HMACSHA1 _hmac; private readonly SHA256 _sha; @@ -160,5 +160,11 @@ private string PreserveCase(string url, string replacementString) var startIndex = url.IndexOf(replacementString, StringComparison.OrdinalIgnoreCase); return url.Substring(startIndex, replacementString.Length); } + + public void Dispose() + { + _hmac.Dispose() + _sha.Dispose(); + } } } From c3c01661cec332b986417e3ae08a1b6755c49d51 Mon Sep 17 00:00:00 2001 From: Ben McCallum Date: Mon, 7 Feb 2022 11:53:35 +0100 Subject: [PATCH 2/3] fix: Make RequestValidator thread safe --- src/Twilio/Security/RequestValidator.cs | 36 +++++++++++-------- .../Security/RequestValidatorTest.cs | 29 ++++++++++++++- 2 files changed, 50 insertions(+), 15 deletions(-) diff --git a/src/Twilio/Security/RequestValidator.cs b/src/Twilio/Security/RequestValidator.cs index abe6da79a..5e4188acc 100644 --- a/src/Twilio/Security/RequestValidator.cs +++ b/src/Twilio/Security/RequestValidator.cs @@ -11,8 +11,7 @@ namespace Twilio.Security /// public class RequestValidator : IDisposable { - private readonly HMACSHA1 _hmac; - private readonly SHA256 _sha; + private readonly string _secret; /// /// Create a new RequestValidator @@ -20,8 +19,7 @@ public class RequestValidator : IDisposable /// Signing secret public RequestValidator(string secret) { - _hmac = new HMACSHA1(Encoding.UTF8.GetBytes(secret)); - _sha = SHA256.Create(); + _secret = secret; } /// @@ -68,10 +66,15 @@ public bool Validate(string url, string body, string expected) return Validate(url, new Dictionary(), expected) && ValidateBody(body, bodyHash); } - public bool ValidateBody(string rawBody, string expected) + public static bool ValidateBody(string rawBody, string expected) { - var signature = _sha.ComputeHash(Encoding.UTF8.GetBytes(rawBody)); - return SecureCompare(BitConverter.ToString(signature).Replace("-","").ToLower(), expected); + // TODO: In future, use net6's one-shot methods. + // See: https://github.com/twilio/twilio-csharp/issues/466#issuecomment-1028091370 + using (var sha = SHA256.Create()) + { + var signature = sha.ComputeHash(Encoding.UTF8.GetBytes(rawBody)); + return SecureCompare(BitConverter.ToString(signature).Replace("-", "").ToLower(), expected); + } } private static IDictionary ToDictionary(NameValueCollection col) @@ -96,10 +99,15 @@ private string GetValidationSignature(string url, IDictionary pa { b.Append(key).Append(parameters[key] ?? ""); } + } + + // TODO: In future, use net6's one-shot methods. + // See: https://github.com/twilio/twilio-csharp/issues/466#issuecomment-1028091370 + using (var hmac = new HMACSHA1(Encoding.UTF8.GetBytes(_secret))) + { + var hash = hmac.ComputeHash(Encoding.UTF8.GetBytes(b.ToString())); + return Convert.ToBase64String(hash); } - - var hash = _hmac.ComputeHash(Encoding.UTF8.GetBytes(b.ToString())); - return Convert.ToBase64String(hash); } private static bool SecureCompare(string a, string b) @@ -124,18 +132,18 @@ private static bool SecureCompare(string a, string b) return mismatch == 0; } - private string RemovePort(string url) + private static string RemovePort(string url) { return SetPort(url, -1); } - private string AddPort(string url) + private static string AddPort(string url) { var uri = new UriBuilder(url); return SetPort(url, uri.Port); } - private string SetPort(string url, int port) + private static string SetPort(string url, int port) { var uri = new UriBuilder(url); uri.Host = PreserveCase(url, uri.Host); @@ -155,7 +163,7 @@ private string SetPort(string url, int port) return uri.Uri.OriginalString.Replace(uri.Scheme, scheme); } - private string PreserveCase(string url, string replacementString) + private static string PreserveCase(string url, string replacementString) { var startIndex = url.IndexOf(replacementString, StringComparison.OrdinalIgnoreCase); return url.Substring(startIndex, replacementString.Length); diff --git a/test/Twilio.Test/Security/RequestValidatorTest.cs b/test/Twilio.Test/Security/RequestValidatorTest.cs index 22dd894f3..0d3b8646d 100644 --- a/test/Twilio.Test/Security/RequestValidatorTest.cs +++ b/test/Twilio.Test/Security/RequestValidatorTest.cs @@ -1,6 +1,8 @@ using System; using System.Collections.Generic; using System.Collections.Specialized; +using System.Diagnostics; +using System.Threading; using NUnit.Framework; using Twilio.Security; @@ -121,7 +123,7 @@ public void TestValidateCollection() [Test] public void TestValidateBody() { - Assert.IsTrue(_validator.ValidateBody(Body, BodyHash), "Request body validation failed"); + Assert.IsTrue(RequestValidator.ValidateBody(Body, BodyHash), "Request body validation failed"); } [Test] @@ -183,6 +185,31 @@ public void TestValidateAddsCustomPortHttp() { const string url = "http://mycompany.com:1234/myapp.php?foo=1&bar=2"; Assert.IsTrue(_validator.Validate(url, _parameters, "Zmvh+3yNM1Phv2jhDCwEM3q5ebU="), "Request does not match provided signature"); + } + + [Test] + public void TestIsThreadSafe() + { + var validator = new RequestValidator("secret"); + var thread1 = new Thread(Validate); + var thread2 = new Thread(Validate); + + Assert.DoesNotThrow(() => + { + thread1.Start(validator); + thread2.Start(validator); + thread1.Join(); + thread2.Join(); + }); + } + + private static void Validate(object obj) + { + var sw = Stopwatch.StartNew(); + while (sw.ElapsedMilliseconds < 5000) + { + ((RequestValidator)obj).Validate("https://foo.com", "123", "foo"); + } } } } From 48e66dfc269bd8c9c68b14b2d32d77fcf8aff654 Mon Sep 17 00:00:00 2001 From: Ben McCallum Date: Mon, 7 Feb 2022 11:57:18 +0100 Subject: [PATCH 3/3] Remove IDisposable interface as not needed anymore --- src/Twilio/Security/RequestValidator.cs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/Twilio/Security/RequestValidator.cs b/src/Twilio/Security/RequestValidator.cs index 5e4188acc..bd197af6a 100644 --- a/src/Twilio/Security/RequestValidator.cs +++ b/src/Twilio/Security/RequestValidator.cs @@ -9,7 +9,7 @@ namespace Twilio.Security /// /// Twilio request validator /// - public class RequestValidator : IDisposable + public class RequestValidator { private readonly string _secret; @@ -168,11 +168,5 @@ private static string PreserveCase(string url, string replacementString) var startIndex = url.IndexOf(replacementString, StringComparison.OrdinalIgnoreCase); return url.Substring(startIndex, replacementString.Length); } - - public void Dispose() - { - _hmac.Dispose() - _sha.Dispose(); - } } }