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

crypto/x509/pkix: Handle emailAddress OID #78

Merged
merged 1 commit into from
May 7, 2021

Conversation

cjpatton
Copy link
Contributor

Adds support for the emailAddress OID.

@cjpatton cjpatton requested a review from wbl as a code owner April 27, 2021 02:37
src/crypto/x509/pkix/pkix.go Outdated Show resolved Hide resolved
@cjpatton cjpatton changed the title crypto/x509: Handle emailAddress OID crypto/x509/pkix: Handle emailAddress OID Apr 27, 2021
@Lekensteyn
Copy link
Contributor

Lekensteyn commented Apr 27, 2021

This limitation exists in upstream Go too. Earlier reports:

Could you come up with any tests for this if it is important? If possible, you could also submit a proposal upstream.

Edit: RFC 5280 (https://tools.ietf.org/html/rfc5280#page-116) defines:

-- Legacy attributes
pkcs-9 OBJECT IDENTIFIER ::=
       { iso(1) member-body(2) us(840) rsadsi(113549) pkcs(1) 9 }
id-emailAddress      AttributeType ::= { pkcs-9 1 }
EmailAddress ::=     IA5String (SIZE (1..ub-emailaddress-length))

and also in https://tools.ietf.org/html/rfc5280#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.

I think more work is needed if this field is intended to be fully supported for validation purposes. Is it possible to modify the user instead of stdlib?

@cjpatton
Copy link
Contributor Author

Thanks, @Lekensteyn, for providing some context for this change.

Could you come up with any tests for this if it is important? If possible, you could also submit a proposal upstream.

Given (1) this feature is deprecated by RFC5280 and (2) it has been proposed and rejected before, I don't think we should try to upstream this change.

I think more work is needed if this field is intended to be fully supported for validation purposes.

Can you be more specific about what changes are needed? Is it just that you want to see some tests, or are you concerned about the side-effects for users of crypto/x509/pkix?

Is it possible to modify the user instead of stdlib?

This is certainly possible, but it would take some doing. To be clear, the goal is to replicate a feature of boringSSL that we need for our server: https://boringssl.googlesource.com/boringssl/+/refs/heads/master/include/openssl/nid.h#303

@Lekensteyn
Copy link
Contributor

Can you be more specific about what changes are needed? Is it just that you want to see some tests, or are you concerned about the side-effects for users of crypto/x509/pkix?

Both, I was originally concerned about side-effects, but on a quick look there shouldn't be any except those that are strict about the String() output. Some tests would be nice to demonstrate the exact issue that you are trying to solve with this patch.

Copy link
Contributor

@Lekensteyn Lekensteyn left a comment

Choose a reason for hiding this comment

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

One comment about the chosen value in the tests, LGTM otherwise.

src/crypto/x509/pkix/pkix_test.go Outdated Show resolved Hide resolved
@cjpatton cjpatton force-pushed the cjpatton/x509_email_address branch from c4c46f9 to c0c26f5 Compare May 7, 2021 03:05
@cjpatton cjpatton merged commit d37d1ee into cf May 7, 2021
@bwesterb bwesterb deleted the cjpatton/x509_email_address branch August 4, 2022 10:55
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