cert-request: match names against principal aliases #227

frasertweedale · 2016-11-10T11:36:55Z

Currently we do not check Kerberos principal aliases when validating
a CSR. Enhance cert-request to accept the following scenarios:

for hosts and services: CN and SAN dnsNames match a principal
alias (realm and service name must be same as nominated principal)
for all principal types: UPN or KRB5PrincipalName othername match
any principal alias.

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

martbab · 2016-11-14T12:37:33Z

@frasertweedale What is the intended semantics of the checks against principal aliases in SAN? If the requestor can use only the aliases belonging to the entry of the recieving principal, then it should be enough to retrieve the entry by searching for the 'krbprincipalname' from --principal option, retrieve it, and then checking whether all values of dnsName/KRB5PrincipalName are a subset of Kerberos principal aliases.

martbab · 2016-11-14T12:39:55Z

Also, the current execution flow of the command is very confusing (retrieving objects based on intended principal types etc.). As a part of the ticket I was planning to do a sneaky refactoring of the flow which IMHO should look like this:

1.) you search entries by krbprincipalname extracted from 'principal' option (or from bind principal)

2.) If not found, you error out that such entry could not be found

3.) due to syntax overrides in ipaldap, all returned principals will be converted to Principal objects so after you retrieve the entry and ensure that it exists you can test whether it is service, user, etc.

4.) for values in SAN, you check whether the value is already container in the entries principals (as you do in this PR). If the principal is not there, you can try to retrieve the entry from ldap and either error out if not found, or check CA ACLs against it when present.

5.) if all is OK, forward the request to RA backend and issue the certificate.

Do you think that this would extend the scope of the ticket too much? If yes, I can open a separate ticket for this cleanup and do it on top of your work.

martbab · 2016-11-14T13:13:52Z

Also one of the tests in caacl_profile_enforcement suite fails:
https://paste.fedoraproject.org/481011/12920714/

frasertweedale · 2016-11-17T00:08:14Z

@martbab thanks for review; I will revisit this some time in next week (hopefully)

frasertweedale · 2016-11-17T05:15:15Z

@martbab

Semantics:

Subject principal is looked up by --principal option, via {PRINCIPAL_TYPE}_show command. If you think this should be extended to allow --principal to use an alias, I am cool with that.
For host and service principals, CN must match[dns](described below) a principal alias.
For host and service principals, SAN dnsNames must match[dns] a principal alias, or match an alternative principal.
For all principals, SAN KRB5PrincipalName and UPN values must match[exact] a principal alias.

match[dns]: iterate principal aliases. Matches if: alias has same realm as --principal and alias has same service name as --principal and alias hostname equals (case insensitively) the SAN dnsName value. (If we generalise --principal to search all aliases then I would recommend restricting the search to principals with same realm and service name as the krbcanonicalname of the returned principal).

w.r.t. test failure, I cannot reproduce with this patch rebased on latest master.

frasertweedale · 2016-12-02T04:32:28Z

@martbab I agree with doing the refactor you propose, but I deem it out of scope for this ticket.

Doing that refactor entails a cleanup of how we handle the LDAP ;binary transfer encoding option, because this is currently not consistent across user, host and service plugins.

So... do we want to look at getting this merged soon?

martbab · 2016-12-02T08:13:38Z

I agree that it is out of scope of this work and will open a separate ticket for the cert-request refactoring.

martbab · 2016-12-02T12:30:54Z

ipaserver/plugins/cert.py

+
+        # ignore aliases with different realm or service name from
+        # subject principal
+        if alias.realm != principal.realm:


Nitpick: we (currently) do not support aliases w/ realm that is different than IPA realm (*-add-principal will error out loudly) so I am not sure whether we require this check.

I'm going to leave it in anyway. It does no harm and is one less potential bug if we do ever support aliases with different realms, somehow.

martbab · 2016-12-02T12:32:11Z

ipaserver/plugins/cert.py

+
+    """
+    aliases = set(principal_obj.get('krbprincipalname', []))
+    for alias in principal_obj.get('krbcanonicalname', []):


As per http://www.freeipa.org/page/V4/Kerberos_principal_aliases the canonical name MUST be listed among the values of 'krbprincipalname' attribute so there is no need to explicitly add it here (results in a no-op all the time).

OK, thanks. I've removed function _collect_princpial_aliases() and just replaced with principal_obj.get('krbprincipalname', []) at call sites.

martbab · 2016-12-02T12:34:57Z

ipatests/test_xmlrpc/test_caacl_profile_enforcement.py

+
+@pytest.mark.tier1
+class TestPrincipalAliasForSubjectAltNameDnsName(SubjectAltNameOneServiceBase):
+    """Sign certificate request with an invalid SAN dnsName.


Copy-paste error in the docstring here?

Yep, thanks. Corrected in updated patch.

martbab · 2016-12-02T12:39:37Z

ipatests/test_xmlrpc/test_caacl_profile_enforcement.py

+            self, santest_service_host_1):
+        api.Command.service_add_principal(
+            santest_service_host_1.name,
+            'srv/santest-host-2.ipa.local')


Please use santest_host_2 fixture here and use santest_1_host_2.attrs['krbcanonicalname'][0] to construct the alias instead of hard-coding the alias name as string. This test fails due to adding principal name for srv/santest-host-2.ipa.local while the service name in fixture is srv/santest-host-2.ipa.*test*

Thanks for clarifying. I was wonder how the test was failing. It is obvious now thanks to your explanation :)

martbab · 2016-12-02T15:15:50Z

There are small issues I found in the patch. Please address those.

@apophys can quickly you review the new test cases? they pass (except for one pointed out in review) so they should be valid and OK.

martbab · 2016-12-05T11:29:11Z

ipaserver/plugins/cert.py

+    except ValueError:
+        return False
+
+    return principal in principal_obj.get('krbprincpialname', [])


Please fix typo in attribute name.

hah, silly fat fingers. push incoming

Currently we do not check Kerberos principal aliases when validating a CSR. Enhance cert-request to accept the following scenarios: - for hosts and services: CN and SAN dnsNames match a principal alias (realm and service name must be same as nominated principal) - for all principal types: UPN or KRB5PrincipalName othername match any principal alias. Fixes: https://fedorahosted.org/freeipa/ticket/6295

apophys · 2016-12-05T13:30:33Z

The tests look good to me.

martbab · 2016-12-06T15:14:12Z

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/dfbdb5323863e6c3d681c1b33b1eb9d2efefd6c7

frasertweedale force-pushed the feature/6432-cert-request-san-princ-alias branch 2 times, most recently from 8fb85f2 to 6156cf7 Compare November 14, 2016 01:15

frasertweedale changed the title ~~cert-request: match names against principal alises~~ cert-request: match names against principal aliases Nov 14, 2016

martbab self-assigned this Nov 14, 2016

martbab suggested changes Dec 2, 2016

View reviewed changes

frasertweedale force-pushed the feature/6432-cert-request-san-princ-alias branch from 6156cf7 to b84e266 Compare December 5, 2016 11:26

martbab reviewed Dec 5, 2016

View reviewed changes

frasertweedale force-pushed the feature/6432-cert-request-san-princ-alias branch from b84e266 to c347ff8 Compare December 5, 2016 11:31

martbab added the ack Pull Request approved, can be merged label Dec 5, 2016

martbab approved these changes Dec 5, 2016

View reviewed changes

martbab added the pushed Pull Request has already been pushed label Dec 6, 2016

martbab closed this Dec 6, 2016

frasertweedale deleted the feature/6432-cert-request-san-princ-alias branch December 7, 2016 00:18

cert-request: match names against principal aliases #227

cert-request: match names against principal aliases #227

frasertweedale commented Nov 10, 2016 •

edited by martbab

martbab commented Nov 14, 2016

martbab commented Nov 14, 2016

martbab commented Nov 14, 2016

frasertweedale commented Nov 17, 2016

frasertweedale commented Nov 17, 2016

frasertweedale commented Dec 2, 2016

martbab commented Dec 2, 2016 •

edited

martbab Dec 2, 2016

frasertweedale Dec 5, 2016

martbab Dec 5, 2016

martbab Dec 2, 2016

frasertweedale Dec 5, 2016

martbab Dec 2, 2016

frasertweedale Dec 5, 2016

martbab Dec 2, 2016

frasertweedale Dec 5, 2016

martbab commented Dec 2, 2016

martbab Dec 5, 2016

frasertweedale Dec 5, 2016

apophys commented Dec 5, 2016

martbab commented Dec 6, 2016

cert-request: match names against principal aliases #227

cert-request: match names against principal aliases #227

Conversation

frasertweedale commented Nov 10, 2016 • edited by martbab

martbab commented Nov 14, 2016

martbab commented Nov 14, 2016

martbab commented Nov 14, 2016

frasertweedale commented Nov 17, 2016

frasertweedale commented Nov 17, 2016

frasertweedale commented Dec 2, 2016

martbab commented Dec 2, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martbab commented Dec 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apophys commented Dec 5, 2016

martbab commented Dec 6, 2016

frasertweedale commented Nov 10, 2016 •

edited by martbab

martbab commented Dec 2, 2016 •

edited