-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Initial implementation of ASN.1 API #13325
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
077a337 to
df5289a
Compare
|
I added |
|
looks like there's some missed coverage here -- do you want to take a look at the missing test cases before I review? |
src/cryptography/hazmat/asn1/asn1.py
Outdated
|
|
||
| import typing_extensions as te | ||
|
|
||
| from cryptography.hazmat.bindings._rust import asn1_exp |
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 don't love this module name, I'm not sure what exp even stands for :-)
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.
ah lol, experimental, it was just meant to have a different name from the existing asn1 module
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 probably need to find a real name 😂
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'm open to suggestions. asn1_api maybe?
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 suggest declarative_asn1 for now since it provides the declarative ASN.1 API (or decl_asn1 if we need it to be short)
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.
lets use the full word, bytes are free
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
src/cryptography/hazmat/asn1/asn1.py
Outdated
| from typing import ( | ||
| Any, | ||
| ClassVar, | ||
| TypeVar, | ||
| ) |
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.
Similarly, house style is to reference these as typing. for clarity (Any is particularly unclear without the typing.)
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
src/cryptography/hazmat/asn1/asn1.py
Outdated
| ) -> asn1_exp.AnnotatedType: | ||
| annotation = asn1_exp.Annotation() | ||
|
|
||
| if field_type == builtins.int: |
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.
Prefer is when comparing against types
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
src/cryptography/hazmat/asn1/asn1.py
Outdated
|
|
||
| def _register_asn1_type(cls: type[U], root_type: asn1_exp.RootType) -> None: | ||
| raw_fields = te.get_type_hints(cls, include_extras=True) | ||
| setattr(cls, "__asn1_fields__", _annotate_fields(raw_fields)) |
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.
similarly: cls.__asn1_fields__ = _annotate_fields(raw_fields)
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.
mypy complains:
src/cryptography/hazmat/asn1/asn1.py:55: error: "type[U]" has no attribute "__asn1_fields__" [attr-defined]
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.
Hmm, this error doesn't make much sense to me.
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.
Why do we need both __asn1_fields__ and __asn1_root__? It seems like only one should be required.
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.
yup, see below, I removed __asn1_fields__
Missing tests added! |
|
Will review again tomorrow. I should have said at the top: this looks like a solid start! |
src/cryptography/hazmat/asn1/asn1.py
Outdated
|
|
||
| def _register_asn1_type(cls: type[U], root_type: asn1_exp.RootType) -> None: | ||
| raw_fields = te.get_type_hints(cls, include_extras=True) | ||
| setattr(cls, "__asn1_fields__", _annotate_fields(raw_fields)) |
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.
Hmm, this error doesn't make much sense to me.
src/cryptography/hazmat/asn1/asn1.py
Outdated
|
|
||
| def _register_asn1_type(cls: type[U], root_type: asn1_exp.RootType) -> None: | ||
| raw_fields = te.get_type_hints(cls, include_extras=True) | ||
| setattr(cls, "__asn1_fields__", _annotate_fields(raw_fields)) |
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.
Why do we need both __asn1_fields__ and __asn1_root__? It seems like only one should be required.
src/rust/src/asn1_exp/asn1.rs
Outdated
| // 2.0, and the BSD License. See the LICENSE file in the root of this repository | ||
| // for complete details. | ||
|
|
||
| use pyo3::prelude::*; |
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 don't use import *
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
src/rust/src/asn1_exp/encode.rs
Outdated
|
|
||
| use crate::asn1_exp::types::{AnnotatedType, AnnotatedTypeObject, Type}; | ||
|
|
||
| fn write_value<T: SimpleAsn1Writable>( |
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.
This doesn't seem to do very much?
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.
yeah, it's because it's useful to avoid duplication only when all the other types are implemented. See here (it saves us writing that match statement once for each type)
src/rust/src/asn1_exp/encode.rs
Outdated
| Type::Sequence(cls) => { | ||
| let fields = cls | ||
| .getattr(py, "__asn1_fields__") | ||
| .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.
It seems like instead of a separate __asn1_fields__ we could just store the field types in Type::Sequence()?
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.
yup, good idea. Now Type::Sequence contains the root type and the fields.
tests/hazmat/asn1/test_asn1.py
Outdated
| foo: Invalid | ||
|
|
||
|
|
||
| class TestEncoding: |
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.
Let's put tests of specific types in their own test file.
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
ci-constraints-requirements.txt
Outdated
| # mypy | ||
| # virtualenv | ||
| typing-extensions==4.14.1 ; python_full_version >= '3.9' | ||
| typing-extensions==4.13.2 |
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.
This file should be built in a way that uses the highest version for each python version:
uv pip compile -U --universal --python-version 3.8 --all-extras --unsafe-package=cffi --unsafe-package=pycparser --unsafe-package=setuptools --unsafe-package=cryptography-vectors --unsafe-package=bcrypt pyproject.toml -o ci-constraints-requirements.txt -q is the right way to build it.
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
pyproject.toml
Outdated
|
|
||
| # Must be kept in sync with `project.dependencies` | ||
| "cffi>=1.14; platform_python_implementation != 'PyPy'", | ||
| "typing-extensions>=4.13.2", |
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.
Why is this needed at build time?
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.
it's not, ha. Fixed
|
Looks like there's some merge conflicts |
2cae2ee to
9ecdbe3
Compare
fixed! |
alex
left a comment
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.
Two small comments, but otherwise this LGTM. I'd like @reaperhulk to have a look before we merge though.
src/cryptography/hazmat/asn1/asn1.py
Outdated
|
|
||
| setattr(cls, "__asn1_root__", root) | ||
|
|
||
| def new_init(self: U, /, **kwargs: object) -> None: |
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 probably codegen this for performance, but doesn't have to happen in this PR. Can you add a TODO though?
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.
added a TODO comment
tests/hazmat/asn1/test_encoding.py
Outdated
| import cryptography.hazmat.asn1 as asn1 | ||
|
|
||
|
|
||
| class TestEncoding: |
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.
One class per type we're testing teh encoding of (sequence vs. int so far)
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
pyproject.toml
Outdated
| # Must be kept in sync with `build-system.requires` | ||
| "cffi>=1.14; platform_python_implementation != 'PyPy'", | ||
| # Must be kept in sync with ./.github/requirements/build-requirements.{in,txt} | ||
| "typing-extensions>=4.13.2", |
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.
This is required because we need two functions that were added in Python 3.11 right? is it a requirement for >= 3.11?
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.
If it's not, let's use the python_version branch
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.
Looks like typing has this in 3.11+ (and Facundo did note that in a comment 5 days ago, but there's a lot of traffic on this PR 😄). Let's call it from typing where possible and set the python_version on the rest, so we know when we can remove this...in 4 years.
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! there was an extra typing function which we need to use typing-extensions for: typing.get_type_hints which we call with its include_extras parameter. That parameter was added in Python 3.9, so I also added it to the conditional.
Now it will use the typing-extensions variant for any Python < 3.11. I can make it even more precise (only use it for Python < 3.9), but for now I only left a comment saying to remove it when the min version is 3.9.
if sys.version_info < (3, 11):
import typing_extensions
# We use the `include_extras` parameter of `get_type_hints`, which was
# added in Python 3.9, so this can be replaced by the `typing` version
# once the min version is >= 3.9
get_type_hints = typing_extensions.get_type_hints
else:
get_type_hints = typing.get_type_hintsThere 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.
Is there a reason not to make this branch on 3.9? Seems cleaner.
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.
nope, fixed
alex
left a comment
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.
A small handful of comments, but otherwise LGTM!
src/cryptography/hazmat/asn1/asn1.py
Outdated
|
|
||
| @typing_extensions.dataclass_transform(kw_only_default=True) | ||
| def sequence(cls: type[U]) -> type[U]: | ||
| dataclass_cls = dataclasses.dataclass(cls) |
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 don't want all the defaults that dataclass has, I think to start with the only thing we want to define is init, everything else should be false.
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. I had to add another Python version check, since one of the dataclass arguments (match_args) that defaults to True was added in Python 3.10.
| #[pyo3::pyclass(frozen, module = "cryptography.hazmat.bindings._rust.asn1")] | ||
| #[derive(Debug)] | ||
| pub struct AnnotatedType { | ||
| #[pyo3(get)] |
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.
is there a reason to expose these?
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.
Removed it for now, it will be needed in the future
| #[pyo3::pymethods] | ||
| impl Annotation { | ||
| #[new] | ||
| #[pyo3(signature = ())] |
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 don't think this is required?
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.
Removed
0e3bcc0 to
aa6d03e
Compare
dd06339 to
00cacb8
Compare
alex
left a comment
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.
will wait for @reaperhulk to confirm he's goood, but I think this is ready
|
This needs a rebase now, sorry! |
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]>
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]>
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]>
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]>
Signed-off-by: Facundo Tuesca <[email protected]>
Signed-off-by: Facundo Tuesca <[email protected]>
00cacb8 to
871834e
Compare
Part of #12283
Overview
This PR adds the initial structure for the ASN.1 module, plus support for defining
SEQUENCEtypes and usingINTEGERfields. Only the encoding logic is included, to keep the PR size small and to keep the focus on the structure of the code.Here's an example of what using the API looks like:
Structure
The code is divided into a
hazmat.asn1Python module, and a Rust_rust.asn1_expRust module. This last one has a different name in order to avoid mixing it with the existingasn1Rust module (which can be done later, but for now doing it like this simplifies the PR).Details
Taking as an example the code snippet above, this is what happens:
@asn1.sequencedecorator is executed, which goes through theExampleclass and recursively reads its type annotations. This information is converted to a Rust object (AnnotatedType) and stored in theExample.__asn1_root__andExample.__asn1_fields__attributes. All of this happens inhazmat/asn1/asn1.pyExampleobject is instantiated with some values, andasn1.encode_deris called on it.asn1.encode_deris a Rust function defined inasn1_exp/asn1.rs.AnnotatedTypeobject) and the value of the object into anAnnotatedTypeObjectvalue, and encodes it usingrust-asn1'sasn1::writeAPI.AnnotatedTypeObjectimplements theasn1::Asn1Writabletrait. This is done inasn1_exp/encoding.rsSome things (like the empty
Annotationstruct) are there as placeholders for missing features I removed to keep this PR small. I removed their contents but left the empty struct so that the rest of the data and logic that depend on it will keep the same structure once we re-add the missing features.