-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Data obfuscation #44
Data obfuscation #44
Conversation
…o reflect change.
…ely working with unicode encoding.
…f8 as unicode wasn't needed.
…cryption keys before an api user is prepared for use.
… is no longer needed as data is obfuscated by default.
…e atomic upgrade.
Improved Data Migration See merge request !1
…ther than performing them afterwards with ALTER TABLE. Also added missing object qualifier tokens to named unique constraints.
… Salt properties from being passed over the wire.
POLYDEP-63: Obfuscate APIUsers table See merge request Cantarus-Tools/PolyDeploy!18
…ed for ease of understanding.
IP obfuscation See merge request Cantarus-Tools/PolyDeploy!20
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've taken a very general high level look and it all makes sense to me, I'll let Martin have a say based on his more detailed look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, I've requested a few minor formatting changes. But on the whole looks like a good step forward in obfuscating the API clients and IP whitelist at rest.
return bytes; | ||
} | ||
|
||
public static string SHA256HashString(string value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you document this method please, particularly the string formatting for the bytes to fixed width hexadecimal format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method documented.
EncryptionKey = GenerateKey(); | ||
|
||
// Create keys and place them in the readable fields. | ||
apiKey = GenerateKey(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using the this
keyword here would make it clear that these are private fields as opposed to local variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a suggestion:
It's a good practice to avoid the usage of this
, but then a naming convention should be used. Approximately all projects in which I was involved we used an leading _
(underscore) for instance variables. It results in a great visible difference between local and instance variables in a class without using this
.
@TableName [VARCHAR](64), | ||
@ColumnName [VARCHAR](32) | ||
AS | ||
DECLARE @ConstraintName [VARCHAR](128) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add semicolons to the end of each SQL Statement, like so:
DECLARE @ConstraintName [VARCHAR](128);
SET @ConstraintName = (...);
IF (@ConstraintName IS NOT NULL)
BEGIN
EXEC('...');
END
It is particularly unclear to read later on in this file where you're doing several ALTER TABLE
clauses within a conditional block.
I installed this on an Azure test site for #43, and wasn't able to use it because of the IP whitelist. Previously, I would use the DNN SQL console to insert the IP address into the whitelist, but now that it's hashed I can't. I ended up having to uninstall, install PolyDeploy 0.8.0, add the IP address into the database, and then upgrade to 0.9.1 (the upgrade went off without a hitch, kudos 😄). All of that to say, is it time to revisit #24 and remove IP whitelisting from the UI altogether? |
I can confirm @bdukes Concerns |
@bdukes Thanks for the kudos on the upgrade, I won't say it was a quick thing to implement haha. Hopefully I can ease your concern with the news that as part of v1.0.0 (along with this obfuscation work) I'm planning to add the ability to disable the IP whitelisting entirely and set it to be disabled by default. It will however, give you a large warning in both the management and deployment UI's reminding you that it's switched off. |
👍 perfect |
…r encryption alone.
AES-256 encryption See merge request Cantarus-Tools/PolyDeploy!21
This feature obfuscates the data stored within the
APIUsers
andIPSpecs
tables of PolyDeploy. This prevents either of these tables being easily manipulated through the database.More importantly, we no longer store any useful information about an
APIUser
in an un-hashed or unencrypted form. The user's encryption key is encrypted using their api key their api key is salted and hashed before being stored in the database. With this approach the information needed to read a user's encryption key is not stored.