[Pki-devel] [PATCH] 0017 Enable Authority Key Identifier CRL extension

[Endi Sukma Dewata]

On 12/18/2014 7:59 AM, Fraser Tweedale wrote:
> On Wed, Dec 17, 2014 at 10:13:04AM -0800, Christina Fu wrote:
>> Hi Fraser,
>> Regarding CRL, I found the following:
>> https://groups.google.com/forum/#!topic/mozilla.dev.security.policy/ilOoDiCU4JM
>> So I think we can just forget it then, unless you want to install old FF to
>> try.
>> You have an ACK on this patch now.
>>
>> About upgrade,  I can see that you are on the right path there with the
>> upgrade script, and it looks to do the thing, but since I don't have much
>> experience with Python, could you please ask Endi to take a closer look?
>>
> Thanks Christina.
>
> Endi, any comments on upgrade script?
>
> Currently if you opt out of an upgrade step it aborts the whole
> process.  I think there could be scope for marking upgrade steps as
> optional so that the process doesn't bail out, but I haven't
> addressed that in the patch - wanted to solicit feedback first.
>
> Cheers,
>
> Fraser

I have some comments:

1. The upgrade script will run automatically when you install the RPM. 
There's no opt-out mechanism with automatic upgrade, so the behavior of 
existing instances will change. If this is not what we want, we should 
not add an upgrade script.

2. The path to CS.cfg can be constructed like this:
     cfg_path = os.path.join(subsystem.conf_dir, 'CS.cfg')

3. The existing CS.cfg should be backed up before doing anything with it 
using this command:
     self.backup(cfg_path)

4. Ideally the CS.cfg should be read with a proper CS.cfg parser (e.g. 
in case it has multi-line properties). But since the parser only exists 
in Java and we're only modifying a simple property this is fine.

5. If this is going to be added into 10.2.2 you should create an empty 
common/upgrade/10.2.2 folder with a .gitignore file (just copy from 
another folder).

If this is going to be added into 10.2.1 the script should be moved into 
server/upgrade/10.2.1 and be renamed to 02-EnableCRLAKIExtension.

This patch is conditionally ACKed pending changes to address item #2, 
#3, and #5.

-- 
Endi S. Dewata