-
Notifications
You must be signed in to change notification settings - Fork 4
refactor: Implement major interfaces of Java Cryptography Architecture and reorganize existed services into OpenSslProvider
#25
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
…ntException` classes
…chAlgroithmException`
…ner` and serveral exceptions
…`SSLSessionContext`
No, it's not necessary, we can merge everything in one go.
What we need is some tests that cover the classes and can run on JVM as well and have the same results in both JVM and Native.
The only suggestion from skimming over the code is that we don't need the
I agree with you. It seems to be the best approach. |
SSLContext required classes
|
A mark for signatures of major classes have been added and programs are compiled successfully under Scala 2.12, 2.13 and 3. Then next step I will try to implement the details. |
…`, `KeyGenerator` and `SecretKeyFactory` classes
SSLContext required classesOpenSslProvider
OpenSslProviderOpenSslProvider
|
@lolgab Hey, I have updated the content and goal of current PR, could you check the first update for more details? I'm glad to explain anything that's unclear. Any feedback and advice is appreciated! Thanks a lot 😄 |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
|
I'm updating Mill in #26. Let's see if it fixes your problems with Java 25 |
Yes, it works. I add Java 25 back to CI matrix. But now, I changed the CI command to |
| runner.dialect = scala213 | ||
|
|
||
| # automatically appended by scalafmt itself | ||
| project.excludePaths = ["glob:**/out/**", "glob:**/jwt-scala-tests/src/**"] |
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 excluding jwt-scala-tests? Doesn't sound correct.
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 have no idea, I can remove it. Since these also aren't added by myself either. scalafmt (via metals) will hint these while opening current project with IDE combo: VS Code + metals + mill
| val osName = { | ||
| val _osName = Properties.osName.toLowerCase() | ||
| if (_osName.contains("mac")) "darwin" | ||
| else if (_osName.contains("windows")) "windows" | ||
| else "linux" | ||
| } | ||
| val archName = Properties.propOrEmpty("os.arch").toLowerCase() match { | ||
| case "amd64" => "x86_64" // try to follow llvm triple | ||
| case "arm64" => "aarch64" | ||
| case s => s | ||
| } |
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 would avoid this indirection to make java os.arch and osName to look like clang target triple values.
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.
Or follow the GCC triple? I copy similar conditional detections from mill project to detect platform (win/linux/mac). But the os.arch triple is added by myself. I thought scala-native is LLVM based, so I use the LLVM style.
The main purpose is to provide basic snippets for finding proper include libs. Do you have better ideas?
lolgab
left a comment
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.
Left some comments but haven't gone over the whole PR yet.
scala-native-crypto/src/com/github/lolgab/scalanativecrypto/OpenSslProvider.scala
Outdated
Show resolved
Hide resolved
| sealed abstract class CryptoPrimitive | ||
| object CryptoPrimitive { | ||
|
|
||
| /** Symmetric primitive: block cipher */ | ||
| case object BLOCK_CIPHER extends CryptoPrimitive | ||
|
|
||
| /** Asymmetric primitive: key agreement and key distribution */ | ||
| case object KEY_AGREEMENT extends CryptoPrimitive |
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 is not the way to convert a Java enum in Scala. Check for examples in the scala-native main repository
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.
Fixed and add a test to ensure the behavior.
Hey @lolgab,
I'm Lanqing Huang, and working on the Issue Support for
java.net.http.HttpClient.In general, I'm trying to implement a Java
HttpClientshim for Scala Native. My repo lqhuang/scala-native-http is also forked from your lolgab/scala-native-http-client-async. Thank you for your initial work!And obviously, the
SSLContextis a crucial component of theHttpClient. Hence, I recognize projectscala-native-cryptoas an upstream project ofscala-native-http. I hope to contribute several essential classes intoscala-native-crypto.I have finished some classes with signature or basic implementations. It wouldn't be easy to review, I'm thinking I should ask your opinions first :) Like:
And after few works, I found
SSLContexthave many abstractions aboutSSLSocket(well, they're layouted underjavax.net.ssl)So, I'm also thinking I'm probably just need to port related classes under
java.securityandjavax.cryptointoscala-native-cryptoand leftSSLContextand others injavax.netback toscala-native-http?Regards,
Lanqing
First update
I have spent a lot of time to learn
And now the goal of current PR has shifted. Bascially, I add all major interfaces of Java Crypto modules and do some refactors without changing current done features.
I think we could merge something first and then in next PR, I will try to implement crypto algorithms required by
SSLContext. It should be eaiser for both you and me?Here are the updates to help you with your review:
Major interfaces or abstract classes required by JCA, including
AlgorithmParameterGeneratorAlgorithmParametersCertificateFactoryCertPathBuilderCertPathValidatorCertStoreCipherExemptionMechanismKDFKEMKeyAgreementKeyFactoryKeyGeneratorKeyPairGeneratorKeyStoreMessageDigestSecretKeyFactorySecureRandomSignatureAll the methods and their exceptions have aligned with JDK docs
Implement a "Provider" to allow using different crypto backends.
Then we can register custom implementations of providers (openssl / boringssl / s2n-tls / ...) in the future. e.g.: While installing
scala-native-crypto-boringsslwill registerBoringSSLProviderBut it's not perfect. Java's crypto and security modules are "finished" since JDK 1.4 and it have been designed to use dynamic loading for classes which is not usable for Scala Native.
They are two classes need to implement under JCA (
ServiceandSericeSpi, likeCipherandCipherSpi, hereSpistands Service Provider Interface). But I would say it's quite messy and chaos, I found sometime theServiceextends theSerivceSpi, sometime theSericeSpiis passing intoServiceconstructor via composition while I reading JDK docs.The good news is both
ServiceandServiceSpiare not exposed to user. So in this PR, I decide to remove all spi interfaces and keep abstractServiceclasses, which may not exactly the same with JDK architectures.Then, I tried to refactor two current existed services
MacandMessageDigestinto an unifiedOpenSslProvider(The underlying internal implementations haven't been modified), you could check related to see is the design suitable or not?IMHO, we're impossible to get a perfect Java shims, but the current solution just work and it will be clearer to complete all crypto algorithms in the future.
Improve README to provide a list of implemented algorithms for users and contributors
Improve
build.mill.scalaincludepath finder as flag in different platform.Upgrade CI workflow
actions/checkout@v3->actions/checkout@v5,actions/setup-java@v3->actions/setup-java@v5Testing results (comment out
IllegalArgumentExceptiontests forSecretKeySpecconstructor)Flaky parts: I cannot understand why
SecretKeySpecconstructor tests cannot pass now. I have tuned to throwIllegalArgumentExcSeptioninstead ofNullPointerException...Second update
lolgab help me to upgrade mill version to v1.0.6 which fix the Java 25 CI jobs. And he also address the NPE issue. Nice!
All tests have passed. All CI jobs are also resolved.