Skip to content

/salt

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

Fix issue #38366 salt-ssh support private key's passphrase #47238

Merged
merged 13 commits into from Jun 4, 2018

Conversation

Reviewers

@cachedout cachedout

@gtmanfred gtmanfred

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@pengyao @gtmanfred @rallytime @cachedout
@pengyao
Copy link
Contributor

commented Apr 23, 2018

What does this PR do?

Fix issue #38366

What issues does this PR fix or reference?

Support private key's passphrase for salt-ssh

Previous Behavior

Not support

New Behavior

Private key's passphrase could be configured by:

  • --priv-passwd for salt-ssh cli
  • salt_priv_passwd for salt master configure file
  • priv_passwd for salt roster file

@pengyao pengyao requested a review from saltstack/team-core as a code owner Apr 23, 2018

@gtmanfred

gtmanfred suggested changes Apr 23, 2018
View changes

Copy link
Contributor

left a comment

Can you add docs about this config option to the salt ssh topics page?
@cachedout

cachedout approved these changes Apr 23, 2018
View changes

@gtmanfred

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Apr 23, 2018

Also, adding this to the release notes for Fluorine would be greatly appreciated.

@rallytime rallytime referenced this pull request Apr 23, 2018

Closed

team-ssh is not being requested on appropriate salt PRs #17

@pengyao
Add docs for ssh_priv_passwd
Loading status checks…
39cb55a
@gtmanfred

gtmanfred suggested changes Apr 24, 2018
View changes

doc/man/salt-ssh.1 Outdated
@@ -177,6 +177,11 @@ Specify the SSH private key file to be used for authentication.
.UNINDENT
.INDENT 0.0
.TP
.B \-\-priv\-passwd=SSH_PRIV_PASSWD
Specify the SSH private key file's passphrase.

This comment has been minimized.

Sign in to view
Copy link
@gtmanfred

gtmanfred Apr 24, 2018

Contributor

Can you remove this change?

We compile these when they are packaged, it does not need to be updated.
@pengyao
Remove priv_passwd for salt-ssh man page 
It will be updated when packaged
Loading status checks…
9a96d98
@gtmanfred

gtmanfred approved these changes Apr 25, 2018
View changes

@rallytime
Merge branch 'develop' into ssh_private_key_pass
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
f3cecd5
@rallytime

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Apr 25, 2018

This change is causing many of the ssh tests to fail. @pengyao Can you take a look?
@pengyao
fixed a typo
Loading status checks…
3747a3d
@pengyao

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Apr 26, 2018

@rallytime It's my fault. I have fixed it.
@rallytime
Merge branch 'develop' into ssh_private_key_pass
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
ebd190d
@rallytime

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Apr 26, 2018

@pengyao Looks like one more test here needs to be addressed :)

https://jenkins.saltstack.com/job/PR/job/salt-pr-linode-ubuntu14-n/22289/
@cachedout

This comment has been minimized.

Sign in to view
Copy link
Collaborator

commented May 1, 2018

@pengyao Are you able to resolve that final failing test? We would really like to get this merged if we can. :)
Mike Place
Merge branch 'develop' into ssh_private_key_pass
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
3785ee1
@pengyao

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented May 3, 2018

@cachedout @rallytime Jenkins Error is:

No test reports found for the metric 'JUnit' with the resolved pattern 'artifacts/unittests/*.xml'. Configuration error?.

I think the error is not created by this commits
@rallytime

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented May 3, 2018

re-run py
@pengyao

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented May 21, 2018

@rallytime merge into develop branch?
@rallytime
Merge branch 'develop' into ssh_private_key_pass
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
5777854
@rallytime

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented May 21, 2018

@pengyao The following test is still failing:

  • unit.config.schemas.test_ssh.RosterEntryConfigTest.test_config

That will need to be fixed before we can merge this in.

https://jenkins.saltstack.com/job/PR/job/salt-pr-linode-ubuntu14-n/23028/

pengyao added some commits May 22, 2018

@pengyao
add priv_passwd property for Roster unittest
Loading status checks…
21beeb2
@pengyao
Security first
Loading status checks…
b6d5eb5
@rallytime

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented May 22, 2018

Thanks for fixing that @pengyao - that looks good.

I think an addition to the release notes (for Fluorine) would be good here as well. Can you add something for that? Then we can get this in :)
@rallytime

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented May 31, 2018

@pengyao We just need this added to the Fluorine release notes and then we can get this in.
@pengyao

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Jun 4, 2018

@rallytime The addition is added to the Fluorine release notes
@rallytime

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Jun 4, 2018

@pengyao Oh! You're right. I thought I double checked but I must have missed it. Thank you!

@rallytime rallytime merged commit 250226b into saltstack:develop Jun 4, 2018
4 of 10 checks passed

4 of 10 checks passed

jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #10352 — ABORTED
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #19436 — ABORTED
Details
codeclimate 2 issues to fix
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #5380 — FAILURE
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #23314 — FAILURE
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #25576 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #17649 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #22279 — SUCCESS
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.