Cleanup certdb #453
Cleanup certdb #453
Conversation
|
I'm pretty sure the chdir() hack was due to SELinux issues, be sure to test in enforcing mode. It may no longer be required. |
|
Thx Rob, I use |
|
@stlaz You did most work with NSS and certdb recently. Can you have a look at this collection of fixes. Iis it useful for you or do you plan to rip out the module soonish? Either way please feel free to merge or close this PR. |
|
@tiran Thanks for reminding me. I was waiting for some of my fixes to get pushed as well, I will go through your PR first thing tomorrow. |
There was a problem hiding this comment.
I would personally rather restrain from removing the chdir() hack. It's too magical and I would like to avoid any possible regressions at this point of the development phase. I was promised CertDB and NSSDatabase would go away with 4.6 so lets keep our heads down until then and fix possible regressions the change will cause in the next development cycle.
A bit of OT rant - certdb and certs modules could have had unit tests so easily that would have helped accepting all of this and would have helped us so much in the past. I was surprised I could not run any test suite to test this. Hopefully this'll be fixed with the successor of this machinery as well.
ipaserver/install/certs.py
Outdated
| except OSError: | ||
| pass | ||
| self.reqdir = None | ||
| self.nssdb.close() |
There was a problem hiding this comment.
The inner NSSDatabase is always initialized with a specific directory, NSSDatabase.close() only has effect for temporary databases.
There was a problem hiding this comment.
Personally I find it cleaner to always call close. You never know of NSSDatabase is going to need close() for other reasons.
But I'm not going to fight you on this.
There was a problem hiding this comment.
Alright, you can keep it there, I don't mind.
| perms = stat.S_IRUSR | ||
| if write: | ||
| perms |= stat.S_IWUSR | ||
| os.chmod(fname, perms) | ||
| if hasattr(fname, 'fileno'): |
There was a problem hiding this comment.
Only file names are passed to set_perms() so this check is redundant.
There was a problem hiding this comment.
Scratch that, didn't realize you are passing files to it as well.
There was a problem hiding this comment.
Yes, it's both more efficient and more secure to operate on FDs.
tiran
force-pushed
the
certs_cleanup
branch
2 times, most recently
from
be337e5
to
34f1644
Compare
ipaserver/install/certs.py
Outdated
| os.unlink(self.certreq_fname) | ||
| os.unlink(self.certder_fname) | ||
| try: | ||
| cdb.issue_server_cert(self.certreq_fname, self.certder_fname) |
There was a problem hiding this comment.
The behavior here changed a bit, there's no need to ask another database to issue server cert for us since the location of Dogtag agent certificates is know.
Do s/cdb/self/.
ipaserver/install/certs.py
Outdated
| @@ -183,22 +187,20 @@ def setup_cert_request(self): | |||
| self.certreq_fname = self.reqdir + "/tmpcertreq" | |||
| self.certder_fname = self.reqdir + "/tmpcert.der" | |||
|
|
|||
| # When certutil makes a request it creates a file in the cwd, make | |||
| # sure we are in a unique place when this happens | |||
| os.chdir(self.reqdir) | |||
There was a problem hiding this comment.
This changes original functionality, please, add these lines back.
ipaserver/install/certs.py
Outdated
|
|
||
| def run_certutil(self, args, stdin=None, **kwargs): | ||
| # When certutil makes a request it creates a file in the cwd, make | ||
| # sure we are in a unique place when this happens |
There was a problem hiding this comment.
These two comments don't belong here.
|
The changes are fine. Please, squash the two commits together so that we can push it. |
* use with statement to open/close files * prefer fchmod/fchown when a file descriptor is available * set permission before data is written to file Signed-off-by: Christian Heimes <cheimes@redhat.com>
|
master:
|
Do not ever change the working directory of a program. It's a really bad
idea. Just consider what is going to happen if two threads or two
different parts of a process decide to own control over the working
directory?
Signed-off-by: Christian Heimes cheimes@redhat.com