Filter out SHA-1 key exchanges and MD5/truncated SHA-1 MACs

[daniel-beck]

By default, exclude SHA1 based key exchanges. Allow customizing which to exclude, and add logging to understand exactly what happens during filtering.

Exclusion of SHA1 based key exchanges has been implemented in newer versions of Mina according to https://github.com/apache/mina-sshd#implementedavailable-support but upgrading Mina seems like a bigger project (#33). So choose this approach instead.

If this looks like a reasonable approach, we probably want to disable hmac-md5, hmac-md5-96, and hmac-sha1-96 in a similar manner. They are also no longer available by default in Mina 2 (https://issues.apache.org/jira/browse/SSHD-1004).

[oleg-nenashev]

Looks good in principle, needs to be documented somewhere.

@daniel-beck please also request reviews from all core maintainers, not just me. I am quite loaded with other tasks, and I do not commit to provide timely reviews

[jtnord]

code looks OK to me, only comment is that we enabled ciphers, yet disable key-exchange - which seems inconsistent, but 🤷

[daniel-beck]

code looks OK to me, only comment is that we enabled ciphers, yet disable key-exchange - which seems inconsistent, but 🤷

Right, but that entire mechanism seems unfinished, and is only private (package-private for test) with no way for the outside to control it, so I didn't feel like letting myself be influenced too much by it.

[jglick]

upgrading Mina seems like a bigger project

Seems fine to do this as a temporary expedient; just leave in TODO comments reminding us to delete the obsolete code when sshd is converted to a plugin and given an upgrade.

[daniel-beck]

just leave in TODO comments reminding us to delete the obsolete code when sshd is converted to a plugin and given an upgrade.

This option wouldn't be the worst thing to leave in even then IMO. We'd need to commit to track Mina closely for us to likely not need something like this again.

[oleg-nenashev]

Yes, a conditional approval

…

On Fri, Sep 18, 2020 at 3:36 PM Daniel Beck ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/main/java/org/jenkinsci/main/modules/sshd/SSHD.java <#35 (comment)>: > @@ -44,6 +48,12 @@ private static final List<NamedFactory<Cipher>> ENABLED_CIPHERS = Arrays.<NamedFactory<Cipher>>asList( BuiltinCiphers.aes128ctr, BuiltinCiphers.aes192ctr, BuiltinCiphers.aes256ctr ); + + /** + * Comma-separated string of key exchange names to disable. Defaults to a list of DH SHA1 key exchanges, gets its value from ***@***.*** SystemProperties}. + */ + private static final String EXCLUDED_KEY_EXCHANGES = SystemProperties.getString(SSHD.class.getName() + ".excludedKeyExchanges", Is that conditional approval? I'm not writing docs if the entire thing gets shot down 😉 — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#35 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAW4RIFNSHDP2T3JPZKGU3TSGNO4VANCNFSM4RR2BOXQ> .

[jglick]

We'd need to commit to track Mina closely

Dependabot!

[jvz]

I follow Mina development nowadays ever since I contributed a few random things earlier this year (including the security report that raised that linked Jira ticket). Based on that, depending on which APIs you're using, upgrading between minor versions sometimes requires code updates (nothing too serious unless you're highly customizing things).

[jglick]

move to Release Drafter

[daniel-beck]

Way out of scope. The changelog file currently exists and GH releases are unused. I'm only adding this file because of #35 (comment)

[Wadeck]

Code looks fine, not tested

[oleg-nenashev]

Looks good to me, thanks @daniel-beck !