From 8da7da1b94faabea610b1ad6b239475b6c7d722c Mon Sep 17 00:00:00 2001
From: Geoffrey Wilson <geoff@gr-oss.io>
Date: Tue, 5 Nov 2024 13:33:56 -0500
Subject: [PATCH] Kv policy verification (#72)

* verify the upstream user has policy associated with KV item for read or delete.

* Add test of kv_write; verify policy creation

* add kv_delete test

* Add advisory lock to prevent policies race
---
 Gemfile                            |  3 +++
 Gemfile.lock                       |  4 ++++
 app/lib/clients/vault/key_value.rb |  3 ++-
 app/lib/clients/vault/policy.rb    | 18 ++++++++++++++----
 test/lib/clients/vault_test.rb     | 11 ++++++++++-
 5 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/Gemfile b/Gemfile
index 75562b2..6899b36 100644
--- a/Gemfile
+++ b/Gemfile
@@ -48,6 +48,9 @@ gem "jbuilder"
 gem "faraday"
 gem "faraday-retry"
 
+# Use with_advisory_lock for multiprocess mutex
+gem "with_advisory_lock"
+
 group :development, :test do
   # See https://guides.rubyonrails.org/debugging_rails_applications.html#debugging-with-the-debug-gem
   gem "debug", platforms: %i[ mri mswin ], require: "debug/prelude"
diff --git a/Gemfile.lock b/Gemfile.lock
index 2f63d3c..6f4557d 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -269,6 +269,9 @@ GEM
     websocket-driver (0.7.6)
       websocket-extensions (>= 0.1.0)
     websocket-extensions (0.1.5)
+    with_advisory_lock (5.1.0)
+      activerecord (>= 6.1)
+      zeitwerk (>= 2.6)
     zeitwerk (2.7.1)
 
 PLATFORMS
@@ -304,6 +307,7 @@ DEPENDENCIES
   sqlite3 (>= 1.4)
   tzinfo-data
   vault
+  with_advisory_lock
 
 BUNDLED WITH
    2.5.11
diff --git a/app/lib/clients/vault/key_value.rb b/app/lib/clients/vault/key_value.rb
index ac02d10..9f5b59e 100644
--- a/app/lib/clients/vault/key_value.rb
+++ b/app/lib/clients/vault/key_value.rb
@@ -4,6 +4,7 @@ module KeyValue
       extend Policy
 
       def kv_read(identity, path)
+        verify_policy(identity, policy_path(path))
         client.kv(kv_mount).read(path)
       end
 
@@ -14,6 +15,7 @@ def kv_write(identity, path, data)
       end
 
       def kv_delete(identity, path)
+        verify_policy(identity, policy_path(path))
         client.logical.delete("#{kv_mount}/data/#{path}")
       end
 
@@ -33,7 +35,6 @@ def kv_engine_type
         "kv-v2"
       end
 
-
       def create_kv_policy(path)
         client.sys.put_policy(policy_path(path), kv_policy(path))
       end
diff --git a/app/lib/clients/vault/policy.rb b/app/lib/clients/vault/policy.rb
index c3dd0ea..1e6eab3 100644
--- a/app/lib/clients/vault/policy.rb
+++ b/app/lib/clients/vault/policy.rb
@@ -12,10 +12,20 @@ def rotate_token
       def assign_policy(identity, policy_name)
         sub = identity.sub
         email = identity.email
-        policies, metadata = get_entity_data(sub)
-        policies.append(policy_name).uniq!
-        put_entity(sub, policies, metadata)
-        put_entity_alias(sub, email, "oidc")
+        Domain.with_advisory_lock(sub) do
+          policies, metadata = get_entity_data(sub)
+          policies.append(policy_name).uniq!
+          put_entity(sub, policies, metadata)
+          put_entity_alias(sub, email, "oidc")
+        end
+      end
+
+      def verify_policy(identity, policy_name)
+        sub = identity.sub
+        policies, _ = get_entity_data(sub)
+        unless policies.any? { |p| p == policy_name }
+          raise AuthError.new("Policy has not been granted to the identity")
+        end
       end
 
       private
diff --git a/test/lib/clients/vault_test.rb b/test/lib/clients/vault_test.rb
index cd23027..f34f5eb 100644
--- a/test/lib/clients/vault_test.rb
+++ b/test/lib/clients/vault_test.rb
@@ -98,6 +98,16 @@ class VaultTest < ActiveSupport::TestCase
     entity = @client.read_entity(@identity.sub)
     assert_equal "kv_policy/#{path}", entity.data[:policies][0]
 
+    # check kv_read denied to other identity
+    alt_identity = Identity.new
+    alt_identity.sub = SecureRandom.hex(4)
+    err = assert_raises { @client.kv_read(alt_identity, path) }
+    assert_kind_of AuthError, err
+
+    # check kv_delete denied to other identity
+    err = assert_raises { @client.kv_delete(alt_identity, path) }
+    assert_kind_of AuthError, err
+
     # check kv_delete
     del_secret = @client.kv_delete(@identity, path)
     assert del_secret
@@ -105,7 +115,6 @@ class VaultTest < ActiveSupport::TestCase
     assert_nil read_secret
   end
 
-
   test "entity_alias methods" do
     # confirm no entity yet
     err = assert_raises RuntimeError do