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

[Abhishek Koneru]

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:

> 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
> 
> 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 was updated to reflect these
> Dogtag 10.2 changes.
> 
> 
> 
> _______________________________________________
> Pki-devel mailing list
> Pki-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20140802/97977166/attachment.htm>