Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

Check secrets and ssl in production 2 #297

Merged
merged 4 commits into from Sep 4, 2015
Merged

Check secrets and ssl in production 2 #297

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7916090
Implement error controller for requirements and for database exceptions
jordimassaguerpla Aug 25, 2015
5f2e91e
fix spelling in error message
jordimassaguerpla Sep 4, 2015
81d6150
review errors_spec to be safe
jordimassaguerpla Sep 4, 2015
c012164
fix rubocop
jordimassaguerpla Sep 4, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Binary file added app/assets/images/layout/portus-error.png
View file
Open in desktop
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
30 changes: 15 additions & 15 deletions app/controllers/application_controller.rb
View file
Open in desktop
@@ -1,5 +1,6 @@
class ApplicationController < ActionController::Base
before_action :check_requirements
helper_method :fixes
before_action :authenticate_user!
protect_from_forgery with: :exception

Expand All @@ -16,27 +17,26 @@ def after_sign_out_path_for(_resource)
new_user_session_url
end

def fixes
secrets = Rails.application.secrets
{}.tap do |fix|
fix[:ssl] = Rails.env.production? && !request.ssl?
fix[:secret_key_base] = secrets.secret_key_base == "CHANGE_ME"
fix[:secret_machine_fqdn] = secrets.machine_fqdn.nil?
fix[:secret_encryption_private_key_path] = secrets.encryption_private_key_path.nil?
fix[:secret_portus_password] = secrets.portus_password.nil?
fix
end
end

protected

# Check whether certain requirements are met, like ssl configuration
# for production or having setup secrets.
# If they are not met, render a page with status 500
def check_requirements
fix_secrets, fix_ssl = fixes
return unless fix_secrets || fix_ssl

text = "Please review the following configurations"
text += "<ul>"
text += "<li>ssl</li>" if fix_ssl
text += "<li>secrets</li>" if fix_secrets
text += "</ul>"
render text: text, status: 500
end

def fixes
fix_secrets = true if Rails.application.secrets.secret_key_base == "CHANGE_ME"
fix_ssl = true if Rails.env.production? && !request.ssl?
[fix_secrets, fix_ssl]
return unless fixes.value?(true)
redirect_to "/errors/500"
end

def deny_access
Expand Down
10 changes: 10 additions & 0 deletions app/controllers/errors_controller.rb
View file
Open in desktop
@@ -0,0 +1,10 @@
class ErrorsController < ApplicationController
skip_before_action :check_requirements
skip_before_action :authenticate_user!

def show
Copy link
Collaborator

Choose a reason for hiding this comment

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

Leave an empty line before the method definition

@fix = fixes
@fix[:database] = env["action_dispatch.exception"].class.name.starts_with? "ActiveRecord"
render layout: false
end
end
85 changes: 85 additions & 0 deletions app/views/errors/show.html.erb
View file
Open in desktop
@@ -0,0 +1,85 @@
<html>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't you use the main layout ?

Copy link
Member Author

Choose a reason for hiding this comment

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

the main layout has calls to the database thus it will create an infinite loop if the error is the database connection.

<head>
<title>Portus error page</title>
<%= stylesheet_link_tag 'application', media: 'all' %>
<%= javascript_include_tag 'application'%>
<%= csrf_meta_tags %>
</head>
<body>
<center><%= image_tag "layout/portus-error.png" %></center>
<% if @fix.value?(true) %>
<p>
<center><h1>Ops... something went wrong...</h1></center>
<center><h1>Please review your configuration</h1></center>
</p>
<div style="margin: 100px">
<% if @fix[:ssl] %>
<h2>SSL</h2>
<p>For security reasons you need to use SSL. SSL is not configured, please review your server configuration.</p>
<% end %>
<% if @fix[:secret_key_base] %>
<h2>Fix secret key base</h2>
<p>For security reasons you need to set a <i>secret_key_base</i>. You have a default value and this is not safe. Run:</p>
<p><i> rake secret</i></p>
to generate a new secret, and then
<% if Rails.env.production? %>
set <i>PORTUS_SECRET_KEY_BASE</i> environment variable to this value.
<% else %>
set <i>secret_key_base</i> to this value in <i>config/secret.yml</i>.
<% end %>
</p>
<% end %>
<% if @fix[:secret_machine_fqdn] %>
<h2>Fix secret machine fqdn value</h2>
You need to set the machine fdqn value for Portus to work.
<% if Rails.env.production? %>
Set <i>PORTUS_MACHINE_FQDN</i> environment variable.
<% else %>
Set <i>machine_fqdn</i> in <i>config/secrets.yml</i>.
<% end %>
<% end %>
<% if @fix[:secret_encryption_private_key_path] %>
<h2>Fix secret encryption private key path value</h2>
You need to set the secret encryption private key path value for Portus to work.
<% if Rails.env.production? %>
Set <i>PORTUS_KEY_PATH</i> environment variable.
<% else %>
Set <i>encryption_private_key_path</i> in <i>config/secrets.yml</i>.
<% end %>
<% end %>
<% if @fix[:secret_portus_password] %>
<h2>Fix secret portus password</h2>
You need to set the secret encryption private key path value for Portus to work.
<% if Rails.env.production? %>
Set <i>PORTUS_PASSWORD</i> environment variable.
<% else %>
Set <i>portus_password</i> in <i>config/secrets.yml</i>.
<% end %>
<% end %>
<% if @fix[:database] %>
<h2>Database</h2>
<p>We cannot access the database.
<% if Rails.env.production? %>
Please set the following environment variables:
<ul>
<li>PORTUS_PRODUCTION_USERNAME</li>
<li>PORTUS_PRODUCTION_PASSWORD</li>
<li>PORTUS_PRODUCTION_HOST</li>
<li>PORTUS_PRODUCTION_DATABASE</li>
</ul>
<% else %>
Please review <i>config/database.yml</i> file.
<% end %>

<% end %>
</div>
<% else %>
<p>
We're sorry, but something went wrong.<br/>
If you are the application owner check the logs for more information.
</p>
<% end %>

</body>
</html>

1 change: 1 addition & 0 deletions config/application.rb
View file
Open in desktop
Expand Up @@ -24,5 +24,6 @@ class Application < Rails::Application
config.active_record.raise_in_transactional_callbacks = true
config.active_record.observers = :user_observer, :registry_observer
config.autoload_paths << Rails.root.join("lib")
config.exceptions_app = routes
end
end
1 change: 1 addition & 0 deletions config/environments/development.rb
View file
Open in desktop
Expand Up @@ -10,6 +10,7 @@
config.eager_load = false

# Show full error reports and disable caching.
# Set consider_all_requests_local to false to see the errors as in production
config.consider_all_requests_local = true
config.action_controller.perform_caching = false

Expand Down
6 changes: 5 additions & 1 deletion config/environments/production.rb
View file
Open in desktop
Expand Up @@ -80,7 +80,11 @@
# Run pending migrations
unless ENV["SKIP_MIGRATION"]
config.after_initialize do
ActiveRecord::Migrator.migrate(Rails.root.join("db/migrate"), nil)
begin
ActiveRecord::Migrator.migrate(Rails.root.join("db/migrate"), nil)
rescue
$stderr.puts "Error running migration! Please review database configuration"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use the Rails logger

Copy link
Member Author

Choose a reason for hiding this comment

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

Rails logger has not yet been initialized here and it is nil.

end
end
end

Expand Down
3 changes: 2 additions & 1 deletion config/routes.rb
View file
Open in desktop
@@ -1,5 +1,5 @@
Rails.application.routes.draw do

resources :errors, only: [:show]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of doing this, maybe do this:

resources :errors, only: [:show], constraints: { id: /\d{3}/ }

Note that now the status parameter has been renamed to id (to follow Rails conventions on the show parameter).

resources :teams, only: [:index, :show, :create]
resources :team_users, only: [:create, :destroy, :update]
resources :namespaces, only: [:create, :index, :show] do
Expand Down Expand Up @@ -41,5 +41,6 @@
put "toggle_admin", on: :member
end
end
match "(errors)/:status", to: "errors#show", constraints: { status: /\d{3}/ }, via: :all
Copy link
Collaborator

Choose a reason for hiding this comment

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

And now you can remove this match no ?

Copy link
Member Author

Choose a reason for hiding this comment

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

no i can't. This match is for routing the (errors) to the errors controller. By (errors) I mean 500, 400 errors, like when an exception is raised.

Copy link
Member

Choose a reason for hiding this comment

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

Does that include also the errors raised in other circumstances (like the 404 error trying to view a non existing team)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it does. Should we handle it differently? I don't know this case.

Copy link
Member

Choose a reason for hiding this comment

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

No, I just think it's great.


end
9 changes: 6 additions & 3 deletions spec/api/v2/token_spec.rb
View file
Open in desktop
Expand Up @@ -65,14 +65,16 @@
it "does not allow to pull a private namespace from another team" do
# It works for the regular user
get v2_token_url,
{ service: registry.hostname, account: user.username, scope: "repository:#{user.username}/busybox:push,pull" },
{ service: registry.hostname, account: user.username,
scope: "repository:#{user.username}/busybox:push,pull" },
"HTTP_AUTHORIZATION" => auth_mech.encode_credentials(user.username, password)

expect(response.status).to eq 200

# But not for another
get v2_token_url,
{ service: registry.hostname, account: another.username, scope: "repository:#{user.username}/busybox:push,pull" },
{ service: registry.hostname, account: another.username,
scope: "repository:#{user.username}/busybox:push,pull" },
"HTTP_AUTHORIZATION" => auth_mech.encode_credentials(another.username, password)

expect(response.status).to eq 401
Expand Down Expand Up @@ -162,7 +164,8 @@
allow_any_instance_of(NamespacePolicy).to receive(:pull?).and_return(true)

get v2_token_url,
{ service: registry.hostname, account: user.username, scope: "repository:#{user.username}/busybox:push,pull" },
{ service: registry.hostname, account: user.username,
scope: "repository:#{user.username}/busybox:push,pull" },
valid_auth_header

expect(response.status).to eq 200
Expand Down
69 changes: 69 additions & 0 deletions spec/controllers/errors_controller_spec.rb
View file
Open in desktop
@@ -0,0 +1,69 @@
require "rails_helper"

describe ErrorsController do
describe "GET #show" do

before :all do
secrets = Rails.application.secrets
@secret_key_base = secrets.secret_key_base
@secret_machine_fqdn = secrets.machine_fqdn
@secret_encryption_private_key_path = secrets.encryption_private_key_path
@secret_portus_password = secrets.portus_password
end
Copy link
Member

Choose a reason for hiding this comment

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

You can get rid of this block because you have the same code inside of before :each

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not the same. Note the "@". This is to get a backup of the values and restore them before each test example and after all.

Copy link
Member

Choose a reason for hiding this comment

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

I see, sorry


before :each do
secrets = Rails.application.secrets
secrets.secret_key_base = @secret_key_base
secrets.machine_fqdn = @secret_machine_fqdn
secrets.encryption_private_key_path = @secret_encryption_private_key_path
secrets.portus_password = @secret_portus_password
end

after :all do
secrets = Rails.application.secrets
secrets.secret_key_base = @secret_key_base
secrets.machine_fqdn = @secret_machine_fqdn
secrets.encryption_private_key_path = @secret_encryption_private_key_path
secrets.portus_password = @secret_portus_password
end
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need that? Is that because otherwise the other tests are screwed up?

Copy link
Member Author

Choose a reason for hiding this comment

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

exactly


it "sets @fix[:secret_key_base] as true" do
Rails.application.secrets.secret_key_base = "CHANGE_ME"
get :show, id: 1
expect(assigns(:fix)[:secret_key_base]).to be true
end

it "sets @fix[:secret_machine_fqdn] as true" do
Rails.application.secrets.machine_fqdn = nil
get :show, id: 1
expect(assigns(:fix)[:secret_machine_fqdn]).to be true
end

it "sets @fix[:secret_encryption_private_key_path] as true" do
Rails.application.secrets.encryption_private_key_path = nil
get :show, id: 1
expect(assigns(:fix)[:secret_encryption_private_key_path]).to be true
end

it "sets @fix[:secret_portus_password] as true" do
Rails.application.secrets.portus_password = nil
get :show, id: 1
expect(assigns(:fix)[:secret_portus_password]).to be true
end

end

describe "GET #show in production mode" do

after :all do
Rails.env = ActiveSupport::StringInquirer.new("test")
end

it "sets @fix[:ssl] as true" do
Rails.env = ActiveSupport::StringInquirer.new("production")
get :show, id: 1
expect(assigns(:fix)[:ssl]).to be true
end

end
end
31 changes: 31 additions & 0 deletions spec/features/errors_spec.rb
View file
Open in desktop
@@ -0,0 +1,31 @@
require "rails_helper"

feature "custom error handler page" do
describe "with custom handler setup" do

around :each do |example|
begin
Rails.application.config.consider_all_requests_local = false
Rails.application.config.action_dispatch.show_exceptions = true
Rails.env = ActiveSupport::StringInquirer.new("production")
example.run
ensure
Rails.application.config.consider_all_requests_local = true
Rails.application.config.action_dispatch.show_exceptions = false
Rails.env = ActiveSupport::StringInquirer.new("test")
end
end
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be safer to rewrite the code in that way:

around :each do |example|
  begin
    Rails.application.config.consider_all_requests_local = false
    Rails.application.config.action_dispatch.show_exceptions = true
    Rails.env = ActiveSupport::StringInquirer.new("production")
    example.run
  ensure
    Rails.application.config.consider_all_requests_local = true
    Rails.application.config.action_dispatch.show_exceptions = false
    Rails.env = ActiveSupport::StringInquirer.new("test")
  end
end


scenario "when no permissions routes to custom error page" do
visit "/teams/show?id=1234"
expect(page).to have_content("Ops... something went wrong...")
end

scenario "when no database routes to custom error page" do
ActiveRecord::Base.connection.disconnect!
visit "/"
expect(page).to have_content("Ops... something went wrong...")
end

end
end