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 #25848 - Adding SSO func. through CLI using OpenID Connect. #405

Merged
merged 1 commit into from Oct 25, 2019

Conversation

@rahulbajaj0509
Copy link
Member

rahulbajaj0509 commented Jan 14, 2019

My attempt is to use keycloak for authenticating the user
and fetching the token, once we have the token we can send this
token to the foreman api, where we can use the JWT gem to
validate the token and then create a session for the user.

In this pr, I have attempted to fetch the token from keycloak using
the rest-client gem. This token has to be passed to foreman where
it will be validated and accordingly authentication would be a success/failure.

@authenticator = InteractiveBasicAuth.new(username, password)
end
@authenticator
@authenticator = InteractiveTokenAuth.new(settings.get(:_params, :token) || ENV['KEYCLOAK_TOKEN'])

This comment has been minimized.

Copy link
@rahulbajaj0509

rahulbajaj0509 Feb 4, 2019

Author Member

@mbacovsky I am trying to create an object of the class InteractiveTokenAuth but i am failing in doing so. Can you please take a look?

This comment has been minimized.

Copy link
@rahulbajaj0509

rahulbajaj0509 Feb 4, 2019

Author Member

The whole idea is to set the @token in the @authenticator so that it can be passed to foreman api.

@rahulbajaj0509

This comment has been minimized.

Copy link
Member Author

rahulbajaj0509 commented Feb 4, 2019

@mbacovsky you need to use the command hammer -d auth login --url "<your keycloak server url>" --realm "<your realm used in keycloak>" --username "<username>" --password "<password>"

For username and password, you will have to create a username and password on keycloak in a defined realm. Let me know if you need any help with the setup :)

@rahulbajaj0509 rahulbajaj0509 force-pushed the rahulbajaj0509:keycloak branch from 54f90a7 to 7a93083 Feb 12, 2019
@rahulbajaj0509

This comment has been minimized.

Copy link
Member Author

rahulbajaj0509 commented Feb 12, 2019

@mbacovsky after Apipie/apipie-bindings#77, I guess except the clean up required, the pr is ready for testing. It works for me, let me know if you are facing any issues while testing it.

Ah! right now it takes the expiration time of the session from foreman settings, I need to fix that!

@rahulbajaj0509 rahulbajaj0509 force-pushed the rahulbajaj0509:keycloak branch 4 times, most recently from 641bda7 to 347bf39 Mar 6, 2019
require 'hammer_cli_foreman/api/void_auth'

module HammerCLIForeman
module Api
class Connection < HammerCLI::Apipie::ApiConnection
attr_reader :authenticator

def initialize(settings, logger = nil, locale = nil)
default_params = build_default_params(settings, logger, locale)
def initialize(settings, logger = nil, locale = nil, auth_type = AuthTypes[:basic_auth])

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Mar 14, 2019

Member

Can we use constants instead of the hash for auth_type accross the whole PR? Something like AUTH_TYPE_BASIC, AUTH_TYPE_OAUTH_BASIC, AUTH_TYPE_OAUTH_2FA would be more readable.

@@ -66,7 +66,7 @@ def self.record_to_common_format(data)
class Command < HammerCLI::Apipie::Command

def self.connection_name(resource_class)
CONNECTION_NAME
HammerCLIForeman.get_connection_name

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Mar 14, 2019

Member

Why do we have different connection names for different auth methods? This name should refer to which API we are connecting and should be immutable for a resource.

This comment has been minimized.

Copy link
@rahulbajaj0509

rahulbajaj0509 Mar 14, 2019

Author Member

We observed that HammerCLIForeman.foreman_api_connection is being called multiple times when we run any of the login commands.

  def self.foreman_api_connection
    HammerCLI.context[:api_connection].create(CONNECTION_NAME) do
      HammerCLIForeman::Api::Connection.new(HammerCLI::Settings, Logging.logger['API'], HammerCLI::I18n.locale)
    end
  end

If we look at this method, it yields a block only if a previous connection with name {CONNECTION_NAME} (foreman) has not been called already.
From HammerCli::Connection.rb:

def create(name, &create_connector_block)
      unless connections[name]
        connector = yield
        @logger.debug("Registered: #{name}") if @logger
        connections[name] = connector
      end
      connections[name]
end

This worked fine in our initial flow since there was only a single type of authentication flow available (ie basic auth).

When we introduced more authentication ways, and called HammerCLIForeman.foreman_api_connection from our auth.rb file, for other authentication ways, the block was never yielded, because the first time HammerCLIForeman.foreman_api_connection was called, the connection (with basic_auth) was already stored with FOREMAN key.

Hence we did these changes, so that the block is yielded, when we use any other authentiction way except basic_auth.

I will be glad to explain this in more detail, along with the flow as required.

Copy link
Member

mbacovsky left a comment

@rahulbajaj0509 thanks for all the effort on this PR(s). Overall the code looks good and works fine. I'm only concerned about how the new connections are created and a few minor things. I also have a few usability improvements. We discussed that already so I'll put it here for the record. More details inline. My concerns are:

  • we shouldn't change the connection name but rather replace the existing one
  • foreman_api_connection shouldn't have any parameters
  • default auth_type should be configurable in settings
  • session could hold the auth_type for proper resume when it expires
    • can we use the refresh token for refresh with the information?
    • will require change in how the expiration is handled (remove session_id from session vs. do re-login before hammer reload)
  • consider making the sso CA configurable
  • execute_with_params to a module
  • better cli command names
    If there is anything unclear or you find any blockers, let me know.
def self.foreman_api_connection
HammerCLI.context[:api_connection].create(CONNECTION_NAME) do
HammerCLIForeman::Api::Connection.new(HammerCLI::Settings, Logging.logger['API'], HammerCLI::I18n.locale)
def self.foreman_api_connection(auth_type = AuthTypes[:basic_auth])

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Mar 15, 2019

Member

This method should get a connection or create one. The caller should not know anything about the auth_type.
In Login command we should clear the connection and create new one of proper type when we know the type.

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Mar 15, 2019

Member

We could add self.foreman_api_reconnect that will drop the connection and create new one of desired auth_type

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Mar 15, 2019

Member

The connection drop could be achieved by HammerCLI.context[:api_connection].drop(CONNECTION_NAME)

HammerCLI.context[:api_connection].create(CONNECTION_NAME) do
HammerCLIForeman::Api::Connection.new(HammerCLI::Settings, Logging.logger['API'], HammerCLI::I18n.locale)
def self.foreman_api_connection(auth_type = AuthTypes[:basic_auth])
connection = get_connection_name(auth_type)

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Mar 15, 2019

Member

No reason to rename the connection, we are still connecting to the Foreman API.

lib/hammer_cli_foreman/api/connection.rb Show resolved Hide resolved
lib/hammer_cli_foreman/auth.rb Outdated Show resolved Hide resolved
def execute
if !(HammerCLIForeman.foreman_api_connection.authenticator.is_a?(HammerCLIForeman::Api::SessionAuthenticatorWrapper))
print_message(_("Can't perform login. Make sure sessions are enabled in hammer configuration file."))
def self.execute_with_params(auth_type, *args)

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Mar 15, 2019

Member

This method should be in some module as we should not hijack the Login subcommand for that

if !(HammerCLIForeman.foreman_api_connection.authenticator.is_a?(HammerCLIForeman::Api::SessionAuthenticatorWrapper))
print_message(_("Can't perform login. Make sure sessions are enabled in hammer configuration file."))
def self.execute_with_params(auth_type, *args)
connection = HammerCLIForeman.foreman_api_connection(auth_type)

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Mar 15, 2019

Member

We should call foreman_api_reconnect(auth_type)

@rahulbajaj0509

This comment has been minimized.

Copy link
Member Author

rahulbajaj0509 commented Mar 27, 2019

Remaining tasks for this pr are:


  • making the SSL CA configurable.
  • convert execute_with_params method to a module.
  • refactor the create_authenticator method.
  • use net/http or restclient throughout.
  • error handling
  • update test cases for auth.rb and connection.rb
  • test cases - keycloak.rb, interactive_keycloak_auth.rb, interactive_keycloak_auth_via_2fa.rb.

As pointed out by @mbacovsky, few more tasks:


  • :default_auth_type gives NoMethodError (undefined method to_sym' for nil:NilClass) if nothing is set in settings, need to add a default value.
  • hammer auth status gives 'Using sessions, you are currently not logged in.' all the time even when logged in.
  • hammer auth logout prints 'Session exists, currently logged in as ''.' whereas that should not be the case.

These tasks are the total tasks remaining for this pr, I will tick each of them once the task is complete.

def initialize(settings, logger = nil, locale = nil)
default_params = build_default_params(settings, logger, locale)
def initialize(settings, logger = nil, locale = nil, auth_type =
AUTH_TYPES[HammerCLI::Settings.get(:foreman, :default_auth_type).to_sym])

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Apr 8, 2019

Member

if the settings does not contain the :default_auth_type I get
NoMethodError (undefined method to_sym' for nil:NilClass)`. We will need some default value here.

This comment has been minimized.

Copy link
@rahulbajaj0509

rahulbajaj0509 Apr 9, 2019

Author Member

Yeah, it is a known issue. I figured that out :) Need to fix this!

@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Apr 8, 2019

hammer auth status gives me
'Using sessions, you are currently not logged in.' all the time even when logged in.

@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Apr 8, 2019

After hammer auth logout I'm logged out but auth status gives me 'Session exists, currently logged in as ''.'

@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Apr 8, 2019

After some more login/logout excersises I'm getting correct output from auth status when logged in. Needs mode investigation.

if Dir[session_repo+ "/*"].empty?
nil
else
Dir.entries(session_repo)[-1]

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Apr 8, 2019

Member

This is not safe for a couple of reasons. I have multiple session files for multiple foremans I manage in my session dir. We should pick the correct one by url, otherwise we can end up reading wrong file.
Also while this is not predictably sorted list I ended up with '..' on the last position resulting in ugly errors when reading the session.

I wonder if it is time to represent the session file and operations with some class and have the usage unified across the codebase.
Would something with similar interface make sense?:

session = HammerCLIForeman::Sessions.load(url) # load or create new session
expired = session.expired?
session.expire!
session.username
session.username = username
session.auth_type
session.auth_type = auth_type
session.save
session.id
session.id =  id

This comment has been minimized.

Copy link
@rahulbajaj0509

rahulbajaj0509 Apr 9, 2019

Author Member

Yeah! completely makes sense. I was actually assuming that a user will have only one session file and one foreman instance to manage. Thanks for bringing this up :)

Your suggestion about making a new class that takes care of the session file and operation completely makes sense.

@rahulbajaj0509 rahulbajaj0509 force-pushed the rahulbajaj0509:keycloak branch from ffddaec to f00085d Jun 11, 2019
@ntkathole

This comment has been minimized.

Copy link

ntkathole commented Jun 12, 2019

@rahulbajaj0509 Facing following issues:
1.

$ hammer auth status
Error: undefined method `status' for nil:NilClass
$ hammer -u admin -p changeme host list
Error: ApipieBindings::AuthenticatorMissingError
hammer auth login oauth
URL:
Realm:
Client ID:
Username:
Password:
Error: undefined method `request_uri' for #<URI::Generic /auth/realms//protocol/openid-connect/token>
@rahulbajaj0509 rahulbajaj0509 force-pushed the rahulbajaj0509:keycloak branch 5 times, most recently from f243aa7 to 98759f5 Oct 25, 2019
@rahulbajaj0509 rahulbajaj0509 changed the title [WIP]Fixes #25848 - Adding SSO func. through CLI using keycloak Fixes #25848 - Adding SSO func. through CLI using OpenID Connect. Oct 25, 2019
Copy link
Member

ofedoren left a comment

I was able to login/out with both basic and oauth. Just a small nitpick (see inline).

config/foreman.yml Outdated Show resolved Hide resolved
@ofedoren

This comment has been minimized.

Copy link
Member

ofedoren commented Oct 25, 2019

Just one more thing. Is it okay that if user uses two factor auth and if wants to check status via hammer auth status it will ask him the code again?..

@rahulbajaj0509 rahulbajaj0509 force-pushed the rahulbajaj0509:keycloak branch from 98759f5 to 458ac58 Oct 25, 2019

def error(ex)
if ex.is_a?(RestClient::InternalServerError)
self.clear

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Oct 25, 2019

Member

When logging in with invalid password I get
[ERROR 2019-10-25T15:37:22 Exception] Error: undefined method clear' for #<HammerCLIForeman::Api::Oauth::PasswordGrant:0x0000000002fb0098> Error: undefined method clear' for #HammerCLIForeman::Api::Oauth::PasswordGrant:0x0000000002fb0098

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Oct 25, 2019

Member

The clear should probably remove the values from the object

This comment has been minimized.

Copy link
@ofedoren

ofedoren Oct 25, 2019

Member

And that's why we need two reviewers... When I looked at the code I didn't notice that there is no method clear for AuthenticationCodeGrant and PasswordGrant objects.

@rahulbajaj0509, is it a leftover or you forget to define those methods for PasswordGrant and AuthenticationCodeGrant objects? This method exists only in InteractiveBasicAuth.

Copy link
Member

ofedoren left a comment

Please see my above comment (if this is a problem we could fix it later).

Otherwise I have no other required changes for code, so you have finally my approve. Outstanding work! :)

Athough I wanted @mbacovsky to be agreed on everything and push the big green button after he is ready :)

@rahulbajaj0509

This comment has been minimized.

Copy link
Member Author

rahulbajaj0509 commented Oct 25, 2019

@ofedoren it will ask for code only if the session has expired.

@rahulbajaj0509 rahulbajaj0509 force-pushed the rahulbajaj0509:keycloak branch 2 times, most recently from 6072401 to 6e88224 Oct 25, 2019
@rahulbajaj0509 rahulbajaj0509 force-pushed the rahulbajaj0509:keycloak branch from 6e88224 to 2a7d41e Oct 25, 2019
Copy link
Member

mbacovsky left a comment

I'm adding my 👍 finally. I did some more testing and didn't found any breaking changes. I also tested various combinations of parms on CLI and in settings and everything works as expected.
The error messages now look good too and provide useful information to the user.

@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Oct 25, 2019

@rahulbajaj0509 thanks for all the amazing effort you've put into this feature!
@ofedoren thanks for testing and helpful reviews!

@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Oct 25, 2019

🍻 🎆 🍰 💯 comments

@mbacovsky mbacovsky merged commit 57c894a into theforeman:master Oct 25, 2019
2 checks passed
2 checks passed
Redmine issues Valid issues
Details
hammer Build finished. 5454 tests run, 0 skipped, 0 failed.
Details
@rahulbajaj0509

This comment has been minimized.

Copy link
Member Author

rahulbajaj0509 commented Oct 26, 2019

Thanks a lot for your support @mbacovsky. I was stuck so many times when thinking about foreman_ap_reconnect, sessions, getting the correct flows in order and it was all possible because of your constant push.

I made so many mistakes and thanks for catching them all @ofedoren! :P thanks for testing the PR again and again. I know it can be really tedious to test a PR of this size repeatedly.

Thanks a lot @mbacovsky @ofedoren for your valuable inputs, without you folks I wouldn't have made it 😄

@ntkathole

This comment has been minimized.

Copy link

ntkathole commented Oct 26, 2019

@rahulbajaj0509 @mbacovsky

  1. I have sessions true in hammer config and

$ hammer host list
Warning: An error occured while loading module hammer_cli_foreman_remote_execution.
[Foreman] Username: admin
[Foreman] Password for admin: 
undefined local variable or method `logger' for #<Foreman::BruteforceProtection:0x000000000c89ba28>

Traceback - https://paste.fedoraproject.org/paste/NDIeSMgm-uYi9QkOT0rCsw

I am not sure this is related to PR but, how can I resolve it?

$ hammer auth login oauth
Warning: An error occured while loading module hammer_cli_foreman_remote_execution.
Openidc Provider Token Endpoint: 
Client ID: 
Username: 
Password: 
Error: undefined method `request_uri' for #<URI::Generic >
option ["-a", "--oidc-authorization-endpoint"], "OPENIDC-AUTHORIZATION-ENDPOINT", _("Openidc provider URL which issues authentication code")
option ["-c", "--oidc-client-id"], "OPENIDC-CLIENT-ID", _("Client id used in the Openidc provider")
option ["-f", "--two-factor"], :flag, _("Authenticate with two factor")
option ["-r", "--oidc-redirect-uri"], "OPENIDC-REDIRECT-URI", _("Redirect URI for the authencation code grant flow")

This comment has been minimized.

Copy link
@ntkathole

ntkathole Oct 26, 2019

typo s/authencation/authentication

This comment has been minimized.

Copy link
@rahulbajaj0509

rahulbajaj0509 Oct 26, 2019

Author Member

good catch!

@rahulbajaj0509

This comment has been minimized.

Copy link
Member Author

rahulbajaj0509 commented Oct 26, 2019

@ntkathole thanks for having a look 👍

@rahulbajaj0509 @mbacovsky

1. I have sessions true in hammer config and

$ hammer host list
Warning: An error occured while loading module hammer_cli_foreman_remote_execution.
[Foreman] Username: admin
[Foreman] Password for admin: 
undefined local variable or method `logger' for #<Foreman::BruteforceProtection:0x000000000c89ba28>

This issue is not reproducible in my Foreman instance. Let me help you with this once I am back :)

Traceback - https://paste.fedoraproject.org/paste/NDIeSMgm-uYi9QkOT0rCsw

I am not sure this is related to PR but, how can I resolve it?

$ hammer auth login oauth
Warning: An error occured while loading module hammer_cli_foreman_remote_execution.
Openidc Provider Token Endpoint: 
Client ID: 
Username: 
Password: 
Error: undefined method `request_uri' for #<URI::Generic >

Good catch! I will fix this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.