[Pki-devel] [Patch] Alternative CLI password methods

[Endi Sukma Dewata]

On 8/2/2014 4:13 AM, Abhishek Koneru wrote:
> Question: Do we need to provide the -y option? Can we not prompt for a
> user password as we do with the client security database password(or may
> be throw an error message)?

Yeah, the -u option implies username/password authentication, and the -n 
option implies client cert authentication. If the user specifies one of 
these options but doesn't specify the corresponding -w/-W or -c/-C 
option the CLI can prompt for password, so the -y is probably not necessary.

Some additional comments:
1. About URI vs URL, I think URL is more accurate since we're pointing 
to a location, not just an identifier. While URI is technically correct, 
it is more generic. The code itself says URI, but I think it should've 
really been a URL.

2. About the subsystem type, I was previously thinking someday we might 
support multiple subsystems of the same type in the same instance, and 
we will use this parameter to specify the subsystem ID (e.g. ca1, ca2). 
In that case the parameter should be case sensitive. Or we might drop 
this parameter because you can specify the subsystem type (or subsystem 
ID) as the command prefix: <subsystem type/ID>-user-find

3. When an error happens (e.g. password file not found) it will show an 
error message followed by the help message. I think we should not show 
the help message at that point because it actually will make it harder 
to see the error message. If people wants to see the help message they 
can do pki --help later.

4. If the user specifies a password file but it's empty, I think the CLI 
should attempt the operation with an empty password anyway. If it fails 
the authentication, the user will check there's something wrong with the 
password file. But if instead it prompts for password, the user might 
not expect this behavior and it might break automation.

5. The code in read_client_security_database_password_from_file() 
probably could be simplified:
* The token variable doesn't seem to be used, so we probably can remove 
all code related to it.
* If the line contains more then 1 colon, it should take the first colon 
as the delimiter. Anything after the delimiter is considered a password 
(even if it includes more colons). We can let NSS determine if it's a 
valid password (no need to reenforce NSS password policy here).
* If the part before delimiter is empty, we don't need to show a warning 
because token is ignored anyway.
* If the part after delimiter is empty, we don't need to show a warning 
because authentication will fail later.

6. Not sure if this code in 
read_client_security_database_password_from_file() is correct:

   String[] parts = StringUtils.split(line, ":");
   ...
   if (line.endsWith(":")) {
       ...
   } else {
       if (line.startsWith(":")) {
           password = parts[0];  <--- empty string?
       } else {
           password = parts[1];
       }
   }

Does this mean if the line starts with colon it will use an empty 
password? The code probably can just do this:

   int i = line.indexOf(":");
   if (i < 0) {
       password = line;
   } else {
       password = line.substring(i + 1);
   }

7. If the password file for client cert database contains multiple lines 
it will only use the first line. Should there be a mechanism to select 
the password line based on the token name (suppose we're using an 
existing password file)? Alternatively, we probably can just use a 
simple password file format for both -W and -C options.

8. Existing issue, we probably should have checked for mutual 
exclusivity between -u and -n too.

-- 
Endi S. Dewata