Conversation
kraemv
force-pushed
the
feature/set_of_ref
branch
2 times, most recently
from
b38a2cc to
fc1d408
Compare
kraemv
force-pushed
the
feature/set_of_ref
branch
from
fc1d408 to
7aa875a
Compare
kraemv
force-pushed
the
feature/set_of_ref
branch
from
7aa875a to
43bf173
Compare
franziskuskiefer
left a comment
franziskuskiefer
left a comment
There was a problem hiding this comment.
Just a few quick notes.
|
|
||
| new_set | ||
| .iter() | ||
| .is_sorted_by(|a, b| matches!(a.der_cmp(b), Ok(Ordering::Less))) |
There was a problem hiding this comment.
This doesn't seem right. is_sorted_by expects the comparator to return true when a <= b, but the closure returns true only for Ordering::Less (strict).
There was a problem hiding this comment.
This is intentional, as this will prevent building a set with duplicate elements. Do you think I should elaborate on this in a comment?
There was a problem hiding this comment.
Yes, I think elaborating on this in a comment and arguing why it's correct with respect to the spec would be good. Otherwise this is confusing.
There was a problem hiding this comment.
Actually, in X.680, 27.3 NOTE 3 it states that:
"The set-of type is not a mathematical set of values, thus, as an example, for SET OF INTEGER the values { 1 } and { 1 1 } are distinct."
Hence duplicate values are explicitly allowed. But since the other SET OF implementation does not allow duplicate values, I will also not allow them in a SET OF reference.
There was a problem hiding this comment.
Hmm, good point. I'm noticing that we do have logic to check for duplicates (along with an error variant), but re-reading X.680 I'm not actually seeing a requirement that precludes duplicates for set-of. It says for DER the order needs to be ascending but not necessarily strictly ascending.
There was a problem hiding this comment.
Yes, and the example in X.680 explicitly states an example with duplicate elements.
Only for SETs no duplicates are allowed, as it says in X.690, 26.4.
There was a problem hiding this comment.
What matters for our purposes is whether it’s actually a valid DER production. There are examples of all sorts of things in X.690 which are invalid DER
There was a problem hiding this comment.
I fixed the handling of duplicates in the new PR #2272. Once this gets merged, I can rebase and also change the behavior for the SetOfRef struct.
kraemv
force-pushed
the
feature/set_of_ref
branch
from
43bf173 to
119445e
Compare
kraemv
force-pushed
the
feature/set_of_ref
branch
from
119445e to
1284f02
Compare
4 participants
Hello everyone,
in this PR I add a referenced version to the SetOf struct. This can be useful to implement referenced versions of structs containing SetOfs.
In order to allow both decoding from DER bytes and creating references from existing SetOfs, the SetOfRef contains an internal reference that can be both. The inner reference to raw bytes is not pretty, but I do not see another way to implement this with no_std requirements. Most functions like get are then implemented over iterators, that hide the inner reference type. I check sorting on all constructors.
In order for CI to go through, we need a MSRV of 1.87 for the heapless crate.
I am happy to receive feedback on this PR!