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

Gpg #8

Merged
merged 9 commits into from Jun 4, 2012
Merged

Gpg #8

merged 9 commits into from Jun 4, 2012

Conversation

technomancy
Copy link
Contributor

This will use .netrc.gpg if present and if GPG is installed.

@technomancy
Copy link
Contributor Author

This should be good now; rebased on master as of 0.7.2.

@technomancy
Copy link
Contributor Author

Actually hold off; this doesn't do round-tripping, just reading.

@technomancy
Copy link
Contributor Author

Added support for saving, but I want to try to get someone with a security background to review it first.

raise Error.new("Decrypting #{path} failed.") unless $?.success?
new(path, parse(lex(decrypted.split("\n"))))
elsif File.exists?(path + ".gpg") && system("which gpg > /dev/null")
read(path + ".gpg")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this part needed? If so, shouldn't it still check_permissions and do new(path, parse(...))?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This allows it to transparently use the .gpg version of a netrc file if it's present. If we want the encrypted version only to be used when explicitly requested then we should remove this elsif branch. It's a judgement call. If we do leave it, I think recursing is the right thing here to avoid duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess you could make a good argument against transparently preferring the encrypted version since it makes it impossible to use the unencrypted version if the encrypted one is present. I'm OK with removing the implicit .gpg check if you think that's a problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can see both sides, are there precedents around this we can lean on to help make the call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think "don't add any magic that can't be disabled" is enough of a principle for me. Updated to remove the magic.

geemus pushed a commit that referenced this pull request Jun 4, 2012
@geemus geemus merged commit f11b5be into heroku:master Jun 4, 2012
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.

None yet

2 participants