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

ssl_cert_username_cea: Use CertificateExactAssertion as username #202

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

minfrin
Copy link

@minfrin minfrin commented Jul 26, 2023

Add the ssl_cert_username_cea parameter, which, when present, uses the CertificateExactAssertion as defined in RFC4523 appendix A.1 as the username.

Add the ssl_cert_username_cea parameter, which, when present, uses
the CertificateExactAssertion as defined in RFC4523 appendix A.1
as the username.
# Use the certificate's CertificateExactAssertion for username, as defined
# in RFC4523 appendix A.1. You'll also need to set
# auth_ssl_username_from_cert=yes.
#ssl_cert_username_cea = no
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably could be configured via ssl_cert_username_field = CertificateExactAssertion? Otherwise there's a conflict with these two settings.

Although I don't know, how common is this use case anyway? Maybe this would be better as a more generic setting using variables, like:

ssl_cert_username = { serialNumber %{cert:serial_number} , issuer rdnSequence:"%{cert:issuer_name}" }

Copy link
Author

Choose a reason for hiding this comment

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

Probably could be configured via ssl_cert_username_field = CertificateExactAssertion? Otherwise there's a conflict with these two settings.

I considered it originally, however ssl_cert_username_field refers to rdns inside the subject of the certificate, while CertificateExactAssertion refers to a property of the whole certificate. It felt cleaner to have a separate directive.

ssl_cert_username_cea wins.

Maybe this would be better as a more generic setting using variables

The problem with the variable approach is that the RFC definition of CertificateExactAssertion is precise on the format of both the serialNumber and the issuer.

The XN_FLAG_RFC2253 flag gives the correct format, while X509_NAME_oneline uses a legacy format.

I ran into the same problem when I did Apache httpd's support, the definitions of the existing variables weren't tight enough.


BIO_puts(bio, ", issuer rdnSequence:\"");

issuer = X509_get_issuer_name(x509);
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it should be escaping \ and " in case they happen to exist in the issuer name. str_escape() would do that. And seems like this would be a bit simpler if BIO was used only for the X509_NAME_print_ex() and the rest used either t_strdup_printf() or string_t.

Copy link
Author

Choose a reason for hiding this comment

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

This seems like it should be escaping \ and " in case they happen to exist in the issuer name. str_escape() would do that

Let me check this.

And seems like this would be a bit simpler if BIO was used only for the X509_NAME_print_ex() and the rest used either t_strdup_printf() or string_t.

Felt a bit weird to mix them up, but can do.

Copy link
Author

Choose a reason for hiding this comment

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

This seems like it should be escaping \ and " in case they happen to exist in the issuer name.

Checked - escaping is unnecessary (and would break the string), as the rfc2253 already escapes the issuer name as described below:

https://datatracker.ietf.org/doc/html/rfc2253#section-2.4

Copy link
Author

Choose a reason for hiding this comment

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

And seems like this would be a bit simpler if BIO was used only for the X509_NAME_print_ex() and the rest used either t_strdup_printf() or string_t.

Looking at the code, using a BIO is the simplest way.

If I had to try and convert it to t_strdup_printf(), we would still have the same extraction of the fields, then we would need to t_malloc0() the BIO, then assemble the string, then get rid of the temporary t_malloc0.

Letting openssl do all the work is way cleaner.

Copy link
Contributor

@sirainen sirainen Aug 2, 2023

Choose a reason for hiding this comment

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

Checked - escaping is unnecessary (and would break the string), as the rfc2253 already escapes the issuer name as described below:

I think the certificates could still be evil and have " character without escaping it? So some sanity check for the string would still be useful.

Copy link
Author

Choose a reason for hiding this comment

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

I think the certificates could still be evil and have " character without escaping it? So some sanity check for the string would still be useful.

That's not possible in this case.

RFC4523 appendix A.1 describes a string that consists of the serial number (in a safe format), concatenated with the issuer DN encoded as per rfc2253 (in a safe format).

If the issuer wasn't safely escaped already, someone could create a malicious certificate that contained the target desired issuer with a 0x00 character in it, and then fake the identity of another certificate. This is prevented because rfc2253 mandates that the distinguished name be escaped.

Quoting from rfc2253 to show we are escaped already:

If the UTF-8 string does not have any of the following characters
which need escaping, then that string can be used as the string
representation of the value.

o   a space or "#" character occurring at the beginning of the
    string

o   a space character occurring at the end of the string

o   one of the characters ",", "+", """, "\", "<", ">" or ";"

Implementations MAY escape other characters.

If a character to be escaped is one of the list shown above, then it
is prefixed by a backslash ('' ASCII 92).

Otherwise the character to be escaped is replaced by a backslash and
two hex digits, which form a single byte in the code of the
character.

Any attempt to modify the CEA string outside that described by the RFC above means the string will not match with or interoperate with other software.

@minfrin
Copy link
Author

minfrin commented Aug 20, 2023

Quick ping on this one.

Keen to also contribute a ssl_cert_username_fingerprint=sha256 style option to match the functionality of postfix below, should I do this in the same PR or a new one?

https://www.postfix.org/postconf.5.html#permit_tls_clientcerts

@cmouse
Copy link
Contributor

cmouse commented Sep 19, 2023

We'll try to come back to this soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants