Shautvast/add cbor validation #7169
Conversation
|
Hi @shautvast. Thanks for your contribution! Can you please adjust your commit messages to follow the convention? |
| 1: [* expression] ;; expressions | ||
| } | ||
|
|
||
| expression = [action expr] ;; Expression |
There was a problem hiding this comment.
This grammar does not follow exactly the struct definitions for Expr:
actionis(nil text). Whynil?1..7 expr_datainstead of* expr_data- no distinction between
Expr::SeqandExpr::List - there is no
Expr::Ident
I am not really familiar with CDDL and there might be good reasons why you did it this way. Don't you think we should spend more time to make the grammar more precise?
There was a problem hiding this comment.
#1 why nil: stupid argument: it won't work without it :). The nil is there in the binary cbor (246_u8 to be precise) . This may even be an bug in mini-cbor as far as I know. NB this expresssion is not a choice, it's a ccdl list, so nil followed by text is how the Action struct is serialized. Maybe this is just the way tuple structs are handled, because they don't have named fields.
#2 I don't see [* expr_data]. You're referring to [+ expr_data]? I find that recursive definition quite accurate as it reflects the one in Expr
#3 in cbor representation there is only the enum constant (6 or 7) so that's how it's distinguished when (de)serializing. The enum data is the same.
#4 It's there as enum constant 5. Contents are same as Str, (String -> text) so that's the reason it's not mentioned explicitly
Yes I suspect that the expr cddl can be somewhat more precise. Will look into that
There was a problem hiding this comment.
- The cddl is now both clearer and more strict. Please have a look.
- I found the actual reason for Nil
This is the definition:
pub struct Expression {
#[n(1)]
action: Action,
#[n(2)]
expr: Expr,
}
which is encoded as a 0-indexed list, but the first attribute (Action) hs #[n(1)], so Nil is inserted at index 0 to put action at index 1 in the list. One way to fix this is to replace 1 and 2 by 0 and 1, so that Nil can just disappear from the CDDL and you save 1 byte in the serialized message. Love to hear your thoughts on that
| } | ||
|
|
||
| impl Arbitrary for PolicyList { | ||
| fn arbitrary(g: &mut Gen) -> Self { |
There was a problem hiding this comment.
It would be good to compose an Arbitrary for Action and an Arbitrary for Expr to create this Arbitrary
There was a problem hiding this comment.
agree, but the validate_with_schema test util is in ockam_api and Expr and Action are in ockam_abac
I see no easy and elegant solution to this (but maybe my rust knowledge is incomplete here)
I tried to move the util to ockam_abac, but is it possible to have dependencies on test modules?
otherwise I'd have to copy, which I don't like, obviously
| fn arbitrary(g: &mut Gen) -> Self { | ||
| Expression { | ||
| action: Action::new(String::arbitrary(g).as_str()), | ||
| expr: Expr::List(vec
shautvast
force-pushed
the
shautvast/add_cbor_validation
branch
from
875adf4 to
50601ee
Compare
shautvast
force-pushed
the
shautvast/add_cbor_validation
branch
from
50601ee to
be38259
Compare
shautvast
force-pushed
the
shautvast/add_cbor_validation
branch
from
be38259 to
2479d67
Compare
shautvast
force-pushed
the
shautvast/add_cbor_validation
branch
from
2479d67 to
9bc6625
Compare
Current behavior
Currently structs are serialized using CBOR, but the messages are not validated against a CDDL schema definition
Proposed changes
fix for issue #6690
cddl validation added for:
The cddl schema file was extended where necessary
No validation added for the following structs since they seem to have been removed since the issue was created
Also: as mentioned in the issue as an optional refactor, CowStr and it's lifetime have been removed
Checks