encrypt access keys

Author: KesterTan

app/controllers/autograders_controller.rb

@@ -41,7 +41,14 @@ def edit
 
   action_auth_level :update, :instructor
   def update
-    if @autograder.update(autograder_params) && @assessment.update(assessment_params)
+    # Clear secrets if use_access_key is disabled
+    params_to_update = autograder_params
+    unless params_to_update[:use_access_key]
+      params_to_update[:access_key] = nil
+      params_to_update[:access_key_id] = nil
+    end
+    
+    if @autograder.update(params_to_update) && @assessment.update(assessment_params)
       flash[:success] = "Autograder saved."
       begin
         upload
@@ -115,8 +122,10 @@ def set_autograder
   end
 
   def autograder_params
-    params[:autograder].permit(:autograde_timeout, :autograde_image, :release_score, :access_key,
-                               :access_key_id, :instance_type)
+    params.require(:autograder).permit(
+      :autograde_timeout, :autograde_image, :release_score,
+      :use_access_key, :access_key, :access_key_id, :instance_type
+    )
   end
 
   def assessment_params

app/helpers/assessment_autograde_core.rb

@@ -157,26 +157,27 @@ def get_job_status(job_id)
   # Returns the Tango response
   #
   def tango_add_job(course, assessment, upload_file_list, callback_url, job_name, output_file)
-    job_properties = { "image" => @autograde_prop.autograde_image,
-                       "files" => upload_file_list.map do |f|
-                         { "localFile" => f["remoteFile"],
-                           "destFile" => Pathname.new(f["destFile"]).basename.to_s }
-                       end,
-                       "output_file" => output_file,
-                       "timeout" => @autograde_prop.autograde_timeout,
-                       "callback_url" => callback_url,
-                       "jobName" => job_name,
-                       "disable_network" => assessment.disable_network}
-    if Rails.configuration.x.ec2_ssh.present?
+    job_properties = {
+      "image" => @autograde_prop.autograde_image,
+      "files" => upload_file_list.map do |f|
+        { "localFile" => f["remoteFile"],
+          "destFile" => Pathname.new(f["destFile"]).basename.to_s }
+      end,
+      "output_file" => output_file,
+      "timeout" => @autograde_prop.autograde_timeout,
+      "callback_url" => callback_url,
+      "jobName" => job_name,
+      "disable_network" => assessment.disable_network
+    }
+    
+    if Rails.configuration.x.ec2_ssh
       job_properties["ec2Vmms"] = true
+      job_properties["instanceType"] = @autograde_prop.instance_type.presence || "t3.micro"
+      
       if @autograde_prop.use_access_key?
         job_properties["accessKey"] = @autograde_prop.access_key
         job_properties["accessKeyId"] = @autograde_prop.access_key_id
-      else
-        job_properties["accessKey"] = ""
-        job_properties["accessKeyId"] = ""
       end
-      job_properties["instanceType"] = @autograde_prop.instance_type
     end
 
     job_properties = job_properties.to_json

app/models/autograder.rb

@@ -5,14 +5,26 @@
 class Autograder < ApplicationRecord
   belongs_to :assessment
 
+  # Encryption for AWS credentials
+  has_encrypted :access_key
+  has_encrypted :access_key_id
+
   trim_field :autograde_image
 
-  # extremely short timeout values cause the backend to throw system errors
+  # Validations
   validates :autograde_timeout,
             numericality: { greater_than_or_equal_to: 10, less_than_or_equal_to: 900 }
   validates :autograde_image, :autograde_timeout, presence: true
   validates :autograde_image, length: { maximum: 64 }
 
+  with_options if: :use_access_key do
+    validates :access_key_id, presence: true, format: { 
+      with: /\A[A-Z0-9]{16,24}\z/, 
+      message: "looks invalid" 
+    }
+    validates :access_key, presence: true
+  end
+
   after_commit -> { assessment.dump_yaml }
 
   SERIALIZABLE = Set.new %w[autograde_image autograde_timeout release_score]

app/views/autograders/_ec2_settings.html.erb

@@ -6,8 +6,16 @@
 <%= f.check_box :use_access_key,
                 display_name: "Enable Access Key",
                 help_text: "(Optional) Use your own provided access key to authenticate to different EC2 instances than the default one on Tango" %>
-<%= f.text_field :access_key, display_name: "Access Key" %>
-<%= f.text_field :access_key_id, display_name: "Access Key ID" %>
+<%= f.password_field :access_key, 
+                     display_name: "Access Key", 
+                     value: nil, 
+                     autocomplete: "new-password",
+                     placeholder: "Leave blank to keep existing secret" %>
+<%= f.text_field :access_key_id, 
+                 display_name: "Access Key ID", 
+                 value: nil, 
+                 autocomplete: "off",
+                 placeholder: "Leave blank to keep existing secret" %>
 
 <%
   # Group EC2 instance options by category for better organization

config/initializers/filter_parameter_logging.rb

@@ -1,4 +1,4 @@
 # Be sure to restart your server when you modify this file.
 
 # Configure sensitive parameters which will be filtered from the log file.
-Rails.application.config.filter_parameters += [:password, :session, :warden, :secret, :salt, :cookie, :csrf, :restful_key, :lockbox_master_key, :lti_tool_private_key]
+Rails.application.config.filter_parameters += [:password, :session, :warden, :secret, :salt, :cookie, :csrf, :restful_key, :lockbox_master_key, :lti_tool_private_key, :access_key, :access_key_id]

db/migrate/20251021034813_encrypt_autograder_credentials.rb

@@ -0,0 +1,44 @@
+class EncryptAutograderCredentials < ActiveRecord::Migration[6.1]
+  def up
+    # Add encrypted columns for access keys
+    add_column :autograders, :access_key_ciphertext, :text
+    add_column :autograders, :access_key_id_ciphertext, :text
+
+    # Migrate existing plaintext data to encrypted columns
+    Autograder.reset_column_information
+    Autograder.find_each do |autograder|
+      if autograder.read_attribute(:access_key).present?
+        autograder.access_key = autograder.read_attribute(:access_key)
+      end
+      if autograder.read_attribute(:access_key_id).present?
+        autograder.access_key_id = autograder.read_attribute(:access_key_id)
+      end
+      autograder.save(validate: false) # Skip validation during migration
+    end
+
+    # Remove plaintext columns after migration
+    remove_column :autograders, :access_key
+    remove_column :autograders, :access_key_id
+  end
+
+  def down
+    # Add back plaintext columns
+    add_column :autograders, :access_key, :string, default: ""
+    add_column :autograders, :access_key_id, :string, default: ""
+
+    # Migrate encrypted data back to plaintext columns
+    Autograder.reset_column_information
+    Autograder.find_each do |autograder|
+      if autograder.access_key_ciphertext.present?
+        autograder.update_column(:access_key, autograder.access_key || "")
+      end
+      if autograder.access_key_id_ciphertext.present?
+        autograder.update_column(:access_key_id, autograder.access_key_id || "")
+      end
+    end
+
+    # Remove encrypted columns
+    remove_column :autograders, :access_key_ciphertext
+    remove_column :autograders, :access_key_id_ciphertext
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2024_12_11_042124) do
+ActiveRecord::Schema.define(version: 2025_10_21_034813) do
 
   create_table "active_storage_attachments", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t|
     t.string "name", null: false
@@ -150,9 +150,11 @@
     t.string "autograde_image"
     t.boolean "release_score"
     t.string "instance_type", default: ""
-    t.string "access_key", default: ""
-    t.string "access_key_id", default: ""
     t.boolean "use_access_key", default: false
+    t.string "ami", default: ""
+    t.string "security_group", default: ""
+    t.text "access_key_ciphertext"
+    t.text "access_key_id_ciphertext"
   end
 
   create_table "course_user_data", id: :integer, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t|