Fixed
Status Update
Comments
tb...@atolcd.com <tb...@atolcd.com> #2
[Empty comment from Monorail migration]
sv...@axis.com <sv...@axis.com> #3
"-O" doesn't seem to be a legitimate option for OpenSSH 8.0 which makes unfeasible to change the command ATM.
unknown option -- O
usage: scp [-346BCpqrTv] [-c cipher] [-F ssh_config] [-i identity_file]
[-J destination] [-l limit] [-o ssh_option] [-P port]
[-S program] source ... target
unknown option -- O
usage: scp [-346BCpqrTv] [-c cipher] [-F ssh_config] [-i identity_file]
[-J destination] [-l limit] [-o ssh_option] [-P port]
[-S program] source ... target
tb...@atolcd.com <tb...@atolcd.com> #4
Indeed, the -O flag was apparently added in OpenSSH 8.7 published last summer when they introduced support for the SFTP protocol.
I wonder how much time the old protocol will continue to be supported though.
Maybe the commands could use the "anonymous HTTP" way of retrieving the hook even when cloning over SSH ?
I wonder how much time the old protocol will continue to be supported though.
Maybe the commands could use the "anonymous HTTP" way of retrieving the hook even when cloning over SSH ?
ma...@ergon.ch <ma...@ergon.ch> #5
We have the same problem with users updated to OpenSSH 9.0
I think the only correct fix would be to enable sftp on the SSHD server. I see that MINA was updated for the upcoming 3.6 release - maybe it now supports SFTP? (However I don't quite understand if MINA is used at all by default?)
I think the only correct fix would be to enable sftp on the SSHD server. I see that MINA was updated for the upcoming 3.6 release - maybe it now supports SFTP? (However I don't quite understand if MINA is used at all by default?)
da...@gmail.com <da...@gmail.com> #6
[Empty comment from Monorail migration]
[Deleted User] <[Deleted User]> #7
@David: while I have fixed bugs in the SFTP implementation of Apache MINA sshd, I have never implemented an SFTP server. So I'm not really an expert here.
Here's my thoughts:
The old scp implementation will probably need to stay; people will keep using old OpenSSH versions, too.
To support OpenSSH versions and SFTP access, Gerrit's SSH daemon would need to be configured with an SftpSubsystem using a read-only FileSystem based on Gerrit's ToolsCatalog. Looks like a bit of work if it should be done directly from the in-JAR resources. Perhaps the easiest way to do that might be to dump the in-JAR resources into the file system, maybe under etc/tools, and then use a RootedFileSystemProvider with etc/tools as root?
Here's my thoughts:
The old scp implementation will probably need to stay; people will keep using old OpenSSH versions, too.
To support OpenSSH versions and SFTP access, Gerrit's SSH daemon would need to be configured with an SftpSubsystem using a read-only FileSystem based on Gerrit's ToolsCatalog. Looks like a bit of work if it should be done directly from the in-JAR resources. Perhaps the easiest way to do that might be to dump the in-JAR resources into the file system, maybe under etc/tools, and then use a RootedFileSystemProvider with etc/tools as root?
ta...@gmail.com <ta...@gmail.com> #8
In the meantime, I think it would be useful to extend the error message returned by the commit validator with a note about this. We can't easily return the correct command to run (i.e. with/without -O flag), but we should at least inform people that the flag might help.
Looking at Repology, it seems like OpenSSH 9.0 has not propagated very far yet:https://repology.org/project/openssh/versions
That indicates to me that a good default for now would be to display the command *without* `-O`, but add a note telling people to add it.
Medium-term supporting both protocols should be the right approach.
Looking at Repology, it seems like OpenSSH 9.0 has not propagated very far yet:
That indicates to me that a good default for now would be to display the command *without* `-O`, but add a note telling people to add it.
Medium-term supporting both protocols should be the right approach.
t....@gmail.com <t....@gmail.com> #10
> ...using a read-only FileSystem...
Actually, that might be a bit difficult. But you could make the whole SFTP subsystem read-only by adding a org.apache.sshd.contrib.server.subsystem.sftp.SimpleAccessControlSftpEventListener.READ_ONLY_ACCESSOR. Something like
...
SftpSubsystemFactory.Builder builder = new SftpSubsystemFactory.Builder();
builder.addSftpEventListener(SimpleAccessControlSftpEventListener.READ_ONLY_ACCESSOR);
daemon.setSubsystemFactories(Collections.singletonList(builder.build())); // Actually, add the SFTP factory to whatever other factories there are.
daemon.setFileSystemFactory(new VirtualFileSystemFactory(etcToolsPath));
...
The rest from #comment6 stands: dump the in-JAR resources into etc/tools (with replacements done already), then you should be able to use the Apache MINA sshd built-in support for SFTP without further ado. Or use some other directory (maybe a tmp dir somewhere, cleaned up when the Gerrit process exits?).
Actually, that might be a bit difficult. But you could make the whole SFTP subsystem read-only by adding a org.apache.sshd.contrib.server.subsystem.sftp.SimpleAccessControlSftpEventListener.READ_ONLY_ACCESSOR. Something like
...
SftpSubsystemFactory.Builder builder = new SftpSubsystemFactory.Builder();
builder.addSftpEventListener(SimpleAccessControlSftpEventListener.READ_ONLY_ACCESSOR);
daemon.setSubsystemFactories(Collections.singletonList(builder.build())); // Actually, add the SFTP factory to whatever other factories there are.
daemon.setFileSystemFactory(new VirtualFileSystemFactory(etcToolsPath));
...
The rest from
ek...@google.com <ek...@google.com> #11
[Monorail components: Backend]
th...@sap.com <th...@sap.com> #12
Adding support for SFTP is definitely the best solution.
While this is worked on, I would suggest to add another download command for CloneWIthCommitMsgHook for users of the new OpenSSH version. This should be far easier and less disruptive (could maybe be added to the stable-branches of the download commands plugin). This would be easier to see for users than in the commit validation message. However, it would be obsolete as soon as SFTP is supported.
While this is worked on, I would suggest to add another download command for CloneWIthCommitMsgHook for users of the new OpenSSH version. This should be far easier and less disruptive (could maybe be added to the stable-branches of the download commands plugin). This would be easier to see for users than in the commit validation message. However, it would be obsolete as soon as SFTP is supported.
kr...@google.com <kr...@google.com> #13
Interestingly, in response to the Git push request, Gerrit informs the user about both the `scp` and `curl` command lines.
However, on the Gerrit "Repo settings" screen, for examplehttps://gerrit-review.git.corp.google.com/admin/repos/gerrit,general I have noticed that it only gives one choice. In this case, it seems to be `curl`.
Is this choice configurable? If so, then I could simply choose not to display `scp` anymore, working around this error. Or, could I choose to display both the `curl` and the `scp` command lines, which Gerrit does when talking to Git directly (in the error message received. when you try to do `git push` without having the `commit-msg` hook installed).
Also, as a related suggestion, it would be great to have another copyable command line on this screen, for a common use case: wanting to add Gerrit to an existing repository that you have already cloned. This would use `git remote add` syntax, instead of `git clone` syntax.
However, on the Gerrit "Repo settings" screen, for example
Is this choice configurable? If so, then I could simply choose not to display `scp` anymore, working around this error. Or, could I choose to display both the `curl` and the `scp` command lines, which Gerrit does when talking to Git directly (in the error message received. when you try to do `git push` without having the `commit-msg` hook installed).
Also, as a related suggestion, it would be great to have another copyable command line on this screen, for a common use case: wanting to add Gerrit to an existing repository that you have already cloned. This would use `git remote add` syntax, instead of `git clone` syntax.
ha...@gmail.com <ha...@gmail.com> #14
This issue is also present on Fedora 37 which uses openSSH 8.8, which uses sftp by default (-O workaround works)
na...@linaro.org <na...@linaro.org> #17
Should we note that admins can implement 359823 themselves now using the gerrit.installCommitMsgHookCommand config?
sv...@axis.com <sv...@axis.com> #18
I guess you just did ;-)
Or where do you mean that this should be advertised?
Or where do you mean that this should be advertised?
gi...@appspot.gserviceaccount.com <gi...@appspot.gserviceaccount.com> #19
The following revision refers to this bug:
https://gerrit.googlesource.com/plugins/download-commands/+/90b37d2cad10c37661abb29587ac4ccec8775727
commit 90b37d2cad10c37661abb29587ac4ccec8775727
Author: Sven Selberg <svense@axis.com>
Date: Tue Feb 21 09:20:11 2023
Always use http to get commit-msg hook
Http should always be available from primary servers and using scp
is getting complicated since the implementation of scp varies between
different OpenSSH versions.
Bug:https://crbug.com/gerrit/16705
Bug:https://crbug.com/gerrit/15944
Change-Id: Ifb61fa66325d771cc606814f38af4d99d0d68c87
[modify]https://gerrit.googlesource.com/plugins/download-commands/+/90b37d2cad10c37661abb29587ac4ccec8775727/src/test/java/com/googlesource/gerrit/plugins/download/command/CloneWithCommitMsgHookTest.java
[modify]https://gerrit.googlesource.com/plugins/download-commands/+/90b37d2cad10c37661abb29587ac4ccec8775727/src/main/java/com/googlesource/gerrit/plugins/download/command/CloneWithCommitMsgHook.java
commit 90b37d2cad10c37661abb29587ac4ccec8775727
Author: Sven Selberg <svense@axis.com>
Date: Tue Feb 21 09:20:11 2023
Always use http to get commit-msg hook
Http should always be available from primary servers and using scp
is getting complicated since the implementation of scp varies between
different OpenSSH versions.
Bug:
Bug:
Change-Id: Ifb61fa66325d771cc606814f38af4d99d0d68c87
[modify]
[modify]
sv...@axis.com <sv...@axis.com> #20
Since the download-commands only uses download-command APIs the latest version on master should work for Gerrit versions as far back as 2.15 (no guarantees though).
gi...@appspot.gserviceaccount.com <gi...@appspot.gserviceaccount.com> #21
The following revision refers to this bug:
https://gerrit.googlesource.com/homepage/+/d5418764f357de3439fc69afe6036a2f6f40d883
commit d5418764f357de3439fc69afe6036a2f6f40d883
Author: Sven Selberg <svense@axis.com>
Date: Tue Feb 21 09:39:50 2023
3.8.md : release note for http download of commit-msg hook
Bug:https://crbug.com/gerrit/16705
Bug:https://crbug.com/gerrit/15944
Change-Id: I85073c021d461829e0354724c221fce49198a4fb
[modify]https://gerrit.googlesource.com/homepage/+/d5418764f357de3439fc69afe6036a2f6f40d883/pages/site/releases/3.8.md
commit d5418764f357de3439fc69afe6036a2f6f40d883
Author: Sven Selberg <svense@axis.com>
Date: Tue Feb 21 09:39:50 2023
Bug:
Bug:
Change-Id: I85073c021d461829e0354724c221fce49198a4fb
[modify]
gi...@appspot.gserviceaccount.com <gi...@appspot.gserviceaccount.com> #22
The following revision refers to this bug:
https://gerrit.googlesource.com/gerrit/+/34b4352f73417572ce4b6d02bccfe54bbe98ee64
commit 34b4352f73417572ce4b6d02bccfe54bbe98ee64
Author: Sven Selberg <svense@axis.com>
Date: Thu Mar 02 17:20:06 2023
Always track master on download-commands plugin
The download-commands plugin only uses APIs that were extracted
specifically for the download-commands plugin and very rarely
changes. It doesn't give much benefit to let older releases use
an older version since the only effect is that they get an
outdated plugin.
Also makes the ctor of DownloadConfig public for tests as this was
the only API breakage from master (f0c9b9e).
Update plugins/download-commands to latest master.
Changes since v3.5.5:
* b83ce67 Consistently use `git rev-parse` to find hooks-dir
* 90b37d2 Always use http to get commit-msg hook
* f0c9b9e Add test for CloneWithCommitMsgHook
* a16ebc6 Merge "Annotate methods that return a definitely null value with @Nullable"
* 107b225 Annotate methods that return a definitely null value with @Nullable
* a649bdb Prefer ImmutableList to the convenience methods in List
* 71331e1 Throw IllegalStateException instead of RuntimeException
* 7f73617 URL encode username in http and ssh schemes
* 1a30359 Adapt to enabling error level for the UnnecessaryParentheses bug pattern
* 6f58c1e document the new branch & reset commands
Bug:https://crbug.com/gerrit/15944
Release-Notes: Update plugins/download-commands to latest master
Change-Id: Ie41500963d425ade312e5bb4c4a2f4ba7f4f144d
[modify]https://gerrit.googlesource.com/gerrit/+/34b4352f73417572ce4b6d02bccfe54bbe98ee64/.gitmodules
[modify]https://gerrit.googlesource.com/gerrit/+/34b4352f73417572ce4b6d02bccfe54bbe98ee64/plugins/download-commands
[modify]https://gerrit.googlesource.com/gerrit/+/34b4352f73417572ce4b6d02bccfe54bbe98ee64/java/com/google/gerrit/server/config/DownloadConfig.java
commit 34b4352f73417572ce4b6d02bccfe54bbe98ee64
Author: Sven Selberg <svense@axis.com>
Date: Thu Mar 02 17:20:06 2023
Always track master on download-commands plugin
The download-commands plugin only uses APIs that were extracted
specifically for the download-commands plugin and very rarely
changes. It doesn't give much benefit to let older releases use
an older version since the only effect is that they get an
outdated plugin.
Also makes the ctor of DownloadConfig public for tests as this was
the only API breakage from master (f0c9b9e).
Update plugins/download-commands to latest master.
Changes since v3.5.5:
* b83ce67 Consistently use `git rev-parse` to find hooks-dir
* 90b37d2 Always use http to get commit-msg hook
* f0c9b9e Add test for CloneWithCommitMsgHook
* a16ebc6 Merge "Annotate methods that return a definitely null value with @Nullable"
* 107b225 Annotate methods that return a definitely null value with @Nullable
* a649bdb Prefer ImmutableList to the convenience methods in List
* 71331e1 Throw IllegalStateException instead of RuntimeException
* 7f73617 URL encode username in http and ssh schemes
* 1a30359 Adapt to enabling error level for the UnnecessaryParentheses bug pattern
* 6f58c1e document the new branch & reset commands
Bug:
Release-Notes: Update plugins/download-commands to latest master
Change-Id: Ie41500963d425ade312e5bb4c4a2f4ba7f4f144d
[modify]
[modify]
[modify]
sv...@axis.com <sv...@axis.com> #23
[Empty comment from Monorail migration]
Description
What steps will reproduce the problem?
1. make sure you use OpenSSH 9.0
2. copy de "Clone with commit-msg hook" command from Gerrit UI (it should also reproduce with the scp command from a push without Change-Id)
3. paste it to a terminal
What is the expected output?
commit-msg hook should be successfully retrieved
What do you see instead?
scp command fails with
Please provide any additional information below.
OpenSSH 9.0 changed the default behavior of SCP to use the SFTP protocol rather than the legacy scp/rcp protocol:https://www.openssh.com/releasenotes.html
Adding -O to the command fixes it. Either Gerrit should add it to the suggested commands (easy fix), or it should implement de sftp subsystem (sounds much more involved).