Incompatible change of OCM-Component-Descriptor-Normalisation between OCM-CLI v0.18.0 vs v0.19.0 #1214
Comments
github-actions
bot
added
the
area/ipcei
|
Im assuming this is due to #1026 |
|
digests-ocmcli.tar.gz |
|
As suspected, the diff (https://www.diffchecker.com/hsZgn13L/ for some clarity on the relevant diff) explains and redirects the issue back to the change mentioned above: in v0.18 now becomes in v0.19 The normalization now doesnt encode the version in by default anymore which is a breaking change that killed the signature. this has to be adjusted again (valid bug). the pr tried to circumvent this break with the following
This is only after some superficial checking from me ... but I believe this behavior regressed because the content check for the component version was only targeting handling of new component versions but accidentally also changed how existing component versions are normalized. I am currently theorizing that since this behavior of hashing the component version is an implicit expectation on the signing with jsonNormalization/v1 this might prove difficult... basically we can never change the version identity defualting ever because it is implicitly part of the signing contract. meaning that we have to keep the old behavior the default (encode it) and have to explicitly toggle it off if wanted. What a mess... still investigating further |
|
for confirmation, could you also check the digests when you run with |
|
digests-ocmcli.tar.gz This will yield yet different hash-digests.. |
|
I believe the issue is that the logic that was added will only work if the repository is the same. If we do a transfer between repositories, the modification flag on the component version will be true as the component version is not yet present in the staging repo. Then, when the component version is added into the staging repository, it thinks that we are creating a new version instead of just transferring an old version. This then leads to the new logic kicking in and not writing the extraIdentity by default anymore where it previously did, which then triggers this issue. Now we have to decide what to do:
Still investigating further now. |
|
@jakobmoellerdev : what would happen if we would always (implicitly) add i.e. adding version to extraID basically would be ignored (unless version's value differs from if there is an incompatible change in ocm-cli, we will have to define an upgrade-path (note that our full delivery-chain has a turnaround of roughly two weeks, so ideally such an upgrade-path needs to be automation-friendly). |
This is the defacto rule now (even though with a slight deviation) for the normalization v1 and v2, but only in case we can not uniquely identify a component version without it. Because that got removed in the recent change, the break appeared. Because this is part of the normalization in both v1 and v2, the only way we can ensure backwards compatibility is by reverting how this normalization is handled and then creating a v3 normalization that doesnt have this logic in its expecation. Basically we need to Ensure that json normalization v1 & v2 always assume that extraIdentity will have a field called version defaulted if both the extraIdentity was not set before and there is another resource with the same name (but not the same version). This is now (even though it was never intended) a part of the Normalization Contract as it is implicit behavior. |
|
after verifiyng the component descriptors manually it turned out that one of the descriptors had an extraIdentity version field explicitly set. However during signature calculation in 0.19.0 and above (main) this was niled out so this is definetly buggy. I believe this should be fixeable, checking where this breaks now |
|
considering that a simple and now returns is concerning because the v1.11.3 identity attribute does not actually exist in the repository. I believe that get cv should only return what is in the repository, but the logic seems to have touched rendering of all component versions, regardless if it comes from a repository or not. Current Assumption: Because ocm 0.19.0 now has the version encoded always, it also cannot determine if the identity previously existed or not. Then it will fail to check if it needs to add the version to the identity during the signature check. This will then break. |
|
To fix this issue I now managed to return backwards compatibility to v1+v2 normalizations with a legacy code patch and by introducing a new v3 normalisation algorithm that doesnt need to do this. Additionally I added some test cases to make sure this doesnt happen again. Apart from the extra identity defaulting v2 and v3 are identical. Now also old component versions will be verifiable if using the correct algorithm. I will backport the v1/v2 correction to v0.19 for sure. I will discuss with the team if we even will offer the v3 normalization as part of the hotfix but that shouldnt matter for this bug. If not, then v3 will be available with v0.20 |
|
The problem is not the normalization, which should be completely fine (at least for this scenario, here). We have to assure, that this defaulting is only done during component version composition for creation, not during the transport process. So far, the transport tool checks for a non breaking change during the transport, and then uses the Modification option when composing the target component version. Unfortunately, this option also enables the defaulting. Therefore, the transport changes signature relevant information of the component descriptor, which finally breaks the hash and the signature. A solution is shown in #1223, which disabled this defaulting by using a new modification option. Unfortunately, I don't have access to the SAP internal staging repos anymore, so I cannot test this change for the actual issue. |
|
I checked the normalization explicitly via test case and confirmed it is indeed not fine. The normalized json should not have changed, but it did. The extra identity defaulting in v1/v2 is part of the normalization contract and thus also of the signature. It is much easier for users to migrate to the new normalization algorithm than to have to check for "should modify" everytime they do a transport. We simply cannot continue increasing usage of "should modify" in every single command that touches the descriptors. Not changing the descriptors should be the default but as you mentioned its already messy enough with SetResource as is. I do not want to change behavior anymore without incrementing versions. We can also adopt the flag to prohibit modifying signature relevant elements, but it should still not have broken the existing component versions after the default happened in the transport. |
|
The normalized json only changes, if the extra identity field of the CD is changed. This is not a problem of the normalization, this is the intention of the normalization. In the old version (18) there was no defaulting when calling SetResource, this has been changed with 19. Now the transport indeed changes the extraIdentity field in the CD. This for sure MUST change the normalization version. So, the normalization works perfectly fine. The problem is the change of the extraIdentity field during the transport, which MUST NOT happen. So, the fix is to prohibit this change of the signature relevant CD fields during the transport instead of changing the normalization. The defaulting should not be part of the normalization and it is not part of the normalization. The existing call to the Default method of the serialization version just |
|
If you compare ocm's output of v0.18.0 and v0.19.0 on the same component version, the hash changes. This is not intended, but now unfortunately part of the contract. Since between v0.18.0 and v0.19.0 the digest can not have changed on its own, this cannot be due to the transport. (because the transport was only triggered once and that should have changed the digest only once, however I can reliable "switch" between the digests with the same component version). Thus the normalization is also broken. It is NOT fine for the hash to suddenly change from older to newer versions, even if the behavior is potentially incorrect. This is why v3 exists. |
|
The hash changed AFTER the transport when using version 19 as explained in the issue. It did not happen with version 18. Again, this is the problem. It occurs because of an error in #1026. The behavior of version 19 should NOT be preserved, because it is WRONG. |
|
The behavior of v0.18.0 cannot be restored for component versions which already have the defaults set (already transported component versions which were either transported with v0.19.0 like in this example, or that were transported before but with explicit extra identity attributes matching the version defaulting done in v0.18.0), no matter if explicit or not, because that is part of the normalization. |
|
To be clear, Im not arguing against not defaulting in the transport with a flag (the change you introduced) as this will fix it defaulting on the transport in the first place. But still we cannot change the hash calculation for old component versions. |
|
I don't understand what you mean. If you transport something with version 19, which has been created with v18, is signed AND has been based on the implicit extraIdentity handling for resources with the same name, then the resulting version in the target is corrupted, you cannot verify it anymore without fixing the defaulted version in the extraIdentity of the concerned resources in the CD. Therefore, everything transported with v19 is potentially corrupted and should be re-transported with a fixed version, before it can be used. This has nothing to do with the normalization. We have to fix the transport handling, not the normalization. |
Assume you have a component that already contains an extra identity with the version defaulting in place, as well as a component without it. In this case, v1 and v2 of the jsonNormalization will equalize them and will be equal in v0.18.0 and below, whereas in v0.19.0 they are not. While in itself a good thing, this still is a backwards incompatible change. You are surely right that we have to fix the transport handling (too) to avoid future component verisons having no extraIdentity set in the component descriptor. Nevertheless, the old hash calculation method must be preserved and versioned off correctly, because this could also have happened intentionally. For sure, the transport handling adjustment will allow to actually not have the defaulting in place, which was buggy in 0.19.0. Nevertheless, the hash behavior of 0.19.0 must not change for old component versions which were not modified and for which there is no new signature. It does not matter if the signature methodology is actually the correct one, it needs to be consistent. To be correct, we should urge to resign / normalize with v3. Thus we need:
|
No, they don't! The normalizations as well as the hashes are different for both CVs, but identical each for v18 and v19. If you have two component versions: and The CD for the first one does NOT contain the extraIdentty if created with v18: Therefore, the hashes are different. The normalization never changes the content, neither in v18 nor v19. If you create the implicit version with v19, you get indeed the same as for the explicit one. but this is correct, because the resource meta datas are identical because of the defaulting done by v19 during the COMPOSITION. So, the normalization is correct and not the problem, the problem is the wrong behavior during the transport, switching from the implicit to the explicit representation. |
|
Im not gonna continue going back and forth with you here. The change of the algo is not about the case you described above. Its about component versions that already contain the extra identity. youre looking at components that do not yet contain the extra identity (which is fine for new components, but not already existing ones). The issue is that you assume component versions transported with v0.19.0 are "corrupt". However, because its part of a GA delivery, this is now part of the contract. Its GA for a reason. It is simply not acceptable that there is a component version in repositories now (which btw could have been generated in other ways than through the transport command) that can output different results based on which ocm cli version you use. Of course we could issue a warning that new component versions after transport might contain the extra identity field or not, but the issue is still that the component descriptor itself DID contain the extra identity field before. you simply cannot remove this behavior. |
|
GA or not GA. I introduced a bug, which must be fixed. The behavior is NOT correct. If you transport such a CV created and signed with v18 and transport it with v19 the target is definately corrupted. Even your fix, which by the way violated the basic OCM assumption to never change signature relevant content (especially during signing) cannot change this fact, because both cases (implicitly added by the erroneous transport tool or explicitly given) are using the same normalization method. |
|
First of all. Your test case is wrong. you are testing with a component constructor and you need to test with existing component versions with extra Identity set to {}, for example: This you can then use to test your transfer and also the hash command. Also you can not depend on You will observe that not only does #1223 achieve nothing (because v0.19.0 does not actually default the default identity, as intended before), v0.18 and below (we tested down until v0.10) DO the defaulting of the extra Identity during transport. As such the only behavior that can work, is to introduce a break in the normalization and move on. The defaulting behavior introduced way back was implicitly added to the contract |
…ion/v1 as well as jsonNormalisation/v2 (open-component-model#1218) <!-- markdownlint-disable MD041 --> #### What this PR does / why we need it Package jsonv3 provides a normalization which is completely based on the abstract (internal) version of the component descriptor and is therefore agnostic of the final serialization format. Signatures using this algorithm can be transferred among different schema versions, as long as is able to handle the complete information using for the normalization. jsonv2 is the predecessor of this version but had internal defaulting logic that is no longer included as part of this normalization. Thus v3 should be preferred over v2. Note that between v2 and v3 differences can occur mainly if the "extra identity" field is not unique, in which case the v2 normalization opinionated on how to differentiate these items. This no longer happens in v3, meaning the component descriptor is normalized as is. v2 and v1 were adjusted to accomodate the old(but new because forgotten) legacy behavior in legacy.go. Without this, old signatures would not work. This means this should be (at least partially) back-ported to the last minor versioned released without this correction. #### Which issue(s) this PR fixes <!-- Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`. --> This issue fixes open-component-model#1214 and supercedes open-component-model#1217 as a better solution longterm (by getting rid of the old normalization) and shortterm (by achieving full backwards compatibility + introducing a simple test case) Note that this changes the default normalization algorithm to be `jsonNormalisation/v3` instead of `jsonNormalisation/v1` as it is important for users to migrate as soon as possible.
|
We will soon release v0.19.1 as a release candidate so that this can be tested. To hash existing component versions, please use this will return the same output as previous CLIs. please switch to the new normalization algorithm jsonNormalisation/v3 at your earliest convenience for hashing and validating (the new default) |
…ion/v1 as well as jsonNormalisation/v2 (#1218) (#1230) Note that this introduces a json normalization algorithm v3 which technically will break a minor release. however this is needed as we currently broke hashing and need to offer a working alternative. thus it should have been part of v0.19.0 already: #1214 <!-- markdownlint-disable MD041 --> #### What this PR does / why we need it Package jsonv3 provides a normalization which is completely based on the abstract (internal) version of the component descriptor and is therefore agnostic of the final serialization format. Signatures using this algorithm can be transferred among different schema versions, as long as is able to handle the complete information using for the normalization. jsonv2 is the predecessor of this version but had internal defaulting logic that is no longer included as part of this normalization. Thus v3 should be preferred over v2. Note that between v2 and v3 differences can occur mainly if the "extra identity" field is not unique, in which case the v2 normalization opinionated on how to differentiate these items. This no longer happens in v3, meaning the component descriptor is normalized as is. v2 and v1 were adjusted to accomodate the old(but new because forgotten) legacy behavior in legacy.go. Without this, old signatures would not work. This means this should be (at least partially) back-ported to the last minor versioned released without this correction. #### Which issue(s) this PR fixes <!-- Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`. --> This issue fixes #1214 and supercedes #1217 as a better solution longterm (by getting rid of the old normalization) and shortterm (by achieving full backwards compatibility + introducing a simple test case) Note that this changes the default normalization algorithm to be `jsonNormalisation/v3` instead of `jsonNormalisation/v1` as it is important for users to migrate as soon as possible. This is a backport of a93ecca in #1218
|
The problem is even much more complicated. It consists of two problems:
The problem is caused by a typical error using the Golang range iterator in the default.go sources of the serialization versions. This method was called in v0.18.0 and removed in v0.19.0. Unfortunately it does NOT do this. Because the loop variable Because the ocm lib and the CLI never created an empty map, everything was fine in our tests, but Christian uses a python implementation, which always serializes an empty map. So the defaulting was never done for CV created with the ocm lib, and always when signing a CV created with python. If there are some resources with a nil map and others with an empty map, then the result will be even more confusing, and I think the fix with #1218 would not work correctly. The default itself would be a good idea to be prepared for getting rid of this implicit version handling in the future. However, the fact that is was rarely done, destroys this effect again. So, a compatible fix would be just to add this function again, but with the current behavior (fixing the code to make this obvious). This is the first thing we should do. And then we could clarify how we could improve the messy situation. |
|
I appreciate your thoughts here but I really have to disagree.
The decision will ultimately lay with the team here, and I will bring it up for discussion. Expect an outcome tomorrow. One more thing: If it turns out that we will stay with the new normalization algorithm as intended we can no longer accept #1223 because it now contains an additional commit you added to fix "two things in one PR" again, even after the original was approved. Please refrain from doing this. |
|
Unfortunately, I see only two possibilities, may be you can find better ones:
I agree with you, that on the long term solution 1 seems to be no good idea. BUT:
The consequence is just, that under the desired goal and the path to reach it, keeping the defaulting in the conversion does not disturb; it is a minor change and makes it not more complicated to reach the goal. The amount of coding is negligible compared with the rest of the still required defaulting-related coding. But we could avoid ugly coding in the existing normalization, the deprecation of the old normalization methods and the need to introduce a new one. Under these points of view keeping the old conversion until we finally can get rid of the defaulting at all seems to me the very best solution. If you really want to follow the more disrupting path of changing the complete normalization, then I would at least prefer option 2b without changing the CV. >Anyway, if we follow this path we also have to assure that mixed scenarios are working correctly, so we should fix the defaulting (2a and 2b) to do it for non-nil maps in the original CD, only, as has been done in the conversion logic before (because of this faulty handling of the iterator). I'm not sure, that this is done correctly in the currently merged fix. BTW. solution 2 just moves coding (you call correctly a nightmare) from the conversion to this normalization, is this really much better, here, you don't get rid of it, anyway, until the defaulting can be removed completely. |
On the long term, we do not want to stick with OCM in this library form at all. Thats why my final goal is to stabilize existing behavior, and make breaking changes abundantly clear.
The defaulting coding has to stay in versions that are still GA and are deprecated (this is happening now). It should not be removed completely so as to not break anyone.
This is not deprecating anything, this is called a breaking change. Please try to stick within the constraint of not introducing breaking changes without explicit version increments.
You made this abundantly clear, I still disagree.
Yes it is because it is versioned off. This is what versions are meant for, to break off unintended behavior and introduce a new version without breaking anyone.
Please take a look at the coding with the weird defaulting ( ocm/api/ocm/compdesc/componentdescriptor.go Line 235 in 782970c You can see that it does not matter if the map is nil or not for this specific part of the normalization because there is an if statement that catches this and creates an empty identity anyways. You can test this with |
|
@jakobmoellerdev : let's have another round of follow-up discussion, please. disregarding the code, I think @mandelsoft has a valid point. |
Context
Our delivery-setup encompasses the replication of OCM-Component-Descriptors between multiple OCM-Repositories (dev, staging, ..). We sign our root component-descriptors at the "left-most" OCM-Repository, and validate signatures after replication into "downstream" OCM-Repositories to ensure integrity of replicated contents.
After upgrading from OCM-CLI v0.18.0 to v0.19.0, validation failed for downstream OCM-Repositories (while, interestingly, consistently not failing for the "left-most" OCM-Repository). I assume this deviance might stem from normalisation(s) that happened during downstream replication.
At any rate, it can consistently be shown that
ocm hash componentversionscommand outputs different hash-digests for the same componentversion.Reproducer
Assuming
ocm-v0.18.0andocm-v0.19.0are available from PATH, and are copies of OCM-CLI equal to respective version-suffixed, and furthermore assuming OCM-CLI is provided w/ required credentials, the following script will yield different hash-digests:Wheareas the following calls (same assumptions as before) will yield equal digests (as expected):
The OCM-Repository from the second two-tuple of commands is the "left-most" OCM-Repository, whereas the OCM-Repository from the first two-tuple of commands is target of replication (from left-most repository).
Observed behaviour:
OCM-CLI v0.19.0 and v0.18.0 will yield different normalisations / hash-digests for equal component-descriptor-trees (see above)
Expected behaviour:
OCM-CLI v0.19.0 and v0.18.0 should yield equal normalisations / hash-digest for equal component-descriptor-tree.
The text was updated successfully, but these errors were encountered: