#47891 Admin Server reconfig breaks SSL config
Closed: Fixed None Opened 5 years ago by mreynolds.

When the admin Server is updated or has it's being registered, it replaces/overwrites the security files which breaks SSL.


1) I'm a little confused at the directory name. You create bakup/secfiles at the line 511, and copy savefile to bakup/tmp... One is supposed to be the other?
2) $admConf->{configdir} already has the "bakup" directory? If not, perl mkdir works recursively like "mkdir -p"?
3) Usually, directory's mode is 0755? (If no other user has a chance to go into the directory, it does not matter, though... :)
{{{
511 mkdir ('$admConf->{configdir}/bakup/secfiles', 0744);
512 foreach my $savefile (@reconfigsavefiles) {
513 copy ("$admConf->{configdir}/$savefile", "$admConf->{configdir}/bakup/tmp/");
514 }
}}}

Replying to [comment:2 nhosoi]:

1) I'm a little confused at the directory name. You create bakup/secfiles at the line 511, and copy savefile to bakup/tmp... One is supposed to be the other?

Originally it was bakup/tmp during my testing. but I decided to use a more meaningful name. I obviously didn't fully make that name change, good catch.

2) $admConf->{configdir} already has the "bakup" directory? If not, perl mkdir works recursively like "mkdir -p"?

Yes, the bakup directory already exists.

3) Usually, directory's mode is 0755? (If no other user has a chance to go into the directory, it does not matter, though... :)

It doesn't really matter, because we create and destroy this directory during the reconfiguration process.

New patch is attached.

{{{
511 mkdir ('$admConf->{configdir}/bakup/secfiles', 0744);
512 foreach my $savefile (@reconfigsavefiles) {
513 copy ("$admConf->{configdir}/$savefile", "$admConf->{configdir}/bakup/tmp/");
514 }
}}}

Thanks for the clarification and revision, Mark!

Replying to [comment:4 nhosoi]:

Thanks for the clarification and revision, Mark!

Actually I found some other issues with the fix. I need to rework it...

To ssh://git.fedorahosted.org/git/389/admin.git
376cb98..fa79ba1 master -> master

commit fa79ba174a410571af6206568877f91ccfe9aa8e
Author: Mark Reynolds mreynolds@redhat.com
Date: Mon Sep 8 16:41:23 2014 -0400

git patch file (master) -- Use mkdtemp to create a temporary backup dir.
0001-Ticket-47891-Admin-Server-reconfig-breaks-SSL-config.2.patch

{{{
if ( ! -d $secfile_backup_dir){
519 mkdir ($secfile_backup_dir, 0755);
522 debug(0, "Failed to create backup dir $secfile_backup_dir\n");
523 return 1;
520 524 }
}}}
I think you should use a setup $FATAL msg for this - e.g. something like this:
{{{
$setup->msg($FATAL, 'error_creating_secfile_backup', $secfile_backup_dir, $!);
}}}
where error_creating_secfile_backup is defined in setup-ds-admin.res.in like this:
{{{
error_creating_secfile_backup = Could not create temporary directory %s to\
backup security files. Error: %s
}}}
This should be a fatal error and abort the setup process. Same in restore - use a key like error_reading_secfile_backup and have the message refer to trying to access, not create, the secfile backup dir.

In createAdminServer you should check the return value from reconfig_backup_secfiles and reconfig_restore_secfiles and handle an error return like a fatal error.

Thank you, Rich!

Plus, I'm afraid this code could cause the same issue as #47991. Move between potentially separated file systems. I should change the backup dir to the local area...

{{{

523     $secfile_backup_dir = mkdtemp($my_template_backup_dir);

518 524 if ( ! -d $secfile_backup_dir){
519 mkdir ($secfile_backup_dir, 0755);
525 $setup->msg($FATAL, 'error_creating_secfile_backup', $secfile_backup_dir, $?);
}}}
Does mkdtemp fork/exec/system an external program, or does it handle it in the current process? If it is the former, then using $? is correct. If not, then you want to use $!. See http://perldoc.perl.org/perlvar.html

git patch file (master) -- Use mkdtemp to create a temporary backup dir + revised based upon the comments by Rich (Thank you!!) + replaced '$?' with '$!'
0001-Ticket-47891-Admin-Server-reconfig-breaks-SSL-config.3.patch

Reviewed by Rich (Thank you!!)

Pushed to master:
2524f08..894e5b9 master -> master
commit 894e5b9a64c2edaea6f1a1ef089015a1920169df

Metadata Update from @mreynolds:
- Issue assigned to mreynolds
- Issue set to the milestone: 389-admin,console 1.1.36

2 years ago

Login to comment on this ticket.

Metadata