Tests for certificates with SAN #73
Conversation
There was a problem hiding this comment.
LGTM except of some nitpicks. Tests pass.
We should probably consider some additional test cases like these:
1.) CA ACL contains host1, host2, and service1. Only host1 can manage service1. Host2 requests certificate for service1 using existing CSR and fails since it does not manage service1 (CA ACLs pass but can_write check does not).
2.) positive test case: same as above but host2 can now manage service1. Host2 should be granted CSR for service1 (both CA ACLs and can_write check pass).
I hope I got the cases right and they are valid.
| @@ -709,8 +711,12 @@ def change_principal(user, password, client=None, path=None, | |||
|
|
|||
| try: | |||
| with private_ccache(ccache_name): | |||
| kinit_password(user, password, ccache_name, | |||
| canonicalize=canonicalize, enterprise=enterprise) | |||
| if keytab: | |||
There was a problem hiding this comment.
I would rather be explicit here and user if keytab is not None:
There was a problem hiding this comment.
Is here a typo or word missing? I do not understand. If one specifies keytab I would assume the tester knows what he's doing.
I can document the context manager though.
| try: | ||
| cmd = [paths.IPA_GETKEYTAB, '-p', principal, '-k', keytab_filename] | ||
|
|
||
| if options: |
There was a problem hiding this comment.
keytab_filename is a file that will be created by ipa-getkeytab command, what is there to check?
| """Sign certificate request witn an invalid SAN entry | ||
|
|
||
| Using the environment prepared by the base class, ask to sign | ||
| a certificate request for a service managet by one host only. |
There was a problem hiding this comment.
Nitpick overload: typo in 'managet'
|
|
||
|
|
||
| @pytest.mark.tier1 | ||
| class TestSignServiceCertWithoudSANServiceInACL(CAACLEnforcementOnCertBase): |
| * two host entries | ||
| * santest-host-1 | ||
| * santest-host-2 | ||
| * one service entry |
There was a problem hiding this comment.
Aren't there two service entries? (santest_service_host_1 and santest_service_host_2)
| This test case doesn't have the service hosted on a host in SAN | ||
| in the CA ACL. The assumption is that the issuance will fail. | ||
|
|
||
| Using the environment of the base class, modify the service to be managed |
apophys
force-pushed
the
cert-request-not-allowed-SAN
branch
2 times, most recently
from
be2d6e9
to
74f5f45
Compare
|
NACK: you probably forgot to add service fixtures as params to the added test cases: https://paste.fedoraproject.org/437721/51355181/ In addition please write sensible commit message to commit f43833d and probably squash the last commit into 2d75883 I have also noticed that you linked the commits to a ticket in a already closed milestone. Per our process guidelines you need to open a new ticket and go through a new triage, sorry. |
The patch implements several test cases testing the enforcement of CA ACLs on certificate requests with subject alternative names. https://fedorahosted.org/freeipa/ticket/6366
apophys
force-pushed
the
cert-request-not-allowed-SAN
branch
from
74f5f45
to
f758041
Compare
|
I have fixed typos and implemented the proposed test cases. I have also provided docstring to the change_principal context manager. |
Commits include several new test cases for CA ACLs and cert request for CSRs containing subject alternative name extension.
Also included minor fixes in used tracker and couple of new context managers used in the test cases.