Make the fields of mbedtls_x509_crt_profile public #4671
Conversation
These fields are supposed to be manipulated directly, that's how people create custom profiles. Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
mpg
added
bug
needs-review
include/mbedtls/x509_crt.h
Outdated
| * | ||
| * The fields of this structure are part of the public APi and can be | ||
| * manipulated directly by applications. Future versions of the library may | ||
| * add extra fields. |
There was a problem hiding this comment.
Suggestion:
| * add extra fields. | |
| * add extra fields or reorder fields. |
There was a problem hiding this comment.
Actually, I'm not completely sure about this. Do we want to support applications that initialize with profile = {a, b, c} and are content with keeping whatever 0 means for the other fields? Or should applications only ever start with a built-in profile to get “sensible” values for new fields?
There was a problem hiding this comment.
Let's start with saying we may reorder fields, and we can lift this self-imposed restriction later.
There was a problem hiding this comment.
I think we should document whether applications are are expected to always start from a built-in profile and modify it, or if they can also start with the all-0 profile - that is, is and when we add fields, do we commit to the value 0 for the new field to always mean the same behavior as before that field was added?
There was a problem hiding this comment.
I think given that we define mbedtls_x509_crt_profile_default it seems not particularly useful to support starting from the all-0 profile? +1 for documenting this expectation though.
There was a problem hiding this comment.
it seems not particularly useful to support starting from the all-0 profile
It would make sense to start from an all-is-forbidden rather than from a sensible-default profile if you want to have precise control over what to allow. Unfortunately, all-zero isn't the ideal all-is-forbidden profile: it does mean all is forbidden, but if you start allowing RSA by adding it to allowed_pks, you'll allow RSA keys of any size — a better all-is-forbidden profile would have rsa_min_bitlen == impossibly_high.
There was a problem hiding this comment.
I pushed something which I think clarifies expectations and supports the use case Gilles was mentioning.
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
mpg
force-pushed
the
x509-crt-profile-public
branch
2 times, most recently
from
6588ccd to
55a7fb8
Compare
| * | ||
| * The fields of this structure are part of the public API and can be | ||
| * manipulated directly by applications. Future versions of the library may | ||
| * add extra fields or reorder existing fields. |
There was a problem hiding this comment.
Hmm, we may want to change field types either. Not very likely, but suppose we decide to add a 33rd curve before Mbed TLS 4.0 comes out...
mpg
added
needs-work
and removed
needs-review
mpg
added
needs-review
|
CI is unhappy |
gilles-peskine-arm
added
needs-ci
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
mpg
force-pushed
the
x509-crt-profile-public
branch
from
f59e567 to
9d4c2c4
Compare
Aw sorry, that's embarrassing, the code I pushed wasn't even syntactically correct. I re-enabled my pre-push hook in an attempt to only push slightly less embarrassing mistakes in the future. |
gilles-peskine-arm
added
needs-review
|
@daverodgman There has been changes since your previous approval, can you re-review? (I forgot to mention this yesterday, but this is also something we really want in 3.0.) |
daverodgman
removed
the
needs-review
These fields are supposed to be manipulated directly, that's how people
create custom profiles.
Evidence: we recommend direct manipulation of those fields right in the migration guide
Fixes #4603