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 #32352 - use mod_auth_gssapi instead of mod_auth_kerb #967

Merged
merged 1 commit into from Jul 6, 2021

Conversation

evgeni
Copy link
Member

@evgeni evgeni commented Jul 5, 2021

No description provided.

manifests/config.pp Outdated Show resolved Hide resolved
AuthType GSSAPI
AuthName "GSSAPI Single Sign On Login"
GssapiCredStore keytab:<%= scope.lookupvar('::foreman::http_keytab') %>
GssapiSSLonly On
Copy link
Member Author

Choose a reason for hiding this comment

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

this is not the default 🙄

probably also want to set

GssapiUseSessions …
GssapiAllowedMech krb5

Copy link
Member Author

Choose a reason for hiding this comment

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

After thinking, GssapiUseSessions is not really much of a concern, as we only hit kerberos when the user visits /extlogin, not on every page load.

@evgeni evgeni force-pushed the issue32352 branch 2 times, most recently from 91520f2 to 84b5b8c Compare July 5, 2021 14:20
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.

I really should find time to set up some FreeIPA server and auth against it.

Comment on lines 172 to 174
foreman::config::apache::fragment { 'auth_gssapi':
ssl_content => template('foreman/auth_gssapi.conf.erb'),
}
Copy link
Member

Choose a reason for hiding this comment

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

Rather than including a fragment, should we use a directory entry? It was added in puppetlabs/puppetlabs-apache@891fdaf which means we should also raise the minimum version to 5.7.0 in metadata.json, but that's ok IMHO.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe. Probably.

Right now it's just copy paste to see if it even works :D

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now, I'd prefer to skip this, as that would require more changes: when you specify directories, you also need to pass all other directories you want to configure, like the one for the docroot, as specifying it makes the defaults be dropped.

templates/auth_gssapi.conf.erb Outdated Show resolved Hide resolved
@evgeni evgeni changed the title Fixes #32352 - use mod_gssapi on EL8+ Fixes #32352 - use mod_auth_gssapi instead of mod_auth_kerb Jul 6, 2021
AuthName "Kerberos Login"
KrbMethodNegotiate On
KrbMethodK5Passwd Off
KrbAuthRealms <%= @facts['foreman_ipa']['default_realm'] %>
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 couldn't find an equivalent in the mod_auth_gssapi config 🤷‍♀️

@evgeni evgeni marked this pull request as ready for review July 6, 2021 13:25
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.

Could you change this bit to no longer require default_realm?

unless fact('foreman_ipa.default_server') and fact('foreman_ipa.default_realm') {
fail("${facts['networking']['hostname']}: The system does not seem to be IPA-enrolled")
}

@ekohl ekohl merged commit 91850a2 into theforeman:master Jul 6, 2021
@evgeni evgeni deleted the issue32352 branch July 6, 2021 15:02
@evgeni
Copy link
Member Author

evgeni commented Jul 6, 2021

Should we call that out in the release notes explicitly?

CC @tbrisker

@tbrisker
Copy link
Member

tbrisker commented Jul 6, 2021

@evgeni makes sense. we should also probably update the docs for configuring it. currently it's at https://theforeman.org/manuals/nightly/index.html#5.7ExternalAuthentication, but this might be a good opportunity to actually drop at least part of that section and point it to https://docs.theforeman.org/nightly/Administering_Red_Hat_Satellite/index-foreman-el.html#sect-Administering-Configuring_External_Authentication-Using_Identity_Management with the new instructions.

@evgeni
Copy link
Member Author

evgeni commented Jul 6, 2021

Ack, will put on my todo

@evgeni
Copy link
Member Author

evgeni commented Jul 7, 2021

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

Successfully merging this pull request may close these issues.

None yet

4 participants