-
Notifications
You must be signed in to change notification settings - Fork 1.7k
asn1: Add support for PrintableString
#13496
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
Conversation
Signed-off-by: Facundo Tuesca <[email protected]>
7aba7ee to
8c1812c
Compare
| let inner_str: pyo3::pybacked::PyBackedStr = val | ||
| .get() | ||
| .inner | ||
| .extract(py) | ||
| .map_err(|_| asn1::WriteError::AllocationError)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't need a pybacked string, val.get().inner.bind(py) should be all we need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Signed-off-by: Facundo Tuesca <[email protected]>
Signed-off-by: Facundo Tuesca <[email protected]>
Signed-off-by: Facundo Tuesca <[email protected]>
Signed-off-by: Facundo Tuesca <[email protected]>
Signed-off-by: Facundo Tuesca <[email protected]>
508abec to
3b22b14
Compare
tests/hazmat/asn1/test_api.py
Outdated
| def test_repr_printable_string(self) -> None: | ||
| assert ( | ||
| repr(asn1.PrintableString("MyString")) | ||
| == "PrintableString(MyString)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want PrintableString("MyString") (or single quotes) -- using the repr of the str
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
|
||
| class TestPrintableString: | ||
| def test_ok_printable_string(self) -> None: | ||
| def decoded_eq(a: asn1.PrintableString, b: asn1.PrintableString): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, we should implement __eq__ on asn1.PrintableString.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Signed-off-by: Facundo Tuesca <[email protected]>
| impl PrintableString { | ||
| #[new] | ||
| #[pyo3(signature = (inner,))] | ||
| fn new(inner: pyo3::Py<pyo3::types::PyString>) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should validate that inner is a valid PrintableString, rather than defering that to serialization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I replicate the logic from rust-asn1's PrintableString::verify ? We could also make verify public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
asn1::PrintableString::new(s).is_none() should be enough to check if its invalid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or call rust-asn1's PrintableString::new and discard the result
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Signed-off-by: Facundo Tuesca <[email protected]>
This adds support for encoding/decoding
PrintableString.here's an example of how it would be used:
A couple of implementation notes:
In this comment, I mentioned that the idea was to use
typing.NewTypeto define thePrintableStringtype. However, I had forgotten that aNewTypecannot be told apart from the original type during runtime:The actual solution was to use a Rust type
PrintableStringand expose it directly to Python. This type wraps a normal Python string, and can only be used through a restricted API (similar torust-asn1's):rust-asn1are still limited toAllocationErrors without custom error messages. This is blocked by Allow custom write errors alex/rust-asn1#567, so I'll work on that next.Part of #12283