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

CentOS does not support ed25519; fixes #98 #151

Merged
merged 5 commits into from
Feb 18, 2019

Conversation

alxwr
Copy link
Member

@alxwr alxwr commented Feb 12, 2019

After refactoring map.jinja it should now be easy to exclude certain host key algorithms for older systems.

If this PR is accepted and merged, I plan on implementing an opt-out (use_check_cmd: False) to solve #147.

Tested on Ubuntu 18.04 and FreeBSD 11.2, but I don't have a CentOS 6 here. (Should work though.)

@hudecof @kadogo Could you please test this PR?

@alxwr alxwr self-assigned this Feb 12, 2019
@hudecof
Copy link

hudecof commented Feb 12, 2019

@alxwr unfortunate I gave up saltstack and returned to ansible, so my dev environment do not exists any more ;(

@myii
Copy link
Member

myii commented Feb 12, 2019

Have performed some basic testing limited to the YAML files and map.jinja refactoring. Tested on an Ubuntu minion with pillar.example. Comparing before and after this PR gives:

--- master
+++ alxwr:issue-98
@@ -72,6 +72,7 @@
   "generate_ed25519_keys": false,
   "generate_rsa_keys": false,
   "generate_rsa_size": 4096,
+  "host_key_algos": "ecdsa,ed25519,rsa",
   "known_hosts": {
     "aliases": [
       "cname-to-minion.example.org",
@@ -129,6 +130,55 @@
   "sshd_config_src": "salt://openssh/files/sshd_config",
   "sshd_config_user": "root",
   "sshd_enable": true
 }
+{
+  "Hosts": {
+    "*": {
+      "AddressFamily": "any",
+      "BatchMode": "yes",
+      "CheckHostIP": "yes",
+      "Cipher": "3des",
+      "Ciphers": [
+        "chacha20-poly1305@openssh.com",
+        "aes256-gcm@openssh.com",
+        "aes128-gcm@openssh.com",
+        "aes256-ctr",
+        "aes192-ctr",
+        "aes128-ctr"
+      ],
+      "ConnectTimeout": 0,
+      "ForwardAgent": false,
+      "ForwardX11": false,
+      "GSSAPIAuthentication": false,
+      "GSSAPIDelegateCredentials": false,
+      "HostbasedAuthentication": false,
+      "IdentityFile": "~/.ssh/id_rsa",
+      "KexAlgorithms": [
+        "curve25519-sha256@libssh.org",
+        "diffie-hellman-group-exchange-sha256",
+        "diffie-hellman-group-exchange-sha1",
+        "diffie-hellman-group14-sha1"
+      ],
+      "MACs": [
+        "hmac-sha2-512-etm@openssh.com",
+        "hmac-sha2-256-etm@openssh.com",
+        "umac-128-etm@openssh.com",
+        "hmac-sha2-512",
+        "hmac-sha2-256",
+        "umac-128@openssh.com"
+      ],
+      "PasswordAuthentication": true,
+      "PermitLocalCommand": "no",
+      "Port": 22,
+      "Protocol": 2,
+      "RSAAuthentication": true,
+      "RhostsRSAAuthentication": false,
+      "StrictHostKeyChecking": false,
+      "Tunnel": "no",
+      "TunnelDevice": "any:any",
+      "VisualHostKey": "no"
+    }
+  }
+}
 {
   "AcceptEnv": "LANG LC_*",

Looking at the additions, these are both expected due to:

  1. host_key_algos added to defaults.yaml.
  2. ssh_config also being exported from map.jinja.

So no regressions detected for this part of the refactoring, according to this limited testing at least.

Good work, @alxwr!

@alxwr
Copy link
Member Author

alxwr commented Feb 12, 2019

I may have found a bug when using salt-ssh. defaults.merge does not seem to work.

@myii
Copy link
Member

myii commented Feb 12, 2019

@alxwr @aboe76 @LloydArmstrong It looks like saltstack-formulas/redis-formula#75 really is a problem again re: salt-ssh vs. defaults.merge. Is this a bug upstream?

@alxwr
Copy link
Member Author

alxwr commented Feb 12, 2019

@aboe76 @myii @javierbertoli This PR now works with both salt and salt-ssh.

@myii
Copy link
Member

myii commented Feb 12, 2019

@alxwr Would you be able to confirm that defaults.merge is not available, like in this comment here:

@alxwr
Copy link
Member Author

alxwr commented Feb 12, 2019

@alxwr Would you be able to confirm that defaults.merge is not available, like in this comment here:

Yes. I got the same message in my debug log:

[DEBUG   ] Could not LazyLoad defaults.merge: 'defaults.merge' is not available.

@myii
Copy link
Member

myii commented Feb 12, 2019

@alxwr Upstream bug report: saltstack/salt#51605.

@alxwr
Copy link
Member Author

alxwr commented Feb 12, 2019

@myii Thanks!

@alxwr
Copy link
Member Author

alxwr commented Feb 12, 2019

@myii I'm running salt-ssh from a FreeBSD machine against a Debians minion.

% salt --versions-report
Salt Version:
           Salt: 2018.3.3
 
Dependency Versions:
           cffi: 1.11.5
       cherrypy: Not Installed
       dateutil: Not Installed
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.10
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: 0.31.0
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: 1.2.5
      pycparser: 2.18
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.15 (default, Dec 27 2018, 00:23:42)
   python-gnupg: Not Installed
         PyYAML: 3.13
          PyZMQ: 17.1.2
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.2.3
 
System Versions:
           dist:   
         locale: UTF-8
        machine: amd64
        release: 11.2-RELEASE-p8
         system: FreeBSD
        version: Not Installed

@myii
Copy link
Member

myii commented Feb 12, 2019

@alxwr Thanks, I've updated the upstream report.

I've also run the same test again for map.jinja and everything is still merging the with the same expected diff as above.

Copy link
Member

@aboe76 aboe76 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on my setup and I see no issues.

@aboe76
Copy link
Member

aboe76 commented Feb 12, 2019

@javierbertoli ping

@aboe76
Copy link
Member

aboe76 commented Feb 12, 2019

@alxwr I like this map.jinja style it looks like a merger between new and old style...

@alxwr
Copy link
Member Author

alxwr commented Feb 12, 2019

@alxwr I like this map.jinja style it looks like a merger between new and old style...

Well, it actually is. 😄 Trying to take the best of both worlds.

Copy link
Member

@javierbertoli javierbertoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It LGTM. Really nice work, @alxwr !

@myii myii merged commit 3715cd6 into saltstack-formulas:master Feb 18, 2019
@myii
Copy link
Member

myii commented Feb 18, 2019

Merged. Thanks for the PR, @alxwr. And to @aboe76 and @javierbertoli for the reviews.

@alxwr alxwr deleted the issue-98 branch March 2, 2019 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CentOS 6: check_cmd broken/unsupported key type ed25519
5 participants