Mailing List Archive

[PATCH v2] Remove sshkey_load_private()
Remove sshkey_load_private(), as this function's role
is similar to sshkey_load_private_type().
---
Dependency:
This change depends over recently merged change in openbsd:
https://github.com/openbsd/src/commit/b0c328c8f066f6689874bef7f338179145ce58d0

Change log:
v1->v2
- Remove declaration of sshkey_load_private() in authfile.h

authfile.c | 38 --------------------------------------
authfile.h | 1 -
ssh-keygen.c | 20 +++++++++++---------
sshd.c | 5 +++--
4 files changed, 14 insertions(+), 50 deletions(-)

diff --git a/authfile.c b/authfile.c
index c28652c8bdf..6d86c2dd4c6 100644
--- a/authfile.c
+++ b/authfile.c
@@ -215,44 +215,6 @@ sshkey_load_private_type_fd(int fd, int type, const char *passphrase,
return r;
}

-/* XXX this is almost identical to sshkey_load_private_type() */
-int
-sshkey_load_private(const char *filename, const char *passphrase,
- struct sshkey **keyp, char **commentp)
-{
- struct sshbuf *buffer = NULL;
- int r, fd;
-
- if (keyp != NULL)
- *keyp = NULL;
- if (commentp != NULL)
- *commentp = NULL;
-
- if ((fd = open(filename, O_RDONLY)) == -1)
- return SSH_ERR_SYSTEM_ERROR;
- if (sshkey_perm_ok(fd, filename) != 0) {
- r = SSH_ERR_KEY_BAD_PERMISSIONS;
- goto out;
- }
-
- if ((buffer = sshbuf_new()) == NULL) {
- r = SSH_ERR_ALLOC_FAIL;
- goto out;
- }
- if ((r = sshkey_load_file(fd, buffer)) != 0 ||
- (r = sshkey_parse_private_fileblob(buffer, passphrase, keyp,
- commentp)) != 0)
- goto out;
- if (keyp && *keyp &&
- (r = sshkey_set_filename(*keyp, filename)) != 0)
- goto out;
- r = 0;
- out:
- close(fd);
- sshbuf_free(buffer);
- return r;
-}
-
static int
sshkey_try_load_public(struct sshkey *k, const char *filename, char **commentp)
{
diff --git a/authfile.h b/authfile.h
index a6b9759c5ea..0279a89e2b4 100644
--- a/authfile.h
+++ b/authfile.h
@@ -38,7 +38,6 @@ int sshkey_save_private(struct sshkey *, const char *,
int sshkey_load_file(int, struct sshbuf *);
int sshkey_load_cert(const char *, struct sshkey **);
int sshkey_load_public(const char *, struct sshkey **, char **);
-int sshkey_load_private(const char *, const char *, struct sshkey **, char **);
int sshkey_load_private_cert(int, const char *, const char *,
struct sshkey **);
int sshkey_load_private_type(int, const char *, const char *,
diff --git a/ssh-keygen.c b/ssh-keygen.c
index ea3c0e63888..215693eaca6 100644
--- a/ssh-keygen.c
+++ b/ssh-keygen.c
@@ -275,7 +275,8 @@ load_identity(char *filename)
struct sshkey *prv;
int r;

- if ((r = sshkey_load_private(filename, "", &prv, NULL)) == 0)
+ if ((r = sshkey_load_private_type(KEY_UNSPEC, filename, "",
+ &prv, NULL)) == 0)
return prv;
if (r != SSH_ERR_KEY_WRONG_PASSPHRASE)
fatal("Load key \"%s\": %s", filename, ssh_err(r));
@@ -283,7 +284,7 @@ load_identity(char *filename)
pass = xstrdup(identity_passphrase);
else
pass = read_passphrase("Enter passphrase: ", RP_ALLOW_STDIN);
- r = sshkey_load_private(filename, pass, &prv, NULL);
+ r = sshkey_load_private_type(KEY_UNSPEC, filename, pass, &prv, NULL);
explicit_bzero(pass, strlen(pass));
free(pass);
if (r != 0)
@@ -855,7 +856,7 @@ fingerprint_private(const char *path)
fatal("%s: %s", path, strerror(errno));
if ((r = sshkey_load_public(path, &public, &comment)) != 0) {
debug("load public \"%s\": %s", path, ssh_err(r));
- if ((r = sshkey_load_private(path, NULL,
+ if ((r = sshkey_load_private_type(KEY_UNSPEC, path, NULL,
&public, &comment)) != 0) {
debug("load private \"%s\": %s", path, ssh_err(r));
fatal("%s is not a key file.", path);
@@ -1349,7 +1350,8 @@ do_change_passphrase(struct passwd *pw)
if (stat(identity_file, &st) == -1)
fatal("%s: %s", identity_file, strerror(errno));
/* Try to load the file with empty passphrase. */
- r = sshkey_load_private(identity_file, "", &private, &comment);
+ r = sshkey_load_private_type(KEY_UNSPEC, identity_file, "",
+ &private, &comment);
if (r == SSH_ERR_KEY_WRONG_PASSPHRASE) {
if (identity_passphrase)
old_passphrase = xstrdup(identity_passphrase);
@@ -1357,8 +1359,8 @@ do_change_passphrase(struct passwd *pw)
old_passphrase =
read_passphrase("Enter old passphrase: ",
RP_ALLOW_STDIN);
- r = sshkey_load_private(identity_file, old_passphrase,
- &private, &comment);
+ r = sshkey_load_private_type(KEY_UNSPEC, identity_file,
+ old_passphrase, &private, &comment);
explicit_bzero(old_passphrase, strlen(old_passphrase));
free(old_passphrase);
if (r != 0)
@@ -1461,7 +1463,7 @@ do_change_comment(struct passwd *pw, const char *identity_comment)
ask_filename(pw, "Enter file in which the key is");
if (stat(identity_file, &st) == -1)
fatal("%s: %s", identity_file, strerror(errno));
- if ((r = sshkey_load_private(identity_file, "",
+ if ((r = sshkey_load_private_type(KEY_UNSPEC, identity_file, "",
&private, &comment)) == 0)
passphrase = xstrdup("");
else if (r != SSH_ERR_KEY_WRONG_PASSPHRASE)
@@ -1476,8 +1478,8 @@ do_change_comment(struct passwd *pw, const char *identity_comment)
passphrase = read_passphrase("Enter passphrase: ",
RP_ALLOW_STDIN);
/* Try to load using the passphrase. */
- if ((r = sshkey_load_private(identity_file, passphrase,
- &private, &comment)) != 0) {
+ if ((r = sshkey_load_private_type(KEY_UNSPEC, identity_file,
+ passphrase, &private, &comment)) != 0) {
explicit_bzero(passphrase, strlen(passphrase));
free(passphrase);
fatal("Cannot load private key \"%s\": %s.",
diff --git a/sshd.c b/sshd.c
index 11571c01011..cea85de2404 100644
--- a/sshd.c
+++ b/sshd.c
@@ -1719,8 +1719,9 @@ main(int ac, char **av)

if (options.host_key_files[i] == NULL)
continue;
- if ((r = sshkey_load_private(options.host_key_files[i], "",
- &key, NULL)) != 0 && r != SSH_ERR_SYSTEM_ERROR)
+ if ((r = sshkey_load_private_type(KEY_UNSPEC,
+ options.host_key_files[i], "", &key, NULL)) != 0 &&
+ r != SSH_ERR_SYSTEM_ERROR)
do_log2(ll, "Unable to load host key \"%s\": %s",
options.host_key_files[i], ssh_err(r));
if (r == 0 && (r = sshkey_shield_private(key)) != 0) {
--
2.22.0

_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: [PATCH v2] Remove sshkey_load_private() [ In reply to ]
I think I'd prefer that sshkey_load_private() remains as a wrapper.

On Tue, 6 Aug 2019, Jitendra Sharma wrote:

> Remove sshkey_load_private(), as this function's role
> is similar to sshkey_load_private_type().
> ---
> Dependency:
> This change depends over recently merged change in openbsd:
> https://github.com/openbsd/src/commit/b0c328c8f066f6689874bef7f338179145ce58d0
>
> Change log:
> v1->v2
> - Remove declaration of sshkey_load_private() in authfile.h
>
> authfile.c | 38 --------------------------------------
> authfile.h | 1 -
> ssh-keygen.c | 20 +++++++++++---------
> sshd.c | 5 +++--
> 4 files changed, 14 insertions(+), 50 deletions(-)
>
> diff --git a/authfile.c b/authfile.c
> index c28652c8bdf..6d86c2dd4c6 100644
> --- a/authfile.c
> +++ b/authfile.c
> @@ -215,44 +215,6 @@ sshkey_load_private_type_fd(int fd, int type, const char *passphrase,
> return r;
> }
>
> -/* XXX this is almost identical to sshkey_load_private_type() */
> -int
> -sshkey_load_private(const char *filename, const char *passphrase,
> - struct sshkey **keyp, char **commentp)
> -{
> - struct sshbuf *buffer = NULL;
> - int r, fd;
> -
> - if (keyp != NULL)
> - *keyp = NULL;
> - if (commentp != NULL)
> - *commentp = NULL;
> -
> - if ((fd = open(filename, O_RDONLY)) == -1)
> - return SSH_ERR_SYSTEM_ERROR;
> - if (sshkey_perm_ok(fd, filename) != 0) {
> - r = SSH_ERR_KEY_BAD_PERMISSIONS;
> - goto out;
> - }
> -
> - if ((buffer = sshbuf_new()) == NULL) {
> - r = SSH_ERR_ALLOC_FAIL;
> - goto out;
> - }
> - if ((r = sshkey_load_file(fd, buffer)) != 0 ||
> - (r = sshkey_parse_private_fileblob(buffer, passphrase, keyp,
> - commentp)) != 0)
> - goto out;
> - if (keyp && *keyp &&
> - (r = sshkey_set_filename(*keyp, filename)) != 0)
> - goto out;
> - r = 0;
> - out:
> - close(fd);
> - sshbuf_free(buffer);
> - return r;
> -}
> -
> static int
> sshkey_try_load_public(struct sshkey *k, const char *filename, char **commentp)
> {
> diff --git a/authfile.h b/authfile.h
> index a6b9759c5ea..0279a89e2b4 100644
> --- a/authfile.h
> +++ b/authfile.h
> @@ -38,7 +38,6 @@ int sshkey_save_private(struct sshkey *, const char *,
> int sshkey_load_file(int, struct sshbuf *);
> int sshkey_load_cert(const char *, struct sshkey **);
> int sshkey_load_public(const char *, struct sshkey **, char **);
> -int sshkey_load_private(const char *, const char *, struct sshkey **, char **);
> int sshkey_load_private_cert(int, const char *, const char *,
> struct sshkey **);
> int sshkey_load_private_type(int, const char *, const char *,
> diff --git a/ssh-keygen.c b/ssh-keygen.c
> index ea3c0e63888..215693eaca6 100644
> --- a/ssh-keygen.c
> +++ b/ssh-keygen.c
> @@ -275,7 +275,8 @@ load_identity(char *filename)
> struct sshkey *prv;
> int r;
>
> - if ((r = sshkey_load_private(filename, "", &prv, NULL)) == 0)
> + if ((r = sshkey_load_private_type(KEY_UNSPEC, filename, "",
> + &prv, NULL)) == 0)
> return prv;
> if (r != SSH_ERR_KEY_WRONG_PASSPHRASE)
> fatal("Load key \"%s\": %s", filename, ssh_err(r));
> @@ -283,7 +284,7 @@ load_identity(char *filename)
> pass = xstrdup(identity_passphrase);
> else
> pass = read_passphrase("Enter passphrase: ", RP_ALLOW_STDIN);
> - r = sshkey_load_private(filename, pass, &prv, NULL);
> + r = sshkey_load_private_type(KEY_UNSPEC, filename, pass, &prv, NULL);
> explicit_bzero(pass, strlen(pass));
> free(pass);
> if (r != 0)
> @@ -855,7 +856,7 @@ fingerprint_private(const char *path)
> fatal("%s: %s", path, strerror(errno));
> if ((r = sshkey_load_public(path, &public, &comment)) != 0) {
> debug("load public \"%s\": %s", path, ssh_err(r));
> - if ((r = sshkey_load_private(path, NULL,
> + if ((r = sshkey_load_private_type(KEY_UNSPEC, path, NULL,
> &public, &comment)) != 0) {
> debug("load private \"%s\": %s", path, ssh_err(r));
> fatal("%s is not a key file.", path);
> @@ -1349,7 +1350,8 @@ do_change_passphrase(struct passwd *pw)
> if (stat(identity_file, &st) == -1)
> fatal("%s: %s", identity_file, strerror(errno));
> /* Try to load the file with empty passphrase. */
> - r = sshkey_load_private(identity_file, "", &private, &comment);
> + r = sshkey_load_private_type(KEY_UNSPEC, identity_file, "",
> + &private, &comment);
> if (r == SSH_ERR_KEY_WRONG_PASSPHRASE) {
> if (identity_passphrase)
> old_passphrase = xstrdup(identity_passphrase);
> @@ -1357,8 +1359,8 @@ do_change_passphrase(struct passwd *pw)
> old_passphrase =
> read_passphrase("Enter old passphrase: ",
> RP_ALLOW_STDIN);
> - r = sshkey_load_private(identity_file, old_passphrase,
> - &private, &comment);
> + r = sshkey_load_private_type(KEY_UNSPEC, identity_file,
> + old_passphrase, &private, &comment);
> explicit_bzero(old_passphrase, strlen(old_passphrase));
> free(old_passphrase);
> if (r != 0)
> @@ -1461,7 +1463,7 @@ do_change_comment(struct passwd *pw, const char *identity_comment)
> ask_filename(pw, "Enter file in which the key is");
> if (stat(identity_file, &st) == -1)
> fatal("%s: %s", identity_file, strerror(errno));
> - if ((r = sshkey_load_private(identity_file, "",
> + if ((r = sshkey_load_private_type(KEY_UNSPEC, identity_file, "",
> &private, &comment)) == 0)
> passphrase = xstrdup("");
> else if (r != SSH_ERR_KEY_WRONG_PASSPHRASE)
> @@ -1476,8 +1478,8 @@ do_change_comment(struct passwd *pw, const char *identity_comment)
> passphrase = read_passphrase("Enter passphrase: ",
> RP_ALLOW_STDIN);
> /* Try to load using the passphrase. */
> - if ((r = sshkey_load_private(identity_file, passphrase,
> - &private, &comment)) != 0) {
> + if ((r = sshkey_load_private_type(KEY_UNSPEC, identity_file,
> + passphrase, &private, &comment)) != 0) {
> explicit_bzero(passphrase, strlen(passphrase));
> free(passphrase);
> fatal("Cannot load private key \"%s\": %s.",
> diff --git a/sshd.c b/sshd.c
> index 11571c01011..cea85de2404 100644
> --- a/sshd.c
> +++ b/sshd.c
> @@ -1719,8 +1719,9 @@ main(int ac, char **av)
>
> if (options.host_key_files[i] == NULL)
> continue;
> - if ((r = sshkey_load_private(options.host_key_files[i], "",
> - &key, NULL)) != 0 && r != SSH_ERR_SYSTEM_ERROR)
> + if ((r = sshkey_load_private_type(KEY_UNSPEC,
> + options.host_key_files[i], "", &key, NULL)) != 0 &&
> + r != SSH_ERR_SYSTEM_ERROR)
> do_log2(ll, "Unable to load host key \"%s\": %s",
> options.host_key_files[i], ssh_err(r));
> if (r == 0 && (r = sshkey_shield_private(key)) != 0) {
> --
> 2.22.0
>
> _______________________________________________
> openssh-unix-dev mailing list
> openssh-unix-dev@mindrot.org
> https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
>
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
RE: [PATCH v2] Remove sshkey_load_private() [ In reply to ]
Thanks Damien for replying.

> -----Original Message-----
> From: Damien Miller [mailto:djm@mindrot.org]
> Sent: Thursday, August 8, 2019 4:57 AM
> To: Sharma, Jitendra <jitendra.sharma@intel.com>
> Cc: openssh-unix-dev@mindrot.org
> Subject: Re: [PATCH v2] Remove sshkey_load_private()
>
> I think I'd prefer that sshkey_load_private() remains as a wrapper.
>

I assume there must be a strong reason for keeping sshkey_load_private wrapper, which I am not aware of ?
However if the intent is to provide a facility/wrapper to consumers of this function, then to avoid duplicate code, we could provide a macro interface to end users.
Could you please review this code, if it sounds good:

jitendra@clear-linux~/openssh-portable $ git diff
diff --git a/authfile.c b/authfile.c
index 5e335ce4..89b5ca39 100644
--- a/authfile.c
+++ b/authfile.c
@@ -215,44 +215,6 @@ sshkey_load_private_type_fd(int fd, int type, const char *passphrase,
return r;
}

-/* XXX this is almost identical to sshkey_load_private_type() */
-int
-sshkey_load_private(const char *filename, const char *passphrase,
- struct sshkey **keyp, char **commentp)
-{
- struct sshbuf *buffer = NULL;
- int r, fd;
-
- if (keyp != NULL)
- *keyp = NULL;
- if (commentp != NULL)
- *commentp = NULL;
-
- if ((fd = open(filename, O_RDONLY)) == -1)
- return SSH_ERR_SYSTEM_ERROR;
- if (sshkey_perm_ok(fd, filename) != 0) {
- r = SSH_ERR_KEY_BAD_PERMISSIONS;
- goto out;
- }
-
- if ((buffer = sshbuf_new()) == NULL) {
- r = SSH_ERR_ALLOC_FAIL;
- goto out;
- }
- if ((r = sshkey_load_file(fd, buffer)) != 0 ||
- (r = sshkey_parse_private_fileblob(buffer, passphrase, keyp,
- commentp)) != 0)
- goto out;
- if (keyp && *keyp &&
- (r = sshkey_set_filename(*keyp, filename)) != 0)
- goto out;
- r = 0;
- out:
- close(fd);
- sshbuf_free(buffer);
- return r;
-}
-
static int
sshkey_try_load_public(struct sshkey *k, const char *filename, char **commentp)
{
diff --git a/authfile.h b/authfile.h
index 54df169b..2962fbba 100644
--- a/authfile.h
+++ b/authfile.h
@@ -30,6 +30,9 @@
struct sshbuf;
struct sshkey;

+#define sshkey_load_private(filename, passphrase, keyp, commentp) \
+ sshkey_load_private_type(KEY_UNSPEC, filename, passphrase, keyp, commentp)
+
/* XXX document these */
/* XXX some of these could probably be merged/retired */

@@ -38,7 +41,6 @@ int sshkey_save_private(struct sshkey *, const char *,
int sshkey_load_file(int, struct sshbuf *);
int sshkey_load_cert(const char *, struct sshkey **);
int sshkey_load_public(const char *, struct sshkey **, char **);
-int sshkey_load_private(const char *, const char *, struct sshkey **, char **);
int sshkey_load_private_cert(int, const char *, const char *,
struct sshkey **);
int sshkey_load_private_type(int, const char *, const char *,
(END)


> On Tue, 6 Aug 2019, Jitendra Sharma wrote:
>
> > Remove sshkey_load_private(), as this function's role is similar to
> > sshkey_load_private_type().
> > ---
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev