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.

Sign up for GitHub

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

Turn off interpolation to avoid issues with special characters #6154

Closed
wants to merge 1 commit into from
Closed

Turn off interpolation to avoid issues with special characters #6154

wants to merge 1 commit into from

Conversation

Caligatio
Copy link
Contributor

This fixes https://pagure.io/freeipa/issue/9085

The Python configparser.ConfigParser class enables string interpolation by default which throws errors if % signs are used in any values that don't interpolate correctly. The majority of the code base uses configparser.RawConfigParser which has interpolation disabled by default so ipalib/sysrestore.py can match this functionality by setting interpolation=None on instantiation.

@rcritten
Copy link
Contributor

rcritten commented Feb 2, 2022

Thanks for the patch. Can you update the commit message per https://www.freeipa.org/page/Contribute/Code#Commit_message_requirements

I don't think its a bad thing to disable interpolation but I don't know that this is going to make a NIS domain name with a % in it work everywhere. Have you tried netgroups with this change locally? Do they work as expected?

@Caligatio
Copy link
Contributor Author

Commit message updated!

I think I was unclear about the problem, this bug is preventing me from migrating from a NIS domain with a %. Rough steps to reproduce:

  • Don't use FreeIPA
  • Use a NIS domain with a percent sign (e.g. exam%ple)
  • Decide to move to FreeIPA and pick a proper domain name (e.g. example.com)
  • Run ipa-client-install with default options pointing to the FreeIPA server with example.com set
    • By default, ipa-client-install will configure the NIS domain
    • If an existing NIS domain is detected, (exam%ple), it will back-up that value using ipalib/sysrestore.py:FileStore
    • FileStore uses SafeConfigParser which then explodes while trying to back-up the old/defunct exam%ple domain

I'm not trying to continue support for % in NIS domains, I'm trying to fix it so I can migrate off of it.

@rcritten
Copy link
Contributor

rcritten commented Feb 7, 2022

That makes sense. Kicking off full CI.

@rcritten rcritten added ipa-4-9 Mark for backport to ipa 4.9 re-run Trigger a new run of PR-CI labels Feb 7, 2022
@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Feb 7, 2022
@rcritten
Copy link
Contributor

rcritten commented Feb 9, 2022

The code looks good. Can you update the commit message with the basic reasoning from your latest comment? I think that will add good context for the future and is worth preserving in the repo.

@Caligatio
ipalib: Handle percent signs in saved values
68fce17 
Turn off string interpolation on the FileStore class to avoid
exceptions when a value to be saved contains a percent sign (%).
The underlying SafeConfigParser that is used interprets percent
signs as placeholders to be interpolated which then causes an
exception as the placeholder isn't properly formatted.

ipa-client-install uses the FileStore class to backup certain
values that it overwrites as part of the installation. If those
pre-existing, backed-up values contained a percent sign,
ipa-client-install would throw an exception and thus prevent
installation.

https://pagure.io/freeipa/issue/9085
@Caligatio
Copy link
Contributor Author

I added another paragraph to the comment; hopefully it provides sufficient context!

@flo-renaud flo-renaud added the re-run Trigger a new run of PR-CI label Feb 10, 2022
@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Feb 10, 2022
@abbra abbra added ack Pull Request approved, can be merged pushed Pull Request has already been pushed labels Feb 11, 2022
@abbra
Copy link
Contributor

abbra commented Feb 11, 2022

master:

  • ba796e3 ipalib: Handle percent signs in saved values

@abbra abbra closed this Feb 11, 2022
@abbra abbra mentioned this pull request Feb 11, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
ack Pull Request approved, can be merged ipa-4-9 Mark for backport to ipa 4.9 pushed Pull Request has already been pushed
Projects
None yet
Milestone
No milestone
5 participants
@Caligatio @rcritten @abbra @flo-renaud @freeipa-pr-ci