asn1: Add support for CHOICE fields#14201
Conversation
Signed-off-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>
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 <facundo.tuesca@trailofbits.com>
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.
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.
the docstring is wrong, fixed
There was a problem hiding this comment.
My question was more about the Tag TypeVar.
There was a problem hiding this comment.
How would you annotate it? Literal[str] is not a valid expression
There was a problem hiding this comment.
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.
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.
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.
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.
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.
Why three cals to assert_roundtrips, rather than passing a list to the function.
Co-authored-by: Alex Gaynor <alex.gaynor@gmail.com>
Signed-off-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>
Signed-off-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>
Signed-off-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>
Signed-off-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>
Signed-off-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>
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.
My question was more about the Tag TypeVar.
Signed-off-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>
Signed-off-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>
| assert_roundtrips( | ||
| [(Example(foo=True, bar=1), b"\x30\x06\x01\x01\xff\x02\x01\x01")] | ||
| ) | ||
|
|
||
| assert_roundtrips( | ||
| [(Example(foo=None, bar=1), b"\x30\x03\x02\x01\x01")] | ||
| ) |
There was a problem hiding this comment.
ni: merge into single call to assert_roundtrips
Signed-off-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>
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