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

Remove pkinit options from master/replica on DL0 #640

Closed
wants to merge 4 commits into from
Closed

Remove pkinit options from master/replica on DL0 #640

wants to merge 4 commits into from

Conversation

stlaz
Copy link
Contributor

@stlaz stlaz commented Mar 22, 2017 •
edited by MartinBasti

This patchset removes the ability of setting pkinit options on domain level 0 for server/replica installs. Also fixes a usability issue with --no-pkinit I noticed and did not care creating ticket for.

@stlaz stlaz changed the title Master replica dl0 Remove pkinit options from master/replica on DL0 Mar 22, 2017
Akasurde
Akasurde reviewed Mar 24, 2017
View reviewed changes
ipaserver/install/server/__init__.py Outdated
if (self.no_pkinit or self.pkinit_cert_files is not None or
self.pkinit_pin is not None):
raise RuntimeError(
"pkinit on domain level 0 is not supported. Please don't "
Copy link
Member

Choose a reason for hiding this comment

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

Can you please edit man page as well do describe this change ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will put the behavior of no-pkinit on domain level 0 to the correct man pages.
The reasoning to why this certain change was done is to be found in the commit message.

@MartinBasti MartinBasti self-assigned this Mar 24, 2017
@martbab
Copy link
Contributor

martbab commented Mar 24, 2017

@abbra I believe these changes are in line with our recent discussion regarding pkinit availability on DL0. Do you agree?

MartinBasti
MartinBasti reviewed Mar 24, 2017
View reviewed changes
ipaserver/install/server/__init__.py Outdated
@@ -335,6 +335,14 @@ def dirsrv_config_file(self, value):
def __init__(self, **kwargs):
super(ServerInstallInterface, self).__init__(**kwargs)

if self.domain_level == constants.DOMAIN_LEVEL_0:
if (self.no_pkinit or self.pkinit_cert_files is not None or
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd drop self.no_pkinit from expression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we allow --no-pkinit to be specified on DL0 the user might think that not-using it may change something for them IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

But it is contradicting, user specifies --no-pkinit and get error pkinit on domain level 0 is not supported

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's why there's the part not to use any pkinit-related options.

@abbra
Copy link
Contributor

abbra commented Mar 24, 2017

Good question. I think we should remove all mentioning of PKINIT options for DL0 and explicitly configure local CA there. On DL1 we already require to provide pkinit cert for CA-less setup. However, there we should treat --no-pkinit as use of local CA (certmonger's one).

@MartinBasti
Copy link
Contributor 

ipa-replica-install --no-pkinit  (as negative test without master installed)

2017-03-27T17:04:09Z DEBUG Logging to /var/log/ipareplica-install.log
2017-03-27T17:04:09Z DEBUG   File "/usr/lib/python2.7/site-packages/ipapython/admintool.py", line 172, in execute
    return_value = self.run()
  File "/usr/lib/python2.7/site-packages/ipapython/install/cli.py", line 314, in run
    cfgr = transformed_cls(**kwargs)
  File "/usr/lib/python2.7/site-packages/ipapython/install/common.py", line 102, in __init__
    **kwargs)
  File "/usr/lib/python2.7/site-packages/ipaserver/install/server/__init__.py", line 602, in __init__
    super(ServerReplicaInstall, self).__init__(**kwargs)
  File "/usr/lib/python2.7/site-packages/ipaserver/install/server/__init__.py", line 338, in __init__
    if self.domain_level == constants.DOMAIN_LEVEL_0:
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 611, in __getattr__
    raise AttributeError(name)

2017-03-27T17:04:09Z DEBUG The ipa-replica-install command failed, exception: AttributeError: domain_level

@stlaz
Copy link
Contributor Author

stlaz commented Mar 28, 2017

Ah, right, replica does not have domain_level option 🙄

@MartinBasti MartinBasti mentioned this pull request Mar 28, 2017
Closed
@MartinBasti
Copy link
Contributor

With this PR applied I cannot use webUI with DL0

@stlaz
Fix the order of cert-files check
53cdc14 
Without this patch, if either of dirsrv_cert_files, http_cert_files
or pkinit_cert_files is set along with no-pkinit, the user is first
requested to add the remaining options and when they do that,
they are told that they are using 'no-pkinit' along with
'pkinit-cert-file'.

https://pagure.io/freeipa/issue/6801
@stlaz
Copy link
Contributor Author

stlaz commented Mar 29, 2017 •
edited

@MartinBasti Even though this patchset basically breaks the behavior, it's not in its scope to fix it, it's somehow intended to break it, actually. It will be fixed elsewhere.

I fixed the issue with running this on replica and removed one redundant check as well.

I also noticed that DL0 replica has a usability issue where it checks for either *-cert-file option and requires them all, once it has it, it will say that these options can't be used with replica file. I will not fix that here, though.

@stlaz
Don't allow setting pkinit-related options on DL0
6620562 
pkinit is not supported on DL0, remove options that allow to set it
from ipa-{server,replica}-install.

https://pagure.io/freeipa/issue/6801
@stlaz
replica-prepare man: remove pkinit option refs
b9d9a73 
Remove the references to the pkinit options which was forgotten
about in 46d4d53

https://pagure.io/freeipa/issue/6801
@stlaz
Remove redundant option check for cert files
fc5315b 
There was a redundant check for CA-less install certificate files
for replicas but the same check is done for all installers before
that.

https://pagure.io/freeipa/issue/6801
@stlaz
Copy link
Contributor Author

stlaz commented Mar 29, 2017

Pushed a cleaner version of the previous changes, thanks @HonzaCholasta for the suggestion.

@martbab
Copy link
Contributor

martbab commented Mar 29, 2017

@MartinBasti WebUI not working in DL0/--no-pkinit is beyond the scope of this PR. I am working on fixing that in a separate PR.

@MartinBasti MartinBasti added ack Pull Request approved, can be merged pushed Pull Request has already been pushed labels Mar 30, 2017
@MartinBasti
Copy link
Contributor

master:

  • 6cda150 Fix the order of cert-files check
  • 9e3ae78 Don't allow setting pkinit-related options on DL0
  • 8af884d replica-prepare man: remove pkinit option refs
  • fe7cf1e Remove redundant option check for cert files

ipa-4-5:

  • 497e766 Fix the order of cert-files check
  • a1ad1ff Don't allow setting pkinit-related options on DL0
  • 85720b6 replica-prepare man: remove pkinit option refs
  • 8f7b6c3 Remove redundant option check for cert files

@stlaz stlaz deleted the master_replica_dl0 branch September 11, 2017 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Akasurde Akasurde Akasurde left review comments

@MartinBasti MartinBasti MartinBasti left review comments

Assignees

@MartinBasti MartinBasti

Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
Milestone
No milestone
5 participants
@stlaz @martbab @abbra @MartinBasti @Akasurde