Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Weak support for Diffie Hellman groups for SSH KEX #1320

Closed
hiddeco opened this issue Apr 22, 2021 · 5 comments
Closed

Weak support for Diffie Hellman groups for SSH KEX #1320

hiddeco opened this issue Apr 22, 2021 · 5 comments
Labels
area/bootstrap Bootstrap related issues and pull requests area/git Git related issues and pull requests enhancement New feature or request
Milestone

Comments

@hiddeco
Copy link
Member

hiddeco commented Apr 22, 2021

Discussion ref: #1319

We do at present only support a very limited set of Diffie Hellman groups as SSH KEX algorithms, and based on the data gathered from the discussion above, this becomes an issue when people have hardened their on-primes Git instances.

Our options to extend support for more groups is limited, as we depend on golang/crypto implementations.

Based on the information from the discussion above, I see two options:

  1. Provide configuration flags in the flux CLI and controllers or related (Custom Resource Definition) APIs to extend the configuration of default supported KEX algorithms.
  2. Add diffie-hellman-group-exchange-sha256 to our own defaults in all SSH clients across the project, but as the least preferred option. This means the algorithm will only be selected if no other option is found when the client and server exchange supported algorithms.

Because the list of supported algorithms in golang/crypto is almost static, and the work required to implement option 1 takes a lot more time than option 2, my personal preference is to go with the latter option, which should automagically resolve things for most people.

@hiddeco hiddeco added enhancement New feature or request area/git Git related issues and pull requests area/bootstrap Bootstrap related issues and pull requests labels Apr 22, 2021
@hiddeco
Copy link
Member Author

hiddeco commented Apr 27, 2021

Rough sketch of the steps that would be required to implement option 2:

  • Add a public PreferredKexAlgos variable to the ssh package in fluxcd/pkg, this variable should match the preferredKexAlgos from golang/crypto but include kexAlgoDHGEXSHA256 at the end (list of string values available here: https://github.com/golang/crypto/blob/0c34fe9e7dc2486962ef9867e3edb3503537209f/ssh/kex.go#L23-L34).
  • Optionally: add a helper (SetPreferredKeyAlgos or MakeDefaultClientConfig) to this same package that makes it easier to configure the ClientConfig.Config.KeyExchanges on a given ClientConfig.
  • Use the PreferredKexAlgos or the added helper in the Flux components that make use of the ssh.ClientConfig, from the top of my head these are (should be taken care of in item order):
    • fluxcd/source-controller
    • fluxcd/image-automation-controller
    • fluxcd/flux2
      NB: sometimes the usage of ssh.ClientConfig may be hidden, source-controller for example largely makes it look like it depends on go-git/transport/ssh, but this package uses golang/crypto.

Some more details about what you are actually configuring, because it is always handy to know what exactly you are fiddling with, besides that you make it work:

@stefanprodan
Copy link
Member

@hiddeco was this covered in #1443 ?

@hiddeco
Copy link
Member Author

hiddeco commented May 28, 2021

No, this is another issue.

@0x6a77
Copy link

0x6a77 commented Nov 15, 2021

maybe this workaround is helpful for anyone up against this problem (#1319)

after research into a possible p/r here, i didn't see a way to access ssh.config from fluxcd — it seems like this has to get solved in go-git, or better yet, golang/crypto should support ssh_config. (tho' i might be wrong.) in the meantime i got this to work with fluxcd/terraform-provider-flux changes!

the basic strategy is to switch to libgit2 and "proto-sed" a configmap-based /etc/ssh_config volume mount into the sync manifests as they get processed. each pod then gets an /etc/ssh/ssh_config file with the needed keyex changes. @hiddeco it might be cool to use the HelmRelease configmap volume mount idiom in flux_sync — i bet it will be useful to allow easy volume mounting into the flux install and sync pods

data "flux_sync" "main" {
  target_path        = var.flux_paths
  url                = var.flux_git_url
  branch             = var.git_branch_name
  git_implementation = "libgit2"
}

resource "kubectl_manifest" "sync" {
  for_each   = {
  for v in local.sync : lower(join("/", compact([
    v.data.apiVersion,
    v.data.kind,
    lookup(v.data.metadata, "namespace", ""),
    v.data.metadata.name]))) => replace(
                                  v.content,
                                  "/(?sm)(.*volumeMounts:)(.*volumes:)(.*)/",
                                  "$${1}\n        - mountPath: /etc/ssh/\n          name: ssh-config\n          readOnly: true$${2}\n      - configMap:\n          name: ssh-config\n        name: ssh-config$${3}"
                                )
  }
  yaml_body  = each.value
}

resource "kubernetes_config_map" "ssh-config" {
  metadata {
    name      = "ssh-config"
    namespace = "flux-system"
  }

  data = {
    ssh_config = <<EOF
Host *
  SendEnv LANG LC_*
  #Legacy changes
  KexAlgorithms +diffie-hellman-group-exchange-sha256
EOF
  }
}

@pjbgf pjbgf added this to the GA milestone May 6, 2022
@pjbgf
Copy link
Member

pjbgf commented May 6, 2022

This is now fixed based on:

Closing as there seem to be nothing else left to do here, apart from the occasional update to the latest golang.org/x/crypto, which is already done as part of the project's housekeeping.

@pjbgf pjbgf closed this as completed May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/bootstrap Bootstrap related issues and pull requests area/git Git related issues and pull requests enhancement New feature or request
Projects
Status: Done
Development

No branches or pull requests

4 participants