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.

Sign up for GitHub

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

Jump to bottom

Allow using DOMAIN COMPONENT for AD DS certificates #75

Closed
wants to merge 13 commits into from
Closed

Allow using DOMAIN COMPONENT for AD DS certificates #75

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
6080ea5
Create pull.yml
alfonsrv Sep 21, 2021
7e955fa
Allow using DOMAIN COMPONENT for AD DS certificates
alfonsrv Sep 21, 2021
defd072
Improved sorting for more complicated patterns
alfonsrv Sep 21, 2021
b6a176c
USE_TZ compatibility
alfonsrv Sep 22, 2021
38c6e4f
Fix overly complex Django admin widget
alfonsrv Sep 22, 2021
be00b58
sign_cert management command different expiring logic
alfonsrv Sep 24, 2021
bad161d
Fixing otherName UTF8 encoding
alfonsrv Sep 25, 2021
082ccd3
Add documentation on custom extensions
alfonsrv Sep 25, 2021
83619b2
Adding tests x minor code-base alignments
alfonsrv Sep 26, 2021
d4fa7b6
Merge branch 'mathiasertl:master' into django-dev
alfonsrv Sep 26, 2021
0c9dec8
Migrations
alfonsrv Sep 27, 2021
a0b8bc4
Move DC above OU, docs header fix
alfonsrv Oct 3, 2021
446b050
Merge branch 'mathiasertl:master' into django-dev
alfonsrv Oct 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ca/django_ca/fields.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ class SubjectField(forms.MultiValueField):

def __init__(self, **kwargs: typing.Any) -> None:
fields = (
forms.CharField(required=False), # DC
forms.CharField(required=False), # C
forms.CharField(required=False), # ST
forms.CharField(required=False), # L
Expand Down
2 changes: 1 addition & 1 deletion ca/django_ca/forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ def __init__(self, *args: typing.Any, **kwargs: typing.Any) -> None:
help_text=_("Password for the private key. If not given, the private key must be unencrypted."),
)
expires = forms.DateField(initial=_initial_expires, widget=AdminDateWidget())
subject = SubjectField(label="Subject", required=True)
subject = SubjectField(label="Subject", required=False)
mathiasertl marked this conversation as resolved.
Show resolved Hide resolved
subject_alternative_name = SubjectAltNameField(
label="subjectAltName",
required=False,
Expand Down
22 changes: 16 additions & 6 deletions ca/django_ca/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import typing
from collections import abc
from datetime import datetime
from datetime import timezone
from datetime import timedelta
from ipaddress import ip_address
from ipaddress import ip_network
Expand All @@ -35,7 +36,7 @@

import idna

from asn1crypto.core import OctetString
from asn1crypto.core import OctetString, UTF8String
from cryptography import x509
from cryptography.hazmat.backends import default_backend
from cryptography.hazmat.primitives import hashes
Expand All @@ -49,6 +50,7 @@
from django.core.validators import URLValidator
from django.utils.encoding import force_bytes
from django.utils.encoding import force_str
from django.utils.timezone import get_current_timezone
from django.utils.translation import gettext_lazy as _

from . import ca_settings
Expand All @@ -65,6 +67,7 @@

# List of possible subject fields, in order
SUBJECT_FIELDS = [
"DC",
"C",
"ST",
"L",
Expand Down Expand Up @@ -104,6 +107,7 @@

#: Map OID objects to IDs used in subject strings
OID_NAME_MAPPINGS: Dict[x509.ObjectIdentifier, str] = {
NameOID.DOMAIN_COMPONENT: "DC",
NameOID.COUNTRY_NAME: "C",
NameOID.STATE_OR_PROVINCE_NAME: "ST",
NameOID.LOCALITY_NAME: "L",
Expand All @@ -124,6 +128,7 @@

# Some OIDs can occur multiple times
MULTIPLE_OIDS = (
NameOID.DOMAIN_COMPONENT,
NameOID.ORGANIZATIONAL_UNIT_NAME,
NameOID.STREET_ADDRESS,
)
Expand Down Expand Up @@ -209,8 +214,11 @@ def getter(self, method): # type: ignore


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]))
"""Returns the subject in the correct order for a x509 subject, while respecting
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]))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. 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.
  2. What does that if check do?
  3. Definitely add tests for an updated version of this function.

Copy link
Contributor Author

@alfonsrv alfonsrv Sep 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

@mathiasertl mathiasertl Sep 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



def encode_url(url: str) -> str:
Expand Down Expand Up @@ -837,7 +845,7 @@ def parse_general_name(name: ParsableGeneralName) -> x509.GeneralName:
if match is not None:
oid, asn_typ, val = match.groups()
if asn_typ == "UTF8":
parsed_value = val.encode("utf-8")
parsed_value = UTF8String(val)
mathiasertl marked this conversation as resolved.
Show resolved Hide resolved
elif asn_typ == "OctetString":
parsed_value = OctetString(bytes(bytearray.fromhex(val))).dump()
else:
Expand Down Expand Up @@ -959,8 +967,10 @@ def parse_encoding(value: Optional[Union[str, Encoding]] = None) -> Encoding:
def parse_expires(expires: Expires = None) -> datetime:
"""Parse a value specifying an expiry into a concrete datetime."""

now = datetime.utcnow().replace(second=0, microsecond=0)
now = datetime.now(timezone.utc).replace(second=0, microsecond=0)

if hasattr(expires, 'tzinfo') and not expires.tzinfo:
expires = expires.replace(tzinfo=get_current_timezone())
alfonsrv marked this conversation as resolved.
Show resolved Hide resolved
if isinstance(expires, int):
return now + timedelta(days=expires)
if isinstance(expires, timedelta):
Expand Down Expand Up @@ -1040,7 +1050,7 @@ def get_cert_builder(expires: datetime, serial: Optional[int] = None) -> x509.Ce
to generate such a value. By default, a value will be generated.
"""

now = datetime.utcnow().replace(second=0, microsecond=0)
now = datetime.now(timezone.utc).replace(second=0, microsecond=0)

# NOTE: Explicitly passing a serial is used when creating a CA, where we want to add extensions where the
# value references the serial.
Expand Down
8 changes: 7 additions & 1 deletion ca/django_ca/widgets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ class SubjectWidget(CustomMultiWidget):

def __init__(self, attrs: typing.Optional[typing.Dict[str, str]] = None) -> None:
_widgets = (
SubjectTextInput(label=_("Domain Component")),
mathiasertl marked this conversation as resolved.
Show resolved Hide resolved
SubjectTextInput(label=_("Country"), attrs={"placeholder": "2 character country code"}),
SubjectTextInput(label=_("State")),
SubjectTextInput(label=_("Location")),
Expand All @@ -118,14 +119,19 @@ def decompress(
) -> typing.List[typing.Union[str, typing.List[str]]]:
if value is None: # pragma: no cover
return ["", "", "", "", "", ""]


# Multiple OUs are not supported in webinterface
mathiasertl marked this conversation as resolved.
Show resolved Hide resolved
domain_component = value.get("DC", "")
if isinstance(domain_component, list) and domain_component:
domain_component = domain_component[0]
# Multiple OUs are not supported in webinterface
org_unit = value.get("OU", "")
if isinstance(org_unit, list) and org_unit:
org_unit = org_unit[0]

# Used e.g. for initial form data (e.g. resigning a cert)
return [
domain_component,
value.get("C", ""),
value.get("ST", ""),
value.get("L", ""),
Expand Down