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

GssapiForceSession: workaround for Graylog 3.1 issue #209

Closed
wants to merge 4 commits into from

Conversation

t-gergely
Copy link

@t-gergely t-gergely commented Nov 12, 2019

request to /api/system/sessions breaks Kerberos auth [...] by adding HTTP header "Authorization: Basic dW5kZWZpbmVkOnNlc3Npb24="

My patch resolves this issue Graylog2/graylog-plugin-auth-sso#16 for Graylog 3.1 if using new "GssapiForceSession On" parameter. If accepted, someone with better English should add a description.
Thanks.

@simo5 simo5 changed the title GssapiForceSession: workaround for https://github.com/Graylog2/graylog-plugin-auth-sso/issues/16 GssapiForceSession: workaround for hGraylog 3.1 issue Nov 12, 2019
@simo5 simo5 changed the title GssapiForceSession: workaround for hGraylog 3.1 issue GssapiForceSession: workaround for Graylog 3.1 issue Nov 12, 2019
@simo5
Copy link
Contributor

simo5 commented Nov 12, 2019

@t-gergely can you please explain why this is needed ?
I would assume this kind of manipulation can be done directly in apache by removing whatever offending header and then adding it back after authentication if needed.

Why is this something you need to handle in mod_auth_gssapi itself?

In any case:

  • documentation is missing
  • squash patches
  • proper indentation of the changed condition is needed to understand what matches what

@simo5
Copy link
Contributor

simo5 commented Nov 12, 2019

Also why is this something that can't be patched in graylog? Sounds that that project is at fault of misusing standard headers?

@frozencemetery
Copy link
Member

I would also like to know why the workaround @sigmaris posted in that ticket doesn't work for you.

@t-gergely
Copy link
Author

t-gergely commented Nov 12, 2019

Thanks for your replies.

why is this something that can't be patched in graylog

I'm sure it can be. But I don't know how. It seems a lot of work nobody did in the last 3 years.

I would also like to know why the workaround @sigmaris posted in that ticket doesn't work for you.

First of all, I don't use nginx, and it would be difficult to change all the current configs. And...

I would assume this kind of manipulation can be done directly in apache by removing whatever offending header and then adding it back after authentication if needed.

I tried something like that, too, but I couldn't even remove that header with apache httpd 2.4 for the mod auth modules to not see it (eg. "RequestHeader unset Authorization early" didn't seem to change anything). "Early directives cannot depend on a request path, so they will fail in contexts such as
<Directory> or <Location>."

I'm probably a novice at Graylog, Git and Apache and all, sorry. :)

squash patches

Does it mean a single commit? If not, please refer me to the definition.
Does it involve a new pull request or can this one be amended?

documentation is missing

I could whip up an English-like text, should you intend to accept this pull-request. It'd probably need a rewrite, though. :)

@simo5
Copy link
Contributor

simo5 commented Nov 12, 2019

Thanks for your replies.

why is this something that can't be patched in graylog

I'm sure it can be. But I don't know how. It seems a lot of work nobody did in the last 3 years.

I would also like to know why the workaround @sigmaris posted in that ticket doesn't work for you.

First of all, I don't use nginx, and it would be difficult to change all the current configs. And...

I would assume this kind of manipulation can be done directly in apache by removing whatever offending header and then adding it back after authentication if needed.

I tried something like that, too, but I couldn't even remove that header with apache httpd 2.4 for the mod auth modules to not see it (eg. "RequestHeader unset Authorization early" didn't seem to change anything). "Early directives cannot depend on a request path, so they will fail in contexts such as
<Directory> or <Location>."

I'm probably a novice at Graylog, Git and Apache and all, sorry. :)

squash patches

Does it mean a single commit? If not, please refer me to the definition.

Yes it means squashing (technical git term) all patches in one.

Does it involve a new pull request or can this one be amended?

You can rebase your branch and force push and this will update this PR.

documentation is missing

I could whip up an English-like text, should you intend to accept this pull-request. It'd probably need a rewrite, though. :)

See the README file in the root directory, that's where all new options must be documented.

That said, I am not convinced yet we should accept this new option, I do not see general value for it, and I am very cautious about adding random options as they explode the test matrix (speaking of which , there are no tests either :-)

If there is a clear case where this option is necessary and cannot be handled better within apache I would be easily convinced, but here it just seem a hack to shortcut a more proper apache configuration or fixing improper use of headers by graylog.

@t-gergely
Copy link
Author

Thanks. I don't think I can convince you, so it'll probably stay a private patch for a long time. :) I don't think there's an easy alternative solution, either: I doubt if apache can be configured for this without writing a knew module which I won't have time for. And I don't think graylog's use of headers is incorrect, per se. Its use of basic auth for session handling may be a bad idea, but it works by default. The SSO plugin is an afterthought and it doesn't solve its task (cleanly). AFAIK it's based on Shiro that doesn't handle SPNEGO either.
You may close this request.

@simo5
Copy link
Contributor

simo5 commented Nov 12, 2019

Graylog use of an authentication mechanism to handle session data is definitely a bad idea, but I would say also incorrect, as it interacts badly with your auth mechanism, and not only yours.

Sorry to say no in this case, I feel your pain, but the maintainer hat imposes me to look at the larger picture as well..

@simo5 simo5 closed this Nov 12, 2019
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.

3 participants