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

String/Bytes fix #270

Closed
wants to merge 1 commit into from
Closed

String/Bytes fix #270

wants to merge 1 commit into from

Conversation

RedProkofiev
Copy link
Contributor

Resolves #146
Resolves #209

The PR adds a new test which sends bytestrings through on_message, and through the test isolated two different issues with bytestring handling:

 if 'application/pkcs7-mime' in text or 'application/x-pkcs7-mime' in text:
            try:
                text = crypto.decrypt(text, self._cert, self._key)
            except crypto.CryptoException as e:
                error = 'Failed to decrypt message: %s' % e
                log.error(error)
                return None, None, error

In _handle_msg, the string comparison with 'in' would trigger a byte-not-str error,

 log.warning("Message rejected: %s", err_msg)
 name = self._rejectq.add({'body': body,
                                          'signer': signer,
                                          'empaid': empaid,
                                          'error': err_msg})
 log.info("Message saved to reject queue as %s", name)

In _save_msg_to_queue, errors would be caught by the IO/OS error handler, but removing that showed that the raw bytestring from 'body' was being placed into Dirq which handled it as if it was a string causing a decode error.

Our Dirq schemas suggest that we only want strings in this point. I added a ducktype test that solves both problems in _save_msg_to_queue.

ssm/ssm2.py Fixed Show fixed Hide fixed
ssm/ssm2.py Fixed Show fixed Hide fixed
@RedProkofiev RedProkofiev marked this pull request as ready for review September 29, 2023 14:58
@RedProkofiev RedProkofiev requested a review from a team as a code owner September 29, 2023 14:58
@RedProkofiev
Copy link
Contributor Author

Can confirm, works with python 2.

@tofu-rocketry
Copy link
Member

Can confirm, works with python 2.

Except note the Travis CI failure with Python 2 - test_ssm.py needs a Unicode declaration at the top.

ssm/ssm2.py Outdated Show resolved Hide resolved
@tofu-rocketry
Copy link
Member

tofu-rocketry commented Feb 7, 2024

Actually sending the test string via AMS works in 2 & 3, using directory as the queue type only.

Receiving doesn't work in 2 (but does in 3).

2024-02-07 12:56:17,077 - ssmreceive - ERROR - Unexpected exception: 'ascii' codec can't encode character u'\xe9' in position 275: ordinal not in range(128)
2024-02-07 12:56:17,077 - ssmreceive - ERROR - Exception type: <type 'exceptions.UnicodeEncodeError'>
2024-02-07 12:56:17,077 - ssmreceive - ERROR - The SSM will exit.

@tofu-rocketry
Copy link
Member

Oh, I should test dirq sending rather than just directory, as the former was reported as the issue in #270.

@tofu-rocketry
Copy link
Member

Ok, adding UTF-8 into the mix here was hiding the issue that the original tickets reported. And testing using directory rather than dirq worked around the original issue.

The problem is the lack of strict encoding and decoding at the program boundaries. This does also mean that there's a choice to make about codec to use, but need to be careful not to break the APEL loader.

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

Successfully merging this pull request may close these issues.

Check issue about type (str/bytes) of messages in outgoing queue bytes vs. text and python 3
2 participants