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

password option in genkeys() #1

Closed
gadenbuie opened this issue Feb 25, 2019 · 7 comments

Comments

Projects
None yet
2 participants
@gadenbuie
Copy link

commented Feb 25, 2019

Thanks for your work on this package! I've been playing with it a bit and exploring the API and have two suggestions related to the setting of the password in genkeys().

The first is to suggest exposing the password argument in genkeys(). As currently implemented, it not possible for a user to programmatically generate keys due to the askpass() prompt.

encryptr/R/genkeys.R

Lines 33 to 37 in 4624dbc

openssl::write_pem(key,
"id_rsa",
password = openssl::askpass(prompt = "Please choose a password for your private key.
This password CANNOT be recovered if lost.
Please store the password in a safe location."))

You could easily refactor the password prompt into

ask_pass <- function() {
  askpass::askpass(prompt = paste(
    "Please choose a password for your private key.",
    "This password CANNOT be recovered if lost.",
    "Please store the password in a safe location.",
    sep = "\n"))
}

and set the default value of password = ask_pass(), allowing users to choose to override the behavior but keeping the default workflow.

The second suggestion is that on RStudio the prompt is quite jumbled when displayed.

image

The above refactoring also displays properly in RStudio.

image

Finally, while looking at genkeys() I realized that the key_name arguments aren't called in the write_pem() step, so all keys will be named id_rsa by default regardless of the input to genkeys().

genkeys <- function(private_key_name = "id_rsa", public_key_name = "id_rsa.pub"){

encryptr/R/genkeys.R

Lines 38 to 39 in 4624dbc

openssl::write_pem(pubkey,
"id_rsa.pub")

If you're interested in the above I could quickly work these suggestions into a PR.

gadenbuie added a commit to gadenbuie/encryptr that referenced this issue Feb 25, 2019

@ewenharrison

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

Many thanks for this and the work in the pull request. It is greatly appreciated.
Along with some other changes, I have incorporated your suggestions in the latest push.
All but the option of exposing the password option.
This was designed in this way intentionally to avoid having passwords written into scripts. This seemed to the team exactly what we were trying to avoid.
Is there a particular situation you see in which the password must be included in the script? If there is a good argument we will include. But there are lots of good reasons not to do this.
Many thanks again.

@gadenbuie

This comment has been minimized.

Copy link
Author

commented Feb 25, 2019

Exposing password wouldn't be for end-users as much as other developers wanting to extend, wrap or build upon encryptr. On the other hand, because genkeys() is a convenience function around a couple lines of standard openssl usage, it wouldn't be that hard or limiting to have to reimplement genkeys() if there is a need for access to password.

It's also worth noting that askpass::askpass() returns NULL in a non-interactive setting, so when run inside a source script it's not at all possible to set a password.

Despite returning NULL non-interactively, you can still use all but the last test I added in the PR.

@ewenharrison

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

I guess I feel pretty strongly about not allowing users the option to make a mistake with a password. Together with wanting to go to information governance authorities and say we are using a system in which the password is not recorded in the script.

An alternative (that may be over thinking it) is to have genkeys_() specifically for developers that allows that option, but is not included in the standard package vignettes etc.

Thanks again for this help and discussion. I should have said above that I haven't incorporated the tests, but will do with a PR when we conclude this thread.

@gadenbuie

This comment has been minimized.

Copy link
Author

commented Feb 25, 2019

That makes sense. And any developer writing code around that part of the workflow should probably be using the lower level packages to have full control over the process. (Meaning I wouldn't recommend supporting a developer-only option there.)

@ewenharrison

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

Great thank you. Ok, I’ll incorporate the tests from your pull request, unless you want to adjust it. I never know the etiquette of these things. Best wishes.

@gadenbuie

This comment has been minimized.

Copy link
Author

commented Feb 25, 2019

Cool and no worries. Feel free to take what you need from the PR and just close it when you're done so I know to update my fork. Cheers!

ewenharrison added a commit that referenced this issue Feb 26, 2019

@ewenharrison

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

Changes made. Thanks again.

ewenharrison added a commit that referenced this issue Feb 26, 2019

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.