Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue6386 nss dir #143

Closed
wants to merge 1 commit into from
Closed

Issue6386 nss dir #143

wants to merge 1 commit into from

Conversation

tiran
Copy link
Member

@tiran tiran commented Oct 6, 2016 •
edited by MartinBasti

See https://fedorahosted.org/freeipa/ticket/6386

  • use api.env.nss_dir in all ipaclient plugins
  • set api.env.nss_dir to confdir/nssdb

HonzaCholasta
HonzaCholasta reviewed Oct 17, 2016
View reviewed changes
ipaclient/plugins/vault.py
@@ -752,8 +751,7 @@ def forward(self, *args, **options):
error=_('Invalid vault type'))

# initialize NSS database
current_dbdir = paths.IPA_NSSDB_DIR
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NACK, current_dbdir is a global variable, the value must be changed here otherwise NSS initialization might explode later.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's not a global. The variable is only in the local scope. You are probably confusing the plugin with ipapython/nsslib.py.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I stand corrected. But it looks suspicious anyway. I think that the original intent was to actually use nsslib.current_dbdir and that the local variable is used by mistake. But given that nobody complained about NSS initialization failures in vault so far, it is probably OK this way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rather think the author of the code copy and pasted some code and kept the style.

HonzaCholasta
HonzaCholasta reviewed Oct 17, 2016
View reviewed changes
ipaclient/plugins/vault.py
@@ -912,8 +910,7 @@ def forward(self, *args, **options):
vault_type = vault['ipavaulttype'][0]

# initialize NSS database
current_dbdir = paths.IPA_NSSDB_DIR
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NACK, same as above.

HonzaCholasta
HonzaCholasta reviewed Oct 17, 2016
View reviewed changes
ipalib/config.py
@@ -518,6 +518,10 @@ def _finalize_core(self, **defaults):
self._merge_from_file(self.conf)
self._merge_from_file(self.conf_default)

# Set nss_dir to nssdb directory in confdir
if 'nss_dir' not in self:
self.nss_dir = self._join('confdir', 'nssdb')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NACK, the default should be whatever is defined in ipaplatform.paths, changing the default is out of the scope of this fix.

@HonzaCholasta
Copy link
Contributor

NACK, see inline comments.

@tiran
Copy link
Member Author

tiran commented Oct 24, 2016

Please ack.

@tiran tiran mentioned this pull request Oct 25, 2016
Closed
@tkrizek
Copy link
Contributor

tkrizek commented Nov 7, 2016

Functional ACK.

In the ticket, you mention other places where paths.IPA_NSSDB_DIR is used. What's the reason this change affects only client plugins?

@tiran
Use api.env.nss_dir instead of paths.IPA_NSSDB_DIR
648dc4f 
ipaclient plugins are now using nss_dir from api.env instead of
hard-coded paths.IPA_NSSDB_DIR.

Closes: https://fedorahosted.org/freeipa/ticket/6386
Signed-off-by: Christian Heimes <cheimes@redhat.com>
@tiran
Copy link
Member Author

tiran commented Nov 9, 2016

I have fixed all places that don't depend on hard-coded paths. The other places are used for client enrolment and depend on hard-coded paths for sysrestore. Some places use the path before ipalib.api is initialized.

@tkrizek tkrizek added the ack Pull Request approved, can be merged label Nov 10, 2016
@HonzaCholasta
Copy link
Contributor

OK, but you should at least make sure that where the code depends on hard-coded paths, the API is bootstrapped with a hard coded confdir as well, otherwise things might break.

@tiran
Copy link
Member Author

tiran commented Nov 10, 2016

The other locations are used for FreeIPA installation and therefore out of scope for this change.

@HonzaCholasta
Copy link
Contributor

Sure, just please keep this in mind for your other changes.

@tiran
Copy link
Member Author

tiran commented Nov 10, 2016

I don't understand your comment.

@HonzaCholasta
Copy link
Contributor

For example, if your IPA_CONFDIR PR was merged, setting the variable could break ipa-client-install, because the hard coded half of it assumes that the configuration directory is always /etc/ipa, but the API half would use something else.

@tiran
Copy link
Member Author

tiran commented Nov 10, 2016

No, #182 does not break ipa-client-install in a bad way. The command simply refuses to work in the presence of IPA_CONFDIR. api.bootstrap() does not support IPA_CONFDIR for some contexts in order to prevent this kind of issue. I just pushed another change to #182 that raises an exception when IPA_CONFDIR is set in a reserved context (server, installer, updater etc.).

@MartinBasti
Copy link
Contributor

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/a22a5dd676f581910ac7872c1a20322278fc7d4a

@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label Nov 14, 2016
@tiran tiran deleted the issue6386_nss_dir branch November 15, 2016 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HonzaCholasta HonzaCholasta HonzaCholasta left review comments

Assignees

@HonzaCholasta HonzaCholasta

Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
Milestone
No milestone
4 participants
@tiran @HonzaCholasta @tkrizek @MartinBasti