Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ecdsa-p256 TLV support removed and breaking updates for everyone #1690

Closed
nordicjm opened this issue Apr 26, 2023 · 7 comments
Closed

ecdsa-p256 TLV support removed and breaking updates for everyone #1690

nordicjm opened this issue Apr 26, 2023 · 7 comments
Assignees
Labels
area: core Affects core functionality area: zephyr Affects the Zephyr port bug

Comments

@nordicjm
Copy link
Collaborator

I've just had a look at some the commits which were merged today which change the TLV of the ecdsa-p256 TLV, which previously had an ID of 0x22 to now have an ID of 0x25. So I've gave this a little try from zephyr of building mcuboot and an image with the version of mcuboot there and the same but with the latest version of mcuboot from here at main with the key set to ECDSA-P256 and it seems that with the new mcuboot build, it is wholly unable to boot images built for older versions of zephyr or using older versions of mcuboot.

Now maybe I'm just overlooking something simple and it's actually all fine, but @Roolli @davidvincze @adeaarm could you please confirm if the above is true or not, and if the above is true, why you think it's acceptable to have just broken mcuboot/mcuboot-built images for essentially everyone in the world using mcuboot that was using ecdsa-p256?

@davidvincze
Copy link
Collaborator

Hi Jamie,
I'll get back to this issue tomorrow, but in the meantime, I would recommend using
the imgtool's --legacy-ecdsa-tlv option for the 0x22 TLV ID; or using the latest v1.10.0 release tag.

@carlescufi
Copy link
Collaborator

carlescufi commented Apr 26, 2023

it seems that with the new mcuboot build, it is wholly unable to boot images built for older versions of zephyr or using older versions of mcuboot.

I don't think this is acceptable if I understand it correctly @davidvincze. This has never happened before in MCUboot (that I am aware of) and I do not think this should happen at all. Thanks in advance for looking into it.

In the future, could we try and make sure that PRs like #1617 have a wider review audience before they are merged?

cc @d3zd3z @joerchan @sigvartmh

@carlescufi carlescufi added area: core Affects core functionality area: zephyr Affects the Zephyr port labels Apr 26, 2023
@de-nordic
Copy link
Collaborator

There should be some process of depreciating features. This is no a 0-day thing that requires immediate response and fix.
You could have just starting with MCUboot and building something around the feature, that now have to decide whether they finish they work on older version of MCUboot and then adapt or start reworking what they have to adapt now.
imgtool is not only signing tool used with MCUboot, if people can not adapt their projects they will be left out of important fixes that may appear or will have to cherry pick stuff on top of older MCUboot, just to keep their toolsets working.
This is not stable base for development.

@davidvincze
Copy link
Collaborator

I'm really sorry for the inconvenience and of course, it was not my intention.
#1617 was open for 2 months, but now I see I made a mistake and should have drawn more attention to it.

There should be some process of depreciating features. This is no a 0-day thing that requires immediate response and fix.

I agree, this could have been avoided. The basic idea was that it's unnecessary to have different TLV types for separate algorithms and it just cannot be expected that backward compatibility is kept forever; in the future MCUboot might change its behaviour for example by introducing new mandatory TLVs (breaking backward compatibility) or refactoring/abandoning the complete TLV area - but again, I understand that it happened too suddenly.

I'm going to upload a patch soon to "rehabilitate" the IMAGE_TLV_ECDSA256 TLV type. I'd really appreciate your help with the review. It was a change of little importance, but greater impact...

@davidvincze
Copy link
Collaborator

#1694 to fix this issue by restoring the original ECDSA TLV usage has been merged,
and #1691 was opened as a follow-up issue to prevent similar cases in the future.

@nordicjm can we close this issue?

@nordicjm
Copy link
Collaborator Author

Can be closed, yes.

@carlescufi
Copy link
Collaborator

Very much appreciate the fast turnaround time fixing this @davidvincze!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: core Affects core functionality area: zephyr Affects the Zephyr port bug
Projects
None yet
Development

No branches or pull requests

4 participants