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

salt.utils.hashutils: Only decode to utf-8 on Windows #48041

Merged
merged 1 commit into from Jun 11, 2018

Conversation

Projects
None yet
4 participants
@terminalmage
Member

terminalmage commented Jun 8, 2018

We only decode to unicode so that we can use some of these functions in Jinja filters. Since CP1252 will incorrectly decode certain bytestrings, force utf8 decoding on Windows.

salt.utils.hashutils: Only decode to utf-8 on Windows
We only decode to unicode so that we can use some of these functions in
Jinja filters. Since CP1252 will incorrectly decode certain bytestrings,
force utf8 decoding on Windows.
@damon-atkins

This comment has been minimized.

Member

damon-atkins commented Jun 9, 2018

Keep in mine https://en.wikipedia.org/wiki/Base64 restrictions. i.e. Character Set Allowed, hence its not just Windows.

@terminalmage

This comment has been minimized.

Member

terminalmage commented Jun 10, 2018

@damon-atkins Can you be a little more clear? I don't know what you're trying to say.

@damon-atkins

This comment has been minimized.

Member

damon-atkins commented Jun 11, 2018

I suspect the raw string always needs to be US ASCII because a hash/checksum or base64 can only contain US ASCII characters as far as I have notice. Hence for windows where the language is not US ASCII and not UTF8 issues occurred. But it could also be the same for other platforms. Should this fix just be for windows or does it make more sense for it to be for all platforms?

From https://tools.ietf.org/html/rfc3548.html

   Base encoding of data is used in many situations to store or transfer
   data in environments that, perhaps for legacy reasons, are restricted
   to only US-ASCII [9] data.
@terminalmage

This comment has been minimized.

Member

terminalmage commented Jun 11, 2018

I don't think this is needed on non-Windows platforms. The reason for doing this is because CP1252 is awful and will successfully decode bytestrings that it shouldn't, with corrupted results. UTF-8, latin-1, ascii, all don't have this problem. Forcing use of UTF-8 when decoding the bytestring on Windows keeps us from trying CP1252.

@rallytime rallytime merged commit 6c53891 into saltstack:2018.3 Jun 11, 2018

6 of 9 checks passed

jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #5601 — ABORTED
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #19655 — ABORTED
Details
default Build finished.
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #25799 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #17864 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #23531 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #10572 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #22495 — SUCCESS
Details

@terminalmage terminalmage deleted the terminalmage:hashutils-fix-windows branch Jun 12, 2018

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