Using SQL with encryption

This is my first project I am doing in VB.NET, and also my first real programming project. There is sensitive data, so I am utilizing Microsoft's Encryption/Decryption class (clsCrypt).

For optimization, quality and best practice standards, which code the 'best' way to retrieve encrypted data from a MS SQL Server 2008 R2 db, and decrypt it, based on what the user enters in text boxes? (First Name, Last Name)

Note: I did not accommodate the text box values into the last code snippet.

```
Public Class Form1

Dim eFirst As String
Dim eLast As String
Dim dFirst As String
Dim dLast As String

Public Sub Searchbtn_Click(sender As System.Object, e As System.EventArgs) Handles Searchbtn.Click

Me.DataGridView1.Show()
Dim SQLConnection As New SqlConnection("Data Source=SQLTEST_HR,4000\SQLEXPRESS;Integrated Security=True") 'Declare Connection String'
Dim SqlCommand As New SqlCommand 'Declare variable for SQL command'

Dim dt As New DataTable

Dim strKey As String = "Key1" 'encryption Key'
Dim clsEncrypt As clsCrypt 'Assigns a variable to clsCrypt class'
clsEncrypt = New clsCrypt(strKey) ' creates a new instance of the clsCrypt class'

eFirst = clsEncrypt.EncryptData(SearchFirsttxt.Text.Trim.ToUpper)
eLast = clsEncrypt.EncryptData(SearchLastTxt.Text.Trim.ToUpper)

SQLConnection.Open() 'Opens database Connection'
SqlCommand.Connection = SQLConnection 'Assigns connection to the command'

If SearchFirsttxt.Text = "" Then
SqlCommand.CommandText = "Select * FROM PARTICIPANT WHERE LAST_NM_TXT = '" & eLast & "';"
ElseIf SearchLastTxt.Text = "" Then
SqlCommand.CommandText = "Select * FROM PARTICIPANT WHERE FIRST_NM_TXT = '" & eFirst & "';"
ElseIf SearchFirsttxt.Text IsNot Nothing And SearchLastTxt.Text IsNot Nothing Then
SqlCommand.CommandText = "Select * FROM PARTICIPANT WHERE FIRST_NM_TXT = '" & eFirst & "' and LAST_NM_TXT = '" & eLast & "';"
Else
SqlCommand.CommandText = "Sel

Okay, best practices...

Data Access

Never do data access directly in the UI. Have a separate class file to do this. Best practice would suggest creating an interface for each 'aggregate root' (collection of classes that act as an integral whole). that you want to retrieve, then a subclass that inherits from the interface.

To be honest, and for simplicity, you can use something like the Gateway pattern. This is like a central place to do all database work. Its fine to start with, but may get cluttered depending on how complex your Data Access code will get:

Here is a typical pattern to separate the DB from the UI:

(Note - I don't work with VB.NET, so apologies for incorrect statements)

Public Interface IDbGateway 
   Function SearchForParticipant(firstName As String, surname As String) As IQueryable Of(Participant)
   Function GetParticipant(id As Int32) As Participant 
   Function AddOrUpdateParticipant(part As Participant) As Participant 
   Sub Delete(part as Participant);
End Interface

Public Class SqlDbGateway
       Implements IDbGateway

   Dim connectionString as String = null

   ' Your most commonly used way
   Public Sub New()
      Me.New "MyConnectionStringKey"

      ' In your App.config add this:
      ' 
      '   
      '      
      '   
      ' 
   End New

   Public Sub New (connectionStringKey as String)
      Me.connectionString = ConfigurationManager.ConnectionStrings.Get(connectionStringKey).ConnectionString;
   End New

   ' Implement interface here
End Class

Your UI could use a Dependency Injection framework, but you can simply go with (again, tactically)..

Public Class MyUIForm Inherits Form
   Dim dataAccess as IDbGateway = new SqlDbGateway();
End Class

Okay, step back - What have we achieved?

• You can now develop and test the data access layer in isolation.

• If your DB environment is different in production, than on your development (which is almost always the case), then you just supply a new App.config file.

• The UI only sees what the IDbGateway interface is exposing, not what back end is hitting.

• You have also (partly) removed the knowledge of the data source (XML, CSV, Oracle etc)

So next step is now removing the dependency of DataTables. You can happily use DataTables in the SqlDataAccess class, but convert the rows to Participant classes before returning them. Therefore the UI only sees concrete classes, and DataGrid's will happily binding to IEnumerable(Of T) classes.

DB Performance

Never do SELECT * FROM in a production application. Its fine for SSMS in a development environment to get a look over everything. In a production app, you should only pull out the columns you want, rather than everything. If a DBA decided to change this schema behind your application, you will start pulling in more columns, which could be dangerous, especially if there's something like 'IS_BLACK_LISTED', 'HAS_CRIMINAL_RECORD' .. etc.

Using T-SQL like you are is fine, as its parameterised. But .. (as you'll see in encryption), if you are super paranoid, you should use Stored Procedures, rather than revealing the T-SQL you are querying with.
The barrier created by the IDbGateway ensures the UI doesn't have to care where it comes from, it just wants the data back. This pattern is called 'Inversion Of Control'.

Encryption

I need to know more details about what you are doing, but lets review what is going on:

• A user types text into a UI (unencrypted)

• The UI encrypts this text (encrypted)

• Send data to the database (unencrypted transport)

• Data returns from the DB (unencrypted transport)

• User sees results from DB (unencrypted)

The UI encrypts it, but there is no other encryption going on as far as I can gather?

So simplify encryption..

-
Password fields can stop the UI revealing sensitive data.

-
Use the SecureString class to encrypt at UI level. It isn't the most friendly of classes, as you have to retrieve items character by character. But if you want secure - you've got it and its relatively easy to use, rather than digging into the Crypto32 API.

-
For database connections, you can install SSL certificates and used the "Encrypt=true". An MSDN article is here. This makes code even cleaner. Also use stored procedures to not reveal what the app is doing with the DB.

-
See (3)

-
User sees results.. 4 out of 5 isn't bad ;-)

--

Thats all I can say about it now. In summary:

• Separate data access from UI and business logic

• Only retrieve what you need, rather than everything

• Use a DTO (Data Transfer Object) to convert from the data sources (tables) to the UI layer. There are tools that can do this for you like Entity Framework and NHibernate.

• Look at all encryption options. How secure do you want this to be? To what extent should you go to (i.e. obfuscate the application code? SSL the DB connections? Encrypt the DB?)

Raise another StackOverflow question for (4) and you should get a more helpful answer.

StackExchange Code Review Q#40973, answer score: 2