Skip to content

Remove serialization for derived key handles, remove keySpec property#40

Merged
jkoenig134 merged 7 commits into
nmshd:feature/cal-integrationfrom
WyvernIXTL:feature/cal-integration
Jun 23, 2025
Merged

Remove serialization for derived key handles, remove keySpec property#40
jkoenig134 merged 7 commits into
nmshd:feature/cal-integrationfrom
WyvernIXTL:feature/cal-integration

Conversation

@WyvernIXTL

@WyvernIXTL WyvernIXTL commented Jun 5, 2025

Copy link
Copy Markdown

Readiness checklist

  • I ensured that the PR title is good enough for the changelog.
  • I labeled the PR.
  • I self-reviewed the PR.

Description

  • Remove the keySpec property of BaseKeyHandle and derivatives, due to the property neither being used, nor ts-serval being able to skip certain verifications (instanceof with @validate()).
  • Have Derived variants of of key handle not be able to serialize. A derived key handle is ephemeral. Deserializing such key handle is only possible if a provider stores an ephemeral key handle, or if the serialized representation stores the secret key. Neither is in the spirit of a key handle.
  • Fix from* methods of derivatives of BaseKeyHandle inferring BaseKeyHandle as return type.

Removed `KeySpec` from BaseKeyHandle as there is currently no use in sync context.
Refactored *Derived variant of key handles to not use BaseKeyHandle anymore, but rather a
DerivedBaseKeyHandle that does not support serialization. The need stems from
derived key handles being ephemeral. If they are being dropped, they do not exist anymore.
Hence deserializing ephemeral key handles does not make sense and will result in an error.
@Milena-Czierlinski

Copy link
Copy Markdown
Member

@WyvernIXTL Please note, that we switched to GitHub Releases in this repo. Since we now use labels to generate the release notes, we no longer prefix the PR titles.

@WyvernIXTL WyvernIXTL changed the title Refactor/remove serialization for derived key handles, remove keySpec property Remove serialization for derived key handles, remove keySpec property Jun 5, 2025
@WyvernIXTL WyvernIXTL marked this pull request as ready for review June 5, 2025 14:17
@WyvernIXTL

Copy link
Copy Markdown
Author

@Milena-Czierlinski Hi, sorry, I overlooked that. I cannot label this PR or assign anyone to review this PR for that matter.

@Milena-Czierlinski Milena-Czierlinski added the refactoring Refactoring of code label Jun 6, 2025
@Milena-Czierlinski

Copy link
Copy Markdown
Member

I cannot label this PR or assign anyone to review this PR for that matter.

Mhh that is strange. Would you like me to assign a reviewer for you for now? @WyvernIXTL

@WyvernIXTL

Copy link
Copy Markdown
Author

@Milena-Czierlinski Yes, please.

@Milena-Czierlinski

Copy link
Copy Markdown
Member

And whom...? 😂

@WyvernIXTL

WyvernIXTL commented Jun 6, 2025

Copy link
Copy Markdown
Author

@Milena-Czierlinski ┐⁠(⁠‘⁠~⁠`⁠;⁠)⁠┌
Maybe @ngussek (doesn't wanna)

@ngussek

ngussek commented Jun 10, 2025

Copy link
Copy Markdown

@Milena-Czierlinski I don't think I'm the most qualified person to do that review. Could you choose someone who worked on ts-crypto before or someone who is experienced with developing the runtime in general?

@Milena-Czierlinski

Milena-Czierlinski commented Jun 18, 2025

Copy link
Copy Markdown
Member

In the failing pipeline it says ERROR: PRs from forks are only supported when trigger on "pull_request_target". According to this post the reason seems to be that this PR was created from a forked repository. Hence, we should evaluate either the working method or our GitHub Actions setting. But since this might lead to insecurities, I would suggest to discuss this with @jkoenig134.

@jkoenig134

Copy link
Copy Markdown
Contributor

Hm, the quick solution for that would be to finally switch to a non forked version of this repo as discussed with @ngussek and @WyvernIXTL.

But we can check if it's possible to support this pipeline for forks later.

@jkoenig134 jkoenig134 merged commit 45df57f into nmshd:feature/cal-integration Jun 23, 2025
1 of 5 checks passed
@jkoenig134

Copy link
Copy Markdown
Contributor

@WyvernIXTL released in https://github.com/nmshd/ts-crypto/releases/tag/2.2.0-alpha.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactoring Refactoring of code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants