10:28 p.m.
Please review the attached patch which implements alternative CLI
password methods to address the following PKI TRAC ticket:
* PKI TRAC Ticket #555 - Other ways to specify CLI password
<https://fedorahosted.org/pki/ticket/555>
This patch contains two additional password implementations for Basic
Authentication ('-W <user password file>' and '-y' which allows
prompting for the user password), and one additional password
implementation for Client Authentication ('-C <client security database
password file>').
As a part of this patch, the *pki *man page was updated significantly.
Additionally, the Dogtag CLI Design Wiki
<http://pki.fedoraproject.org/wiki/CLI>was updated to reflect these
Dogtag 10.2 changes.
4:13 a.m.
Some comments for the patch.
pki man page -
I feel this line is not required -
Finally, to use the \fBpki\fP command-line tool with basic
authentication interactively, user passwords may be prompted for:
A few lines, i feel, that need a change:
(First of all, a) A client certificate associated with the desired PKI
server must be used for client authentication. This can be done by
importing the client certificate into an NSS security database and
passing the values to the relevant options provided by the \fBpki\fP CLI
framework.
When issuing the first pki command using the authentication parameters
(after completion of the setup of the client
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)?
MainCLI.java:
1. The method names must be camel cased:
promptForPassword, readPlaintextPasswordFromFile and so on.
2. In the method read_plaintext_password_from_file:
The streams must be closed in a finally block. It is a good practice to
ensure that there are no open streams in case of an exception.
The try block can be written as follows:
BufferedReader br = null;
try {
br = new BufferedReader(new FileReader(pwfile));
password = br.readLine();
if (password == null) {
System.err.println("\nWarning: File '" + pwfile +
"' is
empty!\n");
// Attempt to prompt for the password
password = prompt_for_password();
}
} catch (Exception e) {
errMsgs.add("Error: " + e.getMessage());
printHelp(errMsgs);
System.exit(-1);
} finally{
if(br != null)
br.close();
}
In Java7 it can be further simplified as:
try(BufferedReader br = new BufferedReader(new FileReader(pwfile))){
password = br.readLine();
if (password == null || password.trim().length() !=0) { //
Additional check for an empty file. Assuming that the password must be
present on the first line.
System.err.println("\nWarning: File '" + pwfile +
"' is
empty!\n");
// Attempt to prompt for the password
password = prompt_for_password();
}
}catch (Exception e) {
errMsgs.add("Error: " + e.getMessage());
printHelp(errMsgs);
System.exit(-1);
}
The BufferedReader resource used will be auto closed.
3. Similar file read optimization can be applied in
read_client_security_database_password_from_file method.
4. We have been traditionally using "kra" in the URI, i think it is not
required to provide an option to enter DRM in the URI and try to
convert it it internally. May be we can specify that in the man page
itself.
5. You have explained thoroughly, the mutually exclusive nature of the
various options. I think it would be better to throw an error message
when an invalid case
is encountered rather than storing the error messages and printing them
together.
I haven't tested the patch yet. But it looks good logical wise.
-- Abhishek
On Thu, 2014-07-31 at 20:28 -0700, Matthew Harmsen wrote:
10:47 a.m.
On 8/2/2014 4:13 AM, Abhishek Koneru wrote:
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
3958
days inactive
3961
days old
2 comments
3 participants
participants (3)
-
Abhishek Koneru -
Endi Sukma Dewata -
Matthew Harmsen