sslcontext-kickstart icon indicating copy to clipboard operation
sslcontext-kickstart copied to clipboard

You may like securitybuilder

Open wsargent opened this issue 3 years ago • 13 comments

I wrote a similar library securitybuilder -- it's more developer oriented (certificate chains) and deals with some different aspects of security like private keys and keystores.

wsargent avatar Aug 21 '22 21:08 wsargent

Hi Will, thank you for sharing it! I liked your library already some time ago when I discovered it at Awesome Java List and also have starred it back than 😄 Creating certificates programatically in Java can be very verbose and tough to understand and your library makes it really easy thanks to the fluent api!

Hakky54 avatar Aug 21 '22 22:08 Hakky54

Cool stuff :)

I'm just random person passing by here in this github issue but I wonder if I dare to "advertise" that I too created a library, though this one only addresses issuing test certificates https://github.com/tsaarni/java-certy. It is very small library, however I used BouncyCastle on the background which is not that small.

I guess @wsargent you are the person who wrote https://tersesystems.com/blog/2018/09/08/keymanagers-and-keystores/ few years back? I wanted to say Big Thanks for that article! It was the missing manual for my own work.

tsaarni avatar Aug 30 '22 14:08 tsaarni

Hi @tsaarni

I discovered your pull request previous week at Vertx regarding hot reloading ssl https://github.com/eclipse-vertx/vert.x/pull/4372 which seems pretty advanced! Really nice to see you now here and your library is really convenient to use for just creating certificates during a projects unit test. I have bookmarked it, thank you for sharing it here. It seems like we all have made some nice tools by providing either utility classes, builder classes or wrappers to simplify things and hide the java verbosity away to just make it simple for the end-user when they need to setup something related to ssl. I think it would be nice to combine the effort of the three projects into a single library, what do you guys think?

Hakky54 avatar Aug 30 '22 18:08 Hakky54

Hi @Hakky54! I saw your PR where you also try to push the hot-reload issue forward, thanks! Event though I did not comment anything there in your PR, I have moved to investigate the approach you pointed out. I'm wishing for Quarkus to allow application to set KeyManagers and TrustMangers to Vert.x (check this one), so that application can then take the responsibility of setting things up. BTW during working with my Vert.x hot-reload PR which got bit stuck, I realized it was you who previously added support to Vert.x for using own Key/TrustManager, which is greatly appreciated :-) I have learned a lot about Java TLS this year from both of you!

I was thinking about the different TLS libraries from the perspective of my own work: lets say I want to contribute hot-reload support to some application and I'm not a maintainer myself. If I send a PR that uses a new 3rd party package, would they be willing to accept a new dependency for this? They might be OK with the proposal, but often they might be concerned about adding runtime dependencies, especially if the library looks to be maintained by (mainly) a single developer. From that perspective, having a strong alternative with several maintainers would likely have much better chance of being accepted.

I myself ended up just putting the learnings together and creating yet another library which I'm willing to just copy over to any project I work with and make it theirs, regarding copyrights, licenses, formatting, testing... I have not actually managed to do this with any project yet, so my approach is not proven. I was just hoping the functionality can be done in minimal way, not getting people scared about the code, which might be wrong assumption :)

Sorry for repurposing this github issue as a chat forum :-)

tsaarni avatar Aug 30 '22 19:08 tsaarni

Yeah, I indeed added the support of configuring Vert.x with a keymanager and trustmanager with the wrap method, well discovered 😊 I discovered that Vert.x didn't had this option compared to other 40+ http clients, see here: https://github.com/Hakky54/mutual-tls-ssl#tested-clients and I thought it would be handy to add that option for different usecases and now it seems like even Quarkus can make use of it now. Really happy to see that! 😄

I was thinking about the different TLS libraries from the perspective of my own work: lets say I want to contribute hot-reload support to some application and I'm not a maintainer myself. If I send a PR that uses a new 3rd party package, would they be willing to accept a new dependency for this? They might be OK with the proposal, but often they might be concerned about adding runtime dependencies, especially if the library looks to be maintained by (mainly) a single developer. From that perspective, having a strong alternative with several maintainers would likely have much better chance of being accepted.

Your feeling is indeed correct. Users prefer to use something which is well maintained, has a big community so they can quickly ask at GitHub or Stackoverflow and has no license issue. In your case you can try submitting a PR having your own library as a transitive dependency. It is also the responsibility of the reviewer to check if they really need that library and if there is a easier way (not using external library and implementing own solution) or alternative library. In the past I tried to add it to couple of projects and I got pushed back with questions wether it was really needed and if I could remove it. Promoting your own library by creating a PR with it is hard to get approved, that is what I have experienced. It feels actually better if someone discoveres your library and adds it to his repo and starts using it without you knowing it!

I myself ended up just putting the learnings together and creating yet another library which I'm willing to just copy over to any project I work with and make it theirs, regarding copyrights, licenses, formatting, testing... I have not actually managed to do this with any project yet, so my approach is not proven. I was just hoping the functionality can be done in minimal way, not getting people scared about the code, which might be wrong assumption :)

It seems like your library solves the ssl reloading completely differently than my approach, really awesome to see how we both try to solve the same problem but have different solutions! I think your solution is intresting as it is more related to a custom KeyStore which has the capability of reloading, but how are you going to reload the ssl if someone gets the keys or certificates from an inputstream or as a pem string from a database, or maybe fetched from an external sources (downloaded with an http client)?

By the way I noticed that the source code of your library is copied over to the PR of vert.x https://github.com/eclipse-vertx/vert.x/pull/4372/files by yourself which has a different copyright author compared to your library while the source code is equal. It is conflicting with the Apache 2 license. It is allowed to copy source code but the copyright author needs to be exactly the same.

And I think the pull request regarding ssl reloading on vert.x can be simplified, but I don't know exactly the business requirements are. Basically only what you need is something like this: HotSwappableX509ExtendedKeyManager

I have not actually managed to do this with any project yet, so my approach is not proven. I was just hoping the functionality can be done in minimal way, not getting people scared about the code, which might be wrong assumption :)

It really helps to make a reference implementation/demo repo, unit test, integration test to demonstrate your solution is working. In that way others are getting more trust in your solution and it will be much easier to adapt your solution. I did an attempt here: https://github.com/Hakky54/java-tutorials#security- But I don't know if developers know this page, so I don't know if it really helps 🤣 It contains various demo app including ssl reloading for spring, vertx, grpc, netty etc which is helpful I think for developers who want to quickly try it out and also for copying it to their own code base.

Hakky54 avatar Aug 30 '22 20:08 Hakky54

Hi @Hakky54,

It seems like your library solves the ssl reloading completely differently than my approach, really awesome to see how we both try to solve the same problem but have different solutions! I think your solution is intresting as it is more related to a custom KeyStore which has the capability of reloading,

I came to the conclusion that there are some benefits in that approach after reading OpenJDK's NewSunx509 key manager's source code. They wrote "explicitly designed to accommodate KeyStores that change over the lifetime of the process" (link) which really got my attention. I realized that they intended people to implement their own custom KeyStores that can change content over time, while still using just the standard KeyManager. Digging deeper, I discovered that KeyStore has an SPI underneath for the customization purpose.

I wrote my findings about this approach here for myself and potentially others as well. I included reasoning why I think it could be better approach than doing it at the KeyManager level.

but how are you going to reload the ssl if someone gets the keys or certificates from an inputstream or as a pem string from a database, or maybe fetched from an external sources (downloaded with an http client)?

I did not need that yet myself, but I would implement method like this, but taking inputstream or PEM strings as parameter. In that example I read PEM content from Path, but it could have been inputstream or a string.

By the way I noticed that the source code of your library is copied over to the PR of vert.x https://github.com/eclipse-vertx/vert.x/pull/4372/files by yourself which has a different copyright author compared to your library while the source code is equal. It is conflicting with the Apache 2 license. It is allowed to copy source code but the copyright author needs to be exactly the same.

My thinking was that since I'm the original author of that code, I can hand over the copyright to the receiving project if they have the convention that contributions need to be done under certain copyright. I could also re-license my own code under some different license if the receiving project does not happen to be under Apache license.

If you think there could be a problem in that thinking, I'd appreciate to be corrected :)

And I think the pull request regarding ssl reloading on vert.x can be simplified, but I don't know exactly the business requirements are. Basically only what you need is something like this: HotSwappableX509ExtendedKeyManager

Logically there is not that much difference here, since practically equivalent is done in my custom KeyStore SPI implementation here.

The complications I had when writing the Vert.X PR were related to the existing logic in the SSL helper code inside Vert.X. It splits the content of user-provided keystore that can hold several certificates, into many in-memory KeyStores and KeyManagers that hold just single certificate. Vert.x looks at incoming SNI value and picks one of those KeyManagers (with single certificate). But in this step where we tear down original keystore with multiple keys, we also lost the association with the underlying credentials file. Reloading becomes challenging. I wrote in the PR

"Previously selection of server certificate according to TLS SNI servername was implemented by custom-built logic by splitting each certificate in individual KeyStore(s) and KeyManager(s) and picking one of those according to the SNI information from Netty, when handling new TLS connection. The NewSunX509 version of KeyManager (from JDK 8) has support for selecting from multiple certificates in keystore according to SNI and other criterias, such as key type. "

In the initial version of my PR I had custom KeyManager, similiar like you pointed out. But after long struggles I ended up in unresolvable problems, not getting that to work with the part of Vert.X that really wants to split single keystore into many for supporting SNI. It seemed that whatever I tried, I broke that code logic and test cases failed. I ended up studying how they implemented SNI in Vert.X, how it was implemented in Netty, and how it is done in Java's NewSunX509.

After all of that effort, the current version of that PR using NewSunX509 and its SNI was actually simplified in my opinion, and it even improves the SNI support in Vert.x! :-) But I can understand how it looks too complicated. The reason was I needed to refactor the SNI code. It was pretty hard to communicate this all, there is a lot of SSL helper code dealing with SNI in Vert.x and I tried to do minimal refactoring. It also seemed to me that the maintainer does not have much time now to guide me with the PR :)

It really helps to make a reference implementation/demo repo, unit test, integration test to demonstrate your solution is working. In that way others are getting more trust in your solution and it will be much easier to adapt your solution. I did an attempt here: https://github.com/Hakky54/java-tutorials#security- But I don't know if developers know this page, so I don't know if it really helps

I was googling quite extensively and reading all the different examples and libraries over the internet and reading how they work inside. It really helped to understand this part of Java TLS.

In case of my own small reloading-keystore implementation, I was thinking that probably nobody will use it as such, because of the reasons we already discussed about 3rd party libraries. It might be useful for studying so I added lot of comments and wrote the implementation description explaining some findings I thought were not covered completely elsewhere. I added tests for my own sake. And I published it on maven central to learn how packages are published :)

tsaarni avatar Aug 30 '22 22:08 tsaarni

Returning to:

but how are you going to reload the ssl if someone gets the keys or certificates from an inputstream or as a pem string from a database, or maybe fetched from an external sources (downloaded with an http client)?

I now see what you meant: how to update the credentials later at runtime when content was originally given as inputstream or string. Sorry for my confusion :). One would need to keep reference to the KeyStore instance and have it expose interface to swap the delegate KeyStore. Yeah, I guess it would be ackward, though I guess doable similarly as extending KeyManager.

In the cases I have worked, though mainly in other languages than Java, the problem has been simply that credentials are loaded from file path and the application restart was required when certificate was rotated. When the file path information is "lost" by the above layers of the stack by passing the credential content through the layers instead of path, the more difficult the task tends to become, since the number of layers needing to provide support and participate in the hot-reload use case increases.

tsaarni avatar Aug 31 '22 04:08 tsaarni

I came to the conclusion that there are some benefits in that approach after reading OpenJDK's NewSunx509 key manager's source code. They wrote "explicitly designed to accommodate KeyStores that change over the lifetime of the process" (link) which really got my attention. I realized that they intended people to implement their own custom KeyStores that can change content over time, while still using just the standard KeyManager. Digging deeper, I discovered that KeyStore has an SPI underneath for the customization purpose.

I wrote my findings about this approach here for myself and potentially others as well. I included reasoning why I think it could be better approach than doing it at the KeyManager level.

I was not aware of this NewSunx509 type of KeyManager, thank you for sharing. This is really nice! I will do some research on it :)

My thinking was that since I'm the original author of that code, I can hand over the copyright to the receiving project if they have the convention that contributions need to be done under certain copyright. I could also re-license my own code under some different license if the receiving project does not happen to be under Apache license.

If you think there could be a problem in that thinking, I'd appreciate to be corrected :)

I am unfortunatelly not an expert on this topic, but maybe handy to further investigate.

After all of that effort, the current version of that PR using NewSunX509 and its SNI was actually simplified in my opinion, and it even improves the SNI support in Vert.x! :-) But I can understand how it looks too complicated. The reason was I needed to refactor the SNI code. It was pretty hard to communicate this all, there is a lot of SSL helper code dealing with SNI in Vert.x and I tried to do minimal refactoring.

Yeah, I was suprised by the amount of changes which was needed to make it working. But I now understand that it was a bit tricky because of the SNI.

It also seemed to me that the maintainer does not have much time now to guide me with the PR :)

The Vert.x maintainer is indeed quite busy as he is the main maintainer. He has also other repositories which keeps him busy, so pretty understandable :) The downside is that getting feedback and getting the PR approved could take more time.

In case of my own small reloading-keystore implementation, I was thinking that probably nobody will use it as such, because of the reasons we already discussed about 3rd party libraries. It might be useful for studying so I added lot of comments and wrote the implementation description explaining some findings I thought were not covered completely elsewhere. I added tests for my own sake. And I published it on maven central to learn how packages are published :)

I think having a library like this is usefull, as I also started like that. I needed some functionality which I used within different projects. Copying to all of the projects and maintaining it is a-lot work. Changing something within the library at one place is more maintainable.

I now see what you meant: how to update the credentials later at runtime when content was originally given as inputstream or string. Sorry for my confusion :). One would need to keep reference to the KeyStore instance and have it expose interface to swap the delegate KeyStore. Yeah, I guess it would be ackward, though I guess doable similarly as extending KeyManager.

In the cases I have worked, though mainly in other languages than Java, the problem has been simply that credentials are loaded from file path and the application restart was required when certificate was rotated. When the file path information is "lost" by the above layers of the stack by passing the credential content through the layers instead of path, the more difficult the task tends to become, since the number of layers needing to provide support and participate in the hot-reload use case increases.

Yes, indeed. So your use case was more related to files on the file system which could be pem or keystore file which would be updated over time. In my case it was slighty different, so I was just curious how you would resolve that by having other type of sources for ssl material.

I have starred/bookmarked your project! Thank you again for sharing, it is nice to see this kind of investigation and sharing of knowledge :)

Hakky54 avatar Aug 31 '22 09:08 Hakky54

I'm behind on my reading (and a bunch of other stuff) so I will catch up on this, but in the mean time I just remembered CloudFoundry did some work with reloadable key/trust managers as well:

https://github.com/cloudfoundry/java-buildpack-security-provider

wsargent avatar Sep 10 '22 22:09 wsargent

the solution of cloudfoundry is also a nice one, I also discovered that Elasticsearch has their own custom solution as well, see here: https://github.com/elastic/elasticsearch/blob/be7c7415627377a1b795400fb8dfcc6cbdf0e322/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLService.java

Hakky54 avatar Sep 11 '22 11:09 Hakky54

I have wondered that doesn't the solutions which are based on swapping delegate X509ExtendedKeyManager fail occasionally, because the swap can happen between calls to getPrivateKey() and getCertificateChain(), causing them to return mix of old/new credentials? I found out that there was code dedicated to avoiding this in NewSunX509 implementation of key manager and none of these custom key managers that I've seen have that. I wrote about it also here:

One neglected aspect when implementing custom KeyManager(s) is the lack of synchronization between two methods called during the TLS handshake: getCertificateChain(String alias) and getPrivateKey(String alias). KeyManager API was not designed for KeyStores that change their content at runtime: what if certificate and private key are updated during the handshake? The resulting mix of old and new credentials would be invalid. It could result in infrequent authentication error that might be hard to troubleshoot. NewSunX509 caches the PrivateKeyEntry instances (a combination of certificate chain and private key) internally to make it more likely it has loaded a consistent set of credentials. It uses its own prefixed alias naming scheme to refer to the cached entries. Calls to get the certificate chain or private key with given alias sticks to the cached key entries. The calls do not proceed all the way to the KeyStoreSpi instance, risking that it would hit credentials that have been updated between calls. The next round-trip to SPI happens when next server or client alias selection is requested. The updated credentials will be picked up at that point.

tsaarni avatar Sep 11 '22 14:09 tsaarni

I get your point, but some projects use other KeyManagers/TrustManagers implementation for a their own specific reasons for example from IBM, Google Conscrypt, Bouncy Castle and maybe other companies or libraries have their own implementation. I don't know the reason, but it could be that it is more secure, or is faster or is less strict or maybe has a better algorithm or some other reason. And that makes it hard for them to just switch over to NewSunX509 to enable reloading the ssl material. A KeyManager/TrustManager which just delegates the calls and has the ability of switching the internal KeyManager/TrustManager will work with all different implementations of KeyManager/TrustManager, so the end-user does not need to use a different KeyManager/TrustManager Algorithm

Hakky54 avatar Sep 11 '22 15:09 Hakky54

Having own KeyManager can of course be justified by many reasons! I just think that many reloading ones may share this problem without realizing that swap can happen between those method calls. It would be possible to use similar solution as the NewSunX509 does also in own custom key manager. It is not too complicated. That is: guarantee that swap happens only when new server/client alias selection is requested.

tsaarni avatar Sep 11 '22 16:09 tsaarni

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Sep 25 '22 20:09 stale[bot]