Skip to content

WIP Use machine key for encryption instead of ProtectSection()#22

Open
ColsterJ wants to merge 9 commits into
AcademyPgh:masterfrom
ColsterJ:master
Open

WIP Use machine key for encryption instead of ProtectSection()#22
ColsterJ wants to merge 9 commits into
AcademyPgh:masterfrom
ColsterJ:master

Conversation

@ColsterJ
Copy link
Copy Markdown

@ColsterJ ColsterJ commented Dec 9, 2019

No description provided.

Comment thread Server/LauncherClient/Launcher.cs Outdated
if(key == "Secret")
{
//string encryptedValue = encryption.Encrypt(value);
string encryptedValue = MachineKeyEncryption.Protect(value, "Secret");
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "Secret" part here should be something more unique. Possibly something like $"Secret for computer {computerKey}"

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, I'll adjust that

Copy link
Copy Markdown
Author

@ColsterJ ColsterJ Dec 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be fixed in commit 92bb6e5


namespace LauncherServerClasses
{
public static class MachineKeyEncryption
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is added to provide a different encryption method for hiding the secret key, locally, on the client machine. The "Protect" and "UnProtect" methods are used for encrypting and decrypting the secret key using the machine key, instead of using RSA.


// We must decrypt the secret key using the machine key
//machineKeyEncryption = new Encryption("the machine key");
secretKey = MachineKeyEncryption.UnProtect(secretKey, "Secret");
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "Secret" here should match the one you changed in "SetConfigValue" - It shouldn't work if they are different.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I missed that. I edited the function call to match in a145cf0 and it seems to be functioning now.

host.StartHost();

//encryption = new Encryption("the machine key");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to leave this comment in the code?


if(key == "Secret")
{
//string encryptedValue = encryption.Encrypt(value);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you mean to leave this comment in the code?


configuration.AppSettings.Settings[key].Value = value;
configuration.AppSettings.SectionInformation.ProtectSection(null);
//configuration.AppSettings.SectionInformation.ProtectSection(null);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I promise I'm gonna stop leaving this comment after this, but...

did you mean to leave this comment in the code?

please keep thinking about it as you look at this PR in total, even though I've stopped commenting.

Copy link
Copy Markdown

@jeanlange jeanlange left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice, tight pull request - way to go. The only thing I'd look at is removing the commented out parts. You can always find them again using git history.

</PropertyGroup>
<PropertyGroup>
<SignManifests>true</SignManifests>
<SignManifests>false</SignManifests>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this change do?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants