[Freeipa-devel] [patch 0038-0040] Sub CA test patches

[Fraser Tweedale]

On Fri, Jul 01, 2016 at 03:57:29PM +0200, Milan Kubík wrote:
> On 06/27/2016 01:31 PM, Milan Kubík wrote:
> > On 06/27/2016 02:57 AM, Fraser Tweedale wrote:
> > > On Fri, Jun 24, 2016 at 12:08:24PM +0200, Milan Kubík wrote:
> > > > On 06/24/2016 03:42 AM, Fraser Tweedale wrote:
> > > > > On Tue, Jun 21, 2016 at 05:01:35PM +0200, Milan Kubík wrote:
> > > > > > Hi Fraser and list,
> > > > > > 
> > > > > > I have made changes to the test plan on the wiki [1] according to the
> > > > > > information in "[Testplan review] Sub CAs" thread.
> > > > > > 
> > > > > > I also implemented the tests in the test plan:
> > > > > > 
> > > > > > patch 0038 - CATracker and CA CRUD test
> > > > > > patch 0039 - extension to CA ACL test
> > > > > > patch 0040 - functional test with ACLs and certificate
> > > > > > profile, reusing my
> > > > > > previous S/MIME based tests. This patch also tests for
> > > > > > the cert-request
> > > > > > behavior when profile ID or CA cn are ommited.
> > > > > > 
> > > > > > The tests ATM do not verify the Issuer name in the
> > > > > > certificate itself, just
> > > > > > from the ipa entry of the certificate.
> > > > > > 
> > > > > The approach you are using::
> > > > > 
> > > > >       assert cert_info['result']['issuer'] ==
> > > > > smime_signing_ca.ipasubjectdn
> > > > > 
> > > > > is not quite as you describe (these are virtual attributes, not
> > > > > attributes of an actual entry); but the approach is valid.
> > > > The issue then is in the wording? The other approach I could
> > > > have used here
> > > > is to retrieve the two certificates and compare the fields manually.
> > > > Are these virtual attributes created from the certificate itself?
> > > > 
> > > That's correct.
> > > 
> > > > > > Fraser, could you please verify my reasoning behind the
> > > > > > test cases for
> > > > > > cert-request in the patch 40?
> > > > > > 
> > > > > The tests look OK.  With the default CA / default profiles, is there
> > > > > appropriate isolation between test cases to ensure that if, e.g.
> > > > > some other test case adds/modifies CA ACLs such that these
> > > > > expected-to-fail tests now pass, that this does not affect the
> > > > > TestCertSignMIMEwithSubCA test case?
> > > > > 
> > > > > Thanks,
> > > > > Fraser
> > > > The ACL, SMIME CA and S/MIME profile lifetime is constrained by
> > > > the class
> > > > scope
> > > > enforced by pytest.
> > > > The two test cases depend on the fact documented in the designs
> > > > and that is
> > > > what
> > > > cert-request fallbacks to when CA or profile ID are not provided.
> > > > Unless something changes caIPAserviceCert profile or affiliated
> > > > ACL, then
> > > > the test cases
> > > > are safe.
> > > > 
> > > If you have thought about possible interference from other tests, I
> > > am happy.
> > > 
> > > Note another problematic scenario: what if a different (preceding)
> > > test adds a CA ACL that would allow the requests that you expect to
> > > fail?  Just something to think about :)
> > > 
> > > Thanks,
> > > Fraser
> > Then the failure would be problem of the preceding test and we would
> > need to fix it. We are dealing with test side effects
> > in other parts of the execution already...
> > 
> > The test is constructed in a way that isolates it (to a certain degree)
> > by the mechanisms available
> > in pytest. Of course I cannot make the test future-proof or guarantee
> > that a bug in some other test
> > will not affect the execution of other tests as they all run against one
> > IPA instance.
> > I do not think, however, that potential misbehaving test case that will
> > interfere
> > should prevent us from implementing this and similar test cases.
> > 
> > If you have some specific issue that is in the patch, I'm happy to fix
> > them.
> > > > I will try to think more about corner cases here.
> > > > > > [1]: http://www.freeipa.org/page/V4/Sub-CAs/Test_Plan
> > > > > > 
> > > > > > Cheers
> > > > > > 
> > > > > > -- 
> > > > > > Milan Kubik
> > > > > > 
> > > > Attaching rebased patches and removing the expected fail from
> > > > one of the
> > > > tests as ticket 5981 has fix posted.
> > > > 
> > > > -- 
> > > > Milan Kubik
> > > > 
> > 
> > 
> 
> Hi Fraser,
> 
> can we continue with the review, please?
> 
Hi Milan,

Yes, we can :)  Two issues, outlined below.


1)
Running the tests, I get error in
test_create_subca_with_subject_conflict cleanup::

    ____________ ERROR at teardown of TestCAbasicCRUD.test_create_subca_with_subject_conflict _____________

        def cleanup():
            created = self.exists
            try:
                del_command()

    <snip>
    E               NotFound: crud-subca-2: Certificate Authority not found


I do not know testing framework very well but it looks like
track_create() sets 'self.exists = True' before the create command
throws the (expected) DuplicateEntry error.  (These are called from
create() in the tracker 'base' class).  Later, cleanup() catches a
NotFound but re-throws it because it believes the entry should have
existed.


2)
the usercert.conf.tmpl does not like a subject base with spaces in
it, i.e. if 'openssl req' config template gets formatted like:

    [ dn ]
    commonName = "alice"
    o=IPA.LOCAL 201606201330

then 'openssl req' fails with nasty error like:

    140644791924600:error:0D06407A:asn1 encoding routines:a2d_ASN1_OBJECT:first num too large:a_object.c:108:
    140644791924600:error:0B083077:x509 certificate routines:X509_NAME_ENTRY_create_by_txt:invalid field name:x509name.c:295:name=o

and CalledProcessError gets raised and the test fails.

Simplest solution is to simply remove the '{ipacertbase}' from the
template, because AFAIK it is not needed and parsing and formatting
the certbase (which could have multiple AVAs) is more complex than
the test calls for, IMO.


Thanks,
Fraser