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

Allow id_ed25519 keypair name #54

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from
Draft

Conversation

weshinsley
Copy link
Collaborator

Addresses issue #53

@weshinsley weshinsley marked this pull request as draft November 5, 2022 21:48
@weshinsley weshinsley marked this pull request as ready for review November 5, 2022 22:05
@weshinsley weshinsley marked this pull request as draft November 6, 2022 18:01
@weshinsley weshinsley marked this pull request as ready for review November 6, 2022 18:09
@weshinsley weshinsley requested a review from richfitz November 6, 2022 18:14
Copy link
Member

@richfitz richfitz left a comment

Choose a reason for hiding this comment

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

Thanks - some bits that we might add:

  • an item to the NEWS.md file
  • check through the vignette and add an explanatory message so that people are not alarmed if their keypair differs
  • probably worth in a future issue offering to create these new keys rather than rsa as they are meant to be more secure - probably worth doing that before CRAN release though

R/util.R Outdated
@@ -146,3 +146,33 @@ cyphr_file <- function(...) {
file_copy <- function(...) {
stopifnot(file.copy(...))
}

guess_key_options <- function(error = FALSE) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't love this bit - particularly that the error is a different sort of thing. Perhaps:

guess_key_error <- function() {
  paste(sprintf("%s[.pub] pair", guess_key()), collapse = " or ")
}

replacing use of guess_key_options(error = TRUE)

Later we might want to generalise this approach and allow setting key priorities?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree - have separated that out so we have one function providing the list of key-stubs and separate error func...

@weshinsley
Copy link
Collaborator Author

  • Above fix done
  • News item added
  • Vignette refreshed
  • The order of filenames in acceptable_key_filenames() currently (crudely) allows for prioritisation should both of the recognised keypairs exist; I'll create separate internal issue for allowing key creation to do the new type.

@weshinsley weshinsley requested a review from richfitz November 8, 2022 10:46
@weshinsley weshinsley marked this pull request as draft November 16, 2022 11:40
@weshinsley
Copy link
Collaborator Author

Going back to draft, as it seems from the issue that just detecting the filename is not sufficient

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.

2 participants