Feature/hg hooks over tcp #1416

sdorra · 2020-11-10T10:08:12Z

Proposed changes

Implement mercurial hook callback over a separate tcp socket. This replaces the http based solution which is theoretically vulnerable to manipulated http request, see #1411 for more details.

The change is tested with https://github.com/scm-manager/hg-server-spec

Your checklist for this pull request

PR is well described
Related issues linked to PR if existing and labels set
Target branch is not master (in most cases develop should bet the target of choice)
Code does not conflict with target branch
New code is covered with unit tests
CHANGELOG.md updated
Definition of Done's fulfilled: DoD // UI DoD
Documentation updated (only necessary for new features or changed behaviour)

Checklist for branch merge request (not required for forks)

Branch is green/blue on Jenkins
Quality Gate passed on SonarQube

We use JWT for session management, so we can disable shiro session management and this allows usage of SecurityManager outside of a http request.

eheimbuch · 2020-11-17T15:01:02Z

scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/hooks/HookServer.java

+      try {
+        hookHandler.run();
+      } finally {
+        ThreadContext.unbindSubject();


Why would you unbind the subject here?

This because the hook handler does a login, which binds a subject to the thread. We use a cached thread pool, which reuse existing threads and to avoid that a subject is used from a prior hook, we have to unbind it.

eheimbuch · 2020-11-17T15:07:01Z

...lugins/scm-hg-plugin/src/main/java/sonia/scm/repository/spi/HgRepositoryServiceProvider.java

-  /** Field description */
-  public static final Set<Feature> FEATURES =
-    EnumSet.of(Feature.COMBINED_DEFAULT_BRANCH);
+  public static final Set<Feature> FEATURES = EnumSet.of(Feature.COMBINED_DEFAULT_BRANCH);


Make this field protected like SonarCloud suggests?

I don't know why SonarQube thinks that this should be protected, because FEATURES is used by HgRepositoryHandler. Changing FEATURES to protected leads to a compiler error.

sdorra · 2020-11-18T09:36:38Z

The failing build seams to come from the jest upgrade storybookjs/storybook#10942

pfeuffer · 2020-11-18T10:02:29Z

The message pipeline seems broken with these changes. You can check this easily with the commit message checker and an arbitrary failing regex check. The corresponding error does not reach the client (neither cli nor internal editor)

sdorra · 2020-11-18T10:14:04Z

The message pipeline seems broken with these changes. You can check this easily with the commit message checker and an arbitrary failing regex check. The corresponding error does not reach the client (neither cli nor internal editor)

Okay, i'll take a look at this.

sdorra · 2020-11-18T11:00:13Z

The failing build seams to come from the jest upgrade storybookjs/storybook#10942

I was able to fix the jest tests.

pfeuffer · 2020-11-18T13:40:19Z

May it be possible to "transmit" the transaction id to the hook handler, so that the logs can be assigned to the correct request?

pfeuffer · 2020-11-18T13:51:28Z

scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/hooks/DefaultHookHandler.java

+    } catch (Exception ex) {
+      LOG.warn("unknown error on hook occurred", ex);
+      return error("unknown error");


This catch block fetches all Exceptions from plugins, like for example the commit message checker. I don't think that a warn log is appropriate for these cases. On the other hand exceptions that are not thrown on purpose by plugins can indicate a problem. Problem is: I don't know a way how to differentiate between them. The PathWP plugin for example uses a simple exception that derives from RuntimeException. So we cannot rely on the usage of ExceptionWithContext.
On second thought, I think would do this:

Suggested change

} catch (Exception ex) {

LOG.warn("unknown error on hook occurred", ex);

return error("unknown error");

} catch (ExceptioWithContextn ex) {

LOG.debug("scm exception on hook occurred", ex);

return error(ex.getMessage());

} catch (Exception ex) {

LOG.warn("unknown error on hook occurred", ex);

return error("unknown error");

I've applied your suggestion, but i've passed the message of the exception instead of "unkown error". This restores the behaviour before that change, but i'm not sure if this is the best way. Because we could leak technical details on unwanted exceptions.

The leak is exactly why i've left it the way you've done it before for unknown exceptions. If you want to have proper messages, one should use ExceptioWithContext.

scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/hooks/HookServer.java

scm-webapp/src/main/java/sonia/scm/security/JwtAccessTokenBuilder.java

…m-manager into feature/hg_hooks_over_tcp

sdorra · 2020-11-19T13:36:29Z

May it be possible to "transmit" the transaction id to the hook handler, so that the logs can be assigned to the correct request?

Done

pfeuffer · 2020-11-20T14:36:05Z

Ok, don't know exactly how I got here, but I try to recap:

For a hg repository I configured the message checker plugin only to accept special commit messages
Using the editor plugin, I changed a file in this repo and tried to commit it with an invalid message
- I got the correct error message
I tried to commit again (simply pressing the button again, and the server hung up
I restarted the server
Again, I changed the file using the editor plugin and tried to commit
- this time I got a generic error message and the log said that an exception was mapped to 500
- I could click the button again and got the same result
I cloned this repo in the shell
... changed a file and committed it
On push I got a generic error.
I disabled the commit message check and
pushed again

This is what I got now:

2020-11-20 15:25:16.006 [qtp1006565608-94] [BpSGtKXdRg] DEBUG sonia.scm.web.cgi.DefaultCGIExecutor - execute cgi: python /home/rene/scm-home/lib/python/hgweb.py
2020-11-20 15:25:16.135 [cgi-pool-1] [          ] WARN  sonia.scm.web.cgi.DefaultCGIExecutor - Traceback (most recent call last):
  File "/home/rene/scm-home/lib/python/hgweb.py", line 54, in <module>
    wsgicgi.launch(application)
  File "/usr/lib/python2.7/dist-packages/mercurial/hgweb/wsgicgi.py", line 89, in launch
    for chunk in content:
  File "/usr/lib/python2.7/dist-packages/mercurial/hgweb/hgweb_mod.py", line 326, in run_wsgi
    for r in self._runwsgi(req, res, repo):
  File "/usr/lib/python2.7/dist-packages/mercurial/hgweb/hgweb_mod.py", line 353, in _runwsgi
    rctx, req, res, self.check_perm
  File "/usr/lib/python2.7/dist-packages/mercurial/wireprotoserver.py", line 233, in handlewsgirequest
    _callhttp(repo, req, res, proto, cmd)
  File "/usr/lib/python2.7/dist-packages/mercurial/wireprotoserver.py", line 469, in _callhttp
    rsp = wireprotov1server.dispatch(repo, proto, cmd)
  File "/usr/lib/python2.7/dist-packages/mercurial/wireprotov1server.py", line 82, in dispatch
    return func(repo, proto, *args)
  File "/usr/lib/python2.7/dist-packages/mercurial/wireprotov1server.py", line 664, in unbundle
    repo, gen, their_heads, b'serve', proto.client()
  File "/usr/lib/python2.7/dist-packages/mercurial/exchange.py", line 2773, in unbundle
    gettransaction()
  File "/usr/lib/python2.7/dist-packages/mercurial/exchange.py", line 2762, in gettransaction
    lockandtr[2] = repo.transaction(source)
  File "/usr/lib/python2.7/dist-packages/mercurial/localrepo.py", line 2058, in transaction
    hint=_(b"run 'hg recover' to clean up transaction"),
mercurial.error.RepoError: abandoned transaction found

2020-11-20 15:25:16.135 [qtp1006565608-94] [BpSGtKXdRg] WARN  sonia.scm.web.HgCGIExceptionHandler - Mercurial/Python process ends with return code 1

And here is the generic 500 exception:

2020-11-20 15:34:57.984 [qtp1859390802-86] [FpSGtMyx7G] TRACE sonia.scm.api.rest.ContextualExceptionMapper - map ExceptionWithContext to status code 500
sonia.scm.repository.spi.IntegrateChangesFromWorkdirException: errors from hook
	at sonia.scm.repository.spi.IntegrateChangesFromWorkdirException$MessageExtractor.forMessage(IntegrateChangesFromWorkdirException.java:75)
	at sonia.scm.repository.spi.HgModifyCommand.pullModifyChangesToCentralRepository(HgModifyCommand.java:136)
	at sonia.scm.repository.spi.HgModifyCommand.execute(HgModifyCommand.java:116)
	at sonia.scm.repository.api.ModifyCommandBuilder.execute(ModifyCommandBuilder.java:135)
	at com.cloudogu.scm.editor.EditorService$FileUploader.done(EditorService.java:151)
	at com.cloudogu.scm.editor.EditorResource.processFiles(EditorResource.java:591)
	at com.cloudogu.scm.editor.EditorResource.modify(EditorResource.java:501)
	at com.cloudogu.scm.editor.EditorResource.modifyInRoot(EditorResource.java:357)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.jboss.resteasy.core.MethodInjectorImpl.invoke(MethodInjectorImpl.java:170)
	at org.jboss.resteasy.core.MethodInjectorImpl.invoke(MethodInjectorImpl.java:130)
	at org.jboss.resteasy.core.ResourceMethodInvoker.internalInvokeOnTarget(ResourceMethodInvoker.java:643)
	at org.jboss.resteasy.core.ResourceMethodInvoker.invokeOnTargetAfterFilter(ResourceMethodInvoker.java:507)
	at org.jboss.resteasy.core.ResourceMethodInvoker.lambda$invokeOnTarget$2(ResourceMethodInvoker.java:457)
	at org.jboss.resteasy.core.interception.jaxrs.PreMatchContainerRequestContext.filter(PreMatchContainerRequestContext.java:364)
	at org.jboss.resteasy.core.ResourceMethodInvoker.invokeOnTarget(ResourceMethodInvoker.java:459)

I could reproduce this in a second repository: On the first change, I got the correct error message, the following commits hung up, after a restart I got generic exceptions due to the stack trace above.

sdorra · 2020-11-22T12:42:05Z

I could reproduce this in a second repository: On the first change, I got the correct error message, the following commits hung up, after a restart I got generic exceptions due to the stack trace above.

The problem was introduced with the passthrough of the transaction id. However i've modified the hook socket protocol to be more robust, because the former error has lead to an endless loop in the python process. With the new implementation this should never happen again.

pfeuffer · 2020-11-23T08:02:31Z

I could reproduce this in a second repository: On the first change, I got the correct error message, the following commits hung up, after a restart I got generic exceptions due to the stack trace above.

The problem was introduced with the passthrough of the transaction id. However i've modified the hook socket protocol to be more robust, because the former error has lead to an endless loop in the python process. With the new implementation this should never happen again.

Okay, now the process does not hang up, but again the error messages (for example from the custom message validator) are not propagated.

sdorra · 2020-11-23T11:28:38Z

I can't confirm this. I get the custom error messages. How did you test this?

pfeuffer · 2020-11-23T11:39:49Z

This is interesting. Indeed I get the correct error messaage with a repository I've not used for testing this before, but not with the one I used for the last test. Is the latter one broken somehow?

sdorra · 2020-11-23T12:07:26Z

It is possible, that the old repository had a non finished transaction (abort: abandoned transaction found). This could happen with the prior version, when the pre receive hook hangs. I think it is possible to fix the abandoned transaction manually with hg recover.

pfeuffer · 2020-11-23T12:17:26Z

It is possible, that the old repository had a non finished transaction (abort: abandoned transaction found). This could happen with the prior version, when the pre receive hook hangs. I think it is possible to fix the abandoned transaction manually with hg recover.

Indeed this fixed this issue, thanks! We'll have a look at the code later and get back to you ;-)

…d writing

# Conflicts: # CHANGELOG.md

Use waitFor instead of exitValue to wait for process to finish

sonarcloud · 2020-11-27T10:25:09Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

66.1% Coverage
0.0% Duplication

Fixes a regression which was introduced with #1416. In #1416 we have reimplemented the way configuration is passed to the mercurial cgi handler. Before #1416 we used environment variables which are picked up by hgweb.py, after #1416 we pass mercurial configurations as command line parameters directly in the HgCGIServlet. But sadly the configuration option for httppostargs uses still an environment variable, which is not picked up by anyone. See #1525

sdorra added 12 commits November 10, 2020 10:03

Implement mercurial hooks over sockets

2331766

Refactor nearly the whole scm-hg-plugin for new hook implementation

d518af4

Disable xsrf for mercurial hook tokens

d86b2f7

Disable shiro session storage

36aa059

We use JWT for session management, so we can disable shiro session management and this allows usage of SecurityManager outside of a http request.

Rename mercurial hooks to match python coding specs

e774c77

Improve logging for mercurial hooks

5270195

Fix multiple accessor threads

5118294

Improve connection handling of mercurial hook python client

e038eeb

Update changelog

70969b5

Remove version script because it is not longer required

8b4f92f

Remove disableHookSSLValidation field from mercurial settings form

3fe678e

Update pull request link

84aa8b4

sdorra added bug Something isn't working enhancement New feature or request labels Nov 10, 2020

eheimbuch self-assigned this Nov 17, 2020

eheimbuch reviewed Nov 17, 2020

View reviewed changes

Merge with develop branch

58a0c51

Update storybook components to be compatible with jest 26

c2df6d2

Fix test for jest 26

5311cd5

pfeuffer requested changes Nov 18, 2020

View reviewed changes

sdorra and others added 5 commits November 19, 2020 09:26

Fixed missing messages on exceptions during hook

b6c5a25

Fix review findings

982743e

Fix review findings

29faa5e

Pass transaction id from request to mercurial hooks

1311061

Merge branch 'develop' into feature/hg_hooks_over_tcp

0b96e7d

Merge branch 'feature/hg_hooks_over_tcp' of github.com:scm-manager/sc…

0841a0a

…m-manager into feature/hg_hooks_over_tcp

sdorra and others added 2 commits November 19, 2020 14:49

Fix compiler error

959dfb8

Merge branch 'develop' into feature/hg_hooks_over_tcp

33e7a13

Do not expose internal exception messages

73b2c4a

Implement more robust socket hook protocol

c0ae910

sdorra force-pushed the feature/hg_hooks_over_tcp branch from abaa7b9 to c0ae910 Compare November 22, 2020 12:44

sdorra added 3 commits November 27, 2020 08:10

Use DataInputStream and DataOutputStream instead of manual reading an…

7357d83

…d writing

Do not use socket.MSG_WAITALL

3e19f28

Merge branch 'develop' into feature/hg_hooks_over_tcp

8ee8c8b

# Conflicts: # CHANGELOG.md

sdorra force-pushed the feature/hg_hooks_over_tcp branch from 41953e1 to 8ee8c8b Compare November 27, 2020 07:57

sdorra added 2 commits November 27, 2020 10:46

Fix broken HgVersionCommand

136cdbb

Use waitFor instead of exitValue to wait for process to finish

Merge with develop branch

52f26ab

Fix typo

d732100

pfeuffer merged commit 4e326fe into develop Nov 27, 2020

pfeuffer deleted the feature/hg_hooks_over_tcp branch November 27, 2020 12:12

sdorra mentioned this pull request Feb 8, 2021

Fix broken mercurial http post args configuration #1532

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/hg hooks over tcp #1416

Feature/hg hooks over tcp #1416

sdorra commented Nov 10, 2020

eheimbuch Nov 17, 2020

sdorra Nov 18, 2020

eheimbuch Nov 17, 2020 •

edited

sdorra Nov 18, 2020

sdorra commented Nov 18, 2020

pfeuffer commented Nov 18, 2020

sdorra commented Nov 18, 2020

sdorra commented Nov 18, 2020

pfeuffer commented Nov 18, 2020

pfeuffer Nov 18, 2020

sdorra Nov 19, 2020

pfeuffer Nov 20, 2020

sdorra commented Nov 19, 2020

pfeuffer commented Nov 20, 2020 •

edited

sdorra commented Nov 22, 2020

pfeuffer commented Nov 23, 2020

sdorra commented Nov 23, 2020

pfeuffer commented Nov 23, 2020

sdorra commented Nov 23, 2020 •

edited

pfeuffer commented Nov 23, 2020

sonarcloud bot commented Nov 27, 2020

Feature/hg hooks over tcp #1416

Feature/hg hooks over tcp #1416

Conversation

sdorra commented Nov 10, 2020

Proposed changes

Your checklist for this pull request

Checklist for branch merge request (not required for forks)

eheimbuch Nov 17, 2020

Choose a reason for hiding this comment

sdorra Nov 18, 2020

Choose a reason for hiding this comment

eheimbuch Nov 17, 2020 • edited

Choose a reason for hiding this comment

sdorra Nov 18, 2020

Choose a reason for hiding this comment

sdorra commented Nov 18, 2020

pfeuffer commented Nov 18, 2020

sdorra commented Nov 18, 2020

sdorra commented Nov 18, 2020

pfeuffer commented Nov 18, 2020

pfeuffer Nov 18, 2020

Choose a reason for hiding this comment

sdorra Nov 19, 2020

Choose a reason for hiding this comment

pfeuffer Nov 20, 2020

Choose a reason for hiding this comment

sdorra commented Nov 19, 2020

pfeuffer commented Nov 20, 2020 • edited

sdorra commented Nov 22, 2020

pfeuffer commented Nov 23, 2020

sdorra commented Nov 23, 2020

pfeuffer commented Nov 23, 2020

sdorra commented Nov 23, 2020 • edited

pfeuffer commented Nov 23, 2020

sonarcloud bot commented Nov 27, 2020

eheimbuch Nov 17, 2020 •

edited

pfeuffer commented Nov 20, 2020 •

edited

sdorra commented Nov 23, 2020 •

edited