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

[BUG] ssh_auth.present.source doesn't work with key types containing @ or . #61299

Open
FineTralfazz opened this issue Nov 29, 2021 · 9 comments
Assignees
Labels
Bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around
Milestone

Comments

@FineTralfazz
Copy link

Description
When using a key with a type containing @ or . the regex doesn't correctly capture the key, so it doesn't end up on the target.

Setup
SLS file:

christopher ssh keys:
  ssh_auth.present:
    - user: christopher
    - source: salt://configs/authorized_keys

root ssh keys:
  ssh_auth.present:
    - user: root
    - source: salt://configs/authorized_keys

Contents of salt://configs/authorized_keys:

ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBHw4SdWB/ga87fmiPUX/4gDjW3xEIMq48zCflXPskFeuC42abrVrN0T+WaRYmWWvvXN2zkCvutYP9hMkZE3e7og= Black5CKey
sk-ssh-ed25519@openssh.com AAAAGnNrLXNzaC1lZDI1NTE5QG9wZW5zc2guY29tAAAAIECqQqYa6bn+oM/jgGGSwjdicHPygDJEZlxz9kNZO2OMAAAABHNzaDo= BlueKey
ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDgP+qBR0GY+hCm+giREUsTphPyE89a1u85z7/Bjw9D6IFTMVx/uKRR0gAHCD7kT1QI2iy/9MJXtisnDfHPDjRUuHpzcrPrelIdVCT/WDUXMU4xeqJl5O4VOT/dtMvdWuq9g3g+Bt9E7CQQIZ2jmF6UjucaGjuJhy/hwDstMm8u03y2UOh45EBkp1d59p5zzOChHkitZJ2Xh8zyBNt5+hOCv+zYGzQDUwluHkQlLvdrYsTkIzcL3KNw55e7HPmXddPZoM1pQw0F+vm/CxlqtVRA3FIjcDFSHJHX03A1WrreDoH7G8nW5xTfD+PAzZfMqP60ii9Ug8bc6XEk03REgIK5 Static

The file that gets dropped on the target hosts:

ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBHw4SdWB/ga87fmiPUX/4gDjW3xEIMq48zCflXPskFeuC42abrVrN0T+WaRYmWWvvXN2zkCvutYP9hMkZE3e7og= Black5CKey
ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDgP+qBR0GY+hCm+giREUsTphPyE89a1u85z7/Bjw9D6IFTMVx/uKRR0gAHCD7kT1QI2iy/9MJXtisnDfHPDjRUuHpzcrPrelIdVCT/WDUXMU4xeqJl5O4VOT/dtMvdWuq9g3g+Bt9E7CQQIZ2jmF6UjucaGjuJhy/hwDstMm8u03y2UOh45EBkp1d59p5zzOChHkitZJ2Xh8zyBNt5+hOCv+zYGzQDUwluHkQlLvdrYsTkIzcL3KNw55e7HPmXddPZoM1pQw0F+vm/CxlqtVRA3FIjcDFSHJHX03A1WrreDoH7G8nW5xTfD+PAzZfMqP60ii9Ug8bc6XEk03REgIK5 Static

Steps to Reproduce the behavior
^ Use that SLS and authorized_keys file.

Expected behavior
The whole file is added to the target hosts.

Versions Report

salt --versions-report (Provided by running salt --versions-report. Please also mention any differences in master/minion versions.)
Salt Version:
          Salt: 3004

Dependency Versions:
          cffi: Not Installed
      cherrypy: Not Installed
      dateutil: 2.7.3
     docker-py: Not Installed
         gitdb: 2.0.6
     gitpython: 3.0.7
        Jinja2: 2.10.1
       libgit2: Not Installed
      M2Crypto: Not Installed
          Mako: Not Installed
       msgpack: 0.6.2
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     pycparser: Not Installed
      pycrypto: Not Installed
  pycryptodome: 3.6.1
        pygit2: Not Installed
        Python: 3.8.10 (default, Sep 28 2021, 16:10:42)
  python-gnupg: 0.4.5
        PyYAML: 5.3.1
         PyZMQ: 18.1.1
         smmap: 2.0.5
       timelib: Not Installed
       Tornado: 4.5.3
           ZMQ: 4.3.2

System Versions:
          dist: ubuntu 20.04 focal
        locale: utf-8
       machine: x86_64
       release: 5.4.0-88-generic
        system: Linux
       version: Ubuntu 20.04 focal

Additional context
I played around with the regex in ssh_auth.py and I think changing it to ^(.*?)\s?((?:sk-)?(?:ssh\-|ecds)[@\.\w-]+\s.+)$ would fix the problem. But I'm not familiar enough with the codebase to confidently submit a PR, so I'm hoping an active contributor can confirm and fix it.

@FineTralfazz FineTralfazz added Bug broken, incorrect, or confusing behavior needs-triage labels Nov 29, 2021
@welcome
Copy link

welcome bot commented Nov 29, 2021

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at saltproject@vmware.com. We’re glad you’ve joined our community and look forward to doing awesome things with you!

@OrangeDog
Copy link
Contributor

OrangeDog commented Nov 29, 2021

Does it work without the @openssh.com or is this some non-standard implementation that needs it?

It's not clear why the regex is written like that. I'd use something like

^(((?P<options>\S+)\s)?(?P<enc>\S+)\s)?(?P<key>[0-9A-Za-z/+=]+)(\s(?P<comment>.+))?$

@OrangeDog OrangeDog added severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around and removed needs-triage labels Nov 29, 2021
@FineTralfazz
Copy link
Author

I just tested and it only works with the @openssh.com suffix. This is a key type that was added about two years ago to support U2F/FIDO keys (more info here) and my guess is that the current implementation in OpenSSH-specific rather than part of a standard, so they added a suffix to denote that.

@OrangeDog OrangeDog added this to the Approved milestone Nov 30, 2021
@OrangeDog
Copy link
Contributor

OrangeDog commented Nov 30, 2021

It looks like providing a source with multiple keys has bugs of its own, as it calls set_auth_key_from_file with the whole file for every line, depending on whether there are options.

@HerHde
Copy link
Contributor

HerHde commented Jan 29, 2022

This should have been fixed along with #59429 by #60128

@svenseeberg
Copy link
Contributor

svenseeberg commented Aug 31, 2022

It seems that this does still not work as of v3005, at least for sk-ssh-ed25519@openssh.com

@r-lindner
Copy link

It also fails with - names instead of - source. In the names-code-branch the same regexes are use again, but without @. next to the \w.

@jamesharr
Copy link

Note that the duplicate #64723 issue mentions a couple points where a fix could be applied, including a couple suggested regexp changes

@b10n1k
Copy link

b10n1k commented May 27, 2024

I tried to reproduce the problem with the content provided by @FineTralfazz and the missing key now appears in the file. is already this fixed? or not how can I reproduce it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around
Projects
None yet
Development

No branches or pull requests

8 participants