[10] RFR 8177085: Accept including .conf files in krb5.conf's includedir

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[10] RFR 8177085: Accept including .conf files in krb5.conf's includedir

Weijun Wang
Please review the code change at

    http://cr.openjdk.java.net/~weijun/8177085/webrev.00/

This is to be consistent with MIT krb5 [1]

  "Including a directory includes all files within the directory whose
names consist solely of alphanumeric characters, dashes, or underscores.
Starting in release 1.15, files with names ending in ”.conf” are also
included."

New case added to test. Also some rename to make it clearer.

Thanks
Max

[1]
http://web.mit.edu/kerberos/krb5-devel/doc/admin/conf_files/krb5_conf.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8177085: Accept including .conf files in krb5.conf's includedir

Weijun Wang
Kindly please also review the release note text at

   https://bugs.openjdk.java.net/browse/JDK-8177088

Thanks
Max

On 03/19/2017 04:35 PM, Weijun Wang wrote:

> Please review the code change at
>
>    http://cr.openjdk.java.net/~weijun/8177085/webrev.00/
>
> This is to be consistent with MIT krb5 [1]
>
>  "Including a directory includes all files within the directory whose
> names consist solely of alphanumeric characters, dashes, or underscores.
> Starting in release 1.15, files with names ending in ”.conf” are also
> included."
>
> New case added to test. Also some rename to make it clearer.
>
> Thanks
> Max
>
> [1]
> http://web.mit.edu/kerberos/krb5-devel/doc/admin/conf_files/krb5_conf.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8177085: Accept including .conf files in krb5.conf's includedir

Jamil Nimeh
In reply to this post by Weijun Wang
Hi Max,

Do you know if the MIT krb5 code accepts any filename with the .conf
extension?  So filenames with spaces and periods with a .conf suffix are
fine?  I just wanted to make sure because your test code doesn't have
any examples that would go outside the old alphanum, +, _, - set of
characters (e.g. "foo.bar yak.config") but should otherwise be OK
because it ends with .conf.  If that's the desired behavior then that's
fine, I was more curious than anything else.  Maybe not a big deal
because I think even "a.conf" would run down the same codepath as
"foo.bar yak.config".

Nit: Test code, line 110, looks like there are a couple spaces where
you're chaining methods together that you don't do elsewhere in the
code.  Is that intentional?

Otherwise looks good.

Thanks,
--Jamil

On 3/19/2017 1:35 AM, Weijun Wang wrote:

> Please review the code change at
>
>    http://cr.openjdk.java.net/~weijun/8177085/webrev.00/
>
> This is to be consistent with MIT krb5 [1]
>
>  "Including a directory includes all files within the directory whose
> names consist solely of alphanumeric characters, dashes, or
> underscores. Starting in release 1.15, files with names ending in
> ”.conf” are also included."
>
> New case added to test. Also some rename to make it clearer.
>
> Thanks
> Max
>
> [1]
> http://web.mit.edu/kerberos/krb5-devel/doc/admin/conf_files/krb5_conf.html

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8177085: Accept including .conf files in krb5.conf's includedir

Weijun Wang


On 03/19/2017 11:41 PM, Jamil Nimeh wrote:

> Hi Max,
>
> Do you know if the MIT krb5 code accepts any filename with the .conf
> extension?  So filenames with spaces and periods with a .conf suffix are
> fine?  I just wanted to make sure because your test code doesn't have
> any examples that would go outside the old alphanum, +, _, - set of
> characters (e.g. "foo.bar yak.config") but should otherwise be OK
> because it ends with .conf.  If that's the desired behavior then that's
> fine, I was more curious than anything else.  Maybe not a big deal
> because I think even "a.conf" would run down the same codepath as
> "foo.bar yak.config".

The MIT krb5 code has

     if (len >= 5 && !strcmp(filename + len - 5, ".conf"))
         return 1;

So even a bare ".conf" is allowed. My understanding of the old rule is
to exclude OS-generated files like .DS_Store and desktop.ini. Do you
know of any possibilities that a "*.conf" file will be generated this way?

As for the test, we can say "k4.conf" already contains "." which was not
allowed before.

>
> Nit: Test code, line 110, looks like there are a couple spaces where
> you're chaining methods together that you don't do elsewhere in the
> code.  Is that intentional?

No. I cannot remember where the spaces come from. Maybe after breaking
and joining lines in vi?

Thanks
Max

>
> Otherwise looks good.
>
> Thanks,
> --Jamil
>
> On 3/19/2017 1:35 AM, Weijun Wang wrote:
>> Please review the code change at
>>
>>    http://cr.openjdk.java.net/~weijun/8177085/webrev.00/
>>
>> This is to be consistent with MIT krb5 [1]
>>
>>  "Including a directory includes all files within the directory whose
>> names consist solely of alphanumeric characters, dashes, or
>> underscores. Starting in release 1.15, files with names ending in
>> ”.conf” are also included."
>>
>> New case added to test. Also some rename to make it clearer.
>>
>> Thanks
>> Max
>>
>> [1]
>> http://web.mit.edu/kerberos/krb5-devel/doc/admin/conf_files/krb5_conf.html
>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8177085: Accept including .conf files in krb5.conf's includedir

Jamil Nimeh
Hi Max,

On 3/20/2017 7:18 PM, Weijun Wang wrote:

>
>
> On 03/19/2017 11:41 PM, Jamil Nimeh wrote:
>> Hi Max,
>>
>> Do you know if the MIT krb5 code accepts any filename with the .conf
>> extension?  So filenames with spaces and periods with a .conf suffix are
>> fine?  I just wanted to make sure because your test code doesn't have
>> any examples that would go outside the old alphanum, +, _, - set of
>> characters (e.g. "foo.bar yak.config") but should otherwise be OK
>> because it ends with .conf.  If that's the desired behavior then that's
>> fine, I was more curious than anything else.  Maybe not a big deal
>> because I think even "a.conf" would run down the same codepath as
>> "foo.bar yak.config".
>
> The MIT krb5 code has
>
>     if (len >= 5 && !strcmp(filename + len - 5, ".conf"))
>         return 1;
>
> So even a bare ".conf" is allowed. My understanding of the old rule is
> to exclude OS-generated files like .DS_Store and desktop.ini. Do you
> know of any possibilities that a "*.conf" file will be generated this
> way?
Honestly, I can't.  I could see a sysadmin maybe moving a file like
foo.conf maybe to .foo.conf in order to "hide" it, but that wouldn't do
much now (it will still be processed) and now you have a situation where
the admin has a file being processed that doesn't readily show up in a
simple "ls."  Point gun at foot, pull trigger. I don't have a lot of
experience with Kerberos implementations, so I can't think of a case
where the OS would do something like that.  At least not for a
system-level config file.  Maybe if there was a homedir-based conf
file...sometimes those are made as dot files (e.g. the local .ssh
directory...but that's a directory with non-hidden conf files inside).

Since you're consistent with the MIT stuff, it looks good to me.  I was
just curious more than anything else.

>
> As for the test, we can say "k4.conf" already contains "." which was
> not allowed before.
>
>>
>> Nit: Test code, line 110, looks like there are a couple spaces where
>> you're chaining methods together that you don't do elsewhere in the
>> code.  Is that intentional?
>
> No. I cannot remember where the spaces come from. Maybe after breaking
> and joining lines in vi?
Sounds like a likely explanation to me.

>
> Thanks
> Max
>
>>
>> Otherwise looks good.
>>
>> Thanks,
>> --Jamil
>>
>> On 3/19/2017 1:35 AM, Weijun Wang wrote:
>>> Please review the code change at
>>>
>>>    http://cr.openjdk.java.net/~weijun/8177085/webrev.00/
>>>
>>> This is to be consistent with MIT krb5 [1]
>>>
>>>  "Including a directory includes all files within the directory whose
>>> names consist solely of alphanumeric characters, dashes, or
>>> underscores. Starting in release 1.15, files with names ending in
>>> ”.conf” are also included."
>>>
>>> New case added to test. Also some rename to make it clearer.
>>>
>>> Thanks
>>> Max
>>>
>>> [1]
>>> http://web.mit.edu/kerberos/krb5-devel/doc/admin/conf_files/krb5_conf.html 
>>>
>>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8177085: Accept including .conf files in krb5.conf's includedir

Weijun Wang


On 03/21/2017 11:03 PM, Jamil Nimeh wrote:
> Honestly, I can't.  I could see a sysadmin maybe moving a file like
> foo.conf maybe to .foo.conf in order to "hide" it, but that wouldn't do
> much now (it will still be processed) and now you have a situation where
> the admin has a file being processed that doesn't readily show up in a
> simple "ls."  Point gun at foot, pull trigger.

Hopefully this won't happen.

Before supporting .conf, his old foo.conf was not processed. So he might
have never written that file.

> I don't have a lot of
> experience with Kerberos implementations,

No, I am not asking for Kerberos experiences. Just want to know if
people would accidentally create these files. For example, vi will
create .swp. I remember inserting a FAT USB disk into a Mac and some
mysterious files will be generated (alternative streams of resources?)
that is "._" plus original file names. Maybe I can report this to MIT
krb5 and ask if they are afraid of it.

> so I can't think of a case
> where the OS would do something like that.  At least not for a
> system-level config file.  Maybe if there was a homedir-based conf
> file...sometimes those are made as dot files (e.g. the local .ssh
> directory...but that's a directory with non-hidden conf files inside).

Thanks
Max

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8177085: Accept including .conf files in krb5.conf's includedir

Weijun Wang


On 03/21/2017 11:13 PM, Weijun Wang wrote:
> Maybe I can report this to MIT
> krb5 and ask if they are afraid of it.

See http://mailman.mit.edu/pipermail/krbdev/2017-March/012744.html.

Do you suggest we exclude files starting with a dot now? Or we wait
until MIT krb5 implements it first?

Thanks
Max

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8177085: Accept including .conf files in krb5.conf's includedir

Jamil Nimeh

On 3/21/2017 5:53 PM, Weijun Wang wrote:

>
>
> On 03/21/2017 11:13 PM, Weijun Wang wrote:
>> Maybe I can report this to MIT
>> krb5 and ask if they are afraid of it.
>
> See http://mailman.mit.edu/pipermail/krbdev/2017-March/012744.html.
>
> Do you suggest we exclude files starting with a dot now? Or we wait
> until MIT krb5 implements it first?
>
> Thanks
> Max
>
I'm a bit on the fence on this one.  My gut says wait until the MIT krb5
implementation does it so we remain consistent with their implementation
as it is currently.  We could take the initiative given the email,
however, if there's a bug/rfe that's actively being worked by the MIT
krb5 guys on their end.

--Jamil

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8177085: Accept including .conf files in krb5.conf's includedir

Weijun Wang
Good, and I can push my current changeset now.

Thanks
Max

On 03/22/2017 11:48 AM, Jamil Nimeh wrote:

>
> On 3/21/2017 5:53 PM, Weijun Wang wrote:
>>
>>
>> On 03/21/2017 11:13 PM, Weijun Wang wrote:
>>> Maybe I can report this to MIT
>>> krb5 and ask if they are afraid of it.
>>
>> See http://mailman.mit.edu/pipermail/krbdev/2017-March/012744.html.
>>
>> Do you suggest we exclude files starting with a dot now? Or we wait
>> until MIT krb5 implements it first?
>>
>> Thanks
>> Max
>>
> I'm a bit on the fence on this one.  My gut says wait until the MIT krb5
> implementation does it so we remain consistent with their implementation
> as it is currently.  We could take the initiative given the email,
> however, if there's a bug/rfe that's actively being worked by the MIT
> krb5 guys on their end.
>
> --Jamil
>
Loading...