pulsar icon indicating copy to clipboard operation
pulsar copied to clipboard

[fix][sec]disable trace in web service

Open leizhiyuan opened this issue 2 years ago • 6 comments

Fixes #18091

Master Issue: #18091

Motivation

close TRACE

refactor DisableDebugHttpMethodFilter , use the ConstraintSecurityHandler to achieve this.

because we can not access proxyConfig and WorkerConfig in DisableDebugHttpMethodFilter , the proxy module and worker module depends on broker-common, if I use the interface, PulsarConfiguration ,I need to do some tricky thing.

Modifications

Verifying this change

  • [ ] Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • [ ] Dependencies (add or upgrade a dependency)
  • [ ] The public API
  • [ ] The schema
  • [ ] The default values of configurations
  • [ ] The threading model
  • [ ] The binary protocol
  • [ ] The REST endpoints
  • [ ] The admin CLI options
  • [ ] Anything that affects deployment

Documentation

  • [ ] doc
  • [ ] doc-required
  • [x] doc-not-needed
  • [ ] doc-complete

Matching PR in forked repository

PR in forked repository:

leizhiyuan avatar Oct 18 '22 12:10 leizhiyuan

Codecov Report

Merging #18092 (74ddac5) into master (6c65ca0) will decrease coverage by 7.01%. The diff coverage is 58.06%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18092      +/-   ##
============================================
- Coverage     34.91%   27.90%   -7.02%     
+ Complexity     5707     3694    -2013     
============================================
  Files           607      393     -214     
  Lines         53396    43488    -9908     
  Branches       5712     4472    -1240     
============================================
- Hits          18644    12134    -6510     
+ Misses        32119    29458    -2661     
+ Partials       2633     1896     -737     
Flag Coverage Δ
unittests 27.90% <58.06%> (-7.02%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../org/apache/pulsar/broker/admin/v2/Namespaces.java 10.65% <0.00%> (+2.63%) :arrow_up:
.../pulsar/broker/service/AbstractBaseDispatcher.java 34.35% <ø> (-11.51%) :arrow_down:
.../service/SystemTopicBasedTopicPoliciesService.java 54.11% <0.00%> (+2.52%) :arrow_up:
.../pulsar/broker/stats/BrokerOperabilityMetrics.java 98.21% <ø> (+5.56%) :arrow_up:
.../java/org/apache/pulsar/broker/web/WebService.java 72.72% <0.00%> (-8.56%) :arrow_down:
...g/apache/pulsar/compaction/CompactedTopicImpl.java 10.71% <0.00%> (ø)
...java/org/apache/pulsar/proxy/server/WebServer.java 73.37% <0.00%> (-0.97%) :arrow_down:
...broker/delayed/InMemoryDelayedDeliveryTracker.java 22.00% <50.00%> (+22.00%) :arrow_up:
...ache/pulsar/broker/service/EntryFilterSupport.java 27.02% <50.00%> (+2.02%) :arrow_up:
...apache/pulsar/proxy/server/DirectProxyHandler.java 63.63% <50.00%> (ø)
... and 319 more

codecov-commenter avatar Oct 20 '22 07:10 codecov-commenter

@leizhiyuan the related test failed

WebServiceTest.testDisableHttpTraceAndTrackMethods

expected [405] but found [403]

And I think that we should add this to all the webservers in Pulsar (Proxy, Function Worker)

nicoloboschi avatar Oct 20 '22 08:10 nicoloboschi

After I checked the testcase , I find it seems disableHttpDebugMethods do the same thing. https://github.com/apache/pulsar/pull/7907

we can set disableHttpDebugMethods=true to solve the issue?

and I can add the disableHttpDebugMethods to Proxy, Function Worker

leizhiyuan avatar Oct 20 '22 09:10 leizhiyuan

After I checked the testcase , I find it seems disableHttpDebugMethods do the same thing. #7907

we can set disableHttpDebugMethods=true to solve the issue?

and I can add the disableHttpDebugMethods to Proxy, Function Worker

Makes sense to me. Before merging the pull it would be good to advise on the dev@. Since this can be considered as breaking change we must be careful on cherry-picking on the release branches, although in this case it's a benefit for all the users/operators.

nicoloboschi avatar Oct 20 '22 09:10 nicoloboschi

After I checked the testcase , I find it seems disableHttpDebugMethods do the same thing. #7907 we can set disableHttpDebugMethods=true to solve the issue? and I can add the disableHttpDebugMethods to Proxy, Function Worker

Makes sense to me. Before merging the pull it would be good to advise on the dev@. Since this can be considered as breaking change we must be careful on cherry-picking on the release branches, although in this case it's a benefit for all the users/operators.

agree with you , maybe we can use disableHttpDebugMethods in other component, so we won't break the original scene. but If users want to use the sec feature, they can open the switch.

leizhiyuan avatar Oct 20 '22 09:10 leizhiyuan

/pulsarbot rerun-failure-checks

poorbarcode avatar Nov 02 '22 04:11 poorbarcode

@leizhiyuan hi, I move this PR to release/2.9.5, if you have any questions, please ping me. thanks.

congbobo184 avatar Nov 17 '22 12:11 congbobo184

The pr had no activity for 30 days, mark with Stale label.

github-actions[bot] avatar Jan 17 '23 02:01 github-actions[bot]

As discussed on the mailing list https://lists.apache.org/thread/w4jzk27qhtosgsz7l9bmhf1t7o9mxjhp, there is no plan to release 2.9.6, so I am going to remove the release/2.9.6 label

michaeljmarshall avatar Jun 27 '23 21:06 michaeljmarshall

@leizhiyuan Please either rebase and test, or close the PR.

dave2wave avatar Jul 16 '23 19:07 dave2wave