Bug 3068 - Duplicate code in sshkey_load_private() function
Summary: Duplicate code in sshkey_load_private() function
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: ssh-keygen (show other bugs)
Version: 8.0p1
Hardware: Other Windows 10
: P5 enhancement
Assignee: Assigned to nobody
URL:
Keywords:
Depends on:
Blocks: V_8_4
  Show dependency treegraph
 
Reported: 2019-09-10 14:30 AEST by Jitendra Sharma
Modified: 2020-10-02 14:55 AEST (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jitendra Sharma 2019-09-10 14:30:17 AEST
sshkey_load_private() function's role is almost similar to sshkey_load_private_type().

By checking at the source code for sshkey_load_private(), we could see that there is lot of code duplicacy there (as similar code is present in sshkey_load_private_type()).

Could we explore if we could remove sshkey_load_private(), and keep sshkey_load_private_type(). This will help in avoiding code duplicacy.
Comment 1 Jitendra Sharma 2019-09-10 14:35:53 AEST
In the openssh mailing list already that discussion started, https://lists.gt.net/openssh/dev/74450

Where it was suggested that it would be preferred to keep sshkey_load_private() as a wrapper.

If this is the case could we explore if we could make sshkey_load_private as a macro, so that its client users would not get impacted and they will have this wrapper functionality available to them.

If this idea seems ok, then could this approach be implemented like below:

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)
Comment 2 Damien Miller 2020-07-10 14:07:26 AEST
sshkey_load_private() was removed back in April
Comment 3 Darren Tucker 2020-10-02 14:55:07 AEST
Mass close of all bugs fixed in 8.4 release.