-
Notifications
You must be signed in to change notification settings - Fork 998
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes #25809 - JWT auth for external users #6549
Conversation
Issues: #25809 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rahulbajaj0509: I checked out the code. I still believe you should not hook into the SSO::JWT
workflow as this is unrelated to what you want to achieve. It both uses JWTs, that's the only similarity.
You do need money to buy a tree or a space station. But both a tree and a space station are totally unrelated. The JWT is just the technique used to transfer payload. I'd suggest to create a separate SSO implementation for Open Id Connect and try to use a gem that offers a proper implementation.
reset_session | ||
session[:user] = user.id | ||
session[:api_authenticated_session] = true | ||
if jwt_data[:sso_method] == "SSO::Jwt" | ||
add_jwt_sesssion_values(jwt_data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about session.merge!(jwt_data)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this store data in the session that was read from the session a couple of lines before? 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should have, but since we have the reset_session
on line no.79, it resets all the values and therefore I had to use this method to restore the value again.
@@ -26,7 +26,9 @@ def update_activity_time | |||
end | |||
|
|||
def set_activity_time | |||
session[:expires_at] = Setting[:idle_timeout].minutes.from_now.to_i | |||
unless session[:sso_method] == 'SSO::Jwt' | |||
session[:expires_at] = Setting[:idle_timeout].minutes.from_now.to_i |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we ask the SSO if it sets the expiry time instead of hardcoding stuff for JWT here?
def set_activity_time
return if available_sso.sets_expiry_time?
session[:expires_at] = Setting[:idle_timeout].minutes.from_now.to_i
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think I can achieve this, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timogoebel I tried using available_sso.sets_expiry_time?
.
It works fine when we first authenticate the user.
However, when we use sessions in subsequent calls the method fails.
That is if I run hammer auth login basic --username admin --password changeme
it works correctly.
Immediately after when I run hammer os list
(which will use session since we are already autheticated in the previous command), it fails because all the SSO methods (Basic, JWT, openIDConnect etc) return a false for {SSO::Method}.available?
..
Hence we get available_sso
value as nil
when using session
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to use get_sso_method
to get the SSO object stored in the session.
@timogoebel let me explain a bit about what I am trying to achieve here, let me know if I am thinking in the correct direction here. So at the hammer-cli-foreman side, I authenticate the user by keycloak using the OAuth out of bound implementation. In return to that, the Keycloak returns me with a JWT that has the authenticated users authorization information which I send to Foreman(here) and create a user in Foreman using that JWT. This is the flow that I am trying to achieve. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rahulbajaj0509 could you please point us to respective part that actually issues the JWT token? I'm a bit confused here about the whole flow. I suppose something creates the JWT which is added to requests to Foreman. If that's the case, we download the public key from some URL and decode the token (which does the token signature verification) and then read information out of that token.
app/services/jwt_token_validate.rb
Outdated
|
||
def generate_jwks_uri | ||
response = RestClient::Request.execute( | ||
:url => "https://ssoo1.usersys.redhat.com/auth/realms/hammer-cli/protocol/openid-connect/certs", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this url? it does not resolve, IIUC we download the keycloak public key from here and then use it for signature verification, should this URL be part of JWT token?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added this to the settings, this URL is different for each user, so it may differ. To obtain the URL user can read the description of the field as seen here
app/services/jwt_token_validate.rb
Outdated
:url => "https://ssoo1.usersys.redhat.com/auth/realms/hammer-cli/protocol/openid-connect/certs", | ||
:method => :get, | ||
:headers => { content_type: 'application/x-www-form-urlencoded'}, | ||
:verify_ssl => false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not a good idea, we need to trust the side that provides the public key, otherwise an attacker could give us wrong public key for verification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ares thanks, makes sense, I have change the setting to :verify_ssl to true.
app/services/jwt_token_validate.rb
Outdated
key.set_key(base64_to_long(modulus), base64_to_long(exponent), nil) | ||
end | ||
|
||
def base64_to_long(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this code taken from some library? if not, can you describe what's going on in here, e.g. what format we're trying to build here, we don't use unpack('C*')
every day :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it was but as @tbrisker mentioned, it is not good to handle the encryption by ourself, so I am using the JWT gem to do all this for us.
app/services/jwt_token_validate.rb
Outdated
end | ||
|
||
def to_hex(int) | ||
int < 16 ? '0' + int.to_s(16) : int.to_s(16) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.to_s(16).rjust(2, '0')
is more straightforward
app/services/jwt_token.rb
Outdated
return nil if secret.blank? | ||
|
||
payload = JWT.decode(token, secret.token) | ||
if secret.blank? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a better way to detect what type of token this is? Can we get that information out of decoded_payload
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should never rely on information in decoded_payload
as it hasn't been verified (the signature wasn't validated).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We verify the signature later, but we need to decide what is the purpose of this token here. We can decode and if that's for SSO, we proceed with verification. If it's not and we just decode like we did before.
I don't like this too much, hence I'm asking for a better way of differentiation. Is it possible to use other header or that breaks jwt conventions? Can we somehow detect the signature is present, meaning this is SSO token? If we detect there's signature, then perform verification (which JWT.decode
does for us if we pass public key I believe)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use the Auth-Header. The RFC states to use two parts:
TYPE SECRET
. The type can be an arbitrary string, e.g. Basic
. I would suggest using Bearer
for OIDC and JWT
for JWT's issued by Foreman.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@ares thanks for reviewing the pull request :) We receive the token from the hammer-cli-foreman which can be seen here. Hammer uses the rest-client gem to authenticate the user from Keycloak and if the authentication is successful it receives a JWT token in return. Now we send this JWT here in Foreman and then create a user with external login. When we send the JWT token in Foreman, we need to validate it and make sure it the same token as we had sent from Hammer. There are two ways of validating a JWT token on the foreman side: Method 1: We need to verify a few claims and check the signature:
Method 2: we can also invoke the token introspection endpoint on Keycloak, but that's then a remote call to Keycloak to validate the token.
|
This comment has been minimized.
This comment has been minimized.
app/models/setting/auth.rb
Outdated
@@ -26,6 +26,10 @@ def self.default_settings | |||
self.set('idle_timeout', N_("Log out idle users after a certain number of minutes"), 60, N_('Idle timeout')), | |||
self.set('bcrypt_cost', N_("Cost value of bcrypt password hash function for internal auth-sources (4-30). Higher value is safer but verification is slower particularly for stateless API calls and UI logins. Password change needed to take effect."), 4, N_('BCrypt password cost')), | |||
self.set('bmc_credentials_accessible', N_("Permits access to BMC interface passwords through ENC YAML output and in templates"), true, N_('BMC credentials access')), | |||
self.set('oidc_jwks_url', N_("OpenID Connect JSON Web Key Set(JWKS) URL. For example if you are using Keycloak this url(https://<your keycloak server>/auth/realms/<realm name>/protocol/openid-connect/certs) would be found as `jwk_uri` at https://<your keycloak server>/auth/realms/<realm name>/.well-known/openid-configuration."), nil, N_('OIDC JWKs URL')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the whole idea behind .well-known
that software can rely on it and humans never need to care? As a user I don't know which of the two I should enter. If only the well-known one should be specified, I'd write something like:
self.set('oidc_jwks_url', N_("OpenID Connect JSON Web Key Set(JWKS) URL. For example if you are using Keycloak this url(https://<your keycloak server>/auth/realms/<realm name>/protocol/openid-connect/certs) would be found as `jwk_uri` at https://<your keycloak server>/auth/realms/<realm name>/.well-known/openid-configuration."), nil, N_('OIDC JWKs URL')), | |
self.set('oidc_jwks_url', N_("OpenID Connect JSON Web Key Set(JWKS) URL. Typically https://keycloak.example.com/auth/realms/<realm name>/.well-known/openid-configuration."), nil, N_('OIDC JWKs URL')), |
app/models/setting/auth.rb
Outdated
@@ -26,6 +26,10 @@ def self.default_settings | |||
self.set('idle_timeout', N_("Log out idle users after a certain number of minutes"), 60, N_('Idle timeout')), | |||
self.set('bcrypt_cost', N_("Cost value of bcrypt password hash function for internal auth-sources (4-30). Higher value is safer but verification is slower particularly for stateless API calls and UI logins. Password change needed to take effect."), 4, N_('BCrypt password cost')), | |||
self.set('bmc_credentials_accessible', N_("Permits access to BMC interface passwords through ENC YAML output and in templates"), true, N_('BMC credentials access')), | |||
self.set('oidc_jwks_url', N_("OpenID Connect JSON Web Key Set(JWKS) URL. For example if you are using Keycloak this url(https://<your keycloak server>/auth/realms/<realm name>/protocol/openid-connect/certs) would be found as `jwk_uri` at https://<your keycloak server>/auth/realms/<realm name>/.well-known/openid-configuration."), nil, N_('OIDC JWKs URL')), | |||
self.set('oidc_audience', N_("Name of the OpenID Connect Audience that is being used for Authentication. For example in case of Keycloak this is the Client ID."), nil, N_('OIDC Audience')), | |||
self.set('oidc_issuer', N_("The iss(issuer) claim identifies the principal that issued the JWT, which exists at a `/.well-known/openid-configuration` in case of most of the IDP's."), nil, N_('OIDC Issuer')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is iss(issuer)
a typo? Maybe IDP should also be written out since most users don't know what it stands for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iss(issuer) is a very term in the JWT world. It is one of the most important claims and this line is directly taken from the spec(https://tools.ietf.org/html/rfc7519#section-4.1.1). I would like to stick to it, if you don't mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a space in between like the RFC did.
app/models/setting/auth.rb
Outdated
self.set('oidc_jwks_url', N_("OpenID Connect JSON Web Key Set(JWKS) URL. For example if you are using Keycloak this url(https://<your keycloak server>/auth/realms/<realm name>/protocol/openid-connect/certs) would be found as `jwk_uri` at https://<your keycloak server>/auth/realms/<realm name>/.well-known/openid-configuration."), nil, N_('OIDC JWKs URL')), | ||
self.set('oidc_audience', N_("Name of the OpenID Connect Audience that is being used for Authentication. For example in case of Keycloak this is the Client ID."), nil, N_('OIDC Audience')), | ||
self.set('oidc_issuer', N_("The iss(issuer) claim identifies the principal that issued the JWT, which exists at a `/.well-known/openid-configuration` in case of most of the IDP's."), nil, N_('OIDC Issuer')), | ||
self.set('oidc_algorithm', N_("The algorithm with which JWT was encoded in the IDP."), nil, N_('OIDC Algorithm')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.set('oidc_algorithm', N_("The algorithm with which JWT was encoded in the IDP."), nil, N_('OIDC Algorithm')), | |
self.set('oidc_algorithm', N_("The algorithm used to encode the JWT in the IDP."), nil, N_('OIDC Algorithm')), |
app/services/oidc_jwt_validate.rb
Outdated
if json_response.is_a?(Hash) | ||
jwks_keys = json_response['keys'] | ||
@cached_keys = nil if options[:invalidate] # need to reload the keys | ||
@cached_keys ||= { keys: jwks_keys.map(&:symbolize_keys) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are cached keys, does that mean you don't need to fetch a response in the first place and the method could return early?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @tbrisker explained here: #6549 (comment).
test/unit/oidc_jwt_validate_test.rb
Outdated
context '#decoded_payload?' do | ||
def setup | ||
skip "SSO feature is not available for Ruby < 2.4.0" unless RUBY_VERSION >= '2.4' | ||
@jwk = JWT::JWK.new(OpenSSL::PKey::RSA.new(2048)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is setup
ran for every test
or only for the class? The test would be a lot faster if you could reuse the key.
app/services/oidc_jwt_validate.rb
Outdated
jwks_keys = json_response['keys'] | ||
{ keys: jwks_keys.map(&:symbolize_keys) } | ||
else | ||
raise JSON::ParserError.new('Invalid JWKS response') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of raising here you can log and return an empty hash (but still keep the rescue below in case JSON.parse fails)
test/fixtures/settings.yml
Outdated
attribute88: | ||
name: oidc_jwks_url | ||
category: Setting::Auth | ||
default: 127.0.0.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This default is very different from the description since it's not a URL. If there is no sane default, should it be empty?
test/fixtures/settings.yml
Outdated
name: oidc_audience | ||
category: Setting::Auth | ||
default: 'rest-client' | ||
description: 'Name of the OpenID Connect Audience that is being used for Authentication. For exmaple in case of Keycloak this is the Client ID.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description: 'Name of the OpenID Connect Audience that is being used for Authentication. For exmaple in case of Keycloak this is the Client ID.' | |
description: 'Name of the OpenID Connect Audience that is being used for Authentication. For example in case of Keycloak this is the Client ID.' |
test/fixtures/settings.yml
Outdated
name: oidc_jwks_url | ||
category: Setting::Auth | ||
default: 127.0.0.1 | ||
description: 'OpenID Connect JSON Web Key Set(JWKS) URL. For example if you are using Keycloak this url(https://<your keycloak server>/auth/realms/<realm name>/protocol/openid-connect/certs) would be found as `jwk_uri` at https://<your keycloak server>/auth/realms/<realm name>/.well-known/openid-configuration.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be synced to the Ruby code?
e7cc972
to
edb22f2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me. I'm good with merging this.
@tbrisker: Feel free to merge at your own discretion. Note, we still need the packaging to be done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as well, pending packaging. Great job @rahulbajaj0509 on pushing this through the very long process!
@ekohl - unless you have any other comments feel free to merge once the packaging side is ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall 👍 besides some small textual nits
app/models/setting/auth.rb
Outdated
@@ -26,6 +26,10 @@ def self.default_settings | |||
self.set('idle_timeout', N_("Log out idle users after a certain number of minutes"), 60, N_('Idle timeout')), | |||
self.set('bcrypt_cost', N_("Cost value of bcrypt password hash function for internal auth-sources (4-30). Higher value is safer but verification is slower particularly for stateless API calls and UI logins. Password change needed to take effect."), 4, N_('BCrypt password cost')), | |||
self.set('bmc_credentials_accessible', N_("Permits access to BMC interface passwords through ENC YAML output and in templates"), true, N_('BMC credentials access')), | |||
self.set('oidc_jwks_url', N_("OpenID Connect JSON Web Key Set(JWKS) URL. Typically https://keycloak.example.com/auth/realms/<realm name>/protocol/openid-connect/certs if you are using Keycloak as an IDP"), nil, N_('OIDC JWKs URL')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I think it's possible to avoid the personal pronoun which generally the preferred style. Maybe a native speaker has a better suggestion
self.set('oidc_jwks_url', N_("OpenID Connect JSON Web Key Set(JWKS) URL. Typically https://keycloak.example.com/auth/realms/<realm name>/protocol/openid-connect/certs if you are using Keycloak as an IDP"), nil, N_('OIDC JWKs URL')), | |
self.set('oidc_jwks_url', N_("OpenID Connect JSON Web Key Set(JWKS) URL. Typically https://keycloak.example.com/auth/realms/<realm name>/protocol/openid-connect/certs when using Keycloak as an IDP"), nil, N_('OIDC JWKs URL')), |
app/models/setting/auth.rb
Outdated
@@ -26,6 +26,10 @@ def self.default_settings | |||
self.set('idle_timeout', N_("Log out idle users after a certain number of minutes"), 60, N_('Idle timeout')), | |||
self.set('bcrypt_cost', N_("Cost value of bcrypt password hash function for internal auth-sources (4-30). Higher value is safer but verification is slower particularly for stateless API calls and UI logins. Password change needed to take effect."), 4, N_('BCrypt password cost')), | |||
self.set('bmc_credentials_accessible', N_("Permits access to BMC interface passwords through ENC YAML output and in templates"), true, N_('BMC credentials access')), | |||
self.set('oidc_jwks_url', N_("OpenID Connect JSON Web Key Set(JWKS) URL. Typically https://keycloak.example.com/auth/realms/<realm name>/protocol/openid-connect/certs if you are using Keycloak as an IDP"), nil, N_('OIDC JWKs URL')), | |||
self.set('oidc_audience', N_("Name of the OpenID Connect Audience that is being used for Authentication. For example in case of Keycloak this is the Client ID."), nil, N_('OIDC Audience')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: phrasing could be a bit more compact. For example and in case are redundant.
self.set('oidc_audience', N_("Name of the OpenID Connect Audience that is being used for Authentication. For example in case of Keycloak this is the Client ID."), nil, N_('OIDC Audience')), | |
self.set('oidc_audience', N_("Name of the OpenID Connect Audience used for Authentication. In case of Keycloak this is the Client ID."), nil, N_('OIDC Audience')), |
test/unit/oidc_jwt_validate_test.rb
Outdated
assert_nil expected, actual | ||
end | ||
|
||
test 'if audiance is not valid' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test 'if audiance is not valid' do | |
test 'if audience is not valid' do |
4cf928a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 from a packaging perspective.
|
||
private | ||
|
||
def jwt_token |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still like to avoid this naming (https://en.wikipedia.org/wiki/RAS_syndrome) but I won't block on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand but we're already using that in multiple places and it seems pretty common in general (e.g. https://www.google.com/search?q=%22jwt+token%22).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rahulbajaj0509 and @timogoebel, @ekohl and everyone else who took part in reviewing!
Please add this in the headline features for 1.24 and where it makes sense in the manual (e.g. after https://theforeman.org/manuals/nightly/index.html#5.1.9UsingOAuth or https://theforeman.org/manuals/nightly/index.html#5.7ExternalAuthentication) |
Thanks a lot everyone! I think I made many mistakes and without your help I would have never learnt so many things. Few points I want to add:
|
Building a feature that touches the system at a deep level does expose you to a lot of details :) |
This pr enables Foreman to act as a consumer of the JWT token. It makes sure that an external user, authenticated via a third party application(in my case keycloak), can be created using the JWT token(without the need of a password).