Bug 1159 - %u and %h not handled in IdentityFile
Summary: %u and %h not handled in IdentityFile
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: ssh (show other bugs)
Version: 4.3p2
Hardware: All Linux
: P2 normal
Assignee: Assigned to nobody
URL: http://www.math.ualberta.ca/imaging/s...
Keywords: patch
: 95 (view as bug list)
Depends on:
Blocks: V_4_4
  Show dependency treegraph
 
Reported: 2006-02-22 16:40 AEDT by John Bowman
Modified: 2023-01-13 13:57 AEDT (History)
3 users (show)

See Also:


Attachments
User-dependent IdentityFile (3.51 KB, patch)
2006-02-22 16:41 AEDT, John Bowman
no flags Details | Diff
User-dependent IdentityFile (3.14 KB, patch)
2006-02-22 16:42 AEDT, John Bowman
djm: ok-
Details | Diff
Revised diff (1.22 KB, patch)
2006-02-26 12:53 AEDT, Damien Miller
no flags Details | Diff
Improved diff, with dtucker's suggestions (1.26 KB, patch)
2006-03-12 15:47 AEDT, Damien Miller
dtucker: ok+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Bowman 2006-02-22 16:40:00 AEDT
Here is a patch to allow private key files to be placed system wide (for all users) in a secure (non-NFS) mounted location on systems where home directories are NFS mounted. This addresses an important security hole on systems where home directories are NFS mounted, particularly if there are users who use blank passphrases (or when lpd is tunneled through ssh on systems running lpd as user lp) instead of ssh-agent. IdentityFile now accepts the same %u, %h, %% options that AuthorizedKeysFile accepts (see man sshd). For example, one can specify a user-dependent IdentityFile in ssh_config:

IdentityFile /ssh/%u/id_rsa
Comment 1 John Bowman 2006-02-22 16:41:30 AEDT
Created attachment 1076 [details]
User-dependent IdentityFile

portable version
Comment 2 John Bowman 2006-02-22 16:42:47 AEDT
Created attachment 1077 [details]
User-dependent IdentityFile

OpenBSD version
Comment 3 Damien Miller 2006-02-26 12:49:03 AEDT
Comment on attachment 1077 [details]
User-dependent IdentityFile

>diff -ru ssh/ssh-keygen.c sshJ/ssh-keygen.c
>--- ssh/ssh-keygen.c	2005-11-28 19:04:55.000000000 -0700
>+++ sshJ/ssh-keygen.c	2006-02-21 15:52:36.000000000 -0700
...
>+ 	/* Read systemwide configuration file after user config. */
>+ 	(void)read_config_file(_PATH_HOST_CONFIG_FILE, hostname, &options, 0);

I don't think we want the other tools to depend on ssh_config. 

>--- ssh/ssh.c	2005-12-19 21:41:07.000000000 -0700
>+++ sshJ/ssh.c	2006-02-21 15:52:36.000000000 -0700
...
> 	for (; i < options.num_identity_files; i++) {
> 		filename = tilde_expand_filename(options.identity_files[i],
> 		    original_real_uid);
>+		filename = percent_expand(filename, "h", pw->pw_dir,	
>+					  "u", pw->pw_name, (char *)NULL);
> 		public = key_load_public(filename, NULL);

This leaks memory. Also, I think it would be better to have:

%u -> user
%h -> local hostname
%d -> home directory

I'll attach a revised patch
Comment 4 Damien Miller 2006-02-26 12:53:54 AEDT
Created attachment 1083 [details]
Revised diff

This diff fixes the memory leak and uses the mnemonics from Comment #3.

Note that the diff doesn't touch ssh-keygen.
Comment 5 Darren Tucker 2006-02-26 17:15:20 AEDT
(In reply to comment #3)
> Also, I think it would be better to have:
> 
> %u -> user
> %h -> local hostname
> %d -> home directory

The current uses of percent_expand() are, with this proposal at the bottom:

                ClHost  ClUser  SrvHost SrvUser SrvPort Homedir
authorized_keys                         %u              %h
ControlPath     %l              %h      %r      %p
ProxyCommand                    %h              %p
IdentityFile    %h      %u                              %d

To keep the client-side consistent, those could be:
%u -> local user
%l -> local hostname
%d -> home directory

The following might also be useful:
%h -> remote host
%r -> remote username
Comment 6 Damien Miller 2006-03-12 15:47:48 AEDT
Created attachment 1097 [details]
Improved diff, with dtucker's suggestions

Good points Darren, revised diff attached.
Comment 7 Damien Miller 2006-03-12 16:06:03 AEDT
*** Bug 95 has been marked as a duplicate of this bug. ***
Comment 8 Darren Tucker 2006-03-30 21:27:16 AEDT
Comment on attachment 1097 [details]
Improved diff, with dtucker's suggestions

Looks and tests fine here.

I would have used something other than "me" ("thishost"? "lhost"?) to prevent confusing it with the local user name.
Comment 9 Damien Miller 2006-03-30 21:42:40 AEDT
Patch applied (with s/me/thishost/) and will be in 4.4. Thanks
Comment 10 Darren Tucker 2006-09-28 19:25:51 AEST
With the release of 4.4, we believe that this bug is now closed.  For information about the release please see http://www.openssh.com/txt/release-4.4 .