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

Remove "Request Certificate with SubjectAltName" permission #299

Conversation

frasertweedale
Copy link
Contributor

@frasertweedale frasertweedale commented Dec 2, 2016

Fixes: https://fedorahosted.org/freeipa/ticket/6526

Note: the ticket hasn't been triaged or even agreed to. But here is the code
^_^

subjectAltName is required or relevant in most certificate use cases
(esp. TLS, where carrying DNS name in Subject DN CN attribute is
deprecated). Therefore it does not really make sense to have a
special permission for this, over and above "request certificate"
permission.

Furthermore, we already do rigorously validate SAN contents again
the subject principal, and the permission is waived for self-service
requests or if the operator is a host principal.

So remove the permission, the associated virtual operation, and the
associated code in cert_request.

@martbab
Copy link
Contributor

martbab commented Dec 12, 2016

I have put on my Travis moustache and found these two failing tests, you will have to fix them:

=================================== FAILURES ===================================
 test_permission_legacy.test_command[0000: permission_find: Check that some legacy permission is found in $SUFFIX]


self = <ipatests.test_xmlrpc.test_permission_plugin.test_permission_legacy object at 0x7fbf642026d0>
index = 0
declarative_test_definition = {'command': ('permission_find', [], {'ipapermlocation': ipapython.dn.DN('dc=ipa,dc=test'), 'version': '2.216'}), 'desc...6e430230>, 'truncated': False}, 'nice': '0000: permission_find: Check that some legacy permission is found in $SUFFIX'}
    def test_command(self, index, declarative_test_definition):
        """Run an individual test

            The arguments are provided by the pytest plugin.
            """
        if callable(declarative_test_definition):
            declarative_test_definition(self)

        else:

>           self.check(**declarative_test_definition)

/usr/lib/python2.7/site-packages/ipatests/test_xmlrpc/xmlrpc_test.py:318:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
/usr/lib/python2.7/site-packages/ipatests/test_xmlrpc/xmlrpc_test.py:330: in check
    self.check_output(nice, cmd, args, options, expected, extra_check)
/usr/lib/python2.7/site-packages/ipatests/test_xmlrpc/xmlrpc_test.py:379: in check_output
    assert_deepequal(expected, got, nice)
/usr/lib/python2.7/site-packages/ipatests/util.py:388: in assert_deepequal
    assert_deepequal(e_sub, g_sub, doc, stack + (key,))
/usr/lib/python2.7/site-packages/ipatests/util.py:390: in assert_deepequal
    if not expected(got):
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

results = [{'attrs': ('objectclass',), 'cn': ('Certificate Remove Hold',), 'dn': 'cn=Certificate Remove Hold,cn=permissions,cn=p...eve Certificates from the CA,cn=permissions,cn=pbac,dc=ipa,dc=test', 'ipapermbindruletype': ('permission',), ...}, ...]

    def check_legacy_results(results):
        """Check that the expected number of legacy permissions are in $SUFFIX"""
        legacy_permissions = [p for p in results
                              if not p.get('ipapermissiontype')]

        print(legacy_permissions)

>       assert len(legacy_permissions) == 9, len(legacy_permissions)
E       AssertionError: 8
E       assert 8 == 9

E        +  where 8 = len([{'attrs': ('objectclass',), 'cn': ('Certificate Remove Hold',), 'dn': 'cn=Certificate Remove Hold,cn=permissions,cn=p...eve Certificates from the CA,cn=permissions,cn=pbac,dc=ipa,dc=test', 'ipapermbindruletype': ('permission',), ...}, ...])

/usr/lib/python2.7/site-packages/ipatests/test_xmlrpc/test_permission_plugin.py:3128: AssertionError

I also wonder if there is a possibility for this removal to break replica install against older (IPA v3) masters.

@martbab
Copy link
Contributor

martbab commented Dec 20, 2016

Bumping this PR as it seems a bit forgotten.

@frasertweedale
Copy link
Contributor Author

frasertweedale commented Dec 20, 2016 via email

subjectAltName is required or relevant in most certificate use cases
(esp. TLS, where carrying DNS name in Subject DN CN attribute is
deprecated).  Therefore it does not really make sense to have a
special permission for this, over and above "request certificate"
permission.

Furthermore, we already do rigorously validate SAN contents again
the subject principal, and the permission is waived for self-service
requests or if the operator is a host principal.

So remove the permission, the associated virtual operation, and the
associated code in cert_request.

Fixes: https://fedorahosted.org/freeipa/ticket/6526
@frasertweedale frasertweedale force-pushed the fix/6526-rm-req-cert-with-san-perm branch from 524e1ab to 837a225 Compare December 21, 2016 06:52
@frasertweedale
Copy link
Contributor Author

@martbab I don't think this will break migrations from v3; it does not actively remove the permission from existing deployments, it just doesn't add it for new installations. (Admittedly, it is the next thing to test but I have not done so yet).

@martbab martbab added the ack Pull Request approved, can be merged label Dec 21, 2016
@martbab
Copy link
Contributor

martbab commented Dec 21, 2016

@martbab martbab added the pushed Pull Request has already been pushed label Dec 21, 2016
@martbab martbab closed this Dec 21, 2016
@frasertweedale frasertweedale deleted the fix/6526-rm-req-cert-with-san-perm branch December 21, 2016 22:00
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
2 participants