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

ssh-auth becomes a chatty Cathy during test runs #5374

Closed
QuinnyPig opened this issue Jun 3, 2013 · 8 comments · Fixed by #6134
Closed

ssh-auth becomes a chatty Cathy during test runs #5374

QuinnyPig opened this issue Jun 3, 2013 · 8 comments · Fixed by #6134
Assignees
Labels
Bug broken, incorrect, or confusing behavior
Milestone

Comments

@QuinnyPig
Copy link
Contributor

[root@salt cquinn]# salt 'www*' state.highstate; salt 'www*' state.highstate test=True
www.epicquinn.net:
www.epicquinn.net:
----------
    State: - ssh_auth
    Name:      AAAAB3NzaC1yc2EAAAABIwAAAQEA0Cgc+ktd2DILpbjlv0mhnApFz0p2oDw2K1v3JtGraXh0ja+c3n52DSYiHXVrdoFG58me9P094DlZiFutkt7MRfBiH059MAj4LvzcoxVmFYz78REOytKp5zXe3SpTVoNxRVIqFCmql4xlnBL+sxHbsbH1HLaO3e2kbF0GkgE2zuLWJ3vf1DA0SpqzCoX6HMgMSkMXMQAGNXKJuOo01vm5sZR4vMiCxVOqa/YX9q5WwlJmMNTzTZ/fXtNSMQR46EBlzXNCXRWLnMiAjj2onyFOasvtdHdKJGBU7D0+OooKIQ3x7iDnCVoxHQHDyOUImMqg0/5NAoNKo9k6iS7JQ53Aww==
    Function:  present
        Result:    None
        Comment:   Key AAAAB3NzaC1yc2EAAAABIwAAAQEA0Cgc+ktd2DILpbjlv0mhnApFz0p2oDw2K1v3JtGraXh0ja+c3n52DSYiHXVrdoFG58me9P094DlZiFutkt7MRfBiH059MAj4LvzcoxVmFYz78REOytKp5zXe3SpTVoNxRVIqFCmql4xlnBL+sxHbsbH1HLaO3e2kbF0GkgE2zuLWJ3vf1DA0SpqzCoX6HMgMSkMXMQAGNXKJuOo01vm5sZR4vMiCxVOqa/YX9q5WwlJmMNTzTZ/fXtNSMQR46EBlzXNCXRWLnMiAjj2onyFOasvtdHdKJGBU7D0+OooKIQ3x7iDnCVoxHQHDyOUImMqg0/5NAoNKo9k6iS7JQ53Aww== for user cquinn is set to be updated
        Changes:   
----------
    State: - ssh_auth
    Name:      AAAAB3NzaC1yc2EAAAABIwAAAgEA6p1M3AocqeI5y7YlUXrOwv8U3CgkAp3MhC23+jsf0vakCBzLMpYBYUtH2JKRZS0JtyaGNX/+3kmFrP1zNEQgqT4zmkbren3tj7W7DGtzMnqb4XPFA/8aTbMS64c63CZQcoJcQcFLpg6mrpdCb7FG4gfqtM1SHsvqtw9pUuD/bV1BSFHBTpWlFVIeG5Wh92CajsZm2nIrR5Mn5jmGShFRz70aKS0UhVK4PjDbl+QntCKRbstAyII7ftYlmBo7GgXy/n5UmWejsOjJbEXLnSeXnMNLStZUDWtHwIS/IxYbMPOSklHzTEJiI3K6H5tkJrc5vlP3/QGBh36K16Hlc/2pvTPD9NIzU0kt1zI8G6WUo8Ug5L5AhyMoL5BpZMJX9wvDJKpYJkqY3vUlWS6XJiOOaTTgpoQ23sOdMQLZcdSPIZVo0uQxqP2mtP8c/eO5ra0yFMsbBfdAG4OH3k3ujcRS9xcpMZL4fNMWoj3qJHdalOsQazZvOYIkMOYTNShx1NV2CAk/beteiEFc99+EltRAUbA8gvLYyznINOaA0GcJjvz7XvKtEBp4UVGAhYqbOCwJectUroSSji/J3ybfFXsWndMGz1FaerLWihh9j6FhVJJvSAoDb5R34oisCBvrsl9XF1g2sjfjlDe16EW9+RHTMU8ktg7SqJWQFLwVO8RLksU=
    Function:  present
        Result:    None
        Comment:   Key AAAAB3NzaC1yc2EAAAABIwAAAgEA6p1M3AocqeI5y7YlUXrOwv8U3CgkAp3MhC23+jsf0vakCBzLMpYBYUtH2JKRZS0JtyaGNX/+3kmFrP1zNEQgqT4zmkbren3tj7W7DGtzMnqb4XPFA/8aTbMS64c63CZQcoJcQcFLpg6mrpdCb7FG4gfqtM1SHsvqtw9pUuD/bV1BSFHBTpWlFVIeG5Wh92CajsZm2nIrR5Mn5jmGShFRz70aKS0UhVK4PjDbl+QntCKRbstAyII7ftYlmBo7GgXy/n5UmWejsOjJbEXLnSeXnMNLStZUDWtHwIS/IxYbMPOSklHzTEJiI3K6H5tkJrc5vlP3/QGBh36K16Hlc/2pvTPD9NIzU0kt1zI8G6WUo8Ug5L5AhyMoL5BpZMJX9wvDJKpYJkqY3vUlWS6XJiOOaTTgpoQ23sOdMQLZcdSPIZVo0uQxqP2mtP8c/eO5ra0yFMsbBfdAG4OH3k3ujcRS9xcpMZL4fNMWoj3qJHdalOsQazZvOYIkMOYTNShx1NV2CAk/beteiEFc99+EltRAUbA8gvLYyznINOaA0GcJjvz7XvKtEBp4UVGAhYqbOCwJectUroSSji/J3ybfFXsWndMGz1FaerLWihh9j6FhVJJvSAoDb5R34oisCBvrsl9XF1g2sjfjlDe16EW9+RHTMU8ktg7SqJWQFLwVO8RLksU= for user cquinn is set to be updated
        Changes:  
@QuinnyPig
Copy link
Contributor Author

State file:

cquinn:
  group:
    - present
    - gid: 1500
  user:
    - present
    - shell: /bin/zsh
    - uid: 1500
    - gid: 1500
    - home: /home/cquinn
{% if grains['os'] == 'Ubuntu' %}
    - groups:
      - sudo
{% endif %}
    - require:
      - group: cquinn
cquinn-rsa:
  ssh_auth:
    - present
    - user: cquinn
    - enc: rsa
    - names:
      -  AAAAB3NzaC1yc2EAAAABIwAAAgEA6p1M3AocqeI5y7YlUXrOwv8U3CgkAp3MhC23+jsf0vakCBzLMpYBYUtH2JKRZS0JtyaGNX/+3kmFrP1zNEQgqT4zmkbren3tj7W7DGtzMnqb4XPFA/8aTbMS64c63CZQcoJcQcFLpg6mrpdCb7FG4gfqtM1SHsvqtw9pUuD/bV1BSFHBTpWlFVIeG5Wh92CajsZm2nIrR5Mn5jmGShFRz70aKS0UhVK4PjDbl+QntCKRbstAyII7ftYlmBo7GgXy/n5UmWejsOjJbEXLnSeXnMNLStZUDWtHwIS/IxYbMPOSklHzTEJiI3K6H5tkJrc5vlP3/QGBh36K16Hlc/2pvTPD9NIzU0kt1zI8G6WUo8Ug5L5AhyMoL5BpZMJX9wvDJKpYJkqY3vUlWS6XJiOOaTTgpoQ23sOdMQLZcdSPIZVo0uQxqP2mtP8c/eO5ra0yFMsbBfdAG4OH3k3ujcRS9xcpMZL4fNMWoj3qJHdalOsQazZvOYIkMOYTNShx1NV2CAk/beteiEFc99+EltRAUbA8gvLYyznINOaA0GcJjvz7XvKtEBp4UVGAhYqbOCwJectUroSSji/J3ybfFXsWndMGz1FaerLWihh9j6FhVJJvSAoDb5R34oisCBvrsl9XF1g2sjfjlDe16EW9+RHTMU8ktg7SqJWQFLwVO8RLksU=
      - AAAAB3NzaC1yc2EAAAABIwAAAQEA0Cgc+ktd2DILpbjlv0mhnApFz0p2oDw2K1v3JtGraXh0ja+c3n52DSYiHXVrdoFG58me9P094DlZiFutkt7MRfBiH059MAj4LvzcoxVmFYz78REOytKp5zXe3SpTVoNxRVIqFCmql4xlnBL+sxHbsbH1HLaO3e2kbF0GkgE2zuLWJ3vf1DA0SpqzCoX6HMgMSkMXMQAGNXKJuOo01vm5sZR4vMiCxVOqa/YX9q5WwlJmMNTzTZ/fXtNSMQR46EBlzXNCXRWLnMiAjj2onyFOasvtdHdKJGBU7D0+OooKIQ3x7iDnCVoxHQHDyOUImMqg0/5NAoNKo9k6iS7JQ53Aww==
    - require:
      - user: cquinn

@basepi
Copy link
Contributor

basepi commented Jun 3, 2013

Please remember to use code blocks (using the triple backtick/grave) to make it more readable. =)

And thanks for the report.

@basepi
Copy link
Contributor

basepi commented Jun 3, 2013

I assume your complaint here is that it prints the keys?

@QuinnyPig
Copy link
Contributor Author

Yes, sorry if that's unclear.

There are no changes that need to be made when the state is run, but it reports that the ssh keys will be changed. This has the unfortunate side effect of obscuring actual changes with very verbose lines of ssh pubkeys.

@basepi
Copy link
Contributor

basepi commented Jun 3, 2013

So if we were to change it to something like Key for user cquinn is set to be updated: fdjsklfhjeklwhfkljds... would that be more acceptable? Or what exactly are you looking for?

@QuinnyPig
Copy link
Contributor Author

In this context? I don't want to see anything at all about the ssh key, since it's a false positive. The ssh key is already in place, is already correct-- so why is Salt telling me that it's going to be updated in the "I'd change this" shade of yellow?

Remember, the invocation was:

[root@salt cquinn]# salt 'www*' state.highstate; salt 'www*' state.highstate test=True

No changes were made to the states during that run. :-)

@basepi
Copy link
Contributor

basepi commented Jun 3, 2013

Ohhhh, so if you leave off the test=True then it says no changes were made?

Probably just an oversight in the test=True part of the state. We'll look into it. =)

@terminalmage
Copy link
Contributor

@KB1JWQ I was able to replicate this. So, salt seems to support leaving off the ssh- from the beginning of the encoding type, but the encoding type does not match what is detected in the authorized_keys file when rsa is used in the SLS instead of ssh-rsa. Additionally, if the comment field is not specified in the SLS, then that None value does not match the existing comment and this also causes the key to appear to need changing.

I'll look at it a little closer and see what I can find. It's possible that in those occasions where rsa (as opposed to ssh-rsa) is used as the enc parameter in the SLS, that salt is still editing the authorized_keys file when editing is not necessary.

@ghost ghost assigned terminalmage Jun 14, 2013
terminalmage added a commit to terminalmage/salt that referenced this issue Jul 13, 2013
If 'rsa' or 'dss', etc. are used as the 'enc' value in ssh_auth.present
states (rather than 'ssh-rsa' or 'ssh-dss'), the call to
salt.modules.ssh.check_key() will fail, which causes ssh_auth.present
states to mistakenly report the key as needing to be changed. This
commit adds a call to salt.modules.ssh._refine_enc(), which normalizes
the 'enc' param so that the call to ssh.check_key() does not fail.

Fixes saltstack#5374.
terminalmage added a commit that referenced this issue Jul 16, 2013
If 'rsa' or 'dss', etc. are used as the 'enc' value in ssh_auth.present
states (rather than 'ssh-rsa' or 'ssh-dss'), the call to
salt.modules.ssh.check_key() will fail, which causes ssh_auth.present
states to mistakenly report the key as needing to be changed. This
commit adds a call to salt.modules.ssh._refine_enc(), which normalizes
the 'enc' param so that the call to ssh.check_key() does not fail.

Fixes #5374.
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
Projects
None yet
3 participants