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

test_caless: add pkinit option and test it #769

Closed
wants to merge 2 commits into from

Conversation

Rezney
Copy link
Collaborator

@Rezney Rezney commented May 9, 2017

What was done?

1.) caless-create-pki

The script was kind of merged with https://github.com/freeipa/freeipa-tools/blob/master/makepki.sh. Standa took care of PKINIT certificates generation so that write_chain() function was introduced which handles cert chain in the pkcs12 files and also reverse chanin order for openssl command.

Then gen_pkinit_extensions() and gen_pkinit_cert() are handling the PKINIT certificate generation. See https://web.mit.edu/kerberos/krb5-1.13/doc/admin/pkinit.html for details.

2.) test_caless.py

As the tests are currently failing due to the pkinit option not provided "pkinit_pin, pkinit_pkcs12_exists and pkinit_pkcs12" parameters were added to both install_server() and prepare_replica methods and particular options are added to installator. Then copy_pkinit() is handling pkinit certs transfer.

TestPKINIT class contains test_server_replica_install_pkinit() test which checks both server and replica install with pkinit for a starter.

Eventually added "raiseonerr=False" to ipa_certs_cleanup() cause tests were failing there but that whole workaround for ticket 4639 will be removed in different commit.

What can be improved? (at least what I am aware of)

Currently pkinit certificates are not inside nss db so we copy it separately (we could also move it to certdir and copy as whole). Tried to put it there with pk12util but the certs were getting nicknames from openssl friendly names (I guess). Added -name parameter to "openssl pkcs12 -export" command and the nicknames were fine (e.g. "ca1/pkinit-server" after certuril -L) however after the "caless-create-pki" script was done all pkinit cert nicknames were just prefixed with "ca1/" (instead of ca1/ ca2/ etc.).

Issues found:

Replica install with pkinit is not failing anymore with "Certificate issuance failed (CA_UNREACHABLE)", however the ERROR message is still presented:


[ipa.ipatests.pytest_plugins.integration.host.Host.vm-021.cmd26]   [1/1]: installing X509 Certificate for PKINIT
[ipa.ipatests.pytest_plugins.integration.host.Host.vm-021.cmd26] ipa         : ERROR    PKINIT certificate request failed: Certificate issuance failed (CA_UNREACHABLE)
[ipa.ipatests.pytest_plugins.integration.host.Host.vm-021.cmd26] ipa         : ERROR    Failed to configure PKINIT
[ipa.ipatests.pytest_plugins.integration.host.Host.vm-021.cmd26] Done configuring Kerberos KDC (krb5kdc).
[ipa.ipatests.pytest_plugins.integration.host.Host.vm-021.cmd26] Applying LDAP updates
[ipa.ipatests.pytest_plugins.integration.host.Host.vm-021.cmd26] Upgrading IPA:. Estimated time: 1 minute 30 seconds
[ipa.ipatests.pytest_plugins.integration.host.Host.vm-021.cmd26]   [1/9]: stopping directory server

@stlaz stlaz self-assigned this May 9, 2017
@MartinBasti MartinBasti mentioned this pull request May 10, 2017
@stlaz
Copy link
Contributor

stlaz commented May 18, 2017

The changes are fine. Please note the licence at the original file which you're copying here:

 #!/bin/bash -e
 #
 # Copyright (c) 2015, Jan Cholasta <jcholast@redhat.com>
 #
 # Permission to use, copy, modify, and/or distribute this software for any
 # purpose with or without fee is hereby granted, provided that the above
 # copyright notice and this permission notice appear in all copies.
 #
 # THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
 # WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
 # MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
 # ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
 # WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
 # ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
 # OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.

I don't want to appear amid some multi-licence hell, so I suppose you should add at least the common header for FreeIPA files:

#
# Copyright (C) 2017  FreeIPA Contributors see COPYING for license
#

to caless-create-pki with the kind permission of @HonzaCholasta.

@stlaz
Copy link
Contributor

stlaz commented May 18, 2017

Please note that tests trying to use ipa-server-certinstall with certificates signed by an intermediate CA fail because of https://pagure.io/freeipa/issue/6955

@HonzaCholasta
Copy link
Contributor

@stlaz & @Rezney, kind permission given.

@stlaz
Copy link
Contributor

stlaz commented May 19, 2017

Please add the xfail part to another commit, put appropriate tracker tickets to the commit messages. I believe we should probably file a ticket that ipa-server-certinstall should add any intermediate CA certs a server cert is signed with and put that one in the xfail description so that we don't have a closed ticket there.

change "caless-create-pki" so pkinit certificates can be
generated.

See https://web.mit.edu/kerberos/krb5-1.13/doc/admin/pkinit.html for details.

add pkinit option to the ipa installer and test both master and replica
install with pkinit.

https://pagure.io/freeipa/issue/6854

Signed-off-by: Michal Reznik <mreznik@redhat.com>
mark TestCertinstall intermediate CA tests (http, ds) as xfail
until freeipa#6959 is solved

https://pagure.io/freeipa/issue/6959

Signed-off-by: Michal Reznik <mreznik@redhat.com>
@stlaz stlaz added the ack Pull Request approved, can be merged label May 19, 2017
@MartinBasti
Copy link
Contributor

master:

  • f7c4039 test_caless: add pkinit option and test it
  • d5e84d7 test_caless: mark TestCertinstall intermediate CA tests as xfail

ipa-4-5:

  • cea4242 test_caless: add pkinit option and test it
  • f9bf76e test_caless: mark TestCertinstall intermediate CA tests as xfail

@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label May 19, 2017
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 pushed Pull Request has already been pushed
Projects
None yet
4 participants