search

a-sit-plus / signum

Kotlin Multiplatform Crypto/PKI Library and ASN1 Parser + Encoder
https://a-sit-plus.github.io/signum/
Apache License 2.0
68 stars 4 forks source link

Tmp/json key id on init #95

Closed n0900 closed 3 months ago

n0900 commented 4 months ago

When a JsonWebKey is created from a CryptopublicKey which in turn was created by a keyPair it does not have a keyId. However if we transform this JsonWebKey back to a CryptoPublicKey the identifier gets saved to keyId and if we then transform it back again to JsonWebKey it now has a keyid -> the previous identifier.

As an easy fix I propose this change, however other ideas are welcome.

JesusMcCloud commented 4 months ago

seems legit. do we need something like that for COSE keys too? also API docs and tests need fixing, but we should go for this!

nodh commented 4 months ago

But won't this also add a keyId, when converting from a JWK to a CryptoPublicKey and then back again?

iaik-jheher commented 4 months ago

So if I understand you correctly, the conversion from JsonWebKey (no keyId) to CryptoPublicKey is adding a key ID, where none exists?

If so, why isn't that the issue to fix?

n0900 commented 4 months ago

But won't this also add a keyId, when converting from a JWK to a CryptoPublicKey and then back again?

True, this will override a JWK kid if it was null to begin with. Not perfect.

n0900 commented 4 months ago

So if I understand you correctly, the conversion from JsonWebKey (no keyId) to CryptoPublicKey is adding a key ID, where none exists?

If so, why isn't that the issue to fix?

Because if we fix it like that then no JsonWebKey we generate using our library will ever have a keyID, which might not be ideal

iaik-jheher commented 4 months ago

Maybe it should be possible to specify a keyId when converting to JsonWebKey, then?

n0900 commented 3 months ago

Maybe it should be possible to specify a keyId when converting to JsonWebKey, then?

we could lift the additional property from the jsonwebkey to a main property of the CryptoPublicKey itself, what do you think of the recent commit as a base? If this is okay I will extend it for JWK and COSE.

n0900 commented 3 months ago

Obviously we would then need remove the additional property, instead we can move the JWK thumbprint as an additional property which then makes it easily accessable as it was/is quite flaky right now.

nodh commented 3 months ago

Better to squash all commits into one when merging? (-:

iaik-jheher commented 3 months ago

We squash merge anyway, no need to force push the PR branch... this just ends up making the CIs take even longer to finish.