-
Notifications
You must be signed in to change notification settings - Fork 1.7k
asn1: Add support for CHOICE fields #14201
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Facundo Tuesca <[email protected]>
a4d382e to
db9795f
Compare
|
I haven't reviewed the impl closely yet, but this is probably my favorite of the various APIs we've brainstormed. @reaperhulk wdyt? |
|
Hmm, what does this look like when applied to the basic real world example of a generalname? (I would actually do this myself but am currently mobile) |
Something like this (assuming the non-trivial types are defined elsewhere): # GeneralName ::= CHOICE {
# otherName [0] OtherName,
# rfc822Name [1] IA5String,
# dNSName [2] IA5String,
# x400Address [3] ORAddress,
# directoryName [4] Name,
# ediPartyName [5] EDIPartyName,
# uniformResourceIdentifier [6] IA5String,
# iPAddress [7] OCTET STRING,
# registeredID [8] OBJECT IDENTIFIER }
GeneralName: typing.TypeAlias = (
Annotated[OtherName, asn1.Implicit(0)]
| Annotated[
asn1.Variant[asn1.IA5String, typing.Literal["rfc822Name"]],
asn1.Implicit(1),
]
| Annotated[
asn1.Variant[asn1.IA5String, typing.Literal["dNSName"]],
asn1.Implicit(2),
]
| Annotated[ORAddress, asn1.Implicit(3)]
| Annotated[Name, asn1.Implicit(4)]
| Annotated[EDIPartyName, asn1.Implicit(5)]
| Annotated[
asn1.Variant[
asn1.IA5String, typing.Literal["uniformResourceIdentifier"]
],
asn1.Implicit(6),
]
| Annotated[bytes, asn1.Implicit(7)]
| Annotated[x509.ObjectIdentifier, asn1.Implicit(8)]
)
@asn1.sequence
class Example:
name: GeneralName
obj = Example(name=asn1.Variant(asn1.IA5String("abcd"), "dNSName"))
encoded = encode_der(obj)
decoded = decode(Example, encoded)
assert isinstance(decoded.name.value, asn1.IA5String)
assert decoded.name.tag == "dNSName"
assert decoded.name.value.as_str() == "abcd" |
|
I wonder if maybe we should require the use of # current API, this would be the `iPAddress [7] OCTET STRING` variant
obj = Example(name=b"\x01\x02")
encoded = encode_der(obj)
decoded = decode(Example, encoded)
assert isinstance(decoded.name, bytes)
assert decoded.name == b"\x01\x02"
# API where every variant is defined with a `Variant(type, tag)` annotation
GeneralName: typing.TypeAlias = (
#...
| Annotated[
asn1.Variant[bytes, typing.Literal["iPAddress"]],
asn1.Implicit(7),
]
# ...
obj = Example(name=asn1.Variant(b"\x01\x02", "iPAddress"))
encoded = encode_der(obj)
decoded = decode(Example, encoded)
assert isinstance(decoded.name.value, bytes)
assert decoded.name.tag == "ipAddress"
assert decoded.name.value == b"\x01\x02" |
|
Either all variant or no-variant perhaps. |
how would no-variant work? |
|
Would be fine for cases where all the variants have different types, and therefore don't need anything to disambiguate them. |
Signed-off-by: Facundo Tuesca <[email protected]>
Done, I also updated the PR description with the changes. |
src/cryptography/hazmat/asn1/asn1.py
Outdated
| Tag = typing.TypeVar("Tag") | ||
|
|
||
|
|
||
| @dataclasses.dataclass(frozen=True) | ||
| class Variant(typing.Generic[U, Tag]): | ||
| """ | ||
| A tagged variant for CHOICE fields with the same underlying type. | ||
|
|
||
| Use this when you have multiple CHOICE alternatives with the same type | ||
| and need to distinguish between them: | ||
|
|
||
| foo: ( | ||
| Annotated[Variant[int, "IntA"], Implicit(0)] | ||
| | Annotated[Variant[int, "IntB"], Implicit(1)] | ||
| ) | ||
|
|
||
| Usage: | ||
| example = Example(foo=Variant(5, "IntA")) | ||
| decoded.foo.value # The int value | ||
| decoded.foo.tag # "IntA" or "IntB" | ||
| """ | ||
|
|
||
| value: U | ||
| tag: 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.
Does Tag either need to be annotated to indicate its a Literal[str], or should we just allow arbitrary tag 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.
the docstring is wrong, fixed
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.
My question was more about the Tag 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.
How would you annotate it? Literal[str] is not a valid expression
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 not sure! The answer is maybe that this is all correct -- but I was wondering if the Tag typevar should have a bound of some sort on 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.
We can add a bound of LiteralString. It's not perfect, str and any subclass of str will pass the type check if passed as a Tag, but we can catch those at runtime.
So something like:
Tag = typing.TypeVar("Tag", bound=typing.LiteralString)
@dataclasses.dataclass(frozen=True)
class Variant(typing.Generic[U, Tag]):and we keep the runtime check for Tag being a typing.Literal.
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 pushed a commit with this ^ change
| let expected_tags = expected_tags_for_type(py, inner, encoding); | ||
| match parser.peek_tag() { | ||
| Some(next_tag) if expected_tags.contains(&next_tag) => (), | ||
| _ => return Ok(default.clone_ref(py).into_bound(py)), |
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 seems bad, in that it's going to result in tons of allocations, can we instead have an is_tag_valid_for_type or something?
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 good catch, fixed
| assert_roundtrips([(Example(foo=9), b"\x30\x03\x02\x01\x09")]) | ||
| assert_roundtrips([(Example(foo=True), b"\x30\x03\x01\x01\xff")]) | ||
| assert_roundtrips([(Example(foo="a"), b"\x30\x03\x0c\x01a")]) |
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 three cals to assert_roundtrips, rather than passing a list to the function.
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
Co-authored-by: Alex Gaynor <[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]>
2ce5211 to
35a56da
Compare
src/cryptography/hazmat/asn1/asn1.py
Outdated
| Tag = typing.TypeVar("Tag") | ||
|
|
||
|
|
||
| @dataclasses.dataclass(frozen=True) | ||
| class Variant(typing.Generic[U, Tag]): | ||
| """ | ||
| A tagged variant for CHOICE fields with the same underlying type. | ||
|
|
||
| Use this when you have multiple CHOICE alternatives with the same type | ||
| and need to distinguish between them: | ||
|
|
||
| foo: ( | ||
| Annotated[Variant[int, "IntA"], Implicit(0)] | ||
| | Annotated[Variant[int, "IntB"], Implicit(1)] | ||
| ) | ||
|
|
||
| Usage: | ||
| example = Example(foo=Variant(5, "IntA")) | ||
| decoded.foo.value # The int value | ||
| decoded.foo.tag # "IntA" or "IntB" | ||
| """ | ||
|
|
||
| value: U | ||
| tag: 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.
My question was more about the Tag TypeVar.
| }; | ||
| // Parse the TLV data (which contains the field without the EXPLICIT | ||
| // wrapper) | ||
| asn1::parse(tlv.full_data(), |d| { |
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 tlv.parse()
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.
tlv.parse() requires knowing the type at compile time:
pub fn parse<T: Asn1Readable<'a>>(&self) -> ParseResult<T> {| mod tests { | ||
| use crate::declarative_asn1::types::{AnnotatedType, Annotation, Encoding, Type, Variant}; | ||
| #[test] | ||
| fn test_decode_implicit_choice() { |
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, why do we need a rust test case for this? Not reachable from the PYthon API?
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, we fail first at the Python level if someone annotates a CHOICE with IMPLICIT
|
|
||
| match encoding { | ||
| // Utility function to see if a tag matches an unnanotated variant. | ||
| pub(crate) fn is_tag_valid_for_variant( |
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.
Can you refactor this out into a seperate PR for ease of review.
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.
done, see #14231
Signed-off-by: Facundo Tuesca <[email protected]>
Signed-off-by: Facundo Tuesca <[email protected]>
Following the discussion in #14183, here's an alternative implementation for CHOICE fields.
If one of the types inside the union is a
asn1.Variant, all the other types should be too.Part of #12283