search

eclipse-californium / californium

CoAP/DTLS Java Implementation
https://www.eclipse.org/californium/
Other
732 stars 367 forks source link

[Feature request] Ability to define custom cipherSuite. #1693

Closed Nick-The-Uncharted closed 3 years ago

Nick-The-Uncharted commented 3 years ago

Our whole certificate chain is RSA and it's kind of impossible to push our client to give us a EDDSA certificate. I searched issues of this repo and I got #517 . I know there's no point for implementing RSA support in this library, but it would be nice if we can offer our own implementation in our project.

Nick-The-Uncharted commented 3 years ago

I tried rewriting org.eclipse.californium.scandium.dtls.cipher.CipherSuite in my own project to overrwrite original class. I got java.lang.SecurityException: class "org.eclipse.californium.scandium.dtls.cipher.CipherSuite"'s signer information does not match signer information of other classes in the same package. It seems impossible to get around java signature mechanism.

boaks commented 3 years ago

Yes, don't use the maven groupid, this eclipse project is using.

parent - pom.xml

<groupId>org.eclipse.californium</groupId>
<artifactId>parent</artifactId>
<groupId>com.uncharted.rsa.californium</groupId>
<artifactId>parent</artifactId>

I haven't fully tested that, but I guess you will find out, if it works for you.

boaks commented 3 years ago

About [Feature request] Ability to define custom cipherSuite: Though you have implemented it, you know, that this requires a "little" more than just add a value to the enumeration. If you did it for a 2.6 you may have to adapt your changes for a 3.0 as well. There are no plans from my side to keep a API stable to do such an extension,

boaks commented 3 years ago

Just if you're interested, you may try to contribute it (to 3.x). If it doesn't mix up too much, that may be possible.

Nick-The-Uncharted commented 3 years ago

@boaks I will try to submit a mr in this month. If we are going to allow user to define custom cipherSuite, we can no longer use enum to manage cipherSuite. If that's is acceptable, I will try to implement it. But for sure I can't allow everything to be customized, maybe in the end only the certificate is customizable (key_exchange is too much for me).

Nick-The-Uncharted commented 3 years ago

And may I add a rsa cipherSuite to 2.6?

boaks commented 3 years ago

If we are going to allow user to define custom cipherSuite, we can no longer use enum to manage cipherSuite.

Therefore I would not go to support "custom cipherSuite"!

You either adapt it for your own implementation and build it on your own (other groupid), or you try to contribute the changes.

boaks commented 3 years ago

And may I add a rsa cipherSuite to 2.6?

To add function would require a review. Therefore 3.0 is the right place to go. I hope we can release at least a 3.0 in the next 1-2 months (Sept. or October).

Nick-The-Uncharted commented 3 years ago

@boaks My company has very strict security policy, I am not allowed to download opensource code and built my own variant. And we can't use not-yet-stable releases. The only viable ways for me to achieve my goal are:

  1. Submit an mr which support rsa cipherSuite to 2.6
  2. Remove signature of scandium and try to override class I need using hacks.
boaks commented 3 years ago

If that's is acceptable, I will try to implement it. But for sure I can't allow everything to be customized, maybe in the end only the certificate is customizable

Sorry, to make the cipher suite customizable seems for me to be too much! That includes in my opinion to declare a lot of the API to be public (and so to be kept stable). What I was considering is, that, if your changes are not that complex, and rather good tested, that it may get included in Californium.

(key_exchange is too much for me).

You need to adapt the key_echange, because ECDSA_ECDHE is based on ECDSA. if you want to use RSA (server certificate), you need to adapt the key_exchange. If it's only intended for the client side, that may be easier (and doesn't require to use new cipher suites at all).

Nick-The-Uncharted commented 3 years ago

Thanks your your reply. 1-2 months is okay for my schedule. So If I just implement rsa related cipherSuite with controlable code change and good code quality, it can be merged after reviewing?

boaks commented 3 years ago

That seems for me to go in the wrong direction!

My company has very strict security policy, I am not allowed to download opensource code and built my own variant.

Though I'm not able to "grant" ahead, that your work will be merged, the only way for your company is exactly to allow you to modify Californium on your own responsibility!

If your company has some legal concerns, let me try to explain, that the eclipse license is exactly granting you that use. If some of the legal department in your company has doubts about that, they may contact the eclipse foundation to proof that. if some help is needed for that, let me know.

If this is for security/cryptography reasons, then I don't understand that either. Please also read the license carefully, especially "5. NO WARRANTY". If the consideration is, that we/I will spend a lot of time in a review or do security checks, let me say, that's not my plan. The only thing which is possible, is that if the code doesn't change too much other stuff, we can include it.

So If I just implement rsa related cipherSuite with controlable code change and good code quality, it can be merged after reviewing?

If it passes the review, yes. Please ensure, that your company agrees on your contribution and you can sign a ECA. Read CONTRIBUTING . In my opinion that comes with the same security and risks, if you modify it and use your custom build. So I don't see, the advantage for your company here.

Nick-The-Uncharted commented 3 years ago

@boaks I will try to submit a mr first. Using custom build means a lots of paperwork and explaination with dozens of people and I am not confidient that I can persuade everyone of them...

boaks commented 3 years ago

You will need a ECA . That will also require that your company agrees on your work! That is really important, your company must agree on your contribution!

Nick-The-Uncharted commented 3 years ago

@boaks Understood, already signed the ECA.

boaks commented 3 years ago

Any progress? Any first results?

Nick-The-Uncharted commented 3 years ago

@boaks Working on that, past two weeks has been too busy. I'm still trying to upgrade leshan to lastest version (but it seems only compactible with californiumn 2.6.3. I doubt that even with the latest leshan I can't use californiumn 3.x).

boaks commented 3 years ago

I'm still trying to upgrade leshan to lastest version

Ahh, I overseen that! My bad, sorry ...

Over that last months I prepared some PRs in order to migrate leshan to Californium 3.0. See Leshan PR 1023 - included in master and Leshan PR 1073 - in discussion.

With that, the current leshan master works with Californium 3.0.0-M4 (before the Configuration redesign).

That Configuration Redesign turned at to require more discussions, than I expected (and to be frank, I'm able and willing to spend.)

Just in the case your interested, see issue: Issue #1648 Issue #1708 Issue #1709

According your work: I don't know, what the outcome and final decision will be. You may work based on leshan master and californium 3.0.0-M4. Or on Leshan PR 1073 and californium master.

Just in the case you decide to go with the Leshan PR and Californium master, please tell me. I stopped my work on that PR also waiting for the final decision. If you want to apply your work on the top of that PR and Californium master, then I would update that PR to my latest local working state,

boaks commented 3 years ago

Let me add:

This discussion, mainly about static field for defaults, is currently blocking the Californium Release. We will see, if mid of October is still realistic or not.

I guess, you will require also a Leshan release. That's out of my possibilities and out of my scope. I don't know, when that release will come.

sbernard31 commented 3 years ago

This discussion, mainly about static field for defaults, is currently blocking the Californium Release. We will see, if mid of October is realistic or not.

Just to add some precisions :

More concerns are listed than the static vs dynamic question. (maybe some are already fixed, I don't check recently)

One of the possibility to not block the release was proposed by you (and I was agree with that):

But if we feel uncomfortable with the changes it forces right now, postpone that to 4.0 will also work.

(from https://github.com/eclipse/leshan/pull/1073#discussion_r691114336)

So I would say that past choice and several discussions are blocking the Californium release.

Nick-The-Uncharted commented 3 years ago

@boaks Thanks a lot, I will try leshan master. As to the configuration redesgin I actully comfortable with simple setter method....BTW, kafka has similiar mechanism and loading in static block doesn't seems to bother them (see org.apache.kafka.clients.producer.ProducerConfig and org.apache.kafka.common.config.SslConfigs).

boaks commented 3 years ago

@Nick-The-Uncharted

Just, if your interested in express your opinion and preferences, please read issue #1709.

I closed the voting, because I didn't expect more interest and there are/have been jobs to be done depending on that outcome. For me it will be OK, if you indicate your interests even now. I don't know, what the outcome will be and who will then have the time to work on it. I will be next weeks in vacation and after that I'm sure, we will find a way.

boaks commented 3 years ago

I will try leshan master.

Great, if you then prepare a PR for Californium, please use the "3.0.0-M3.x" branch. That will make it easier for you.

boaks commented 3 years ago

Thanks for your vote!

Would it be possible for you, to write a comment? I don't want to influence you too much, but for us it's important, if you vote, just because

sbernard31 commented 3 years ago

@Nick-The-Uncharted,

As to the configuration redesgin I actully comfortable with simple setter method....

I agree

BTW, kafka has similiar mechanism and loading in static block doesn't seems to bother them...

I'm not sure about that at least I didn't find it at (SslConfigs.java) Actually regarding the code, I even understand that they rather choose :

At first sight, I see any of the concerns I raised for californium in the kafka code but maybe I missed something.

So there is maybe a misunderstanding, the issue is not about using static field for each definition, this is about the registration in static block. (e.g. here)

boaks commented 3 years ago

here

That's the result of my first experience with that new Configuration API.

With that, a module is register once with the first usage in the "DEFAULT MODULES". It's hard to see for me, why someone has good reason to use this class, if it is not intended to be used for Configuration. Sure, there are some practice out in the wild, which want to ensure, the something is not in use and therefore use it. But that seems for me to be not that relevant.

I try to improve the javadoc of the Configuration in PR #1734. (And I'm still on it). Especially, if an "application set" should use different definitions in different properties file (which will be possible, with the APi introduced with PR #1701, that was my proposed ans so called "compromise" to continue), that may or may not confuse users. My intention was, as I wrote in that PR in leshan, to leave that experience to leshan users. Keeping the very simple (and therefore not dynamic) approach for me and the (other) Californium dummies.

Nick-The-Uncharted commented 3 years ago

I'm not sure about that at least I didn't find it at (SslConfigs.java)

org.apache.kafka.common.config.SaslConfigs#addClientSaslSupport is acutlly called by static blocks of ProducerConfig/ConsumerConfig. From my point of view it's actully very alike to what's has been implemented for californium. (Those configuration are registered in static blocks and hold in static field)

Nick-The-Uncharted commented 3 years ago

@boaks I saw org.eclipse.californium.scandium.dtls.SignatureAndHashAlgorithm#getSignatureAlgorithms is changed here. Leading to valueOf ED25519 always throw an Exception. ED25519 not supported!

boaks commented 3 years ago

Which jre/jce are you using? And can you please copy&paste the stacktrace (at least the lines within the Californium code).

Nick-The-Uncharted commented 3 years ago

@boaks I am still using Java8 (Sorry, but that's not under my control). So I integrated bouncycastle for supporting of Ed25519. There is no "with/WITH" in "ED25519", so org.eclipse.californium.scandium.dtls.SignatureAndHashAlgorithm#valueOf always return null. Following is the stackTrace:

java.lang.IllegalArgumentException: Ed25519 not supported!
    at org.eclipse.californium.scandium.dtls.SignatureAndHashAlgorithm.getSignatureAlgorithms(SignatureAndHashAlgorithm.java:354) ~[scandium-3.0.0-M5-SNAPSHOT.jar:?]
    at org.eclipse.californium.scandium.dtls.x509.CertificateConfigurationHelper.addConfigurationDefaultsFor(CertificateConfigurationHelper.java:153) ~[scandium-3.0.0-M5-SNAPSHOT.jar:?]
    at org.eclipse.californium.scandium.dtls.x509.SingleCertificateProvider.setupConfigurationHelper(SingleCertificateProvider.java:156) ~[scandium-3.0.0-M5-SNAPSHOT.jar:?]
    at org.eclipse.californium.scandium.config.DtlsConnectorConfig$Builder.build(DtlsConnectorConfig.java:3408) ~[scandium-3.0.0-M5-SNAPSHOT.jar:?]
    at org.eclipse.leshan.server.californium.LeshanServerBuilder.build(LeshanServerBuilder.java:530) ~[leshan-server-cf-2.0.0-SNAPSHOT.jar:?]
boaks commented 3 years ago

In Californium BC wasn't considered in the past.

The usage of ECC in TLS itself seems to consider, that, if curves are supported, that this is done for both ECDSA and ECDHE. At least, supported curves must be negotiated in the handshake, and there is no difference between using that curve for ECDHE or ECDSA according that negotiation (AFAIK). For that, if you use BC for ED25519, that requires X25519 support as well. Though that comes with java 11 (not using an other JCE), it's not available for java 8. In order to have one "jar", I decided at that time to use a solution based on reflection assuming the java 11 API. With that, you can't use ED/X25519 without java 11.

In the past months, especially users of Android convinced me, to start considering BC as well. But that's just started.

PR #1703 (only on master, not 3.0.0-M3.x!). See XECDHECryptography.

But, please note: For now, I can't provide too much support, if using BC doesn't work with Californium. Nor I'm able to support BC itself.

And verify important: In my experience (D)TLS is no "mosaic" using java/JCE . It doesn't work magically, just putting two function together. There are sometimes hidden inner relations between such parts, which prevent that working. As that stuff with the ECC curve.

boaks commented 3 years ago

Anyway, if there is something not working left with EdDSA certificates in the master, let me know.

Nick-The-Uncharted commented 3 years ago

@boaks I don't think this problem is related to BC, since in org.eclipse.californium.scandium.dtls.SignatureAndHashAlgorithm#getSignatureAlgorithms, certificate.getSigAlgName will always return "ED25519" even with java15(see here). So "ED25519" will be passed as an parameter to org.eclipse.californium.scandium.dtls.SignatureAndHashAlgorithm#valueOf which returns null and triggers throw new IllegalArgumentException(sigAlgName + " not supported!");

NOTE: I don't have java15 at my hand so I just read jdk source code. I do have a java13 machine but it doesn't recoginze ed25519 certificate and retrun "1.3.101.112" when calling java.security.cert.X509Certificate#getSigAlgName

boaks commented 3 years ago

Do you now want do use Ed25519? Or not?

Nick-The-Uncharted commented 3 years ago

@boaks I don't want to use it, just tested it because I happens to have one ed25519 cert. It works before this commit when null return of org.eclipse.californium.scandium.dtls.SignatureAndHashAlgorithm#valueOf won't leads to Exception and Ed25519 was added by getDefaultSignatureAlgorithms

boaks commented 3 years ago

Sorry, sometimes a bugfix also cause unclear situations to fail.

I added PR #1740 to demonstrate, that SignatureAndHashAlgorithm algorithm = SignatureAndHashAlgorithm.valueOf("ED25519"); succeeds.

You mainly use a combination java8 + BC, which is not considered by Californium. If that seems to work at a timepoint, sorry, it snot supported what means, changes are not tested against it.

boaks commented 3 years ago

In other word, if the implementation doesn't really negotiate, that ed25519 is supported on both sides, and just send the cert, and both sides support it without negotiation, then it seems to work ... but that's more by random, than by design.

Nick-The-Uncharted commented 3 years ago

@boaks Thank you, I failed to notice SignatureAndHashAlgorithm.valueOf("ED25519") no longer return null. So in master branch everything works fine. And my implementation of java8 + BC actully choose curve secp256r1 with signature algorithm ed25519 for key exchange and Cipher Suite TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384. (I thought maybe that works because of we are using "ECDHE" instead of "ECDH")

sbernard31 commented 3 years ago

@Nick-The-Uncharted

org.apache.kafka.common.config.SaslConfigs#addClientSaslSupport is actutlly called by static blocks of ProducerConfig/ConsumerConfig. From my point of view it's actully very alike to what's has been implemented for californium. (Those configuration are registered in static blocks and hold in static field)

I don't really agree because :

That sounds very different too me.

Anyway, recent events made that I decided to not take part anymore about this kind of question on Californium. I don't know if my concerns are justified or not but I stop to try to get consensus on this.

Nick-The-Uncharted commented 3 years ago

@sbernard31 I understand, it's normal that people don't agree with codeing style of each other. For me the only principle of good code is "Easy to read/maintain and hard to introduce bug when extend/modify."

For example I merged vo/po of my company's project and use "jackson/javax.persistence" annotation to determine how they interact with db and serialization. And I removed most interface of service layer since they does nothing but added the cost of maintainance(it's rare for bussiness code to have two different implementation.). Those are seriously disagree by my collages. But util now they works perfectly fine.

boaks commented 3 years ago

Should we move the design discussion to issue #1709 ? Maybe other will be interested and that may be then a better base to decide, in which way we go in the mid-future?

(Or a new issue?)

boaks commented 3 years ago

@Nick-The-Uncharted

Just as PoC, BC and ED25519, see PR #1741. Please retest with the setup demonstrated there.

With that setup, java 8 + BC, Ed25519 should work. At least in the interoperability tests, it was working.

(Currently not all unit-test are successful with that setup, but the add sig-alg is working.)

sbernard31 commented 3 years ago

Should we move the design discussion to issue #1709 ? Maybe other will be interested and that may be then a better base to decide, in which way we go in the mid-future?

You're right that this is not the right place. Sorry about that. I'm not sure #1709 is about the design ? I feel this is more #1648 or #1708 but choose whatever you want. Feel free to copy/past or refer there what you think could be useful if you want and or close the issue I opened.

boaks commented 3 years ago

I guess, #1708 will do it.

boaks commented 3 years ago

I finally succeeded to run the unit tests also with BC.

See PR #1742

Use the profile bc-tests to run the unit-tests in scandium and the interoperability tests with bouncy castle.

mvn clean intall -P bc-tests
Nick-The-Uncharted commented 3 years ago

@boaks I succeed to use a rsa certificate in my local setup. The changes are actully quite small. I will submit a mr in the follwoing weeks(adding some test cases and polishing my code). I have some doubts for org.eclipse.californium.scandium.dtls.cipher.DefaultCipherSuiteSelector#selectForCertificate.

Do we need to check whether EC cert support selected curves? For ECDHE the keys are generated on the fly so I think it's not related to cert key. According to https://datatracker.ietf.org/doc/html/rfc4492#section-5.3, even for ECDHE-ECDSA the Certificate only need to contain an ECDSA-capable public key.

boaks commented 3 years ago

Do we need to check whether EC cert support selected curves?

I'm not sure, what you mean:

EC is used for two things: ECDSA (that's the key in the certificate) ECDHE (that's the ephemeral keys)

The supported curves are negotiated for both. If ECDSA is not used, then for ECDHE.

rfc4492

NOTE: A server participating in an ECDHE-ECDSA key exchange may use different curves for (i) the ECDSA key in its certificate, and (ii) the ephemeral ECDH key in the ServerKeyExchange message. The server must consider the extensions in both cases.

Even if that refers specially to ECDHE_ECDSA, it specifies, that the extension must be considered in both cases. For "ECDHE_RSA" then only for the ephemeral key.

Anyway, I'm not sure, why introducing RSA should be related to the curves for ECDHE?

Sure, some checks, also in the dtls-configuration, which assume "ECDHE_ECDSA" key exchanges, must be adapted to the RSA variant.

boaks commented 3 years ago

Add spend some more time in an experimental bouncy castle support. The PR #1742 has been update and contains now the newest version. To use Bouncy Castle 1.69 for the unit tests, use the maven profile bc-tests. (See the readme in that PR.)

boaks commented 3 years ago

Check PR #1767 , #1768 , and #1769

boaks commented 3 years ago

@Nick-The-Uncharted

Doing some final pollish, I stumbled over the logging: BC is using JUL. Therefore PR #1811 adds a jul2slf4j2 bridge (only) on loading BC. If possible, please check, if that works for you. Unfortunately, I want to release 3.0 today. So feedback from you, if you could have a short test in time or next time (postponing the release a day) would be welcome. Otherwise a 3.0.1 (maybe with other fixes) will also be chance to fix something, if the "last minute change" doesn't work for you.