Skip to content
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

Better configuration #222

Open
jendakol opened this issue Nov 15, 2019 · 1 comment
Open

Better configuration #222

jendakol opened this issue Nov 15, 2019 · 1 comment

Comments

@jendakol
Copy link

Hello, I'd like to propose some major changes to this library.
Let me start with the motivation. We need to be able to create SSLContext from Lightbend Config and we have some working code from past but now we wanted to go with something "standard", something used by the others and be compatible with them.

That's how I found this library and was about to use it but then I realised there are some in my opinion weird things in it.
In general, I like this library for being very complex and providing huge configurability, so that's why I even care.

I see following problems:

  1. Configuration from Config is bound to using kebab-case however it'd be great if it supports whatever casing the user wants - camelCase, snake_case etc. Currently, the ssl-config configuration just doesn't fit into rest of the application because of this (unless one uses the same casing). BTW I made a seperate issue some time ago related to this.
  2. In general, it's good if one doesn't need to specify all values in the Config but the library provides some defaults. ssl-config does it but IMHO in a weird way - it doesn't explicitly merge some defaults when configuring, but defaults are somewhat implicitly (when loading Config) IF one holds the convention to put his configuration to same path (ssl-config in root). Strange thing is that SSLConfigFactory.parse method takes any Config instance which kind of collides with the previous thing - in that case one must set all values manually because defaults are not applied; so why to even take the Config instance as an input? What I'd expect it to do is actually made manually in tests which kind of emphasizes this existing problem. Everyone needs to do this merging by his own.
  3. This is very related to previous point. By kind of a forcing users to place their config to the same global path in Config, it's possible to create only a single SSLContext in whole application, which is very common but can cause some troubles in specific situations.

What I propose is:

  1. To support different casing, default may remain the current kebab-case, so backward compatibility.
  2. To support built-in merging of defaults, so noone would be forced to place his config to some predefined place. For older applications, it would work as now, it would just be merged explicitly instead of implicitly (even though inside the library).
  3. To polish lib's API a little bit, provide more clear entry point with support for different casings. This would not be backward compatible.

I also have in my mind another option, but backward incompatible: to split core of the library and it's configuration into two separate modules where the configuration from Config would by done by some clever library, most probably Pureconfig (potentially by more of them, via multiple modules). I think that even with this variant could be possible to be backward compatible in terms of configuration itself, just entrypoint would be different.

I'm not sure what are all current usages of this library (Akka, Play??) and how much they count with current behavior (e.g. a single global configuration) so I don't want to break anything; however I believe my changes would make the library just better and much more friendly for other potential adopters.

And, as I final note, I'd like to say that I'm willing to implement all changes we will agree about.

Thank you for considering.

@wsargent
Copy link
Contributor

As the original author, I have a lot to apologize for. It was basically my first try at understanding the JSSE / JCA API and as such, it has all the warts that the API does and then adds a few more in the form of loose options, debug options, and so on.

Play WS, Akka, and some other Lightbend libraries depend on ssl-config. The basic core has remained the same since it was first written.

The primary thing I'd do to ssl-config is to split it into two packages so the internal logic is clearer. There's a parser that generates case classes (with HOCON as an input), and various generators that build up trust managers, key managers, and SSL contexts from those case classes. Those should have been in different packages or even modules from the beginning.

Cleverness in the configuration is up to the parser, but I don't think it's necessary. Adding in pureconfig just adds another dependency, whereas what would really help would be a clearer API and documentation. Then you could parse from YAML or XML if you felt like it and still provide the same data structure for the generators.

I also think the generators are messy, and I think it would have been better to start from a builder API -- for example the https://github.com/tersesystems/securitybuilder codebase -- and then provide the case classes as a way to short-circuit a builder chain.

Finally, much of the checking done by Ciphers and Algorithms can be thrown out now since the JSSE codebase is no longer insecure by default in JDK 1.8.

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

No branches or pull requests

2 participants