Kv group auth

Gemfile

@@ -60,4 +60,7 @@ group :development, :test do
 
   # Omakase Ruby styling [https://github.com/rails/rubocop-rails-omakase/]
   gem "rubocop-rails-omakase", require: false
+
+  # Mocking for tests
+  gem "mocha"
 end

Gemfile.lock

@@ -132,6 +132,8 @@ GEM
     marcel (1.0.4)
     mini_mime (1.1.5)
     minitest (5.25.1)
+    mocha (2.5.0)
+      ruby2_keywords (>= 0.0.5)
     msgpack (1.7.2)
     net-http (0.4.1)
       uri
@@ -243,6 +245,7 @@ GEM
       rubocop-performance
       rubocop-rails
     ruby-progressbar (1.13.0)
+    ruby2_keywords (0.0.5)
     securerandom (0.3.1)
     sqlite3 (2.2.0-aarch64-linux-gnu)
     sqlite3 (2.2.0-aarch64-linux-musl)
@@ -300,6 +303,7 @@ DEPENDENCIES
   jbuilder
   json_tagged_logger
   jwt
+  mocha
   ostruct
   puma (>= 5.0)
   rails (~> 7.2.2)

app/controllers/secrets_controller.rb

@@ -39,6 +39,6 @@ def destroy
   private
 
   def params_permitted
-    params.require(:secret).permit(:path, data: {})
+    params.require(:secret).permit(:path, :groups, data: {})
   end
 end

app/interactors/write_secret.rb

@@ -1,6 +1,6 @@
 class WriteSecret < ApplicationInteractor
   def call
-    if secret = Services::KeyValue.write(context.identity, context.request.path, context.request.data)
+    if secret = Services::KeyValue.write(context.identity, context.request.groups_array, context.request.path, context.request.data)
       context.secret = secret
     else
       context.fail!(message: "Failed to store secret")

app/lib/clients/vault/certificate.rb

@@ -9,7 +9,7 @@ def issue_cert(identity, cert_issue_request)
         opts = cert_issue_request.attributes
         # Generate the TLS certificate using the intermediate CA
         tls_cert = client.logical.write(cert_path, opts)
-        assign_policy(identity, GENERIC_CERT_POLICY_NAME)
+        assign_identity_policy(identity, GENERIC_CERT_POLICY_NAME)
         OpenStruct.new tls_cert.data
       end
 

app/lib/clients/vault/key_value.rb

@@ -4,20 +4,30 @@ module KeyValue
       extend Policy
 
       def kv_read(identity, path)
-        verify_policy(identity, policy_path(path))
+        verify_policy(identity, producer_policy_path(path), consumer_policy_path(path))
         client.kv(kv_mount).read(path)
       end
 
-      def kv_write(identity, path, data)
-        create_kv_policy(path)
-        assign_policy(identity, policy_path(path))
+      def kv_write(identity, groups, path, data)
+        # only producer can replace existing secret
+        if client.kv(kv_mount).read(path)
+          verify_policy(identity, producer_policy_path(path))
+        end
+
+        create_kv_policies(path)
+        assign_identity_policy(identity, producer_policy_path(path))
+        assign_groups_policy(groups, consumer_policy_path(path))
         client.logical.write("#{kv_mount}/data/#{path}", data: data)
       end
 
       def kv_delete(identity, path)
-        verify_policy(identity, policy_path(path))
+        unless client.kv(kv_mount).read(path)
+          return
+        end
+        verify_policy(identity, producer_policy_path(path))
         client.logical.delete("#{kv_mount}/data/#{path}")
-        remove_policy(identity, policy_path(path))
+        remove_identity_policy(identity, producer_policy_path(path))
+        remove_groups_policy(consumer_policy_path(path))
       end
 
       def configure_kv
@@ -36,21 +46,34 @@ def kv_engine_type
         "kv-v2"
       end
 
-      def create_kv_policy(path)
-        client.sys.put_policy(policy_path(path), kv_policy(path))
+      def create_kv_policies(path)
+        client.sys.put_policy(producer_policy_path(path), kv_producer_policy(path))
+        client.sys.put_policy(consumer_policy_path(path), kv_consumer_policy(path))
       end
 
-      def policy_path(path)
-        "kv_policy/#{path}"
+      def producer_policy_path(path)
+        "kv_policy/#{path}/producer"
       end
 
-      def kv_policy(path)
+      def consumer_policy_path(path)
+        "kv_policy/#{path}/consumer"
+      end
+
+      def kv_producer_policy(path)
         policy = <<-EOH
                path "#{path}" {
                  capabilities = ["create", "read", "update", "delete"]
                }
         EOH
       end
+
+      def kv_consumer_policy(path)
+        policy = <<-EOH
+               path "#{path}" {
+                 capabilities = ["read"]
+               }
+        EOH
+      end
     end
   end
 end

app/lib/clients/vault/oidc.rb

@@ -19,6 +19,24 @@ def get_oidc_client_config
         client.logical.read("auth/oidc/config")
       end
 
+      def create_oidc_role(role_name, groups, policy_name)
+        client.logical.write("auth/oidc/role/#{role_name}",
+                             user_claim: "sub",
+                             groups_claim: "groups",
+                             bound_claims: { "groups" => groups },
+                             policies: policy_name,
+                             oidc_scopes: "email groups",
+                             allowed_redirect_uris: Config[:oidc_redirect_uris])
+      end
+
+      def remove_oidc_role(role_name)
+        client.logical.delete("auth/oidc/role/#{role_name}")
+      end
+
+      def read_oidc_role(role_name)
+        client.logical.read("auth/oidc/role/#{role_name}")
+      end
+
       private
 
       def create_client_config(issuer, client_id, client_secret)

app/lib/clients/vault/policy.rb

@@ -2,14 +2,15 @@ module Clients
   class Vault
     module Policy
       extend Entity
+      extend Oidc
 
       def rotate_token
         create_astral_policy
         token = create_astral_token
         Clients::Vault.token = token
       end
 
-      def assign_policy(identity, policy_name)
+      def assign_identity_policy(identity, policy_name)
         sub = identity.sub
         email = identity.email
         Domain.with_advisory_lock(sub) do
@@ -20,15 +21,25 @@ def assign_policy(identity, policy_name)
         end
       end
 
-      def verify_policy(identity, policy_name)
+      def assign_groups_policy(groups, policy_name)
+        create_oidc_role(make_role_name(policy_name), groups, policy_name)
+      end
+
+      def verify_policy(identity, producer_policy_name, consumer_policy_name = nil)
+        # check identity policies
         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")
+        return if (policies || []).any? { |p| p == producer_policy_name }
+
+        # check group membership in consumer policy if given
+        if consumer_policy_name.present?
+          role = read_oidc_role(make_role_name(consumer_policy_name))
+          return if ((role&.data&.dig(:bound_claims, :groups) || []) & identity.groups).any?
         end
+        raise AuthError.new("Policy has not been granted to the identity")
       end
 
-      def remove_policy(identity, policy_name)
+      def remove_identity_policy(identity, policy_name)
         sub = identity.sub
         Domain.with_advisory_lock(sub) do
           policies, metadata = get_entity_data(sub)
@@ -38,8 +49,16 @@ def remove_policy(identity, policy_name)
         client.sys.delete_policy(policy_name)
       end
 
+      def remove_groups_policy(policy_name)
+        remove_oidc_role(make_role_name(policy_name))
+      end
+
       private
 
+      def make_role_name(policy_name)
+        %Q(#{policy_name.gsub("/", "_")}-role)
+      end
+
       def create_astral_policy
         policy = <<-HCL
         path "#{intermediate_ca_mount}/roles/astral" {
@@ -66,6 +85,9 @@ def create_astral_policy
         path "auth/oidc/config" {
           capabilities = ["read"]
         }
+        path "auth/oidc/role/*" {
+          capabilities = ["create", "read", "update", "delete", "list"]
+        }
         path "/sys/policy/*" {
           capabilities = ["create", "read", "update", "delete", "list"]
         }

app/lib/requests/secret_request.rb

@@ -4,9 +4,14 @@ class SecretRequest
     include ActiveModel::Attributes
 
     attribute :path, :string
+    attribute :groups, :string
     attribute :data
     alias_attribute :kv_path, :path
 
     validates :path, presence: true
+
+    def groups_array
+      (groups || "").split(",").sort.uniq
+    end
   end
 end

app/lib/services/key_value.rb

@@ -5,8 +5,8 @@ def read(identity, path)
         impl.kv_read(identity, path)
       end
 
-      def write(identity, path, data)
-        impl.kv_write(identity, path, data)
+      def write(identity, read_groups, path, data)
+        impl.kv_write(identity, read_groups, path, data)
       end
 
       def delete(identity, path)

app/models/domain.rb

@@ -5,7 +5,6 @@ class Domain < ApplicationRecord
     encrypts :fqdn, :users, :groups
   end
 
-
   def groups_array
     (groups || "").split(",").sort.uniq
   end

doc/openapi/paths/secrets.yml

@@ -15,6 +15,10 @@ postSecrets:
                 type: string
                 description: "Path where the secret is stored"
                 example: "secret/storage/path"
+              groups:
+                type: string
+                description: "Comma-separated list of OIDC groups allowed to read the secret"
+                example: "svc-account1,dev-group1"
               data:
                 type: object
                 description: "The secret data"

test/lib/clients/vault/policy_test.rb

@@ -0,0 +1,43 @@
+require "test_helper"
+
+class PolicyTest < ActiveSupport::TestCase
+  setup do
+    @client = Clients::Vault
+    @identity = Identity.new
+    email = SecureRandom.hex(4)
+    @identity.sub = email
+  end
+
+  test "#verify_policy raises when identity does not have the policy" do
+    policy_name = "some/policy/name"
+    @client.expects(:get_entity_data).with(@identity.sub).returns([ [ "some/other/policy" ], nil ])
+    err = assert_raises { @client.verify_policy(@identity, policy_name) }
+    assert_kind_of AuthError, err
+  end
+
+  test "#verify_policy permits identity having the policy" do
+    policy_name = "some/policy/name"
+    @client.expects(:get_entity_data).with(@identity.sub).returns([ [ policy_name ], nil ])
+    assert_nil @client.verify_policy(@identity, policy_name)
+  end
+
+  test "#verify_policy looks for a role corresponding to consumer policy when supplied" do
+    producer_policy = "some/policy/name"
+    consumer_policy = "some/policy/other"
+    read_oidc_response = OpenStruct.new(data: { bound_claims: { groups: [ "my-group" ] } })
+    @client.expects(:get_entity_data).with(@identity.sub).returns([ [], nil ])
+    @client.expects(:read_oidc_role).with("some_policy_other-role").returns(read_oidc_response)
+    err = assert_raises { @client.verify_policy(@identity, producer_policy, consumer_policy) }
+    assert_kind_of AuthError, err
+  end
+
+  test "#verify_policy permits identity having group linked to consumer policy role" do
+    producer_policy = "some/policy/name"
+    consumer_policy = "some/policy/other"
+    @identity.groups = [ "my-group" ]
+    read_oidc_response = OpenStruct.new(data: { bound_claims: { groups: [ "my-group" ] } })
+    @client.expects(:get_entity_data).with(@identity.sub).returns([ [], nil ])
+    @client.expects(:read_oidc_role).with("some_policy_other-role").returns(read_oidc_response)
+    assert_nil @client.verify_policy(@identity, producer_policy, consumer_policy)
+  end
+end

test/lib/clients/vault_test.rb

@@ -64,11 +64,13 @@ class VaultTest < ActiveSupport::TestCase
   test ".rotate_token" do
     # begins with default token
     assert_equal vault_token, @client.token
-    assert @client.rotate_token
     # now has a new token
+    assert @client.rotate_token
     assert_not_equal vault_token, @client.token
     # ensure we can write with the new token
-    assert_instance_of Vault::Secret, @client.kv_write(@identity, "testing/secret", { password: "sicr3t" })
+    kv_path = "testing/#{SecureRandom.hex}"
+    assert_instance_of Vault::Secret, @client.kv_write(@identity, [], kv_path, { password: "sicr3t" })
+    assert @client.kv_delete(@identity, kv_path)
   end
 
   test "entity methods" do
@@ -87,7 +89,7 @@ class VaultTest < ActiveSupport::TestCase
   test "kv methods" do
     # check kv_write
     path = "test/path/#{SecureRandom.hex}"
-    secret = @client.kv_write(@identity, path, { data: "data" })
+    secret = @client.kv_write(@identity, [ "group_can_read" ], path, { data: "data" })
     assert_kind_of Vault::Secret, secret
 
     # check kv_read
@@ -96,15 +98,20 @@ class VaultTest < ActiveSupport::TestCase
 
     # check policy is created
     entity = @client.read_entity(@identity.sub)
-    assert_includes entity.data[:policies], "kv_policy/#{path}"
+    assert_includes entity.data[:policies], "kv_policy/#{path}/producer"
 
-    # check kv_read denied to other identity
+    # check kv_read denied to other identity by default
     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
+    # check kv_read permitted to other identity with group membership
+    alt_identity.groups = [ "group_can_read" ]
+    group_read_secret = @client.kv_read(alt_identity, path)
+    assert_kind_of Vault::Secret, group_read_secret
+
+    # check kv_delete denied to other identity even with group
     err = assert_raises { @client.kv_delete(alt_identity, path) }
     assert_kind_of AuthError, err
 
@@ -146,8 +153,8 @@ class VaultTest < ActiveSupport::TestCase
     assert_match /no such alias/, err.message
   end
 
-  test ".assign_policy creates valid entity" do
-    @client.assign_policy(@identity, "test_path")
+  test ".assign_identity_policy creates valid entity" do
+    @client.assign_identity_policy(@identity, "test_path")
     entity = @client.read_entity(@identity.sub)
     assert entity.data[:policies].any? { |p|
       p == "test_path" }

test/test_helper.rb

@@ -2,6 +2,8 @@
 require_relative "../config/environment"
 require "rails/test_help"
 require "minitest/mock"
+require "minitest/spec"
+require "mocha/minitest"
 
 module ActiveSupport
   class TestCase