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

Removing sh -c wrapper around the bootstrapper command string #8885

Conversation

Nimesh-Msys
Copy link
Contributor

@Nimesh-Msys Nimesh-Msys commented Sep 13, 2019

Avoids leaking of the secret key on console output while bootstrapping

Signed-off-by: Nimesh-Msys nimesh.patni@msystechnologies.com

Description

  • Train handles it and they are no more required here
  • Tested manually found no impacts
  • Ensured the test cases

Related Issue

#8859
#3887

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

  • I have read the CONTRIBUTING document.
  • I have run the pre-merge tests locally and they pass.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commits have been signed-off for the Developer Certificate of Origin.

@Nimesh-Msys Nimesh-Msys requested review from a team as code owners September 13, 2019 13:17
@lamont-granquist
Copy link
Contributor

I think this is a breaking change and that we should still support the validation key @marcparadise ?

The issue is that we should remove the sh -c since I think we now ship the script over scp and don't execute it entirely over ssh? And that way we keep all of the contents of the file off of the ps listing, which this change wouldn't accomplish.

@Nimesh-Msys Nimesh-Msys changed the title Removing validation_key from bootstrap templates [Do Not Merge] Removing validation_key from bootstrap templates Sep 19, 2019
@Nimesh-Msys
Copy link
Contributor Author

Tested on the latest version (tested on 15.4.2), it does not display the keys. Checking the prescribed changes for prior versions.

- It avoids security leakage
- Also allows handling single quotes

Signed-off-by: Nimesh-Msys <nimesh.patni@msystechnologies.com>
@Nimesh-Msys Nimesh-Msys force-pushed the Nimesh/MSYS-1095_remove_validation_key branch from 7313708 to 6d8fa46 Compare September 26, 2019 06:38
@Nimesh-Msys Nimesh-Msys changed the title [Do Not Merge] Removing validation_key from bootstrap templates Removing sh -c wrapper around the bootstrapper command string Sep 26, 2019
@Nimesh-Msys
Copy link
Contributor Author

The issue is that we should remove the sh -c since I think we now ship the script over scp and don't execute it entirely over ssh? And that way we keep all of the contents of the file off of the ps listing, which this change wouldn't accomplish.

Thanks for the review. Have made the required changes and tested.

@lock
Copy link

lock bot commented Oct 15, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants