Fixing the cert-request command comparing whole email address case-sensitively. #736

felipevolpone · 2017-04-26T14:12:00Z

Now, the cert-request command compares the domain part of the email case-insensitively.

Fixes: https://pagure.io/freeipa/issue/5919

frasertweedale

The approach is good. A couple of things to address (comments inline).

frasertweedale · 2017-04-27T00:18:57Z

ipaserver/plugins/cert.py

+    """
+
+    def lower_domain(email):
+        return email.split('@')[0] + '@' + email.split('@')[1].lower()


The case where the input has multiple '@' characters should be accounted for. Otherwise we
could end up accepting an invalid value. We must also handle the case where input does not
have an '@' character. Obviously this is not a valid email but the current code would raise
IndexError.

Also we should avoid doing the split multiple times.

frasertweedale · 2017-04-27T00:20:24Z

ipaserver/plugins/cert.py

@@ -705,7 +705,8 @@ def execute(self, csr, all=False, raw=False, chain=False, **kw):
            # fail if any email addr from DN does not appear in ldap entry
            email_addrs = csr_obj.subject.get_attributes_for_oid(
                    cryptography.x509.oid.NameOID.EMAIL_ADDRESS)
-            if len(set(email_addrs) - set(principal_obj.get('mail', []))) > 0:
+            if not _emails_are_valid(email_addrs,
+                                     principal_obj.get('mail', [])):


This is checking the email addresses (if any) in the Subject DN. We also need to check rfc822Names in the
Subject Alternative Name extension.

felipevolpone · 2017-05-02T15:42:18Z

@frasertweedale I did the check in SAN extension.

However, I'm not sure if these are valid situations:
Case 1)
The principal email is A@email.com
The email in the certificate is B@email.com
The emails in the SAN extensions are: A@email.com, C@email.com

or this:

Case 2)
The principal email is A@email.com
The email in the certificate is B@email.com, A@email.com
The email in the SAN extensions is: C@email.com

If the case 1 is valid, the check in line 799 (below) is not right, because it expects that all emails in SAN extension are in the principal.

elif isinstance(gn, cryptography.x509.general_name.RFC822Name):
    if principal_type == USER:
        if principal_obj and gn.value not in principal_obj.get(
                'mail', []):
            raise errors.ValidationError(
                name='csr',
                error=_(
                    "RFC822Name does not match "
                    "any of user's email addresses")
            )
    else:
        raise errors.ValidationError(
            name='csr',
            error=_(
                "subject alt name type %s is forbidden "
                "for non-user principals") % "RFC822Name"

frasertweedale · 2017-05-04T02:46:40Z

ipaserver/plugins/cert.py

@@ -705,7 +705,13 @@ def execute(self, csr, all=False, raw=False, chain=False, **kw):
            # fail if any email addr from DN does not appear in ldap entry
            email_addrs = csr_obj.subject.get_attributes_for_oid(
                    cryptography.x509.oid.NameOID.EMAIL_ADDRESS)
-            if len(set(email_addrs) - set(principal_obj.get('mail', []))) > 0:
+
+            san_email_addrs = csr_obj.extensions.get_extension_for_oid(


Actually, a bit later on we already retrieve, parse and check the SAN extension. See the code following:

elif isinstance(gn, cryptography.x509.general_name.RFC822Name): ...

This is where the check for SAN emails should occur.

Also, this is not correct - there are various types of SAN and they must be checked to see what type they
are. Only the email type (RFC822Name) is checked for email match.

frasertweedale · 2017-05-04T02:48:22Z

ipaserver/plugins/cert.py

+                    cryptography.x509.oid.ExtensionOID
+                                     .SUBJECT_ALTERNATIVE_NAME)
+
+            if not _emails_are_valid(email_addrs, san_email_addrs,


I think we want to check a single iterable of emails, and then either a) call this function from two
different places or b) union all the emails from Subject DN and SAN, and call it once.

I prefer the option the option "a", since it seems more straightforward.

frasertweedale · 2017-05-04T02:51:17Z

ipaserver/plugins/cert.py

@@ -860,6 +866,33 @@ def execute(self, csr, all=False, raw=False, chain=False, **kw):
        )


+def _emails_are_valid(subject_emails, san_emails, principal_emails):


As discussed above, we probably just want to check a single iterable, and call this
function twice - once for Subject DN emails and once for SAN rfc822name values.

def _emails_are_valid(principal_emails, csr_emails): ...

frasertweedale · 2017-05-04T02:56:10Z

ipaserver/plugins/cert.py

+    san_emails_lower = list(map(lower_domain, san_emails))
+
+    return (set(principal_emails_lower).issubset(subject_emails_lower) or
+            set(principal_emails_lower).issubset(san_emails_lower))


This is not the correct way to check. Emails in the CSR must be a subset of principal principal emails,
not the other way around. Every email in the CSR must match one of the principal emails.

frasertweedale · 2017-05-04T03:09:39Z

ipaserver/plugins/cert.py

+            return ''
+
+        email_splited = email.split('@')
+        return '{}@{}'.format(email_splited[0], email_splited[1].lower())


This logic still needs to be fixed. It is simple to construct a CSR with emails that have no @ (or multiple).
(It probably possible to make a bogus LDAP attribute value, too).

So I'd suggest something like:

email_split = email.split('@', 1) # split only at first occurrence of '@' if len(email_split) > 1: email_split[1] = email_split[1].lower() return email_split

There is no need to "rejoin" the parts back into a string; we can compare on the resulting
lists. If you end up using set then you can turn the lists into tuples so that you have a
hashable value.

I did just a little bit different because the example above it doesn't work. The split method would return a list like ["fbarreto", "redhat.com"], so, it's necessary to join the two parts with a @.

felipevolpone · 2017-05-04T20:17:22Z

I hope it's fine now

frasertweedale

Nice work, getting into "nit-pick" territory now. I'll try to build and test your changes today.

frasertweedale · 2017-05-04T22:39:36Z

ipaserver/plugins/cert.py

-                    if principal_obj and gn.value not in principal_obj.get(
-                            'mail', []):
+                    gn_value = (gn.value if isinstance(gn.value, list)
+                                         else [gn.value])


I think gn.value will always be a string, so you can just put:

if not _emails_are_valid([gn.value], principal_obj.get('mail', [])):

If I'm wrong about that, could you please explain?

In all tests that I did, gn.value was a string. However, since I don't know very much all about the code, I thought it would be a "safer" solution. Changed to your suggestion :)

frasertweedale · 2017-05-04T22:41:38Z

ipaserver/plugins/cert.py

+        email_splited = email.split('@', 1)
+        email_splited[1] = email_splited[1].lower()
+
+        return '@'.join(email_splited)


The join is OK, but I think not necessary, because you will always be comparing values
returned from lower_domain, and lists will compare properly. So joining is just a bit of
unneeded but harmless work.

Yeah, I prefer to stay with the '@' join to be more consistent with the method name and all the context. If you think that it's not relevant at all, I can just remove it :)

Well, it's not needed, but it doesn't hurt, and I do not mind if we keep it.

frasertweedale · 2017-05-04T22:43:57Z

ipaserver/plugins/cert.py

+    principal_emails_lower = set(map(lower_domain, principal_emails))
+    csr_emails_lower = set(map(lower_domain, csr_emails))
+
+    return csr_emails_lower.issubset(principal_emails_lower)


This looks correct 👍

frasertweedale · 2017-05-04T22:44:37Z

ipatests/test_xmlrpc/test_cert_plugin.py

+        NameAttr = namedtuple('NameAttr', 'value')
+
+        subject_addrs = [NameAttr(u'any@EmAiL.CoM')]
+        result = _emails_are_valid(subject_addrs, [], [u'any@email.com'])


These tests need updating now that signature of _emails_are_valid changed.

Done. I was waiting to we agree with the implementation and then change the tests.

frasertweedale · 2017-05-10T06:35:12Z

ipaserver/plugins/cert.py

+    """
+
+    if not any(principal_emails) or not any(csr_emails):
+        return False


We should get rid of this. The subject check is sufficient. If there are no csr_emails, then
the test should be vacuously True, not False. Similarly, if there are no principal_emails, then
the valid if and only if there are also no csr_email. All of this is covered by the subset check,
so just remove these two lines.

frasertweedale · 2017-05-10T06:36:04Z

ipaserver/plugins/cert.py

+        email_splited = email.split('@', 1)
+        email_splited[1] = email_splited[1].lower()
+
+        return '@'.join(email_splited)


Well, it's not needed, but it doesn't hurt, and I do not mind if we keep it.

frasertweedale · 2017-05-10T06:37:35Z

ipatests/test_xmlrpc/test_cert_plugin.py

+        assert True == result, result
+
+        result = _emails_are_valid([], [u'any@email.com'])
+        assert False == result, result


As discussed above, this case should be True.

felipevolpone · 2017-05-10T13:34:59Z

ipaserver/plugins/cert.py

-        email_splited = email.split('@', 1)
-        email_splited[1] = email_splited[1].lower()
+        email_splitted = email.split('@', 1)
+        if len(email_splitted) > 1:


This became necessary since we don't have the if statement (in line 964) anymore.

frasertweedale

Now we are good! Thanks for your work. Just one
tiny docstring fix, and please squash the commits, then we can merge this.

frasertweedale · 2017-05-11T04:54:52Z

ipaserver/plugins/cert.py

@@ -953,6 +955,25 @@ def execute(self, csr, all=False, raw=False, chain=False, **kw):
        )


+def _emails_are_valid(csr_emails, principal_emails):
+    """
+    Checks if any email address from certificate does not


Should say "certificate request"

Now, the cert-request command compares the domain part of the email case-insensitively. https://pagure.io/freeipa/issue/5919

felipevolpone · 2017-05-11T13:28:41Z

Done! Thank you Fraser :)) 👍

frasertweedale · 2017-05-12T02:04:25Z

ipaserver/plugins/cert.py

@@ -884,8 +886,8 @@ def execute(self, csr, all=False, raw=False, chain=False, **kw):
                            "match requested principal") % gn.name)
            elif isinstance(gn, cryptography.x509.general_name.RFC822Name):
                if principal_type == USER:
-                    if principal_obj and gn.value not in principal_obj.get(


@felipevolpone sorry mate, one more tiny thing that I missed. We need to retain the if principal_obj and ... because there is a scenario where principal_obj = None - namely, the PKINIT KDC certificate use case. While it's unlikely that the CSR in this use case will include an RFC822Name, we should make the code resilient to this case.

Hi Fraser, ok, no problem.
So, we should have this?

if principal_obj and not _emails_are_valid([gn.value], principal_obj.get('mail', [])):

@felipevolpone actually, I was confused. This whole bit is guarded by an if principal_type == USER, and if that is true, then principal_obj is set. So we are actually good to go; the check that was there before your change was
superfluous.

frasertweedale

Good to go.

frasertweedale · 2017-05-15T00:19:12Z

ipaserver/plugins/cert.py

@@ -884,8 +886,8 @@ def execute(self, csr, all=False, raw=False, chain=False, **kw):
                            "match requested principal") % gn.name)
            elif isinstance(gn, cryptography.x509.general_name.RFC822Name):
                if principal_type == USER:
-                    if principal_obj and gn.value not in principal_obj.get(


@felipevolpone actually, I was confused. This whole bit is guarded by an if principal_type == USER, and if that is true, then principal_obj is set. So we are actually good to go; the check that was there before your change was
superfluous.

MartinBasti · 2017-05-16T08:14:57Z

master:

d973168 Fixing the cert-request comparing whole email address case-sensitively.

frasertweedale self-requested a review April 27, 2017 00:12

frasertweedale requested changes Apr 27, 2017

View reviewed changes

felipevolpone force-pushed the fix-5919 branch from e8d567c to e98274b Compare May 3, 2017 19:30

frasertweedale requested changes May 4, 2017

View reviewed changes

felipevolpone force-pushed the fix-5919 branch from 4cab064 to b1077cf Compare May 4, 2017 20:12

frasertweedale requested changes May 4, 2017

View reviewed changes

felipevolpone force-pushed the fix-5919 branch 2 times, most recently from 85e931f to 97725d8 Compare May 5, 2017 14:25

frasertweedale reviewed May 10, 2017

View reviewed changes

felipevolpone force-pushed the fix-5919 branch from b45cab8 to ac87ca8 Compare May 10, 2017 13:33

felipevolpone commented May 10, 2017

View reviewed changes

frasertweedale requested changes May 11, 2017

View reviewed changes

Fixing the cert-request comparing whole email address case-sensitively.

6eb1169

Now, the cert-request command compares the domain part of the email case-insensitively. https://pagure.io/freeipa/issue/5919

felipevolpone force-pushed the fix-5919 branch from ac87ca8 to 6eb1169 Compare May 11, 2017 13:20

frasertweedale reviewed May 12, 2017

View reviewed changes

frasertweedale approved these changes May 15, 2017

View reviewed changes

frasertweedale added the ack Pull Request approved, can be merged label May 15, 2017

MartinBasti added the pushed Pull Request has already been pushed label May 16, 2017

MartinBasti closed this May 16, 2017

Fixing the cert-request command comparing whole email address case-sensitively. #736

Fixing the cert-request command comparing whole email address case-sensitively. #736

felipevolpone commented Apr 26, 2017 •

edited by MartinBasti

frasertweedale left a comment

frasertweedale Apr 27, 2017

frasertweedale Apr 27, 2017

felipevolpone commented May 2, 2017

frasertweedale May 4, 2017

frasertweedale May 4, 2017

felipevolpone May 4, 2017

frasertweedale May 4, 2017

frasertweedale May 4, 2017

felipevolpone May 4, 2017

frasertweedale May 4, 2017

felipevolpone May 4, 2017

felipevolpone commented May 4, 2017

frasertweedale left a comment

frasertweedale May 4, 2017

felipevolpone May 5, 2017

frasertweedale May 4, 2017

felipevolpone May 5, 2017

frasertweedale May 10, 2017

frasertweedale May 4, 2017

frasertweedale May 4, 2017

felipevolpone May 5, 2017

frasertweedale May 10, 2017

felipevolpone May 10, 2017

frasertweedale May 10, 2017

frasertweedale May 10, 2017

felipevolpone May 10, 2017

frasertweedale left a comment

frasertweedale May 11, 2017

felipevolpone commented May 11, 2017

frasertweedale May 12, 2017

felipevolpone May 12, 2017

frasertweedale May 15, 2017

frasertweedale left a comment

frasertweedale May 15, 2017

MartinBasti commented May 16, 2017

		@@ -860,6 +866,33 @@ def execute(self, csr, all=False, raw=False, chain=False, **kw):
		)


		def _emails_are_valid(subject_emails, san_emails, principal_emails):

Fixing the cert-request command comparing whole email address case-sensitively. #736

Fixing the cert-request command comparing whole email address case-sensitively. #736

Conversation

felipevolpone commented Apr 26, 2017 • edited by MartinBasti

frasertweedale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipevolpone commented May 2, 2017

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

felipevolpone commented May 4, 2017

frasertweedale left a 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

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

frasertweedale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipevolpone commented May 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frasertweedale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinBasti commented May 16, 2017

felipevolpone commented Apr 26, 2017 •

edited by MartinBasti