Skip to content
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

Merged
merged 1 commit into from Oct 10, 2019

Conversation

rabajaj0509
Copy link
Member

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).

@theforeman-bot
Copy link
Member

Issues: #25809

@theforeman-bot

This comment has been minimized.

@theforeman-bot

This comment has been minimized.

Copy link
Member

@timogoebel timogoebel left a 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)
Copy link
Member

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)

Copy link
Member

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? 😕

Copy link
Member Author

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
Copy link
Member

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

Copy link
Member Author

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!

Copy link
Member Author

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

Copy link
Member

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.

app/services/jwt_token.rb Outdated Show resolved Hide resolved
app/services/sso/jwt.rb Outdated Show resolved Hide resolved
@rabajaj0509
Copy link
Member Author

@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.

@theforeman-bot

This comment has been minimized.

app/services/jwt_token_validate.rb Outdated Show resolved Hide resolved
app/services/jwt_token_validate.rb Outdated Show resolved Hide resolved
app/services/jwt_token_validate.rb Outdated Show resolved Hide resolved
app/services/jwt_token_validate.rb Outdated Show resolved Hide resolved
app/services/jwt_token_validate.rb Outdated Show resolved Hide resolved
@theforeman-bot

This comment has been minimized.

ares
ares previously requested changes Mar 18, 2019
Copy link
Member

@ares ares left a 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.


def generate_jwks_uri
response = RestClient::Request.execute(
:url => "https://ssoo1.usersys.redhat.com/auth/realms/hammer-cli/protocol/openid-connect/certs",
Copy link
Member

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?

Copy link
Member Author

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

: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
Copy link
Member

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

Copy link
Member Author

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.

key.set_key(base64_to_long(modulus), base64_to_long(exponent), nil)
end

def base64_to_long(data)
Copy link
Member

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 :-)

Copy link
Member Author

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.

end

def to_hex(int)
int < 16 ? '0' + int.to_s(16) : int.to_s(16)
Copy link
Member

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

return nil if secret.blank?

payload = JWT.decode(token, secret.token)
if secret.blank?
Copy link
Member

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?

Copy link
Member

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).

Copy link
Member

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)

Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@rabajaj0509
Copy link
Member Author

@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:

  • The important bits to check in the token:

    • typ - should be Bearer
    • aud - should include the client_id of the service
    • nbf - should be in the past (check against current time - 1 sec)
    • exp - should be in the future (check against current time + 1 sec)
  • For this, I am using the JWT gem that basically does all the checks
    when we do JWT.decode with the token that we receive from Hammer and public key that we generate using the generate_key_from_jwks method.

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.

  • Basically, this can be done by invoking the introspection endpoint with the token. In return, we will recieve a valid and JSON of the token that can be future matched/validated with our token.

@theforeman-bot

This comment has been minimized.

@@ -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')),
Copy link
Member

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:

Suggested change
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')),

@@ -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')),
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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.

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')),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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')),

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) }
Copy link
Member

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?

Copy link
Member Author

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).

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))
Copy link
Member

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.

test/unit/sso/openid_connect_test.rb Show resolved Hide resolved
jwks_keys = json_response['keys']
{ keys: jwks_keys.map(&:symbolize_keys) }
else
raise JSON::ParserError.new('Invalid JWKS response')
Copy link
Member

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)

app/services/sso/jwt.rb Show resolved Hide resolved
attribute88:
name: oidc_jwks_url
category: Setting::Auth
default: 127.0.0.1
Copy link
Member

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?

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.'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.'

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.'
Copy link
Member

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?

@rabajaj0509 rabajaj0509 force-pushed the jwt-auth branch 2 times, most recently from e7cc972 to edb22f2 Compare October 7, 2019 16:50
timogoebel
timogoebel previously approved these changes Oct 8, 2019
Copy link
Member

@timogoebel timogoebel left a 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.

tbrisker
tbrisker previously approved these changes Oct 8, 2019
Copy link
Member

@tbrisker tbrisker left a 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.

app/services/sso/jwt.rb Show resolved Hide resolved
ekohl
ekohl previously approved these changes Oct 8, 2019
Copy link
Member

@ekohl ekohl left a 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

@@ -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')),
Copy link
Member

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

Suggested change
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')),

@@ -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')),
Copy link
Member

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.

Suggested change
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')),

assert_nil expected, actual
end

test 'if audiance is not valid' do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
test 'if audiance is not valid' do
test 'if audience is not valid' do

Copy link
Member

@ekohl ekohl left a 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
Copy link
Member

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.

Copy link
Member

@tbrisker tbrisker Oct 10, 2019

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).

Copy link
Member

@tbrisker tbrisker left a 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!

@tbrisker tbrisker merged commit 56b625e into theforeman:develop Oct 10, 2019
@tbrisker
Copy link
Member

@rabajaj0509
Copy link
Member Author

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:

  • I was really struggling with the session handling part, I had to read about how sessions actually worked and @timogoebel pointed me out to the code and how to handle that properly.
  • There were many scenarios for test cases which I didn't think about and was not sure how to go about handling the JWT gem and its corresponding version handling. @tbrisker pointed me out to various links which finally helped me understand.
  • I am always in two-minds while selecting names for a method, also while defining descriptions for settings and have never been involved in packaging, so thanks to @ekohl for helping me with that!
  • Thanks to @ntkathole for testing the PR again and again as there were many changes made from time to time.
  • Thanks @gnurag @ares, @kgaikwad, @ofedoren for reviewing as well :)

@ekohl
Copy link
Member

ekohl commented Oct 10, 2019

Building a feature that touches the system at a deep level does expose you to a lot of details :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet