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

Set cookie issue 438 #449

Merged
merged 4 commits into from
Apr 27, 2022
Merged

Set cookie issue 438 #449

merged 4 commits into from
Apr 27, 2022

Conversation

ashah-splunk
Copy link
Contributor

Changes in response to the issue #438

  • Updated the hasCookie method to check for the cookie received from Splunk (i.e "splunkd_") instead of the existing length check.
  • Updated the test cases with respect to the code changes.

akaila-splunk and others added 4 commits April 1, 2022 10:30
fix for issue 'splunklib expects any Set-Cookie to be an auth cookie from Splunk. This is a problem when authenticating with a bearer token.'
fix for issue 'splunklib expects any Set-Cookie to be an auth cookie from Splunk. This is a problem when authenticating with a bearer token.'
@ashah-splunk ashah-splunk merged commit c282acc into develop Apr 27, 2022
@bendikro
Copy link

This fix now ensures that cookies only will be used when there is an auth cookie splunkd_<XXXX> availlable in self.http._cookies. However, it introduces a new bug which is that if there is a cookie available, e.g. from a Set-Cookie from a middleware, when there is not an auth cookie availlable, this cookie will not be included in the requests.

@fantavlik
Copy link
Contributor

This fix now ensures that cookies only will be used when there is an auth cookie splunkd_<XXXX> availlable in self.http._cookies. However, it introduces a new bug which is that if there is a cookie available, e.g. from a Set-Cookie from a middleware, when there is not an auth cookie availlable, this cookie will not be included in the requests.

Hi @bendikro - the fact that any/all cookies were being sent if detected was an unintentional side-effect of the bug that was addressed in this change. By design the SDK should not include cookies unrelated to Splunk - although any additional headers including cookies can be set by users to be sent by the SDK client here:

:param headers: List of extra HTTP headers to send (optional).
:type headers: ``list`` of 2-tuples.
does this address your concern here? I hope I'm capturing your middleware use case.

@ashah-splunk ashah-splunk deleted the set-cookie-issue-438 branch May 2, 2022 06:57
@bendikro
Copy link

bendikro commented May 5, 2022

Hi @bendikro - the fact that any/all cookies were being sent if detected was an unintentional side-effect of the bug that was addressed in this change. By design the SDK should not include cookies unrelated to Splunk - although any additional headers including cookies can be set by users to be sent by the SDK client here:

:param headers: List of extra HTTP headers to send (optional).
:type headers: ``list`` of 2-tuples.

does this address your concern here? I hope I'm capturing your middleware use case.

Hi @fantavlik

What is the rationale for not including cookies in the requests when a Set-Cookie has been included in a response? What is gained by ignoring these?

With "cookies unrelated to Splunk" you mean Set-Cookies that are not set by the Splunk server?
I challenge your definition of "unrelated to Splunk", and for our case I'd argue the Set-Cookie (BIGipServer<MyPoolName>) from the F5 load balancer is very much related to Splunk even though it's not the Splunk server that generates the Set-Cookie. And by ignoring this Set-Cookie (set by the F5 middleware in our case), the Splunk SDK doesn't work properly when the network is configured with any load balancer that utilizes this technique.

The official Splunk docs even has a section on Use a load balancer with search head clustering:

Splunk recommends that you run a third-party hardware or software load balancer in front of your set of clustered search heads. That way, users can access the set of search heads through a single interface, without needing to specify a particular one.

There are a variety of third-party load balancers available that you can use for this purpose. Select a load balancer that employs layer-7 (application-level) processing.

Configure the load balancer so that user sessions are "sticky" or "persistent." This ensures that the user remains on a single search head throughout their session.

From this we find:

  • Splunk recommends using a load balancer in front of the clustered search heads.
  • The sticky sessions technique is specifically mentioned to ensure the session requests remain on a single search head.

The F5 doc has a page HTTP Load Balancing with Cookie Persistence, where "Cookie Persistence" is the "sticky" or "persistent" sessions referenced in the Splunk docs above.

The solution to include additional headers manually in the client is how we worked around the issue in the first place, but it shouldn't be necessary to manually extract the BIGip cookie (after initial requests to Splunk) and then create a new client with this cookie as an additional header. It is the responsibility of the Splunk SDK client to handle this.

If this behavior of the SDK truly is by design, I think the design should be reconsidered.

@vmalaviya-splunk vmalaviya-splunk mentioned this pull request Jun 6, 2022
@fantavlik
Copy link
Contributor

Hi @bendikro thanks for giving us a detailed breakdown of your use case. I believe we've addressed your request in this PR: #463 which will be part of our next release in the coming days.

@bendikro
Copy link

@fantavlik Thanks! That fix has a bug though (#438 (comment)) but after resolving that it seems to be working properly.

@Trek333
Copy link
Contributor

Trek333 commented Aug 18, 2022

@fantavlik and @bendikro The latest version of the SOAR Splunk app is impacted by this issue also. I would suggest that the next release of the Splunk Enterprise Python SDK features list should mention support of Load Balancer "sticky sessions" (persistent cookies) to make it more clear which software package that handles this feature.

The Splunk Enterprise Python SDK is included with the SOAR Splunk app to connect to a Splunk Enterprise Search Head (SH) server and/or Search Head Cluster (SHC). Using the proxy feature of the Burp Suite app to observe the behavior of the http requests & responses by the SOAR Splunk app via the Splunk Enterprise Python SDK to a Splunk Enterprise SHC with a F5 Load Balancer in front of it, the BIGipServer cookie used for persistence ("sticking sessions") is not included in subsequent http requests after receiving the BIGipServer cookie in the response of the first http request from the Splunk SOAR app. However as a comparison using a Firefox browser instead of the SOAR Splunk app as client to the Splunk Enterprise SHC, a Firefox browser does send the BIGipServer cookie in subsequent http requests after receiving the BIGipServer cookie in the response of the first http request from the Firefox browser.

@bendikro
Copy link

bendikro commented Sep 8, 2022

@Trek333 I agree the feature should be listed, and the unit tests should be updated with a test to avoid any future regressions.

@ashah-splunk
Copy link
Contributor Author

@bendikro we did include unit tests where if the Auth Cookie is not available, authentication is being done with username and password. Test cases were updated as part of this PR itself, would request you to refer them. Also there were multiple PRs for the fix of the issue #438 and the fix was delivered in 2 different releases. As for mentioning this feature it was mentioned in the CHANGELOG.md but under Bug Fix(version 1.6.20) and Minor Changes(version 1.7.0) - Reference. I hope I have answered your concern, please do let me know if anything I have missed. Thanks

@Trek333
Copy link
Contributor

Trek333 commented Sep 9, 2022

@ashah-splunk and @bendikro #485 pull request has been submitted to add a unit test that is specific to regression testing the persistence of 3rd party cookies by the Splunk Enterprise Python SDK. More specifically, this unit test is targeted at the use case of the Splunk Enterprise Python SDK supporting "sticky sessions" of a Load Balancer for a Splunk Enterprise SHC (Search Head Cluster). Please let me know your feedback and feel to free to add more unit tests for 3rd party cookie persistence. Thanks

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.

5 participants