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

Fix #159 - more secure credential handling #176

Merged
merged 2 commits into from
May 3, 2017
Merged

Fix #159 - more secure credential handling #176

merged 2 commits into from
May 3, 2017

Conversation

adobeDan
Copy link
Contributor

@adobeDan adobeDan commented May 3, 2017

Also fixed #175 while I was in there.

adobeDan added 2 commits May 2, 2017 21:28
* Convention: a filename of $(...) is actually a process specification.
* The process is executed in the current directory of the containing config file.
* The standard output of the process is used as the config file content and parsed.
* The process specification is handed to the platform shell for execution.
* This was introduced when we first "defaulted" the `--users` argument (a long time ago).
* The fix is to act as if *no* `--users` argument is `--users` by itself, because what the defaulting code really did was to default the argument to `all`.
@adobeDan adobeDan requested review from phil-levy and ianmak May 3, 2017 05:07
@adobeDan
Copy link
Contributor Author

adobeDan commented May 3, 2017

@phil-levy I believe we are now code-complete for the 2.1 release. We need docs, to bump the version number, to update the version history file, and to add some example files showing how to use the new feature. I leave those to you; I've attached a user-sync-config.yml that shows the process config feature; to make this run on windows, change the cat command to a type command. We might want to actually do a sample program that reads the config from a secure S3 and/or Azure file using instance access roles.

Copy link
Contributor

@phil-levy phil-levy left a comment

Choose a reason for hiding this comment

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

The only question I had was around unicode returned in the byte string and whether that would cause us any problems.

@adobeDan
Copy link
Contributor Author

adobeDan commented May 3, 2017

The assumption is that the pipe will return data in the platform-specific charset and the YAML parser will accept that. It's really between the YAML parser and the process as to what platform-specific charsets are understood; the use of bytes gets away from Python having to understand which one is in use. The customer may have to adjust what their binary returns if they see inconsistencies.

I need to take a look at #167 to see what's happening there, but it shouldn't relate to this.

@adobeDan adobeDan merged commit 9088d63 into v2 May 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--users value doesn't default to all but to None
2 participants