thr3ads.net - openssh unix dev - [PATCH v2] Remove sshkey_load

If this information is useful, please help other people find it:
Share via:

Jitendra Sharma

2019-Aug-06 10:17 UTC

[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

Damien Miller

2019-Aug-07 23:26 UTC

head link

[PATCH v2] Remove sshkey_load_private()

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 at mindrot.org
> https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
>

Sharma, Jitendra

2019-Aug-09 11:44 UTC

head link

[PATCH v2] Remove sshkey_load_private()

Thanks Damien for replying.
> -----Original Message-----
> From: Damien Miller [mailto:djm at mindrot.org]
> Sent: Thursday, August 8, 2019 4:57 AM
> To: Sharma, Jitendra <jitendra.sharma at intel.com>
> Cc: openssh-unix-dev at 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 at 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().
> > ---

Apparently Analagous Threads

Search for more possibly parallel threads

openssh unix dev - Aug 2019 - [PATCH v2] Remove sshkey_load_private()

[PATCH v2] Remove sshkey_load_private()

[PATCH v2] Remove sshkey_load_private()

[PATCH v2] Remove sshkey_load_private()

Apparently Analagous Threads