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

Pluggable auth #2367

Merged
merged 30 commits into from Jul 19, 2016
Merged

Pluggable auth #2367

merged 30 commits into from Jul 19, 2016

Conversation

@kroepke
Copy link
Member

@kroepke kroepke commented Jun 16, 2016

This change refactors the way the authentication subsystem works:

  • Rather than hardcoding AuthenticationRealm implementations, they are contributed by an orderable, injected configuration.
  • The UI for users, roles and authentication is unified and refactored to allow plugins to contribute new implementations.
  • LDAP UI is now separate from the users list.
  • a first implementation of an authentication plugin can be found at https://github.com/Graylog2/graylog-plugin-auth-sso
  • to be able to use request context information a new token implementation exposes the request HTTP headers to authenticators if they declare to use them (again see sso plugin for an example)
  • validating a session during SessionStore initialization can now return a new session and a new user to allow "automatic" login, which is used by the SSO plugin

fixes #2232

@kroepke kroepke added this to the 2.1.0 milestone Jun 16, 2016
@kroepke kroepke self-assigned this Jun 16, 2016
kroepke added 14 commits Jun 1, 2016
similar to message processor ordering, allow ordering of authenticating realms at runtime
a default order is maintained for the built-in providers, additional providers can be ordered freely as well

WIP
moved configuration of built-in authenticating realms in their constructors and simplified the SecurityManager provider
actually hook up the new ordered realms collection to enable runtime reordering
mostly dummy components, no config endpoints yet etc
add "subpage" prop to PageHeader, which doesn't render boxes around the content, by default it's off
redirect back to users list after successful updates
@kroepke kroepke force-pushed the pluggable-auth branch from 74cd067 to b2087b1 Jul 6, 2016
@joschi joschi self-assigned this Jul 11, 2016
kroepke added 7 commits Jul 12, 2016
…ls/session validation

rationale is that once an authenticator looks at externally supplied credentials or pre-authenticated principals, such as HTTP headers,
it can request an early creation of a Graylog session.
This is the case with HTTP header based authentication, which typically comes from SSO proxies.

slightly refactor the LDAP entry to user creation, we'll need to create a better interface for other authenticators in the next few steps

for the UI, once the session validation returns a new username and session id, we'll simply accept that and log the user in.
likewise, on logout we validate the session again, in case the credentials were external.

a current problem with the implementation is that existing sessions for the same user and client aren't reused yet, because we cannot detec them: this creates uncessary sessions in the database, which will eventually time out.
@AutoValue
@JsonAutoDetect
public abstract class SessionValidationResponse {
@JsonProperty("is_valid")
public abstract boolean isValid();

@JsonProperty("new_session_id")
@Nullable
public abstract String newSessionId();

This comment has been minimized.

@joschi

joschi Jul 18, 2016
Contributor

Why "new_session_id" and not just "session_id"?

This comment has been minimized.

@kroepke

kroepke Jul 19, 2016
Author Member

The intention was that this field contains a different session id than the one which had been passed in. But I guess session_id is enough 👍

This comment has been minimized.

@joschi

joschi Jul 19, 2016
Contributor

"really_new_new_session_id2_bak" :trollface:

public AuthenticationConfig withRealms(final Set<String> availableRealms) {
final List<String> newOrder = new ArrayList<>();

// Check if processor actually exists.

This comment has been minimized.

@joschi

joschi Jul 18, 2016
Contributor

"processor" vs. "realm" in comments

@joschi
Copy link
Contributor

@joschi joschi commented Jul 18, 2016

When trying to access the System → Authentication page after running mvn clean install (which processes the JavaScript files) and starting Graylog, I get a blank page (except for the menu bar) and the following error in my Developer Console:

Uncaught Error: Requested actions "Authentication" is not registered.
plugin.org.graylog.plugins.pipelineprocessor.PipelineProcessorPlugin.c3f2b6e….js:14207 
getActions @ plugin.org.graylog.plugins.pipelineprocessor.PipelineProcessorPlugin.c3f2b6e….js:14207
(anonymous function) @ AuthenticationComponent.jsx:47
(anonymous function) @ AuthenticationComponent.jsx:225
827 @ 16.16.dcc96b5….js:3
n @ app.dcc96b5….js:1
(anonymous function) @ AuthenticationPage.jsx?./~/react-hot-loader!./~/babel-loader:37
(anonymous function) @ AuthenticationPage.jsx?./~/react-hot-loader!./~/babel-loader:93
1011 @ 16.16.dcc96b5….js:3
n @ app.dcc96b5….js:1
(anonymous function) @ AuthenticationPage.jsx:7
window.webpackJsonp @ app.dcc96b5….js:1
(anonymous function) @ 16.16.dcc96b5….js:1

EDIT: Seems to work when using the webpack dev server (running npm start in the graylog2-web-interface directory).

final String host = headers.getFirst(HttpHeaders.HOST);
final Request grizzlyRequest = grizzlyRequestProvider.get();

final String host = grizzlyRequest.getRemoteAddr();

This comment has been minimized.

@joschi

joschi Jul 18, 2016
Contributor

This won't work with reverse proxies in front of Graylog. We should at least support the "standard" headers like X-Forwarded-For (also see RestTools#getRemoteAddrFromRequest()).

This comment has been minimized.

@kroepke

kroepke Jul 19, 2016
Author Member

Aye!


@Override
public Object getPrincipal() {
return httpHeaders.get("remote_user");

This comment has been minimized.

@joschi

joschi Jul 18, 2016
Contributor

Is this a header name chosen by us? If so, I'd recommend using X-Graylog-Remote-User instead.

This comment has been minimized.

@kroepke

kroepke Jul 19, 2016
Author Member

see below.


@Override
public Object getCredentials() {
return httpHeaders.get("remote_credentials");

This comment has been minimized.

@joschi

joschi Jul 18, 2016
Contributor

Is this a header name chosen by us? If so, I'd recommend using X-Graylog-Remote-Credentials instead.

This comment has been minimized.

@kroepke

kroepke Jul 19, 2016
Author Member

These are really only return values because the interface requires it.
The intention for a HttpHeadersToken is that a plugin (such as the SSO one) can pick any header(s) it likes to get the relevant information. The only user of this token is not even using getPrincipal/getCredentials.
However, I think this should be clearer, so I'll add documentation comments to that effect.
I think remote_user is a good choice as it is often used, remote_credentials is probably obscure. I'll check if we can simply return null here safely.

This comment has been minimized.

@joschi

joschi Jul 19, 2016
Contributor

Isn't "REMOTE_USER" usually just a CGI environment variable rather than an HTTP request header?

This comment has been minimized.

@kroepke

kroepke Jul 19, 2016
Author Member

I guess this is largely a question of convention. We can also leave it as null.

@joschi
Copy link
Contributor

@joschi joschi commented Jul 19, 2016

#2367 (comment) has been fixed by rebuilding all related projects (I hate computers…).

Thanks @edmundoa for helping to figure this out!

the real values need to be retrieved from the headers explicitly, simply because no sane default exists
@Override
@Nullable
public Object getPrincipal() {
return null;

This comment has been minimized.

@joschi

joschi Jul 19, 2016
Contributor

Would throwing an UnsupportedOperationException work here or are we afraid that stuff™ will break because of it?

This comment has been minimized.

@kroepke

kroepke Jul 19, 2016
Author Member

I'd simply return null, not sure if this is used somewhere in the framework's code.

This comment has been minimized.

@joschi

joschi Jul 19, 2016
Contributor

kroepke added 4 commits Jul 19, 2016
keep the parameter name as newSessionId to make it clear to the developer that it expects a new session id here
@joschi
Copy link
Contributor

@joschi joschi commented Jul 19, 2016

LGTM. 👍

@joschi joschi merged commit 9d967e0 into master Jul 19, 2016
4 checks passed
4 checks passed
@garybot2
ci-server-integration Jenkins build graylog2-server-integration-pr 1120 has succeeded
Details
@garybot2
ci-web-linter Jenkins build graylog-pr-linter-check 606 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@joschi joschi deleted the pluggable-auth branch Jul 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants