Allow using DOMAIN COMPONENT for AD DS certificates #75

alfonsrv · 2021-09-21T20:30:20Z

Adding support to use DC directive in certificates.

An AD DS-bound certificate's subject used for Windows Smart Card authentication is composed of the absolute path of the user object in AD DS – this path is also reflected in the user's distinguishedName attribute and used for authentication, e.g. [('CN', 'John Doe'), ('OU', 'Users'), ('DC', 'contoso'), ('DC', 'loc')].

As per RFC2253, the formatting is reversed to x509 and the DC and OU directive can occur multiple times. Because the order is important when authenticating users, I extended the sort_name function a bit (with some online help of @kiwwisk)

ca/django_ca/utils.py

ca/django_ca/forms.py

ca/django_ca/utils.py

ca/django_ca/widgets.py

mathiasertl

Looks pretty good to me, only minor updates here

ca/django_ca/forms.py

ca/django_ca/widgets.py

ca/django_ca/utils.py

alfonsrv · 2021-09-26T11:56:29Z

Cheers – I pushed the changes for review.

mathiasertl · 2021-09-26T13:19:10Z

ca/django_ca/utils.py

+    the original list order for possible subject fields allowing for MULTIPLE_OIDS."""
+    if SUBJECT_FIELDS.index(subject[0][0]) > SUBJECT_FIELDS.index(subject[-1][0]):
+        return sorted(subject[::-1], key=lambda k: SUBJECT_FIELDS.index(k[0]))
+    return sorted(subject, key=lambda k: SUBJECT_FIELDS.index(k[0]))


This function here: I think I understand what your updated comment means to express, but the code is far from obvious. And I'm frankly surprised that the old code does not respect sort stability.

I think subject[::-1] is just reversing the list. At least a simple test did that. If yes, then use the more obvious reversed() instead.

What does that if check do?

Definitely add tests for an updated version of this function.

See comment below. I'm not sure how it does its magic exactly anymore, I just know it reliably yields the desired results.

Exemplary test cases as mentioned below:

>>> subject = [('CN', 'John Doe'), ('OU', 'Users'), ('DC', 'contoso'), ('DC', 'loc')] >>> sort_name(subject) [('DC', 'loc'), ('DC', 'contoso'), ('OU', 'Users'), ('CN', 'John Doe')] >>> subject = [('CN', 'John Doe'), ('OU', 'Marketing'), ('OU', 'Users'), ('OU', 'OtherUnit'), ('DC', 'contoso'), ('DC', 'loc')] >>> sort_name(subject) [('DC', 'loc'), ('DC', 'contoso'), ('OU', 'OtherUnit'), ('OU', 'Users'), ('OU', 'Marketing'), ('CN', 'John Doe')] >>> subject = [('DC', 'loc'), ('DC', 'contoso'), ('DC', 'ad'), ('OU', 'Users'), ('CN', 'John Doe')] >>> sort_name(subject) [('DC', 'loc'), ('DC', 'contoso'), ('DC', 'ad'), ('OU', 'Users'), ('CN', 'John Doe')]

Let me know if you feel comfortable with the sort function so we can move forward. Halting any further effort until then.

Now that's behavior that I don't understand: In testcase two, loc is the last DC in the input and ends up as first element.

In the third test case, loc is the first DC, and still ends up as last. Is that supposed to be?

Yes. In the first and second test case, the user input the subject with CN being the first element – this is a classic case of assuming the hierarchy goes from left-to-right with CN being the lowest weighted element.

In test case three, we're looking at the opposite. The user's hierarchical input is correct as per X509 with CN being the last element.

In all three test cases the user respect the general hierarchical-structure, albeit in a reversed manner.

ca/django_ca/widgets.py

mathiasertl · 2021-09-26T13:21:35Z

By the way, the test suite in the main branch is now (finally) fixed again. That means you're runs should pass as well before being merged.

Update: Tipp: Be sure to merge the latest master into your branch.

mathiasertl · 2021-09-26T14:17:09Z

As per RFC2253, the formatting is reversed to x509 and the DC and OU directive can occur multiple times. Because the order is important when authenticating users, I extended the sort_name function a bit

After thinking a little bit about this, I'm currently uncomfortable with that particular change. Can you please explain what exactly it is you're trying to achieve with this change? Do you just want to reverse the order when printing a subject with a DC component present? Or is this change also important when parsing the subject on the command line?

alfonsrv · 2021-09-26T15:27:29Z

Currently on mobile, but try sorting CN=John Doe,OU=DevOps,OU=IT,DC=domain,DC=tld with your previous sorting algorithm. You'll notice it doesn't reverse/sort the DC and OU elements properly.

Should be DC=tld,DC=domain,OU=IT,OU=DevOps,CN=John Doe, but actually results in DC=domain,DC=tld,OU=DevOps,OU=IT,CN=John Doe.

This also affects CLI; so I'd argue that the previous sorting algorithm wasn't designed to yield the desired results when e.g. multiple OUs are committed.

If you have a more elegant way to do it, I'm not emotionally attached to the new sorting algorithm. :)

docs/source/cli/certs.rst

mathiasertl · 2021-09-28T16:16:57Z

Huh? What happened with that migration? Where is that coming from? I don't see any models changed!

mathiasertl · 2021-09-28T16:29:34Z

What do you think of this change to sort_name instead?

@@ -210,7 +213,7 @@ except ImportError:  # pragma: no cover
 
 def sort_name(subject: List[Tuple[str, str]]) -> List[Tuple[str, str]]:
     """Returns the subject in the correct order for a x509 subject."""
-    return sorted(subject, key=lambda e: SUBJECT_FIELDS.index(e[0]))
+    return reversed(sorted(subject, key=lambda e: SUBJECT_FIELDS.index(e[0])))
 
 
 def encode_url(url: str) -> str:

It passes your first two test cases, but not the third one, where it's arguably more consistent.

alfonsrv · 2021-09-28T18:50:11Z

The issue is that you don't know how a user is going to input the subject into the form/certificate request/cli. It may either be the expected X509 format which has the CN last or the other format which has the CN first – for example Active Directory heavily suggest that CN always comes first.

In fact being a system administrator I have seen many inexperienced sysadmins make this "mistake" assuming CN is the highest weighted attribute and therefore comes first, adding all other attributes after.

A sorting algorithm should be able to expect both assumptions and abstract that knowledge and level of detail from the user, being able to order it the right way in any case, to produce a valid X509 certificate.

mathiasertl · 2021-09-28T20:43:19Z

A sorting algorithm should be able to expect both assumptions and abstract that knowledge and level of detail from the user, being able to order it the right way in any case, to produce a valid X509 certificate.

That seems like an impossible assumption to me.

I think we should do the following: the sort_name() issue does not technically break anything. Your PR still adds a feature and fixes the SAN parsing issue. Let's revert that particular change here in this PR and go through that in a separate issue discussion where we can analyze the issue in more detail. Other then this issue (update) and that out-of-nowhere migration, I see no reason not to merge.

alfonsrv · 2021-10-03T11:14:27Z

I had to create the migration after the latest merge, otherwise it refused to run. Could have something to do with Django 3.2.7 requiring explicit definition of the the PrimaryKey type. Can delete it or leave it here to provide compatibility. What do you prefer?

As for the others – I have pushed the minor adjustments I'm aware of that needed to be changed – not sure how we have remained on proceeding with the sorting algorithm. If there's something I am missing, let me know, so I can adjust it accordingly.

mathiasertl · 2021-10-03T11:34:13Z

I had to create the migration after the latest merge, otherwise it refused to run. Could have something to do with Django 3.2.7 requiring explicit definition of the the PrimaryKey type. Can delete it or leave it here to provide compatibility. What do you prefer?

I don't see any change in recent Django minor versions that would require that. It doesn't ask me to do that migration locally either. Please remove it - or show precisely when Django asks you to create it.

not sure how we have remained on proceeding with the sorting algorithm.

Please remove it from this PR and create a new issue describing your problem. TBH, I'm not really sure there even is an issue - sort order seems stable from changes I made in 24324de.

As for the others – I have pushed the minor adjustments I'm aware of that needed to be changed

The rest should be fine IMHO. But note that you'll have to get GitHub CI to pass. Right now master passes as well, so merging it to your branch should show any remaining issues iin your PR.

alfonsrv · 2021-10-03T11:48:05Z

https://docs.djangoproject.com/en/3.2/releases/3.2/#customizing-type-of-auto-created-primary-keys

mathiasertl · 2021-10-03T11:57:17Z

closed by accident? I can re-open of course?

alfonsrv · 2021-10-03T15:28:38Z

No, I decided contributing is too cumbersome.

mathiasertl · 2021-10-05T19:13:16Z

I'm very sorry to hear that. I hope you don't mind me picking up the fixes that you made.

alfonsrv and others added 4 commits September 21, 2021 11:36

Create pull.yml

6080ea5

Allow using DOMAIN COMPONENT for AD DS certificates

7e955fa

Improved sorting for more complicated patterns

defd072

USE_TZ compatibility

b6a176c

alfonsrv commented Sep 22, 2021

View reviewed changes

ca/django_ca/utils.py Outdated Show resolved Hide resolved

Fix overly complex Django admin widget

38c6e4f

alfonsrv commented Sep 22, 2021

View reviewed changes

ca/django_ca/forms.py Show resolved Hide resolved

alfonsrv added 2 commits September 24, 2021 12:15

sign_cert management command different expiring logic

be00b58

Fixing otherName UTF8 encoding

bad161d

alfonsrv commented Sep 25, 2021

View reviewed changes

ca/django_ca/utils.py Outdated Show resolved Hide resolved

alfonsrv commented Sep 25, 2021

View reviewed changes

ca/django_ca/widgets.py Outdated Show resolved Hide resolved

Add documentation on custom extensions

082ccd3

alfonsrv changed the title ~~Allow using DOMAIN COMPONENT OID for AD DS certificates~~ Allow using DOMAIN COMPONENT for AD DS certificates Sep 25, 2021

mathiasertl reviewed Sep 26, 2021

View reviewed changes

ca/django_ca/forms.py Show resolved Hide resolved

ca/django_ca/widgets.py Outdated Show resolved Hide resolved

ca/django_ca/widgets.py Outdated Show resolved Hide resolved

ca/django_ca/utils.py Outdated Show resolved Hide resolved

Adding tests x minor code-base alignments

83619b2

mathiasertl reviewed Sep 26, 2021

View reviewed changes

alfonsrv mentioned this pull request Sep 26, 2021

Improve Django Admin Subject Input Form #77

Closed

alfonsrv added 2 commits September 26, 2021 21:36

Merge branch 'mathiasertl:master' into django-dev

d4fa7b6

Migrations

0c9dec8

mathiasertl reviewed Sep 28, 2021

View reviewed changes

docs/source/cli/certs.rst Outdated Show resolved Hide resolved

mathiasertl added a commit that referenced this pull request Oct 3, 2021

test multiple OUs more closely (see #75)

24324de

alfonsrv added 2 commits October 3, 2021 13:15

Move DC above OU, docs header fix

a0b8bc4

Merge branch 'mathiasertl:master' into django-dev

446b050

alfonsrv closed this Oct 3, 2021

alfonsrv deleted the django-dev branch October 3, 2021 11:49

mathiasertl added a commit that referenced this pull request Oct 5, 2021

fix parsing of ASN.1 OtherNames from the command line (inspired by #75)

067dd73

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow using DOMAIN COMPONENT for AD DS certificates #75

Allow using DOMAIN COMPONENT for AD DS certificates #75

alfonsrv commented Sep 21, 2021 •

edited

mathiasertl left a comment

alfonsrv commented Sep 26, 2021

mathiasertl Sep 26, 2021

alfonsrv Sep 26, 2021 •

edited

mathiasertl Sep 28, 2021 •

edited

alfonsrv Oct 3, 2021

mathiasertl commented Sep 26, 2021 •

edited

mathiasertl commented Sep 26, 2021

alfonsrv commented Sep 26, 2021 •

edited

mathiasertl commented Sep 28, 2021

mathiasertl commented Sep 28, 2021

alfonsrv commented Sep 28, 2021 •

edited

mathiasertl commented Sep 28, 2021 •

edited

alfonsrv commented Oct 3, 2021

mathiasertl commented Oct 3, 2021

alfonsrv commented Oct 3, 2021

mathiasertl commented Oct 3, 2021

alfonsrv commented Oct 3, 2021

mathiasertl commented Oct 5, 2021

Allow using DOMAIN COMPONENT for AD DS certificates #75

Allow using DOMAIN COMPONENT for AD DS certificates #75

Conversation

alfonsrv commented Sep 21, 2021 • edited

mathiasertl left a comment

Choose a reason for hiding this comment

alfonsrv commented Sep 26, 2021

mathiasertl Sep 26, 2021

Choose a reason for hiding this comment

alfonsrv Sep 26, 2021 • edited

Choose a reason for hiding this comment

mathiasertl Sep 28, 2021 • edited

Choose a reason for hiding this comment

alfonsrv Oct 3, 2021

Choose a reason for hiding this comment

mathiasertl commented Sep 26, 2021 • edited

mathiasertl commented Sep 26, 2021

alfonsrv commented Sep 26, 2021 • edited

mathiasertl commented Sep 28, 2021

mathiasertl commented Sep 28, 2021

alfonsrv commented Sep 28, 2021 • edited

mathiasertl commented Sep 28, 2021 • edited

alfonsrv commented Oct 3, 2021

mathiasertl commented Oct 3, 2021

alfonsrv commented Oct 3, 2021

mathiasertl commented Oct 3, 2021

alfonsrv commented Oct 3, 2021

mathiasertl commented Oct 5, 2021

alfonsrv commented Sep 21, 2021 •

edited

alfonsrv Sep 26, 2021 •

edited

mathiasertl Sep 28, 2021 •

edited

mathiasertl commented Sep 26, 2021 •

edited

alfonsrv commented Sep 26, 2021 •

edited

alfonsrv commented Sep 28, 2021 •

edited

mathiasertl commented Sep 28, 2021 •

edited