fixes #2429 - Permanent OpenID secrets storage #549
Conversation
|
@ares could you file an issue in redmine for each PR please? New issue in Foreman and then update the commit message to start with |
| @@ -1,6 +1,8 @@ | |||
| begin | |||
| require 'rack/openid' | |||
| Rails.configuration.middleware.use Rack::OpenID | |||
| require 'openid/store/filesystem' | |||
| openid_store_path = Pathname.new(Rails.root).join('db').join('openid-store') | |||
There was a problem hiding this comment.
This may need a corresponding update to the SELinux policy. Might it be better to move it to the tmp/ directory, which we use for caches?
There was a problem hiding this comment.
I'm not sure that this should be viewed as cache. If it gets lost it could cause Signo authentication unusable until someone clears Signo keys manually. But if you think tmp/ is persistent enough, I can change it.
There was a problem hiding this comment.
Ah, in that case tmp/ isn't good enough as on F17 etc it's on tmpfs (it's symlinked to /run, which is).
There was a problem hiding this comment.
Also, make sure the file gets proper permissions!
LZ
On Wed, Apr 24, 2013 at 07:47:26AM -0700, Dominic Cleal wrote:
@@ -1,6 +1,8 @@
begin
require 'rack/openid'
- Rails.configuration.middleware.use Rack::OpenID
- require 'openid/store/filesystem'
- openid_store_path = Pathname.new(Rails.root).join('db').join('openid-store')
Ah, in that case tmp/ isn't good enough as on F17 etc it's on tmpfs (it's symlinked to /run, which is).
Reply to this email directly or view it on GitHub:
https://github.com/theforeman/foreman/pull/549/files#r3938069
Later,
Lukas "lzap" Zapletal
irc: lzap #theforeman
There was a problem hiding this comment.
Why not use the session / db to store this? as saving plain files will not
work when having multiple foreman instances.
There was a problem hiding this comment.
@lzap files are created with 0600 permissions so only application can manipulate with them
@ohadlevy ruby openid library currently provides only memory/memcache/filesystem storage backends, we could make this optional in future to leverage memcache or we could write our own storage backend when needed. Filesystem storage can be shared if needed using nfs or similar solution.
There was a problem hiding this comment.
great, in this case we can use the foreman_memcache plugin to change the
store
There was a problem hiding this comment.
Maybe add to .gitignore too.
|
is there anything I can do to get this PR merged? |
| @@ -76,6 +76,8 @@ def require_login | |||
| elsif available_sso.support_login? | |||
| available_sso.authenticate! | |||
| return | |||
| else | |||
| logger.warn("SSO failed, falling back to login form") | |||
There was a problem hiding this comment.
@ares could you confirm if I'm missing something, but if either Apache or Signo is enabled, will API requests go into this path? It looks like available_sso will be present, which will then cause a warning for every API request. Is it possible to even do an API request with Signo now?
There was a problem hiding this comment.
API requests runs authorize before filter which does an authentication (see https://github.com/ares/foreman/blob/fd6867f8033a7de7e423f8dfc12a79cb2e47a9c9/app/controllers/api/base_controller.rb#L66 and https://github.com/ares/foreman/blob/fd6867f8033a7de7e423f8dfc12a79cb2e47a9c9/lib/api/authorization.rb#L11) and that disables require_login before filter (see here
https://github.com/ares/foreman/blob/fd6867f8033a7de7e423f8dfc12a79cb2e47a9c9/app/controllers/application_controller.rb#L67).
However it's just lucky coincidence and you made a good point, Signo should not be available? when it's an API request. I added second commit fixing this issue. Also adding storage dir .gitignore. Could you please review and let me know so I can squash it to one commit?
|
@ares thanks, that looks good. I'd assumed storage was a file rather than a dir though. As such, could you add Please squash and amend the commit message to the format |
Also fallback to form login when Signo fails and log a warning. Make Signo SSO backend unavailable for API.
|
@domcleal added .gitkeep and squashed |
Also fallback to form login when Signo fails and log a warning.