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

Set the string-mask to utf8only #115

Closed
wants to merge 6 commits into from

Conversation

Spindel
Copy link
Contributor

@Spindel Spindel commented May 31, 2014

If subject had utf-8 characters in them, the encoding chosen by OpenSSL for
defaults T61.

Requires changes in cryptography, commit id f8561cdf06f163806a57b0dd36290192414e3496

From the OpenSSL source code:
* utf8only : only use UTF8Strings (RFC2459 recommendation for 2004).

That was 10 years ago, and the last remnant that had problems with it
was Netscape, which is no longer a problem.

A request changes from:
13:d=5 hl=2 l= 3 prim: OBJECT :commonName
18:d=5 hl=2 l= 9 prim: T61STRING :Gurka ���

To:
13:d=5 hl=2 l= 3 prim: OBJECT :commonName
18:d=5 hl=2 l= 12 prim: UTF8STRING :Gurka åäö

OpenSSL/test/test_crypto.py
Update test DER data to have utf8string.
( \x0c instead of \0x13, PrintableString )

If subject had utf-8 characters in them, the encoding chosen by OpenSSL for
defaults T61.

From the OpenSSL source code:
	 * utf8only : only use UTF8Strings (RFC2459 recommendation for 2004).

That was 10 years ago, and the last remnant that had problems with it
was Netscape, which is no longer a problem.

A request changes from:
   13:d=5  hl=2 l=   3 prim: OBJECT            :commonName
   18:d=5  hl=2 l=   9 prim: T61STRING         :Gurka ���

To:
   13:d=5  hl=2 l=   3 prim: OBJECT            :commonName
   18:d=5  hl=2 l=  12 prim: UTF8STRING        :Gurka åäö

OpenSSL/test/test_crypto.py
	Update test DER data to have utf8string.
	( \x0c instead of \0x13, PrintableString )
@reaperhulk
Copy link
Member

As you noted in pyca/cryptography#1088, the call to ASN1_STRING_set_default_mask_asc is global so it only needs to happen once.

I am a bit concerned about this patch because it makes the assumption that UTF8String is equally compatible with PrintableString, et al. Looking more closely at X509_NAME_add_entry_by_NID it claims that passing MBSTRING_UTF8 should automatically do the right thing (although clearly the invocation here is not doing that). From the man page:

The use of string types such as MBSTRING_ASC or MBSTRING_UTF8 is strongly recommened for the type parameter. This allows the internal code to correctly determine the type of the field and to apply length checks according to the relevant standards. This is done using ASN1_STRING_set_by_NID().

I'd suggest that we err on the side of caution and have anything that's representable by PrintableString remain PrintableString and only use UTF8String when the character set requires it. Hypothetically it seems like ASN1_mbstring_ncopy (which is declared in a_mbstr.c and is called when X509_NAME_add_entry_by_NID is called) should be doing this for us, but maybe that's not what it's meant to do?

@Spindel
Copy link
Contributor Author

Spindel commented May 31, 2014

I agree on the global one, but I'm not sure where to put global library instantiations.

Regarding PrintableString, I'm not sure it's a good default really, quoting RFC5280:

CAs conforming to this profile MUST use either the PrintableString or
UTF8String encoding of DirectoryString, with two exceptions...

The exceptions mentioned are relevant, and declare that if issuer is in T61 or BMPString, Subject must be in the same encoding. Further the RFC states:

TeletexString, BMPString, and UniversalString are included
for backward compatibility, and SHOULD NOT be used for
certificates for new subjects.

Also, further down:

Conforming implementations MUST support UTF8String and PrintableString.
(which isn't the case today ;)

Also! The current state in OpenSSL upstream is to have openssl.cnf declare "string_mask = utf8only" by default. This has also been the default since 2005 ( upstream commit id 29b9763d)

So, this is why I'm suggesting the current change to PyOpenSSL. It would bring the defaults in line with how upstream has done it for the last 9 years.

@Spindel
Copy link
Contributor Author

Spindel commented May 31, 2014

Looking at upstreams:

  • CentOS 5.x has default to "MASK:0x2002" which means utf8string + PrintableString. For netscape compability.
  • CentOS 6.x has default mask to utf8only

@reaperhulk
Copy link
Member

I suspect @exarkun has an opinion on where global settings should live :)

Thanks for the info about when OpenSSL changed this default in their conf (openssl/openssl@29b9763d is the link to the commit). Looks like it's a safe default then, especially given the RHEL/CentOS information.

I suppose if we want to be very conservative we could do 0x2002 as well. Just as a note, most (all?) major CAs still use PrintableString where they can.

@Spindel
Copy link
Contributor Author

Spindel commented May 31, 2014

What's causing these changes right now is that we're working with a CA that's rejecting anything pyOpenSSL creates due to this.

@Spindel
Copy link
Contributor Author

Spindel commented May 31, 2014

Looking further, it seems as if there's some initialization done in crypto.py at the end ( OpenSSL_add_all_algorithms() and SSL_load_error_strings() ) Should this go at the same place?

@reaperhulk
Copy link
Member

Yes, that's a good place for it to be. Would 0x2002 work for your purposes? That should prevent it from using T61 or BMP right?

@Spindel
Copy link
Contributor Author

Spindel commented May 31, 2014

0x2002 ought to work as a default, though we'd want an override. It's not a good form to mix printable string types, especially not when subject / issuer / subjectKeyIdentifier contain different string types.

@reaperhulk
Copy link
Member

Nothing in x509 is good form. 😢

@Spindel
Copy link
Contributor Author

Spindel commented May 31, 2014

ASN.1, let me count the ways I hate you. I'm hoping that we can migrate to SSH tunnels and OpenSSH certificate formats. At least it's sane.

@Spindel
Copy link
Contributor Author

Spindel commented May 31, 2014

Updated commit. moves the initialization to the end of crypto.py, but keeps utf8only.

Sidenote, how should the dependency on cryptography look? >= 0.5? Or should I simply test for existance of the function first?

@reaperhulk
Copy link
Member

That's ultimately going to be @exarkun's call. My own opinion would be make the dependency >=0.5 so there's no ambiguity over what PyOpenSSL is doing with ASN.1 string types based on what version of cryptography the user may have installed.

@Spindel
Copy link
Contributor Author

Spindel commented May 31, 2014

I'd be inclined to agree, as it also changes the DER encoded strings in the testcases from T.61 to utf8. ( Those were already ASCII, so it's weird that they aren't PrintableStrings to begin with )

@Spindel
Copy link
Contributor Author

Spindel commented May 31, 2014

Well, until the cryptography things are either released, or I push an alternative URI in the setup.py, I guess the travis changes will fail. You're the maintainers, so I'll defer to your good judgement!

Signing off for today!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) when pulling 44875ae on MyTemp:no-more-T61Strings into 06ddbf3 on pyca:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.11%) when pulling 225e21f on MyTemp:no-more-T61Strings into 06ddbf3 on pyca:master.

@Spindel
Copy link
Contributor Author

Spindel commented Jun 1, 2014

Just a head's up about defaults, LibreSSL has changed the default
string_mask to be B_ASN1_UTF8STRING.

//D.S.

On Sat, May 31, 2014 at 6:47 PM, Paul Kehrer [email protected]
wrote:

That's ultimately going to be @exarkun https://github.com/exarkun's
call. My own opinion would be make the dependency >=0.5 so there's no
ambiguity over what PyOpenSSL is doing with ASN.1 string types based on
what version of cryptography the user may have installed.


Reply to this email directly or view it on GitHub
#115 (comment).

@Spindel
Copy link
Contributor Author

Spindel commented Jun 1, 2014

And another fix on this, OpenSSL upstream has now changed the default of
the library to be utf8only as well.

//D.S.

On Sat, May 31, 2014 at 6:47 PM, Paul Kehrer [email protected]
wrote:

That's ultimately going to be @exarkun https://github.com/exarkun's
call. My own opinion would be make the dependency >=0.5 so there's no
ambiguity over what PyOpenSSL is doing with ASN.1 string types based on
what version of cryptography the user may have installed.


Reply to this email directly or view it on GitHub
#115 (comment).

@marienz
Copy link

marienz commented Jun 22, 2014

Note OpenSSL changing the default (openssl/openssl@3009244 present in openssl 1.0.1h) causes test_der to fail. I filed duplicate issue #129 about that. Applying the changes suggested on this issue would fix that, and make pyopenssl behave consistently again independently of the openssl version it is using.

@reaperhulk
Copy link
Member

This has been rebased and is in #234.

@reaperhulk reaperhulk closed this Apr 15, 2015
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants