After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 790910 - ssh-agent interface does not support SHA2 extension
ssh-agent interface does not support SHA2 extension
Product: gnome-keyring
Classification: Core
Component: ssh-agent
git master
Other Linux
: Normal normal
: ---
Assigned To: GNOME keyring maintainer(s)
GNOME keyring maintainer(s)
Depends on:
Reported: 2017-11-27 17:09 UTC by Jakub Jelen
Modified: 2018-06-30 17:03 UTC
See Also:
GNOME target: ---
GNOME version: ---

Support SHA2 extension for RSA signatures (8.53 KB, patch)
2017-11-27 17:09 UTC, Jakub Jelen
none Details | Review
Support SHA2 extension for RSA signatures v2 (13.05 KB, patch)
2017-11-28 13:21 UTC, Jakub Jelen
accepted-commit_now Details | Review

Description Jakub Jelen 2017-11-27 17:09:40 UTC
Created attachment 364516 [details] [review]
Support SHA2 extension for RSA signatures

OpenSSH since version 7.2 (Feb 2016) [0] is using SHA2 extension to provide RSA signatures using safe hash algorithm [1]. This extension is not negotiated with the ssh-agents and so far it worked because of the bug in OpenSSH signature verification [2], but it will hopefully be soon fixed and at that time, RSA signatures provided be gnome-keyring will stop working (because it is ignoring the passed flags).

The attached patch adds a support to provide these signatures.

The testsuite is adjusted accordingly and passes. Also the behavior was verified against upstream OpenSSH with the patch attached to the OpenSSH bug.

Comment 1 Daiki Ueno 2017-11-28 11:55:46 UTC
Review of attachment 364516 [details] [review]:

Thank you, nice.  I only have a stylistic comment about the gkd_ssh_agent_proto_algo_to_keytype function.

::: daemon/ssh-agent/gkd-ssh-agent-proto.c
@@ +151,2 @@
 const gchar*
+gkd_ssh_agent_proto_algo_to_keytype (gulong algo, GQuark curve_oid, GChecksumType halgo)

This interface is becoming a bit bloated; curve_oid is only relevant to ECC, and halgo is only relevant to RSA.  It is not obvious how this function is supposed to work, by only looking at the caller.

I would suggest to either:
- expand this at the caller side, or
- split this function to:
  gkd_ssh_agent_proto_rsa_algo_to_keytype(algo, halgo)
  gkd_ssh_agent_proto_ec_algo_to_keytype(algo, curve_oid)

This would increase the code complexity, but I guess it would be acceptable amount, given that there are only two places where the function is called.

@@ +682,3 @@
+	salgo = gkd_ssh_agent_proto_algo_to_keytype (algo, oid, 0);

Here, 0 == G_CHECKSUM_MD5.  As I mentioned above, it's not obvious how gkd_ssh_agent_proto_algo_to_keytype() treats that value without seeing the function definition.
Comment 2 Jakub Jelen 2017-11-28 13:21:58 UTC
Created attachment 364557 [details] [review]
Support SHA2 extension for RSA signatures v2

Thank you for the review. It was a bit cumbersome.

I revised it and expanded the problematic parts little bit. And created separate functions for separate key types and moved the handling of them into keytype-specific places. It makes the code little bit longer, but hopefully more understandable.

For the gkd_ssh_agent_proto_write_public(), now I used directly SHA1 constant (default), rather than bogus 0, since this is not identification of signature algorithm, but simple raw key type of the same name.

I hope it is more clear now.
Comment 3 Daiki Ueno 2017-11-29 14:10:04 UTC
Review of attachment 364557 [details] [review]:

Thank you, looks fine.
Comment 4 Daiki Ueno 2017-11-29 14:12:40 UTC
(In reply to Jakub Jelen from comment #0)

> ... but it will hopefully be soon fixed and
> at that time, RSA signatures provided be gnome-keyring will stop working
> (because it is ignoring the passed flags).

Does that mean we should backport the patch to the stable branch?
Comment 5 Jakub Jelen 2017-11-29 14:15:25 UTC
I don't think it is needed. Next regular release should be fine, since the change in OpenSSH will not be earlier than in their next release (in 3 to 6 months I guess) and then there will be some time before distributions will update if they ever do.
Comment 6 Daiki Ueno 2017-11-29 16:29:57 UTC
OK, thanks for the confirmation.  Pushed as: