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

Delegate authentication to reverse proxy #182

Merged
merged 8 commits into from
Feb 24, 2017
Merged

Delegate authentication to reverse proxy #182

merged 8 commits into from
Feb 24, 2017

Conversation

innotech-research
Copy link
Contributor

@innotech-research innotech-research commented Feb 21, 2017

Supports delegation of authentication to a reverse proxy, which then supplies the authenticated username in a HTTP header (currently hardcoded to X-Forwarded-User).

Configuration can be specified in access_control_rules or users block by proxy_auth: []. Any user can be accepted by using a '*'.

example configuration:

- name: Admin
  type: allow
  proxy_auth: ["admin"]

- name: User
  type: allow
  proxy_auth: ["*"]
  indices: ["index1"]

- name: Unauthorized
  type: forbid
  proxy_auth: ["*"]

Copy link
Owner

@sscarduzio sscarduzio left a comment

Choose a reason for hiding this comment

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

Apart from the extra logs, LGTM. Thank you!

@@ -105,6 +107,12 @@ public Block(Settings s, List<Settings> userList, Logger logger) {
} catch (RuleNotConfiguredException e) {
}
try {
logger.info("Checking ProxyAuthRule settings");
Copy link
Owner

Choose a reason for hiding this comment

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

I think these log lines can go away like for the other rules

@sscarduzio
Copy link
Owner

@innotech-research thanks guys for this.

Believe it or not, I thought about this feature this afternoon in a conversation with @psaiz.

Really useful for people having weird auth flows, so they can reuse some battle-tested nginx/httpd extensions as reverse proxy.

@sscarduzio
Copy link
Owner

@innotech-research can you also address those easy fixes from codacy? Thanks!

Fix codacy issues
@innotech-research
Copy link
Contributor Author

OK, I should have made all the requested changes and removed the unnecessary logging.

@sscarduzio
Copy link
Owner

@innotech-research we have some problems merging this after the LDAP merge. Could you please make the PR against the new master?

@innotech-research
Copy link
Contributor Author

I'm trying to merge (it'll take a while - the ldap pull is huge), but I'm currently in jar hell.
I've just done a new git clone from your repository, run a gradlew build and that fails as well in jar hell.
Not sure what I can do if the master is broken.

@psaiz
Copy link

psaiz commented Feb 23, 2017

Thanks for doing this! This is very useful indeed

@coutoPL
Copy link
Collaborator

coutoPL commented Feb 23, 2017

@innotech-research, try ./bin/build.sh.
./gradlew test should also work.

@sscarduzio
Copy link
Owner

FYI the jar hell situation will be resolved by @coutoPL's PR on testcontainers testcontainers/testcontainers-java#292

@coutoPL do you have any news on when to expect a new release from them? Or is there a snapshot version we can depend on?

@coutoPL
Copy link
Collaborator

coutoPL commented Feb 23, 2017

I thought that this fix will help me, but no ... there was another, and another jar hell, so I gave up :| Maybe solution for this issue is splitting tests into two modules: one with unit tests (all currently enabled) and another for integration tests. The unit tests module should be ES independent, so the jarhell check will not occur.

@sscarduzio
Copy link
Owner

@innotech-research in the meanwhile, could you skip doing the integration tests like suggested by @coutoPL?
BTW @innotech-research I'm so sorry for the trouble. Super grateful for the two important PRs, but they came on the same day (after months of no-PRs) 🤷‍♂️

Of course, if you have suggestions on how handle this kind of situation better, I'm all ears. :octocat:

@coutoPL
Copy link
Collaborator

coutoPL commented Feb 23, 2017

I've found 3 integration tests in project. 2 of them ware commented. I had to comment third one to make tests work. As I said... ./gradlew test works and you could try it. Building plugin with ./gradlew assembly also works.

@innotech-research if you want to, I could help you with this merge.

@sscarduzio
Copy link
Owner

@coutoPL yeah I don't really care of those integration tests when we have the possibility to use something like testcontainers to bring up an actual ES, install the plugin, give a real configuration and throw some real HTTP request to it.

I was already am doing this (see /integration_tests) but manually before releasing. With testcontainers we can put all of this in a Java test class which means Travis-CI can do this for me 👍

@coutoPL
Copy link
Collaborator

coutoPL commented Feb 23, 2017

@sscarduzio, yes totally agree. I use testcontainers in this way and IMO it is the best approach to do integration tests currently.

@sscarduzio
Copy link
Owner

OK Then, let's remove the the testcontainers dependency and create a separate project for integration tests in a separate project.

I'm quite reluctant on using gradle subprojects, I barely manage to make it work for a simple project: docs are 💩 and ES froze the dependency to an old gradle version.

@coutoPL what do you propose? You're the testcontainers expert. I can later port my existing tests to whatever you come up to: Maven, Gradle, SBT, Scala, whatever.

@coutoPL
Copy link
Collaborator

coutoPL commented Feb 23, 2017

IMO we should merge this PR first. Then in other branch we could try to get rid of ES dependency. I don't know gradle well (and I'm not a big fun too) ... now I use SBT most of time, but IMO Java developers use in most gradle, so it's better to stay with it.

But if you want to, I can handle it. But not in a few coming days, so let's merge it first.

@innotech-research
Copy link
Contributor Author

I am in the process of merging the changes (if you can call it that). The changes mean that a 'simple' merge means that the result doesn't work due to the class name changes etc. I can't run tests (my build system can't run docker) and I can't run build. I'll see how quickly I can get it running again.

@innotech-research
Copy link
Contributor Author

OK. This should work. I'll pause on the next set of changes until the testing and building approach have been agreed and implemented.

@sscarduzio sscarduzio merged commit baa2f6e into sscarduzio:master Feb 24, 2017
@sscarduzio
Copy link
Owner

@innotech-research master is now building ok, you can make a new PR for the next set of changes

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

Successfully merging this pull request may close these issues.

None yet

4 participants