-
Notifications
You must be signed in to change notification settings - Fork 4
feat: implemented cwt status list #81
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
base: main
Are you sure you want to change the base?
Conversation
…List, and CWTStatusToken classes
|
…ns objects for better clarity and maintainability
…y type and export CborStatusListOptions interface
…aims to use camelCase for consistency
…able names for clarity;
… verification methods
…pe for type definitions
… StatusArray and StatusList
…ven URI with timeout handling
… in CWTStatusToken
@@ -0,0 +1,56 @@ | |||
import * as zlib from 'pako' | |||
|
|||
const arraySize = 1024 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we hardcode it to 1024?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the specs there is no limit actually
const expirationTime = claims.get(String(CwtStatusListClaims.ExpirationTime)) | ||
if (expirationTime && typeof expirationTime === 'number' && expirationTime < Math.floor(Date.now() / 1000)) { | ||
throw new Error('CWT status token has expired') | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shoud also check iat
/ nbf
claims if they are valid right now? Would be good to move the iat
/ exp
and nbf
check to the CWT verification method, rather than the Status list (since it's verification that needs to happen for all CWTs).
Also some nits:
- I see
Math.floor(Date.now() / 1000)
several times in the code. I usually create an utildateToSeconds
which optionally date a Date (and otherwise creates a new date). - I usually allow providing an optional
now?: Date
that we use as the current date if you want to for some reason have now be different than Date.now(), (in tests can be really useful as well).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the spec these are not mandatory claims for a CWT.
} else if (cwt instanceof Mac0) { | ||
coseType = CoseType.Mac0 | ||
} else { | ||
throw new Error('Unimplemented/Unsupported CWT type') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw new Error('Unimplemented/Unsupported CWT type') | |
throw new Error(`Unsupported CWT structure type ${coseType}. Supported values are sign1 and mac0`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will also fail for untagged Sign1 and Mac0 structures. However, I am not sure common that is.
return false | ||
} | ||
|
||
static async fetchStatusListUri(statusListUri: string): Promise<Uint8Array> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be great if:
- you can provide a custom fetcher to override the usage of this default one
- allow providing a custom timeout to override the default timeout.
e.g. you can look at sd-jwt-js as an example: https://github.com/openwallet-foundation/sd-jwt-js/blob/main/packages/sd-jwt-vc/src/sd-jwt-vc-instance.ts#L274
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Um, it's a static function itself. Not sure, what do you mean by custom fetcher because in the example you provided, it seems to be used by some other functions where it makes sense. But maybe not in our scenario atm
src/credential-status/status-list.ts
Outdated
aggregationUri?: string | ||
} | ||
|
||
interface CborStatusList { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If CborStatusList is a cose structure, can you make it like is done for the rest of the structures in the library? This way, we have a really simple and consistent way of dealing with cose structures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@berendsliedrecht Can you please give some example. Will help to me to get more context and understand what you mean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
every file in this folder has a cose structure. https://github.com/animo/mdoc/tree/main/src/mdoc/models
} | ||
|
||
static async verifyStatusToken(options: CWTStatusTokenVerifyOptions): Promise<boolean> { | ||
const cwt = cborDecode(options.token) as Sign1 | Mac0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we use the class like the rest of the lib, like mentioned above, we don't have to call cborDecode here and I think its <STRUCTURE>.fromEncodedStructure(<string>)
, which provides better support than decoding and type casting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But in here, options.token is a Uint8Array and Sign1.fromEncodedStructure() expect this [Uint8Array, Map<unknown, unknown>, Uint8Array | null, Uint8Array]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you try Sign1.decode
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might work, but we typically can't determine whether it's a Sign1 or a Mac0, which could cause issues during parsing. I believe the current approach is more reliable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait why can't we determine that? sign1 is tagged as 18
and mac0 is 17
, i.e. something detectable that we can use to decode it.
This current way is not consistent with every other part of the library and we should always avoid that.
} else if (cwt instanceof Mac0) { | ||
coseType = CoseType.Mac0 | ||
} else { | ||
throw new Error('Unimplemented/Unsupported CWT type') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will also fail for untagged Sign1 and Mac0 structures. However, I am not sure common that is.
…ken and CWT classes
…es; adjust StatusList to use updated bitsPerEntry accessor
… CwtOptions interfaces
…ns; update related methods and tests
…e utility; improve claim handling
…replace generic errors with specific ones
…eSecurityObject and IssuerSignedBuilder
…eSecurityObject; replace StatusInfoStructure with StatusInfoOptions
…s and encoding logic
…y; update issuer signed builder tests to include status token verification
No description provided.