Repo split sibling: Fix oid backwards deps #29

Closed

mpg wants to merge 2 commits from dev/mpg/sibling-oid-backwards-deps into development

mpg commented

2019-01-28 03:09:04 -06:00

(Migrated from github.com)

As a preparation for deduplicating files, fix backwards include and config dependencies in the OID module.

This is a sibling PR to https://github.com/ARMmbed/mbedtls/pull/2380

Note: currently this is based on mbedcrypto-0.1.0-beta + ITS fix, in order to avoid pulling unrelated changes on the TLS side when referencing the top commit of this branch.

Note: labeling as "do not merge" for now, as repo split has been postponed for a few weeks.

As a preparation for deduplicating files, fix backwards include and config dependencies in the OID module. This is a sibling PR to https://github.com/ARMmbed/mbedtls/pull/2380 Note: currently this is based on mbedcrypto-0.1.0-beta + ITS fix, in order to avoid pulling unrelated changes on the TLS side when referencing the top commit of this branch. **Note:** labeling as "do not merge" for now, as repo split has been postponed for a few weeks.

Patater (Migrated from github.com) reviewed

2019-01-28 03:09:04 -06:00

hanno-becker (Migrated from github.com) reviewed

2019-01-28 03:09:04 -06:00

gilles-peskine-arm (Migrated from github.com) approved these changes

2019-01-29 13:04:07 -06:00

gilles-peskine-arm (Migrated from github.com) left a comment

Looks ok: it's backward-compatible and resolves the dependencies of oid.h and oid.c on x509.h and X509-related configuration. I'm slightly worried about the code size increase in a build where oid.c is included only for the sake of rsa or pkparse, but I doubt that it would be a blocker.

Looks ok: it's backward-compatible and resolves the dependencies of `oid.h` and `oid.c` on `x509.h` and X509-related configuration. I'm slightly worried about the code size increase in a build where oid.c is included only for the sake of rsa or pkparse, but I doubt that it would be a blocker.

library/oid.c

How much of a code size increase does this cause in a build with MBEDTLS_OID_C (for the sake of rsa or pkparse) but without X509?

How much of a code size increase does this cause in a build with `MBEDTLS_OID_C` (for the sake of rsa or pkparse) but without X509?

gilles-peskine-arm commented

2019-01-29 13:05:33 -06:00

(Migrated from github.com)

What's the merge strategy here? Why not merge https://github.com/ARMmbed/mbedtls/pull/2380 and then update mbed-crypto/development to the latest mbedtls/development? Cc @Patater

What's the merge strategy here? Why not merge https://github.com/ARMmbed/mbedtls/pull/2380 and then update `mbed-crypto/development` to the latest `mbedtls/development`? Cc @Patater

mpg commented

2019-01-30 02:12:50 -06:00

(Migrated from github.com)

@gilles-peskine-arm Good question. I suggest we come back to this when we're ready to progress on that front; yesterday we decided to postpone the split to after Q1, so I consider this PR and associated discussion parked for now.

Patater commented

2019-02-19 10:28:10 -06:00

(Migrated from github.com)

Let's merge into Mbed TLS development first and then merge to Mbed Crypto development. We did sibling PRs in the last release because there wasn't anything on the Mbed TLS development branch to merge from, only development-psa. Sibling PRs also made it a bit annoying to reconcile the changes when we did merge from TLS development to Crypto development, so it's preferable to avoid them.

👍 1