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

certupdate: update config when deployment becomes CA-ful #4861

Conversation

frasertweedale
Copy link
Contributor

@frasertweedale frasertweedale commented Jun 26, 2020

Another aged-in-oak branch I've had sitting around for years.

When a deployment gets promoted from CA-less to CA-ful other
replicas still have enable_ra=False in default.conf, and do not have
the ra-agent key and certificate. Enhance ipa-certupdate to detect
when the deployment has become CA-ful; retrieve the ra-agent
credential and update default.conf.

The rationale for adding this behaviour to ipa-certupdate is that it
is already necessary to use this command to update local trust
stores with the new CA certificate(s). So by using ipa-certupdate
we avoid introducing additional steps for administrators.

It is necessary to choose a CA master to use as the ca_host. We use
the first server returned by LDAP. A better heuristic might be to
choose a master in the same location but this is just left as a
comment unless or until the need is proven.

This change also addresses the case of a CA server being removed
from the topology, i.e. ipa-certupdate detects when non-CA replicas
are pointing at the removed server, and chooses a new ca_host.

HOW TO TEST:

  1. Install a CA-less server (first server).

  2. Install a CA-less replica.

  3. Run 'ipa-ca-install' on first server, promoting deployment from
    CA-less to CA-ful.

  4. Run 'ipa-certupdate' on second server.

  5. Exceute 'ipa cert-show 5' on second server. Should succeed,
    because ra-agent credential was retrieve and default.conf
    updated at step Fix man page ipa-replica-manage: remove duplicate -c option from --no-lookup #4.

Fixes: https://pagure.io/freeipa/issue/7188

@frasertweedale frasertweedale added ipa-4-8 Mark for backport to ipa 4.8 needs review Pull Request is waiting for a review WIP Work in progress - not ready yet for review and removed needs review Pull Request is waiting for a review labels Jun 26, 2020
@frasertweedale
Copy link
Contributor Author

Don't merge this yet. I think the next problem is that IPA RA cert is not present on the CA-less replicas after promoting deployment from CA-less to CA-ful. Need to investigate further, but out of time today.

@frasertweedale frasertweedale force-pushed the fix/7188-caless-to-caful-replica-config branch 2 times, most recently from eee1f53 to 8138a86 Compare June 30, 2020 02:08
@frasertweedale frasertweedale added needs review Pull Request is waiting for a review and removed WIP Work in progress - not ready yet for review labels Jun 30, 2020
@frasertweedale
Copy link
Contributor Author

Ready for review now. Test steps in PR description.

@frasertweedale frasertweedale added ipa-next Mark as master (4.13) only and removed ipa-4-8 Mark for backport to ipa 4.8 labels Jul 2, 2020
@frasertweedale
Copy link
Contributor Author

I'm suggesting ipa-next to be conservative, because there is a substantial new behaviour in ipa-certupdate. I think a shakedown in master only is a good idea before backports are considered.

@flo-renaud flo-renaud self-assigned this Jul 3, 2020
Copy link
Contributor

@flo-renaud flo-renaud left a comment

Choose a reason for hiding this comment

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

Hi @frasertweedale
Thanks for the PR. I have a few comments:

  • if the server where the CA is installed is in SElinux enforcing mode, ipa-certupdate fails on the replica in fetch_key. Probably an AVC on the CA server while getting the key with /ipa/keys/ra/ipaCert.
  • after the /etc/ipa/default.conf file has been updated, a restart of httpd is required, otherwise ipa cert-show commands fail.

@frasertweedale
Copy link
Contributor Author

@flo-renaud thanks, I am developing on f31 and it worked. I'll investigate on f32 next week. Cheers!

role_servrole=u'CA server',
status='enabled',
)
ca_servers = [server['server_server'] for server in resp['result']]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also going to require that the current principal has read access to roles. Today this works:

kinit someuser
ipa-certupdate

but ca_servers will be an empty list and I don't know what impact that will have.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see it will skip things. Still perhaps not expected but at least it won't break anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rcritten yes, empty result is handled. But also, unprivileged users can read serverrole objects with default ACLs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I must have run ipa server-find instead or something to verify. So yeah, this is fine.

@freeipa-pr-ci freeipa-pr-ci added the needs rebase Pull Request cannot be automatically merged - needs to be rebased label Jul 9, 2020
@frasertweedale frasertweedale force-pushed the fix/7188-caless-to-caful-replica-config branch from 8138a86 to 0b80587 Compare July 9, 2020 00:33
@frasertweedale
Copy link
Contributor Author

I still need to investigate the failures described by @flo-renaud.

@frasertweedale frasertweedale force-pushed the fix/7188-caless-to-caful-replica-config branch from 0b80587 to ebd0b57 Compare July 9, 2020 02:32
@frasertweedale
Copy link
Contributor Author

Hi @frasertweedale
Thanks for the PR. I have a few comments:

* if the server where the CA is installed is in SElinux enforcing mode, ipa-certupdate fails on the replica in fetch_key. Probably an AVC on the CA server while getting the key with /ipa/keys/ra/ipaCert.

* after the /etc/ipa/default.conf file has been updated, a restart of httpd is required, otherwise ipa cert-show commands fail.

@flo-renaud I was unable to reproduce this behaviour on f32. I encountered no issues with SELinux, and cert-show command worked immediately after ipa-certupdate (no explicit restart was required).

If you are still experiencing the issue could you please provide more details about your environment, transcript of steps, and any relevant logs.

Thanks for reviewing and testing this change!

@frasertweedale frasertweedale removed the needs rebase Pull Request cannot be automatically merged - needs to be rebased label Jul 9, 2020
@flo-renaud
Copy link
Contributor

Hi @frasertweedale
here is my procedure to reproduce the 2nd issue:

  • (server) ipa-server-install CA-less
  • (replica) ipa-client-install - kinit admin - ipa-replica-install CA-less
  • (server) ipa-ca-install
  • (replica) ipa-certupdate
  • (replica) ipa cert-show 5 => error
  • (replica) systemctl restart httpd; ipa cert-show 5 => success
    Both server and replica are installed with fedora32, updated with latest pkgs from updates repo.
    I wasn't able to reproduce the selinux issue, probably went away when I updated my machines.

Logs for the cert-show issue: on (replica)

ipa: ERROR: non-public: AttributeError: ra
Traceback (most recent call last):
  File "/usr/lib/python3.8/site-packages/ipalib/plugable.py", line 350, in __getattr__
    return self[key]
  File "/usr/lib/python3.8/site-packages/ipalib/plugable.py", line 342, in __getitem__
    plugin = self.get_plugin(key) 
  File "/usr/lib/python3.8/site-packages/ipalib/plugable.py", line 339, in get_plugin
    return self.__plugins_by_key[key]
KeyError: 'ra'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/lib/python3.8/site-packages/ipaserver/rpcserver.py", line 395, in wsgi_execute
    result = command(*args, **options)
  File "/usr/lib/python3.8/site-packages/ipalib/frontend.py", line 471, in __call__
    return self.__do_call(*args, **options)
  File "/usr/lib/python3.8/site-packages/ipalib/frontend.py", line 499, in __do_call
    ret = self.run(*args, **options)
  File "/usr/lib/python3.8/site-packages/ipalib/frontend.py", line 821, in run
    return self.execute(*args, **options)
  File "/usr/lib/python3.8/site-packages/ipaserver/plugins/cert.py", line 1373, in execute
    result = self.Backend.ra.get_certificate(str(serial_number))
  File "/usr/lib/python3.8/site-packages/ipalib/plugable.py", line 352, in __getattr__
    raise AttributeError(key)
AttributeError: ra
ipa: INFO: [jsonserver_session] admin@IPA.TEST: cert_show/1('1', version='2.239'): InternalError

@frasertweedale
Copy link
Contributor Author

@flo-renaud thank you, I have reproduced the issue.

Enhance cainstance.update_ipa_conf() to allow specifying the
ca_host.  This will be used to update replica configurations when a
CA-less deployment gets promoted to CA-ful.

Part of: https://pagure.io/freeipa/issue/7188
After upgrading a deployment from CA-less to CA-ful it is necessary
to install the RA Agent credential on non-CA servers.  To facilitate
this, extract this behaviour from CAInstance so that it is callable
from other code.

Several other methods became @staticmethod as a result of this
change.  This makes those methods callable without an instance of
CAInstance and also documents that those methods do not use 'self'.

Part of: https://pagure.io/freeipa/issue/7188
When a deployment gets promoted from CA-less to CA-ful other
replicas still have enable_ra=False in default.conf, and do not have
the ra-agent key and certificate.  Enhance ipa-certupdate to detect
when the deployment has become CA-ful; retrieve the ra-agent
credential and update default.conf.

The rationale for adding this behaviour to ipa-certupdate is that it
is already necessary to use this command to update local trust
stores with the new CA certificate(s).  So by using ipa-certupdate
we avoid introducing additional steps for administrators.

It is necessary to choose a CA master to use as the ca_host.  We use
the first server returned by LDAP.  A better heuristic might be to
choose a master in the same location but this is just left as a
comment unless or until the need is proven.

Finally, defer the httpd service restart until after the possible
update of default.conf so that the IPA API executes with the new
configuration.

This change also addresses the case of a CA server being removed
from the topology, i.e. ipa-certupdate detects when non-CA replicas
are pointing at the removed server, and chooses a new ca_host.

HOW TO TEST:

1. Install a CA-less server (first server).

2. Install a CA-less replica.

3. Run 'ipa-ca-install' on first server, promoting deployment from
   CA-less to CA-ful.

4. Run 'ipa-certupdate' on second server.

5. Exceute 'ipa cert-show 5' on second server.  Should succeed,
   because ra-agent credential was retrieved and default.conf
   updated at step freeipa#4.

Fixes: https://pagure.io/freeipa/issue/7188
@frasertweedale frasertweedale force-pushed the fix/7188-caless-to-caful-replica-config branch from ebd0b57 to 6688d2e Compare July 15, 2020 07:28
@frasertweedale
Copy link
Contributor Author

@flo-renaud I pushed an update that should avoid having to explicitly restart httpd. Thanks again for review and testing this change!

@frasertweedale frasertweedale added the re-run Trigger a new run of PR-CI label Jul 15, 2020
@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Jul 15, 2020
@flo-renaud
Copy link
Contributor

Hi @frasertweedale
I re-ran my procedure and it's now working with your update, thanks.
Also tested the uninstallation of a CA server (topology with server1: CA, server2: CA, server3: no CA), and the replica without CA is properly reconfigured to use the other CA.

@flo-renaud flo-renaud added ack Pull Request approved, can be merged and removed needs review Pull Request is waiting for a review labels Jul 15, 2020
@frasertweedale frasertweedale added the pushed Pull Request has already been pushed label Jul 16, 2020
@frasertweedale
Copy link
Contributor Author

master:

  • 2fcc260 cainstance.update_ipa_conf: allow specifying ca_host
  • a1b3b34 cainstance: extract function import_ra_key
  • 53d472b (HEAD) certupdate: update config after deployment becomes CA-ful

@frasertweedale frasertweedale deleted the fix/7188-caless-to-caful-replica-config branch July 16, 2020 05:32
@frasertweedale
Copy link
Contributor Author

@flo-renaud thank you so much for your thorough testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged ipa-next Mark as master (4.13) only pushed Pull Request has already been pushed
Projects
None yet
4 participants