RFR 8029994: Support "include" and "includedir" in krb5.conf

[Wang Weijun]

On Jun 19, 2014, at 0:17, Sean Mullan <sean.mullan at oracle.com> wrote:

> Just a few comments on Config.java:
> 
> 479         if (dups.contains(file)) {
> 480             throw new IOException("Profile path included more than once");
> 481         } else {
> 482             dups.add(file);
> 483         }
> 
> This could be compressed into one call as:
> 
> if (!dups.add(file)) {
>    throw new IOException("Profile path included more than once");
> }

OK.

> 
> 506                                 // if dir is abosulte, so is p
> 
> typo: absolute

OK.

> 
> 511                 } else if (line.startsWith("include ")) {
> 
> why doesn't the file name syntax check on line 505 also apply to include?

According to http://web.mit.edu/kerberos/krb5-devel/doc/admin/conf_files/krb5_conf.html

   include FILENAME
   includedir DIRNAME

   FILENAME or DIRNAME should be an absolute path. The named file
   or directory must exist and be readable. Including a directory
   includes all files within the directory whose names consist solely
   of alphanumeric characters, dashes, or underscores. Included
   profile files are syntactically independent of their parents,
   so each included file must begin with a section header.

So the syntax check is only done when iterating through all files for an includedir directive. I think the reason is that when using "include" to include a file, the writer spells out the file name explicitly and we should respect his choice. On the other hand, when using "includedir", there will be files in the directory that the writer might not intend to include. For example, file system browser generates .DS_Store on Mac and desktop.ini on Windows, Dropbox generates .dropbox, etc.

> 
> 570                         public Void run() throws Exception {
> 
> This can be declared to throw IOException, then you can change lines 586-591 to:
> 
> throw pe.getException();

You mean javac will be smart enough to find out that pe's cause can only be IOException? It does not compile here.

Thanks
Max

> 
> --Sean
> 
> On 04/10/2014 07:40 AM, Weijun Wang wrote:
>> Hi All
>> 
>> Please review the code changes at
>> 
>>    http://cr.openjdk.java.net/~weijun/8029994/webrev.01/
>> 
>> Two major changes made:
>> 
>> 1. The include and includedir directives are supported now. Read
>> http://web.mit.edu/kerberos/krb5-current/doc/admin/conf_files/krb5_conf.html
>> for a description. The part we support in this RFE is:
>> 
>> -----START-----
>> The krb5.conf file can include other files using either of the following
>> directives at the beginning of a line:
>> 
>> include FILENAME
>> includedir DIRNAME
>> 
>> FILENAME or DIRNAME should be an absolute path. The named file or
>> directory must exist and be readable. Including a directory includes all
>> files within the directory whose names consist solely of alphanumeric
>> characters, dashes, or underscores. Included profile files are
>> syntactically independent of their parents, so each included file must
>> begin with a section header.
>> -----END-----
>> 
>> 2. When the same key appears more than once in krb5.conf, Java used to
>> choose the last value, while MIT krb5 chooses the first one. While it's
>> debatable whether latecomers should be able to override earlier
>> definitions or not, it's more important to have consistent behavior
>> across implementations. Therefore we adopt the MIT krb5 way. The
>> compatibility risk should be very low since it's very unlikely people
>> assigns values to duplicate keys in a single krb5.conf file, which is
>> what we support before this enhancement.
>> 
>> One code change that might look strange is in the Config constructor:
>> 
>>          } catch (IOException ioe) {
>> -            // I/O error, mostly like krb5.conf missing.
>> -            // No problem. We'll use DNS or system property etc.
>> +            throw new KrbException(ioe);
>>          }
>> 
>> Before this, the only possible IOException thrown is
>> FileNotFoundException when krb5.conf is not found, but now there can be
>> much more. So I move the FNFE check inside the loadConfigFile() method as
>> 
>> +                Path path = Paths.get(fileName);
>> +                if (!Files.exists(path)) {
>> +                    // This is OK. There are other ways to get
>> +                    // Kerberos 5 settings
>> +                    return null;
>> +                } else {
>> +                    return readConfigFileLines(
>> +                            fullp, raw, dupsCheck);
>> +                }
>> 
>> Thanks
>> Max