Crypto module improvements #108
Conversation
- When importing public keys, `Extractable` function parameter no longer required, as public keys can always be extracted.
- The `Task` for exporting public keys no longer has the `ExportKeyError` error type and instead has `{}`. This is due to exporting public key always succeeding (they can always be exported).
Tests updated to reflect these changes. Ran and confirmed tests still pass with the above changes.
The returned `Bytes` are the `Bytes` that were verified. Allows easier mapping or other actions to be taken with those verified `Bytes`. The error type remains `x` for composability. Ran tests locally and confirmed everything is passing on my machine.
|
@robinheghan One question I have here pertaining to consistency in type signatures: In the case of failure of Tasks who cannot fail, is it preferable to have the type signature be |
|
@joeybright |
src/Crypto.gren
Outdated
| {-|-} | ||
| importEcdhPublicKeyFromJwk : EcNamedCurve -> Json.Encode.Value -> Extractable -> SecureContext -> Task ImportEcKeyError (PublicKey EcdhKey EcKeyParams) | ||
| importEcdhPublicKeyFromJwk namedCurve jwk extractable _context = | ||
| importEcdhPublicKeyFromJwk : EcNamedCurve -> Json.Encode.Value -> SecureContext -> Task ImportEcKeyError (PublicKey EcdhKey EcKeyParams) |
There was a problem hiding this comment.
Have we discussed the argument order with regards to SecureContext before?
SecureContext acts a permission value, and we tend to have that as the first parameter in other API's.
Do you remember the rationale for setting it last here?
There was a problem hiding this comment.
@robinheghan I don't think you mentioned it before, no. I don't remember the motivation behind the decision and don't feel strongly about it. If you want to make this change, let me know, and I'll get it done!
There was a problem hiding this comment.
In that case I'd prefer the SecureContext to be the first parameter.
|
This fixes the |
…es (vs. function not expected to error at all)
The function is not expected to error and, for that reason, the error type is unnecessary. Shored up testing to make sure it doesn't error in all known cases. New tests pass on my machine.
The function is not expected to error and, for that reason, the error type is unnecessary. Tests still pass on my machine with the change.
Updated tests - they compile, run, and pass on my machine.
src/Crypto.gren
Outdated
|
|
||
|
|
||
| {-|-} | ||
| exportKeyAsSpki : Key a b -> Task ExportKeyError Bytes |
There was a problem hiding this comment.
This function doesn't seem to be used anywhere or exposed.
There was a problem hiding this comment.
@MartinSStewart I see it used a few places, including here:
Line 865 in c2f284d
There was a problem hiding this comment.
Sorry, I quoted the wrong line. I meant the next function down exportKeyAsSpki
There was a problem hiding this comment.
@MartinSStewart Got it! Yeah, it looks like no private keys can be exported to the SPKI format with the Web Crypto API. I went ahead and removed that function given it's not being used.
Now that verification functions, when successfully run, return the verified bytes, checking if those bytes are the same as the bytes originally passed to the function.
4 participants
Improvements implemented based on feedback from Discord and #106. See commits for descriptions of what changed.