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

fullchain_pem should be set to a string in ClientV2 and BackwardsCompatibleClientV2 #5606

Closed
ohemorange opened this issue Feb 22, 2018 · 2 comments · Fixed by #5654
Closed

Comments

@ohemorange
Copy link
Contributor

Part of #5367.
fullchain_pem in the order is bytes; set the fullchain_pem property of the order resource to a string

@bmw
Copy link
Member

bmw commented Feb 26, 2018

Worth thinking about what the correct type in Python 2.7 is. I think the answer is unicode, but not sure how annoying this is for us and others and how much this deviates from the acme modules current behavior (and if we should even care what the current behavior is).

EDIT: The answer is definitely unicode. For example, I recently (re)learned that calling decode on a str in Python 2.7 returns a unicode object. This is also the most common pattern in acme and other tools that handle Python 2 and 3 compatibility in a sane way.

EDIT EDIT: Another reason for this choice: six.text_type is unicode in Python 2 and str in Python 3.

@bmw
Copy link
Member

bmw commented Mar 1, 2018

My very minimal start to this issue is in bytes-fullchain-bites which should fix the problem on the acme side for ACMEv2. We still need to:

  1. Have the backwards compatible client set fullchain_pem to unicode/str for ACMEv1.
  2. Handle this change in Certbot (almost certainly by immediately calling encode on fullchain_pem for now).

Only other note is a function like acme.crypto_util.dump_pyopenssl_chain should almost certainly still return bytes. Why? If the function supports DER encoding (which that function does), it can't return unicode/str because that's a binary format that doesn't necessarily contain all Unicode characters. We could change the return type based on the encoding requested, but that seems like a bad idea to me.

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

Successfully merging a pull request may close this issue.

2 participants