From e9d41fd01b48011e01938974bcfc568d46f167a4 Mon Sep 17 00:00:00 2001 From: David Sparer Date: Thu, 6 Apr 2017 09:16:14 -0600 Subject: [PATCH] changed interface for loading credentials from a repo to get around issues with providing a key --- .../Utilities/XmlCredentialRepoBuilder.cs | 2 +- .../Config/CredentialRecordLoaderTests.cs | 12 +++--- ...edentialPasswordDecryptorDecoratorTests.cs | 4 +- ...icDeserializerKeyProviderDecoratorTests.cs | 41 ------------------- .../XmlCredentialSerializerLifeCycleTests.cs | 18 ++++---- mRemoteNGTests/mRemoteNGTests.csproj | 1 - .../App/Initialization/CredsAndConsSetup.cs | 2 +- mRemoteV1/Config/CredentialRecordLoader.cs | 8 ++-- .../CredentialRepositoryListDeserializer.cs | 4 +- ...XmlCredentialPasswordDecryptorDecorator.cs | 22 +++------- .../Config/Serializers/ISecureDeserializer.cs | 9 ++++ .../CredentialRepositoryFactory.cs | 4 +- .../Repositories/XmlCredentialRepository.cs | 6 +-- .../StaticDeserializerKeyProviderDecorator.cs | 41 ------------------- mRemoteV1/mRemoteV1.csproj | 2 +- 15 files changed, 44 insertions(+), 132 deletions(-) delete mode 100644 mRemoteNGTests/Credential/StaticDeserializerKeyProviderDecoratorTests.cs create mode 100644 mRemoteV1/Config/Serializers/ISecureDeserializer.cs delete mode 100644 mRemoteV1/Credential/StaticDeserializerKeyProviderDecorator.cs diff --git a/mRemoteNG.Specs/Utilities/XmlCredentialRepoBuilder.cs b/mRemoteNG.Specs/Utilities/XmlCredentialRepoBuilder.cs index 71847757a..4fccd05a1 100644 --- a/mRemoteNG.Specs/Utilities/XmlCredentialRepoBuilder.cs +++ b/mRemoteNG.Specs/Utilities/XmlCredentialRepoBuilder.cs @@ -34,7 +34,7 @@ namespace mRemoteNG.Specs.Utilities new StaticSerializerKeyProviderDecorator, string>(encryptor, encryptor) { Key = EncryptionKey } ), new CredentialRecordLoader( dataProvider, - new StaticDeserializerKeyProviderDecorator>(decryptor, decryptor) { Key = EncryptionKey } + decryptor ) ); } diff --git a/mRemoteNGTests/Config/CredentialRecordLoaderTests.cs b/mRemoteNGTests/Config/CredentialRecordLoaderTests.cs index 3a417716e..e78f41337 100644 --- a/mRemoteNGTests/Config/CredentialRecordLoaderTests.cs +++ b/mRemoteNGTests/Config/CredentialRecordLoaderTests.cs @@ -1,4 +1,5 @@ using System.Collections.Generic; +using System.Security; using mRemoteNG.Config; using mRemoteNG.Config.DataProviders; using mRemoteNG.Config.Serializers; @@ -12,29 +13,30 @@ namespace mRemoteNGTests.Config { private CredentialRecordLoader _credentialRecordLoader; private IDataProvider _dataProvider; - private IDeserializer> _deserializer; + private ISecureDeserializer> _deserializer; [SetUp] public void Setup() { _dataProvider = Substitute.For>(); - _deserializer = Substitute.For>>(); + _deserializer = Substitute.For>>(); _credentialRecordLoader = new CredentialRecordLoader(_dataProvider, _deserializer); } [Test] public void LoadsFromDataProvider() { - _credentialRecordLoader.Load(); + _credentialRecordLoader.Load(new SecureString()); _dataProvider.Received(1).Load(); } [Test] public void DeserializesDataFromDataProvider() { + var key = new SecureString(); _dataProvider.Load().Returns("mydata"); - _credentialRecordLoader.Load(); - _deserializer.Received(1).Deserialize("mydata"); + _credentialRecordLoader.Load(key); + _deserializer.Received(1).Deserialize("mydata", key); } } } \ No newline at end of file diff --git a/mRemoteNGTests/Config/Serializers/CredentialSerializers/XmlCredentialPasswordDecryptorDecoratorTests.cs b/mRemoteNGTests/Config/Serializers/CredentialSerializers/XmlCredentialPasswordDecryptorDecoratorTests.cs index fbc928ebb..52640dcee 100644 --- a/mRemoteNGTests/Config/Serializers/CredentialSerializers/XmlCredentialPasswordDecryptorDecoratorTests.cs +++ b/mRemoteNGTests/Config/Serializers/CredentialSerializers/XmlCredentialPasswordDecryptorDecoratorTests.cs @@ -17,14 +17,14 @@ namespace mRemoteNGTests.Config.Serializers.CredentialSerializers public void Setup() { var baseDeserializer = new XmlCredentialRecordDeserializer(); - _sut = new XmlCredentialPasswordDecryptorDecorator(baseDeserializer) {Key = _decryptionKey}; + _sut = new XmlCredentialPasswordDecryptorDecorator(baseDeserializer); } [Test] public void OutputedCredentialHasDecryptedPassword() { var xml = GenerateXml(); - var output = _sut.Deserialize(xml); + var output = _sut.Deserialize(xml, _decryptionKey); Assert.That(output.First().Password.ConvertToUnsecureString(), Is.EqualTo(_unencryptedPassword)); } diff --git a/mRemoteNGTests/Credential/StaticDeserializerKeyProviderDecoratorTests.cs b/mRemoteNGTests/Credential/StaticDeserializerKeyProviderDecoratorTests.cs deleted file mode 100644 index 987af3ee8..000000000 --- a/mRemoteNGTests/Credential/StaticDeserializerKeyProviderDecoratorTests.cs +++ /dev/null @@ -1,41 +0,0 @@ -using System.Security; -using mRemoteNG.Config.Serializers; -using mRemoteNG.Credential; -using mRemoteNG.Security; -using NSubstitute; -using NUnit.Framework; - -namespace mRemoteNGTests.Credential -{ - public class StaticDeserializerKeyProviderDecoratorTests - { - private StaticDeserializerKeyProviderDecorator _deserializerKeyProvider; - private IDeserializer _baseDeserializer; - private IHasKey _objThatNeedsKey; - private readonly SecureString _key = "someKey1".ConvertToSecureString(); - - [SetUp] - public void Setup() - { - _objThatNeedsKey = Substitute.For(); - _baseDeserializer = Substitute.For>(); - _deserializerKeyProvider = new StaticDeserializerKeyProviderDecorator(_objThatNeedsKey, _baseDeserializer) { Key = _key }; - } - - [Test] - public void CallsBaseSerializer() - { - var creds = new[] { Substitute.For() }; - _deserializerKeyProvider.Deserialize(creds); - _baseDeserializer.Received().Deserialize(creds); - } - - [Test] - public void PassesOnItsKey() - { - var creds = new[] { Substitute.For() }; - _deserializerKeyProvider.Deserialize(creds); - Assert.That(_objThatNeedsKey.Key, Is.EqualTo(_deserializerKeyProvider.Key)); - } - } -} diff --git a/mRemoteNGTests/IntegrationTests/XmlCredentialSerializerLifeCycleTests.cs b/mRemoteNGTests/IntegrationTests/XmlCredentialSerializerLifeCycleTests.cs index b7caf2c1d..eb694258d 100644 --- a/mRemoteNGTests/IntegrationTests/XmlCredentialSerializerLifeCycleTests.cs +++ b/mRemoteNGTests/IntegrationTests/XmlCredentialSerializerLifeCycleTests.cs @@ -15,21 +15,21 @@ namespace mRemoteNGTests.IntegrationTests public class XmlCredentialSerializerLifeCycleTests { private ISerializer, string> _serializer; - private IDeserializer> _deserializer; + private ISecureDeserializer> _deserializer; private readonly Guid _id = Guid.NewGuid(); private const string Title = "mycredential1"; private const string Username = "user1"; private const string Domain = "domain1"; - private readonly SecureString _password = "myPassword1!".ConvertToSecureString(); + private readonly SecureString _key = "myPassword1!".ConvertToSecureString(); [SetUp] public void Setup() { var keyProvider = Substitute.For(); - keyProvider.GetKey().Returns(_password); + keyProvider.GetKey().Returns(_key); var cryptoProvider = new CryptoProviderFactory(BlockCipherEngines.AES, BlockCipherModes.CCM).Build(); - _serializer = new XmlCredentialPasswordEncryptorDecorator(cryptoProvider, new XmlCredentialRecordSerializer()) { Key = _password }; - _deserializer = new XmlCredentialPasswordDecryptorDecorator(new XmlCredentialRecordDeserializer()) { Key = _password }; + _serializer = new XmlCredentialPasswordEncryptorDecorator(cryptoProvider, new XmlCredentialRecordSerializer()) { Key = _key }; + _deserializer = new XmlCredentialPasswordDecryptorDecorator(new XmlCredentialRecordDeserializer()); } [Test] @@ -37,7 +37,7 @@ namespace mRemoteNGTests.IntegrationTests { var credentials = new[] { new CredentialRecord(), new CredentialRecord() }; var serializedCredentials = _serializer.Serialize(credentials); - var deserializedCredentials = _deserializer.Deserialize(serializedCredentials); + var deserializedCredentials = _deserializer.Deserialize(serializedCredentials, _key); Assert.That(deserializedCredentials.Count(), Is.EqualTo(2)); } @@ -73,17 +73,17 @@ namespace mRemoteNGTests.IntegrationTests public void PasswordConsistentAfterSerialization() { var sut = SerializeThenDeserializeCredential(); - Assert.That(sut.Password.ConvertToUnsecureString(), Is.EqualTo(_password.ConvertToUnsecureString())); + Assert.That(sut.Password.ConvertToUnsecureString(), Is.EqualTo(_key.ConvertToUnsecureString())); } private ICredentialRecord SerializeThenDeserializeCredential() { var credentials = new[] { - new CredentialRecord(_id) {Title = Title, Username = Username, Domain = Domain, Password = _password} + new CredentialRecord(_id) {Title = Title, Username = Username, Domain = Domain, Password = _key} }; var serializedCredentials = _serializer.Serialize(credentials); - var deserializedCredentials = _deserializer.Deserialize(serializedCredentials); + var deserializedCredentials = _deserializer.Deserialize(serializedCredentials, _key); return deserializedCredentials.First(); } } diff --git a/mRemoteNGTests/mRemoteNGTests.csproj b/mRemoteNGTests/mRemoteNGTests.csproj index 073ef3192..713493b8e 100644 --- a/mRemoteNGTests/mRemoteNGTests.csproj +++ b/mRemoteNGTests/mRemoteNGTests.csproj @@ -137,7 +137,6 @@ - diff --git a/mRemoteV1/App/Initialization/CredsAndConsSetup.cs b/mRemoteV1/App/Initialization/CredsAndConsSetup.cs index d997a7a44..45e68e802 100644 --- a/mRemoteV1/App/Initialization/CredsAndConsSetup.cs +++ b/mRemoteV1/App/Initialization/CredsAndConsSetup.cs @@ -23,7 +23,7 @@ namespace mRemoteNG.App.Initialization private readonly string _credentialRepoListPath = Path.Combine(SettingsFileInfo.SettingsPath, "credentialRepositories.xml"); private readonly ICredentialRepositoryList _credentialRepositoryList; private readonly ISerializer, string> _credRepoSerializer; - private readonly IDeserializer> _credRepoDeserializer; + private readonly ISecureDeserializer> _credRepoDeserializer; private readonly string _credentialFilePath; public CredsAndConsSetup(ICredentialRepositoryList credentialRepositoryList, string credentialFilePath) diff --git a/mRemoteV1/Config/CredentialRecordLoader.cs b/mRemoteV1/Config/CredentialRecordLoader.cs index a96162341..8fe6d83e9 100644 --- a/mRemoteV1/Config/CredentialRecordLoader.cs +++ b/mRemoteV1/Config/CredentialRecordLoader.cs @@ -11,9 +11,9 @@ namespace mRemoteNG.Config public class CredentialRecordLoader { private readonly IDataProvider _dataProvider; - private readonly IDeserializer> _deserializer; + private readonly ISecureDeserializer> _deserializer; - public CredentialRecordLoader(IDataProvider dataProvider, IDeserializer> deserializer) + public CredentialRecordLoader(IDataProvider dataProvider, ISecureDeserializer> deserializer) { if (dataProvider == null) throw new ArgumentNullException(nameof(dataProvider)); @@ -24,10 +24,10 @@ namespace mRemoteNG.Config _deserializer = deserializer; } - public IEnumerable Load() + public IEnumerable Load(SecureString key) { var serializedCredentials = _dataProvider.Load(); - return _deserializer.Deserialize(serializedCredentials); + return _deserializer.Deserialize(serializedCredentials, key); } } } \ No newline at end of file diff --git a/mRemoteV1/Config/Serializers/CredentialProviderSerializer/CredentialRepositoryListDeserializer.cs b/mRemoteV1/Config/Serializers/CredentialProviderSerializer/CredentialRepositoryListDeserializer.cs index b7311b4ff..8d712920f 100644 --- a/mRemoteV1/Config/Serializers/CredentialProviderSerializer/CredentialRepositoryListDeserializer.cs +++ b/mRemoteV1/Config/Serializers/CredentialProviderSerializer/CredentialRepositoryListDeserializer.cs @@ -10,9 +10,9 @@ namespace mRemoteNG.Config.Serializers.CredentialProviderSerializer public class CredentialRepositoryListDeserializer { private readonly ISerializer, string> _serializer; - private readonly IDeserializer> _deserializer; + private readonly ISecureDeserializer> _deserializer; - public CredentialRepositoryListDeserializer(ISerializer, string> serializer, IDeserializer> deserializer) + public CredentialRepositoryListDeserializer(ISerializer, string> serializer, ISecureDeserializer> deserializer) { if (serializer == null) throw new ArgumentNullException(nameof(serializer)); diff --git a/mRemoteV1/Config/Serializers/CredentialSerializer/XmlCredentialPasswordDecryptorDecorator.cs b/mRemoteV1/Config/Serializers/CredentialSerializer/XmlCredentialPasswordDecryptorDecorator.cs index 24be6e4e4..99b7acffa 100644 --- a/mRemoteV1/Config/Serializers/CredentialSerializer/XmlCredentialPasswordDecryptorDecorator.cs +++ b/mRemoteV1/Config/Serializers/CredentialSerializer/XmlCredentialPasswordDecryptorDecorator.cs @@ -3,25 +3,13 @@ using System.Collections.Generic; using System.Security; using System.Xml.Linq; using mRemoteNG.Credential; -using mRemoteNG.Security; using mRemoteNG.Security.Factories; namespace mRemoteNG.Config.Serializers.CredentialSerializer { - public class XmlCredentialPasswordDecryptorDecorator : IDeserializer>, IHasKey + public class XmlCredentialPasswordDecryptorDecorator : ISecureDeserializer> { private readonly IDeserializer> _baseDeserializer; - private SecureString _decryptionKey = new SecureString(); - - public SecureString Key - { - get { return _decryptionKey; } - set - { - if (value == null) return; - _decryptionKey = value; - } - } public XmlCredentialPasswordDecryptorDecorator(IDeserializer> baseDeserializer) { @@ -31,13 +19,13 @@ namespace mRemoteNG.Config.Serializers.CredentialSerializer _baseDeserializer = baseDeserializer; } - public IEnumerable Deserialize(string xml) + public IEnumerable Deserialize(string xml, SecureString key) { - var decryptedXml = DecryptPasswords(xml); + var decryptedXml = DecryptPasswords(xml, key); return _baseDeserializer.Deserialize(decryptedXml); } - private string DecryptPasswords(string xml) + private string DecryptPasswords(string xml, SecureString key) { var xdoc = XDocument.Parse(xml); var cryptoProvider = new CryptoProviderFactoryFromXml(xdoc.Root).Build(); @@ -45,7 +33,7 @@ namespace mRemoteNG.Config.Serializers.CredentialSerializer { var passwordAttribute = credentialElement.Attribute("Password"); if (passwordAttribute == null) continue; - var decryptedPassword = cryptoProvider.Decrypt(passwordAttribute.Value, _decryptionKey); + var decryptedPassword = cryptoProvider.Decrypt(passwordAttribute.Value, key); passwordAttribute.SetValue(decryptedPassword); } return xdoc.ToString(); diff --git a/mRemoteV1/Config/Serializers/ISecureDeserializer.cs b/mRemoteV1/Config/Serializers/ISecureDeserializer.cs new file mode 100644 index 000000000..5a812f0c9 --- /dev/null +++ b/mRemoteV1/Config/Serializers/ISecureDeserializer.cs @@ -0,0 +1,9 @@ +using System.Security; + +namespace mRemoteNG.Config.Serializers +{ + public interface ISecureDeserializer + { + TOut Deserialize(TIn serializedData, SecureString key); + } +} \ No newline at end of file diff --git a/mRemoteV1/Credential/Repositories/CredentialRepositoryFactory.cs b/mRemoteV1/Credential/Repositories/CredentialRepositoryFactory.cs index fde7069d8..cad6ba783 100644 --- a/mRemoteV1/Credential/Repositories/CredentialRepositoryFactory.cs +++ b/mRemoteV1/Credential/Repositories/CredentialRepositoryFactory.cs @@ -10,9 +10,9 @@ namespace mRemoteNG.Credential.Repositories public class CredentialRepositoryFactory { private readonly ISerializer, string> _serializer; - private readonly IDeserializer> _deserializer; + private readonly ISecureDeserializer> _deserializer; - public CredentialRepositoryFactory(ISerializer, string> serializer, IDeserializer> deserializer) + public CredentialRepositoryFactory(ISerializer, string> serializer, ISecureDeserializer> deserializer) { if (serializer == null) throw new ArgumentNullException(nameof(serializer)); diff --git a/mRemoteV1/Credential/Repositories/XmlCredentialRepository.cs b/mRemoteV1/Credential/Repositories/XmlCredentialRepository.cs index 5acc4eea2..080d9987e 100644 --- a/mRemoteV1/Credential/Repositories/XmlCredentialRepository.cs +++ b/mRemoteV1/Credential/Repositories/XmlCredentialRepository.cs @@ -3,11 +3,7 @@ using System.Collections.Generic; using System.ComponentModel; using System.Linq; using mRemoteNG.Config; -using mRemoteNG.Config.DataProviders; -using mRemoteNG.Config.Serializers.CredentialSerializer; -using mRemoteNG.Security; using mRemoteNG.Tools.CustomCollections; -using mRemoteNG.UI.Forms; namespace mRemoteNG.Credential.Repositories { @@ -39,7 +35,7 @@ namespace mRemoteNG.Credential.Repositories public void LoadCredentials() { - var credentials = _credentialRecordLoader.Load(); + var credentials = _credentialRecordLoader.Load(Config.Key); foreach (var newCredential in credentials) { if (ThisIsADuplicateCredentialRecord(newCredential)) continue; diff --git a/mRemoteV1/Credential/StaticDeserializerKeyProviderDecorator.cs b/mRemoteV1/Credential/StaticDeserializerKeyProviderDecorator.cs deleted file mode 100644 index 643400e17..000000000 --- a/mRemoteV1/Credential/StaticDeserializerKeyProviderDecorator.cs +++ /dev/null @@ -1,41 +0,0 @@ -using System; -using System.Security; -using mRemoteNG.Config.Serializers; -using mRemoteNG.Security; - -namespace mRemoteNG.Credential -{ - public class StaticDeserializerKeyProviderDecorator : IDeserializer, IHasKey - { - private readonly IDeserializer _baseDeserializer; - private readonly IHasKey _objThatNeedsKey; - private SecureString _key = new SecureString(); - - public SecureString Key - { - get { return _key; } - set - { - if (value == null) return; - _key = value; - } - } - - public StaticDeserializerKeyProviderDecorator(IHasKey objThatNeedsKey, IDeserializer baseDeserializer) - { - if (objThatNeedsKey == null) - throw new ArgumentNullException(nameof(objThatNeedsKey)); - if (baseDeserializer == null) - throw new ArgumentNullException(nameof(baseDeserializer)); - - _objThatNeedsKey = objThatNeedsKey; - _baseDeserializer = baseDeserializer; - } - - public TOut Deserialize(TIn serializedData) - { - _objThatNeedsKey.Key = Key; - return _baseDeserializer.Deserialize(serializedData); - } - } -} diff --git a/mRemoteV1/mRemoteV1.csproj b/mRemoteV1/mRemoteV1.csproj index 535c9101f..43d5cb074 100644 --- a/mRemoteV1/mRemoteV1.csproj +++ b/mRemoteV1/mRemoteV1.csproj @@ -145,6 +145,7 @@ + @@ -215,7 +216,6 @@ -