Add support for rfc822Name SAN in SMIME certificates #177

mkosmach · 2016-01-28T15:35:52Z

replace #176 request

mkosmach · 2017-05-02T07:51:14Z

Can anybody review my patch?
I think that patch is needed in 4.4-(stable,trunk) branches too.

gvde · 2017-07-28T13:35:40Z

SubjectAltName is undefined if there is no subjectaltname attribute in the certificate. In that case there is an error

Can't use an undefined value as an ARRAY reference at /usr/local/rt4/sbin/../lib/RT/Crypt/SMIME.pm line 902, <$fh> line 1.

I am not sure if Subject can ever be empty, but I guess it wouldn't hurt to check that is defined, too before using it and initializing data{String} to some placeholder string if the subject is empty. Then you should have covered all bases...

I also think it would be better to prefer the email address from the dn if it is set. subjectAltName means "subject alternative name", thus it's only an alternative if the dn has also an email address...

mkosmach · 2017-07-28T14:27:06Z

AFAIR rfc822Name should be preferred way to store email.
And yes, I should add some checks to patch.

gvde · 2017-07-28T15:17:24Z

You are correct. RFC 5280 says in section 4.1.2.6:

Conforming implementations generating new certificates with
electronic mail addresses MUST use the rfc822Name in the subject
alternative name extension (Section 4.2.1.6) to describe such
identities. Simultaneous inclusion of the emailAddress attribute in
the subject distinguished name to support legacy implementations is
deprecated but permitted.

subjectAltName is the preferred way to set the email address.

mkosmach · 2017-07-28T17:13:26Z

Added fixes for certificates w/o rfc822name

gvde · 2017-07-29T13:02:28Z

I have also noticed that the smime verification does not verify the sender address against the address in the certificate. That seems strange. If the email is signed by a certificate of a trusted ca, it's shown as good signature with full trust, even though it may actually a certificate by someone else not matching the sender address...

mkosmach · 2017-07-30T09:08:58Z

https://tools.ietf.org/html/rfc5750#section-3
Receiving agents MUST check that the address in the
From or Sender header of a mail message matches an Internet mail
address, if present, in the signer's certificate, if mail addresses
are present in the certificate. A receiving agent SHOULD provide
some explicit alternate processing of the message if this comparison
fails, which may be to display a message that shows the recipient the
addresses in the certificate or other certificate details.

So yes, RT must check sender address and should provide alternative variant (certificate in user certificate store?) if check is failed.
So please add issue for this problem at https://issues.bestpractical.com/

gvde · 2017-07-31T05:46:56Z

lib/RT/Crypt/SMIME.pm

+            $data{String} = 'DN: ' . join(',',@{$cert->Issuer});
+        } elsif ($type eq 'subject') {
+            if ($cert->SubjectAltName && grep /rfc822name/i, @{$cert->SubjectAltName}) {
+                $data{String} = join( ',', map { s/rfc822name=//i; $_ } grep /rfc822name/i, @{$cert->SubjectAltName});


I would suggest
$data{String} = Email::Address->new( $data{'Name'}, join( ',', map { s/rfc822name=//i; $_ } grep /rfc822name/i, @{$cert->SubjectAltName}))->format;
to format the email address with the name if available.

I am running my RT server with my suggestion and it looks good. Do you think you can update your pull request?

What's the chance to get this into the next release?

Of course I can add you patch to my PR.
But nobody from BTS devs did not reply on this PR for more than a year so I think that BTS doesn't have any plans to fix SMIME support.

PS. If You have any time - please add some tests for new features.
I haven't time for this work right now :(

While I no longer work at BPS, I'm the original author of the SMIME support, and can take a gander and potentially merge it.

Adding tests would make it much easier to review. I'll see if I understand the change well enough to add some.

Hi, @alexmv. Glad to see You in this issue.

What Do You think about moving SMIME from openssl smime to openssl cms?
Or using some of existing perl smime packages (Crypt::SMIME / Crypt::OpenSSL::SMIME / Crypt::SMIMEEngine/etc).

Now RT work with smime keys/messages on their own. And I think that native smime in openssl (now openssl cms ) can do this better. I think that openssl cms already can verify sender email, key status (crl/ocsp), CA chains, etc.
So maybe we should switch to use native openssl functions instead of partially reimplement it in RT code?

Please do take this suggestion and use Email::Address to construct the address.

That said, I don't think joining all of the rfc822name= SubjectAltName addresses we find with , is correct. It constructs a String which looks like "Someone" <alice@example.com, bob@example.com> which we attempt to parse out later using Email::Address. Unfortunately, such a string doesn't round-trip, and Email::Address just finds alice@example.com in it, with no name.

The Most Right thing here would be to make it a list of Email::Address objects, and later iterate over them looking for any one match, using Load -- and finally resort to calling LoadOrCreate using just one of them. I think the simple solution is just to take the first rfc822 name and be done with it, and I'd merge a change which simply did that.

I don't know for sure how SubjectAltNames are made, but I suspect that the grep regex wants to be anchored. I would like it to also contain the = that we later assume is there to strip off, and general code style is to use the block form of grep, not the statement form.

sartak · 2017-09-05T22:41:03Z

I'm on board, in principal, with switching to openssl cms or openssl smime

…

@mkosmach commented on this pull request. In lib/RT/Crypt/SMIME.pm: > @@ -883,8 +884,18 @@ sub GetCertificateInfo { my $method = $type . "_" . $USER_MAP{$_}; $data{$_} = $cert->$method if $cert->can($method); } - $data{String} = Email::Address->new( @DaTa{'Name', 'EmailAddress'} )->format - if $data{EmailAddress}; + if ($type eq 'issuer') { + $data{String} = 'DN: ' . join(',',@{$cert->Issuer}); + } elsif ($type eq 'subject') { + if ($cert->SubjectAltName && grep /rfc822name/i, @{$cert->SubjectAltName}) { + $data{String} = join( ',', map { s/rfc822name=//i; $_ } grep /rfc822name/i, @{$cert->SubjectAltName}); Hi, @alexmv. Glad to see You in this issue. What Do You think about moving SMIME from openssl smime to openssl cms? Or using some of existing perl smime packages (Crypt::SMIME / Crypt::OpenSSL::SMIME / Crypt::SMIMEEngine/etc). Now RT work with smime keys/messages on their own. And I think that native smime in openssl (now openssl cms ) can do this better. I think that openssl cms already can verify sender email, key status (crl/ocsp), CA chains, etc. So maybe we should switch to use native openssl functions instead of partially reimplement it in RT code? ― You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

gvde · 2017-09-06T05:11:14Z

Would it still be possible to pull this? Currently, there are errors in the logs and the text shown is "broken" ('SMIME: The signature is good, signed by , trust is full') if the certificate has the only e-mail address set as subjectaltname.

See https://forum.bestpractical.com/t/smime-support-does-not-support-email-address-in-x509v3-extension/32145

Reimplementing the code may be a better long-term solution but for now (as this pull request is dated back to January 2016...) a quick fix would be much appreciated...

alexmv

I concur that discussing or planning a shift to openssl cms is out of scope -- I have Feelings about it (openssl smime is horrible and we actively decided against it) but this is not the place to discuss them.

I'm going to see if I can suss out how to use rt/t/data/smime/keys/demoCA/ to make a key. Without being able to test it, I have one bug that's visible by inspection, commented below.

Please squash your changes into one commit, as they're one logical change.

alexmv · 2017-09-06T07:12:34Z

lib/RT/Crypt/SMIME.pm

@@ -460,6 +460,7 @@ sub Verify {

        $signer = $info{info}[0];
        last unless $signer and $signer->{User}[0]{String};
+        last if $signer->{User}[0]{String} =~ 'DN:';


DN: could possibly be part of an RFC822-compliant email address: "Alex DN: Thing" <alex@example.com>

This needs to be =~ /^DN: / in order to correctly not have false-positives

alexmv · 2017-09-06T07:47:51Z

lib/RT/Crypt/SMIME.pm

+            $data{String} = 'DN: ' . join(',',@{$cert->Issuer});
+        } elsif ($type eq 'subject') {
+            if ($cert->SubjectAltName && grep /rfc822name/i, @{$cert->SubjectAltName}) {
+                $data{String} = join( ',', map { s/rfc822name=//i; $_ } grep /rfc822name/i, @{$cert->SubjectAltName});


Please do take this suggestion and use Email::Address to construct the address.

That said, I don't think joining all of the rfc822name= SubjectAltName addresses we find with , is correct. It constructs a String which looks like "Someone" <alice@example.com, bob@example.com> which we attempt to parse out later using Email::Address. Unfortunately, such a string doesn't round-trip, and Email::Address just finds alice@example.com in it, with no name.

The Most Right thing here would be to make it a list of Email::Address objects, and later iterate over them looking for any one match, using Load -- and finally resort to calling LoadOrCreate using just one of them. I think the simple solution is just to take the first rfc822 name and be done with it, and I'd merge a change which simply did that.

I don't know for sure how SubjectAltNames are made, but I suspect that the grep regex wants to be anchored. I would like it to also contain the = that we later assume is there to strip off, and general code style is to use the block form of grep, not the statement form.

alexmv · 2017-09-06T09:06:51Z

I've succeeded in bending openssl ca to my whims; I'll push a few commits to document and make use of it tomorrow.

mkosmach · 2017-09-06T11:12:14Z

Thank You very much, Alex!
I'm sorry but I have no time now to integrate all suggestions in my PR or to make new PR with openssl cms inside. So I'll be glad if You make correct fix and push it to upstream.

PS. Please add checks for sender email == signer email as Gerald suggested earlier.

alexmv · 2017-09-07T10:04:51Z

Pushed 4.2/smime-subjectaltname with tests. Will follow up with the commits to resolve this issue atop those.

mkosmach · 2017-09-07T10:15:10Z

@alexmv, thank You
is this branch only for 4.2? It's needed for 4.4 too.

alexmv · 2017-09-07T11:02:50Z

Bugfixes are branched from the earliest supported release that the bug was found in. After it's merged into 4.2-trunk, 4.2-trunk will be merged up to 4.4-trunk, thus picking up the fix in 4.4. And 4.4-trunk is merged up into master.

gvde · 2019-03-18T07:04:31Z

Now we have 4.4.4 and 2019 and this update still hasn't been merged. I can't help it but it seems to me RT is a dying product... For several years now I have to repatch our installation after each update.

Add support for rfc822Name SAN in SMIME certificates

f3cbff5

Fix case with certificate without rfc822name

721cdf1

gvde reviewed Jul 31, 2017

View reviewed changes

alexmv suggested changes Sep 6, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for rfc822Name SAN in SMIME certificates #177

Add support for rfc822Name SAN in SMIME certificates #177

mkosmach commented Jan 28, 2016

mkosmach commented May 2, 2017

gvde commented Jul 28, 2017 •

edited

Loading

mkosmach commented Jul 28, 2017

gvde commented Jul 28, 2017

mkosmach commented Jul 28, 2017

gvde commented Jul 29, 2017

mkosmach commented Jul 30, 2017

gvde Jul 31, 2017

gvde Sep 1, 2017

mkosmach Sep 1, 2017

alexmv Sep 4, 2017

mkosmach Sep 5, 2017

alexmv Sep 6, 2017

sartak commented Sep 5, 2017 via email

gvde commented Sep 6, 2017

alexmv left a comment

alexmv Sep 6, 2017

alexmv Sep 6, 2017

alexmv commented Sep 6, 2017

mkosmach commented Sep 6, 2017

alexmv commented Sep 7, 2017

mkosmach commented Sep 7, 2017

alexmv commented Sep 7, 2017

gvde commented Mar 18, 2019

Add support for rfc822Name SAN in SMIME certificates #177

Are you sure you want to change the base?

Add support for rfc822Name SAN in SMIME certificates #177

Conversation

mkosmach commented Jan 28, 2016

mkosmach commented May 2, 2017

gvde commented Jul 28, 2017 • edited Loading

mkosmach commented Jul 28, 2017

gvde commented Jul 28, 2017

mkosmach commented Jul 28, 2017

gvde commented Jul 29, 2017

mkosmach commented Jul 30, 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

sartak commented Sep 5, 2017 via email

gvde commented Sep 6, 2017

alexmv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexmv commented Sep 6, 2017

mkosmach commented Sep 6, 2017

alexmv commented Sep 7, 2017

mkosmach commented Sep 7, 2017

alexmv commented Sep 7, 2017

gvde commented Mar 18, 2019

gvde commented Jul 28, 2017 •

edited

Loading