Skip to content

Commit

Permalink
Improve ID token validation and data extraction
Browse files Browse the repository at this point in the history
* Removes use of the 'upn' claim for email addresses, since if an
  'email' claim is not provided, the user's email is not verified
* Uses the combination of 'tid' and 'oid' as a uuid, since 'oid' is
  only unique within a single-tenant
* Adds validation of 'aud', 'iss', 'nbf' and 'exp' id token claims

Ref RIPAGlobal#33
  • Loading branch information
tom-brouwer-bex committed Sep 11, 2024
1 parent b239e6d commit 295de28
Show file tree
Hide file tree
Showing 2 changed files with 147 additions and 9 deletions.
27 changes: 25 additions & 2 deletions lib/omniauth/strategies/azure_activedirectory_v2.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ class AzureActivedirectoryV2 < OmniAuth::Strategies::OAuth2

option :name, 'azure_activedirectory_v2'
option :tenant_provider, nil
option :jwt_leeway, 60


DEFAULT_SCOPE = 'openid profile email'

Expand Down Expand Up @@ -64,12 +66,17 @@ def client
super
end

uid { raw_info['oid'] }
uid do
# as instructed by https://learn.microsoft.com/en-us/entra/identity-platform/migrate-off-email-claim-authorization
raw_info['tid'] + raw_info['oid']
# Alternative would be to use 'sub' but this is only unique in client/app registration context. If a different
# app registration is used, the 'sub' values can be different
end

info do
{
name: raw_info['name'],
email: raw_info['email'] || raw_info['upn'],
email: raw_info['email'],
nickname: raw_info['unique_name'],
first_name: raw_info['given_name'],
last_name: raw_info['family_name']
Expand Down Expand Up @@ -101,6 +108,22 @@ def raw_info
rescue StandardError
{}
end

# For multi-tenant apps ('common' tenant_id) it doesn't make any sense to verify the token issuer, because the
# value of 'iss' in the token depends on the 'tid' in the token itself
issuer = options.tenant_id.nil? ? nil : "#{options.base_azure_url}/#{options.tenant_id}/v2.0"

# https://learn.microsoft.com/en-us/entra/identity-platform/id-tokens#validate-tokens
JWT::Verify.verify_claims(
id_token_data,
verify_iss: !issuer.nil?,
iss: issuer,
verify_aud: true,
aud: options.client_id,
verify_expiration: true,
verify_not_before: true,
leeway: options[:jwt_leeway]
)
auth_token_data = begin
::JWT.decode(access_token.token, nil, false).first
rescue StandardError
Expand Down
129 changes: 122 additions & 7 deletions spec/omniauth/strategies/azure_activedirectory_v2_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -380,10 +380,22 @@ def adfs?
end

let(:id_token_info) do
issued_at = Time.now.utc.to_i
expires_at = (Time.now + 3600).to_i
{
oid: 'my_id',
name: 'Bob Doe',
email: '[email protected]',
ver: '2.0',
iss: 'https://login.microsoftonline.com/9188040d-6c67-4c5b-b112-36a304b66dad/v2.0',
sub: 'sdfkjllAkdkWkeiidkcXKfjjsl',
aud: 'id',
exp: expires_at,
iat: issued_at,
nbf: issued_at,
name: 'Bob Doe',
preferred_username: '[email protected]',
oid: 'my_id',
email: '[email protected]',
tid: '9188040d-6c67-4c5b-b112-36a304b66dad',
aio: 'KSslldiwDkfjjsoeiruosKD',
unique_name: 'bobby'
}
end
Expand Down Expand Up @@ -413,15 +425,27 @@ def adfs?
end

it 'returns correct uid' do
expect(subject.uid).to eq('my_id')
expect(subject.uid).to eq('9188040d-6c67-4c5b-b112-36a304b66dadmy_id')
end
end # "context 'with information only in the ID token' do"

context 'with extra information in the auth token' do
let(:auth_token_info) do
issued_at = Time.now.utc.to_i
expires_at = (Time.now + 3600).to_i
{
oid: 'overridden_id',
email: '[email protected]',
ver: '2.0',
iss: 'https://login.microsoftonline.com/9188040d-6c67-4c5b-b112-36a304b66dad/v2.0',
sub: 'sdfkjllAkdkWkeiidkcXKfjjsl',
aud: 'id',
exp: expires_at,
iat: issued_at,
nbf: issued_at,
preferred_username: '[email protected]',
oid: 'overridden_id',
email: '[email protected]',
tid: '9188040d-6c67-4c5b-b112-36a304b66dad',
aio: 'KSslldiwDkfjjsoeiruosKD',
unique_name: 'Bobby Definitely Doe',
given_name: 'Bob',
family_name: 'Doe'
Expand All @@ -447,9 +471,100 @@ def adfs?
end

it 'returns correct uid' do
expect(subject.uid).to eq('overridden_id')
expect(subject.uid).to eq('9188040d-6c67-4c5b-b112-36a304b66dadoverridden_id')
end
end # "context 'with extra information in the auth token' do"

context 'with an invalid audience' do
let(:id_token_info) do
issued_at = Time.now.utc.to_i
expires_at = (Time.now + 3600).to_i
{
ver: '2.0',
iss: 'https://login.microsoftonline.com/9188040d-6c67-4c5b-b112-36a304b66dad/v2.0',
sub: 'sdfkjllAkdkWkeiidkcXKfjjsl',
aud: 'other-id',
exp: expires_at,
iat: issued_at,
nbf: issued_at,
name: 'Bob Doe',
preferred_username: '[email protected]',
oid: 'my_id',
email: '[email protected]',
tid: '9188040d-6c67-4c5b-b112-36a304b66dad',
aio: 'KSslldiwDkfjjsoeiruosKD',
unique_name: 'bobby'
}
end

it 'fails validation' do
expect { subject.info }.to raise_error(JWT::InvalidAudError)
end
end

context 'with an invalid issuer' do
subject do
OmniAuth::Strategies::AzureActivedirectoryV2.new(app, {client_id: 'id', client_secret: 'secret', tenant_id: 'test-tenant'})
end

it 'fails validation' do
expect { subject.info }.to raise_error(JWT::InvalidIssuerError)
end
end

context 'with an invalid not_before' do
let(:id_token_info) do
issued_at = (Time.now + 70).to_i # Since leeway is 60 seconds
expires_at = (Time.now + 3600).to_i
{
ver: '2.0',
iss: 'https://login.microsoftonline.com/9188040d-6c67-4c5b-b112-36a304b66dad/v2.0',
sub: 'sdfkjllAkdkWkeiidkcXKfjjsl',
aud: 'id',
exp: expires_at,
iat: issued_at,
nbf: issued_at,
name: 'Bob Doe',
preferred_username: '[email protected]',
oid: 'my_id',
email: '[email protected]',
tid: '9188040d-6c67-4c5b-b112-36a304b66dad',
aio: 'KSslldiwDkfjjsoeiruosKD',
unique_name: 'bobby'
}
end

it 'fails validation' do
expect { subject.info }.to raise_error(JWT::ImmatureSignature)
end
end

context 'with an expired token' do
let(:id_token_info) do
issued_at = (Time.now - 3600).to_i
expires_at = (Time.now - 70).to_i # Since leeway is 60 seconds
{
ver: '2.0',
iss: 'https://login.microsoftonline.com/9188040d-6c67-4c5b-b112-36a304b66dad/v2.0',
sub: 'sdfkjllAkdkWkeiidkcXKfjjsl',
aud: 'id',
exp: expires_at,
iat: issued_at,
nbf: issued_at,
name: 'Bob Doe',
preferred_username: '[email protected]',
oid: 'my_id',
email: '[email protected]',
tid: '9188040d-6c67-4c5b-b112-36a304b66dad',
aio: 'KSslldiwDkfjjsoeiruosKD',
unique_name: 'bobby'
}
end

it 'fails validation' do
expect { subject.info }.to raise_error(JWT::ExpiredSignature)
end
end
end # "describe 'raw_info' do"

describe 'callback_url' do
Expand Down

0 comments on commit 295de28

Please sign in to comment.