Marshaling refactor #30
Conversation
|
The PR is still in Draft state, is there anything that is still been worked upon? Also, just realised this is a breaking change, in a way that currently CCA Scheme do not explicitly call Validate Functions, but now they will need to call the specific variant that Validates or call two functions in sequence. |
Argh, no, for some reason, it seems to have defaulted to "draft". It is ready for review. (draft state should be removed now).
Yes, that is correct. It is marked as a breaking change in the commit. |
|
|
||
| return nil | ||
| // ValidateAndSign validates and then signs the given evidence using the |
There was a problem hiding this comment.
In my view, we should not mirror Signing to look exactly as CBOR and JSON Marshal/UnMarshal.
A Sign method, by default should always Validate!
This is the current mainline implementation
If needed ( I am still struggling with the use case though), we should provide a new method:
that lets caller, Sign UnValidatedClaims().
There was a problem hiding this comment.
Sigining and validation are two distinct operations that if the calling code wants to perform separately, it should . E.g. when generationg knowingly bad inputs (such as for testing, or to work around quirks), or to avoid annecessary re-validation when signing a previously validated structure.
There was a problem hiding this comment.
If you see the commit message, this change, does not align well with the commit intentions and in general the scope of the PR. I suggest, we can discuss this internally and once everyone agrees then re-insert this part of change which modifies the existing sign interface, as a separate PR!
There was a problem hiding this comment.
Good point about the commit message; i reused the pastoken one, and it should be ammended to reflect this change as well. However, this change is absolutely aligned with the intent behind this commit, which is to align with convenetion/expectation and reduce surprise. And is absolutely not the sepectation that Sign() performs validation as well (especially as there is already a separate Validate() ).
While sepate from marshaling, Sign() is part of the same problem, namely that validation has been confounded with other operations, necessaitating the addition of the cruft-y "unvalidated" API, and fixing the problem involves fixing Sign() as well.
There was a problem hiding this comment.
ok, then please change the commit title to Marshaling and Sign refactor, to be clear.
Also make the same change to psatoken as it is symmetrical
setrofim
force-pushed
the
marshaling
branch
3 times, most recently
from
e202f5e
to
777912b
Compare
This commit refactors marshaling and signing API. It has two objectives: - Align how marshaling and signing work with conventional Go practices, ensuring new users familiar with other Go libraries have minimal surprises. - Minimize the impact of marshaling on IClaims interface, relieving the burden on current and future implementations. To that end, the following changes are made: - From/To* marshaling API are replaced with implementations of standard cbor and json marshaling interfaces (i.e. Unmasrshal/Marshal*). As per convention, these implementations do not validate. - There are no validating versions of marshaling methods (thus no requirement for every IClaims implementation to re-implement them). Instead, there are convenience functions that instantiate, marshal, and validate in a single call. - The functions have been renamed such that Decode* now only performs unmarshaling, and DecodeAndValidate* unmarshals and validates. - Decoding/Encoding functions for IClaims have been moved from profile.go to iclaims.go. - Sign(), similarly has been changed to not validate, SignUnvalidated() has been dropped, and ValidateAndSign() convenience method has been added (the later replicating the old Sign() behavior)> note: this is implemented on top of a similar refactor inside psatoken BREAKING CHANGE: the From/To marshaling API have been removed; Decode* functions have been renamed to be consistent with marshaling (rather then DecodeXXX--that validates--and DecodeUnvalidatedXXX, there is now DecodeAndValidateXXX and DecodeXXX--that does not validate). BREAKING CHANGE: Sign() no longer validates; SignUnvalidated has been dropped. Signed-off-by: Sergei Trofimov <sergei.trofimov@arm.com>
This commit refactors marshaling API. It has two objectives:
To that end, the following changes are made:
note: this is implemented on top of a similar refactor inside psatoken