From 8da7da1b94faabea610b1ad6b239475b6c7d722c Mon Sep 17 00:00:00 2001 From: Geoffrey Wilson Date: Tue, 5 Nov 2024 13:33:56 -0500 Subject: [PATCH 1/2] 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 From 93424773f6e5d1cb69a350d12adcaa995646fd39 Mon Sep 17 00:00:00 2001 From: GeorgeJahad Date: Tue, 5 Nov 2024 10:48:54 -0800 Subject: [PATCH 2/2] Add support for JWKS decoding of JWT's (#68) --- README.md | 4 +- app/lib/jwt/jwks_decoder.rb | 40 +++++++++++++++++++ app/lib/jwt/secret_decoder.rb | 20 ++++++++++ app/lib/services/auth.rb | 13 +----- app/lib/utils/decoder_factory.rb | 23 +++++++++++ config/astral.yml | 3 ++ test/lib/jwt/jwks_decoder_test.rb | 39 ++++++++++++++++++ test/lib/jwt/secret_decoder_test.rb | 10 +++++ test/lib/utils/decoder_factory_test.rb | 29 ++++++++++++++ .../{clients => utils}/oidc_provider_test.rb | 0 10 files changed, 168 insertions(+), 13 deletions(-) create mode 100644 app/lib/jwt/jwks_decoder.rb create mode 100644 app/lib/jwt/secret_decoder.rb create mode 100644 app/lib/utils/decoder_factory.rb create mode 100644 test/lib/jwt/jwks_decoder_test.rb create mode 100644 test/lib/jwt/secret_decoder_test.rb create mode 100644 test/lib/utils/decoder_factory_test.rb rename test/lib/{clients => utils}/oidc_provider_test.rb (100%) diff --git a/README.md b/README.md index c32f6cf..0caa8fa 100644 --- a/README.md +++ b/README.md @@ -201,4 +201,6 @@ the provider settings, so you will need to clear the browser's * Vault login failed. Expired or missing OAuth state. ``` - +# Decoding JWKS based tokens +To decode JWKS based tokens, set the astral.yml "jwks_url" parameter to the +jwks endpoint of your auth provider. diff --git a/app/lib/jwt/jwks_decoder.rb b/app/lib/jwt/jwks_decoder.rb new file mode 100644 index 0000000..021d3e4 --- /dev/null +++ b/app/lib/jwt/jwks_decoder.rb @@ -0,0 +1,40 @@ +require "open-uri" +module Jwt + class JwksDecoder + def initialize(url) + @url = url + end + + def configured?(config) + !@url.nil? + end + + # Decode a JWT token signed with JWKS + def decode(token) + jwks = get_jwks_keyset_from_configured_url + jwks = filter_out_non_signing_keys(jwks) + body = JWT.decode(token, nil, true, + algorithms: get_algorithms_from_keyset(jwks), + jwks: jwks)[0] + Identity.new(body) + rescue => e + Rails.logger.warn "Unable to decode token: #{e}" + nil + end + + private + + def get_jwks_keyset_from_configured_url + jwks_json = URI.open(@url) { |f| f.read } + JWT::JWK::Set.new(JSON.parse(jwks_json)) + end + + def filter_out_non_signing_keys(jwks) + jwks.filter { |k| k[:use] == "sig" } + end + + def get_algorithms_from_keyset(jwks) + jwks.map { |k| k[:alg] }.compact.uniq + end + end +end diff --git a/app/lib/jwt/secret_decoder.rb b/app/lib/jwt/secret_decoder.rb new file mode 100644 index 0000000..7cb34e7 --- /dev/null +++ b/app/lib/jwt/secret_decoder.rb @@ -0,0 +1,20 @@ +module Jwt + class SecretDecoder + def initialize(secret) + @secret = secret + end + + def configured?(config) + !@secret.nil? + end + + def decode(token) + # Decode a JWT access token using the configured base. + body = JWT.decode(token, Config[:jwt_signing_key])[0] + Identity.new(body) + rescue => e + Rails.logger.warn "Unable to decode token: #{e}" + nil + end + end +end diff --git a/app/lib/services/auth.rb b/app/lib/services/auth.rb index 963b7d0..6c07dbe 100644 --- a/app/lib/services/auth.rb +++ b/app/lib/services/auth.rb @@ -2,22 +2,11 @@ module Services class Auth class << self def authenticate!(token) - identity = decode(token) + identity = Utils::DecoderFactory.get(Config).decode(token) raise AuthError unless identity # TODO verify identity with authority? identity end - - private - - def decode(token) - # Decode a JWT access token using the configured base. - body = JWT.decode(token, Config[:jwt_signing_key])[0] - Identity.new(body) - rescue => e - Rails.logger.warn "Unable to decode token: #{e}" - nil - end end end end diff --git a/app/lib/utils/decoder_factory.rb b/app/lib/utils/decoder_factory.rb new file mode 100644 index 0000000..bad2f95 --- /dev/null +++ b/app/lib/utils/decoder_factory.rb @@ -0,0 +1,23 @@ +module Utils + class DecoderFactory + cattr_reader :decoders + class << self + # Any new decoders should be added here: + @@decoders = [ Jwt::JwksDecoder.new(Config[:jwks_url]), + Jwt::SecretDecoder.new(Config[:jwt_signing_key]) ] + + def get(config) + configured_decoders = getConfiguredDecoders(config) + if configured_decoders.length != 1 + raise "Exactly one decoder must be configured" + end + configured_decoders.first + end + + private + def getConfiguredDecoders(config) + decoders.filter { |c| c.configured?(config) } + end + end + end +end diff --git a/config/astral.yml b/config/astral.yml index 287e475..8700280 100644 --- a/config/astral.yml +++ b/config/astral.yml @@ -21,6 +21,9 @@ shared: jwt_signing_key: + # define this to allow jwks decoding of JWT's + jwks_url: + # When using AppRegistry for Domain Ownership information app_registry_addr: app_registry_token: diff --git a/test/lib/jwt/jwks_decoder_test.rb b/test/lib/jwt/jwks_decoder_test.rb new file mode 100644 index 0000000..3b06875 --- /dev/null +++ b/test/lib/jwt/jwks_decoder_test.rb @@ -0,0 +1,39 @@ +require "test_helper" +require "jwt" +require "openssl" +require "json" +require "tempfile" + +class JwksDecoderTest < ActiveSupport::TestCase + test ".decode returns correct identity" do + jwk = generate_jwks_signing_key + token = generate_jwks_token(jwk) + keyset_path = generate_jwks_keyset(jwk) + + identity = Jwt::JwksDecoder.new(keyset_path).decode(token) + assert_equal "john.doe@example.com", identity.sub + assert_equal "astral", identity.aud + end + + private + + def generate_jwks_signing_key + optional_parameters = { kid: "1", use: "sig", alg: "RS256" } + jwk = JWT::JWK.new(OpenSSL::PKey::RSA.new(2048), optional_parameters) + end + + def generate_jwks_token(jwk) + payload = { "sub"=>"john.doe@example.com", "name"=>"John Doe", "iat"=>1516239022, + "groups"=>[ "group1", "group2" ], "aud"=>"astral" } + + JWT.encode(payload, jwk.signing_key, jwk[:alg], kid: jwk[:kid]) + end + + def generate_jwks_keyset(jwk) + jwks_hash = JWT::JWK::Set.new(jwk).export + f = Tempfile.new + f.write(JSON.pretty_generate(jwks_hash)) + f.close + f.path + end +end diff --git a/test/lib/jwt/secret_decoder_test.rb b/test/lib/jwt/secret_decoder_test.rb new file mode 100644 index 0000000..be08472 --- /dev/null +++ b/test/lib/jwt/secret_decoder_test.rb @@ -0,0 +1,10 @@ +require "test_helper" + +class SecretDecoderTest < ActiveSupport::TestCase + test ".decode returns correct identity" do + identity = Jwt::SecretDecoder.new(Config[:jwt_signing_key]). + decode(jwt_authorized) + assert_equal "john.doe@example.com", identity.sub + assert_equal "astral", identity.aud + end +end diff --git a/test/lib/utils/decoder_factory_test.rb b/test/lib/utils/decoder_factory_test.rb new file mode 100644 index 0000000..4f56c11 --- /dev/null +++ b/test/lib/utils/decoder_factory_test.rb @@ -0,0 +1,29 @@ +require "test_helper" + +class DecoderFactoryTest < ActiveSupport::TestCase + test ".get returns configured decoder" do + decoders = [ UnconfiguredDecoder.new, ConfiguredDecoder.new ] + Utils::DecoderFactory.stub :decoders, decoders do + decoder = Utils::DecoderFactory.get({}) + assert decoder.instance_of?(ConfiguredDecoder) + end + end + + test ".get recognizes invalid config" do + decoders = [ ConfiguredDecoder.new, ConfiguredDecoder.new ] + Utils::DecoderFactory.stub :decoders, decoders do + assert_raises( + RuntimeError, "Exactly one decoder must be configured") do + decoder = Utils::DecoderFactory.get({}) + end + end + end + + class ConfiguredDecoder + def configured?(c) = true + end + + class UnconfiguredDecoder + def configured?(c) = false + end +end diff --git a/test/lib/clients/oidc_provider_test.rb b/test/lib/utils/oidc_provider_test.rb similarity index 100% rename from test/lib/clients/oidc_provider_test.rb rename to test/lib/utils/oidc_provider_test.rb