Skip to content

Conversation

ariawisp
Copy link

@ariawisp ariawisp commented Sep 4, 2025

This is a rebased version of #61 that also addresses the outstanding comments on the original PR, and adds Apple providers.

@whyoleg whyoleg self-requested a review September 4, 2025 18:23
Copy link
Owner

@whyoleg whyoleg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks for the PR! I've performed first round of review and left quite a lot of comments. Hope that most of them could be easily solved!

P.S. I'm quite curious, was any AI tool used to implement any part of the PR? There is no problem with it, just curiocity :)

- Docs: remove duplicated EdDSA/XDH sections (JDK/OpenSSL3); fix CryptoKit notes; clarify WebCrypto availability wording and remove opt-in flag.
- WebCrypto: drop engine gating + experimental flag; pass key.algorithm to EdDSA ops; fix private key JWK/RAW mapping; use KeyDeriveAlgorithm for ECDH.
- JDK EdDSA: support RAW via PKCS8/SPKI wrap/unwrap.
- OpenSSL3 EdDSA/XDH: add RAW key format via DER wrap/unwrap; pass curve through wrappers; extract shared derive to operations.
- Tests: remove WebCrypto skips; align JDK EdDSA gating to JDK<11; add EdDSA/XDH compatibility tests; include in generator.
…ers; Compatibility tests: include JWK for WebCrypto in EdDSA/XDH
…ot path); JDK: unify RAW SPKI/PKCS#8 unwrap via helper; wire OID constants
… fix curve param decoding for EdDSA/XDH compat tests; adjust ASN.1 AlgorithmIdentifier to allow missing parameters (RFC 8410).
… Tests: use context.provider.isCryptoKit in EdDSA/XDH default tests\n- Compatibility: skip Ed448/X448 for CryptoKit to match capabilities\n- ASN.1: tolerate absent/NULL AlgorithmIdentifier parameters (RFC 8410)\n- SwiftInterop: generate SPM platforms with .vN style; prefer OS runtime\n\nAlso includes current local changes in support files and providers.
…t EdDSA\n\n- Extract accumulating signature update logic to provider-base\n- Replace CryptoKit EdDSA local functions with shared base impls
@ariawisp ariawisp requested a review from whyoleg September 5, 2025 01:47
@ariawisp
Copy link
Author

ariawisp commented Sep 5, 2025

Thanks for the review @whyoleg ! All the comments should be addressed now.

To answer your question regarding AI: yes, OpenAI's Codex CLI with GPT-5 (high reasoning) was used for the entirety of this PR.

Highlights of changes since your review:

  • Docs:

    • jdk.md/openssl3.md: removed extra “EdDSA/XDH” sections to avoid duplication.
    • cryptokit.md: clarified only Ed25519/X25519 supported; DER/PEM and RAW encodings available via provider APIs.
    • webcrypto.md: clarified EdDSA/XDH were added later to WebCrypto; added RFC references (RFC 8032, RFC 7748).
  • WebCrypto:

    • Removed engine detection and all gating; rely on runtime availability errors from the engine.
    • Implemented JWK and RAW support for EdDSA/XDH (public/private) with PEM/DER wrapping via processors.
    • Unified key derivation to use KeyDeriveAlgorithm (removed the separate ECDH variant).
  • OpenSSL3:

    • Added RAW key format support for EdDSA/XDH via SPKI/PKCS#8 wrap/unwrap on encode/decode.
    • Extracted shared-secret derivation into operations and reused it.
    • Switched EdDSA to one‑shot mode (nullable hashAlgorithm) in digest-based sign/verify.
  • CryptoKit:

    • Moved Ed25519/X25519 OIDs into asn1-modules and used them.
    • Extracted accumulating sign/verify into provider-base (AccumulatingSignFunction/VerifyFunction) and adopted in EdDSA.
    • Compatibility tests skip only unsupported curves (Ed448/X448).
  • JDK:

    • Added RAW support to EdDSA (via SPKI/PKCS#8 wrap/unwrap).
    • Tests now reflect correct split: XDH since JDK 11, EdDSA since JDK 15.
  • Tests:

    • Fixed provider checks to use context.provider where appropriate.
    • Simplified SupportedAlgorithmsTest gating (Apple vs others).
    • Removed WebCrypto browser skip rules; let engines handle availability at runtime.
  • ASN.1:

    • AlgorithmIdentifier deserializer now tolerates absent/NULL parameters (RFC 8410) to avoid decode errors on SPKI/PKCS#8.
    • Added Edwards/Montgomery OIDs to asn1-modules.

Copy link
Owner

@whyoleg whyoleg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR is really big so reviewing it becoming a bit hard, but here is the second round of review.

If the whole PR is done via AI - I will probably need some time more to review it locally and perform some additional manual changes, to split commits/changes, so that the history will be nicer. Or you can do it if you like :)
Depending on your answer, I will provide more instructions

@@ -8,6 +8,7 @@ For supported targets and algorithms, please consult [Supported primitives secti

* KeyFormat: doesn't support `JWK` key format yet
* AES.GCM supports only a default tag size of 96 bits
* EdDSA/XDH: CryptoKit supports only Ed25519 and X25519. DER/PEM and RAW key encodings are available via the provider APIs.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* EdDSA/XDH: CryptoKit supports only Ed25519 and X25519. DER/PEM and RAW key encodings are available via the provider APIs.
* EdDSA/XDH: supports only Ed25519 and X25519

@@ -21,6 +21,7 @@ For supported targets and algorithms, please consult [Supported primitives secti

* KeyFormat: doesn't support `JWK` key format yet


Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

@@ -9,10 +9,12 @@ For supported targets and algorithms, please consult [Supported primitives secti
* only `suspend` functions are supported, because `WebCrypto` API is async by default
* AES.* (browser only): may not support `192 bit` keys
* AES.CBC: only `padding=true` is supported
* EdDSA/XDH: These algorithms were added later to WebCrypto and might not be available in all engines/browsers yet (see [RFC 8032] and [RFC 7748]). If unsupported, operations will fail at runtime with an error from the underlying engine.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* EdDSA/XDH: These algorithms were added later to WebCrypto and might not be available in all engines/browsers yet (see [RFC 8032] and [RFC 7748]). If unsupported, operations will fail at runtime with an error from the underlying engine.
* EdDSA/XDH were added later to WebCrypto and might not be available in all browsers (https://github.com/w3c/webcrypto/pull/362)


## Example

```kotlin
// default provider
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// default provider

Comment on lines +37 to +39

[RFC 8032]: https://www.rfc-editor.org/rfc/rfc8032
[RFC 7748]: https://www.rfc-editor.org/rfc/rfc7748
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[RFC 8032]: https://www.rfc-editor.org/rfc/rfc8032
[RFC 7748]: https://www.rfc-editor.org/rfc/rfc7748

@@ -44,11 +44,12 @@ abstract class GenerateSwiftPackageDefinitionTask : DefaultTask() {
outputDirectory.get().asFile.recreateDirectories()

val swiftinteropModuleName = swiftinteropModuleName.get()
fun ver(v: String) = ".v$v"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those changes are most likely unrelated and should be reverted, why are they needed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes were made to address linker/compiler issues on macOS 26 beta / Xcode 26.

@@ -69,7 +70,11 @@ abstract class GenerateSwiftPackageDefinitionTask : DefaultTask() {
dependencies: [],
targets: [
.target(
name: "$swiftinteropModuleName"
name: "$swiftinteropModuleName",
swiftSettings: [
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those changes are most likely unrelated and should be reverted, why are they needed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes were made to address linker/compiler issues on macOS 26 beta / Xcode 26.

@@ -45,13 +45,19 @@ abstract class XcodebuildBuildTask : DefaultTask() {

exec.exec {
it.workingDir(temporaryDir)
it.commandLine(
val developerDir = System.getenv("DEVELOPER_DIR") ?: ""
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those changes are most likely unrelated and should be reverted, why are they needed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes were made to address linker/compiler issues on macOS 26 beta / Xcode 26.

val parameters = decodeParameters(algorithm)
check(decodeElementIndex(descriptor) == CompositeDecoder.DECODE_DONE)
parameters
when (val idx = decodeElementIndex(descriptor)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to implement those changes in a separate PR/commit with additional tests

@@ -26,8 +26,10 @@ internal object KeyAlgorithmIdentifierSerializer : AlgorithmIdentifierSerializer
}
ObjectIdentifier.EC -> EcKeyAlgorithmIdentifier(decodeParameters(EcParameters.serializer()))
else -> {
// TODO: somehow we should ignore parameters here
// For algorithms like Ed25519/Ed448/X25519/X448 (RFC 8410), parameters MUST be absent.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without tests it's not clear, why are those needed

@ariawisp
Copy link
Author

ariawisp commented Sep 6, 2025

Thanks for the second review @whyoleg - let me see what I can do for breaking this up into more smaller commits/PRs

@ariawisp ariawisp marked this pull request as draft September 6, 2025 15:19
@ariawisp
Copy link
Author

ariawisp commented Sep 7, 2025

Split PRs Overview: EdDSA/XDH Series

Below is a concise list of the new PRs that split the original combined work, with links, scope, and dependencies. All target main and keep test/CI setup unchanged. Recommended merge order is Foundation → Providers → Tests.

Merge Order (high level)

Foundation

Providers

Tests

Notes

@ariawisp ariawisp closed this Sep 7, 2025
@whyoleg
Copy link
Owner

whyoleg commented Sep 7, 2025

Thanks for the split!
I will take a look at all of the PRs in detail in the coming days.
I do have a question: do you mind if I commit minor changes in those PRs, if I see that it will be faster/easier, compared to convincing AI to do the correct thing? :)
You will still be mentioned in commits, of course.

@ariawisp
Copy link
Author

ariawisp commented Sep 7, 2025

I don't mind at all, please do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants