From 4418a5dcf9c6d932fb63196651af77acec95b0e1 Mon Sep 17 00:00:00 2001 From: Geoff Wilson Date: Wed, 4 Sep 2024 15:54:01 -0400 Subject: [PATCH 01/10] Add domains table and adjust code to lookup --- app/controllers/application_controller.rb | 6 +++-- app/controllers/certificates_controller.rb | 5 ++-- app/interactors/authenticate_request.rb | 7 ++++++ app/interactors/issue_cert.rb | 2 +- app/lib/bad_request_error.rb | 3 +++ app/lib/services/domain_ownership_service.rb | 9 ++++---- app/models/domain.rb | 8 +++++++ app/models/domain_info.rb | 8 ------- db/migrate/20240904175652_create_domains.rb | 12 ++++++++++ db/schema.rb | 23 +++++++++++++++++++ .../services/domain_ownership_service_test.rb | 2 +- 11 files changed, 66 insertions(+), 19 deletions(-) create mode 100644 app/interactors/authenticate_request.rb create mode 100644 app/lib/bad_request_error.rb create mode 100644 app/models/domain.rb delete mode 100644 app/models/domain_info.rb create mode 100644 db/migrate/20240904175652_create_domains.rb create mode 100644 db/schema.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 9c01126..b46a4fa 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -2,6 +2,7 @@ class ApplicationController < ActionController::API before_action :set_default_format rescue_from StandardError, with: :handle_standard_error rescue_from AuthError, with: :handle_auth_error + rescue_from BadRequestError, with: :handle_bad_request_error rescue_from ActionController::ParameterMissing, with: :handle_bad_request attr_reader :identity # decoded and verified JWT @@ -29,7 +30,8 @@ def handle_auth_error(exception) render json: { error: "Unauthorized" }, status: :unauthorized end - def handle_bad_request(exception) - render json: { error: exception }, status: :bad_request + def handle_bad_request_error(exception) + render json: { error: exception.message }, status: :bad_request end + end diff --git a/app/controllers/certificates_controller.rb b/app/controllers/certificates_controller.rb index 3b3b3f6..5355f80 100644 --- a/app/controllers/certificates_controller.rb +++ b/app/controllers/certificates_controller.rb @@ -4,10 +4,9 @@ class CertificatesController < ApplicationController def create req = CertIssueRequest.new(params_permitted) if !req.valid? - render json: { error: req.errors }, status: :bad_request - return + raise BadRequestError.new req.errors.full_messages end - result = IssueCert.call(request: req) + result = IssueCert.call(request: req, identity: @identity) if result.failure? raise StandardError.new result.message end diff --git a/app/interactors/authenticate_request.rb b/app/interactors/authenticate_request.rb new file mode 100644 index 0000000..abfefce --- /dev/null +++ b/app/interactors/authenticate_request.rb @@ -0,0 +1,7 @@ +class AuthenticateRequest + include Interactor + + def call + Services::DomainOwnershipService.new.authorize!(context.identity, context.request) + end +end diff --git a/app/interactors/issue_cert.rb b/app/interactors/issue_cert.rb index a9ae6d4..9606182 100644 --- a/app/interactors/issue_cert.rb +++ b/app/interactors/issue_cert.rb @@ -1,5 +1,5 @@ class IssueCert include Interactor::Organizer - organize CheckPolicy, ObtainCert, Log + organize AuthenticateRequest, ObtainCert, Log end diff --git a/app/lib/bad_request_error.rb b/app/lib/bad_request_error.rb new file mode 100644 index 0000000..4f34565 --- /dev/null +++ b/app/lib/bad_request_error.rb @@ -0,0 +1,3 @@ +# Error representing a bad request +class BadRequestError < StandardError +end diff --git a/app/lib/services/domain_ownership_service.rb b/app/lib/services/domain_ownership_service.rb index 69ee9c6..70e594e 100644 --- a/app/lib/services/domain_ownership_service.rb +++ b/app/lib/services/domain_ownership_service.rb @@ -3,9 +3,10 @@ class DomainOwnershipService def authorize!(identity, cert_req) cert_req.fqdns.each do |fqdn| domain = get_domain_name(fqdn) - raise AuthError unless domain.owner == identity.subject || - (domain.group_delegation && - (domain.groups & identity.groups).any?) + raise AuthError unless domain.present? && + (domain.owner == identity.subject || + (domain.group_delegation && + (domain.groups & identity.groups).any?)) end nil end @@ -13,7 +14,7 @@ def authorize!(identity, cert_req) private def get_domain_name(fqdn) - # TODO implement + Domain.where(fqdn: fqdn).first end end end diff --git a/app/models/domain.rb b/app/models/domain.rb new file mode 100644 index 0000000..ad12d13 --- /dev/null +++ b/app/models/domain.rb @@ -0,0 +1,8 @@ +class Domain < ApplicationRecord + serialize :groups, coder: JSON, type: Array + before_save :clean_groups + + def clean_groups + this.groups = groups.sort.uniq + end +end diff --git a/app/models/domain_info.rb b/app/models/domain_info.rb deleted file mode 100644 index ce724d5..0000000 --- a/app/models/domain_info.rb +++ /dev/null @@ -1,8 +0,0 @@ -class DomainInfo - include ActiveModel::Model - include ActiveModel::Attributes - - attribute :owner, :string - attribute :groups, array: :string, default: [] - attribute :group_delegation, :boolean, default: false -end diff --git a/db/migrate/20240904175652_create_domains.rb b/db/migrate/20240904175652_create_domains.rb new file mode 100644 index 0000000..1225da9 --- /dev/null +++ b/db/migrate/20240904175652_create_domains.rb @@ -0,0 +1,12 @@ +class CreateDomains < ActiveRecord::Migration[7.2] + def change + create_table :domains do |t| + t.string :fqdn, null: false + t.string :owner, null: false + t.text :groups + t.boolean :group_delegation, default: false + t.timestamps + t.index :fqdn, unique: true + end + end +end diff --git a/db/schema.rb b/db/schema.rb new file mode 100644 index 0000000..4e5f14d --- /dev/null +++ b/db/schema.rb @@ -0,0 +1,23 @@ +# This file is auto-generated from the current state of the database. Instead +# of editing this file, please use the migrations feature of Active Record to +# incrementally modify your database, and then regenerate this schema definition. +# +# This file is the source Rails uses to define your schema when running `bin/rails +# db:schema:load`. When creating a new database, `bin/rails db:schema:load` tends to +# be faster and is potentially less error prone than running all of your +# migrations from scratch. Old migrations may fail to apply correctly if those +# migrations use external dependencies or application code. +# +# It's strongly recommended that you check this file into your version control system. + +ActiveRecord::Schema[7.2].define(version: 2024_09_04_175652) do + create_table "domains", force: :cascade do |t| + t.string "fqdn", null: false + t.string "owner", null: false + t.text "groups" + t.boolean "group_delegation", default: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["fqdn"], name: "index_domains_on_fqdn", unique: true + end +end diff --git a/test/lib/services/domain_ownership_service_test.rb b/test/lib/services/domain_ownership_service_test.rb index 3abe508..e35ab43 100644 --- a/test/lib/services/domain_ownership_service_test.rb +++ b/test/lib/services/domain_ownership_service_test.rb @@ -3,7 +3,7 @@ class DomainOwnershipServiceTest < ActiveSupport::TestCase def setup @identity = Identity.new(subject: "test@example.com", groups: [ "admin_group" ]) - @domain = DomainInfo.new(owner: "test@example.com", group_delegation: false, groups: [ "admin_group" ]) + @domain = Domain.new(owner: "test@example.com", group_delegation: false, groups: [ "admin_group" ]) end test "#authorize! with matching owner" do From 75b064ae13f2fe47d33073c851b8a4737b629299 Mon Sep 17 00:00:00 2001 From: Geoff Wilson Date: Wed, 4 Sep 2024 18:48:49 -0400 Subject: [PATCH 02/10] Add fixtures and test for Domain name ownership matches --- app/models/domain.rb | 2 +- test/fixtures/domains.yml | 18 +++++++++++++ .../certificates_controller_test.rb | 25 ++++++++++++++++++- 3 files changed, 43 insertions(+), 2 deletions(-) create mode 100644 test/fixtures/domains.yml diff --git a/app/models/domain.rb b/app/models/domain.rb index ad12d13..fd6a0b9 100644 --- a/app/models/domain.rb +++ b/app/models/domain.rb @@ -1,5 +1,5 @@ class Domain < ApplicationRecord - serialize :groups, coder: JSON, type: Array + serialize :groups, coder: YAML, type: Array before_save :clean_groups def clean_groups diff --git a/test/fixtures/domains.yml b/test/fixtures/domains.yml new file mode 100644 index 0000000..a0ecfad --- /dev/null +++ b/test/fixtures/domains.yml @@ -0,0 +1,18 @@ +owner_match: + fqdn: example.com + owner: john.doe@example.com + group_delegation: false + +group_match: + fqdn: example2.com + owner: some.other@example2.com + group_delegation: true + groups: + - "group1" + +no_match: + fqdn: example3.com + owner: some.other@example2.com + group_delegation: true + groups: + - "group3" diff --git a/test/integration/certificates_controller_test.rb b/test/integration/certificates_controller_test.rb index f952fa4..d2f887c 100644 --- a/test/integration/certificates_controller_test.rb +++ b/test/integration/certificates_controller_test.rb @@ -12,7 +12,7 @@ class CertificatesControllerTest < ActionDispatch::IntegrationTest assert_response :unauthorized end - test "#create authorized" do + test "#create authorized as owner" do jwt = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJqb2huLmRvZUBleGFtcGxlLmNvbSIsIm5hbWUiOiJKb2huIERvZSIsImlhdCI6MTUxNjIzOTAyMiwiZ3JvdXBzIjpbImdyb3VwMSIsImdyb3VwMiJdLCJhdWQiOiJhc3RyYWwifQ.tfRLXmE_eq-piP88_clwPWrYfMAQbCJAeZQI6OFxZSI" post certificates_path, headers: { "Authorization" => "Bearer #{jwt}" }, params: { cert_issue_request: { common_name: "example.com" } } @@ -27,4 +27,27 @@ class CertificatesControllerTest < ActionDispatch::IntegrationTest assert_includes response.parsed_body.keys, key end end + + test "#create authorized by group" do + jwt = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJqb2huLmRvZUBleGFtcGxlLmNvbSIsIm5hbWUiOiJKb2huIERvZSIsImlhdCI6MTUxNjIzOTAyMiwiZ3JvdXBzIjpbImdyb3VwMSIsImdyb3VwMiJdLCJhdWQiOiJhc3RyYWwifQ.tfRLXmE_eq-piP88_clwPWrYfMAQbCJAeZQI6OFxZSI" + post certificates_path, headers: { "Authorization" => "Bearer #{jwt}" }, + params: { cert_issue_request: { common_name: "example2.com" } } + assert_response :success + %w[ ca_chain + certificate + expiration + issuing_ca + private_key + private_key_type + serial_number ].each do |key| + assert_includes response.parsed_body.keys, key + end + end + + test "#create not authorized by group" do + jwt = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJqb2huLmRvZUBleGFtcGxlLmNvbSIsIm5hbWUiOiJKb2huIERvZSIsImlhdCI6MTUxNjIzOTAyMiwiZ3JvdXBzIjpbImdyb3VwMSIsImdyb3VwMiJdLCJhdWQiOiJhc3RyYWwifQ.tfRLXmE_eq-piP88_clwPWrYfMAQbCJAeZQI6OFxZSI" + post certificates_path, headers: { "Authorization" => "Bearer #{jwt}" }, + params: { cert_issue_request: { common_name: "example3.com" } } + assert_response :unauthorized + end end From a86e926a57cc5bc25c642bc36421bce04084c838 Mon Sep 17 00:00:00 2001 From: Geoff Wilson Date: Wed, 4 Sep 2024 18:59:35 -0400 Subject: [PATCH 03/10] seed db so that Curl example works --- .devcontainer/devcontainer.json | 2 +- app/models/domain.rb | 2 +- db/seeds.rb | 5 +++++ 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json index 9b99382..18300bb 100644 --- a/.devcontainer/devcontainer.json +++ b/.devcontainer/devcontainer.json @@ -14,7 +14,7 @@ "forwardPorts": [3000, 5432, 8200], // Use 'postCreateCommand' to run commands after the container is created. - "postCreateCommand": "bundle install && rake vault:setup", + "postCreateCommand": "bundle install && rake db:setup && rake vault:setup", // Configure tool-specific properties. // "customizations": {}, diff --git a/app/models/domain.rb b/app/models/domain.rb index fd6a0b9..078f297 100644 --- a/app/models/domain.rb +++ b/app/models/domain.rb @@ -3,6 +3,6 @@ class Domain < ApplicationRecord before_save :clean_groups def clean_groups - this.groups = groups.sort.uniq + self.groups = groups.sort.uniq end end diff --git a/db/seeds.rb b/db/seeds.rb index 4fbd6ed..ea8f5ce 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -7,3 +7,8 @@ # ["Action", "Comedy", "Drama", "Horror"].each do |genre_name| # MovieGenre.find_or_create_by!(name: genre_name) # end + +# this is seed is for development only +if Rails.env.development? + Domain.first_or_create!(fqdn: "example.com", owner: "john.doe@example.com") +end From 9d6c43d2b58d918d9c346983460b33b77fdd66e8 Mon Sep 17 00:00:00 2001 From: Geoff Wilson Date: Thu, 5 Sep 2024 11:44:57 -0400 Subject: [PATCH 04/10] Fix comment and lint --- app/controllers/application_controller.rb | 1 - db/seeds.rb | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index b46a4fa..02bc7ff 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -33,5 +33,4 @@ def handle_auth_error(exception) def handle_bad_request_error(exception) render json: { error: exception.message }, status: :bad_request end - end diff --git a/db/seeds.rb b/db/seeds.rb index ea8f5ce..11fded0 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -8,7 +8,7 @@ # MovieGenre.find_or_create_by!(name: genre_name) # end -# this is seed is for development only +# this seed is for development only if Rails.env.development? Domain.first_or_create!(fqdn: "example.com", owner: "john.doe@example.com") end From ef1c8bd8b64f18ba70cfba957a914abd70614fea Mon Sep 17 00:00:00 2001 From: Geoff Wilson Date: Thu, 5 Sep 2024 12:40:05 -0400 Subject: [PATCH 05/10] Add validations and unit test to Domain model --- app/models/domain.rb | 2 ++ test/models/domain_test.rb | 40 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+) create mode 100644 test/models/domain_test.rb diff --git a/app/models/domain.rb b/app/models/domain.rb index 078f297..b0bd65c 100644 --- a/app/models/domain.rb +++ b/app/models/domain.rb @@ -2,6 +2,8 @@ class Domain < ApplicationRecord serialize :groups, coder: YAML, type: Array before_save :clean_groups + validates :fqdn, :owner, presence: true + def clean_groups self.groups = groups.sort.uniq end diff --git a/test/models/domain_test.rb b/test/models/domain_test.rb new file mode 100644 index 0000000..0a9192f --- /dev/null +++ b/test/models/domain_test.rb @@ -0,0 +1,40 @@ +# test/models/cert_issue_request_test.rb +require "test_helper" + +class DomainTest < ActiveSupport::TestCase + def setup + @attributes = { + fqdn: "example4.com", + owner: "john.doe@example.com", + } + @domain = Domain.new(@attributes) + end + + test "#new should set attributes from attributes argument" do + @attributes.each do |key, value| + assert_equal value, @domain.send(key), "Attribute #{key} was not set correctly" + end + end + + test "#valid? should be valid with valid attributes" do + assert @domain.valid? + end + + test "#valid? should require an fqdn" do + @domain.fqdn = nil + assert_not @domain.valid? + assert_includes @domain.errors[:fqdn], "can't be blank" + end + + test "#valid? should require an owner" do + @domain.owner = nil + assert_not @domain.valid? + assert_includes @domain.errors[:owner], "can't be blank" + end + + test "before_save should sort and dedupe groups" do + @domain.groups = [ "two", "two", "one" ] + @domain.save + assert_equal [ "one", "two" ], @domain.groups + end +end From e81fc6686c113b1c44f5ee70c1d11a8b6c8f7481 Mon Sep 17 00:00:00 2001 From: Geoff Wilson Date: Thu, 5 Sep 2024 12:48:38 -0400 Subject: [PATCH 06/10] fix lint --- test/models/domain_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/models/domain_test.rb b/test/models/domain_test.rb index 0a9192f..059b142 100644 --- a/test/models/domain_test.rb +++ b/test/models/domain_test.rb @@ -5,7 +5,7 @@ class DomainTest < ActiveSupport::TestCase def setup @attributes = { fqdn: "example4.com", - owner: "john.doe@example.com", + owner: "john.doe@example.com" } @domain = Domain.new(@attributes) end From d8deae17a0c72e82e0440ffb7d01581b150a2cff Mon Sep 17 00:00:00 2001 From: Geoff Wilson Date: Thu, 5 Sep 2024 15:23:35 -0400 Subject: [PATCH 07/10] rename AuthenticateRequest -> AuthorizeRequest --- .../{authenticate_request.rb => authorize_request.rb} | 2 +- app/interactors/issue_cert.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename app/interactors/{authenticate_request.rb => authorize_request.rb} (83%) diff --git a/app/interactors/authenticate_request.rb b/app/interactors/authorize_request.rb similarity index 83% rename from app/interactors/authenticate_request.rb rename to app/interactors/authorize_request.rb index abfefce..8f033c5 100644 --- a/app/interactors/authenticate_request.rb +++ b/app/interactors/authorize_request.rb @@ -1,4 +1,4 @@ -class AuthenticateRequest +class AuthorizeRequest include Interactor def call diff --git a/app/interactors/issue_cert.rb b/app/interactors/issue_cert.rb index 9606182..9a80f80 100644 --- a/app/interactors/issue_cert.rb +++ b/app/interactors/issue_cert.rb @@ -1,5 +1,5 @@ class IssueCert include Interactor::Organizer - organize AuthenticateRequest, ObtainCert, Log + organize AuthorizeRequest, ObtainCert, Log end From 5d414deb1e25162d3f5ac27e333421a2d65f7695 Mon Sep 17 00:00:00 2001 From: Geoff Wilson Date: Thu, 5 Sep 2024 15:23:57 -0400 Subject: [PATCH 08/10] add ports to docker-compose --- .devcontainer/docker-compose.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.devcontainer/docker-compose.yml b/.devcontainer/docker-compose.yml index 50cd8ff..56426d5 100644 --- a/.devcontainer/docker-compose.yml +++ b/.devcontainer/docker-compose.yml @@ -5,18 +5,16 @@ services: build: context: .. dockerfile: .devcontainer/Dockerfile - volumes: - ../..:/workspaces:cached - + ports: + - 3000:3000 # Overrides default command so things don't shut down after the process ends. command: sleep infinity - # Runs app on the same network as the database container, allows "forwardPorts" in devcontainer.json function. networks: astral: ipv4_address: "10.1.10.200" - environment: VAULT_ADDR: http://10.1.10.100:8200 VAULT_TOKEN: root_token @@ -25,6 +23,8 @@ services: vault: image: hashicorp/vault:latest restart: unless-stopped + ports: + - 8200:8200 environment: VAULT_DEV_ROOT_TOKEN_ID: root_token VAULT_DEV_LISTEN_ADDRESS: 0.0.0.0:8200 From 49c2eda6106142c46ec68c444ee4414498de0243 Mon Sep 17 00:00:00 2001 From: Geoff Wilson Date: Fri, 6 Sep 2024 11:46:11 -0400 Subject: [PATCH 09/10] remove extra private method --- app/lib/services/domain_ownership_service.rb | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/app/lib/services/domain_ownership_service.rb b/app/lib/services/domain_ownership_service.rb index 70e594e..1035e37 100644 --- a/app/lib/services/domain_ownership_service.rb +++ b/app/lib/services/domain_ownership_service.rb @@ -2,7 +2,7 @@ module Services class DomainOwnershipService def authorize!(identity, cert_req) cert_req.fqdns.each do |fqdn| - domain = get_domain_name(fqdn) + domain = Domain.where(fqdn: fqdn).first raise AuthError unless domain.present? && (domain.owner == identity.subject || (domain.group_delegation && @@ -10,11 +10,5 @@ def authorize!(identity, cert_req) end nil end - - private - - def get_domain_name(fqdn) - Domain.where(fqdn: fqdn).first - end end end From 212fa874434a644cfd19a73178eb31df988fda66 Mon Sep 17 00:00:00 2001 From: Geoff Wilson Date: Fri, 6 Sep 2024 11:57:24 -0400 Subject: [PATCH 10/10] update test to use db Domain --- .../services/domain_ownership_service_test.rb | 38 +++++++------------ 1 file changed, 14 insertions(+), 24 deletions(-) diff --git a/test/lib/services/domain_ownership_service_test.rb b/test/lib/services/domain_ownership_service_test.rb index e35ab43..4f5815a 100644 --- a/test/lib/services/domain_ownership_service_test.rb +++ b/test/lib/services/domain_ownership_service_test.rb @@ -2,44 +2,34 @@ class DomainOwnershipServiceTest < ActiveSupport::TestCase def setup - @identity = Identity.new(subject: "test@example.com", groups: [ "admin_group" ]) - @domain = Domain.new(owner: "test@example.com", group_delegation: false, groups: [ "admin_group" ]) + @domain = domains(:group_match) + @identity = Identity.new(subject: @domain.owner) + @cr = CertIssueRequest.new(common_name: @domain.fqdn) + @ds = Services::DomainOwnershipService.new end test "#authorize! with matching owner" do - ds = Services::DomainOwnershipService.new - ds.stub :get_domain_name, @domain do - assert_nil(ds.authorize!(@identity, CertIssueRequest.new)) - end + assert_nil(@ds.authorize!(@identity, @cr)) end test "#authorize! with non-matching owner" do - ds = Services::DomainOwnershipService.new - @domain.owner = "different_owner@example.com" - ds.stub :get_domain_name, @domain do - assert_raises(AuthError) do - ds.authorize!(@identity, CertIssueRequest.new) - end + @identity.subject = "different_owner@example.com" + assert_raises(AuthError) do + @ds.authorize!(@identity, @cr) end end test "#authorize! with matching group" do - ds = Services::DomainOwnershipService.new - @domain.owner = "different_owner@example.com" - @domain.group_delegation = true - ds.stub :get_domain_name, @domain do - assert_nil(ds.authorize!(@identity, CertIssueRequest.new)) - end + @domain.update(owner: "different_owner@example.com") + @identity.groups = @domain.groups + assert_nil(@ds.authorize!(@identity, @cr)) end test "#authorize! with non-matching group" do - ds = Services::DomainOwnershipService.new - @domain.owner = "different_owner@example.com" + @domain.update(owner: "different_owner@example.com") @identity.groups = [ "different_group" ] - ds.stub :get_domain_name, @domain do - assert_raises(AuthError) do - ds.authorize!(@identity, CertIssueRequest.new) - end + assert_raises(AuthError) do + @ds.authorize!(@identity, @cr) end end end